diff mbox series

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

Message ID 20230227163718.62003-5-miguel.luis@oracle.com
State New
Headers show
Series QEMU v7.2.0 aarch64 Nested Virtualization Support | expand

Commit Message

Miguel Luis Feb. 27, 2023, 4:37 p.m. UTC 
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(-)

Comments

Richard Henderson Feb. 27, 2023, 7:24 p.m. UTC | #1 
On 2/27/23 06: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;
>   }

No, this predicate is testing if EL2 supports AArch32 more.

> @@ -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;
> +    }

This is the test you want...

> @@ -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;
> +    }

... here.

While you could add a new isar_feature predicate for EL2 supported in AArch64 mode, the 
feature test is equivalent and good enough, and is more obviously tied to the required KVM 
support.


r~
Miguel Luis Feb. 28, 2023, 12:23 p.m. UTC | #2 
Hi Richard,

> On 27 Feb 2023, at 18:24, Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> On 2/27/23 06: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;
>>  }
> 
> No, this predicate is testing if EL2 supports AArch32 more.
> 
>> @@ -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;
>> +    }
> 
> This is the test you want...
> 
>> @@ -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;
>> +    }
> 
> ... here.
> 
> While you could add a new isar_feature predicate for EL2 supported in AArch64 mode, the feature test is equivalent and good enough, and is more obviously tied to the required KVM support.
> 
> 

Thank you for the explanation. I will take it into consideration on the next version.

Miguel

> r~
Eric Auger July 6, 2023, 8:16 a.m. UTC | #3 
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.

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.

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);
Miguel Luis July 14, 2023, 12:45 p.m. UTC | #4 
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);
diff mbox series

Patch

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;
+    }
+
     /*
      * 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);