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

[Marcelo Tosatti]

On Thu, Apr 05, 2012 at 11:07:15AM -0600, Alex Williamson wrote:
> As Michael Tsirkin demonstrated, current PCI hotplug is vulnerable
> to a few races.  The first is a race with other hotplug operations
> because we clear the up & down registers at each event.  If a new
> event comes before the last is processed, up/down is cleared and
> the event is lost.
> 
> To fix this for the down register, we create a life cycle for
> the event request that starts with the hot unplug request in
> piix4_device_hotplug() and ends when the device is ejected.
> This allows us to mask and clear individual bits, preserving them
> against races.  For the up register, we have no clear end point
> for when the event is finished.  We could modify the BIOS to
> acknowledge the bit and clear it, but this creates BIOS compatibiliy
> issues without offering a complete solution.  Instead we note that
> gratuitous ACPI device checks are not harmful, which allows us to
> issue a device check for every slot.  We know which slots are present
> and we know which slots are hotpluggable, so we can easily reduce
> this to a more manageable set for the guest.
> 
> The other race Michael noted was that an unplug request followed
> by reset may also lose the eject notification, which may also
> result in the eject request being lost which a subsequent add
> or remove.  Once we're in reset, the device is unused and we can
> flush the queue of device removals ourselves.  Previously if a
> device_del was issued to a guest without ACPI PCI hotplug support,
> it was necessary to shutdown the guest to recover the device.
> With this, a guest reboot is sufficient.
> 
> Signed-off-by: Alex Williamson <address@hidden>
> ---
> 
>  hw/acpi_piix4.c |   74 
> +++++++++++++++++++++++++++++++++++++++----------------
>  1 files changed, 53 insertions(+), 21 deletions(-)
> 
> 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;

Why can't you use the "up" field for this? Fail to see the point
of the new variable.


>  } 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);
> +        }
> +    }
> +}
> +
>  static void piix4_update_hotplug(PIIX4PMState *s)
>  {
>      PCIDevice *dev = &s->dev;
>      BusState *bus = qdev_get_parent_bus(&dev->qdev);
>      DeviceState *qdev, *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(qdev, &bus->children, sibling, next) {
>          PCIDevice *pdev = PCI_DEVICE(qdev);
> @@ -283,8 +321,10 @@ static void piix4_update_hotplug(PIIX4PMState *s)
>          int slot = PCI_SLOT(pdev->devfn);
>  
>          if (pc->no_hotplug) {
> -            s->pci0_hotplug_enable &= ~(1 << slot);
> +            s->pci0_hotplug_enable &= ~(1U << slot);
>          }
> +
> +        s->pci0_slot_device_present |= (1U << slot);
>      }
>  }
>  
> @@ -452,7 +492,11 @@ static void gpe_writeb(void *opaque, uint32_t addr, 
> uint32_t val)
>  static uint32_t pci_up_read(void *opaque, uint32_t addr)
>  {
>      PIIX4PMState *s = opaque;
> -    uint32_t val = s->pci0_status.up;
> +    uint32_t val;
> +
> +    /* 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 %x\n", val);
>      return val;
> @@ -475,18 +519,7 @@ static uint32_t pciej_read(void *opaque, uint32_t addr)
>  
>  static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
>  {
> -    BusState *bus = opaque;
> -    DeviceState *qdev, *next;
> -    int slot = ffs(val) - 1;
> -
> -    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) {
> -            qdev_free(qdev);
> -        }
> -    }
> -
> +    acpi_piix_eject_slot(opaque, val);
>  
>      PIIX4_DPRINTF("pciej write %x <== %d\n", addr, val);
>  }
> @@ -516,8 +549,8 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, 
> PIIX4PMState *s)
>      register_ioport_read(PCI_UP_BASE, 4, 4, pci_up_read, s);
>      register_ioport_read(PCI_DOWN_BASE, 4, 4, pci_down_read, s);
>  
> -    register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, bus);
> -    register_ioport_read(PCI_EJ_BASE, 4, 4,  pciej_read, bus);
> +    register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, s);
> +    register_ioport_read(PCI_EJ_BASE, 4, 4,  pciej_read, s);
>  
>      register_ioport_write(PCI_RMV_BASE, 4, 4, pcirmv_write, s);
>      register_ioport_read(PCI_RMV_BASE, 4, 4,  pcirmv_read, s);
> @@ -528,13 +561,13 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, 
> PIIX4PMState *s)
>  static void enable_device(PIIX4PMState *s, int slot)
>  {
>      s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
> -    s->pci0_status.up |= (1 << slot);
> +    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 |= (1 << slot);
> +    s->pci0_status.down |= (1U << slot);
>  }
>  
>  static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> @@ -548,11 +581,10 @@ static int piix4_device_hotplug(DeviceState *qdev, 
> PCIDevice *dev,
>       * 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);

Why is this necessary?