Re: [PATCH v1 2/2] target-arm: Extend PAR format determination

["Edgar E. Iglesias" <edgar.iglesias@xilinx.com>]

On Tue, Jul 11, 2017 at 11:14:04AM +0100, Peter Maydell wrote:
> On 11 July 2017 at 11:03, Edgar E. Iglesias <edgar.iglesias@xilinx.com> wrote:
> > On Mon, Jul 10, 2017 at 04:11:29PM +0100, Peter Maydell wrote:
> >> So this kind of worries me, because what it's coded as is "determine
> >> whether architecturally we should be returning a 64-bit or 32-bit
> >> PAR format", but what the code below it uses the format64 flag for is
> >> "manipulate whatever PAR we got handed back by get_phys_addr()".
> >> So we have two separate bits of code that are both choosing
> >> 32 vs 64 bit PAR (the code in this patch, and the logic inside
> >> get_phys_addr()), and they have to come to the same conclusion
> >> or we'll silently mangle the PAR. It seems like it would be
> >> better to either have get_phys_addr() explicitly tell us what kind
> >> of format it is returning to us, or to have the caller tell it
> >> what kind of PAR it needs.
> >
> > Yes, I see your point and that's exactly what's happenning before the patch.
> > Some of these new checks are generic in the sense that they check for LPAE/64bitness
> > but others are I guess ATS specific for lack of a better term.
> > It feels a bit weird to put the ATS specific PAR format logic into get_phys_addr.
> >
> > The basic idea here is that we never downgrade to the 32bit format, we only uprgade.
> > The following line was meant to get the initial format I think you are requesting:
> > format64 = regime_using_lpae_format(env, mmu_idx);
> >
> > After that, we apply possible ATS specfic upgrades to 64bit PAR format if needed.
> >
> > For clarity, perhaps we could make get_phys_addr return this same initial format, and then
> > we can follow up with the ATS specific upgrades. E.g:
> >
> >     ret = get_phys_addr(env, value, access_type, mmu_idx,
> >                         &phys_addr, &attrs, &prot, &page_size, &fsr, &fi,
> >                         &format64);
> >
> >     /* Apply possible ATS/PAR 64bit upgrades if format64 is false.  */
> >     if (is_a64(env)) {
> >         format64 = true;
> >     } else if (arm_feature(env, ARM_FEATURE_LPAE)) {
> >         if (arm_feature(env, ARM_FEATURE_EL2)) {
> >             if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> >                 format64 |= env->cp15.hcr_el2 & HCR_VM;
> >             } else {
> >                 format64 |= arm_current_el(env) == 2;
> >             }
> >         }
> >     }
> 
> This still has the same problem, doesn't it? If get_phys_addr()
> has given you back a short-descriptor format PAR then you cannot
> simply "upgrade" it to a long-descriptor format PAR -- the
> fault status codes are all different. I think the "short desc
> vs long desc" condition used to be simple but the various
> upgrades to get_phys_addr() to handle EL2 have made it much
> more complicated, and so we'll be much better off just handing
> get_phys_addr() a flag to say how we want the status reported,
> if it's really dependent on ATS vs not-ATS.
>

Another way could also be to have get_phys_addr() fill in generic
fields in the FaultInfo struct and then have a faultinfo_to_fsr
mapping function to populate FSR/PAR. Do you see any issues with that?

Perhaps that's better, not sure.

Best regards,
Edgar