Re: [Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Aaron Lindsay <alindsay@codeaurora.org>]

On Apr 17 16:00, Peter Maydell wrote:
> On 17 April 2018 at 15:23, Aaron Lindsay <alindsay@codeaurora.org> 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.