Re: [Qemu-devel] [PATCH v2 21/21] q35: add acpi-based pci hotplug.

[Michael S. Tsirkin]

On Mon, Oct 08, 2012 at 11:30:39PM -0400, Jason Baron wrote:
> From: Jason Baron <address@hidden>
> 
> Add piix style acpi hotplug to q35.
> 
> Signed-off-by: Jason Baron <address@hidden>

Something I don't understand here: this only handles hotplug
of devices behind the root, no?
Don't we need support for hotplug/hot remove of devices behind
bridges?

> ---
>  hw/acpi_ich9.c |  172 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  hw/acpi_ich9.h |   10 +++
>  2 files changed, 181 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/acpi_ich9.c b/hw/acpi_ich9.c
> index 1412ad3..412c9c1 100644
> --- a/hw/acpi_ich9.c
> +++ b/hw/acpi_ich9.c
> @@ -41,6 +41,13 @@ do { printf("%s "fmt, __func__, ## __VA_ARGS__); } while 
> (0)
>  #define ICH9_DEBUG(fmt, ...)    do { } while (0)
>  #endif
>  
> +#define PCI_UP_BASE 0xae00
> +#define PCI_DOWN_BASE 0xae04
> +#define PCI_EJ_BASE 0xae08
> +#define PCI_RMV_BASE 0xae0c
> +#define ICH9_PCI_HOTPLUG_STATUS 2
> +
> +
>  static void pm_ioport_write_fallback(void *opaque, uint32_t addr, int len,
>                                       uint32_t val);
>  static uint32_t pm_ioport_read_fallback(void *opaque, uint32_t addr, int 
> len);
> @@ -55,7 +62,10 @@ static void pm_update_sci(ICH9LPCPMRegs *pm)
>                    (ACPI_BITMASK_RT_CLOCK_ENABLE |
>                     ACPI_BITMASK_POWER_BUTTON_ENABLE |
>                     ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
> -                   ACPI_BITMASK_TIMER_ENABLE)) != 0);
> +                   ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
> +         (((pm->acpi_regs.gpe.sts[0] & pm->acpi_regs.gpe.en[0])
> +          & ICH9_PCI_HOTPLUG_STATUS) != 0);
> +
>      qemu_set_irq(pm->irq, sci_level);
>  
>      /* schedule a timer interruption if needed */
> @@ -77,6 +87,7 @@ static void pm_ioport_writeb(void *opaque, uint32_t addr, 
> uint32_t val)
>      switch (addr & ICH9_PMIO_MASK) {
>      case ICH9_PMIO_GPE0_STS ... (ICH9_PMIO_GPE0_STS + ICH9_PMIO_GPE0_LEN - 
> 1):
>          acpi_gpe_ioport_writeb(&pm->acpi_regs, addr, val);
> +        pm_update_sci(pm);
>          break;
>      default:
>          break;
> @@ -283,6 +294,65 @@ const VMStateDescription vmstate_ich9_pm = {
>      }
>  };
>  
> +static void acpi_ich9_eject_slot(ICH9LPCPMRegs *opaque, unsigned slots)
> +{
> +    BusChild *kid, *next;
> +    ICH9LPCPMRegs *pm = opaque;
> +    ICH9LPCState *lpc = container_of(pm, ICH9LPCState, pm);
> +    PCIDevice *s = PCI_DEVICE(lpc);
> +    BusState *bus = qdev_get_parent_bus(&s->qdev);
> +    int slot = ffs(slots) - 1;
> +    bool slot_free = true;
> +
> +    /* Mark request as complete */
> +    pm->pci0_status.down &= ~(1U << slot);
> +
> +    QTAILQ_FOREACH_SAFE(kid, &bus->children, sibling, next) {
> +        DeviceState *qdev = kid->child;
> +        PCIDevice *dev = PCI_DEVICE(qdev);
> +        PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> +        if (PCI_SLOT(dev->devfn) == slot) {
> +            if (pc->no_hotplug) {
> +                slot_free = false;
> +            } else {
> +                qdev_free(qdev);
> +            }
> +        }
> +    }
> +    if (slot_free) {
> +        pm->pci0_slot_device_present &= ~(1U << slot);
> +    }
> +}
> +
> +static void acpi_ich9_update_hotplug(ICH9LPCPMRegs *pm)
> +{
> +    ICH9LPCState *lpc = container_of(pm, ICH9LPCState, pm);
> +    PCIDevice *dev = PCI_DEVICE(lpc);
> +    BusState *bus = qdev_get_parent_bus(&dev->qdev);
> +    BusChild *kid, *next;
> +
> +    /* Execute any pending removes during reset */
> +    while (pm->pci0_status.down) {
> +        acpi_ich9_eject_slot(pm, pm->pci0_status.down);
> +    }
> +
> +    pm->pci0_hotplug_enable = ~0;
> +    pm->pci0_slot_device_present = 0;
> +
> +    QTAILQ_FOREACH_SAFE(kid, &bus->children, sibling, next) {
> +        DeviceState *qdev = kid->child;
> +        PCIDevice *pdev = PCI_DEVICE(qdev);
> +        PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pdev);
> +        int slot = PCI_SLOT(pdev->devfn);
> +
> +        if (pc->no_hotplug) {
> +            pm->pci0_hotplug_enable &= ~(1U << slot);
> +        }
> +
> +        pm->pci0_slot_device_present |= (1U << slot);
> +    }
> +}
> +
>  static void pm_reset(void *opaque)
>  {
>      ICH9LPCPMRegs *pm = opaque;
> @@ -300,6 +370,7 @@ static void pm_reset(void *opaque)
>      }
>  
>      pm_update_sci(pm);
> +    acpi_ich9_update_hotplug(pm);
>  }
>  
>  static void pm_powerdown_req(Notifier *n, void *opaque)
> @@ -309,6 +380,104 @@ static void pm_powerdown_req(Notifier *n, void *opaque)
>      acpi_pm1_evt_power_down(&pm->acpi_regs);
>  }
>  
> +static uint32_t pci_up_read(void *opaque, uint32_t addr)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +    uint32_t val;
> +
> +    /* Manufacture an "up" value to cause a device check on any hotplug
> +     * slot with a device.  Extra device checks are harmless. */
> +    val = pm->pci0_slot_device_present & pm->pci0_hotplug_enable;
> +
> +    ICH9_DEBUG("pci_up_read %x\n", val);
> +    return val;
> +}
> +
> +static uint32_t pci_down_read(void *opaque, uint32_t addr)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +    uint32_t val = pm->pci0_status.down;
> +
> +    ICH9_DEBUG("pci_down_read %x\n", val);
> +    return val;
> +}
> +
> +static uint32_t pci_features_read(void *opaque, uint32_t addr)
> +{
> +    /* No feature defined yet */
> +    ICH9_DEBUG("pci_features_read %x\n", 0);
> +    return 0;
> +}
> +
> +static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
> +{
> +    acpi_ich9_eject_slot(opaque, val);
> +
> +    ICH9_DEBUG("pciej write %x <== %d\n", addr, val);
> +}
> +
> +static uint32_t pcirmv_read(void *opaque, uint32_t addr)
> +{
> +    ICH9LPCPMRegs *pm = opaque;
> +
> +    return pm->pci0_hotplug_enable;
> +}
> +
> +static void enable_device(ICH9LPCPMRegs *pm, int slot)
> +{
> +    pm->acpi_regs.gpe.sts[0] |= ICH9_PCI_HOTPLUG_STATUS;
> +    pm->pci0_slot_device_present |= (1U << slot);
> +}
> +
> +static void disable_device(ICH9LPCPMRegs *pm, int slot)
> +{
> +    pm->acpi_regs.gpe.sts[0] |= ICH9_PCI_HOTPLUG_STATUS;
> +    pm->pci0_status.down |= (1U << slot);
> +}
> +
> +static int ich9_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> +                PCIHotplugState state)
> +{
> +    int slot = PCI_SLOT(dev->devfn);
> +    ICH9LPCState *lpc = DO_UPCAST(ICH9LPCState, d,
> +                                PCI_DEVICE(qdev));
> +    ICH9LPCPMRegs *pm = &lpc->pm;
> +
> +    /* Don't send event when device is enabled during qemu machine creation:
> +     * it is present on boot, no hotplug event is necessary. We do send an
> +     * event when the device is disabled later. */
> +    if (state == PCI_COLDPLUG_ENABLED) {
> +        pm->pci0_slot_device_present |= (1U << slot);
> +        return 0;
> +    }
> +
> +    if (state == PCI_HOTPLUG_ENABLED) {
> +        enable_device(pm, slot);
> +    } else {
> +        disable_device(pm, slot);
> +    }
> +
> +    pm_update_sci(pm);
> +
> +    return 0;
> +}
> +
> +static void ich9_acpi_system_hot_add_init(ICH9LPCPMRegs *s)
> +{
> +    ICH9LPCState *lpc = container_of(s, ICH9LPCState, pm);
> +    PCIDevice *pdev = PCI_DEVICE(lpc);
> +
> +    register_ioport_read(PCI_UP_BASE, 4, 4, pci_up_read, s);
> +    register_ioport_read(PCI_DOWN_BASE, 4, 4, pci_down_read, s);
> +
> +    register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, s);
> +    register_ioport_read(PCI_EJ_BASE, 4, 4,  pci_features_read, s);
> +
> +    register_ioport_read(PCI_RMV_BASE, 4, 4,  pcirmv_read, s);
> +
> +    pci_bus_hotplug(pdev->bus, ich9_device_hotplug, &pdev->qdev);
> +}
> +
>  void ich9_pm_init(ICH9LPCPMRegs *pm, qemu_irq sci_irq, qemu_irq cmos_s3)
>  {
>      acpi_pm_tmr_init(&pm->acpi_regs, ich9_pm_update_sci_fn);
> @@ -319,4 +488,5 @@ void ich9_pm_init(ICH9LPCPMRegs *pm, qemu_irq sci_irq, 
> qemu_irq cmos_s3)
>      qemu_register_reset(pm_reset, pm);
>      pm->powerdown_notifier.notify = pm_powerdown_req;
>      qemu_register_powerdown_notifier(&pm->powerdown_notifier);
> +    ich9_acpi_system_hot_add_init(pm);
>  }
> diff --git a/hw/acpi_ich9.h b/hw/acpi_ich9.h
> index 180c406..b4e2aff 100644
> --- a/hw/acpi_ich9.h
> +++ b/hw/acpi_ich9.h
> @@ -23,6 +23,11 @@
>  
>  #include "acpi.h"
>  
> +struct pci_status {
> +    uint32_t up; /* deprecated, maintained for migration compatibility */
> +    uint32_t down;
> +};
> +
>  typedef struct ICH9LPCPMRegs {
>      /*
>       * In ich9 spec says that pm1_cnt register is 32bit width and
> @@ -37,6 +42,11 @@ typedef struct ICH9LPCPMRegs {
>  
>      uint32_t pm_io_base;
>      Notifier powerdown_notifier;
> +
> +    /* for pci hotplug */
> +    struct pci_status pci0_status;
> +    uint32_t pci0_hotplug_enable;
> +    uint32_t pci0_slot_device_present;
>  } ICH9LPCPMRegs;
>  
>  void ich9_pm_init(ICH9LPCPMRegs *pm,
> -- 
> 1.7.1