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

[Alex Williamson <alex.williamson@redhat.com>]

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

Alex