| Message ID | 1390573387-15386-1-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | Accepted |
| Commit | 5224a733d3bd4d0db3548712047506c50487085e |
| Headers | show |
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); >
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); > > > >
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.
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 >
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.
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.
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);
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(-)