Re: [PATCH v3 05/18] arm: poison initmem when it is freed.

[Julien Grall <julien.grall@arm.com>]

Hi Konrad,

On 11/09/2016 22:35, Konrad Rzeszutek Wilk wrote:
> The current byte sequence is '0xcc' which makes sense on x86,
> but on ARM it is:
>
> cccccccc        stclgt  12, cr12, [ip], {204}   ; 0xcc
>
> Picking something more ARM applicable such as:
>
> efefefef        svc     0x00efefef

Whilst I agree it is much better to get a trap rather than executing a 
random instruction (though co-processor 12 is reserved and a trap should 
(?) occur), I see two problems with this choice.

Firstly, it might be possible that in the future we decide to use svc in 
the hypervisor (it is dumb but valid use case).

Secondly, AArch64 is using a different set (and therefore encoding). So 
far this encoding is not allocated, but it is not rule out that this 
encoding will not be used in the future.

So I would suggest to point initmem with:
     - AArch32: udf instruction i.e 0xe7f000f0 (see A8.8.247 in ARM DDI 
0406C.c)
     - AArch64: a break point (possibly the encoding AARCH64_BREAK_FAULT 
(see asm-arm/arm64/brk.h).

What do you think?

>
> Creates a nice crash if one executes that code:
> (XEN) CPU1: Unexpected Trap: Supervisor Call
>
> We don't have to worry about Thumb code so this instruction
> is a safe to execute.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> ---
>  xen/arch/arm/mm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 07e2037..0fa5623 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -995,7 +995,7 @@ void free_init_memory(void)
>      paddr_t pa = virt_to_maddr(__init_begin);
>      unsigned long len = __init_end - __init_begin;
>      set_pte_flags_on_range(__init_begin, len, mg_rw);
> -    memset(__init_begin, 0xcc, len);
> +    memset(__init_begin, 0xef, len);

Regardless the final decision, I think we should document the meaning of 
the value in the code.

>      set_pte_flags_on_range(__init_begin, len, mg_clear);
>      init_domheap_pages(pa, pa + len);
>      printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
>

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel