[PATCH 0/3] target/arm: Fix kvm probe of ID_AA64ZFR0

[PATCH 0/3] target/arm: Fix kvm probe of ID_AA64ZFR0

[Richard Henderson]

Our probing of this SVE register was done within an incorrect
vCPU environment, so that the id register was always RAZ.


r~


Richard Henderson (3):
  target/arm: Create kvm_arm_svm_supported
  target/arm: Set KVM_ARM_VCPU_SVE while probing the host
  target/arm: Move sve probe inside kvm >= 4.15 branch

 target/arm/kvm64.c | 50 +++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 23 deletions(-)

-- 
2.34.1

[PATCH 1/3] target/arm: Create kvm_arm_svm_supported

[Richard Henderson]

Indication for support for SVE will not depend on whether we
perform the query on the main kvm_state or the temp vcpu.
Mirror kvm_arm_pauth_supported.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/kvm64.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index d16d4ea250..36ff20c9e3 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -496,6 +496,11 @@ static bool kvm_arm_pauth_supported(void)
             kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
 }
 
+static bool kvm_arm_svm_supported(void)
+{
+    return kvm_check_extension(kvm_state, KVM_CAP_ARM_SVE);
+}
+
 bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
 {
     /* Identify the feature bits corresponding to the host CPU, and
@@ -675,7 +680,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
         }
     }
 
-    sve_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SVE) > 0;
+    sve_supported = kvm_arm_svm_supported();
 
     /* Add feature bits that can't appear until after VCPU init. */
     if (sve_supported) {
-- 
2.34.1

Re: [PATCH 1/3] target/arm: Create kvm_arm_svm_supported

[Zenghui Yu via]

Hi Richard,

On 2022/7/26 2:14, Richard Henderson wrote:
> Indication for support for SVE will not depend on whether we
> perform the query on the main kvm_state or the temp vcpu.
> Mirror kvm_arm_pauth_supported.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/kvm64.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index d16d4ea250..36ff20c9e3 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -496,6 +496,11 @@ static bool kvm_arm_pauth_supported(void)
>              kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
>  }
>  
> +static bool kvm_arm_svm_supported(void)
> +{
> +    return kvm_check_extension(kvm_state, KVM_CAP_ARM_SVE);
> +}
> +

Not sure if it's a typo. Maybe we should instead use
kvm_arm_sve_supported() which was introduced in commit 14e99e0fbbc6.

Zenghui

Re: [PATCH 1/3] target/arm: Create kvm_arm_svm_supported

[Richard Henderson]

On 7/25/22 19:02, Zenghui Yu wrote:
> Hi Richard,
> 
> On 2022/7/26 2:14, Richard Henderson wrote:
>> Indication for support for SVE will not depend on whether we
>> perform the query on the main kvm_state or the temp vcpu.
>> Mirror kvm_arm_pauth_supported.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  target/arm/kvm64.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
>> index d16d4ea250..36ff20c9e3 100644
>> --- a/target/arm/kvm64.c
>> +++ b/target/arm/kvm64.c
>> @@ -496,6 +496,11 @@ static bool kvm_arm_pauth_supported(void)
>>              kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
>>  }
>>
>> +static bool kvm_arm_svm_supported(void)
>> +{
>> +    return kvm_check_extension(kvm_state, KVM_CAP_ARM_SVE);
>> +}
>> +
> 
> Not sure if it's a typo. Maybe we should instead use
> kvm_arm_sve_supported() which was introduced in commit 14e99e0fbbc6.

Oof, it certainly is.

r~

[PATCH 2/3] target/arm: Set KVM_ARM_VCPU_SVE while probing the host

[Richard Henderson]

Because we weren't setting this flag, our probe of ID_AA64ZFR0
was always returning zero.  This also obviates the adjustment
of ID_AA64PFR0, which had sanitized the SVE field.

Reported-by: Zenghui Yu <yuzenghui@huawei.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/kvm64.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 36ff20c9e3..8b2ae9948a 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -512,7 +512,6 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
     bool sve_supported;
     bool pmu_supported = false;
     uint64_t features = 0;
-    uint64_t t;
     int err;
 
     /* Old kernels may not know about the PREFERRED_TARGET ioctl: however
@@ -533,10 +532,17 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
     struct kvm_vcpu_init init = { .target = -1, };
 
     /*
-     * Ask for Pointer Authentication if supported. We can't play the
-     * SVE trick of synthesising the ID reg as KVM won't tell us
-     * whether we have the architected or IMPDEF version of PAuth, so
-     * we have to use the actual ID regs.
+     * Ask for SVE if supported, so that we can query ID_AA64ZFR0,
+     * which is otherwise RAZ.
+     */
+    sve_supported = kvm_arm_svm_supported();
+    if (sve_supported) {
+        init.features[0] |= 1 << KVM_ARM_VCPU_SVE;
+    }
+
+    /*
+     * Ask for Pointer Authentication if supported, so that we get
+     * the unsanitized field values for AA64ISAR1_EL1.
      */
     if (kvm_arm_pauth_supported()) {
         init.features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
@@ -680,20 +686,13 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
         }
     }
 
