This confuses disassemblers, at the very least. Move
.altinstr_replacement to .init.text. The previously redundant ALIGN()
now gets converted to page alignment, such that the hypervisor mapping
won't have this as executable (it'll instead get mapped r/w, which I'm
told is intended to be adjusted at some point).

Note that for the actual patching logic's purposes this part of
.init.text _has_ to live after _einittext (or before _sinittext), or
else branch_insn_requires_update() would produce wrong results.

Also, to have .altinstr_replacement have consistent attributes in the
object files, add "x" to the one instance where it was missing.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Put past _einittext.

--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -148,6 +148,8 @@ SECTIONS
        _sinittext = .;
        *(.init.text)
        _einittext = .;
+       . = ALIGN(PAGE_SIZE);        /* Avoid mapping alt insns executable */
+       *(.altinstr_replacement)
   } :text
   . = ALIGN(PAGE_SIZE);
   .init.data : {
@@ -169,8 +171,6 @@ SECTIONS
        __alt_instructions = .;
        *(.altinstructions)
        __alt_instructions_end = .;
-       . = ALIGN(4);
-       *(.altinstr_replacement)
 
 #ifdef CONFIG_DEBUG_LOCK_PROFILE
        . = ALIGN(POINTER_ALIGN);
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -67,7 +67,7 @@ int apply_alternatives(const struct alt_
 	ALTINSTR_ENTRY(feature,cb)					\
 	".popsection\n"							\
 	" .if " __stringify(cb) " == 0\n"				\
-	".pushsection .altinstr_replacement, \"a\"\n"			\
+	".pushsection .altinstr_replacement, \"ax\"\n"			\
 	"663:\n\t"							\
 	newinstr "\n"							\
 	"664:\n\t"							\

Hi Jan,

On 14/06/2021 15:52, Jan Beulich wrote:
> This confuses disassemblers, at the very least. Move
> .altinstr_replacement to .init.text. The previously redundant ALIGN()
> now gets converted to page alignment, such that the hypervisor mapping
> won't have this as executable (it'll instead get mapped r/w, which I'm
> told is intended to be adjusted at some point).
> 
> Note that for the actual patching logic's purposes this part of
> .init.text _has_ to live after _einittext (or before _sinittext), or
> else branch_insn_requires_update() would produce wrong results.
> 
> Also, to have .altinstr_replacement have consistent attributes in the
> object files, add "x" to the one instance where it was missing.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

> ---
> v2: Put past _einittext.
> 
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -148,6 +148,8 @@ SECTIONS
>          _sinittext = .;
>          *(.init.text)
>          _einittext = .;
> +       . = ALIGN(PAGE_SIZE);        /* Avoid mapping alt insns executable */
> +       *(.altinstr_replacement)
>     } :text
>     . = ALIGN(PAGE_SIZE);
>     .init.data : {
> @@ -169,8 +171,6 @@ SECTIONS
>          __alt_instructions = .;
>          *(.altinstructions)
>          __alt_instructions_end = .;
> -       . = ALIGN(4);
> -       *(.altinstr_replacement)
>   
>   #ifdef CONFIG_DEBUG_LOCK_PROFILE
>          . = ALIGN(POINTER_ALIGN);
> --- a/xen/include/asm-arm/alternative.h
> +++ b/xen/include/asm-arm/alternative.h
> @@ -67,7 +67,7 @@ int apply_alternatives(const struct alt_
>   	ALTINSTR_ENTRY(feature,cb)					\
>   	".popsection\n"							\
>   	" .if " __stringify(cb) " == 0\n"				\
> -	".pushsection .altinstr_replacement, \"a\"\n"			\
> +	".pushsection .altinstr_replacement, \"ax\"\n"			\
>   	"663:\n\t"							\
>   	newinstr "\n"							\
>   	"664:\n\t"							\
> 

-- 
Julien Grall

On 14/06/2021 15:54, Julien Grall wrote:
> Hi Jan,
> 
> On 14/06/2021 15:52, Jan Beulich wrote:
>> This confuses disassemblers, at the very least. Move
>> .altinstr_replacement to .init.text. The previously redundant ALIGN()
>> now gets converted to page alignment, such that the hypervisor mapping
>> won't have this as executable (it'll instead get mapped r/w, which I'm
>> told is intended to be adjusted at some point).
>>
>> Note that for the actual patching logic's purposes this part of
>> .init.text _has_ to live after _einittext (or before _sinittext), or
>> else branch_insn_requires_update() would produce wrong results.
>>
>> Also, to have .altinstr_replacement have consistent attributes in the
>> object files, add "x" to the one instance where it was missing.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

Comitted.

Cheers,

-- 
Julien Grall