[Qemu-devel] [PATCH] Fix race resulting in loosing event bit in GPE.1.sts

[Qemu-devel] [PATCH] Fix race resulting in loosing event bit in GPE.1.sts

[Igor Mammedov]

After receiving hotplug gpe event, guest masks event in
GPE.1.en register, executes associated AML handler and then resets
event bit in GPE.1.sts. If another pci device was hot-plugged
after AML handler has been executed and before event bit is
reset in GPE.1.sts, then guest will loose GPE event and it will
not see all hotplugged devices.

Could be reproduced with:
 ./QMP/qmp device_add --driver=e1000 && sleep 0.X && ./QMP/qmp device_add --driver=e1000

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi.c       |   23 ++++++++++++++++++++++-
 hw/acpi.h       |    1 +
 hw/acpi_piix4.c |    7 +++++++
 3 files changed, 30 insertions(+), 1 deletions(-)

diff --git a/hw/acpi.c b/hw/acpi.c
index 5d521e5..be6efab 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -412,6 +412,7 @@ void acpi_gpe_init(ACPIREGS *ar, uint8_t len)
     ar->gpe.len = len;
     ar->gpe.sts = g_malloc0(len / 2);
     ar->gpe.en = g_malloc0(len / 2);
+    ar->gpe.pending_sts = g_malloc0(len / 2);
 }
 
 void acpi_gpe_blk(ACPIREGS *ar, uint32_t blk)
@@ -423,6 +424,7 @@ void acpi_gpe_reset(ACPIREGS *ar)
 {
     memset(ar->gpe.sts, 0, ar->gpe.len / 2);
     memset(ar->gpe.en, 0, ar->gpe.len / 2);
+    memset(ar->gpe.pending_sts, 0, ar->gpe.len / 2);
 }
 
 static uint8_t *acpi_gpe_ioport_get_ptr(ACPIREGS *ar, uint32_t addr)
@@ -440,15 +442,34 @@ static uint8_t *acpi_gpe_ioport_get_ptr(ACPIREGS *ar, uint32_t addr)
     return cur;
 }
 
+static uint8_t *acpi_gpe_get_pend_sts_ptr(ACPIREGS *ar, uint32_t addr)
+{
+    uint8_t *cur = NULL;
+
+    if (addr < ar->gpe.len / 2) {
+        cur = ar->gpe.pending_sts + addr;
+    } else {
+        abort();
+    }
+
+    return cur;
+
+}
+
 void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr, uint32_t val)
 {
-    uint8_t *cur;
+    uint8_t *cur, *psts;
 
     addr -= ar->gpe.blk;
     cur = acpi_gpe_ioport_get_ptr(ar, addr);
     if (addr < ar->gpe.len / 2) {
         /* GPE_STS */
         *cur = (*cur) & ~val;
+        psts = acpi_gpe_get_pend_sts_ptr(ar, addr);
+        if (*cur != *psts) {
+            *cur |= *psts;
+            *psts = 0;
+        }
     } else if (addr < ar->gpe.len) {
         /* GPE_EN */
         *cur = val;
diff --git a/hw/acpi.h b/hw/acpi.h
index fe8cdb4..6a6953d 100644
--- a/hw/acpi.h
+++ b/hw/acpi.h
@@ -104,6 +104,7 @@ struct ACPIGPE {
 
     uint8_t *sts;
     uint8_t *en;
+    uint8_t *pending_sts;
 };
 
 struct ACPIREGS {
diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 797ed24..ce50d85 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -66,6 +66,9 @@ typedef struct PIIX4PMState {
     int kvm_enabled;
     Notifier machine_ready;
 
+    /* for hotplug */
+    uint16_t pending_gpe_events;
+
     /* for pci hotplug */
     struct pci_status pci0_status;
     uint32_t pci0_hotplug_enable;
@@ -575,6 +578,10 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
         disable_device(s, slot);
     }
 
+    if (~s->ar.gpe.en[0] & PIIX4_PCI_HOTPLUG_STATUS) {
+        s->ar.gpe.pending_sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
+    }
+
     pm_update_sci(s);
 
     return 0;
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH] Fix race resulting in loosing event bit in GPE.1.sts

[Igor Mammedov]

Tried another approach, that involves only seabios change as specified in
acpi50.spec: 5.6.4 General-Purpose Event Handling
by switching from level to edge handler.

diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
index 4e04c48..51906ad 100644
--- a/src/acpi-dsdt.dsl
+++ b/src/acpi-dsdt.dsl
@@ -723,7 +723,7 @@ DefinitionBlock (
         Method(_L00) {
             Return(0x01)
         }
-        Method(_L01) {
+        Method(_E01) {
             // PCI hotplug event
             Return(\_SB.PCI0.PCNF())
         }

It fixes problem (at least I wasn't able to hit window between GPE00.sts read and
GPE00.sts reset, too small interval).
It seems to work as expected with rhel6, 3.3+ kernel, winxp, ws2008r2.
Will post patch to seabios list and hope we won't need this one in qemu.