Re: [Qemu-devel] [PATCH v2 2/5] acpi_piix4: Fix PCI hotplug race

[Michael S. Tsirkin]

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 <address@hidden>

---
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)