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