Re: [PATCH] Arm32: avoid .rodata to be marked as executable

[Julien Grall <julien@xxxxxxx>]

Hi,

On 11/06/2021 11:19, Jan Beulich wrote:
> This confuses disassemblers, at the very least. When this data still
> lived in .init.*, this probably didn't matter much, albeit the
> "#execinstr" would have been suspicious to me already then. But the
> latest with their movement to .rodata these attributes should have been
> dropped.
I don't quite understand why this wasn't really a problem for .init.data 
but it is a problem for .rodata. Can you expand your thought?
> Fixes: 9cbe093b7b84 ("xen/arm: link: Link proc_info_list in .rodata instead of 
> .init.data")
I don't view this commit as the buggy one. I would prefer if there is no 
Fixes tag. But if you want one, then it should be the patch that 
introduced #execinstr.
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
The change below looks good to me. But I don't understand the commit 
message, so I will wait an answer before acking it.
> ---
> The PRINT() macro in head.S is another source of such confusion of code
> vs data. While in head.o there are mapping symbols guiding disassemblers,
> these mapping symbols are gone when looking at xen-syms. But I realize
> adr's reach is too limited to allow for a halfway reasonable approach of
> moving those strings (e.g. to, at least, have them all together).
I have tried it in the past. The solution I have was to differentiate 
call with MMU on from the one with MMU off. But I considered this wasn't 
worth the trouble.
> --- a/xen/arch/arm/arm32/proc-v7.S
> +++ b/xen/arch/arm/arm32/proc-v7.S
> @@ -29,7 +29,7 @@ brahma15mp_init:
>           mcr   CP32(r0, ACTLR)
>           mov   pc, lr
>   
> -        .section ".proc.info", #alloc, #execinstr
> +        .section ".proc.info", #alloc
>           .type __v7_ca15mp_proc_info, #object
>   __v7_ca15mp_proc_info:
>           .long 0x410FC0F0             /* Cortex-A15 */
> @@ -38,7 +38,7 @@ __v7_ca15mp_proc_info:
>           .long caxx_processor
>           .size __v7_ca15mp_proc_info, . - __v7_ca15mp_proc_info
>   
> -        .section ".proc.info", #alloc, #execinstr
> +        .section ".proc.info", #alloc
>           .type __v7_ca7mp_proc_info, #object
>   __v7_ca7mp_proc_info:
>           .long 0x410FC070             /* Cortex-A7 */
> @@ -47,7 +47,7 @@ __v7_ca7mp_proc_info:
>           .long caxx_processor
>           .size __v7_ca7mp_proc_info, . - __v7_ca7mp_proc_info
>   
> -        .section ".proc.info", #alloc, #execinstr
> +        .section ".proc.info", #alloc
>           .type __v7_brahma15mp_proc_info, #object
>   __v7_brahma15mp_proc_info:
>           .long 0x420F00F0             /* Broadcom Brahma-B15 */
Cheers,

--
Julien Grall