[PATCHv3] piix: fix up/down races

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 <alex.williamson@redhat.com>
--


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.  Clearing down from PCNT doesn't really buy us
anything.  Thanks,

Alex

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 <alex.williamson@redhat.com>

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.