* [PATCH] xen/arm64: disable alignment check @ 2014-04-27 9:10 Vladimir Murzin 2014-04-28 9:48 ` Ian Campbell 0 siblings, 1 reply; 17+ messages in thread From: Vladimir Murzin @ 2014-04-27 9:10 UTC (permalink / raw) To: xen-devel; +Cc: Vladimir Murzin, Ian.Campbell Alignment check is enabled by default at Xen boot. This leads to: (XEN) Hypervisor Trap. HSR=0x96000021 EC=0x25 IL=1 Syndrome=21 (XEN) CPU0: Unexpected Trap: Hypervisor (XEN) ----[ Xen-4.4.0 arm64 debug=n Not tainted ]---- (XEN) CPU: 0 (XEN) PC: 000000000020a0e8 evtchn_fifo_init+0x3c/0x88 (XEN) LR: 00000000002096b4 (XEN) SP: 000080007fddfd20 (XEN) CPSR: 00000349 MODE:64-bit EL2h (Hypervisor, handler) (XEN) X0: 0000000000000000 X1: 000080000800a060 X2: 000000000000000c (XEN) X3: 000080003dce5000 X4: 000080000800b000 X5: 0000000000000060 (XEN) X6: 000080000800a000 X7: ffffffffffffffff X8: 000000000000001d (XEN) X9: 1999999999999999 X10: 0101010101010101 X11: 0000000000000020 (XEN) X12: 0000000000000007 X13: 0000000000000080 X14: 0000000000000000 (XEN) X15: 0000007fa3b5e598 X16: 0000000000000020 X17: 0000007fa3b90420 (XEN) X18: 0000007fdc8e50b0 X19: 000080000800b000 X20: ffffffc01cf5fdd0 (XEN) X21: 000080007fddfdb8 X22: 00000000002dd000 X23: 000080000800b000 (XEN) X24: 000080000800b000 X25: 0000000000000003 X26: 000080000800b0b8 (XEN) X27: 000080000800a020 X28: 000080000800a060 FP: ffffffc01cf5fda0 (XEN) (XEN) VTCR_EL2: 80022558 (XEN) VTTBR_EL2: 0001000088014000 (XEN) (XEN) SCTLR_EL2: 30cd183f (XEN) HCR_EL2: 0000000080282835 (XEN) TTBR0_EL2: 00000000ffeca000 (XEN) (XEN) ESR_EL2: 96000021 (XEN) HPFAR_EL2: 0000000000000000 (XEN) FAR_EL2: 000080003dce500c (XEN) (XEN) Xen stack trace from sp=000080007fddfd20: (XEN) 0000000000000001 00000000002096b4 000080007fddfdb8 0000000000208ae8 (XEN) 000080007fddfeb0 000080007fddfeb0 ffffffffffffffff ffffffc000093578 (XEN) 0000000080000145 0000000000000015 0000000000000114 000000000000001d (XEN) ffffffc00061f000 ffffffc01cf5c000 000000000023e3f0 000080007fd32000 (XEN) 000000064cbe0760 00000000002dd938 000000000021e000 000000011dcd0000 (XEN) 000000001d966440 00000000002a0800 000000005a000ea1 000000000023f1f0 (XEN) 00000000002d8458 00000000002a3b00 0000000000084501 ffffffc01c045540 (XEN) ffffffffffffffff ffffffc000093578 0000000080000145 0000000000000015 (XEN) 0000000000000114 000000000000001d ffffffc00061f000 ffffffc01cf5c000 (XEN) 0000000000241acc 0000000000000000 ffffffc01d96a8e8 ffffffc01cf67da0 (XEN) 00000000000004bc 0000000000000007 0000000000239a24 0000000000000000 (XEN) 000000000023f47c 0000000000000100 0000800008010f30 0000000000000100 (XEN) ffffffc01cf5c000 0000000000241b84 0000000000000000 ffffffc01cf5fdd0 (XEN) 0000000000000001 0000000100000000 0000007fdc8e5328 0000000000004000 (XEN) 0000000000000000 ffffffffffffffff 000000000000001d 1999999999999999 (XEN) 0101010101010101 0000000000000020 0000000000000007 0000000000000080 (XEN) 0000000000000000 0000007fa3b5e598 0000000000000020 0000007fa3b90420 (XEN) 0000007fdc8e50b0 0000000000084501 ffffffc01c045540 0000007fdc8e5320 (XEN) ffffffc01df35858 0000007fdc8e5320 0000000000000015 0000000000000114 (XEN) 000000000000001d ffffffc00061f000 ffffffc01cf5c000 ffffffc01cf5fda0 (XEN) ffffffc0002b7a8c ffffffffffffffff ffffffc000093578 0000000080000145 (XEN) 0000000080000000 0000000000000000 0000000000000000 0000007fdc8e5310 (XEN) ffffffc01cf5fda0 0000007fa3abf1fc cfdfdfdfdfdfdfcf cfdfdfdfdfdfdfcf (XEN) Xen call trace: (XEN) [<000000000020a0e8>] evtchn_fifo_init+0x3c/0x88 (PC) (XEN) [<00000000002096b4>] do_event_channel_op+0xd6c/0xe54 (LR) (XEN) Faulting instruction is: 20a0e8: f8626860 ldr x0, [x3,x2] Disabling alignment check has no effect on exclusive load/store instructions. --- xen/arch/arm/arm64/head.S | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 31afdd0..0599bf9 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -239,9 +239,9 @@ skip_bss: * Write-implies-XN disabled (for now), * D-cache disabled (for now), * I-cache enabled, - * Alignment checking enabled, + * Alignment checking disabled, * MMU translation disabled (for now). */ - ldr x0, =(HSCTLR_BASE|SCTLR_A) + ldr x0, =(HSCTLR_BASE) msr SCTLR_EL2, x0 /* Rebuild the boot pagetable's first-level entries. The structure -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] xen/arm64: disable alignment check 2014-04-27 9:10 [PATCH] xen/arm64: disable alignment check Vladimir Murzin @ 2014-04-28 9:48 ` Ian Campbell 2014-04-28 10:24 ` David Vrabel 0 siblings, 1 reply; 17+ messages in thread From: Ian Campbell @ 2014-04-28 9:48 UTC (permalink / raw) To: Vladimir Murzin; +Cc: xen-devel, David Vrabel On Sun, 2014-04-27 at 10:10 +0100, Vladimir Murzin wrote: > Alignment check is enabled by default at Xen boot. This has already been disabled in the development branch via: commit 58bbe7d71239db508c30099bf7b6db7c458f3336 Author: Ian Campbell <ian.campbell@citrix.com> Date: Wed Mar 26 13:38:45 2014 +0000 xen: arm64: disable alignment traps The mem* primitives which I am about to import from Linux in a subsequent patch rely on the hardware handling misalignment. The benefits of an optimised memcpy etc outweigh the downsides. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Acked-by: Julien Grall <julien.grall@linaro.org> Acked-by: Tim Deegan <tim@xen.org> I will consider this for backport, but first I'd like to consider whether we shouldn't fix the hypervisor side evtchn FIFO code along the same lines as the kernel side. David, any thoughts? Is the problematic "ldr x0, [x3,x2]" instruction coming from the compiler? I don't think it is coming from xen/arch/arm/arm64/lib/bitops.S via the call to setbits so I expect the answer is yes (if so then disabling alignment traps seems to be the correct response, if it were lib/*.S I'd consider fixing those instead). Ian. > This leads to: > > (XEN) Hypervisor Trap. HSR=0x96000021 EC=0x25 IL=1 Syndrome=21 > (XEN) CPU0: Unexpected Trap: Hypervisor > (XEN) ----[ Xen-4.4.0 arm64 debug=n Not tainted ]---- > (XEN) CPU: 0 > (XEN) PC: 000000000020a0e8 evtchn_fifo_init+0x3c/0x88 > (XEN) LR: 00000000002096b4 > (XEN) SP: 000080007fddfd20 > (XEN) CPSR: 00000349 MODE:64-bit EL2h (Hypervisor, handler) > (XEN) X0: 0000000000000000 X1: 000080000800a060 X2: 000000000000000c > (XEN) X3: 000080003dce5000 X4: 000080000800b000 X5: 0000000000000060 > (XEN) X6: 000080000800a000 X7: ffffffffffffffff X8: 000000000000001d > (XEN) X9: 1999999999999999 X10: 0101010101010101 X11: 0000000000000020 > (XEN) X12: 0000000000000007 X13: 0000000000000080 X14: 0000000000000000 > (XEN) X15: 0000007fa3b5e598 X16: 0000000000000020 X17: 0000007fa3b90420 > (XEN) X18: 0000007fdc8e50b0 X19: 000080000800b000 X20: ffffffc01cf5fdd0 > (XEN) X21: 000080007fddfdb8 X22: 00000000002dd000 X23: 000080000800b000 > (XEN) X24: 000080000800b000 X25: 0000000000000003 X26: 000080000800b0b8 > (XEN) X27: 000080000800a020 X28: 000080000800a060 FP: ffffffc01cf5fda0 > (XEN) > (XEN) VTCR_EL2: 80022558 > (XEN) VTTBR_EL2: 0001000088014000 > (XEN) > (XEN) SCTLR_EL2: 30cd183f > (XEN) HCR_EL2: 0000000080282835 > (XEN) TTBR0_EL2: 00000000ffeca000 > (XEN) > (XEN) ESR_EL2: 96000021 > (XEN) HPFAR_EL2: 0000000000000000 > (XEN) FAR_EL2: 000080003dce500c > (XEN) > (XEN) Xen stack trace from sp=000080007fddfd20: > (XEN) 0000000000000001 00000000002096b4 000080007fddfdb8 0000000000208ae8 > (XEN) 000080007fddfeb0 000080007fddfeb0 ffffffffffffffff ffffffc000093578 > (XEN) 0000000080000145 0000000000000015 0000000000000114 000000000000001d > (XEN) ffffffc00061f000 ffffffc01cf5c000 000000000023e3f0 000080007fd32000 > (XEN) 000000064cbe0760 00000000002dd938 000000000021e000 000000011dcd0000 > (XEN) 000000001d966440 00000000002a0800 000000005a000ea1 000000000023f1f0 > (XEN) 00000000002d8458 00000000002a3b00 0000000000084501 ffffffc01c045540 > (XEN) ffffffffffffffff ffffffc000093578 0000000080000145 0000000000000015 > (XEN) 0000000000000114 000000000000001d ffffffc00061f000 ffffffc01cf5c000 > (XEN) 0000000000241acc 0000000000000000 ffffffc01d96a8e8 ffffffc01cf67da0 > (XEN) 00000000000004bc 0000000000000007 0000000000239a24 0000000000000000 > (XEN) 000000000023f47c 0000000000000100 0000800008010f30 0000000000000100 > (XEN) ffffffc01cf5c000 0000000000241b84 0000000000000000 ffffffc01cf5fdd0 > (XEN) 0000000000000001 0000000100000000 0000007fdc8e5328 0000000000004000 > (XEN) 0000000000000000 ffffffffffffffff 000000000000001d 1999999999999999 > (XEN) 0101010101010101 0000000000000020 0000000000000007 0000000000000080 > (XEN) 0000000000000000 0000007fa3b5e598 0000000000000020 0000007fa3b90420 > (XEN) 0000007fdc8e50b0 0000000000084501 ffffffc01c045540 0000007fdc8e5320 > (XEN) ffffffc01df35858 0000007fdc8e5320 0000000000000015 0000000000000114 > (XEN) 000000000000001d ffffffc00061f000 ffffffc01cf5c000 ffffffc01cf5fda0 > (XEN) ffffffc0002b7a8c ffffffffffffffff ffffffc000093578 0000000080000145 > (XEN) 0000000080000000 0000000000000000 0000000000000000 0000007fdc8e5310 > (XEN) ffffffc01cf5fda0 0000007fa3abf1fc cfdfdfdfdfdfdfcf cfdfdfdfdfdfdfcf > (XEN) Xen call trace: > (XEN) [<000000000020a0e8>] evtchn_fifo_init+0x3c/0x88 (PC) > (XEN) [<00000000002096b4>] do_event_channel_op+0xd6c/0xe54 (LR) > (XEN) > > Faulting instruction is: > 20a0e8: f8626860 ldr x0, [x3,x2] > > Disabling alignment check has no effect on exclusive load/store instructions. > --- > xen/arch/arm/arm64/head.S | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 31afdd0..0599bf9 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -239,9 +239,9 @@ skip_bss: > * Write-implies-XN disabled (for now), > * D-cache disabled (for now), > * I-cache enabled, > - * Alignment checking enabled, > + * Alignment checking disabled, > * MMU translation disabled (for now). */ > - ldr x0, =(HSCTLR_BASE|SCTLR_A) > + ldr x0, =(HSCTLR_BASE) > msr SCTLR_EL2, x0 > > /* Rebuild the boot pagetable's first-level entries. The structure ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] xen/arm64: disable alignment check 2014-04-28 9:48 ` Ian Campbell @ 2014-04-28 10:24 ` David Vrabel 2014-04-28 10:36 ` Ian Campbell 0 siblings, 1 reply; 17+ messages in thread From: David Vrabel @ 2014-04-28 10:24 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel, Vladimir Murzin, Jan Beulich On 28/04/14 10:48, Ian Campbell wrote: > On Sun, 2014-04-27 at 10:10 +0100, Vladimir Murzin wrote: >> Alignment check is enabled by default at Xen boot. > > This has already been disabled in the development branch via: > commit 58bbe7d71239db508c30099bf7b6db7c458f3336 > Author: Ian Campbell <ian.campbell@citrix.com> > Date: Wed Mar 26 13:38:45 2014 +0000 > > xen: arm64: disable alignment traps > > The mem* primitives which I am about to import from Linux in a subsequent > patch rely on the hardware handling misalignment. > > The benefits of an optimised memcpy etc outweigh the downsides. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > Acked-by: Julien Grall <julien.grall@linaro.org> > Acked-by: Tim Deegan <tim@xen.org> > > I will consider this for backport, but first I'd like to consider > whether we shouldn't fix the hypervisor side evtchn FIFO code along the > same lines as the kernel side. David, any thoughts? I believe Jan suggested making Xen's bitops handle 32-bit alignment or adding a new set of 32-bit bitops. David ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] xen/arm64: disable alignment check 2014-04-28 10:24 ` David Vrabel @ 2014-04-28 10:36 ` Ian Campbell 2014-04-28 10:37 ` David Vrabel 0 siblings, 1 reply; 17+ messages in thread From: Ian Campbell @ 2014-04-28 10:36 UTC (permalink / raw) To: David Vrabel; +Cc: xen-devel, Vladimir Murzin, Jan Beulich On Mon, 2014-04-28 at 11:24 +0100, David Vrabel wrote: > On 28/04/14 10:48, Ian Campbell wrote: > > On Sun, 2014-04-27 at 10:10 +0100, Vladimir Murzin wrote: > >> Alignment check is enabled by default at Xen boot. > > > > This has already been disabled in the development branch via: > > commit 58bbe7d71239db508c30099bf7b6db7c458f3336 > > Author: Ian Campbell <ian.campbell@citrix.com> > > Date: Wed Mar 26 13:38:45 2014 +0000 > > > > xen: arm64: disable alignment traps > > > > The mem* primitives which I am about to import from Linux in a subsequent > > patch rely on the hardware handling misalignment. > > > > The benefits of an optimised memcpy etc outweigh the downsides. > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > Acked-by: Julien Grall <julien.grall@linaro.org> > > Acked-by: Tim Deegan <tim@xen.org> > > > > I will consider this for backport, but first I'd like to consider > > whether we shouldn't fix the hypervisor side evtchn FIFO code along the > > same lines as the kernel side. David, any thoughts? > > I believe Jan suggested making Xen's bitops handle 32-bit alignment or > adding a new set of 32-bit bitops. This is already the case for the arm64 bitops, we deliberately diverged from Linux here because there are a bunch of other "misaligned" 4-byte bitmasks (one in the malloc implementation springs to mind). Ian. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] xen/arm64: disable alignment check 2014-04-28 10:36 ` Ian Campbell @ 2014-04-28 10:37 ` David Vrabel 2014-04-28 10:43 ` Ian Campbell 0 siblings, 1 reply; 17+ messages in thread From: David Vrabel @ 2014-04-28 10:37 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel, Vladimir Murzin, Jan Beulich On 28/04/14 11:36, Ian Campbell wrote: > On Mon, 2014-04-28 at 11:24 +0100, David Vrabel wrote: >> On 28/04/14 10:48, Ian Campbell wrote: >>> On Sun, 2014-04-27 at 10:10 +0100, Vladimir Murzin wrote: >>>> Alignment check is enabled by default at Xen boot. >>> >>> This has already been disabled in the development branch via: >>> commit 58bbe7d71239db508c30099bf7b6db7c458f3336 >>> Author: Ian Campbell <ian.campbell@citrix.com> >>> Date: Wed Mar 26 13:38:45 2014 +0000 >>> >>> xen: arm64: disable alignment traps >>> >>> The mem* primitives which I am about to import from Linux in a subsequent >>> patch rely on the hardware handling misalignment. >>> >>> The benefits of an optimised memcpy etc outweigh the downsides. >>> >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >>> Acked-by: Julien Grall <julien.grall@linaro.org> >>> Acked-by: Tim Deegan <tim@xen.org> >>> >>> I will consider this for backport, but first I'd like to consider >>> whether we shouldn't fix the hypervisor side evtchn FIFO code along the >>> same lines as the kernel side. David, any thoughts? >> >> I believe Jan suggested making Xen's bitops handle 32-bit alignment or >> adding a new set of 32-bit bitops. > > This is already the case for the arm64 bitops, we deliberately diverged > from Linux here because there are a bunch of other "misaligned" 4-byte > bitmasks (one in the malloc implementation springs to mind). Then I'm not sure what it is you're trying to fix in Xen? David ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] xen/arm64: disable alignment check 2014-04-28 10:37 ` David Vrabel @ 2014-04-28 10:43 ` Ian Campbell 2014-04-29 7:38 ` Vladimir Murzin 0 siblings, 1 reply; 17+ messages in thread From: Ian Campbell @ 2014-04-28 10:43 UTC (permalink / raw) To: David Vrabel; +Cc: xen-devel, Vladimir Murzin, Jan Beulich On Mon, 2014-04-28 at 11:37 +0100, David Vrabel wrote: > On 28/04/14 11:36, Ian Campbell wrote: > > On Mon, 2014-04-28 at 11:24 +0100, David Vrabel wrote: > >> On 28/04/14 10:48, Ian Campbell wrote: > >>> On Sun, 2014-04-27 at 10:10 +0100, Vladimir Murzin wrote: > >>>> Alignment check is enabled by default at Xen boot. > >>> > >>> This has already been disabled in the development branch via: > >>> commit 58bbe7d71239db508c30099bf7b6db7c458f3336 > >>> Author: Ian Campbell <ian.campbell@citrix.com> > >>> Date: Wed Mar 26 13:38:45 2014 +0000 > >>> > >>> xen: arm64: disable alignment traps > >>> > >>> The mem* primitives which I am about to import from Linux in a subsequent > >>> patch rely on the hardware handling misalignment. > >>> > >>> The benefits of an optimised memcpy etc outweigh the downsides. > >>> > >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > >>> Acked-by: Julien Grall <julien.grall@linaro.org> > >>> Acked-by: Tim Deegan <tim@xen.org> > >>> > >>> I will consider this for backport, but first I'd like to consider > >>> whether we shouldn't fix the hypervisor side evtchn FIFO code along the > >>> same lines as the kernel side. David, any thoughts? > >> > >> I believe Jan suggested making Xen's bitops handle 32-bit alignment or > >> adding a new set of 32-bit bitops. > > > > This is already the case for the arm64 bitops, we deliberately diverged > > from Linux here because there are a bunch of other "misaligned" 4-byte > > bitmasks (one in the malloc implementation springs to mind). > > Then I'm not sure what it is you're trying to fix in Xen? I partially hust wanted to consider whether anything should be better changed than rely on the bitops being more flexible, for some reason. But I also wanted confirmation that the problematic instruction was generated by gcc and not by some handcoded asm somewhere which we hadn't properly fixed. Ian. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] xen/arm64: disable alignment check 2014-04-28 10:43 ` Ian Campbell @ 2014-04-29 7:38 ` Vladimir Murzin 2014-04-29 8:58 ` Ian Campbell 0 siblings, 1 reply; 17+ messages in thread From: Vladimir Murzin @ 2014-04-29 7:38 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel@lists.xenproject.org, David Vrabel, Jan Beulich On Mon, Apr 28, 2014 at 11:43 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Mon, 2014-04-28 at 11:37 +0100, David Vrabel wrote: >> On 28/04/14 11:36, Ian Campbell wrote: >> > On Mon, 2014-04-28 at 11:24 +0100, David Vrabel wrote: >> >> On 28/04/14 10:48, Ian Campbell wrote: >> >>> On Sun, 2014-04-27 at 10:10 +0100, Vladimir Murzin wrote: >> >>>> Alignment check is enabled by default at Xen boot. >> >>> >> >>> This has already been disabled in the development branch via: >> >>> commit 58bbe7d71239db508c30099bf7b6db7c458f3336 >> >>> Author: Ian Campbell <ian.campbell@citrix.com> >> >>> Date: Wed Mar 26 13:38:45 2014 +0000 >> >>> >> >>> xen: arm64: disable alignment traps >> >>> >> >>> The mem* primitives which I am about to import from Linux in a subsequent >> >>> patch rely on the hardware handling misalignment. >> >>> >> >>> The benefits of an optimised memcpy etc outweigh the downsides. >> >>> >> >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >> >>> Acked-by: Julien Grall <julien.grall@linaro.org> >> >>> Acked-by: Tim Deegan <tim@xen.org> >> >>> >> >>> I will consider this for backport, but first I'd like to consider >> >>> whether we shouldn't fix the hypervisor side evtchn FIFO code along the >> >>> same lines as the kernel side. David, any thoughts? >> >> >> >> I believe Jan suggested making Xen's bitops handle 32-bit alignment or >> >> adding a new set of 32-bit bitops. >> > >> > This is already the case for the arm64 bitops, we deliberately diverged >> > from Linux here because there are a bunch of other "misaligned" 4-byte >> > bitmasks (one in the malloc implementation springs to mind). >> >> Then I'm not sure what it is you're trying to fix in Xen? > > I partially hust wanted to consider whether anything should be better > changed than rely on the bitops being more flexible, for some reason. > But I also wanted confirmation that the problematic instruction was > generated by gcc and not by some handcoded asm somewhere which we hadn't > properly fixed. > > Ian. > > I believe it comes form test_bit (xen/include/asm-arm/bitops.h). Vladimir ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] xen/arm64: disable alignment check 2014-04-29 7:38 ` Vladimir Murzin @ 2014-04-29 8:58 ` Ian Campbell 2014-05-09 13:24 ` [PATCH] xen: arm: bitops take unsigned int (Was: Re: [PATCH] xen/arm64: disable alignment check) Ian Campbell 0 siblings, 1 reply; 17+ messages in thread From: Ian Campbell @ 2014-04-29 8:58 UTC (permalink / raw) To: Vladimir Murzin; +Cc: xen-devel@lists.xenproject.org, David Vrabel, Jan Beulich On Tue, 2014-04-29 at 08:38 +0100, Vladimir Murzin wrote: > On Mon, Apr 28, 2014 at 11:43 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > But I also wanted confirmation that the problematic instruction was > > generated by gcc and not by some handcoded asm somewhere which we hadn't > > properly fixed. > I believe it comes form test_bit (xen/include/asm-arm/bitops.h). Ah, then I think this code needs fixing too. Probably switching to unsigned int * throughout would work, what do you think? Ian. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] xen: arm: bitops take unsigned int (Was: Re: [PATCH] xen/arm64: disable alignment check) 2014-04-29 8:58 ` Ian Campbell @ 2014-05-09 13:24 ` Ian Campbell 2014-05-09 16:19 ` Ian Campbell 0 siblings, 1 reply; 17+ messages in thread From: Ian Campbell @ 2014-05-09 13:24 UTC (permalink / raw) To: Vladimir Murzin; +Cc: xen-devel@lists.xenproject.org, David Vrabel, Jan Beulich On Tue, 2014-04-29 at 09:58 +0100, Ian Campbell wrote: > On Tue, 2014-04-29 at 08:38 +0100, Vladimir Murzin wrote: > > On Mon, Apr 28, 2014 at 11:43 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > > But I also wanted confirmation that the problematic instruction was > > > generated by gcc and not by some handcoded asm somewhere which we hadn't > > > properly fixed. > > > I believe it comes form test_bit (xen/include/asm-arm/bitops.h). > > Ah, then I think this code needs fixing too. Probably switching to > unsigned int * throughout would work, what do you think? I finally managed to upgrade to a new enough kernel to trigger this. This Works For Me(tm), along with the Linux patch "xen/events/fifo: correctly align bitops" which is queued for 3.15 Linus (but not sent yet?) 8<------------------- >From aa6afe6520ea22241fb0ce430ef315c49a73867f Mon Sep 17 00:00:00 2001 From: Ian Campbell <ian.campbell@citrix.com> Date: Thu, 8 May 2014 16:13:55 +0100 Subject: [PATCH] xen: arm: bitops take unsigned int Xen bitmaps can be 4 rather than 8 byte aligned, so use the appropriate type. Otherwise the compiler can generate unaligned 8 byte accesses and cause traps. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- xen/include/asm-arm/bitops.h | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/xen/include/asm-arm/bitops.h b/xen/include/asm-arm/bitops.h index 0a7caee..25f96c8 100644 --- a/xen/include/asm-arm/bitops.h +++ b/xen/include/asm-arm/bitops.h @@ -18,13 +18,14 @@ #define __set_bit(n,p) set_bit(n,p) #define __clear_bit(n,p) clear_bit(n,p) +#define BITS_PER_WORD 32 #define BIT(nr) (1UL << (nr)) -#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) -#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) +#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_WORD)) +#define BIT_WORD(nr) ((nr) / BITS_PER_WORD) #define BITS_PER_BYTE 8 -#define ADDR (*(volatile long *) addr) -#define CONST_ADDR (*(const volatile long *) addr) +#define ADDR (*(volatile int *) addr) +#define CONST_ADDR (*(const volatile int *) addr) #if defined(CONFIG_ARM_32) # include <asm/arm32/bitops.h> @@ -45,10 +46,10 @@ */ static inline int __test_and_set_bit(int nr, volatile void *addr) { - unsigned long mask = BIT_MASK(nr); - volatile unsigned long *p = - ((volatile unsigned long *)addr) + BIT_WORD(nr); - unsigned long old = *p; + unsigned int mask = BIT_MASK(nr); + volatile unsigned int *p = + ((volatile unsigned int *)addr) + BIT_WORD(nr); + unsigned int old = *p; *p = old | mask; return (old & mask) != 0; @@ -65,10 +66,10 @@ static inline int __test_and_set_bit(int nr, volatile void *addr) */ static inline int __test_and_clear_bit(int nr, volatile void *addr) { - unsigned long mask = BIT_MASK(nr); - volatile unsigned long *p = - ((volatile unsigned long *)addr) + BIT_WORD(nr); - unsigned long old = *p; + unsigned int mask = BIT_MASK(nr); + volatile unsigned int *p = + ((volatile unsigned int *)addr) + BIT_WORD(nr); + unsigned int old = *p; *p = old & ~mask; return (old & mask) != 0; @@ -78,10 +79,10 @@ static inline int __test_and_clear_bit(int nr, volatile void *addr) static inline int __test_and_change_bit(int nr, volatile void *addr) { - unsigned long mask = BIT_MASK(nr); - volatile unsigned long *p = - ((volatile unsigned long *)addr) + BIT_WORD(nr); - unsigned long old = *p; + unsigned int mask = BIT_MASK(nr); + volatile unsigned int *p = + ((volatile unsigned int *)addr) + BIT_WORD(nr); + unsigned int old = *p; *p = old ^ mask; return (old & mask) != 0; @@ -94,8 +95,8 @@ static inline int __test_and_change_bit(int nr, */ static inline int test_bit(int nr, const volatile void *addr) { - const volatile unsigned long *p = (const volatile unsigned long *)addr; - return 1UL & (p[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1))); + const volatile unsigned int *p = (const volatile unsigned int *)addr; + return 1UL & (p[BIT_WORD(nr)] >> (nr & (BITS_PER_WORD-1))); } static inline int constant_fls(int x) -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] xen: arm: bitops take unsigned int (Was: Re: [PATCH] xen/arm64: disable alignment check) 2014-05-09 13:24 ` [PATCH] xen: arm: bitops take unsigned int (Was: Re: [PATCH] xen/arm64: disable alignment check) Ian Campbell @ 2014-05-09 16:19 ` Ian Campbell 2014-05-11 18:34 ` Stefano Stabellini 2014-05-12 12:15 ` Julien Grall 0 siblings, 2 replies; 17+ messages in thread From: Ian Campbell @ 2014-05-09 16:19 UTC (permalink / raw) To: Vladimir Murzin Cc: Tim Deegan, Julien Grall, Stefano Stabellini, David Vrabel, Jan Beulich, xen-devel@lists.xenproject.org (Just adding the other ARM guys...) On Fri, 2014-05-09 at 14:24 +0100, Ian Campbell wrote: > On Tue, 2014-04-29 at 09:58 +0100, Ian Campbell wrote: > > On Tue, 2014-04-29 at 08:38 +0100, Vladimir Murzin wrote: > > > On Mon, Apr 28, 2014 at 11:43 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > > > But I also wanted confirmation that the problematic instruction was > > > > generated by gcc and not by some handcoded asm somewhere which we hadn't > > > > properly fixed. > > > > > I believe it comes form test_bit (xen/include/asm-arm/bitops.h). > > > > Ah, then I think this code needs fixing too. Probably switching to > > unsigned int * throughout would work, what do you think? > > I finally managed to upgrade to a new enough kernel to trigger this. > > This Works For Me(tm), along with the Linux patch "xen/events/fifo: > correctly align bitops" which is queued for 3.15 Linus (but not sent > yet?) > > 8<------------------- > > From aa6afe6520ea22241fb0ce430ef315c49a73867f Mon Sep 17 00:00:00 2001 > From: Ian Campbell <ian.campbell@citrix.com> > Date: Thu, 8 May 2014 16:13:55 +0100 > Subject: [PATCH] xen: arm: bitops take unsigned int > > Xen bitmaps can be 4 rather than 8 byte aligned, so use the appropriate type. > Otherwise the compiler can generate unaligned 8 byte accesses and cause traps. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > --- > xen/include/asm-arm/bitops.h | 37 +++++++++++++++++++------------------ > 1 file changed, 19 insertions(+), 18 deletions(-) > > diff --git a/xen/include/asm-arm/bitops.h b/xen/include/asm-arm/bitops.h > index 0a7caee..25f96c8 100644 > --- a/xen/include/asm-arm/bitops.h > +++ b/xen/include/asm-arm/bitops.h > @@ -18,13 +18,14 @@ > #define __set_bit(n,p) set_bit(n,p) > #define __clear_bit(n,p) clear_bit(n,p) > > +#define BITS_PER_WORD 32 > #define BIT(nr) (1UL << (nr)) > -#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) > -#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) > +#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_WORD)) > +#define BIT_WORD(nr) ((nr) / BITS_PER_WORD) > #define BITS_PER_BYTE 8 > > -#define ADDR (*(volatile long *) addr) > -#define CONST_ADDR (*(const volatile long *) addr) > +#define ADDR (*(volatile int *) addr) > +#define CONST_ADDR (*(const volatile int *) addr) > > #if defined(CONFIG_ARM_32) > # include <asm/arm32/bitops.h> > @@ -45,10 +46,10 @@ > */ > static inline int __test_and_set_bit(int nr, volatile void *addr) > { > - unsigned long mask = BIT_MASK(nr); > - volatile unsigned long *p = > - ((volatile unsigned long *)addr) + BIT_WORD(nr); > - unsigned long old = *p; > + unsigned int mask = BIT_MASK(nr); > + volatile unsigned int *p = > + ((volatile unsigned int *)addr) + BIT_WORD(nr); > + unsigned int old = *p; > > *p = old | mask; > return (old & mask) != 0; > @@ -65,10 +66,10 @@ static inline int __test_and_set_bit(int nr, volatile void *addr) > */ > static inline int __test_and_clear_bit(int nr, volatile void *addr) > { > - unsigned long mask = BIT_MASK(nr); > - volatile unsigned long *p = > - ((volatile unsigned long *)addr) + BIT_WORD(nr); > - unsigned long old = *p; > + unsigned int mask = BIT_MASK(nr); > + volatile unsigned int *p = > + ((volatile unsigned int *)addr) + BIT_WORD(nr); > + unsigned int old = *p; > > *p = old & ~mask; > return (old & mask) != 0; > @@ -78,10 +79,10 @@ static inline int __test_and_clear_bit(int nr, volatile void *addr) > static inline int __test_and_change_bit(int nr, > volatile void *addr) > { > - unsigned long mask = BIT_MASK(nr); > - volatile unsigned long *p = > - ((volatile unsigned long *)addr) + BIT_WORD(nr); > - unsigned long old = *p; > + unsigned int mask = BIT_MASK(nr); > + volatile unsigned int *p = > + ((volatile unsigned int *)addr) + BIT_WORD(nr); > + unsigned int old = *p; > > *p = old ^ mask; > return (old & mask) != 0; > @@ -94,8 +95,8 @@ static inline int __test_and_change_bit(int nr, > */ > static inline int test_bit(int nr, const volatile void *addr) > { > - const volatile unsigned long *p = (const volatile unsigned long *)addr; > - return 1UL & (p[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1))); > + const volatile unsigned int *p = (const volatile unsigned int *)addr; > + return 1UL & (p[BIT_WORD(nr)] >> (nr & (BITS_PER_WORD-1))); > } > > static inline int constant_fls(int x) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] xen: arm: bitops take unsigned int (Was: Re: [PATCH] xen/arm64: disable alignment check) 2014-05-09 16:19 ` Ian Campbell @ 2014-05-11 18:34 ` Stefano Stabellini 2014-05-12 8:36 ` Ian Campbell 2014-05-12 12:15 ` Julien Grall 1 sibling, 1 reply; 17+ messages in thread From: Stefano Stabellini @ 2014-05-11 18:34 UTC (permalink / raw) To: Ian Campbell Cc: Vladimir Murzin, Tim Deegan, Julien Grall, Stefano Stabellini, David Vrabel, Jan Beulich, xen-devel@lists.xenproject.org On Fri, 9 May 2014, Ian Campbell wrote: > (Just adding the other ARM guys...) > > On Fri, 2014-05-09 at 14:24 +0100, Ian Campbell wrote: > > On Tue, 2014-04-29 at 09:58 +0100, Ian Campbell wrote: > > > On Tue, 2014-04-29 at 08:38 +0100, Vladimir Murzin wrote: > > > > On Mon, Apr 28, 2014 at 11:43 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > > > > But I also wanted confirmation that the problematic instruction was > > > > > generated by gcc and not by some handcoded asm somewhere which we hadn't > > > > > properly fixed. > > > > > > > I believe it comes form test_bit (xen/include/asm-arm/bitops.h). > > > > > > Ah, then I think this code needs fixing too. Probably switching to > > > unsigned int * throughout would work, what do you think? > > > > I finally managed to upgrade to a new enough kernel to trigger this. > > > > This Works For Me(tm), along with the Linux patch "xen/events/fifo: > > correctly align bitops" which is queued for 3.15 Linus (but not sent > > yet?) The change looks good to me. > > 8<------------------- > > > > From aa6afe6520ea22241fb0ce430ef315c49a73867f Mon Sep 17 00:00:00 2001 > > From: Ian Campbell <ian.campbell@citrix.com> > > Date: Thu, 8 May 2014 16:13:55 +0100 > > Subject: [PATCH] xen: arm: bitops take unsigned int > > > > Xen bitmaps can be 4 rather than 8 byte aligned, so use the appropriate type. > > Otherwise the compiler can generate unaligned 8 byte accesses and cause traps. > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > --- > > xen/include/asm-arm/bitops.h | 37 +++++++++++++++++++------------------ > > 1 file changed, 19 insertions(+), 18 deletions(-) > > > > diff --git a/xen/include/asm-arm/bitops.h b/xen/include/asm-arm/bitops.h > > index 0a7caee..25f96c8 100644 > > --- a/xen/include/asm-arm/bitops.h > > +++ b/xen/include/asm-arm/bitops.h > > @@ -18,13 +18,14 @@ > > #define __set_bit(n,p) set_bit(n,p) > > #define __clear_bit(n,p) clear_bit(n,p) > > > > +#define BITS_PER_WORD 32 > > #define BIT(nr) (1UL << (nr)) > > -#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) > > -#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) > > +#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_WORD)) > > +#define BIT_WORD(nr) ((nr) / BITS_PER_WORD) > > #define BITS_PER_BYTE 8 > > > > -#define ADDR (*(volatile long *) addr) > > -#define CONST_ADDR (*(const volatile long *) addr) > > +#define ADDR (*(volatile int *) addr) > > +#define CONST_ADDR (*(const volatile int *) addr) > > > > #if defined(CONFIG_ARM_32) > > # include <asm/arm32/bitops.h> > > @@ -45,10 +46,10 @@ > > */ > > static inline int __test_and_set_bit(int nr, volatile void *addr) > > { > > - unsigned long mask = BIT_MASK(nr); > > - volatile unsigned long *p = > > - ((volatile unsigned long *)addr) + BIT_WORD(nr); > > - unsigned long old = *p; > > + unsigned int mask = BIT_MASK(nr); > > + volatile unsigned int *p = > > + ((volatile unsigned int *)addr) + BIT_WORD(nr); > > + unsigned int old = *p; > > > > *p = old | mask; > > return (old & mask) != 0; > > @@ -65,10 +66,10 @@ static inline int __test_and_set_bit(int nr, volatile void *addr) > > */ > > static inline int __test_and_clear_bit(int nr, volatile void *addr) > > { > > - unsigned long mask = BIT_MASK(nr); > > - volatile unsigned long *p = > > - ((volatile unsigned long *)addr) + BIT_WORD(nr); > > - unsigned long old = *p; > > + unsigned int mask = BIT_MASK(nr); > > + volatile unsigned int *p = > > + ((volatile unsigned int *)addr) + BIT_WORD(nr); > > + unsigned int old = *p; > > > > *p = old & ~mask; > > return (old & mask) != 0; > > @@ -78,10 +79,10 @@ static inline int __test_and_clear_bit(int nr, volatile void *addr) > > static inline int __test_and_change_bit(int nr, > > volatile void *addr) > > { > > - unsigned long mask = BIT_MASK(nr); > > - volatile unsigned long *p = > > - ((volatile unsigned long *)addr) + BIT_WORD(nr); > > - unsigned long old = *p; > > + unsigned int mask = BIT_MASK(nr); > > + volatile unsigned int *p = > > + ((volatile unsigned int *)addr) + BIT_WORD(nr); > > + unsigned int old = *p; > > > > *p = old ^ mask; > > return (old & mask) != 0; > > @@ -94,8 +95,8 @@ static inline int __test_and_change_bit(int nr, > > */ > > static inline int test_bit(int nr, const volatile void *addr) > > { > > - const volatile unsigned long *p = (const volatile unsigned long *)addr; > > - return 1UL & (p[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1))); > > + const volatile unsigned int *p = (const volatile unsigned int *)addr; > > + return 1UL & (p[BIT_WORD(nr)] >> (nr & (BITS_PER_WORD-1))); > > } > > > > static inline int constant_fls(int x) > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] xen: arm: bitops take unsigned int (Was: Re: [PATCH] xen/arm64: disable alignment check) 2014-05-11 18:34 ` Stefano Stabellini @ 2014-05-12 8:36 ` Ian Campbell 2014-05-12 9:45 ` Stefano Stabellini 0 siblings, 1 reply; 17+ messages in thread From: Ian Campbell @ 2014-05-12 8:36 UTC (permalink / raw) To: Stefano Stabellini Cc: Vladimir Murzin, Tim Deegan, Julien Grall, Stefano Stabellini, David Vrabel, Jan Beulich, xen-devel@lists.xenproject.org On Sun, 2014-05-11 at 19:34 +0100, Stefano Stabellini wrote: > On Fri, 9 May 2014, Ian Campbell wrote: > > (Just adding the other ARM guys...) > > > > On Fri, 2014-05-09 at 14:24 +0100, Ian Campbell wrote: > > > On Tue, 2014-04-29 at 09:58 +0100, Ian Campbell wrote: > > > > On Tue, 2014-04-29 at 08:38 +0100, Vladimir Murzin wrote: > > > > > On Mon, Apr 28, 2014 at 11:43 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > > > > > But I also wanted confirmation that the problematic instruction was > > > > > > generated by gcc and not by some handcoded asm somewhere which we hadn't > > > > > > properly fixed. > > > > > > > > > I believe it comes form test_bit (xen/include/asm-arm/bitops.h). > > > > > > > > Ah, then I think this code needs fixing too. Probably switching to > > > > unsigned int * throughout would work, what do you think? > > > > > > I finally managed to upgrade to a new enough kernel to trigger this. > > > > > > This Works For Me(tm), along with the Linux patch "xen/events/fifo: > > > correctly align bitops" which is queued for 3.15 Linus (but not sent > > > yet?) > > The change looks good to me. Are you acking it? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] xen: arm: bitops take unsigned int (Was: Re: [PATCH] xen/arm64: disable alignment check) 2014-05-12 8:36 ` Ian Campbell @ 2014-05-12 9:45 ` Stefano Stabellini 0 siblings, 0 replies; 17+ messages in thread From: Stefano Stabellini @ 2014-05-12 9:45 UTC (permalink / raw) To: Ian Campbell Cc: Stefano Stabellini, Vladimir Murzin, Tim Deegan, Julien Grall, Stefano Stabellini, David Vrabel, Jan Beulich, xen-devel@lists.xenproject.org On Mon, 12 May 2014, Ian Campbell wrote: > On Sun, 2014-05-11 at 19:34 +0100, Stefano Stabellini wrote: > > On Fri, 9 May 2014, Ian Campbell wrote: > > > (Just adding the other ARM guys...) > > > > > > On Fri, 2014-05-09 at 14:24 +0100, Ian Campbell wrote: > > > > On Tue, 2014-04-29 at 09:58 +0100, Ian Campbell wrote: > > > > > On Tue, 2014-04-29 at 08:38 +0100, Vladimir Murzin wrote: > > > > > > On Mon, Apr 28, 2014 at 11:43 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > > > > > > But I also wanted confirmation that the problematic instruction was > > > > > > > generated by gcc and not by some handcoded asm somewhere which we hadn't > > > > > > > properly fixed. > > > > > > > > > > > I believe it comes form test_bit (xen/include/asm-arm/bitops.h). > > > > > > > > > > Ah, then I think this code needs fixing too. Probably switching to > > > > > unsigned int * throughout would work, what do you think? > > > > > > > > I finally managed to upgrade to a new enough kernel to trigger this. > > > > > > > > This Works For Me(tm), along with the Linux patch "xen/events/fifo: > > > > correctly align bitops" which is queued for 3.15 Linus (but not sent > > > > yet?) > > > > The change looks good to me. > > Are you acking it? Yes ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] xen: arm: bitops take unsigned int (Was: Re: [PATCH] xen/arm64: disable alignment check) 2014-05-09 16:19 ` Ian Campbell 2014-05-11 18:34 ` Stefano Stabellini @ 2014-05-12 12:15 ` Julien Grall 2014-05-12 12:18 ` Ian Campbell 1 sibling, 1 reply; 17+ messages in thread From: Julien Grall @ 2014-05-12 12:15 UTC (permalink / raw) To: Ian Campbell, Vladimir Murzin Cc: xen-devel@lists.xenproject.org, Stefano Stabellini, David Vrabel, Jan Beulich, Tim Deegan Hi Ian, On 05/09/2014 05:19 PM, Ian Campbell wrote: > (Just adding the other ARM guys...) > > On Fri, 2014-05-09 at 14:24 +0100, Ian Campbell wrote: >> On Tue, 2014-04-29 at 09:58 +0100, Ian Campbell wrote: >>> On Tue, 2014-04-29 at 08:38 +0100, Vladimir Murzin wrote: >>>> On Mon, Apr 28, 2014 at 11:43 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: >>>>> But I also wanted confirmation that the problematic instruction was >>>>> generated by gcc and not by some handcoded asm somewhere which we hadn't >>>>> properly fixed. >>> >>>> I believe it comes form test_bit (xen/include/asm-arm/bitops.h). >>> >>> Ah, then I think this code needs fixing too. Probably switching to >>> unsigned int * throughout would work, what do you think? >> >> I finally managed to upgrade to a new enough kernel to trigger this. >> >> This Works For Me(tm), along with the Linux patch "xen/events/fifo: >> correctly align bitops" which is queued for 3.15 Linus (but not sent >> yet?) >> >> 8<------------------- >> >> From aa6afe6520ea22241fb0ce430ef315c49a73867f Mon Sep 17 00:00:00 2001 >> From: Ian Campbell <ian.campbell@citrix.com> >> Date: Thu, 8 May 2014 16:13:55 +0100 >> Subject: [PATCH] xen: arm: bitops take unsigned int >> >> Xen bitmaps can be 4 rather than 8 byte aligned, so use the appropriate type. >> Otherwise the compiler can generate unaligned 8 byte accesses and cause traps. >> >> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >> --- >> xen/include/asm-arm/bitops.h | 37 +++++++++++++++++++------------------ >> 1 file changed, 19 insertions(+), 18 deletions(-) >> >> diff --git a/xen/include/asm-arm/bitops.h b/xen/include/asm-arm/bitops.h >> index 0a7caee..25f96c8 100644 >> --- a/xen/include/asm-arm/bitops.h >> +++ b/xen/include/asm-arm/bitops.h >> @@ -18,13 +18,14 @@ >> #define __set_bit(n,p) set_bit(n,p) >> #define __clear_bit(n,p) clear_bit(n,p) >> >> +#define BITS_PER_WORD 32 Can you define BITS_PER_WORD in asm-arm/config.h rather than here? Regards, -- Julien Grall ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] xen: arm: bitops take unsigned int (Was: Re: [PATCH] xen/arm64: disable alignment check) 2014-05-12 12:15 ` Julien Grall @ 2014-05-12 12:18 ` Ian Campbell 2014-05-12 13:44 ` Julien Grall 0 siblings, 1 reply; 17+ messages in thread From: Ian Campbell @ 2014-05-12 12:18 UTC (permalink / raw) To: Julien Grall Cc: Vladimir Murzin, Tim Deegan, Stefano Stabellini, David Vrabel, Jan Beulich, xen-devel@lists.xenproject.org On Mon, 2014-05-12 at 13:15 +0100, Julien Grall wrote: > Hi Ian, > > On 05/09/2014 05:19 PM, Ian Campbell wrote: > > (Just adding the other ARM guys...) > > > > On Fri, 2014-05-09 at 14:24 +0100, Ian Campbell wrote: > >> On Tue, 2014-04-29 at 09:58 +0100, Ian Campbell wrote: > >>> On Tue, 2014-04-29 at 08:38 +0100, Vladimir Murzin wrote: > >>>> On Mon, Apr 28, 2014 at 11:43 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: > >>>>> But I also wanted confirmation that the problematic instruction was > >>>>> generated by gcc and not by some handcoded asm somewhere which we hadn't > >>>>> properly fixed. > >>> > >>>> I believe it comes form test_bit (xen/include/asm-arm/bitops.h). > >>> > >>> Ah, then I think this code needs fixing too. Probably switching to > >>> unsigned int * throughout would work, what do you think? > >> > >> I finally managed to upgrade to a new enough kernel to trigger this. > >> > >> This Works For Me(tm), along with the Linux patch "xen/events/fifo: > >> correctly align bitops" which is queued for 3.15 Linus (but not sent > >> yet?) > >> > >> 8<------------------- > >> > >> From aa6afe6520ea22241fb0ce430ef315c49a73867f Mon Sep 17 00:00:00 2001 > >> From: Ian Campbell <ian.campbell@citrix.com> > >> Date: Thu, 8 May 2014 16:13:55 +0100 > >> Subject: [PATCH] xen: arm: bitops take unsigned int > >> > >> Xen bitmaps can be 4 rather than 8 byte aligned, so use the appropriate type. > >> Otherwise the compiler can generate unaligned 8 byte accesses and cause traps. > >> > >> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > >> --- > >> xen/include/asm-arm/bitops.h | 37 +++++++++++++++++++------------------ > >> 1 file changed, 19 insertions(+), 18 deletions(-) > >> > >> diff --git a/xen/include/asm-arm/bitops.h b/xen/include/asm-arm/bitops.h > >> index 0a7caee..25f96c8 100644 > >> --- a/xen/include/asm-arm/bitops.h > >> +++ b/xen/include/asm-arm/bitops.h > >> @@ -18,13 +18,14 @@ > >> #define __set_bit(n,p) set_bit(n,p) > >> #define __clear_bit(n,p) clear_bit(n,p) > >> > >> +#define BITS_PER_WORD 32 > > Can you define BITS_PER_WORD in asm-arm/config.h rather than here? For better or worse BITS_PER_BYTE is already defined in bitops.h and since I've already run the majority of my pre-push commit checks on a branch containing this fix (along with some other bits and bobs) I'm not inclined to restart that process just for this change. Ian. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] xen: arm: bitops take unsigned int (Was: Re: [PATCH] xen/arm64: disable alignment check) 2014-05-12 12:18 ` Ian Campbell @ 2014-05-12 13:44 ` Julien Grall 2014-05-12 14:08 ` Ian Campbell 0 siblings, 1 reply; 17+ messages in thread From: Julien Grall @ 2014-05-12 13:44 UTC (permalink / raw) To: Ian Campbell, Julien Grall Cc: Vladimir Murzin, Tim Deegan, Stefano Stabellini, David Vrabel, Jan Beulich, xen-devel@lists.xenproject.org On 05/12/2014 01:18 PM, Ian Campbell wrote: > On Mon, 2014-05-12 at 13:15 +0100, Julien Grall wrote: >> Hi Ian, >> >> On 05/09/2014 05:19 PM, Ian Campbell wrote: >>> (Just adding the other ARM guys...) >>> >>> On Fri, 2014-05-09 at 14:24 +0100, Ian Campbell wrote: >>>> On Tue, 2014-04-29 at 09:58 +0100, Ian Campbell wrote: >>>>> On Tue, 2014-04-29 at 08:38 +0100, Vladimir Murzin wrote: >>>>>> On Mon, Apr 28, 2014 at 11:43 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote: >>>>>>> But I also wanted confirmation that the problematic instruction was >>>>>>> generated by gcc and not by some handcoded asm somewhere which we hadn't >>>>>>> properly fixed. >>>>> >>>>>> I believe it comes form test_bit (xen/include/asm-arm/bitops.h). >>>>> >>>>> Ah, then I think this code needs fixing too. Probably switching to >>>>> unsigned int * throughout would work, what do you think? >>>> >>>> I finally managed to upgrade to a new enough kernel to trigger this. >>>> >>>> This Works For Me(tm), along with the Linux patch "xen/events/fifo: >>>> correctly align bitops" which is queued for 3.15 Linus (but not sent >>>> yet?) >>>> >>>> 8<------------------- >>>> >>>> From aa6afe6520ea22241fb0ce430ef315c49a73867f Mon Sep 17 00:00:00 2001 >>>> From: Ian Campbell <ian.campbell@citrix.com> >>>> Date: Thu, 8 May 2014 16:13:55 +0100 >>>> Subject: [PATCH] xen: arm: bitops take unsigned int >>>> >>>> Xen bitmaps can be 4 rather than 8 byte aligned, so use the appropriate type. >>>> Otherwise the compiler can generate unaligned 8 byte accesses and cause traps. >>>> >>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >>>> --- >>>> xen/include/asm-arm/bitops.h | 37 +++++++++++++++++++------------------ >>>> 1 file changed, 19 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/xen/include/asm-arm/bitops.h b/xen/include/asm-arm/bitops.h >>>> index 0a7caee..25f96c8 100644 >>>> --- a/xen/include/asm-arm/bitops.h >>>> +++ b/xen/include/asm-arm/bitops.h >>>> @@ -18,13 +18,14 @@ >>>> #define __set_bit(n,p) set_bit(n,p) >>>> #define __clear_bit(n,p) clear_bit(n,p) >>>> >>>> +#define BITS_PER_WORD 32 >> >> Can you define BITS_PER_WORD in asm-arm/config.h rather than here? > > For better or worse BITS_PER_BYTE is already defined in bitops.h and > since I've already run the majority of my pre-push commit checks on a > branch containing this fix (along with some other bits and bobs) I'm not > inclined to restart that process just for this change. No problem. I guess you plan to backport this patch for Xen 4.4? Regards, -- Julien Grall ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] xen: arm: bitops take unsigned int (Was: Re: [PATCH] xen/arm64: disable alignment check) 2014-05-12 13:44 ` Julien Grall @ 2014-05-12 14:08 ` Ian Campbell 0 siblings, 0 replies; 17+ messages in thread From: Ian Campbell @ 2014-05-12 14:08 UTC (permalink / raw) To: Julien Grall Cc: Vladimir Murzin, Tim Deegan, Julien Grall, Stefano Stabellini, David Vrabel, Jan Beulich, xen-devel@lists.xenproject.org On Mon, 2014-05-12 at 14:44 +0100, Julien Grall wrote: > I guess you plan to backport this patch for Xen 4.4? Yes. Ian. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2014-05-12 14:08 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-04-27 9:10 [PATCH] xen/arm64: disable alignment check Vladimir Murzin 2014-04-28 9:48 ` Ian Campbell 2014-04-28 10:24 ` David Vrabel 2014-04-28 10:36 ` Ian Campbell 2014-04-28 10:37 ` David Vrabel 2014-04-28 10:43 ` Ian Campbell 2014-04-29 7:38 ` Vladimir Murzin 2014-04-29 8:58 ` Ian Campbell 2014-05-09 13:24 ` [PATCH] xen: arm: bitops take unsigned int (Was: Re: [PATCH] xen/arm64: disable alignment check) Ian Campbell 2014-05-09 16:19 ` Ian Campbell 2014-05-11 18:34 ` Stefano Stabellini 2014-05-12 8:36 ` Ian Campbell 2014-05-12 9:45 ` Stefano Stabellini 2014-05-12 12:15 ` Julien Grall 2014-05-12 12:18 ` Ian Campbell 2014-05-12 13:44 ` Julien Grall 2014-05-12 14:08 ` Ian Campbell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.