Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1465274 > unrolled thread

[PATCH 5/8] arm64: alternative: Add support for patching adrp instructions

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 5/8] arm64: alternative: Add support for patching adrp instructions Suzuki K Poulose <suzuki.poulose@arm.com> - 2016-08-18 15:20 +0200
    Re: [PATCH 5/8] arm64: alternative: Add support for patching adrp  instructions Will Deacon <will.deacon@arm.com> - 2016-08-22 13:20 +0200
      Re: [PATCH 5/8] arm64: alternative: Add support for patching adrp  instructions Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2016-08-23 11:50 +0200
    Re: [PATCH 5/8] arm64: alternative: Add support for patching adrp instructions Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-08-22 13:50 +0200
      Re: [PATCH 5/8] arm64: alternative: Add support for patching adrp  instructions Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2016-08-23 11:20 +0200
        Re: [PATCH 5/8] arm64: alternative: Add support for patching adrp instructions Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-08-23 13:40 +0200

#1465274 — [PATCH 5/8] arm64: alternative: Add support for patching adrp instructions

adrp uses PC-relative address offset to a page (of 4K size) of
a symbol. If it appears in an alternative code patched in, we
should adjust the offset to reflect the address where it will
be run from. This patch adds support for fixing the offset
for adrp instructions.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/alternative.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index d2ee1b2..71c6962 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -80,6 +80,19 @@ static u32 get_alt_insn(struct alt_instr *alt, u32 *insnptr, u32 *altinsnptr)
 			offset = target - (unsigned long)insnptr;
 			insn = aarch64_set_branch_offset(insn, offset);
 		}
+	} else if (aarch64_insn_is_adrp(insn)) {
+		s32 orig_offset, new_offset;
+		unsigned long target;
+
+		/*
+		 * If we're replacing an adrp instruction, which uses PC-relative
+		 * immediate addressing, adjust the offset to reflect the new
+		 * PC. adrp operates on 4K aligned addresses.
+		 */
+		orig_offset  = aarch64_insn_adrp_get_offset(insn);
+		target = ((unsigned long)altinsnptr & ~0xfffUL) + orig_offset;
+		new_offset = target - ((unsigned long)insnptr & ~0xfffUL);
+		insn = aarch64_insn_adrp_set_offset(insn, new_offset);
 	}
 
 	return insn;
-- 
2.7.4

[toc] | [next] | [standalone]

#1467555 — Re: [PATCH 5/8] arm64: alternative: Add support for patching adrp instructions

On Thu, Aug 18, 2016 at 02:10:29PM +0100, Suzuki K Poulose wrote:
> adrp uses PC-relative address offset to a page (of 4K size) of
> a symbol. If it appears in an alternative code patched in, we
> should adjust the offset to reflect the address where it will
> be run from. This patch adds support for fixing the offset
> for adrp instructions.
> 
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Andre Przywara <andre.przywara@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/kernel/alternative.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
> index d2ee1b2..71c6962 100644
> --- a/arch/arm64/kernel/alternative.c
> +++ b/arch/arm64/kernel/alternative.c
> @@ -80,6 +80,19 @@ static u32 get_alt_insn(struct alt_instr *alt, u32 *insnptr, u32 *altinsnptr)
>  			offset = target - (unsigned long)insnptr;
>  			insn = aarch64_set_branch_offset(insn, offset);
>  		}
> +	} else if (aarch64_insn_is_adrp(insn)) {
> +		s32 orig_offset, new_offset;
> +		unsigned long target;
> +
> +		/*
> +		 * If we're replacing an adrp instruction, which uses PC-relative
> +		 * immediate addressing, adjust the offset to reflect the new
> +		 * PC. adrp operates on 4K aligned addresses.
> +		 */
> +		orig_offset  = aarch64_insn_adrp_get_offset(insn);
> +		target = ((unsigned long)altinsnptr & ~0xfffUL) + orig_offset;
> +		new_offset = target - ((unsigned long)insnptr & ~0xfffUL);

The masking with ~0xfffUL might be nicer if you write it as
align_down(ptr, SZ_4K);

> +		insn = aarch64_insn_adrp_set_offset(insn, new_offset);
>  	}
>  
>  	return insn;

I wonder if we shouldn't have a catch-all for any instructions performing
PC-relative operations here, because silent corruption of the instruction
stream is pretty horrible. What other instructions are there? ADR, LDR
(literal), ... ?

Will

[toc] | [prev] | [next] | [standalone]

#1468442 — Re: [PATCH 5/8] arm64: alternative: Add support for patching adrp instructions

