On Tue, Jul 11, 2017 at 11:14:04AM +0100, Peter Maydell wrote:
> On 11 July 2017 at 11:03, Edgar E. Iglesias <edgar.igles...@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
