Re: [PATCH RFC 5/9] xen: arm: Defer setting of VTCR_EL2 until after CPUs are up

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 07/30/2014 02:47 PM, Ian Campbell wrote:
> Currently we retain the hardcoded values but soon we will want to
> calculate the correct values based upon the CPU properties common to all processors, which are only available once they are all up.

NIT: I think it misses a newline on the second line. It looks strange to
see the first line short and not the second.

> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/p2m.c     |   16 +++++++++++++---
>  xen/arch/arm/setup.c   |    4 ++--
>  xen/arch/arm/smpboot.c |    2 --
>  3 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 705b29b..225d125 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1059,8 +1059,17 @@ err:
>      return page;
>  }
>  
> +static void setup_virt_paging_one(void *data)

I would add __init.

> +{
> +    unsigned long val = (unsigned long)data;

VTCR_EL2 is a 32 bit register. I would use uint32_t for the variable type.

> +    WRITE_SYSREG32(val, VTCR_EL2);
> +    isb();
> +}
> +
>  void __cpuinit setup_virt_paging(void)

This can become __init now.

>  {
> +    unsigned long val;
> +
>      /* Setup Stage 2 address translation */
>      /* SH0=11 (Inner-shareable)
>       * ORGN0=IRGN0=01 (Normal memory, Write-Back Write-Allocate Cacheable)
> @@ -1070,11 +1079,12 @@ void __cpuinit setup_virt_paging(void)
>       *        PS=010 == 40 bits
>       */
>  #ifdef CONFIG_ARM_32
> -    WRITE_SYSREG32(0x80003558, VTCR_EL2);
> +    val = 0x80003558;
>  #else
> -    WRITE_SYSREG32(0x80023558, VTCR_EL2);
> +    val = 0x80023558;
>  #endif
> -    isb();
> +    setup_virt_paging_one((void *)val);
> +    smp_call_function(setup_virt_paging_one, (void *)val, 1);
>  }
>  /*
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 446b4dc..ddaef7e 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -788,8 +788,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>  
>      gic_init();
>  
> -    setup_virt_paging();
> -
>      p2m_vmid_allocator_init();
>  
>      softirq_init();
> @@ -838,6 +836,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>  
>      do_initcalls();
>  
> +    setup_virt_paging();
> +

Assuming there is no CPU hotplug, which IIRC is not yet support, this is
only depends on the SMP bring up.  Can we move this before do_initcalls?
So if we need to initialize some code that is relying on VTCR_EL2, it
will be possible.

I was mostly thinking about IOMMU drivers where a similar trick will be
necessary sooner or later.

Regards,

-- 
Julien Grall