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

[Edgar E. Iglesias]

On Fri, Oct 23, 2015 at 06:09:24PM +0100, Peter Maydell wrote:
> 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?

Yes, those faults are still missing... I can try to add them
in the next round/series together with the detailed error reporting.


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


Agreed, I've moved the setting of HPFAR_EL2 to the callers.

Thanks!
Edgar



> 
> > +                }
> > +                *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