Re: [RFC PATCH 4/5] target/arm: enable feature ARM_FEATURE_EL2 if EL2 is supported

[Miguel Luis]

Hi Eric,

Thanks in advance for your comment.

> On 6 Jul 2023, at 08:16, Eric Auger <eauger@redhat.com> wrote:
> 
> Hi Miguel,
> 
> On 2/27/23 17:37, Miguel Luis wrote:
>> From: Haibo Xu <haibo.xu@linaro.org>
>> 
>> KVM_CAP_ARM_EL2 must be supported by the cpu to enable ARM_FEATURE_EL2.
>> EL2 bits on ID_AA64PFR0 state unsupported on the value 0b0000.
>> 
>> Ref: 
>> https://lore.kernel.org/qemu-devel/b7c2626e6c720ccc43e57197dff3dac72d613640.1616052890.git.haibo.xu@linaro.org/
>> 
>> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
>> [Miguel Luis: use of ID_AA64PFR0 for cpu features]
>> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
>> ---
>> target/arm/cpu.h   |  2 +-
>> target/arm/kvm64.c | 16 ++++++++++++++++
>> 2 files changed, 17 insertions(+), 1 deletion(-)
>> 
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 9aeed3c848..de2a88b43e 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -3961,7 +3961,7 @@ static inline bool isar_feature_aa64_aa32_el1(const 
>> ARMISARegisters *id)
>> 
>> static inline bool isar_feature_aa64_aa32_el2(const ARMISARegisters *id)
>> {
>> -    return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL2) >= 2;
>> +    return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, EL2) != 0;
>> }
>> 
>> static inline bool isar_feature_aa64_ras(const ARMISARegisters *id)
>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
>> index be8144a2b5..f7ebd731aa 100644
>> --- a/target/arm/kvm64.c
>> +++ b/target/arm/kvm64.c
>> @@ -505,6 +505,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures 
>> *ahcf)
>>      */
>>     int fdarray[3];
>>     bool sve_supported;
>> +    bool el2_supported;
>>     bool pmu_supported = false;
>>     uint64_t features = 0;
>>     int err;
>> @@ -535,6 +536,14 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures 
>> *ahcf)
>>         init.features[0] |= 1 << KVM_ARM_VCPU_SVE;
>>     }
>> 
>> +    /*
>> +     * Ask for EL2 if supported.
>> +     */
>> +    el2_supported = kvm_arm_el2_supported();
>> +    if (el2_supported) {
>> +        init.features[0] |= 1 << KVM_ARM_VCPU_HAS_EL2;
> This doesn't work if your host both supports SVE & NV.
> 

May I ask if this prevented your L1 or L2 guest from booting? I’ve addressed all
the previous comments on the thread for the new RFC version and this topic is
what I’m currently addressing.

So far the L1 guest booted successfully but not the L2 guest.

> The error output by qemu is not straightforward
> 
> qemu-system-aarch64: can't apply global host-arm-cpu.sve=off: Property
> 'host-arm-cpu.sve' not found
> 
> The problem is that we create a scratch VM with a CPU featuring both SVE
> and NV and this fails at kernel level, I think on vcpu reset.
> 
> The trouble is that we do that even if sve=off at qemu level. So I think
> this is a more generic issue related to the way we validate host cpu
> features.
> 

OK, I’m having a look into that.

Thank you

Miguel

> Thanks
> 
> Eric
> 
> 
>> +    }
>> +
>>     /*
>>      * Ask for Pointer Authentication if supported, so that we get
>>      * the unsanitized field values for AA64ISAR1_EL1.
>> @@ -714,6 +723,10 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures 
>> *ahcf)
>>     features |= 1ULL << ARM_FEATURE_PMU;
>>     features |= 1ULL << ARM_FEATURE_GENERIC_TIMER;
>> 
>> +    if (el2_supported) {
>> +        features |= 1ULL << ARM_FEATURE_EL2;
>> +    }
>> +
>>     ahcf->features = features;
>> 
>>     return true;
>> @@ -881,6 +894,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>         assert(kvm_arm_sve_supported());
>>         cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
>>     }
>> +    if (cpu_isar_feature(aa64_aa32_el2, cpu)) {
>> +        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_HAS_EL2;
>> +    }
>>     if (cpu_isar_feature(aa64_pauth, cpu)) {
>>         cpu->kvm_init_features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
>>                                       1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);