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

[Michael S. Tsirkin]

On Mon, Jan 27, 2014 at 04:39:55PM +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 <address@hidden>
> ---
> v2:
>  - replace obsolete 'device_present' with 'up' field
>  - add/set ACPI_PCIHP_PROP_BSEL to 0 when running in compatibility
>    mode with old machine types.
> ---
>  hw/acpi/pcihp.c         |   10 +--
>  hw/acpi/piix4.c         |  204 
> +++++++----------------------------------------
>  include/hw/acpi/pcihp.h |    8 ++-
>  3 files changed, 40 insertions(+), 182 deletions(-)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index e48b758..d17040c 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
> @@ -274,14 +272,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)

OK this is a trick: io_size can be smaller than PIIX_PCI_HOTPLUG_SIZE.
If it is smaller, then only first X registers are enabled.
I think it's not a great API.
Just add a flag legacy_piix to acpi_pcihp_init, set size
accordingly.


>  {
>      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 a20453d..d04ac2f 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() */

Pls make it looks like this:

 +    /* for legacy pci hotplug (compatible with qemu 1.6 and older)
 +     * used only for migration and updated in pre_save()
 +     */

or drop it completely.

>      struct pci_status pci0_status;
>      uint32_t pci0_hotplug_enable;
>      uint32_t pci0_slot_device_present;
> @@ -174,16 +167,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. */

This comment is really wrong now, isn't it?

> -    pci0_status->up = s->pci0_slot_device_present & s->pci0_hotplug_enable;
> +    pci0_status->up = s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].up;
>  }
>  
>  static int vmstate_acpi_post_load(void *opaque, int version_id)
>  {
>      PIIX4PMState *s = opaque;
>  
> +    if (!s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> +        s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].down =
> +            s->pci0_status.down;
> +        s->acpi_pci_hotplug.acpi_pcihp_pci_status[0].up =
> +            s->pci0_status.up;
> +    }
> +
>      pm_io_space_update(s);
>      return 0;
>  }

OK if all we do is this, how about just giving
s->acpi_pci_hotplug.acpi_pcihp_pci_status[0]
to vmstate?


> @@ -303,60 +304,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;
> @@ -376,11 +323,7 @@ static void piix4_reset(void *opaque)
>          pci_conf[0x5B] = 0x02;
>      }
>      pm_io_space_update(s);
> -    if (s->acpi_pci_hotplug.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)
> @@ -408,6 +351,11 @@ 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->acpi_pci_hotplug.use_acpi_pci_hotplug &&
> +        (bus != s->acpi_pci_hotplug.root)) {
> +        return;
> +    }

That's an unnecessary complication.
Just call pci_bus_hotplug(d->bus, piix4_acpi_pci_hotplug, s);
in compat mode like we always did.

>      pci_bus_hotplug(bus, piix4_acpi_pci_hotplug, DEVICE(s));
>  }
>  
> @@ -425,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->acpi_pci_hotplug.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)

same as above.

> @@ -620,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);
> @@ -683,65 +575,29 @@ 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)
>  {
> +    uint16_t pcihp_io_size = PIIX_PCI_HOTPLUG_SIZE;
> +
>      memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
>                            "acpi-gpe0", GPE_LEN);
>      memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
>  
> -    if (s->acpi_pci_hotplug.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));
> +    if (!s->acpi_pci_hotplug.use_acpi_pci_hotplug) {
> +        unsigned *bus_bsel = g_malloc(sizeof *bus_bsel);
> +
> +        pcihp_io_size = PIIX_PCI_HOTPLUG_LEGACY_SIZE;
> +
> +        *bus_bsel = 0;
> +        object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> +                                       bus_bsel, NULL);
>      }

Okay, but please add a comment here.
Also how about
#define ACPI_PCIHP_BSEL_DEFAULT 0
and use this in pcihp.c on reset?

> +    acpi_pcihp_init(&s->acpi_pci_hotplug, bus, PIIX_PCI_HOTPLUG_ADDR,
> +                    pcihp_io_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 cff5270..afe1e8c 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

Pls change prefix to ACPI_PCIHP_

Also - required in header?

> +#define PIIX_PCI_HOTPLUG_LEGACY_SIZE 0x000f
> +
>  typedef struct AcpiPciHpPciStatus {
>      uint32_t up;
>      uint32_t down;
> @@ -49,8 +53,8 @@ typedef struct AcpiPciHpState {
>      bool use_acpi_pci_hotplug;
>  } 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