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