From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60919) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dUsZZ-0005tD-V1 for qemu-devel@nongnu.org; Tue, 11 Jul 2017 06:39:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dUsZY-0001Hv-Rp for qemu-devel@nongnu.org; Tue, 11 Jul 2017 06:39:17 -0400 Date: Tue, 11 Jul 2017 12:38:59 +0200 From: "Edgar E. Iglesias" Message-ID: <20170711103859.GC25504@toto> References: <1498830302-19274-1-git-send-email-edgar.iglesias@gmail.com> <1498830302-19274-3-git-send-email-edgar.iglesias@gmail.com> <20170711100334.GA25504@toto> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v1 2/2] target-arm: Extend PAR format determination List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: "Edgar E. Iglesias" , QEMU Developers , Alex =?iso-8859-1?Q?Benn=E9e?= , qemu-arm On Tue, Jul 11, 2017 at 11:14:04AM +0100, Peter Maydell wrote: > On 11 July 2017 at 11:03, Edgar E. Iglesias 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