From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46596) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fB4f1-0001G7-0W for qemu-devel@nongnu.org; Tue, 24 Apr 2018 16:35:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fB4ez-0002DZ-OZ for qemu-devel@nongnu.org; Tue, 24 Apr 2018 16:35:35 -0400 Date: Tue, 24 Apr 2018 16:35:18 -0400 From: Aaron Lindsay Message-ID: <20180424203517.GA4771@codeaurora.org> References: <1521232280-13089-1-git-send-email-alindsay@codeaurora.org> <1521232280-13089-16-git-send-email-alindsay@codeaurora.org> <20180417142318.GO24561@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-arm , Alistair Francis , Wei Huang , Peter Crosthwaite , QEMU Developers , Michael Spradling , Digant Desai On Apr 17 16:00, Peter Maydell wrote: > On 17 April 2018 at 15:23, Aaron Lindsay wrote: > > On Apr 12 18:17, Peter Maydell wrote: > >> What's the difference between this and ARM_FEATURE_EL2 ? > > > > I use ARM_FEATURE_V7VE in a later patch to guard against implementing > > PMOVSSET on v7 machines which don't implement the virtualization > > extensions > > (http://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg04917.html). > > I could use ARM_FEATURE_EL2, but declaring that v7 machines supported > > EL2 didn't feel right. I don't feel strongly one way or the other - how > > do you prefer to handle this? > > So, the underlying issue here is that there's a QEMU specific > fudge going on. Architecturally, if the CPU implements the > Virtualization Extensions, then: > * it has Hyp mode > * it must also implement the Security Extensions > * on reset it starts in the Secure world > * it has LPAE > * it has some stuff that is not inherently tied to having EL2, > like the SDIV and UDIV instructions, and the presence of > PMOVSSET > > In an ideal world, we'd just have a feature flag that turned > all that on. Unfortunately, a combination of backwards compatibility > issues, the order in which various features were implemented > in QEMU, and the fact that KVM can't emulate a guest CPU with > the Security Extensions means that we want to be able to model > variants of some CPUs that don't really exist in real hardware: > Cortex-A15 and -A7 which only implement EL0/EL1 but still have > all the v7VE features that you can see from those ELs. But we > didn't really properly lay out guidelines for how the feature > bits should work in this case, with the result that we have > a bunch of local hacks (for instance get_S1prot() has a check > on the LPAE feature bit, since in practice that bit is set in > exactly the CPUs that have v7VE; and the UDIV/SDIV insns have > their own feature bits.) > > So we should probably sort out this mess first, either by: > > (a) state that we use ARM_FEATURE_LPAE for all checks for > features that are architecturally v7VE but which we want to > exist even on our v7VE-no-Hyp-no-Secure oddballs Are you implying that this would only involve updating the comments to match the existing implementation? > (b) define an ARM_FEATURE_V7VE for them This seems the most attractive to me, though perhaps that's partially aspirational - it is closest to what would be happening in an ideal world where QEMU fully supported V7VE for this hardware. > (c) define separate feature bits for them individually I'm hesitant to formulate a patch for this - I'm not confident I understand the context and interactions of the pieces involved well enough to contribute productively. That said, I understand your motivation to fix this the right way and am willing to try with a little more direction/hand-holding. For instance, which CPUs are we talking about here - I only see both ARM_FEATURE_ARM_DIV and ARM_FEATURE_LPAE advertised together (without full V8) in cortex_a7_initfn and cortex_a15_initfn. I'm assuming that V8 implies the full set of V7VE features we're discussing here so whatever we come up with as a solution would be automatically set in the ARM_FEATURE_V8 "Some features automatically imply others:" `if` statement in arm_cpu_realizefn. -Aaron > In any case we'd retain ARM_FEATURE_EL2 for "and really > has EL2/Hyp mode", and we'd want to do an audit of current > uses of various feature bits to see whether they followed > the new rules. > > (For AArch64 things are a bit less awkward because the > architecture allows the idea of an implementation that > has EL2 but not EL3.) -- Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.