Re: [Qemu-devel] [PATCH 4/4] hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jan 21, 2014 at 03:40:06PM +0100, Igor Mammedov wrote:
> reduces acpi PCI hotplug code duplication by 150LOC,
> and makes pcihp less dependend on piix specific code.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Actually pcihp was made separate intentionally so we
can cleanup the host/guest interface without
worrying about compatibility.

I posted that cleanup patch - I'm not against unifying both chunks of
code but could you put your patch on top?

> ---
>  hw/acpi/pcihp.c         |   10 +--
>  hw/acpi/piix4.c         |  196 +++++------------------------------------------
>  include/hw/acpi/pcihp.h |    8 ++-
>  3 files changed, 31 insertions(+), 183 deletions(-)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 76dce8d..25cc0fe 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -46,8 +46,6 @@
>  # define ACPI_PCIHP_DPRINTF(format, ...)     do { } while (0)
>  #endif
>  
> -#define PCI_HOTPLUG_ADDR 0xae00
> -#define PCI_HOTPLUG_SIZE 0x0014
>  #define PCI_UP_BASE 0x0000
>  #define PCI_DOWN_BASE 0x0004
>  #define PCI_EJ_BASE 0x0008
> @@ -279,14 +277,14 @@ static const MemoryRegionOps acpi_pcihp_io_ops = {
>      },
>  };
>  
> -void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus,
> -                     MemoryRegion *address_space_io)
> +void acpi_pcihp_init(AcpiPciHpState *s, PCIBus *root_bus, uint16_t io_base,
> +                     uint16_t io_size, MemoryRegion *address_space_io)
>  {
>      s->root= root_bus;
>      memory_region_init_io(&s->io, NULL, &acpi_pcihp_io_ops, s,
>                            "acpi-pci-hotplug",
> -                          PCI_HOTPLUG_SIZE);
> -    memory_region_add_subregion(address_space_io, PCI_HOTPLUG_ADDR, &s->io);
> +                          io_size);
> +    memory_region_add_subregion(address_space_io, io_base, &s->io);
>  }
>  
>  const VMStateDescription vmstate_acpi_pcihp_pci_status = {
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 5d55a3c..f818d76 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -44,13 +44,6 @@
>  #define GPE_BASE 0xafe0
>  #define GPE_LEN 4
>  
> -#define PCI_HOTPLUG_ADDR 0xae00
> -#define PCI_HOTPLUG_SIZE 0x000f
> -#define PCI_UP_BASE 0xae00
> -#define PCI_DOWN_BASE 0xae04
> -#define PCI_EJ_BASE 0xae08
> -#define PCI_RMV_BASE 0xae0c
> -
>  #define PIIX4_PCI_HOTPLUG_STATUS 2
>  
>  struct pci_status {
> @@ -80,8 +73,8 @@ typedef struct PIIX4PMState {
>      Notifier machine_ready;
>      Notifier powerdown_notifier;
>  
> -    /* for legacy pci hotplug (compatible with qemu 1.6 and older) */
> -    MemoryRegion io_pci;
> +    /* for legacy pci hotplug (compatible with qemu 1.6 and older)
> +     * used only for migration and updated in pre_save() */
>      struct pci_status pci0_status;
>      uint32_t pci0_hotplug_enable;
>      uint32_t pci0_slot_device_present;
> @@ -175,16 +168,24 @@ static void vmstate_pci_status_pre_save(void *opaque)
>      struct pci_status *pci0_status = opaque;
>      PIIX4PMState *s = container_of(pci0_status, PIIX4PMState, pci0_status);
>  
> +    pci0_status->down = s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].down;
>      /* We no longer track up, so build a safe value for migrating
>       * to a version that still does... of course these might get lost
>       * by an old buggy implementation, but we try. */
> -    pci0_status->up = s->pci0_slot_device_present & s->pci0_hotplug_enable;
> +    pci0_status->up =
> +        s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].device_present &
> +        s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].hotplug_enable;
>  }
>  
>  static int vmstate_acpi_post_load(void *opaque, int version_id)
>  {
>      PIIX4PMState *s = opaque;
>  
> +    if (!s->use_acpi_pci_hotplug) {
> +        s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].down =
> +            s->pci0_status.down;
> +    }
> +
>      pm_io_space_update(s);
>      return 0;
>  }
> @@ -304,60 +305,6 @@ static const VMStateDescription vmstate_acpi = {
>      }
>  };
>  
> -static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
> -{
> -    BusChild *kid, *next;
> -    BusState *bus = qdev_get_parent_bus(DEVICE(s));
> -    int slot = ffs(slots) - 1;
> -    bool slot_free = true;
> -
> -    /* Mark request as complete */
> -    s->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 {
> -                object_unparent(OBJECT(qdev));
> -            }
> -        }
> -    }
> -    if (slot_free) {
> -        s->pci0_slot_device_present &= ~(1U << slot);
> -    }
> -}
> -
> -static void piix4_update_hotplug(PIIX4PMState *s)
> -{
> -    BusState *bus = qdev_get_parent_bus(DEVICE(s));
> -    BusChild *kid, *next;
> -
> -    /* Execute any pending removes during reset */
> -    while (s->pci0_status.down) {
> -        acpi_piix_eject_slot(s, s->pci0_status.down);
> -    }
> -
> -    s->pci0_hotplug_enable = ~0;
> -    s->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) {
> -            s->pci0_hotplug_enable &= ~(1U << slot);
> -        }
> -
> -        s->pci0_slot_device_present |= (1U << slot);
> -    }
> -}
> -
>  static void piix4_reset(void *opaque)
>  {
>      PIIX4PMState *s = opaque;
> @@ -377,11 +324,7 @@ static void piix4_reset(void *opaque)
>          pci_conf[0x5B] = 0x02;
>      }
>      pm_io_space_update(s);
> -    if (s->use_acpi_pci_hotplug) {
> -        acpi_pcihp_reset(&s->acpi_pci_hotplug);
> -    } else {
> -        piix4_update_hotplug(s);
> -    }
> +    acpi_pcihp_reset(&s->acpi_pci_hotplug);
>  }
>  
>  static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
> @@ -409,6 +352,10 @@ static int piix4_acpi_pci_hotplug(DeviceState *qdev, PCIDevice *dev,
>  static void piix4_update_bus_hotplug(PCIBus *bus, void *opaque)
>  {
>      PIIX4PMState *s = opaque;
> +
> +    if (!s->use_acpi_pci_hotplug && (bus != s->acpi_pci_hotplug.root)) {
> +        return;
> +    }
>      pci_bus_hotplug(bus, piix4_acpi_pci_hotplug, DEVICE(s));
>  }
>  
> @@ -426,9 +373,7 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
>      pci_conf[0x67] = (memory_region_present(io_as, 0x3f8) ? 0x08 : 0) |
>          (memory_region_present(io_as, 0x2f8) ? 0x90 : 0);
>  
> -    if (s->use_acpi_pci_hotplug) {
> -        pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
> -    }
> +    pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s);
>  }
>  
>  static void piix4_pm_add_propeties(PIIX4PMState *s)
> @@ -621,60 +566,6 @@ static const MemoryRegionOps piix4_gpe_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> -static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size)
> -{
> -    PIIX4PMState *s = opaque;
> -    uint32_t val = 0;
> -
> -    switch (addr) {
> -    case PCI_UP_BASE - PCI_HOTPLUG_ADDR:
> -        /* Manufacture an "up" value to cause a device check on any hotplug
> -         * slot with a device.  Extra device checks are harmless. */
> -        val = s->pci0_slot_device_present & s->pci0_hotplug_enable;
> -        PIIX4_DPRINTF("pci_up_read %" PRIu32 "\n", val);
> -        break;
> -    case PCI_DOWN_BASE - PCI_HOTPLUG_ADDR:
> -        val = s->pci0_status.down;
> -        PIIX4_DPRINTF("pci_down_read %" PRIu32 "\n", val);
> -        break;
> -    case PCI_EJ_BASE - PCI_HOTPLUG_ADDR:
> -        /* No feature defined yet */
> -        PIIX4_DPRINTF("pci_features_read %" PRIu32 "\n", val);
> -        break;
> -    case PCI_RMV_BASE - PCI_HOTPLUG_ADDR:
> -        val = s->pci0_hotplug_enable;
> -        break;
> -    default:
> -        break;
> -    }
> -
> -    return val;
> -}
> -
> -static void pci_write(void *opaque, hwaddr addr, uint64_t data,
> -                      unsigned int size)
> -{
> -    switch (addr) {
> -    case PCI_EJ_BASE - PCI_HOTPLUG_ADDR:
> -        acpi_piix_eject_slot(opaque, (uint32_t)data);
> -        PIIX4_DPRINTF("pciej write %" HWADDR_PRIx " <== %" PRIu64 "\n",
> -                      addr, data);
> -        break;
> -    default:
> -        break;
> -    }
> -}
> -
> -static const MemoryRegionOps piix4_pci_ops = {
> -    .read = pci_read,
> -    .write = pci_write,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> -    .valid = {
> -        .min_access_size = 4,
> -        .max_access_size = 4,
> -    },
> -};
> -
>  static void piix4_cpu_added_req(Notifier *n, void *opaque)
>  {
>      PIIX4PMState *s = container_of(n, PIIX4PMState, cpu_added_notifier);
> @@ -684,9 +575,6 @@ static void piix4_cpu_added_req(Notifier *n, void *opaque)
>      acpi_update_sci(&s->ar, s->irq);
>  }
>  
> -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> -                                PCIHotplugState state);
> -
>  static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>                                             PCIBus *bus, PIIX4PMState *s)
>  {
> @@ -694,55 +582,13 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>                            "acpi-gpe0", GPE_LEN);
>      memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
>  
> -    if (s->use_acpi_pci_hotplug) {
> -        acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent);
> -    } else {
> -        memory_region_init_io(&s->io_pci, OBJECT(s), &piix4_pci_ops, s,
> -                              "acpi-pci-hotplug", PCI_HOTPLUG_SIZE);
> -        memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR,
> -                                    &s->io_pci);
> -        pci_bus_hotplug(bus, piix4_device_hotplug, DEVICE(s));
> -    }
> +    acpi_pcihp_init(&s->acpi_pci_hotplug, bus, PIIX_PCI_HOTPLUG_ADDR,
> +                    s->use_acpi_pci_hotplug ? PIIX_PCI_HOTPLUG_SIZE :
> +                                              PIIX_PCI_HOTPLUG_LEGACY_SIZE,
> +                    parent);
>  
>      AcpiCpuHotplug_init(parent, OBJECT(s), &s->gpe_cpu,
>                          PIIX4_CPU_HOTPLUG_IO_BASE);
>      s->cpu_added_notifier.notify = piix4_cpu_added_req;
>      qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
>  }
> -
> -static void enable_device(PIIX4PMState *s, int slot)
> -{
> -    s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
> -    s->pci0_slot_device_present |= (1U << slot);
> -}
> -
> -static void disable_device(PIIX4PMState *s, int slot)
> -{
> -    s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
> -    s->pci0_status.down |= (1U << slot);
> -}
> -
> -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> -				PCIHotplugState state)
> -{
> -    int slot = PCI_SLOT(dev->devfn);
> -    PIIX4PMState *s = PIIX4_PM(qdev);
> -
> -    /* 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) {
> -        s->pci0_slot_device_present |= (1U << slot);
> -        return 0;
> -    }
> -
> -    if (state == PCI_HOTPLUG_ENABLED) {
> -        enable_device(s, slot);
> -    } else {
> -        disable_device(s, slot);
> -    }
> -
> -    acpi_update_sci(&s->ar, s->irq);
> -
> -    return 0;
> -}
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index 6230e60..6bd41bb 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -31,6 +31,10 @@
>  #include <qemu/typedefs.h>
>  #include "hw/pci/pci.h" /* for PCIHotplugState */
>  
> +#define PIIX_PCI_HOTPLUG_ADDR 0xae00
> +#define PIIX_PCI_HOTPLUG_SIZE 0x0014
> +#define PIIX_PCI_HOTPLUG_LEGACY_SIZE 0x000f
> +
>  typedef struct AcpiPciHpPciStatus {
>      uint32_t up; /* deprecated, maintained for migration compatibility */
>      uint32_t down;
> @@ -48,8 +52,8 @@ typedef struct AcpiPciHpState {
>      MemoryRegion io;
>  } AcpiPciHpState;
>  
> -void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root,
> -                     MemoryRegion *address_space_io);
> +void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root, uint16_t io_base,
> +                     uint16_t io_size, MemoryRegion *address_space_io);
>  
>  /* Invoke on device hotplug */
>  int acpi_pcihp_device_hotplug(AcpiPciHpState *, PCIDevice *,
> -- 
> 1.7.1