Skip to contentSkip to conversation

fix acpi regression

PATCH Superseded Wen Congyang 3 participants 8 messages Apr 12, 2011 – Apr 13, 2011

lore
Share & tools
lore

Wen Congyang

author
#0 lore raw

PATCH 1/1 fix acpi regression

This bug is introduced by commit 23910d3f.

Signed-off-by Wen Congyang <[email protected]>
---
 hw/acpi.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)
hw/acpi.c +3 -7
index e372474..790bd3b 100644
@@ -355,7 +355,7 @@ static uint8_t *acpi_gpe_ioport_get_ptr(ACPIGPE *gpe, uint32_t addr)
     if (addr < gpe->len / 2) {
         cur = gpe->sts + addr;
     } else if (addr < gpe->len) {
-        cur = gpe->en + addr;
+        cur = gpe->en + addr - gpe->len / 2;
     } else {
         abort();
     }

@@ -369,12 +369,8 @@ void acpi_gpe_ioport_writeb(ACPIGPE *gpe, uint32_t addr, uint32_t val)
 
     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;
+    if (cur != NULL) {
+        *cur = val & 0xff;
     } else {
         abort();
     }
Signature · 1 line
1.7.1
#1 lore raw
Quoted 1 line · Wen Congyang

This bug is introduced by commit 23910d3f.

Oh, Thanks. Good catch. I agree with the first hunk.
But what is the second hunk for? The GPE STS register is W1C. (NULL check and 0xff masking is okay. it's a bit redundant, though)

Quoted 1 line

From ACPI4 spec 4.7.4.1.1.

can only be cleared by software writing a "1" to its respective bit position.

thanks,

Quoted 38 lines
Signed-off-by Wen Congyang <[email protected]>
---
 hw/acpi.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)
hw/acpi.c +3 -7
index e372474..790bd3b 100644
@@ -355,7 +355,7 @@ static uint8_t *acpi_gpe_ioport_get_ptr(ACPIGPE *gpe, uint32_t addr)
     if (addr < gpe->len / 2) {
         cur = gpe->sts + addr;
     } else if (addr < gpe->len) {
-        cur = gpe->en + addr;
+        cur = gpe->en + addr - gpe->len / 2;
     } else {
         abort();
     }

@@ -369,12 +369,8 @@ void acpi_gpe_ioport_writeb(ACPIGPE *gpe, uint32_t addr, uint32_t val)
 
     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;
+    if (cur != NULL) {
+        *cur = val & 0xff;
     } else {
         abort();
     }
Signature · 1 line
1.7.1
Signature · 1 line
yamahata
author
#2 lore raw

At 04/12/2011 04:48 PM, Isaku Yamahata Write:

Quoted 6 lines · Wen Congyang
Quoted 1 line

This bug is introduced by commit 23910d3f.

Oh, Thanks. Good catch. I agree with the first hunk.
But what is the second hunk for? The GPE STS register is W1C. (NULL check and 0xff masking is okay. it's a bit redundant, though)

I found this bug when I hotplug a nic.
I do not know about ACPI.
The second hunk is the same before commit 23910d3f. I will read ACPI spec to confirm it and update this patch.

Thanks

Quoted 44 lines
Quoted 2 lines

From ACPI4 spec 4.7.4.1.1.

can only be cleared by software writing a "1" to its respective bit position.

thanks,

Quoted 38 lines
Signed-off-by Wen Congyang <[email protected]>
---
 hw/acpi.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)
hw/acpi.c +3 -7
index e372474..790bd3b 100644
@@ -355,7 +355,7 @@ static uint8_t *acpi_gpe_ioport_get_ptr(ACPIGPE *gpe, uint32_t addr)
     if (addr < gpe->len / 2) {
         cur = gpe->sts + addr;
     } else if (addr < gpe->len) {
-        cur = gpe->en + addr;
+        cur = gpe->en + addr - gpe->len / 2;
     } else {
         abort();
     }

@@ -369,12 +369,8 @@ void acpi_gpe_ioport_writeb(ACPIGPE *gpe, uint32_t addr, uint32_t val)
 
     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;
+    if (cur != NULL) {
+        *cur = val & 0xff;
     } else {
         abort();
     }
Signature · 1 line
1.7.1
#3 lore raw
Quoted 4 lines · Wen Congyang

At 04/12/2011 04:48 PM, Isaku Yamahata Write:

Quoted 3 lines · Wen Congyang
Quoted 1 line

This bug is introduced by commit 23910d3f.

Quoted 8 lines
Quoted 3 lines

Oh, Thanks. Good catch. I agree with the first hunk.
But what is the second hunk for? The GPE STS register is W1C. (NULL check and 0xff masking is okay. it's a bit redundant, though)

I found this bug when I hotplug a nic.
I do not know about ACPI.
The second hunk is the same before commit 23910d3f. I will read ACPI spec to confirm it and update this patch.

You mean gpe_writeb(), gpe_write_val(), gpe_reset_val()? gpe_reset_val() does W1C.
The GPE0_LEN of piix4 is 4 byte length and the first 2 registers are sts register. From the old code,

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; <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< W1C

    *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;

thanks,

Quoted 49 lines

Thanks

Quoted 44 lines
Quoted 2 lines

From ACPI4 spec 4.7.4.1.1.

can only be cleared by software writing a "1" to its respective bit position.

thanks,

Quoted 38 lines
Signed-off-by Wen Congyang <[email protected]>
---
 hw/acpi.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)
