| Message ID | 20120412102840.GA14321@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 2012-04-12 at 13:28 +0300, Michael S. Tsirkin wrote: > On Thu, Apr 05, 2012 at 11:07:15AM -0600, Alex Williamson wrote: > > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c > > index 5e8b261..0e7af51 100644 > > --- a/hw/acpi_piix4.c > > +++ b/hw/acpi_piix4.c > > @@ -48,7 +48,7 @@ > > #define PIIX4_PCI_HOTPLUG_STATUS 2 > > > > struct pci_status { > > - uint32_t up; > > + uint32_t up; /* deprecated, maintained for migration compatibility */ > > uint32_t down; > > }; > > > > @@ -70,6 +70,7 @@ typedef struct PIIX4PMState { > > /* for pci hotplug */ > > struct pci_status pci0_status; > > uint32_t pci0_hotplug_enable; > > + uint32_t pci0_slot_device_present; > > } PIIX4PMState; > > > > static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s); > > @@ -205,6 +206,17 @@ static void pm_write_config(PCIDevice *d, > > pm_io_space_update((PIIX4PMState *)d); > > } > > > > +static void vmstate_pci_status_pre_save(void *opaque) > > +{ > > + struct pci_status *pci0_status = opaque; > > + PIIX4PMState *s = container_of(pci0_status, PIIX4PMState, pci0_status); > > + > > + /* 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; > > +} > > + > > static int vmstate_acpi_post_load(void *opaque, int version_id) > > { > > PIIX4PMState *s = opaque; > > @@ -241,6 +253,7 @@ static const VMStateDescription vmstate_pci_status = { > > .version_id = 1, > > .minimum_version_id = 1, > > .minimum_version_id_old = 1, > > + .pre_save = vmstate_pci_status_pre_save, > > .fields = (VMStateField []) { > > VMSTATE_UINT32(up, struct pci_status), > > VMSTATE_UINT32(down, struct pci_status), > > @@ -269,13 +282,38 @@ static const VMStateDescription vmstate_acpi = { > > } > > }; > > > > +static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots) > > +{ > > + DeviceState *qdev, *next; > > + BusState *bus = qdev_get_parent_bus(&s->dev.qdev); > > + int slot = ffs(slots) - 1; > > + > > + /* Mark request as complete */ > > + s->pci0_status.down &= ~(1U << slot); > > + > > + QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) { > > + PCIDevice *dev = PCI_DEVICE(qdev); > > + PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev); > > + if (PCI_SLOT(dev->devfn) == slot && !pc->no_hotplug) { > > + s->pci0_slot_device_present &= ~(1U << slot); > > + qdev_free(qdev); > > + } > > + } > > +} > > + > > One small thing that bothers me is how slot bit is cleared > apparently for each device. Hotplug and non hotplug > devices should not mix normally, and we only set > the bit when we add a device so it should all work out, > but still, I think a bit better and more robust > if the bit will get cleared unless a device is left in the slot. > > So I'm thinking of applying this on top: any objections? > Warning: untested. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Sure, I'm not sure how we'd get to that point, but I agree it's more robust to explicitly account for more than one device per slot. Acked-by: Alex Williamson <alex.williamson@redhat.com> > --- > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c > index 11c1f85..585da4e 100644 > --- a/hw/acpi_piix4.c > +++ b/hw/acpi_piix4.c > @@ -287,6 +287,7 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots) > DeviceState *qdev, *next; > BusState *bus = qdev_get_parent_bus(&s->dev.qdev); > int slot = ffs(slots) - 1; > + bool slot_free = true; > > /* Mark request as complete */ > s->pci0_status.down &= ~(1U << slot); > @@ -294,11 +295,17 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots) > QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) { > PCIDevice *dev = PCI_DEVICE(qdev); > PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev); > - if (PCI_SLOT(dev->devfn) == slot && !pc->no_hotplug) { > - s->pci0_slot_device_present &= ~(1U << slot); > - qdev_free(qdev); > + if (PCI_SLOT(dev->devfn) == slot) { > + if (pc->no_hotplug) { > + slot_free = false; > + } else { > + qdev_free(qdev); > + } > } > } > + if (slot_free) { > + s->pci0_slot_device_present &= ~(1U << slot); > + } > } > > static void piix4_update_hotplug(PIIX4PMState *s)
diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c index 11c1f85..585da4e 100644 --- a/hw/acpi_piix4.c +++ b/hw/acpi_piix4.c @@ -287,6 +287,7 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots) DeviceState *qdev, *next; BusState *bus = qdev_get_parent_bus(&s->dev.qdev); int slot = ffs(slots) - 1; + bool slot_free = true; /* Mark request as complete */ s->pci0_status.down &= ~(1U << slot); @@ -294,11 +295,17 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots) QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) { PCIDevice *dev = PCI_DEVICE(qdev); PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev); - if (PCI_SLOT(dev->devfn) == slot && !pc->no_hotplug) { - s->pci0_slot_device_present &= ~(1U << slot); - qdev_free(qdev); + if (PCI_SLOT(dev->devfn) == slot) { + if (pc->no_hotplug) { + slot_free = false; + } else { + qdev_free(qdev); + } } } + if (slot_free) { + s->pci0_slot_device_present &= ~(1U << slot); + } } static void piix4_update_hotplug(PIIX4PMState *s)