On Tue, Apr 12, 2011 at 05:08:18PM +0800, Wen Congyang wrote: > At 04/12/2011 04:48 PM, Isaku Yamahata Write: > > On Tue, Apr 12, 2011 at 01:13:15PM +0800, Wen Congyang wrote: > >> 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.
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, > > Thanks > > > > >>From ACPI4 spec 4.7.4.1.1. > > can only be cleared by software writing a "1" to its respective bit > > position. > > > > thanks, > > > >> > >> Signed-off-by: Wen Congyang <we...@cn.fujitsu.com> > >> > >> --- > >> hw/acpi.c | 10 +++------- > >> 1 files changed, 3 insertions(+), 7 deletions(-) > >> > >> diff --git a/hw/acpi.c b/hw/acpi.c > >> index e372474..790bd3b 100644 > >> --- a/hw/acpi.c > >> +++ b/hw/acpi.c > >> @@ -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(); > >> } > >> -- > >> 1.7.1 > >> > > -- yamahata