hw/acpi.c +3 -7
index e372474..790bd3b 100644
@@ -355,7 +355,7 @@ static uint8_t *acpi_gpe_ioport_get_ptr(ACPIGPE *gpe, uint32_t addr)
     if (addr < gpe->len / 2) {
         cur = gpe->sts + addr;
     } else if (addr < gpe->len) {
-        cur = gpe->en + addr;
+        cur = gpe->en + addr - gpe->len / 2;
     } else {
         abort();
     }

@@ -369,12 +369,8 @@ void acpi_gpe_ioport_writeb(ACPIGPE *gpe, uint32_t addr, uint32_t val)
 
     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;
+    if (cur != NULL) {
+        *cur = val & 0xff;
     } else {
         abort();
     }
Signature · 1 line
1.7.1
Signature · 1 line
yamahata
author
#4 lore raw

At 04/12/2011 05:20 PM, Isaku Yamahata Write:

Quoted 33 lines · Wen Congyang
Quoted 4 lines

At 04/12/2011 04:48 PM, Isaku Yamahata Write:

Quoted 3 lines · Wen Congyang
Quoted 1 line

This bug is introduced by commit 23910d3f.

Quoted 8 lines
Quoted 3 lines

Oh, Thanks. Good catch. I agree with the first hunk.
But what is the second hunk for? The GPE STS register is W1C. (NULL check and 0xff masking is okay. it's a bit redundant, though)

I found this bug when I hotplug a nic.
I do not know about ACPI.
The second hunk is the same before commit 23910d3f. I will read ACPI spec to confirm it and update this patch.

You mean gpe_writeb(), gpe_write_val(), gpe_reset_val()? gpe_reset_val() does W1C.
The GPE0_LEN of piix4 is 4 byte length and the first 2 registers are sts register. From the old code,

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; <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< W1C

    *cur = (*cur & (0xff << (8 - shift))) | (x1 << shift);
}

static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val)

Quoted 9 lines
{   
    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;

Ah, you are right.
Thanks.

Quoted 53 lines

thanks,

Quoted 49 lines

Thanks

Quoted 44 lines
Quoted 2 lines

From ACPI4 spec 4.7.4.1.1.

can only be cleared by software writing a "1" to its respective bit position.

thanks,

Quoted 38 lines
Signed-off-by Wen Congyang <[email protected]>
---
 hw/acpi.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)
hw/acpi.c +3 -7
index e372474..790bd3b 100644
@@ -355,7 +355,7 @@ static uint8_t *acpi_gpe_ioport_get_ptr(ACPIGPE *gpe, uint32_t addr)
     if (addr < gpe->len / 2) {
         cur = gpe->sts + addr;
     } else if (addr < gpe->len) {
-        cur = gpe->en + addr;
+        cur = gpe->en + addr - gpe->len / 2;
     } else {
         abort();
     }

@@ -369,12 +369,8 @@ void acpi_gpe_ioport_writeb(ACPIGPE *gpe, uint32_t addr, uint32_t val)
 
     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;
+    if (cur != NULL) {
+        *cur = val & 0xff;
     } else {
         abort();
     }
Signature · 1 line
1.7.1
author
#5 lore raw

PATCH v2 1/1 fix acpi regression

In reply to Wen Congyang

This bug is introduced by commit 23910d3f.

Signed-off-by Wen Congyang <[email protected]>
---
 hw/acpi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
hw/acpi.c +1 -1
index e372474..ad40fb4 100644
@@ -355,7 +355,7 @@ static uint8_t *acpi_gpe_ioport_get_ptr(ACPIGPE *gpe, uint32_t addr)
     if (addr < gpe->len / 2) {
         cur = gpe->sts + addr;
     } else if (addr < gpe->len) {
-        cur = gpe->en + addr;
+        cur = gpe->en + addr - gpe->len / 2;
     } else {
         abort();
     }
Signature · 1 line
1.7.1
#6 lore raw

Thank you. Looks good.

Quoted 24 lines · Wen Congyang

This bug is introduced by commit 23910d3f.

Signed-off-by Wen Congyang <[email protected]>
---
 hw/acpi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
hw/acpi.c +1 -1
index e372474..ad40fb4 100644
@@ -355,7 +355,7 @@ static uint8_t *acpi_gpe_ioport_get_ptr(ACPIGPE *gpe, uint32_t addr)
     if (addr < gpe->len / 2) {
         cur = gpe->sts + addr;
     } else if (addr < gpe->len) {
-        cur = gpe->en + addr;
+        cur = gpe->en + addr - gpe->len / 2;
     } else {
         abort();
     }
Signature · 1 line
1.7.1
Signature · 1 line
yamahata
Applied
#7 lore raw
In reply to Wen Congyang
Quoted 7 lines · Wen Congyang

This bug is introduced by commit 23910d3f.

Signed-off-by Wen Congyang <[email protected]>
---
 hw/acpi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Thanks, applied.

Quoted 18 lines
hw/acpi.c +1 -1
index e372474..ad40fb4 100644
@@ -355,7 +355,7 @@ static uint8_t *acpi_gpe_ioport_get_ptr(ACPIGPE *gpe, uint32_t addr)
     if (addr < gpe->len / 2) {
         cur = gpe->sts + addr;
     } else if (addr < gpe->len) {
-        cur = gpe->en + addr;
+        cur = gpe->en + addr - gpe->len / 2;
     } else {
         abort();
     }
Signature · 1 line
1.7.1
Signature · 2 lines
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
[email protected]                 http://www.aurel32.net