[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 4/4] hw:piix4:acpi: reuse pcihp code for lega
|
From: |
Igor Mammedov |
|
Subject: |
Re: [Qemu-devel] [PATCH v2 4/4] hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug |
|
Date: |
Thu, 30 Jan 2014 13:20:12 +0100 |
On Thu, 30 Jan 2014 13:25:39 +0200
"Michael S. Tsirkin" <address@hidden> wrote:
> 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.
Size set at init time in piix4_acpi_system_hot_add_init() according to
machine type. Why it will be smaller than PIIX_PCI_HOTPLUG_SIZE?
It will be programming error to pass an incorrect size.
I think it's not good to hardcode piix4 specific constants in code
that will be shared with q35 unless there is compelling reason to do so.
Passing io_base & io_size to acpi_pcihp_init() is the same as
passing region address & size to memory_region_* API.
So I think isolating device specific code from common one is better
API in this case than what you suggest.
>
>
> > {
> > 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:
sure
>
> + /* 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?
yep it should have been dropped in
"pcihp: reduce number of device check events" patch
I'll make a cleanup patch that would take care of it and device+present field.
>
> > - 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?
not sure if it would work with old vmstate but, I'll try it.
>
> > @@ -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?
not really, I'll put them into piix4.c and keep
PIIX_PCI_ prefix to reflect that it's piix4 only defines.
>
> > +#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