Re: [Qemu-arm] [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
> (b) define an ARM_FEATURE_V7VE for them
> (c) define separate feature bits for them individually

From what I can tell, using ARM_FEATURE_LPAE to represent all the
almost-v7ve misfits won't work well because ARM_FEATURE_ARM_DIV may be
supported on some platforms for which ARM_FEATURE_LPAE is not (Cortex
R5), and ARM_FEATURE_ARM_DIV is read from ID_ISAR0 in
kvm_arm_get_host_cpu_features() (and may be set/not set independently of
ARM_FEATURE_LPAE). It appears there is a need to independently
distinguish between them. The same reasoning also seems to rule out
option (b) "one ARM_FEATURE_V7VE to rule them all", leaving me with
option (c).

It almost seems silly to create ARM_FEATURE_PMOVSSET, but I'm not sure
what else makes sense to do here. Am I missing something (I'm almost
hoping I am)?

-Aaron

-- 
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.