On 22/08/16 12:19, Will Deacon wrote:
> On Thu, Aug 18, 2016 at 02:10:29PM +0100, Suzuki K Poulose wrote:
>> adrp uses PC-relative address offset to a page (of 4K size) of
>> a symbol. If it appears in an alternative code patched in, we
>> should adjust the offset to reflect the address where it will
>> be run from. This patch adds support for fixing the offset
>> for adrp instructions.
>>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Andre Przywara <andre.przywara@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>  arch/arm64/kernel/alternative.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
>> index d2ee1b2..71c6962 100644
>> --- a/arch/arm64/kernel/alternative.c
>> +++ b/arch/arm64/kernel/alternative.c
>> @@ -80,6 +80,19 @@ static u32 get_alt_insn(struct alt_instr *alt, u32 *insnptr, u32 *altinsnptr)
>>  			offset = target - (unsigned long)insnptr;
>>  			insn = aarch64_set_branch_offset(insn, offset);
>>  		}
>> +	} else if (aarch64_insn_is_adrp(insn)) {
>> +		s32 orig_offset, new_offset;
>> +		unsigned long target;
>> +
>> +		/*
>> +		 * If we're replacing an adrp instruction, which uses PC-relative
>> +		 * immediate addressing, adjust the offset to reflect the new
>> +		 * PC. adrp operates on 4K aligned addresses.
>> +		 */
>> +		orig_offset  = aarch64_insn_adrp_get_offset(insn);
>> +		target = ((unsigned long)altinsnptr & ~0xfffUL) + orig_offset;
>> +		new_offset = target - ((unsigned long)insnptr & ~0xfffUL);
>
> The masking with ~0xfffUL might be nicer if you write it as
> align_down(ptr, SZ_4K);

Right, that definitely looks better. Will change it.

>
>> +		insn = aarch64_insn_adrp_set_offset(insn, new_offset);
>>  	}
>>
>>  	return insn;
>
> I wonder if we shouldn't have a catch-all for any instructions performing
> PC-relative operations here, because silent corruption of the instruction

Correct, which is what happened initially when I didn't have the adrp handling ;-).

> stream is pretty horrible. What other instructions are there? ADR, LDR
> (literal), ... ?

 From a quick look, all the instructions under "Load register (literal)" :

i.e,
LDR (literal) for GPR/FP_SIMD/32bit/64bit
LDRSW (literal)
PRFM (literal)

and Data processing instructions - immediate group with PC-relative addressing:

ADR, ADRP

I will add a check to catch the unsupported instructions in the alternative code.

Thanks
Suzuki

[toc] | [prev] | [next] | [standalone]

#1467564

