Patchwork [24/26] acpi, acpi_piix: factor out GPE logic

login
register
mail settings
Submitter Isaku Yamahata
Date March 16, 2011, 9:29 a.m.
Message ID <150737c6da67c205a17f0e9c5c8861e3d0a79531.1300266238.git.yamahata@valinux.co.jp>
Download mbox | patch
Permalink /patch/87203/
State New
Headers show

Comments

Isaku Yamahata - March 16, 2011, 9:29 a.m.
factor out ACPI GPE logic. Later it will be used by ICH9 ACPI.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/acpi.c       |   66 ++++++++++++++++++++++++++++++++++++++
 hw/acpi.h       |   17 ++++++++++
 hw/acpi_piix4.c |   95 ++++++++++++++----------------------------------------
 3 files changed, 108 insertions(+), 70 deletions(-)
Avi Kivity - April 17, 2011, 1:17 p.m.
On 03/16/2011 11:29 AM, Isaku Yamahata wrote:
> factor out ACPI GPE logic. Later it will be used by ICH9 ACPI.
>

I think this patch is causing qemu-kvm failures on migration:
(gdb) bt
#0  0x000000000049aff4 in qemu_put_be16s (f=0x1a74490, pv=0x2c02580, 
size=2) at hw/hw.h:108
#1  put_uint16 (f=0x1a74490, pv=0x2c02580, size=2) at savevm.c:855
#2  0x000000000049c3e4 in vmstate_save_state (f=0x1a74490, 
vmsd=0x6f0b00, opaque=0x1842ef0) at savevm.c:1436
#3  0x000000000049c3b6 in vmstate_save_state (f=0x1a74490, 
vmsd=0x6f0aa0, opaque=0x1842b90) at savevm.c:1434
#4  0x000000000049c6f1 in vmstate_save (mon=<value optimized out>, 
f=0x1a74490) at savevm.c:1459
#5  qemu_savevm_state_complete (mon=<value optimized out>, f=0x1a74490) 
at savevm.c:1600
#6  0x000000000049455a in migrate_fd_put_ready (opaque=0x1847890) at 
migration.c:383
#7  0x00000000004ce2eb in qemu_run_timers (clock=<value optimized out>) 
at qemu-timer.c:505
#8  0x00000000004ce806 in qemu_run_all_timers () at qemu-timer.c:619
#9  0x0000000000419463 in main_loop_wait (nonblocking=<value optimized 
out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1339
#10 0x0000000000433927 in kvm_main_loop () at 
/build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1590
#11 0x000000000041a3a6 in main_loop (argc=<value optimized out>, 
argv=<value optimized out>, envp=<value optimized out>)
     at /build/home/tlv/akivity/qemu-kvm/vl.c:1369
#12 main (argc=<value optimized out>, argv=<value optimized out>, 
envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257

The vmstate being migrated is "gpe".



>
> +#define VMSTATE_GPE_ARRAY(_field, _state)                            \
> + {                                                                   \
> +     .name       = (stringify(_field)),                              \
> +     .version_id = 0,                                                \
> +     .num        = GPE_LEN,                                          \
> +     .info       =&vmstate_info_uint16,                             \
> +     .size       = sizeof(uint16_t),                                 \
> +     .flags      = VMS_ARRAY | VMS_POINTER,                          \
> +     .offset     = vmstate_offset_pointer(_state, _field, uint8_t),  \
> + }
> +
>   static const VMStateDescription vmstate_gpe = {
>       .name = "gpe",
>       .version_id = 1,
>       .minimum_version_id = 1,
>       .minimum_version_id_old = 1,
>       .fields      = (VMStateField []) {
> -        VMSTATE_UINT16(sts, struct gpe_regs),
> -        VMSTATE_UINT16(en, struct gpe_regs),
> +        VMSTATE_GPE_ARRAY(sts, ACPIGPE),
> +        VMSTATE_GPE_ARRAY(en, ACPIGPE),
>           VMSTATE_END_OF_LIST()
>       }
>   };

I'm no vmstate expert, but this does look odd.  Why both VMS_ARRAY and 
VMS_POINTER? aren't we trying to save/restore a simple 16-bit value?  Or 
at least we did before this patch.

Patch

diff --git a/hw/acpi.c b/hw/acpi.c
index 24c878a..315ea25 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -348,3 +348,69 @@  void acpi_pm1_cnt_reset(ACPIPM1CNT *pm1_cnt)
         qemu_irq_lower(pm1_cnt->cmos_s3);
     }
 }
