Re: [Qemu-devel] [PATCHv3] piix: fix up/down races

[Michael S. Tsirkin]

On Tue, Apr 03, 2012 at 07:59:49AM -0600, Alex Williamson wrote:
> On Tue, 2012-04-03 at 13:05 +0300, Michael S. Tsirkin wrote:
> > On Tue, Apr 03, 2012 at 02:40:41AM -0400, Igor Mammedov wrote:
> > > > > +     */
> > > > > +    s->pci0_status.down &= ~(1U << slot);
> > > > 
> > > > Should we clear "up" here too?
> > > Is it possible to create pci dev for not yet freed pci slot?
> > 
> > It's not possible ATM.
> > 
> > > If not then we should not care, otherwise we should fix that.
> > > > > @@ -567,8 +595,6 @@ static int piix4_device_hotplug(DeviceState
> > > > > *qdev, PCIDevice *dev,
> > > > >          return 0;
> > > > >      }
> > > > >  
> > > > > -    s->pci0_status.up = 0;
> > > > > -    s->pci0_status.down = 0;
> > > > >      if (state == PCI_HOTPLUG_ENABLED) {
> > > > >          enable_device(s, slot);
> > > > >      } else {
> > > > 
> > > > So if we have an old bios and do an add, followed by a remove, guest
> > > > ACPI finds both "up" and "down" set for the slot and we rely on the
> > > > ordering of the AML checking up before down to keep the duct tape and
> > > > bailing wire from exploding?
> > 
> > In fact UP triggers a rescan - there is no injection event in ACPI.
> > So it seems you can check the events in any order.
> 
> So, we're not concerned with extra device checks and we can clear "down"
> on eject.  What's then the practical advantage of your patch over:
> 
> Signed-off-by: Alex Williamson <address@hidden>

For old bios there is no advantage. But new BIOS can avoid a storm of
notifications by clearing the register properly.

> --
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 0c77730..050061c 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -79,6 +79,7 @@ typedef struct PIIX4PMState {
>  } PIIX4PMState;
>  
>  static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s);
> +static void pciej_write(void *opaque, uint32_t addr, uint32_t val);
>  
>  #define ACPI_ENABLE 0xf1
>  #define ACPI_DISABLE 0xf0
> @@ -291,6 +292,10 @@ static void piix4_update_hotplug(PIIX4PMState *s)
>              s->pci0_hotplug_enable &= ~(1 << slot);
>          }
>      }
> +
> +    while (s->pci0_status.down) {
> +        pciej_write(s, PCI_BASE + 4, s->pci0_status.down);
> +    }
>  }
>  
>  static void piix4_reset(void *opaque)
> @@ -470,38 +475,15 @@ static void gpe_writeb(void *opaque, uint32_t addr, 
> uint32_t val)
>  
>  static uint32_t pcihotplug_read(void *opaque, uint32_t addr)
>  {
> -    uint32_t val = 0;
> +    uint32_t val;
>      struct pci_status *g = opaque;
> -    switch (addr) {
> -        case PCI_BASE:
> -            val = g->up;
> -            break;
> -        case PCI_BASE + 4:
> -            val = g->down;
> -            break;
> -        default:
> -            break;
> -    }
> +
> +    val = g->down;
>  
>      PIIX4_DPRINTF("pcihotplug read %x == %x\n", addr, val);
>      return val;
>  }
>  
> -static void pcihotplug_write(void *opaque, uint32_t addr, uint32_t val)
> -{
> -    struct pci_status *g = opaque;
> -    switch (addr) {
> -        case PCI_BASE:
> -            g->up = val;
> -            break;
> -        case PCI_BASE + 4:
> -            g->down = val;
> -            break;
> -   }
> -
> -    PIIX4_DPRINTF("pcihotplug write %x <== %d\n", addr, val);
> -}
> -
>  static uint32_t pciej_read(void *opaque, uint32_t addr)
>  {
>      PIIX4_DPRINTF("pciej read %x\n", addr);
> @@ -514,6 +496,8 @@ static void pciej_write(void *opaque, uint32_t addr, 
> uint32_t val)
>      DeviceState *qdev, *next;
>      int slot = ffs(val) - 1;
>  
> +    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);
> @@ -560,8 +544,8 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, 
> PIIX4PMState *s)
>      register_ioport_write(PROC_BASE, 32, 1, gpe_writeb, s);
>      register_ioport_read(PROC_BASE, 32, 1,  gpe_readb, s);
>  
> -    register_ioport_write(PCI_BASE, 8, 4, pcihotplug_write, pci0_status);
> -    register_ioport_read(PCI_BASE, 8, 4,  pcihotplug_read, pci0_status);
> +    register_ioport_read(PCI_BASE, 4, 4,  pcirmv_read, s);
> +    register_ioport_read(PCI_BASE + 4, 4, 4,  pcihotplug_read, pci0_status);
>  
>      register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, bus);
>      register_ioport_read(PCI_EJ_BASE, 4, 4,  pciej_read, bus);
> @@ -640,8 +624,6 @@ static int piix4_device_hotplug(DeviceState *qdev, 
> PCIDevice *dev,
>          return 0;
>      }
>  
> -    s->pci0_status.up = 0;
> -    s->pci0_status.down = 0;
>      if (state == PCI_HOTPLUG_ENABLED) {
>          enable_device(s, slot);
>      } else {
> 
> 
> ie. Up statically returns all hotplug slots, check them all.  Down is
> cleared on eject and reset.  Then there's no bios compatibility issue,
> no bios update even.

What you suggest will work but it's a hack.

I do keep the hack around for compatibility but document as such,
so we can get rid of it in qemu 2.0 or whenever we are
ready to drop migration compatibility.

> Clearing down from PCNT doesn't really buy us
> anything.  Thanks,
> 
> Alex

If you don't you get multiple eject notifications.
Yes they are harmless but ugly, new bios should be able to
get a single notification in a clean way.

-- 
MST