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

[Aaron Lindsay <alindsay@codeaurora.org>]

On May 31 15:18, Peter Maydell wrote:
> On 17 May 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > On Apr 17 16:00, Peter Maydell wrote:
> >> 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.
> 
> We need (and already have) a separate feature bit for ARM_DIV
> exactly because it's present on some CPUs which don't have V7VE;
> so we don't want to roll that back into a V7VE feature bit.
> 
> > 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)?
> 
> Sorry, I forgot I hadn't replied to this email yet. Let's do this:

No problem at all - to be fair, my responses have been a bit sporadic,
too. I'm working on putting your suggestions into patch form, with one
clarification below. I also believe I owe you a response on another
thread or two before I push out the next version of this patchset.

> 
>  * define a new ARM_FEATURE_V7VE:
> 
>   ARM_FEATURE_V7VE, /* v7 Virtualization Extensions (non-EL2 parts) */
> 
> In arm_cpu_realizefn:
> 
>  * change the existing "FEATURE_V8 implies V7/ARM_DIV/LPAE" to
>    "FEATURE_V8 implies V7VE"
>  * below that put
> 
>    if (arm_feature(env, ARM_FEATURE_V7VE) {
>        /* v7 Virtualization Extensions. In real hardware this implies
>         * EL2 and also the presence of the Security Extensions.
>         * For QEMU, for backwards-compatibility we implement some
>         * CPUs or CPU configs which have no actual EL2 or EL3 but do
>         * include the various other features that V7VE implies.
>         * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
>         * Security Extensions is ARM_FEATURE_EL3.
>         */
>        set_feature(env, ARM_FEATURE_ARM_DIV);

Is it safe to assume from your comment above regarding keeping ARM_DIV
separate from V7VE that the inclusion of it here is an oversight and
that only LPAE and V7 should be set if V7VE is? (and that V8 should
now directly imply both V7VE and ARM_DIV?)

>        set_feature(env, ARM_FEATURE_LPAE);
>        set_feature(env, ARM_FEATURE_V7);
>    }
>
>  * in kvm_arm_get_host_cpu_features() in kvm32.c add
>    set_feature(&features, ARM_FEATURE_V7VE);
>    where we currently set V7, LPAE, etc.
>    (by definition a host CPU which supports KVM has v7VE.)

Thanks!

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