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

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

Hi Iurii,

OOI, can you give more context why you need to relocate Xen over 4GB?

On 08/04/15 13:36, Iurii Konovalenko wrote:
> From: Iurii Konovalenko <iurii.konovalenko@globallogic.com>
> 
> Primary CPU relocate Xen in over 4GB space and wake up seondary CPUs.

Typoes:

s/relocate/relocates
s/seondary/secondary

> Secondary CPUs run on unrelocated copy of Xen until turning on MMU.
> After turning on MMU secondary CPUs run on relocated copy of Xen.

The non-relocated copy of Xen is not marked as reserved. So Xen may
allocate the area for his own purpose before the secondary CPU has boot.

> 
> To add ability to relocate Xen in over 4GB space add following to
> compilation command: ARM32_RELOCATE_OVER_4GB=y

The virtualization extension requires LPAE. Any reason to make this
optional?

> 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)

Newline here.

> +#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*/

/* ... */

TLBs is confusing. I first though you were talking to Translation
Lookaside Buffer.

> +        PRINT("- Skip construction pagetable, using CPU0 table @")

This very fragile. The CPU0 may drop a part of the 1:1 mapping if it's
clashes with the fixmap and DTB mappings (see after the label "paging").

Therefore secondary CPUs may hang during boot.

> +        mov   r0, r5
> +        bl    putn
> +        mov   r0, r4
> +        bl    putn
> +        PRINT("  -\r\n")
> +        b   skip_constructing
> +#endif
> +
> +1:      /* Setup boot_pgtable: */
>          ldr   r1, =boot_second
>          add   r1, r1, r10            /* r1 := paddr (boot_second) */
>  
> @@ -346,6 +359,7 @@ virtphys_clash:
>          PRINT("- Unable to build boot page tables - virt and phys addresses clash. -\r\n")
>          b     fail
>  
> +skip_constructing:
>  1:
>          PRINT("- Turning on paging -\r\n")
>  
> @@ -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
> +#endif

This chunk need some comment in order to explain what it's doing.

AFAIU the resulting value in r4, will be exactly the same as  "ldr r4,
=init_ttbr".

>          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;
> +#endif

Please define in an header.

>  static int __init rcar2_smp_init(void)
>  {
>      void __iomem *pram;
> @@ -38,7 +41,11 @@ static int __init rcar2_smp_init(void)
>      }
>  
>      /* setup reset vectors */
> -    writel(__pa(init_secondary), pram + RCAR2_SMP_START_OFFSET);
> +    writel(__pa(init_secondary)
> +#ifdef ARM32_RELOCATE_OVER_4GB
> +            - xen_relocation_offset
> +#endif
> +            , pram + RCAR2_SMP_START_OFFSET);
>      iounmap(pram);
>  
>      sev();
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 4ec7c13..66e2834 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -407,7 +407,7 @@ static paddr_t __init get_xen_paddr(void)
>              if ( !e )
>                  continue;
>  
> -#ifdef CONFIG_ARM_32
> +#if defined (CONFIG_ARM_32) && !defined(ARM32_RELOCATE_OVER_4GB)
>              /* Xen must be under 4GB */
>              if ( e > 0x100000000ULL )
>                  e = 0x100000000ULL;
> @@ -699,6 +699,9 @@ void __init setup_cache(void)
>      cacheline_bytes = 1U << (4 + (ccsid & 0x7));
>  }
>  
> +#ifdef ARM32_RELOCATE_OVER_4GB
> +paddr_t xen_relocation_offset;
> +#endif
>  /* C entry point for boot CPU */
>  void __init start_xen(unsigned long boot_phys_offset,
>                        unsigned long fdt_paddr,
> @@ -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

Xen is using /* ... */ for the comment.

> +    xen_relocation_offset = __pa(init_secondary);
> +#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)

Ditto

> +    xen_relocation_offset = __pa(init_secondary) - xen_relocation_offset;

The Xen may be relocated below the boot copy. So the final result may be
negative.

> +#endif
>  
>      /* Update Xen's address now that we have relocated. */
>      printk("Update BOOTMOD_XEN from %"PRIpaddr"-%"PRIpaddr" => %"PRIpaddr"-%"PRIpaddr"\n",
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index a96cda2..731144c 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -31,6 +31,9 @@
>  #include <xen/console.h>
>  #include <asm/gic.h>
>  #include <asm/psci.h>
> +#ifdef ARM32_RELOCATE_OVER_4GB
> +#include <xen/vmap.h>
> +#endif
>  
>  cpumask_t cpu_online_map;
>  cpumask_t cpu_present_map;
> @@ -353,17 +356,33 @@ int __init cpu_up_send_sgi(int cpu)
>      return 0;
>  }
>  
> +#ifdef ARM32_RELOCATE_OVER_4GB
> +extern paddr_t xen_relocation_offset;
> +#endif

This can be avoid by defining xen_relocation_offset in an header.

>  /* Bring up a remote CPU */
>  int __cpu_up(unsigned int cpu)
>  {
>      int rc;
>      s_time_t deadline;
> +#ifdef ARM32_RELOCATE_OVER_4GB
> +    paddr_t p_info = __pa(&smp_up_cpu) - xen_relocation_offset;
> +    unsigned long* info = ioremap_nocache(p_info, sizeof(unsigned long));

unsigned long *info and sizeof(smp_up_cpu).

ioremap_nocache may fail and "info" doesn't have a real meaning.

Regards,

-- 
Julien Grall