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 <m...@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.william...@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)
