[PATCH] xen: arm: correct use of find_next_bit

[PATCH] xen: arm: correct use of find_next_bit

[Ian Campbell]

find_next_bit takes a "const unsigned long *" but forcing a cast of an
"uint32_t *" throws away the alignment constraints and ends up causing an
alignment fault on arm64 if the input happened to be 4 but not 8 byte aligned.

Instead of casting use a temporary variable of the right type.

I've had a look around for similar constructs and the only thing I found was
maintenance_interrupt which cases a uint64_t down to an unsigned long, which
although perhaps not best advised is safe I think.

This was observed with the AArch64 Linaro toolchain 2013.12 but I think that
is just coincidental due to subtle changes to the stack layout etc.

Reported-by: Fu Wei <fu.wei@linaro.org>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/vgic.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 90e9707..553411d 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -362,11 +362,12 @@ read_as_zero:
 
 static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
 {
+    const unsigned long mask = r;
     struct pending_irq *p;
     unsigned int irq;
     int i = 0;
 
-    while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) {
+    while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
         irq = i + (32 * n);
         p = irq_to_pending(v, irq);
         clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
@@ -379,11 +380,12 @@ static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
 
 static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
 {
+    const unsigned long mask = r;
     struct pending_irq *p;
     unsigned int irq;
     int i = 0;
 
-    while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) {
+    while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
         irq = i + (32 * n);
         p = irq_to_pending(v, irq);
         set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
-- 
1.7.10.4

Re: [PATCH] xen: arm: correct use of find_next_bit

[Julien Grall]

On 01/24/2014 02:23 PM, Ian Campbell wrote:
> find_next_bit takes a "const unsigned long *" but forcing a cast of an
> "uint32_t *" throws away the alignment constraints and ends up causing an
> alignment fault on arm64 if the input happened to be 4 but not 8 byte aligned.
> 
> Instead of casting use a temporary variable of the right type.
> 
> I've had a look around for similar constructs and the only thing I found was
> maintenance_interrupt which cases a uint64_t down to an unsigned long, which
> although perhaps not best advised is safe I think.
> 
> This was observed with the AArch64 Linaro toolchain 2013.12 but I think that
> is just coincidental due to subtle changes to the stack layout etc.
> 
> Reported-by: Fu Wei <fu.wei@linaro.org>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Good catch! Do you plan to apply this patch for Xen 4.4?

Acked-by: Julien Grall <julien.grall@linaro.org>

> ---
>  xen/arch/arm/vgic.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 90e9707..553411d 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -362,11 +362,12 @@ read_as_zero:
>  
>  static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
>  {
> +    const unsigned long mask = r;
>      struct pending_irq *p;
>      unsigned int irq;
>      int i = 0;
>  
> -    while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) {
> +    while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>          irq = i + (32 * n);
>          p = irq_to_pending(v, irq);
>          clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> @@ -379,11 +380,12 @@ static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
>  
>  static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>  {
> +    const unsigned long mask = r;
>      struct pending_irq *p;
>      unsigned int irq;
>      int i = 0;
>  
> -    while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) {
> +    while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>          irq = i + (32 * n);
>          p = irq_to_pending(v, irq);
>          set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> 


-- 
Julien Grall

Re: [PATCH] xen: arm: correct use of find_next_bit

[Jan Beulich]

>>> On 24.01.14 at 15:23, Ian Campbell <ian.campbell@citrix.com> wrote:
> find_next_bit takes a "const unsigned long *" but forcing a cast of an
> "uint32_t *" throws away the alignment constraints and ends up causing an
> alignment fault on arm64 if the input happened to be 4 but not 8 byte 
> aligned.
> 
> Instead of casting use a temporary variable of the right type.
> 
> I've had a look around for similar constructs and the only thing I found was
> maintenance_interrupt which cases a uint64_t down to an unsigned long, which
> although perhaps not best advised is safe I think.
> 
> This was observed with the AArch64 Linaro toolchain 2013.12 but I think that
> is just coincidental due to subtle changes to the stack layout etc.
> 
> Reported-by: Fu Wei <fu.wei@linaro.org>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/vgic.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 90e9707..553411d 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -362,11 +362,12 @@ read_as_zero:
>  
>  static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
>  {
> +    const unsigned long mask = r;

Why don't you just change the type of "r" to "unsigned long"?

Jan

>      struct pending_irq *p;
>      unsigned int irq;
>      int i = 0;
>  
> -    while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) 
> {
> +    while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>          irq = i + (32 * n);
>          p = irq_to_pending(v, irq);
>          clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> @@ -379,11 +380,12 @@ static void vgic_disable_irqs(struct vcpu *v, uint32_t 
> r, int n)
>  
>  static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>  {
> +    const unsigned long mask = r;
>      struct pending_irq *p;
>      unsigned int irq;
>      int i = 0;
>  
> -    while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) 
> {
> +    while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>          irq = i + (32 * n);
>          p = irq_to_pending(v, irq);
>          set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [PATCH] xen: arm: correct use of find_next_bit

[Ian Campbell]

On Fri, 2014-01-24 at 14:36 +0000, Julien Grall wrote:
> On 01/24/2014 02:23 PM, Ian Campbell wrote:
> > find_next_bit takes a "const unsigned long *" but forcing a cast of an
> > "uint32_t *" throws away the alignment constraints and ends up causing an
> > alignment fault on arm64 if the input happened to be 4 but not 8 byte aligned.
> > 
> > Instead of casting use a temporary variable of the right type.
> > 
> > I've had a look around for similar constructs and the only thing I found was
> > maintenance_interrupt which cases a uint64_t down to an unsigned long, which
> > although perhaps not best advised is safe I think.
> > 
> > This was observed with the AArch64 Linaro toolchain 2013.12 but I think that
> > is just coincidental due to subtle changes to the stack layout etc.
> > 
> > Reported-by: Fu Wei <fu.wei@linaro.org>
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Good catch! Do you plan to apply this patch for Xen 4.4?

Yes, I think it is a suitable bug fix.

> 
> Acked-by: Julien Grall <julien.grall@linaro.org>
> 
> > ---
> >  xen/arch/arm/vgic.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index 90e9707..553411d 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -362,11 +362,12 @@ read_as_zero:
> >  
> >  static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
> >  {
> > +    const unsigned long mask = r;
> >      struct pending_irq *p;
> >      unsigned int irq;
> >      int i = 0;
> >  
> > -    while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) {
> > +    while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> >          irq = i + (32 * n);
> >          p = irq_to_pending(v, irq);
> >          clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> > @@ -379,11 +380,12 @@ static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
> >  
> >  static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
> >  {
> > +    const unsigned long mask = r;
> >      struct pending_irq *p;
> >      unsigned int irq;
> >      int i = 0;
> >  
> > -    while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) {
> > +    while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> >          irq = i + (32 * n);
> >          p = irq_to_pending(v, irq);
> >          set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> > 
> 
> 

Re: [PATCH] xen: arm: correct use of find_next_bit

[Ian Campbell]

On Fri, 2014-01-24 at 14:40 +0000, Jan Beulich wrote:
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index 90e9707..553411d 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -362,11 +362,12 @@ read_as_zero:
> >  
> >  static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
> >  {
> > +    const unsigned long mask = r;
> 
> Why don't you just change the type of "r" to "unsigned long"?

The MMIO register which this function is emulating is a 32-bit register,
so I preferred to keep this wrinkle confined to the internals.

Ian.

Re: [PATCH] xen: arm: correct use of find_next_bit

[Stefano Stabellini]

On Fri, 24 Jan 2014, Ian Campbell wrote:
> find_next_bit takes a "const unsigned long *" but forcing a cast of an
> "uint32_t *" throws away the alignment constraints and ends up causing an
> alignment fault on arm64 if the input happened to be 4 but not 8 byte aligned.

I am not opposed to this patch, but for the sake of clarity, isn't the
alignment of (uint32_t*) and (const unsigned long*) the same? It should
be 8 bytes in both cases on ARM64.

It seems to me that the problem is not the cast to (const unsigned
long*), but the usage of &r: maybe the tools aren't able to covert &r to
a properly aligned pointer?

Am I getting this wrong?



> Instead of casting use a temporary variable of the right type.
> 
> I've had a look around for similar constructs and the only thing I found was
> maintenance_interrupt which cases a uint64_t down to an unsigned long, which
> although perhaps not best advised is safe I think.
> 
> This was observed with the AArch64 Linaro toolchain 2013.12 but I think that
> is just coincidental due to subtle changes to the stack layout etc.
> 
> Reported-by: Fu Wei <fu.wei@linaro.org>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/vgic.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 90e9707..553411d 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -362,11 +362,12 @@ read_as_zero:
>  
>  static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
>  {
> +    const unsigned long mask = r;
>      struct pending_irq *p;
>      unsigned int irq;
>      int i = 0;
>  
> -    while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) {
> +    while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>          irq = i + (32 * n);
>          p = irq_to_pending(v, irq);
>          clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> @@ -379,11 +380,12 @@ static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
>  
>  static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>  {
> +    const unsigned long mask = r;
>      struct pending_irq *p;
>      unsigned int irq;
>      int i = 0;
>  
> -    while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) {
> +    while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>          irq = i + (32 * n);
>          p = irq_to_pending(v, irq);
>          set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> -- 
> 1.7.10.4
> 

Re: [PATCH] xen: arm: correct use of find_next_bit

[Ian Campbell]

On Mon, 2014-01-27 at 12:33 +0000, Stefano Stabellini wrote:
> On Fri, 24 Jan 2014, Ian Campbell wrote:
> > find_next_bit takes a "const unsigned long *" but forcing a cast of an
> > "uint32_t *" throws away the alignment constraints and ends up causing an
> > alignment fault on arm64 if the input happened to be 4 but not 8 byte aligned.
> 
> I am not opposed to this patch, but for the sake of clarity, isn't the
> alignment of (uint32_t*) and (const unsigned long*) the same? It should
> be 8 bytes in both cases on ARM64.

The target of a uint32_t * only needs to be 4 byte aligned on arm64,
since that is the natural alignment of a 32-bit type.

> It seems to me that the problem is not the cast to (const unsigned
> long*), but the usage of &r: maybe the tools aren't able to covert &r to
> a properly aligned pointer?

No, &r behaves exactly as expected and returns the address of r,
whatever the alignment of r is.

> Am I getting this wrong?

I think so, yes.

Ian.

Re: [PATCH] xen: arm: correct use of find_next_bit

[Ian Campbell]

On Fri, 2014-01-24 at 14:48 +0000, Ian Campbell wrote:
> On Fri, 2014-01-24 at 14:36 +0000, Julien Grall wrote:
> > On 01/24/2014 02:23 PM, Ian Campbell wrote:
> > > find_next_bit takes a "const unsigned long *" but forcing a cast of an
> > > "uint32_t *" throws away the alignment constraints and ends up causing an
> > > alignment fault on arm64 if the input happened to be 4 but not 8 byte aligned.
> > > 
> > > Instead of casting use a temporary variable of the right type.
> > > 
> > > I've had a look around for similar constructs and the only thing I found was
> > > maintenance_interrupt which cases a uint64_t down to an unsigned long, which
> > > although perhaps not best advised is safe I think.
> > > 
> > > This was observed with the AArch64 Linaro toolchain 2013.12 but I think that
> > > is just coincidental due to subtle changes to the stack layout etc.
> > > 
> > > Reported-by: Fu Wei <fu.wei@linaro.org>
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > Good catch! Do you plan to apply this patch for Xen 4.4?
> 
> Yes, I think it is a suitable bug fix.

Applied, thanks.