qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alex Williamson
Subject: Re: [Qemu-devel] [PATCH v2 2/5] acpi_piix4: Fix PCI hotplug race
Date: Thu, 12 Apr 2012 08:31:18 -0600

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

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






reply via email to

[Prev in Thread] Current Thread [Next in Thread]