[PATCH 1/3] Fix GPE registers read/write handling.

[PATCH 1/3] Fix GPE registers read/write handling.

[Gleb Natapov]

For STS register bit are cleared by writing 1 into it.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---

 qemu/hw/acpi.c |   42 ++++++++++++++++++++++++++++++------------
 1 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/qemu/hw/acpi.c b/qemu/hw/acpi.c
index b998225..68513c0 100644
--- a/qemu/hw/acpi.c
+++ b/qemu/hw/acpi.c
@@ -590,6 +590,13 @@ struct pci_status {
 static struct gpe_regs gpe;
 static struct pci_status pci0_status;
 
+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;
@@ -603,16 +610,12 @@ static uint32_t gpe_readb(void *opaque, uint32_t addr)
             break;
 
         case GPE_BASE:
-            val = g->sts & 0xFF;
-            break;
         case GPE_BASE + 1:
-            val =  (g->sts >> 8) & 0xFF;
+            val = gpe_read_val(g->sts, addr);
             break;
         case GPE_BASE + 2:
-            val =  g->en & 0xFF;
-            break;
         case GPE_BASE + 3:
-            val =  (g->en >> 8) & 0xFF;
+            val = gpe_read_val(g->en, addr);
             break;
         default:
             break;
@@ -624,6 +627,25 @@ static uint32_t gpe_readb(void *opaque, uint32_t addr)
     return val;
 }
 
+static uint16_t gpe_write_val(uint16_t cur, int addr, uint32_t val)
+{
+    if (addr & 1)
+        return (cur & 0xff) | (val << 8);
+    return (cur & 0xff00) | (val & 0xff);
+}
+
+static uint16_t 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;
+
+    return (cur & (0xff << (8 - shift))) | (x1 << shift);
+}
+
 static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val)
 {
     struct gpe_regs *g = opaque;
@@ -636,16 +658,12 @@ static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val)
             break;
 
         case GPE_BASE:
-            g->sts = (g->sts & ~0xFFFF) | (val & 0xFFFF);
-            break;
         case GPE_BASE + 1:
-            g->sts = (g->sts & 0xFFFF) | (val << 8);
+            g->sts = gpe_reset_val(g->sts, addr, val);
             break;
         case GPE_BASE + 2:
-            g->en = (g->en & ~0xFFFF) | (val & 0xFFFF);
-            break;
         case GPE_BASE + 3:
-            g->en = (g->en & 0xFFFF) | (val << 8);
+            g->en = gpe_write_val(g->en, addr, val);
             break;
         default:
             break;

[PATCH 2/3] Fix CPU hotplug

[Gleb Natapov]

1) Disabled processor's _STA method should return 0 (this fixes Vista's
   BSOD on resuming after hibernate problem)
2) Disabled processor's _MAT method should return disabled MADT entry
   instead of 0
3) Extend bitmask of hot pluggable CPUs to be 16 bit long
4) Generate interrupt only if corespondent EN bit is set
5) Use reserved STS bits from PIIX4 chipset to avoid clash in the
   future.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---

 qemu/hw/acpi.c |   39 +++++++++++++++++++++------------------
 1 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/bios/acpi-dsdt.dsl b/bios/acpi-dsdt.dsl
index d67616d..c400382 100755
--- a/bios/acpi-dsdt.dsl
+++ b/bios/acpi-dsdt.dsl
@@ -27,28 +27,29 @@ DefinitionBlock (
 {
    Scope (\_PR)
    {
-	OperationRegion( PRST, SystemIO, 0xaf00, 0x02)
-	Field (PRST, ByteAcc, NoLock, WriteAsZeros)
+	OperationRegion( PRST, SystemIO, 0xaf00, 0x04)
+	Field (PRST, WordAcc, NoLock, Preserve)
 	{
-		PRU, 8,
-		PRD, 8,
+		PRU, 16,
+		PRD, 16,
 	}
 
 #define gen_processor(nr, name) 				            \
 	Processor (CPU##name, nr, 0x0000b010, 0x06) {                       \
-            Name (TMP, Buffer(0x8) {0x0, 0x8, nr, nr, 0x1, 0x0, 0x0, 0x0})  \
+            Name (PREN, Buffer(0x8) {0x0, 0x8, nr, nr, 0x1, 0x0, 0x0, 0x0}) \
+            Name (PRDS, Buffer(0x8) {0x0, 0x8, nr, nr, 0x0, 0x0, 0x0, 0x0}) \
             Method(_MAT, 0) {                                               \
-                If (And(\_PR.PRU, ShiftLeft(1, nr))) { Return(TMP) }        \
-                Else { Return(0x0) }                                        \
+                If (And(\_PR.PRU, ShiftLeft(1, nr))) { Return(PREN) }       \
+                Else { Return(PRDS) }                                       \
             }                                                               \
             Method (_STA) {                                                 \
-                Return(0xF)                                                 \
+                If (And(\_PR.PRU, ShiftLeft(1, nr))) { Return(0xF) }        \
+                Else { Return(0x0) }                                        \
             }                                                               \
         }                                                                   \
 
 
-
-        Processor (CPU0, 0x00, 0x0000b010, 0x06) {Method (_STA) { Return(0xF)}}
+	gen_processor(0, 0)
 	gen_processor(1, 1)
 	gen_processor(2, 2)
 	gen_processor(3, 3)
@@ -63,6 +64,28 @@ DefinitionBlock (
 	gen_processor(12, C)
 	gen_processor(13, D)
 	gen_processor(14, E)
+
+	Method (NTFY, 2, NotSerialized) {
+#define gen_ntfy(nr)                              \
+	If (LEqual(Arg0, 0x##nr)) {               \
+		Notify(CPU##nr, Arg1)             \
+	}
+		gen_ntfy(0)
+		gen_ntfy(1)
+		gen_ntfy(2)
+		gen_ntfy(3)
+		gen_ntfy(4)
+		gen_ntfy(5)
+		gen_ntfy(6)
+		gen_ntfy(7)
+		gen_ntfy(8)
+		gen_ntfy(9)
+		gen_ntfy(A)
+		gen_ntfy(B)
+		gen_ntfy(C)
+		gen_ntfy(D)
+		gen_ntfy(E)
+	}
     }
 
     Scope (\)
@@ -666,33 +689,12 @@ DefinitionBlock (
         Zero,  /* reserved */
         Zero   /* reserved */
     })
+
     Scope (\_GPE)
     {
-
-#define gen_cpu_hotplug(name, nr)                      \
-	If (And(\_PR.PRU, ShiftLeft(1, nr))) {     \
-	    Notify(\_PR.CPU##name, 1)              \
-        }                                          \
-	If (And(\_PR.PRD, ShiftLeft(1, nr))) {     \
-	    Notify(\_PR.CPU##name, 3)              \
-        }
+	Name(_HID, "ACPI0006")
 
         Method(_L00) {
-	    gen_cpu_hotplug(1, 1)
-	    gen_cpu_hotplug(2, 2)
-	    gen_cpu_hotplug(3, 3)
-	    gen_cpu_hotplug(4, 4)
-	    gen_cpu_hotplug(5, 5)
-	    gen_cpu_hotplug(6, 6)
-	    gen_cpu_hotplug(7, 7)
-	    gen_cpu_hotplug(8, 8)
-	    gen_cpu_hotplug(9, 9)
-	    gen_cpu_hotplug(A, 10)
-	    gen_cpu_hotplug(B, 11)
-	    gen_cpu_hotplug(C, 12)
-	    gen_cpu_hotplug(D, 13)
-	    gen_cpu_hotplug(E, 14)
-
             Return(0x01)
         }
 
@@ -739,9 +741,29 @@ DefinitionBlock (
 
             Return(0x01)
         }
+
         Method(_L02) {
+	    Store(Zero, Local3)
+	    Store(\_PR.PRU, Local2)
+	    Xor(Local2, \_PR.PRD, Local0)
+	    Store(Local2, \_PR.PRD)
+	    Store(\_PR.PRD, Local1)
+            While (LNotEqual (Local0, Zero)) {
+		Store(ShiftLeft(1, Local3), Local1)
+		If (And(Local0, Local1)) {
+			Store(And(Local0, Not(Local1)), Local0)
+			If (And(Local2, Local1)) {
+	                	Store(1, Local4)
+			} Else {
+	                	Store(3, Local4)
+			}
+	                \_PR.NTFY(Local3, Local4)
+		}
+		Increment(Local3)
+	    }
             Return(0x01)
         }
+
         Method(_L03) {
             Return(0x01)
         }
diff --git a/qemu/hw/acpi.c b/qemu/hw/acpi.c
index 68513c0..83079fa 100644
--- a/qemu/hw/acpi.c
+++ b/qemu/hw/acpi.c
@@ -578,8 +578,8 @@ void qemu_system_powerdown(void)
 struct gpe_regs {
     uint16_t sts; /* status */
     uint16_t en;  /* enabled */
-    uint8_t up;
-    uint8_t down;
+    uint16_t cpus_sts;
+    uint16_t bios_cpus_sts;
 };
 
 struct pci_status {
@@ -603,10 +603,12 @@ static uint32_t gpe_readb(void *opaque, uint32_t addr)
     struct gpe_regs *g = opaque;
     switch (addr) {
         case PROC_BASE:
-            val = g->up;
-            break;
         case PROC_BASE + 1:
-            val = g->down;
+            val = gpe_read_val(g->cpus_sts, addr);
+            break;
+        case PROC_BASE + 2:
+        case PROC_BASE + 3:
+            val = gpe_read_val(g->bios_cpus_sts, addr);
             break;
 
         case GPE_BASE:
@@ -651,10 +653,12 @@ static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val)
     struct gpe_regs *g = opaque;
     switch (addr) {
         case PROC_BASE:
-            g->up = val;
-            break;
         case PROC_BASE + 1:
-            g->down = val;
+            /* don't allow to change cpu_sts from inside a guest */
+            break;
+        case PROC_BASE + 2:
+        case PROC_BASE + 3:
+            g->bios_cpus_sts = gpe_write_val(g->bios_cpus_sts, addr, val);
             break;
 
         case GPE_BASE:
@@ -735,6 +739,7 @@ static const char *model;
 
 void qemu_system_hot_add_init(const char *cpu_model)
 {
+    gpe.bios_cpus_sts = gpe.cpus_sts = (1 << smp_cpus) - 1;
     register_ioport_write(GPE_BASE, 4, 1, gpe_writeb, &gpe);
     register_ioport_read(GPE_BASE, 4, 1,  gpe_readb, &gpe);
 
@@ -752,16 +757,14 @@ void qemu_system_hot_add_init(const char *cpu_model)
 
 static void enable_processor(struct gpe_regs *g, int cpu)
 {
-    g->sts |= 1;
-    g->en |= 1;
-    g->up |= (1 << cpu);
+    g->sts |= 4;
+    g->cpus_sts |= (1 << cpu);
 }
 
 static void disable_processor(struct gpe_regs *g, int cpu)
 {
-    g->sts |= 1;
-    g->en |= 1;
-    g->down |= (1 << cpu);
+    g->sts |= 4;
+    g->cpus_sts &= ~(1 << cpu);
 }
 
 #if defined(TARGET_I386) || defined(TARGET_X86_64)
@@ -802,14 +805,14 @@ void qemu_system_cpu_hot_add(int cpu, int state)
 #endif
     }
 
-    qemu_set_irq(pm_state->irq, 1);
-    gpe.up = 0;
-    gpe.down = 0;
     if (state)
         enable_processor(&gpe, cpu);
     else
         disable_processor(&gpe, cpu);
-    qemu_set_irq(pm_state->irq, 0);
+    if (gpe.en & 4) {
+        qemu_set_irq(pm_state->irq, 1);
+        qemu_set_irq(pm_state->irq, 0);
+    }
 }
 #endif
 

Re: [PATCH 2/3] Fix CPU hotplug

[Avi Kivity]

Gleb Natapov wrote:
> 1) Disabled processor's _STA method should return 0 (this fixes Vista's
>    BSOD on resuming after hibernate problem)
> 2) Disabled processor's _MAT method should return disabled MADT entry
>    instead of 0
> 3) Extend bitmask of hot pluggable CPUs to be 16 bit long
> 4) Generate interrupt only if corespondent EN bit is set
>   

Looks like a good idea, but it is really necessary?  The guest should be 
able to deal with null notifies.
(I'd like to apply this, just want to understand).

> 5) Use reserved STS bits from PIIX4 chipset to avoid clash in the
>    future.
>   

Please split into separate patches.

> @@ -739,9 +741,29 @@ DefinitionBlock (
>  
>              Return(0x01)
>          }
> +
>          Method(_L02) {
> +	    Store(Zero, Local3)
> +	    Store(\_PR.PRU, Local2)
> +	    Xor(Local2, \_PR.PRD, Local0)
> +	    Store(Local2, \_PR.PRD)
> +	    Store(\_PR.PRD, Local1)
> +            While (LNotEqual (Local0, Zero)) {
> +		Store(ShiftLeft(1, Local3), Local1)
> +		If (And(Local0, Local1)) {
> +			Store(And(Local0, Not(Local1)), Local0)
> +			If (And(Local2, Local1)) {
> +	                	Store(1, Local4)
> +			} Else {
> +	                	Store(3, Local4)
> +			}
> +	                \_PR.NTFY(Local3, Local4)
> +		}
> +		Increment(Local3)
> +	    }
>              Return(0x01)
>          }
>   

Please document this.

> --- a/qemu/hw/acpi.c
> +++ b/qemu/hw/acpi.c
> @@ -578,8 +578,8 @@ void qemu_system_powerdown(void)
>  struct gpe_regs {
>      uint16_t sts; /* status */
>      uint16_t en;  /* enabled */
> -    uint8_t up;
> -    uint8_t down;
> +    uint16_t cpus_sts;
> +    uint16_t bios_cpus_sts;
>  };
>   

We'll need to scale this soon.

>  
>  struct pci_status {
> @@ -603,10 +603,12 @@ static uint32_t gpe_readb(void *opaque, uint32_t addr)
>      struct gpe_regs *g = opaque;
>      switch (addr) {
>          case PROC_BASE:
> -            val = g->up;
> -            break;
>          case PROC_BASE + 1:
> -            val = g->down;
> +            val = gpe_read_val(g->cpus_sts, addr);
> +            break;
> +        case PROC_BASE + 2:
> +        case PROC_BASE + 3:
> +            val = gpe_read_val(g->bios_cpus_sts, addr);
>              break;
>   

Why can't the bios maintain bios_cpu_sts in RAM?


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/3] Fix CPU hotplug

[Gleb Natapov]

On Wed, Feb 04, 2009 at 02:45:12PM +0200, Avi Kivity wrote:
> Gleb Natapov wrote:
>> 1) Disabled processor's _STA method should return 0 (this fixes Vista's
>>    BSOD on resuming after hibernate problem)
>> 2) Disabled processor's _MAT method should return disabled MADT entry
>>    instead of 0
>> 3) Extend bitmask of hot pluggable CPUs to be 16 bit long
>> 4) Generate interrupt only if corespondent EN bit is set
>>   
>
> Looks like a good idea, but it is really necessary?  The guest should be  
> able to deal with null notifies.
> (I'd like to apply this, just want to understand).
>
I don't really know what different OSes will do with null notifiers. But
if one of them will not handle them properly I don't want to be the one
who'll debug it :)
 
>> --- a/qemu/hw/acpi.c
>> +++ b/qemu/hw/acpi.c
>> @@ -578,8 +578,8 @@ void qemu_system_powerdown(void)
>>  struct gpe_regs {
>>      uint16_t sts; /* status */
>>      uint16_t en;  /* enabled */
>> -    uint8_t up;
>> -    uint8_t down;
>> +    uint16_t cpus_sts;
>> +    uint16_t bios_cpus_sts;
>>  };
>>   
>
> We'll need to scale this soon.
>
We should think how to handle Windows 2000 15 CPU limit.

>>   struct pci_status {
>> @@ -603,10 +603,12 @@ static uint32_t gpe_readb(void *opaque, uint32_t addr)
>>      struct gpe_regs *g = opaque;
>>      switch (addr) {
>>          case PROC_BASE:
>> -            val = g->up;
>> -            break;
>>          case PROC_BASE + 1:
>> -            val = g->down;
>> +            val = gpe_read_val(g->cpus_sts, addr);
>> +            break;
>> +        case PROC_BASE + 2:
>> +        case PROC_BASE + 3:
>> +            val = gpe_read_val(g->bios_cpus_sts, addr);
>>              break;
>>   
>
> Why can't the bios maintain bios_cpu_sts in RAM?
>
It can, just need to find a place for it. Currently our AML does not use
RAM at all.

--
			Gleb.

Re: [PATCH 2/3] Fix CPU hotplug

[Avi Kivity]

Gleb Natapov wrote:
>>>   struct pci_status {
>>> @@ -603,10 +603,12 @@ static uint32_t gpe_readb(void *opaque, uint32_t addr)
>>>      struct gpe_regs *g = opaque;
>>>      switch (addr) {
>>>          case PROC_BASE:
>>> -            val = g->up;
>>> -            break;
>>>          case PROC_BASE + 1:
>>> -            val = g->down;
>>> +            val = gpe_read_val(g->cpus_sts, addr);
>>> +            break;
>>> +        case PROC_BASE + 2:
>>> +        case PROC_BASE + 3:
>>> +            val = gpe_read_val(g->bios_cpus_sts, addr);
>>>              break;
>>>   
>>>       
>> Why can't the bios maintain bios_cpu_sts in RAM?
>>
>>     
> It can, just need to find a place for it. Currently our AML does not use
> RAM at all.

OperationRegion(..., SystemMemory, ...) should work.  It's better to 
avoid introducing unnecessary virtual hardware.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/3] Fix CPU hotplug

[Gleb Natapov]

On Wed, Feb 04, 2009 at 03:24:59PM +0200, Avi Kivity wrote:
> Gleb Natapov wrote:
>>>>   struct pci_status {
>>>> @@ -603,10 +603,12 @@ static uint32_t gpe_readb(void *opaque, uint32_t addr)
>>>>      struct gpe_regs *g = opaque;
>>>>      switch (addr) {
>>>>          case PROC_BASE:
>>>> -            val = g->up;
>>>> -            break;
>>>>          case PROC_BASE + 1:
>>>> -            val = g->down;
>>>> +            val = gpe_read_val(g->cpus_sts, addr);
>>>> +            break;
>>>> +        case PROC_BASE + 2:
>>>> +        case PROC_BASE + 3:
>>>> +            val = gpe_read_val(g->bios_cpus_sts, addr);
>>>>              break;
>>>>         
>>> Why can't the bios maintain bios_cpu_sts in RAM?
>>>
>>>     
>> It can, just need to find a place for it. Currently our AML does not use
>> RAM at all.
>
> OperationRegion(..., SystemMemory, ...) should work.  It's better to  
> avoid introducing unnecessary virtual hardware.
>
But what address to chose. It needs to be reserved in e820 map and S3
resume should not touch it.

--
			Gleb.

Re: [PATCH 2/3] Fix CPU hotplug

[Avi Kivity]

Gleb Natapov wrote:

    

>>> It can, just need to find a place for it. Currently our AML does not use
>>> RAM at all.
>>>       
>> OperationRegion(..., SystemMemory, ...) should work.  It's better to  
>> avoid introducing unnecessary virtual hardware.
>>
>>     
> But what address to chose. It needs to be reserved in e820 map and S3
> resume should not touch it.
>
>   

The Extended BIOS Data Area, at 640K - epsilon.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 2/3] Fix CPU hotplug

[Gleb Natapov]

On Wed, Feb 04, 2009 at 03:35:16PM +0200, Avi Kivity wrote:
> Gleb Natapov wrote:
>
>    
>
>>>> It can, just need to find a place for it. Currently our AML does not use
>>>> RAM at all.
>>>>       
>>> OperationRegion(..., SystemMemory, ...) should work.  It's better to  
>>> avoid introducing unnecessary virtual hardware.
>>>
>>>     
>> But what address to chose. It needs to be reserved in e820 map and S3
>> resume should not touch it.
>>
>>   
>
> The Extended BIOS Data Area, at 640K - epsilon.
>
EBDA can be moved by add-on BIOS so epsilon may change depending on your
HW.

--
			Gleb.

[PATCH 3/3] PCI hotplug fixes

[Gleb Natapov]

Generate interrupt only if corespondent EN bit is set.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---

 qemu/hw/acpi.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/qemu/hw/acpi.c b/qemu/hw/acpi.c
index 83079fa..337432a 100644
--- a/qemu/hw/acpi.c
+++ b/qemu/hw/acpi.c
@@ -819,25 +819,26 @@ void qemu_system_cpu_hot_add(int cpu, int state)
 static void enable_device(struct pci_status *p, struct gpe_regs *g, int slot)
 {
     g->sts |= 2;
-    g->en |= 2;
     p->up |= (1 << slot);
 }
 
 static void disable_device(struct pci_status *p, struct gpe_regs *g, int slot)
 {
     g->sts |= 2;
-    g->en |= 2;
     p->down |= (1 << slot);
 }
 
 void qemu_system_device_hot_add(int pcibus, int slot, int state)
 {
-    qemu_set_irq(pm_state->irq, 1);
     pci0_status.up = 0;
     pci0_status.down = 0;
     if (state)
         enable_device(&pci0_status, &gpe, slot);
     else
         disable_device(&pci0_status, &gpe, slot);
-    qemu_set_irq(pm_state->irq, 0);
+
+    if (gpe.en & 2) {
+        qemu_set_irq(pm_state->irq, 1);
+        qemu_set_irq(pm_state->irq, 0);
+    }
 }

Re: [PATCH 1/3] Fix GPE registers read/write handling.

[Avi Kivity]

Gleb Natapov wrote:
> For STS register bit are cleared by writing 1 into it.
>
>  
> +static uint16_t gpe_write_val(uint16_t cur, int addr, uint32_t val)
> +{
> +    if (addr & 1)
> +        return (cur & 0xff) | (val << 8);
> +    return (cur & 0xff00) | (val & 0xff);
> +}
> +
> +static uint16_t 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;
> +
> +    return (cur & (0xff << (8 - shift))) | (x1 << shift);
> +}
>   

It's strange that write_val() and reset_val() return a value.  Won't it 
be cleaner to make val a pointer?


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/3] Fix GPE registers read/write handling.

[Gleb Natapov]

On Wed, Feb 04, 2009 at 02:29:07PM +0200, Avi Kivity wrote:
> Gleb Natapov wrote:
>> For STS register bit are cleared by writing 1 into it.
>>
>>  +static uint16_t gpe_write_val(uint16_t cur, int addr, uint32_t val)
>> +{
>> +    if (addr & 1)
>> +        return (cur & 0xff) | (val << 8);
>> +    return (cur & 0xff00) | (val & 0xff);
>> +}
>> +
>> +static uint16_t 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;
>> +
>> +    return (cur & (0xff << (8 - shift))) | (x1 << shift);
>> +}
>>   
>
> It's strange that write_val() and reset_val() return a value.  Won't it  
> be cleaner to make val a pointer?
>
>From functional programing POV no, it is not cleaner. But I don't really
care where to do assignment.

--
			Gleb.

Re: [PATCH 1/3] Fix GPE registers read/write handling.

[Avi Kivity]

Gleb Natapov wrote:

  

>> It's strange that write_val() and reset_val() return a value.  Won't it  
>> be cleaner to make val a pointer?
>>
>>     
> From functional programing POV no, it is not cleaner. But I don't really
> care where to do assignment.
>   

We're not doing functional programming:

   a = f(a, x)

is just duplication, but still has an assignment.

-- 
error compiling committee.c: too many arguments to function