[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: |
Michael S. Tsirkin |
|
Subject: |
Re: [Qemu-devel] [PATCH v2 4/4] hw:piix4:acpi: reuse pcihp code for legacy PCI hotplug |
|
Date: |
Thu, 30 Jan 2014 15:08:23 +0200 |
On Thu, Jan 30, 2014 at 01:20:12PM +0100, Igor Mammedov wrote:
> 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?
that's what you do for legacy. why? because new registers
happen to be at the end. It's a trick and that's okay
since it saves lines of code, but let's keep this
trick local in acpi/pcihp.c. Don't leak it out to piix.
> 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.
Well the legacy mode is piix specific.
There's no way around it.
Let's just make it explicit, keep code in one place.
> 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.
Problem is it's not well isolated.
The idea of keeping all acpi based pci hotplug in one file
is appealing.
But we should not leak out the register layout otherwise
what's the point.
And size is part of layout.
> >
> >
> > > {
> > > 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