* [PATCH] arm64: fix relocation of movz instruction with negative immediate
@ 2016-01-04 16:09 Ard Biesheuvel
2016-01-04 17:21 ` Dave Martin
0 siblings, 1 reply; 5+ messages in thread
From: Ard Biesheuvel @ 2016-01-04 16:09 UTC (permalink / raw)
To: linux-arm-kernel
The test whether a movz instruction with a signed immediate should be
turned into a movn instruction (i.e., when the immediate is negative)
is flawed, since the value of imm is always positive. So check sval
instead.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
arch/arm64/kernel/module.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index f4bc779e62e8..39e4a29cab50 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -128,7 +128,7 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val,
* immediate is less than zero.
*/
insn &= ~(3 << 29);
- if ((s64)imm >= 0) {
+ if (sval >= 0) {
/* >=0: Set the instruction to MOVZ (opcode 10b). */
insn |= 2 << 29;
} else {
--
2.5.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] arm64: fix relocation of movz instruction with negative immediate
2016-01-04 16:09 [PATCH] arm64: fix relocation of movz instruction with negative immediate Ard Biesheuvel
@ 2016-01-04 17:21 ` Dave Martin
2016-01-04 17:33 ` Ard Biesheuvel
2016-01-04 17:48 ` Will Deacon
0 siblings, 2 replies; 5+ messages in thread
From: Dave Martin @ 2016-01-04 17:21 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jan 04, 2016 at 05:09:22PM +0100, Ard Biesheuvel wrote:
> The test whether a movz instruction with a signed immediate should be
> turned into a movn instruction (i.e., when the immediate is negative)
> is flawed, since the value of imm is always positive. So check sval
> instead.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> arch/arm64/kernel/module.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index f4bc779e62e8..39e4a29cab50 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -128,7 +128,7 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val,
#define AARCH64_INSN_IMM_MOVNZ AARCH64_INSN_IMM_MAX
#define AARCH64_INSN_IMM_MOVK AARCH64_INSN_IMM_16
/* ... */
if (imm_type == AARCH64_INSN_IMM_MOVNZ) {
/* ... */
> * immediate is less than zero.
> */
> insn &= ~(3 << 29);
> - if ((s64)imm >= 0) {
> + if (sval >= 0) {
> /* >=0: Set the instruction to MOVZ (opcode 10b). */
> insn |= 2 << 29;
> } else {
I _think_ this may be correct, but...
}
imm_type = AARCH64_INSN_IMM_MOVK;
}
/* Update the instruction with the new encoding. */
insn = aarch64_insn_encode_immediate(imm_type, insn, imm);
/* ... */
leaves imm_type as either AARCH64_INSN_IMM_16 or AARCH64_INSN_IMM_MOVK.
But because AARCH64_INSN_IMM_16 == AARCH64_INSN_IMM_MOVK (required for , the negative
overflow fudge is never applied, no?
if (imm_type != AARCH64_INSN_IMM_16) {
sval++;
limit++;
}
I'm wondering whether there is a less confusing way to do all this...
Cheers
---Dave
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] arm64: fix relocation of movz instruction with negative immediate
2016-01-04 17:21 ` Dave Martin
@ 2016-01-04 17:33 ` Ard Biesheuvel
2016-01-04 18:00 ` Dave Martin
2016-01-04 17:48 ` Will Deacon
1 sibling, 1 reply; 5+ messages in thread
From: Ard Biesheuvel @ 2016-01-04 17:33 UTC (permalink / raw)
To: linux-arm-kernel
On 4 January 2016 at 18:21, Dave Martin <Dave.Martin@arm.com> wrote:
> On Mon, Jan 04, 2016 at 05:09:22PM +0100, Ard Biesheuvel wrote:
>> The test whether a movz instruction with a signed immediate should be
>> turned into a movn instruction (i.e., when the immediate is negative)
>> is flawed, since the value of imm is always positive. So check sval
>> instead.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> arch/arm64/kernel/module.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
>> index f4bc779e62e8..39e4a29cab50 100644
>> --- a/arch/arm64/kernel/module.c
>> +++ b/arch/arm64/kernel/module.c
>> @@ -128,7 +128,7 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val,
>
> #define AARCH64_INSN_IMM_MOVNZ AARCH64_INSN_IMM_MAX
> #define AARCH64_INSN_IMM_MOVK AARCH64_INSN_IMM_16
>
> /* ... */
>
> if (imm_type == AARCH64_INSN_IMM_MOVNZ) {
>
> /* ... */
>
>> * immediate is less than zero.
>> */
>> insn &= ~(3 << 29);
>> - if ((s64)imm >= 0) {
>> + if (sval >= 0) {
>> /* >=0: Set the instruction to MOVZ (opcode 10b). */
>> insn |= 2 << 29;
>> } else {
>
> I _think_ this may be correct, but...
>
> }
> imm_type = AARCH64_INSN_IMM_MOVK;
> }
>
> /* Update the instruction with the new encoding. */
> insn = aarch64_insn_encode_immediate(imm_type, insn, imm);
>
> /* ... */
>
> leaves imm_type as either AARCH64_INSN_IMM_16 or AARCH64_INSN_IMM_MOVK.
>
> But because AARCH64_INSN_IMM_16 == AARCH64_INSN_IMM_MOVK (required for , the negative
> overflow fudge is never applied, no?
>
> if (imm_type != AARCH64_INSN_IMM_16) {
> sval++;
> limit++;
> }
>
>
> I'm wondering whether there is a less confusing way to do all this...
>
I hadn't spotted that AARCH64_INSN_IMM_16 == AARCH64_INSN_IMM_MOVK, so
you are correct that my single change is not sufficient to fix this
code.
Let me try to come up with something better ..
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] arm64: fix relocation of movz instruction with negative immediate
2016-01-04 17:21 ` Dave Martin
2016-01-04 17:33 ` Ard Biesheuvel
@ 2016-01-04 17:48 ` Will Deacon
1 sibling, 0 replies; 5+ messages in thread
From: Will Deacon @ 2016-01-04 17:48 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jan 04, 2016 at 05:21:12PM +0000, Dave Martin wrote:
> On Mon, Jan 04, 2016 at 05:09:22PM +0100, Ard Biesheuvel wrote:
> > The test whether a movz instruction with a signed immediate should be
> > turned into a movn instruction (i.e., when the immediate is negative)
> > is flawed, since the value of imm is always positive. So check sval
> > instead.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> > arch/arm64/kernel/module.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> > index f4bc779e62e8..39e4a29cab50 100644
> > --- a/arch/arm64/kernel/module.c
> > +++ b/arch/arm64/kernel/module.c
> > @@ -128,7 +128,7 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val,
>
> #define AARCH64_INSN_IMM_MOVNZ AARCH64_INSN_IMM_MAX
> #define AARCH64_INSN_IMM_MOVK AARCH64_INSN_IMM_16
>
> /* ... */
>
> if (imm_type == AARCH64_INSN_IMM_MOVNZ) {
>
> /* ... */
>
> > * immediate is less than zero.
> > */
> > insn &= ~(3 << 29);
> > - if ((s64)imm >= 0) {
> > + if (sval >= 0) {
> > /* >=0: Set the instruction to MOVZ (opcode 10b). */
> > insn |= 2 << 29;
> > } else {
>
> I _think_ this may be correct, but...
Yeah, I think this is the right thing to do.
> }
> imm_type = AARCH64_INSN_IMM_MOVK;
> }
>
> /* Update the instruction with the new encoding. */
> insn = aarch64_insn_encode_immediate(imm_type, insn, imm);
>
> /* ... */
>
> leaves imm_type as either AARCH64_INSN_IMM_16 or AARCH64_INSN_IMM_MOVK.
>
> But because AARCH64_INSN_IMM_16 == AARCH64_INSN_IMM_MOVK (required for , the negative
> overflow fudge is never applied, no?
>
> if (imm_type != AARCH64_INSN_IMM_16) {
> sval++;
> limit++;
> }
Hmm, that's a bug introduced by the refactoring of the insn encoding
stuff in c84fced8d990 ("arm64: move encode_insn_immediate() from module.c
to insn.c"). I've restored the old behaviour below.
> I'm wondering whether there is a less confusing way to do all this...
Patches welcome! I didn't have an ELF spec when I wrote the original
code, so it might be easier now. It would also be handy to have a test
module that uses lots of relocs...
Will
--->8
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 266e7490e85c..6546032bb83b 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -140,11 +140,10 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val,
*/
imm = ~imm;
}
- imm_type = AARCH64_INSN_IMM_MOVK;
}
/* Update the instruction with the new encoding. */
- insn = aarch64_insn_encode_immediate(imm_type, insn, imm);
+ insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_16, insn, imm);
*(u32 *)place = cpu_to_le32(insn);
/* Shift out the immediate field. */
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] arm64: fix relocation of movz instruction with negative immediate
2016-01-04 17:33 ` Ard Biesheuvel
@ 2016-01-04 18:00 ` Dave Martin
0 siblings, 0 replies; 5+ messages in thread
From: Dave Martin @ 2016-01-04 18:00 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jan 04, 2016 at 06:33:03PM +0100, Ard Biesheuvel wrote:
> On 4 January 2016 at 18:21, Dave Martin <Dave.Martin@arm.com> wrote:
> > On Mon, Jan 04, 2016 at 05:09:22PM +0100, Ard Biesheuvel wrote:
> >> The test whether a movz instruction with a signed immediate should be
> >> turned into a movn instruction (i.e., when the immediate is negative)
> >> is flawed, since the value of imm is always positive. So check sval
> >> instead.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >> arch/arm64/kernel/module.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> >> index f4bc779e62e8..39e4a29cab50 100644
> >> --- a/arch/arm64/kernel/module.c
> >> +++ b/arch/arm64/kernel/module.c
> >> @@ -128,7 +128,7 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, void *place, u64 val,
> >
> > #define AARCH64_INSN_IMM_MOVNZ AARCH64_INSN_IMM_MAX
> > #define AARCH64_INSN_IMM_MOVK AARCH64_INSN_IMM_16
> >
> > /* ... */
> >
> > if (imm_type == AARCH64_INSN_IMM_MOVNZ) {
> >
> > /* ... */
> >
> >> * immediate is less than zero.
> >> */
> >> insn &= ~(3 << 29);
> >> - if ((s64)imm >= 0) {
> >> + if (sval >= 0) {
> >> /* >=0: Set the instruction to MOVZ (opcode 10b). */
> >> insn |= 2 << 29;
> >> } else {
> >
> > I _think_ this may be correct, but...
> >
> > }
> > imm_type = AARCH64_INSN_IMM_MOVK;
> > }
> >
> > /* Update the instruction with the new encoding. */
> > insn = aarch64_insn_encode_immediate(imm_type, insn, imm);
> >
> > /* ... */
> >
> > leaves imm_type as either AARCH64_INSN_IMM_16 or AARCH64_INSN_IMM_MOVK.
> >
> > But because AARCH64_INSN_IMM_16 == AARCH64_INSN_IMM_MOVK (required for , the negative
> > overflow fudge is never applied, no?
> >
> > if (imm_type != AARCH64_INSN_IMM_16) {
> > sval++;
> > limit++;
> > }
> >
> >
> > I'm wondering whether there is a less confusing way to do all this...
> >
>
> I hadn't spotted that AARCH64_INSN_IMM_16 == AARCH64_INSN_IMM_MOVK, so
> you are correct that my single change is not sufficient to fix this
> code.
>
> Let me try to come up with something better ..
I think a simpler flow might go something like
u64 uval = (u64)do_reloc(op, place, val);
if (is a SABS or PREL reloc that is not _NC, && (uval & (1 << 63))) {
change insn to MOVN;
uval = ~uval;
}
/* encode the insn using the bottom 16 bits of uval */
if (not an _NC reloc && (uval >> 16) > 0)
return -ERANGE;
...or something along those lines.
I haven't tried to implement that yet, though.
Cheers
---Dave
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-01-04 18:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-04 16:09 [PATCH] arm64: fix relocation of movz instruction with negative immediate Ard Biesheuvel
2016-01-04 17:21 ` Dave Martin
2016-01-04 17:33 ` Ard Biesheuvel
2016-01-04 18:00 ` Dave Martin
2016-01-04 17:48 ` Will Deacon
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.