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

[Michael S. Tsirkin]

On Tue, Apr 10, 2012 at 09:14:00AM -0600, Alex Williamson wrote:
> On Sun, 2012-04-08 at 01:57 -0300, Marcelo Tosatti wrote:
> > 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.
> 
> I suppose we could.  Initially I was creating the present device bitmap
> dynamically, but walking the device tree on every access seemed
> excessive versus updating it runtime.  Since there's no need to migrate
> this and we don't want to have it clobbered by an incoming migration, I
> didn't think to re-use the "up" field.  We could do it, but it would
> mean introducing a post_load function that effectively just calls
> piix4_update_hotplug to recreate it, which seems like an unnecessary
> effort to avoid using an extra 4 bytes of memory.

It's probably harmless if we do let it be clobbered by migration
though: worst case we lose an event and that might have
happened before migration :)

I don't mind either way though.

> > 
> > >  } 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?
> 
> I couldn't convince myself that it was completely redundant with
> piix4_update_hotplug called from reset, so I took the paranoia approach.
> If we always do a reset after all the cold plug devices are added, we
> can drop it.  Is that the case?  Thanks,
> 
> Alex
>