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

[Peter Maydell]

On 17 May 2018 at 20:31, Aaron Lindsay <address@hidden> 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:

 * 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);
       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.)

 * perhaps some followon patches that correct checks that were
   testing something else and can now use the new V7VE feature bit.

thanks
-- PMM