-    sve_supported = kvm_arm_svm_supported();
-
-    /* Add feature bits that can't appear until after VCPU init. */
     if (sve_supported) {
-        t = ahcf->isar.id_aa64pfr0;
-        t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1);
-        ahcf->isar.id_aa64pfr0 = t;
-
         /*
          * There is a range of kernels between kernel commit 73433762fcae
          * and f81cb2c3ad41 which have a bug where the kernel doesn't expose
          * SYS_ID_AA64ZFR0_EL1 via the ONE_REG API unless the VM has enabled
-         * SVE support, so we only read it here, rather than together with all
-         * the other ID registers earlier.
+         * SVE support, which resulted in an error rather than RAZ.
+         * So only read the register if we set KVM_ARM_VCPU_SVE above.
          */
         err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64zfr0,
                               ARM64_SYS_REG(3, 0, 0, 4, 4));
-- 
2.34.1

Re: [PATCH 2/3] target/arm: Set KVM_ARM_VCPU_SVE while probing the host

[Richard Henderson]

On 7/25/22 11:14, Richard Henderson wrote:
> Because we weren't setting this flag, our probe of ID_AA64ZFR0
> was always returning zero.  This also obviates the adjustment
> of ID_AA64PFR0, which had sanitized the SVE field.


Oh, I meant to say here that this the effects of the bug are not visible, because the only 
thing that ISAR.ID_AA64ZFR0 is used for within qemu at present is tcg translation.  The 
other tests for SVE within KVM are via ISAR.ID_AA64PFR0.SVE.


r~


> 
> Reported-by: Zenghui Yu <yuzenghui@huawei.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/kvm64.c | 27 +++++++++++++--------------
>   1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 36ff20c9e3..8b2ae9948a 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -512,7 +512,6 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>       bool sve_supported;
>       bool pmu_supported = false;
>       uint64_t features = 0;
> -    uint64_t t;
>       int err;
>   
>       /* Old kernels may not know about the PREFERRED_TARGET ioctl: however
> @@ -533,10 +532,17 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>       struct kvm_vcpu_init init = { .target = -1, };
>   
>       /*
> -     * Ask for Pointer Authentication if supported. We can't play the
> -     * SVE trick of synthesising the ID reg as KVM won't tell us
> -     * whether we have the architected or IMPDEF version of PAuth, so
> -     * we have to use the actual ID regs.
> +     * Ask for SVE if supported, so that we can query ID_AA64ZFR0,
> +     * which is otherwise RAZ.
> +     */
> +    sve_supported = kvm_arm_svm_supported();
> +    if (sve_supported) {
> +        init.features[0] |= 1 << KVM_ARM_VCPU_SVE;
> +    }
> +
> +    /*
> +     * Ask for Pointer Authentication if supported, so that we get
> +     * the unsanitized field values for AA64ISAR1_EL1.
>        */
>       if (kvm_arm_pauth_supported()) {
>           init.features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
> @@ -680,20 +686,13 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>           }
>       }
>   
> -    sve_supported = kvm_arm_svm_supported();
> -
> -    /* Add feature bits that can't appear until after VCPU init. */
>       if (sve_supported) {
> -        t = ahcf->isar.id_aa64pfr0;
> -        t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1);
> -        ahcf->isar.id_aa64pfr0 = t;
> -
>           /*
>            * There is a range of kernels between kernel commit 73433762fcae
>            * and f81cb2c3ad41 which have a bug where the kernel doesn't expose
>            * SYS_ID_AA64ZFR0_EL1 via the ONE_REG API unless the VM has enabled
> -         * SVE support, so we only read it here, rather than together with all
> -         * the other ID registers earlier.
> +         * SVE support, which resulted in an error rather than RAZ.
> +         * So only read the register if we set KVM_ARM_VCPU_SVE above.
>            */
>           err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64zfr0,
>                                 ARM64_SYS_REG(3, 0, 0, 4, 4));

[PATCH 3/3] target/arm: Move sve probe inside kvm >= 4.15 branch

[Richard Henderson]

The test for the IF block indicates no ID registers are exposed, much
less host support for SVE.  Move the SVE probe into the ELSE block.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/kvm64.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 8b2ae9948a..bc3d7d9883 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -684,18 +684,18 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
             err |= read_sys_reg64(fdarray[2], &ahcf->isar.reset_pmcr_el0,
                                   ARM64_SYS_REG(3, 3, 9, 12, 0));
         }
-    }
 
-    if (sve_supported) {
-        /*
-         * There is a range of kernels between kernel commit 73433762fcae
-         * and f81cb2c3ad41 which have a bug where the kernel doesn't expose
-         * SYS_ID_AA64ZFR0_EL1 via the ONE_REG API unless the VM has enabled
-         * SVE support, which resulted in an error rather than RAZ.
-         * So only read the register if we set KVM_ARM_VCPU_SVE above.
-         */
-        err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64zfr0,
-                              ARM64_SYS_REG(3, 0, 0, 4, 4));
+        if (sve_supported) {
+            /*
+             * There is a range of kernels between kernel commit 73433762fcae
+             * and f81cb2c3ad41 which have a bug where the kernel doesn't
+             * expose SYS_ID_AA64ZFR0_EL1 via the ONE_REG API unless the VM has
+             * enabled SVE support, which resulted in an error rather than RAZ.
+             * So only read the register if we set KVM_ARM_VCPU_SVE above.
+             */
+            err |= read_sys_reg64(fdarray[2], &ahcf->isar.id_aa64zfr0,
+                                  ARM64_SYS_REG(3, 0, 0, 4, 4));
+        }
     }
 
     kvm_arm_destroy_scratch_host_vcpu(fdarray);
-- 
2.34.1