* [PATCH 1/2] ARM: kvm: fix a bad BSYM() usage
@ 2015-05-11 9:05 ` Christoffer Dall
0 siblings, 0 replies; 26+ messages in thread
From: Christoffer Dall @ 2015-05-11 9:05 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] ARM: kvm: fix a bad BSYM() usage
2015-05-11 9:05 ` Christoffer Dall
@ 2015-05-11 9:44 ` Ard Biesheuvel
-1 siblings, 0 replies; 26+ messages in thread
From: Ard Biesheuvel @ 2015-05-11 9:44 UTC (permalink / raw)
To: Christoffer Dall
Cc: Russell King, KVM devel mailing list, Nicolas Pitre, Marc Zyngier,
kvmarm@lists.cs.columbia.edu, Gleb Natapov, Paolo Bonzini,
Dave P Martin, linux-arm-kernel@lists.infradead.org
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] ARM: kvm: fix a bad BSYM() usage
@ 2015-05-11 9:44 ` Ard Biesheuvel
0 siblings, 0 replies; 26+ messages in thread
From: Ard Biesheuvel @ 2015-05-11 9:44 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] ARM: kvm: fix a bad BSYM() usage
2015-05-11 9:44 ` Ard Biesheuvel
@ 2015-05-11 10:07 ` Dave P Martin
-1 siblings, 0 replies; 26+ messages in thread
From: Dave P Martin @ 2015-05-11 10:07 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Christoffer Dall, Russell King, KVM devel mailing list,
Nicolas Pitre, Marc Zyngier, kvmarm@lists.cs.columbia.edu,
Gleb Natapov, Paolo Bonzini, linux-arm-kernel@lists.infradead.org
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] ARM: kvm: fix a bad BSYM() usage
@ 2015-05-11 10:07 ` Dave P Martin
0 siblings, 0 replies; 26+ messages in thread
From: Dave P Martin @ 2015-05-11 10:07 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] ARM: kvm: fix a bad BSYM() usage
2015-05-11 9:05 ` Christoffer Dall
@ 2015-05-11 9:56 ` Dave P Martin
-1 siblings, 0 replies; 26+ messages in thread
From: Dave P Martin @ 2015-05-11 9:56 UTC (permalink / raw)
To: Christoffer Dall
Cc: Ard Biesheuvel, Russell King, KVM devel mailing list,
Nicolas Pitre, Marc Zyngier, kvmarm@lists.cs.columbia.edu,
Gleb Natapov, Paolo Bonzini, linux-arm-kernel@lists.infradead.org
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] ARM: kvm: fix a bad BSYM() usage
@ 2015-05-11 9:56 ` Dave P Martin
0 siblings, 0 replies; 26+ messages in thread
From: Dave P Martin @ 2015-05-11 9:56 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] ARM: kvm: fix a bad BSYM() usage
2015-05-11 9:56 ` Dave P Martin
@ 2015-05-11 10:17 ` Russell King - ARM Linux
-1 siblings, 0 replies; 26+ messages in thread
From: Russell King - ARM Linux @ 2015-05-11 10:17 UTC (permalink / raw)
To: Dave P Martin
Cc: Christoffer Dall, Ard Biesheuvel, KVM devel mailing list,
Nicolas Pitre, Marc Zyngier, kvmarm@lists.cs.columbia.edu,
Gleb Natapov, Paolo Bonzini, linux-arm-kernel@lists.infradead.org
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".
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] ARM: kvm: fix a bad BSYM() usage
@ 2015-05-11 10:17 ` Russell King - ARM Linux
0 siblings, 0 replies; 26+ messages in thread
From: Russell King - ARM Linux @ 2015-05-11 10:17 UTC (permalink / raw)
To: linux-arm-kernel
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".
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] ARM: kvm: fix a bad BSYM() usage
2015-05-11 10:17 ` Russell King - ARM Linux
@ 2015-05-11 10:27 ` Dave P Martin
-1 siblings, 0 replies; 26+ messages in thread
From: Dave P Martin @ 2015-05-11 10:27 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Christoffer Dall, Ard Biesheuvel, KVM devel mailing list,
Nicolas Pitre, Marc Zyngier, kvmarm@lists.cs.columbia.edu,
Gleb Natapov, Paolo Bonzini, linux-arm-kernel@lists.infradead.org
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] ARM: kvm: fix a bad BSYM() usage
@ 2015-05-11 10:27 ` Dave P Martin
0 siblings, 0 replies; 26+ messages in thread
From: Dave P Martin @ 2015-05-11 10:27 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 26+ messages in thread