+
+/* ACPI GPE */
+void acpi_gpe_init(ACPIGPE *gpe, uint8_t len)
+{
+    gpe->len = len;
+    gpe->sts = qemu_mallocz(len / 2);
+    gpe->en = qemu_mallocz(len / 2);
+}
+
+void acpi_gpe_blk(ACPIGPE *gpe, uint32_t blk)
+{
+    gpe->blk = blk;
+}
+
+void acpi_gpe_reset(ACPIGPE *gpe)
+{
+    memset(gpe->sts, 0, gpe->len / 2);
+    memset(gpe->en, 0, gpe->len / 2);
+}
+
+static uint8_t *acpi_gpe_ioport_get_ptr(ACPIGPE *gpe, uint32_t addr)
+{
+    uint8_t *cur = NULL;
+
+    if (addr < gpe->len / 2) {
+        cur = gpe->sts + addr;
+    } else if (addr < gpe->len) {
+        cur = gpe->en + addr;
+    } else {
+        abort();
+    }
+
+    return cur;
+}
+
+void acpi_gpe_ioport_writeb(ACPIGPE *gpe, uint32_t addr, uint32_t val)
+{
+    uint8_t *cur;
+
+    addr -= gpe->blk;
+    cur = acpi_gpe_ioport_get_ptr(gpe, addr);
+    if (addr < gpe->len / 2) {
+        /* GPE_STS */
+        *cur = (*cur) & ~val;
+    } else if (addr < gpe->len) {
+        /* GPE_EN */
+        *cur = val;
+    } else {
+        abort();
+    }
+}
+
+uint32_t acpi_gpe_ioport_readb(ACPIGPE *gpe, uint32_t addr)
+{
+    uint8_t *cur;
+    uint32_t val;
+
+    addr -= gpe->blk;
+    cur = acpi_gpe_ioport_get_ptr(gpe, addr);
+    val = 0;
+    if (cur != NULL) {
+        val = *cur;
+    }
+
+    return val;
+}
diff --git a/hw/acpi.h b/hw/acpi.h
index ab70c00..0c666a8 100644
--- a/hw/acpi.h
+++ b/hw/acpi.h
@@ -126,4 +126,21 @@  void acpi_pm1_cnt_update(ACPIPM1CNT *pm1_cnt,
                          bool sci_enable, bool sci_disable);
 void acpi_pm1_cnt_reset(ACPIPM1CNT *pm1_cnt);
 
+/* GPE0 */
+struct ACPIGPE {
+    uint32_t blk;
+    uint8_t len;
+
+    uint8_t *sts;
+    uint8_t *en;
+};
+typedef struct ACPIGPE ACPIGPE;
+
+void acpi_gpe_init(ACPIGPE *gpe, uint8_t len);
+void acpi_gpe_blk(ACPIGPE *gpe, uint32_t blk);
+void acpi_gpe_reset(ACPIGPE *gpe);
+
+void acpi_gpe_ioport_writeb(ACPIGPE *gpe, uint32_t addr, uint32_t val);
+uint32_t acpi_gpe_ioport_readb(ACPIGPE *gpe, uint32_t addr);
+
 #endif /* !QEMU_HW_ACPI_H */
diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index ce86ee3..a60eb57 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -35,17 +35,13 @@ 
 #define ACPI_DBG_IO_ADDR  0xb044
 
 #define GPE_BASE 0xafe0
+#define GPE_LEN 4
 #define PCI_BASE 0xae00
 #define PCI_EJ_BASE 0xae08
 #define PCI_RMV_BASE 0xae0c
 
 #define PIIX4_PCI_HOTPLUG_STATUS 2
 
-struct gpe_regs {
-    uint16_t sts; /* status */
-    uint16_t en;  /* enabled */
-};
-
 struct pci_status {
     uint32_t up;
     uint32_t down;
@@ -69,7 +65,7 @@  typedef struct PIIX4PMState {
     int kvm_enabled;
 
     /* for pci hotplug */
-    struct gpe_regs gpe;
+    ACPIGPE gpe;
     struct pci_status pci0_status;
     uint32_t pci0_hotplug_enable;
 } PIIX4PMState;
@@ -89,7 +85,7 @@  static void pm_update_sci(PIIX4PMState *s)
                    ACPI_BITMASK_POWER_BUTTON_ENABLE |
                    ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
                    ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
-        (((s->gpe.sts & s->gpe.en) & PIIX4_PCI_HOTPLUG_STATUS) != 0);
+        (((s->gpe.sts[0] & s->gpe.en[0]) & PIIX4_PCI_HOTPLUG_STATUS) != 0);
 
     qemu_set_irq(s->irq, sci_level);
     /* schedule a timer interruption if needed */
@@ -213,14 +209,25 @@  static int vmstate_acpi_post_load(void *opaque, int version_id)
     return 0;
 }
 
