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 <imamm...@redhat.com>
> ---
> 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
