Re: [PATCH v7 01/92] target/arm: Add ID_AA64ZFR0 fields and isar_feature_aa64_sve2

[Richard Henderson]

On 7/25/22 00:05, Zenghui Yu wrote:

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index dff85f6db9..37ceadd9a9 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -647,17 +647,26 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures 
*ahcf)

 sve_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SVE) > 
0;

-    kvm_arm_destroy_scratch_host_vcpu(fdarray);
-
-    if (err < 0) {
-    return false;
-    }
-
 /* 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;
+
+    /*
+ * Before v5.1, KVM did not support SVE and did not expose
+ * ID_AA64ZFR0_EL1 even as RAZ.  After v5.1, KVM still does
+ * not expose the register to "user" requests like this
+ * unless the host supports SVE.
+ */
+    err |= read_sys_reg64(fdarray[2], >isar.id_aa64zfr0,
+  ARM64_SYS_REG(3, 0, 0, 4, 4));


If I read it correctly, we haven't yet enabled SVE for the scratch vcpu
(using the KVM_ARM_VCPU_INIT ioctl with KVM_ARM_VCPU_SVE). KVM will
therefore expose ID_AA64ZFR0_EL1 to userspace as RAZ at this point and
isar.id_aa64zfr0 is reset to 0. I wonder if it was intentional?


You are correct, this is a bug.  It appears this is hidden because nothing else actually 
depends on the value within the context of --accel=kvm, e.g. migration.



r~

Re: [PATCH v7 01/92] target/arm: Add ID_AA64ZFR0 fields and isar_feature_aa64_sve2

[Zenghui Yu via]

Hi Richard,

On 2021/5/25 9:02, Richard Henderson wrote:

Will be used for SVE2 isa subset enablement.

Reviewed-by: Alex Bennée 
Signed-off-by: Richard Henderson 
---
v2: Do not read zfr0 from kvm unless sve is available.
v7: Move zfr0 read inside existing sve_enabled block.


[...]


diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index dff85f6db9..37ceadd9a9 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -647,17 +647,26 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures 
*ahcf)
 
 sve_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SVE) > 0;
 
-kvm_arm_destroy_scratch_host_vcpu(fdarray);

-
-if (err < 0) {
-return false;
-}
-
 /* 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;
+
+/*
+ * Before v5.1, KVM did not support SVE and did not expose
+ * ID_AA64ZFR0_EL1 even as RAZ.  After v5.1, KVM still does
+ * not expose the register to "user" requests like this
+ * unless the host supports SVE.
+ */
+err |= read_sys_reg64(fdarray[2], >isar.id_aa64zfr0,
+  ARM64_SYS_REG(3, 0, 0, 4, 4));


If I read it correctly, we haven't yet enabled SVE for the scratch vcpu
(using the KVM_ARM_VCPU_INIT ioctl with KVM_ARM_VCPU_SVE). KVM will
therefore expose ID_AA64ZFR0_EL1 to userspace as RAZ at this point and
isar.id_aa64zfr0 is reset to 0. I wonder if it was intentional?

Thanks,
Zenghui