Re: [PATCH v1 1/2] arm: Add ability to relocate Xen in over 4GB space

[Ian Campbell <ian.campbell@citrix.com>]

On Wed, 2015-04-08 at 15:36 +0300, Iurii Konovalenko wrote:
> From: Iurii Konovalenko <iurii.konovalenko@globallogic.com>
> 
> Primary CPU relocate Xen in over 4GB space and wake up seondary CPUs.
> Secondary CPUs run on unrelocated copy of Xen until turning on MMU.

Which is a significant difference I think, and requires careful checking
that we do not clobber that copy while brining up the primary cpu.

Which would mean it needs to be reserved before we start allocating
heaps and relocating Xen. Ideally only the portion of head.S which we
need would be reserved, not the whole thing. And it should be freed once
everyone is up and running.

Alternatively perhaps the relevant portion of head.S could be copied to
a freshly allocated trampoline page.

> After turning on MMU secondary CPUs run on relocated copy of Xen.
> 
> To add ability to relocate Xen in over 4GB space add following to
> compilation command: ARM32_RELOCATE_OVER_4GB=y
> 
> Signed-off-by: Iurii Konovalenko <iurii.konovalenko@globallogic.com>
> Signed-off-by: Andrii Anisov <andrii.anisov@globallogic.com>
> ---
>  xen/Rules.mk                   |  1 +
>  xen/arch/arm/arm32/head.S      | 21 ++++++++++++++++++++-
>  xen/arch/arm/platforms/rcar2.c |  9 ++++++++-
>  xen/arch/arm/setup.c           | 17 ++++++++++++++++-
>  xen/arch/arm/smpboot.c         | 33 +++++++++++++++++++++++++++++----
>  5 files changed, 74 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/Rules.mk b/xen/Rules.mk
> index feb08d6..fbd34a5 100644
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -64,6 +64,7 @@ CFLAGS-$(HAS_PCI)       += -DHAS_PCI
>  CFLAGS-$(HAS_IOPORTS)   += -DHAS_IOPORTS
>  CFLAGS-$(HAS_PDX)       += -DHAS_PDX
>  CFLAGS-$(frame_pointer) += -fno-omit-frame-pointer -DCONFIG_FRAME_POINTER
> +CFLAGS-$(ARM32_RELOCATE_OVER_4GB) += -DARM32_RELOCATE_OVER_4GB
>  
>  ifneq ($(max_phys_cpus),)
>  CFLAGS-y                += -DMAX_PHYS_CPUS=$(max_phys_cpus)
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 5c0263e..26be1d0 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -262,8 +262,21 @@ cpu_init_done:
>          add   r4, r4, r10            /* r4 := paddr (boot_pagetable) */
>          mov   r5, #0                 /* r4:r5 is paddr (boot_pagetable) */
>          mcrr  CP64(r4, r5, HTTBR)
> +#ifdef ARM32_RELOCATE_OVER_4GB
> +        teq   r7, #0
> +        beq   1f                     /* construct pagetable if CPU0 */
>  
> -        /* Setup boot_pgtable: */
> +        /*Skip constructing TLBs for secondary CPUs, use constructed by CPU0*/

Is this necessary due to the relocation for some reason?

Otherwise, iff its correct, then it should be done as a separate change
before this one.

Also please pay attention to the style used for comments in this file.

> @@ -427,6 +441,11 @@ paging:
>           * setup in init_secondary_pagetables. */
>  
>          ldr   r4, =init_ttbr         /* VA of HTTBR value stashed by CPU 0 */
> +#ifdef ARM32_RELOCATE_OVER_4GB
> +        ldr   r1, =_start
> +        sub r4, r1
> +        add r4, #BOOT_RELOC_VIRT_START

Please follow the existing convention of thoroughly documenting what
each line is doing. (i.e. the "r1 := foo" comments)

> +#endif
>          ldrd  r4, r5, [r4]           /* Actual value */
>          dsb
>          mcrr  CP64(r4, r5, HTTBR)
> diff --git a/xen/arch/arm/platforms/rcar2.c b/xen/arch/arm/platforms/rcar2.c
> index aef544c..4365166 100644
> --- a/xen/arch/arm/platforms/rcar2.c
> +++ b/xen/arch/arm/platforms/rcar2.c
> @@ -25,6 +25,9 @@
>  #define RCAR2_RAM_SIZE                         0x1000
>  #define RCAR2_SMP_START_OFFSET                 0xFFC
>  
> +#ifdef ARM32_RELOCATE_OVER_4GB
> +extern paddr_t xen_relocation_offset;

This certainly doesn't belong here, such things belong in headers.


> @@ -740,8 +743,20 @@ void __init start_xen(unsigned long boot_phys_offset,
>                               (paddr_t)(uintptr_t)(_end - _start + 1), NULL);
>      BUG_ON(!xen_bootmodule);
>  
> +#ifdef ARM32_RELOCATE_OVER_4GB
> +    //save physical address of init_secondary

This is not the Xen style for comments.

> +    xen_relocation_offset = __pa(init_secondary);

How does __pa(init_secondary) end up being an offset? Shouldn't this be
some calculation involving boot_phys_offset?

> +#endif
>      xen_paddr = get_xen_paddr();
>      setup_pagetables(boot_phys_offset, xen_paddr);
> +#ifdef ARM32_RELOCATE_OVER_4GB
> +    //Now Xen is relocated
> +    //Calculate offset of Xen relocation
> +    //It is difference between new physical address of init_secondary an old one
> +    //This offset is needed in several places when we have to write to old Xen location
> +    //(secondary CPUs run on old-located Xen and rely on some variables from CPU0)
> +    xen_relocation_offset = __pa(init_secondary) - xen_relocation_offset;

Oh I see, ewww. I think you should be able to calculate this directly
when you do the relocation, which is much nicer than relying on tricks
of __pa changing.

And rather than adding all of the uses of xen_relocatio_offset I think a
__boot_pa construct which does the adjustment should be used.

Ian.