Re: [Qemu-devel] [PATCH v4 13/13] target-arm: Add support for S1 + S2 MMU translations

[Peter Maydell]

On 14 October 2015 at 23:55, Edgar E. Iglesias <address@hidden> wrote:
> From: "Edgar E. Iglesias" <address@hidden>
>
> Signed-off-by: Edgar E. Iglesias <address@hidden>
> ---
>  target-arm/helper.c | 44 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 7 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 69e24e1..9d70ef2 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -7129,14 +7129,44 @@ static bool get_phys_addr(CPUARMState *env, 
> target_ulong address,
>                            ARMMMUFaultInfo *fi)
>  {
>      if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> -        /* TODO: when we support EL2 we should here call ourselves 
> recursively
> -         * to do the stage 1 and then stage 2 translations. The arm_ld*_ptw
> -         * functions will also need changing to perform ARMMMUIdx_S2NS loads
> -         * rather than direct physical memory loads when appropriate.
> -         * For non-EL2 CPUs a stage1+stage2 translation is just stage 1.
> +        /* Call ourselves recursively to do the stage 1 and then stage 2
> +         * translations.
>           */
> -        assert(!arm_feature(env, ARM_FEATURE_EL2));
> -        mmu_idx += ARMMMUIdx_S1NSE0;
> +        if (arm_feature(env, ARM_FEATURE_EL2)) {
> +            hwaddr ipa;
> +            int s2_prot;
> +            int ret;
> +
> +            ret = get_phys_addr(env, address, access_type,
> +                                mmu_idx + ARMMMUIdx_S1NSE0, &ipa, attrs,
> +                                prot, page_size, fsr, fi);
> +
> +            /* If S1 fails or S2 is disabled, return early.  */
> +            if (ret || regime_translation_disabled(env, ARMMMUIdx_S2NS)) {
> +                if (ret && fi->stage2) {
> +                    /* This is a S2 error while doing S1 PTW.  */
> +                    env->cp15.hpfar_el2 = extract64(fi->s2addr, 12, 47) << 4;

Might be worth a note that it's OK to set the HPFAR here because
this always results in a fault (even if from an AT instruction),
whereas we can't set the FAR registers here because that doesn't
happen for stage 1 faults from AT instructions.

...I think we still need to add the code to cause the exception
if a stage 1 AT instruction results in a stage 2 fault, right?
If the caller has to look into the FaultInfo struct anyway, maybe
we should just let the caller set the HPFAR_EL2 from the s2addr
if it's going to send the exception to EL2.

> +                }
> +                *phys_ptr = ipa;
> +                return ret;
> +            }
> +
> +            /* S1 is done. Now do S2 translation.  */
> +            ret = get_phys_addr_lpae(env, ipa, access_type, ARMMMUIdx_S2NS,
> +                                     phys_ptr, attrs, &s2_prot,
> +                                     page_size, fsr, fi);
> +            if (ret) {
> +                env->cp15.hpfar_el2 = extract64(ipa, 12, 47) << 4;
> +            }
> +            /* Combine the S1 and S2 perms.  */
> +            *prot &= s2_prot;
> +            return ret;
> +        } else {
> +            /*
> +             * For non-EL2 CPUs a stage1+stage2 translation is just stage 1.
> +             */
> +            mmu_idx += ARMMMUIdx_S1NSE0;
> +        }
>      }
>
>      /* The page table entries may downgrade secure to non-secure, but
> --
> 1.9.1
>

thanks
-- PMM