+#define VMSTATE_GPE_ARRAY(_field, _state)                            \
+ {                                                                   \
+     .name       = (stringify(_field)),                              \
+     .version_id = 0,                                                \
+     .num        = GPE_LEN,                                          \
+     .info       = &vmstate_info_uint16,                             \
+     .size       = sizeof(uint16_t),                                 \
+     .flags      = VMS_ARRAY | VMS_POINTER,                          \
+     .offset     = vmstate_offset_pointer(_state, _field, uint8_t),  \
+ }
+
 static const VMStateDescription vmstate_gpe = {
     .name = "gpe",
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .fields      = (VMStateField []) {
-        VMSTATE_UINT16(sts, struct gpe_regs),
-        VMSTATE_UINT16(en, struct gpe_regs),
+        VMSTATE_GPE_ARRAY(sts, ACPIGPE),
+        VMSTATE_GPE_ARRAY(en, ACPIGPE),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -251,7 +258,7 @@  static const VMStateDescription vmstate_acpi = {
         VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
         VMSTATE_TIMER(tmr.timer, PIIX4PMState),
         VMSTATE_INT64(tmr.overflow_time, PIIX4PMState),
-        VMSTATE_STRUCT(gpe, PIIX4PMState, 2, vmstate_gpe, struct gpe_regs),
+        VMSTATE_STRUCT(gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
         VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status,
                        struct pci_status),
         VMSTATE_END_OF_LIST()
@@ -345,6 +352,7 @@  static int piix4_pm_initfn(PCIDevice *dev)
     register_ioport_read(s->smb_io_base, 64, 1, smb_ioport_readb, &s->smb);
 
     acpi_pm_tmr_init(&s->tmr, pm_tmr_timer);
+    acpi_gpe_init(&s->gpe, GPE_LEN);
 
     qemu_system_powerdown = *qemu_allocate_irqs(piix4_powerdown, s, 1);
 
@@ -398,74 +406,20 @@  static void piix4_pm_register(void)
 
 device_init(piix4_pm_register);
 
-static uint32_t gpe_read_val(uint16_t val, uint32_t addr)
-{
-    if (addr & 1)
-        return (val >> 8) & 0xff;
-    return val & 0xff;
-}
-
 static uint32_t gpe_readb(void *opaque, uint32_t addr)
 {
-    uint32_t val = 0;
     PIIX4PMState *s = opaque;
-    struct gpe_regs *g = &s->gpe;
-
-    switch (addr) {
-        case GPE_BASE:
-        case GPE_BASE + 1:
-            val = gpe_read_val(g->sts, addr);
-            break;
-        case GPE_BASE + 2:
-        case GPE_BASE + 3:
-            val = gpe_read_val(g->en, addr);
-            break;
-        default:
-            break;
-    }
+    uint32_t val = acpi_gpe_ioport_readb(&s->gpe, addr);
 
     PIIX4_DPRINTF("gpe read %x == %x\n", addr, val);
     return val;
 }
 
-static void gpe_write_val(uint16_t *cur, int addr, uint32_t val)
-{
-    if (addr & 1)
-        *cur = (*cur & 0xff) | (val << 8);
-    else
-        *cur = (*cur & 0xff00) | (val & 0xff);
-}
-
-static void gpe_reset_val(uint16_t *cur, int addr, uint32_t val)
-{
-    uint16_t x1, x0 = val & 0xff;
-    int shift = (addr & 1) ? 8 : 0;
-
-    x1 = (*cur >> shift) & 0xff;
-
-    x1 = x1 & ~x0;
-
-    *cur = (*cur & (0xff << (8 - shift))) | (x1 << shift);
-}
-
 static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val)
 {
     PIIX4PMState *s = opaque;
-    struct gpe_regs *g = &s->gpe;
-
-    switch (addr) {
-        case GPE_BASE:
-        case GPE_BASE + 1:
-            gpe_reset_val(&g->sts, addr, val);
-            break;
-        case GPE_BASE + 2:
-        case GPE_BASE + 3:
-            gpe_write_val(&g->en, addr, val);
-            break;
-        default:
-            break;
-    }
 
+    acpi_gpe_ioport_writeb(&s->gpe, addr, val);
     pm_update_sci(s);
 
     PIIX4_DPRINTF("gpe write %x <== %d\n", addr, val);
@@ -548,8 +502,9 @@  static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
 {
     struct pci_status *pci0_status = &s->pci0_status;
 
-    register_ioport_write(GPE_BASE, 4, 1, gpe_writeb, s);
-    register_ioport_read(GPE_BASE, 4, 1,  gpe_readb, s);
+    register_ioport_write(GPE_BASE, GPE_LEN, 1, gpe_writeb, s);
+    register_ioport_read(GPE_BASE, GPE_LEN, 1,  gpe_readb, s);
+    acpi_gpe_blk(&s->gpe, GPE_BASE);
 
     register_ioport_write(PCI_BASE, 8, 4, pcihotplug_write, pci0_status);
     register_ioport_read(PCI_BASE, 8, 4,  pcihotplug_read, pci0_status);
@@ -565,13 +520,13 @@  static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
 
 static void enable_device(PIIX4PMState *s, int slot)
 {
-    s->gpe.sts |= PIIX4_PCI_HOTPLUG_STATUS;
+    s->gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
     s->pci0_status.up |= (1 << slot);
 }
 
 static void disable_device(PIIX4PMState *s, int slot)
 {
-    s->gpe.sts |= PIIX4_PCI_HOTPLUG_STATUS;
+    s->gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
     s->pci0_status.down |= (1 << slot);
 }