| Message ID | E1YqkpO-0006zB-QJ@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 8 May 2015, Russell King wrote: > BSYM() should only be used when refering to local symbols in the same > assembly file which are resolved by the assembler, and not for > linker-fixed up symbols. The use of BSYM() with panic is incorrect as > the linker is involved in fixing up this relocation, and it knows > whether panic() is ARM or Thumb. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> Acked-by: Nicolas Pitre <nico@linaro.org> > --- > arch/arm/kvm/interrupts.S | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S > index 79caf79b304a..87847d2c5f99 100644 > --- a/arch/arm/kvm/interrupts.S > +++ b/arch/arm/kvm/interrupts.S > @@ -309,7 +309,7 @@ ENTRY(kvm_call_hyp) > THUMB( orr r2, r2, #PSR_T_BIT ) > msr spsr_cxsf, r2 > mrs r1, ELR_hyp > - ldr r2, =BSYM(panic) > + ldr r2, =panic > msr ELR_hyp, r2 > ldr r0, =\panic_str > clrex @ Clear exclusive monitor > -- > 1.8.3.1 > >
On Fri, May 08, 2015 at 05:08:42PM +0100, Russell King wrote: > BSYM() should only be used when refering to local symbols in the same > assembly file which are resolved by the assembler, and not for > linker-fixed up symbols. The use of BSYM() with panic is incorrect as > the linker is involved in fixing up this relocation, and it knows > whether panic() is ARM or Thumb. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> Acked-by: Dave Martin <Dave.Martin@arm.com> > --- > arch/arm/kvm/interrupts.S | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S > index 79caf79b304a..87847d2c5f99 100644 > --- a/arch/arm/kvm/interrupts.S > +++ b/arch/arm/kvm/interrupts.S > @@ -309,7 +309,7 @@ ENTRY(kvm_call_hyp) > THUMB( orr r2, r2, #PSR_T_BIT ) > msr spsr_cxsf, r2 > mrs r1, ELR_hyp > - ldr r2, =BSYM(panic) > + ldr r2, =panic > msr ELR_hyp, r2 > ldr r0, =\panic_str > clrex @ Clear exclusive monitor > -- > 1.8.3.1 >
On Fri, May 08, 2015 at 05:08:42PM +0100, Russell King wrote: > BSYM() should only be used when refering to local symbols in the same > assembly file which are resolved by the assembler, and not for > linker-fixed up symbols. The use of BSYM() with panic is incorrect as > the linker is involved in fixing up this relocation, and it knows > whether panic() is ARM or Thumb. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > arch/arm/kvm/interrupts.S | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S > index 79caf79b304a..87847d2c5f99 100644 > --- a/arch/arm/kvm/interrupts.S > +++ b/arch/arm/kvm/interrupts.S > @@ -309,7 +309,7 @@ ENTRY(kvm_call_hyp) > THUMB( orr r2, r2, #PSR_T_BIT ) > msr spsr_cxsf, r2 > mrs r1, ELR_hyp > - ldr r2, =BSYM(panic) > + ldr r2, =panic > msr ELR_hyp, r2 > ldr r0, =\panic_str > clrex @ Clear exclusive monitor > -- > 1.8.3.1 > Indeed, the linker figures it out as it should. It does seem like the right result is produced with the BSYM() macro as well so not sure what the harm is. Anyway, I've queued this to merge via the KVM tree. Thanks, -Christoffer
On 9 May 2015 at 22:07, Christoffer Dall <christoffer.dall@linaro.org> wrote: > On Fri, May 08, 2015 at 05:08:42PM +0100, Russell King wrote: >> BSYM() should only be used when refering to local symbols in the same >> assembly file which are resolved by the assembler, and not for >> linker-fixed up symbols. The use of BSYM() with panic is incorrect as >> the linker is involved in fixing up this relocation, and it knows >> whether panic() is ARM or Thumb. >> >> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> >> --- >> arch/arm/kvm/interrupts.S | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S >> index 79caf79b304a..87847d2c5f99 100644 >> --- a/arch/arm/kvm/interrupts.S >> +++ b/arch/arm/kvm/interrupts.S >> @@ -309,7 +309,7 @@ ENTRY(kvm_call_hyp) >> THUMB( orr r2, r2, #PSR_T_BIT ) >> msr spsr_cxsf, r2 >> mrs r1, ELR_hyp >> - ldr r2, =BSYM(panic) >> + ldr r2, =panic >> msr ELR_hyp, r2 >> ldr r0, =\panic_str >> clrex @ Clear exclusive monitor >> -- >> 1.8.3.1 >> > Indeed, the linker figures it out as it should. It does seem like the > right result is produced with the BSYM() macro as well so not sure what > the harm is. > BSYM() is defined as 'sym + 1' not 'sym | 1', so if the symbol has the thumb bit set already, the result is incorrect. > Anyway, I've queued this to merge via the KVM tree. > > Thanks, > -Christoffer > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sat, May 09, 2015 at 10:07:17PM +0200, Christoffer Dall wrote: > On Fri, May 08, 2015 at 05:08:42PM +0100, Russell King wrote: > > BSYM() should only be used when refering to local symbols in the same > > assembly file which are resolved by the assembler, and not for > > linker-fixed up symbols. The use of BSYM() with panic is incorrect as > > the linker is involved in fixing up this relocation, and it knows > > whether panic() is ARM or Thumb. > > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > --- > > arch/arm/kvm/interrupts.S | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S > > index 79caf79b304a..87847d2c5f99 100644 > > --- a/arch/arm/kvm/interrupts.S > > +++ b/arch/arm/kvm/interrupts.S > > @@ -309,7 +309,7 @@ ENTRY(kvm_call_hyp) > > THUMB( orr r2, r2, #PSR_T_BIT ) > > msr spsr_cxsf, r2 > > mrs r1, ELR_hyp > > - ldr r2, =BSYM(panic) > > + ldr r2, =panic > > msr ELR_hyp, r2 > > ldr r0, =\panic_str > > clrex @ Clear exclusive monitor > > -- > > 1.8.3.1 > > > Indeed, the linker figures it out as it should. It does seem like the > right result is produced with the BSYM() macro as well so not sure what > the harm is. > > Anyway, I've queued this to merge via the KVM tree. I already have it in my tree (and linux-next) as the second patch (which removes the BSYM macro entirely) depends on this.
On Sat, May 09, 2015 at 09:10:57PM +0100, Russell King - ARM Linux wrote: > On Sat, May 09, 2015 at 10:07:17PM +0200, Christoffer Dall wrote: > > On Fri, May 08, 2015 at 05:08:42PM +0100, Russell King wrote: > > > BSYM() should only be used when refering to local symbols in the same > > > assembly file which are resolved by the assembler, and not for > > > linker-fixed up symbols. The use of BSYM() with panic is incorrect as > > > the linker is involved in fixing up this relocation, and it knows > > > whether panic() is ARM or Thumb. > > > > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > > --- > > > arch/arm/kvm/interrupts.S | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S > > > index 79caf79b304a..87847d2c5f99 100644 > > > --- a/arch/arm/kvm/interrupts.S > > > +++ b/arch/arm/kvm/interrupts.S > > > @@ -309,7 +309,7 @@ ENTRY(kvm_call_hyp) > > > THUMB( orr r2, r2, #PSR_T_BIT ) > > > msr spsr_cxsf, r2 > > > mrs r1, ELR_hyp > > > - ldr r2, =BSYM(panic) > > > + ldr r2, =panic > > > msr ELR_hyp, r2 > > > ldr r0, =\panic_str > > > clrex @ Clear exclusive monitor > > > -- > > > 1.8.3.1 > > > > > Indeed, the linker figures it out as it should. It does seem like the > > right result is produced with the BSYM() macro as well so not sure what > > the harm is. > > > > Anyway, I've queued this to merge via the KVM tree. > > I already have it in my tree (and linux-next) as the second patch (which > removes the BSYM macro entirely) depends on this. > ok, fine, you can add my ack then if you like: Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
On Sat, May 09, 2015 at 10:10:56PM +0200, Ard Biesheuvel wrote: > On 9 May 2015 at 22:07, Christoffer Dall <christoffer.dall@linaro.org> wrote: > > On Fri, May 08, 2015 at 05:08:42PM +0100, Russell King wrote: > >> BSYM() should only be used when refering to local symbols in the same > >> assembly file which are resolved by the assembler, and not for > >> linker-fixed up symbols. The use of BSYM() with panic is incorrect as > >> the linker is involved in fixing up this relocation, and it knows > >> whether panic() is ARM or Thumb. > >> > >> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > >> --- > >> arch/arm/kvm/interrupts.S | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S > >> index 79caf79b304a..87847d2c5f99 100644 > >> --- a/arch/arm/kvm/interrupts.S > >> +++ b/arch/arm/kvm/interrupts.S > >> @@ -309,7 +309,7 @@ ENTRY(kvm_call_hyp) > >> THUMB( orr r2, r2, #PSR_T_BIT ) > >> msr spsr_cxsf, r2 > >> mrs r1, ELR_hyp > >> - ldr r2, =BSYM(panic) > >> + ldr r2, =panic > >> msr ELR_hyp, r2 > >> ldr r0, =\panic_str > >> clrex @ Clear exclusive monitor > >> -- > >> 1.8.3.1 > >> > > Indeed, the linker figures it out as it should. It does seem like the > > right result is produced with the BSYM() macro as well so not sure what > > the harm is. > > > > BSYM() is defined as 'sym + 1' not 'sym | 1', so if the symbol has the > thumb bit set already, the result is incorrect. > yeah, but the linker will look at the result of 'sym + 1', so on my system it ends up with 'sym + 1' after the linker has done its thing (verified by looking at the disassembly of vmlinux); I assume the linker logic is that it's branching to a thumb function but the target is already the +1 so no action necessary, as opposed to just blindly adding 1. -Christoffer
On 11 May 2015 at 11:05, Christoffer Dall <christoffer.dall@linaro.org> wrote: > On Sat, May 09, 2015 at 10:10:56PM +0200, Ard Biesheuvel wrote: >> On 9 May 2015 at 22:07, Christoffer Dall <christoffer.dall@linaro.org> wrote: >> > On Fri, May 08, 2015 at 05:08:42PM +0100, Russell King wrote: >> >> BSYM() should only be used when refering to local symbols in the same >> >> assembly file which are resolved by the assembler, and not for >> >> linker-fixed up symbols. The use of BSYM() with panic is incorrect as >> >> the linker is involved in fixing up this relocation, and it knows >> >> whether panic() is ARM or Thumb. >> >> >> >> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> >> >> --- >> >> arch/arm/kvm/interrupts.S | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S >> >> index 79caf79b304a..87847d2c5f99 100644 >> >> --- a/arch/arm/kvm/interrupts.S >> >> +++ b/arch/arm/kvm/interrupts.S >> >> @@ -309,7 +309,7 @@ ENTRY(kvm_call_hyp) >> >> THUMB( orr r2, r2, #PSR_T_BIT ) >> >> msr spsr_cxsf, r2 >> >> mrs r1, ELR_hyp >> >> - ldr r2, =BSYM(panic) >> >> + ldr r2, =panic >> >> msr ELR_hyp, r2 >> >> ldr r0, =\panic_str >> >> clrex @ Clear exclusive monitor >> >> -- >> >> 1.8.3.1 >> >> >> > Indeed, the linker figures it out as it should. It does seem like the >> > right result is produced with the BSYM() macro as well so not sure what >> > the harm is. >> > >> >> BSYM() is defined as 'sym + 1' not 'sym | 1', so if the symbol has the >> thumb bit set already, the result is incorrect. >> > yeah, but the linker will look at the result of 'sym + 1', so on my > system it ends up with 'sym + 1' after the linker has done its thing > (verified by looking at the disassembly of vmlinux); Hmm, I though had done the same when this was under discussion a couple of weeks ago, and had arrived at the opposite conclusion, but now I cannot reproduce anymore so apparently not. Sorry for the noise. > I assume the > linker logic is that it's branching to a thumb function but the target > is already the +1 so no action necessary, as opposed to just blindly > adding 1. > > -Christoffer
On Mon, May 11, 2015 at 10:05:37AM +0100, Christoffer Dall wrote: > On Sat, May 09, 2015 at 10:10:56PM +0200, Ard Biesheuvel wrote: > > On 9 May 2015 at 22:07, Christoffer Dall <christoffer.dall@linaro.org> wrote: > > > On Fri, May 08, 2015 at 05:08:42PM +0100, Russell King wrote: > > >> BSYM() should only be used when refering to local symbols in the same > > >> assembly file which are resolved by the assembler, and not for > > >> linker-fixed up symbols. The use of BSYM() with panic is incorrect as > > >> the linker is involved in fixing up this relocation, and it knows > > >> whether panic() is ARM or Thumb. > > >> > > >> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > >> --- > > >> arch/arm/kvm/interrupts.S | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S > > >> index 79caf79b304a..87847d2c5f99 100644 > > >> --- a/arch/arm/kvm/interrupts.S > > >> +++ b/arch/arm/kvm/interrupts.S > > >> @@ -309,7 +309,7 @@ ENTRY(kvm_call_hyp) > > >> THUMB( orr r2, r2, #PSR_T_BIT ) > > >> msr spsr_cxsf, r2 > > >> mrs r1, ELR_hyp > > >> - ldr r2, =BSYM(panic) > > >> + ldr r2, =panic > > >> msr ELR_hyp, r2 > > >> ldr r0, =\panic_str > > >> clrex @ Clear exclusive monitor > > >> -- > > >> 1.8.3.1 > > >> > > > Indeed, the linker figures it out as it should. It does seem like the > > > right result is produced with the BSYM() macro as well so not sure what > > > the harm is. > > > > > > > BSYM() is defined as 'sym + 1' not 'sym | 1', so if the symbol has the > > thumb bit set already, the result is incorrect. > > > yeah, but the linker will look at the result of 'sym + 1', so on my > system it ends up with 'sym + 1' after the linker has done its thing > (verified by looking at the disassembly of vmlinux); I assume the > linker logic is that it's branching to a thumb function but the target > is already the +1 so no action necessary, as opposed to just blindly > adding 1. There are a few overlapping confusions. ldr= will do the right thing *if* the target symbol's type is correctly annotated. This means that ldr =some_local_code_symbol does the right thing for branch target addresses if and only if some_local_code_symbol is marked with .type %function (or ENDPROC). The fact that a symbol is in a code section is *not* enough. For ARM code this never mattered, so local symbols in .S files are probably under-annotated in general. BSYM() might have been used to work around this in some cases. We should check that all the BSYMs removed by this series from ldr= and .long/.word etc. point to a correctly annotated symbol, and add the annotations if not. Cheers ---Dave
On Mon, May 11, 2015 at 10:44:49AM +0100, Ard Biesheuvel wrote: > On 11 May 2015 at 11:05, Christoffer Dall <christoffer.dall@linaro.org> wrote: > > On Sat, May 09, 2015 at 10:10:56PM +0200, Ard Biesheuvel wrote: > >> On 9 May 2015 at 22:07, Christoffer Dall <christoffer.dall@linaro.org> wrote: > >> > On Fri, May 08, 2015 at 05:08:42PM +0100, Russell King wrote: > >> >> BSYM() should only be used when refering to local symbols in the same > >> >> assembly file which are resolved by the assembler, and not for > >> >> linker-fixed up symbols. The use of BSYM() with panic is incorrect as > >> >> the linker is involved in fixing up this relocation, and it knows > >> >> whether panic() is ARM or Thumb. > >> >> > >> >> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > >> >> --- > >> >> arch/arm/kvm/interrupts.S | 2 +- > >> >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> >> > >> >> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S > >> >> index 79caf79b304a..87847d2c5f99 100644 > >> >> --- a/arch/arm/kvm/interrupts.S > >> >> +++ b/arch/arm/kvm/interrupts.S > >> >> @@ -309,7 +309,7 @@ ENTRY(kvm_call_hyp) > >> >> THUMB( orr r2, r2, #PSR_T_BIT ) > >> >> msr spsr_cxsf, r2 > >> >> mrs r1, ELR_hyp > >> >> - ldr r2, =BSYM(panic) > >> >> + ldr r2, =panic > >> >> msr ELR_hyp, r2 > >> >> ldr r0, =\panic_str > >> >> clrex @ Clear exclusive monitor > >> >> -- > >> >> 1.8.3.1 > >> >> > >> > Indeed, the linker figures it out as it should. It does seem like the > >> > right result is produced with the BSYM() macro as well so not sure what > >> > the harm is. > >> > > >> > >> BSYM() is defined as 'sym + 1' not 'sym | 1', so if the symbol has the > >> thumb bit set already, the result is incorrect. > >> > > yeah, but the linker will look at the result of 'sym + 1', so on my > > system it ends up with 'sym + 1' after the linker has done its thing > > (verified by looking at the disassembly of vmlinux); > > Hmm, I though had done the same when this was under discussion a > couple of weeks ago, and had arrived at the opposite conclusion, but > now I cannot reproduce anymore so apparently not. > Sorry for the noise. I think that was me confusing myself. In fact, the linker behaves differently depending on the target symbol type. If the relevant symbols are all correctly annoted with .type %function or ENDPROC, the linker should do the right thing without needing any BSYMs. Cheers ---Dave
On Mon, May 11, 2015 at 10:56:57AM +0100, Dave P Martin wrote: > ldr= will do the right thing *if* the target symbol's type is correctly > annotated. This means that ldr =some_local_code_symbol does the right > thing for branch target addresses if and only if some_local_code_symbol > is marked with .type %function (or ENDPROC). > > The fact that a symbol is in a code section is *not* enough. > > For ARM code this never mattered, so local symbols in .S files are > probably under-annotated in general. BSYM() might have been used to > work around this in some cases. > > We should check that all the BSYMs removed by this series from ldr= > and .long/.word etc. point to a correctly annotated symbol, and add > the annotations if not. I guess the problem here is that people forget what a patch series is actually doing and then start making comments based on hypothetical stuff rather than what the series is actually doing. To recap, this series: 1. Removes the wrong usage of BSYM() in the KVM code. This is used with the symbol "panic" which is a function declared by generic C code. Therefore, panic will have the appropriate annotations attached to this symbol, unless GCC itself is buggy. This is the _only_ case of "ldr rd, =BSYM(sym)". 2. It replaces the remaining "adr rd, BSYM(sym)" with a new "badr" macro, and removes the BSYM() preprocessor macro. The new "badr" macro which is identical in behaviour to the former, but ensures that BSYM() doesn't get mis-used with "ldr".
On Mon, May 11, 2015 at 11:17:39AM +0100, Russell King - ARM Linux wrote: > On Mon, May 11, 2015 at 10:56:57AM +0100, Dave P Martin wrote: > > ldr= will do the right thing *if* the target symbol's type is correctly > > annotated. This means that ldr =some_local_code_symbol does the right > > thing for branch target addresses if and only if some_local_code_symbol > > is marked with .type %function (or ENDPROC). > > > > The fact that a symbol is in a code section is *not* enough. > > > > For ARM code this never mattered, so local symbols in .S files are > > probably under-annotated in general. BSYM() might have been used to > > work around this in some cases. > > > > We should check that all the BSYMs removed by this series from ldr= > > and .long/.word etc. point to a correctly annotated symbol, and add > > the annotations if not. > > I guess the problem here is that people forget what a patch series is > actually doing and then start making comments based on hypothetical > stuff rather than what the series is actually doing. > > To recap, this series: > > 1. Removes the wrong usage of BSYM() in the KVM code. This is used with > the symbol "panic" which is a function declared by generic C code. > Therefore, panic will have the appropriate annotations attached to > this symbol, unless GCC itself is buggy. > > This is the _only_ case of "ldr rd, =BSYM(sym)". That's a sound conclusion -- you did mention before that that was the only BSYM with ldr=, but it slipped my mind again, sorry. > 2. It replaces the remaining "adr rd, BSYM(sym)" with a new "badr" macro, > and removes the BSYM() preprocessor macro. The new "badr" macro which > is identical in behaviour to the former, but ensures that BSYM() > doesn't get mis-used with "ldr". And this part wasn't in question anyway, so all is fine. Cheers ---Dave
diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S index 79caf79b304a..87847d2c5f99 100644 --- a/arch/arm/kvm/interrupts.S +++ b/arch/arm/kvm/interrupts.S @@ -309,7 +309,7 @@ ENTRY(kvm_call_hyp) THUMB( orr r2, r2, #PSR_T_BIT ) msr spsr_cxsf, r2 mrs r1, ELR_hyp - ldr r2, =BSYM(panic) + ldr r2, =panic msr ELR_hyp, r2 ldr r0, =\panic_str clrex @ Clear exclusive monitor
BSYM() should only be used when refering to local symbols in the same assembly file which are resolved by the assembler, and not for linker-fixed up symbols. The use of BSYM() with panic is incorrect as the linker is involved in fixing up this relocation, and it knows whether panic() is ARM or Thumb. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- arch/arm/kvm/interrupts.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)