Re: [PATCH] xen: arm: force guest memory accesses to cacheable when MMU is disabled

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

On 01/07/2014 04:02 PM, Ian Campbell wrote:
.>
> One thing I'm not sure about is reverting the previous fix in a0035ecc0d82.
> It's reasonably recent so reverting it takes us back to a pretty well
> understood state in the libraries. The functionality is harmless if
> incomplete. I think given the first argument I would lean towards reverting.

I would also prefer reverting the previous patch.

> 
> Obviously if you think I'm being to easy on myself please say so.

Without this patch, we will likely crash a guest in production, that
it's not acceptable for a release.

> 
> Actually, if you think my judgement is right I'd appreciate being told so too.
> ---
>  xen/arch/arm/domain.c           |    5 ++
>  xen/arch/arm/domain_build.c     |    1 +
>  xen/arch/arm/traps.c            |  153 ++++++++++++++++++++++++++++++++++++++-
>  xen/arch/arm/vtimer.c           |    6 +-
>  xen/include/asm-arm/cpregs.h    |    4 +
>  xen/include/asm-arm/processor.h |    2 +-
>  xen/include/asm-arm/sysregs.h   |   19 ++++-
>  7 files changed, 182 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 124cccf..104d228 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -219,6 +219,11 @@ static void ctxt_switch_to(struct vcpu *n)
>      else
>          hcr |= HCR_RW;
>  
> +    if ( n->arch.sctlr & SCTLR_M )
> +        hcr &= ~(HCR_TVM|HCR_DC);
> +    else
> +        hcr |= (HCR_TVM|HCR_DC);
> +
>      WRITE_SYSREG(hcr, HCR_EL2);
>      isb();
>  
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 47b781b..bb31db8 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1026,6 +1026,7 @@ int construct_dom0(struct domain *d)
>      else
>          WRITE_SYSREG(READ_SYSREG(HCR_EL2) | HCR_RW, HCR_EL2);
>  #endif
> +    WRITE_SYSREG(READ_SYSREG(HCR_EL2) | HCR_DC | HCR_TVM, HCR_EL2);

Is it useful? As I understand, we will at least context switch one time
before booting dom0.

If we need it, perhaps the better place to setup it is init_traps?

>  
>      /*
>       * kernel_load will determine the placement of the initrd & fdt in
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 7c5ab19..d00bba3 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1279,6 +1279,23 @@ static void advance_pc(struct cpu_user_regs *regs, union hsr hsr)
>      regs->pc += hsr.len ? 4 : 2;
>  }
>  
> +static void update_sctlr(uint32_t v)
> +{
> +    /*
> +     * If MMU (SCTLR_M) is now enabled then we must disable HCR.DC
> +     * because they are incompatible.
> +     *
> +     * Once HCR.DC is disabled then we do not need HCR_TVM either,
> +     * since it's only purpose was to catch the MMU being enabled.
> +     *
> +     * Both are set appropriately on context switch but we need to
> +     * clear them now since we may not context switch on return to
> +     * guest.
> +     */
> +    if ( v & SCTLR_M )
> +        WRITE_SYSREG(READ_SYSREG(HCR_EL2) & ~(HCR_DC|HCR_TVM), HCR_EL2);

Even if it's unlikely, can we handle the case where the guest disable
the case?

Also from ARM ARM B3.2.1, a TLB flush by VMID is required if HCR_DC is
disabled and the VMID is not changed.

-- 
Julien Grall