Re: [PATCH 12/12] xen/arm: add cache coloring support for Xen

[Julien Grall <julien@xxxxxxx>]

Hi Carlo,

I haven't fully reviewed the patch yet. Just point out a couple of 
things I noticed while looking how you dealt with the relocation (I need 
some code to test my series :)).
On 26/08/2022 13:51, Carlo Nonato wrote:
> -#ifdef CONFIG_ARM_64
> +#ifdef CONFIG_CACHE_COLORING
> +    /* Copy Xen to the new location */
> +    memcpy((void *)BOOT_RELOC_VIRT_START, (const void *)XEN_VIRT_START,
> +           (_end - _start));
If I am not mistaken, at this point, Xen will still be using the stack 
that is part of Xen binary (see cpu0_boot_stack). However, until the 
point switch_ttbr() is called the stack can still be used. In particular...
> +    clean_dcache_va_range((void *)BOOT_RELOC_VIRT_START, (_end - _start)); > +
> +    ttbr = virt_to_maddr_colored((vaddr_t)xen_pgtable);
> +#elif CONFIG_ARM_64
>       ttbr = (uintptr_t) xen_pgtable + phys_offset;
>   #else
>       ttbr = (uintptr_t) cpu0_pgtable + phys_offset;
> @@ -530,6 +610,18 @@ void __init setup_pagetables(unsigned long 
> boot_phys_offset)
>   
... the compiler may need to save some information on the stack. And then...

>       switch_ttbr(ttbr);
... restore it after switch_ttbr(). Xen will be using a different stack 
(same virtual address but different physical address) so the wrong value 
will be loaded.
I am not aware of any guaranteed from the AAPCS{32, 64} that this can 
point (please provide a pointer if I am wrong). So I think we either 
want to copy the new Xen to the correct position in the assembly.
This means we would want to revive partially f60658c6ae47 "xen/arm: Stop 
relocating Xen".
Cheers,

--
Julien Grall