On 18 August 2016 at 15:10, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> adrp uses PC-relative address offset to a page (of 4K size) of
> a symbol. If it appears in an alternative code patched in, we
> should adjust the offset to reflect the address where it will
> be run from. This patch adds support for fixing the offset
> for adrp instructions.
>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Andre Przywara <andre.przywara@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/kernel/alternative.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
> index d2ee1b2..71c6962 100644
> --- a/arch/arm64/kernel/alternative.c
> +++ b/arch/arm64/kernel/alternative.c
> @@ -80,6 +80,19 @@ static u32 get_alt_insn(struct alt_instr *alt, u32 *insnptr, u32 *altinsnptr)
>                         offset = target - (unsigned long)insnptr;
>                         insn = aarch64_set_branch_offset(insn, offset);
>                 }
> +       } else if (aarch64_insn_is_adrp(insn)) {
> +               s32 orig_offset, new_offset;
> +               unsigned long target;
> +
> +               /*
> +                * If we're replacing an adrp instruction, which uses PC-relative
> +                * immediate addressing, adjust the offset to reflect the new
> +                * PC. adrp operates on 4K aligned addresses.
> +                */
> +               orig_offset  = aarch64_insn_adrp_get_offset(insn);
> +               target = ((unsigned long)altinsnptr & ~0xfffUL) + orig_offset;
> +               new_offset = target - ((unsigned long)insnptr & ~0xfffUL);
> +               insn = aarch64_insn_adrp_set_offset(insn, new_offset);

Are orig_offset and new_offset guaranteed to be equal modulo 4 KB?
Otherwise, you will have to track down and patch the associated :lo12:
add/ldr instruction as well.

[toc] | [prev] | [next] | [standalone]

#1468428 — Re: [PATCH 5/8] arm64: alternative: Add support for patching adrp instructions

On 22/08/16 12:45, Ard Biesheuvel wrote:
> On 18 August 2016 at 15:10, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>> adrp uses PC-relative address offset to a page (of 4K size) of
>> a symbol. If it appears in an alternative code patched in, we
>> should adjust the offset to reflect the address where it will
>> be run from. This patch adds support for fixing the offset
>> for adrp instructions.
>>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Andre Przywara <andre.przywara@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>  arch/arm64/kernel/alternative.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
>> index d2ee1b2..71c6962 100644
>> --- a/arch/arm64/kernel/alternative.c
>> +++ b/arch/arm64/kernel/alternative.c
>> @@ -80,6 +80,19 @@ static u32 get_alt_insn(struct alt_instr *alt, u32 *insnptr, u32 *altinsnptr)
>>                         offset = target - (unsigned long)insnptr;
>>                         insn = aarch64_set_branch_offset(insn, offset);
>>                 }
>> +       } else if (aarch64_insn_is_adrp(insn)) {
>> +               s32 orig_offset, new_offset;
>> +               unsigned long target;
>> +
>> +               /*
>> +                * If we're replacing an adrp instruction, which uses PC-relative
>> +                * immediate addressing, adjust the offset to reflect the new
>> +                * PC. adrp operates on 4K aligned addresses.
>> +                */
>> +               orig_offset  = aarch64_insn_adrp_get_offset(insn);
>> +               target = ((unsigned long)altinsnptr & ~0xfffUL) + orig_offset;
>> +               new_offset = target - ((unsigned long)insnptr & ~0xfffUL);
>> +               insn = aarch64_insn_adrp_set_offset(insn, new_offset);
>
> Are orig_offset and new_offset guaranteed to be equal modulo 4 KB?
> Otherwise, you will have to track down and patch the associated :lo12:
> add/ldr instruction as well.

We are modifying the alternative instruction to accommodate for the new PC,
where this instruction will be executed from, while the referenced symbol
remains the same. Hence the associated :lo12: doesn't change. Does that
address your concern ? Or did I miss something ?

Suzuki

[toc] | [prev] | [next] | [standalone]

#1468498

On 23 August 2016 at 11:16, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 22/08/16 12:45, Ard Biesheuvel wrote:
>>
>> On 18 August 2016 at 15:10, Suzuki K Poulose <suzuki.poulose@arm.com>
>> wrote:
>>>
>>> adrp uses PC-relative address offset to a page (of 4K size) of
>>> a symbol. If it appears in an alternative code patched in, we
>>> should adjust the offset to reflect the address where it will
>>> be run from. This patch adds support for fixing the offset
>>> for adrp instructions.
>>>
>>> Cc: Will Deacon <will.deacon@arm.com>
>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>> Cc: Andre Przywara <andre.przywara@arm.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> ---
>>>  arch/arm64/kernel/alternative.c | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/arch/arm64/kernel/alternative.c
>>> b/arch/arm64/kernel/alternative.c
>>> index d2ee1b2..71c6962 100644
>>> --- a/arch/arm64/kernel/alternative.c
>>> +++ b/arch/arm64/kernel/alternative.c
>>> @@ -80,6 +80,19 @@ static u32 get_alt_insn(struct alt_instr *alt, u32
>>> *insnptr, u32 *altinsnptr)
>>>                         offset = target - (unsigned long)insnptr;
>>>                         insn = aarch64_set_branch_offset(insn, offset);
>>>                 }
>>> +       } else if (aarch64_insn_is_adrp(insn)) {
>>> +               s32 orig_offset, new_offset;
>>> +               unsigned long target;
>>> +
>>> +               /*
>>> +                * If we're replacing an adrp instruction, which uses
>>> PC-relative
>>> +                * immediate addressing, adjust the offset to reflect the
>>> new
>>> +                * PC. adrp operates on 4K aligned addresses.
>>> +                */
>>> +               orig_offset  = aarch64_insn_adrp_get_offset(insn);
>>> +               target = ((unsigned long)altinsnptr & ~0xfffUL) +
>>> orig_offset;
>>> +               new_offset = target - ((unsigned long)insnptr &
>>> ~0xfffUL);
>>> +               insn = aarch64_insn_adrp_set_offset(insn, new_offset);
>>
>>
>> Are orig_offset and new_offset guaranteed to be equal modulo 4 KB?
>> Otherwise, you will have to track down and patch the associated :lo12:
>> add/ldr instruction as well.
>
>
> We are modifying the alternative instruction to accommodate for the new PC,
> where this instruction will be executed from, while the referenced symbol
> remains the same. Hence the associated :lo12: doesn't change. Does that
> address your concern ? Or did I miss something ?
>

Ah, of course. Yes, that should work fine, given that the symbol stays
in place, and so its offset into the containing 4 KB page does not
change either.

Thanks,
Ard.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web