| Message ID | 1473626125-13683-6-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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,
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); 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);
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 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(-)