[PATCH for 4.6] xen/arm: vgic: Correctly emulate write when byte is used

[PATCH for 4.6] xen/arm: vgic: Correctly emulate write when byte is used

[Julien Grall]

When a guest is writing a byte, the value will be located in bits[7:0]
of the register.

Although the current implementation is expecting the byte at the Nth
byte of the register where N = address & 4;

When the address is not 4-byte aligned, the corresponding byte in the
internal state will always be set to zero rather.

Note that byte access are only used for GICD_IPRIORITYR and
GICD_ITARGETSR. So the worst things that could happen is not setting the
priority correctly and ignore the target vCPU written.

Signed-off-by: Julien Grall <julien.grall@citrix.com>

---

This patch is a candidate for Xen 4.6 and backport to Xen 4.5 and Xen
4.4. Without it, write a byte to a register won't work as expected.

Spotted while doing some test on the vGICv2 driver. FWIW, Linux doesn't
use byte access in both GICv3 and GICv2 driver.

Note that Xen 4.4 will require a different patch because the function
was living in vgic.c at that time.
---
 xen/include/asm-arm/vgic.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 41cadb1..96839f0 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -174,10 +174,10 @@ static inline void vgic_byte_write(uint32_t *reg, uint32_t var, int offset)
 {
     int byte = offset & 0x3;
 
-    var &= (0xff << (8*byte));
+    var &= 0xff;
 
     *reg &= ~(0xff << (8*byte));
-    *reg |= var;
+    *reg |= (var << (8*byte));
 }
 
 enum gic_sgi_mode;
-- 
2.1.4

Re: [PATCH for 4.6] xen/arm: vgic: Correctly emulate write when byte is used

[Wei Liu]

On Tue, Sep 22, 2015 at 09:18:48PM +0100, Julien Grall wrote:
> When a guest is writing a byte, the value will be located in bits[7:0]
> of the register.
> 
> Although the current implementation is expecting the byte at the Nth
> byte of the register where N = address & 4;
> 
> When the address is not 4-byte aligned, the corresponding byte in the
> internal state will always be set to zero rather.
> 
> Note that byte access are only used for GICD_IPRIORITYR and
> GICD_ITARGETSR. So the worst things that could happen is not setting the
> priority correctly and ignore the target vCPU written.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 

Subject to an ack / review from ARM folks:

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
> 
> This patch is a candidate for Xen 4.6 and backport to Xen 4.5 and Xen
> 4.4. Without it, write a byte to a register won't work as expected.
> 
> Spotted while doing some test on the vGICv2 driver. FWIW, Linux doesn't
> use byte access in both GICv3 and GICv2 driver.
> 
> Note that Xen 4.4 will require a different patch because the function
> was living in vgic.c at that time.
> ---
>  xen/include/asm-arm/vgic.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 41cadb1..96839f0 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -174,10 +174,10 @@ static inline void vgic_byte_write(uint32_t *reg, uint32_t var, int offset)
>  {
>      int byte = offset & 0x3;
>  
> -    var &= (0xff << (8*byte));
> +    var &= 0xff;
>  
>      *reg &= ~(0xff << (8*byte));
> -    *reg |= var;
> +    *reg |= (var << (8*byte));
>  }
>  
>  enum gic_sgi_mode;
> -- 
> 2.1.4

Re: [PATCH for 4.6] xen/arm: vgic: Correctly emulate write when byte is used

[Ian Campbell]

On Wed, 2015-09-23 at 14:23 +0100, Wei Liu wrote:
> On Tue, Sep 22, 2015 at 09:18:48PM +0100, Julien Grall wrote:
> > When a guest is writing a byte, the value will be located in bits[7:0]
> > of the register.
> > 
> > Although the current implementation is expecting the byte at the Nth
> > byte of the register where N = address & 4;
> > 
> > When the address is not 4-byte aligned, the corresponding byte in the
> > internal state will always be set to zero rather.
> > 
> > Note that byte access are only used for GICD_IPRIORITYR and
> > GICD_ITARGETSR. So the worst things that could happen is not setting
> > the
> > priority correctly and ignore the target vCPU written.
> > 
> > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > 
> 
> Subject to an ack / review from ARM folks:
> 
> Release-acked-by: Wei Liu <wei.liu2@citrix.com>

Acked + applied to staging and staging-4.6, noted for later backport to 4.5
and 4.4.