Assert that the value to be written is the correct size.
No change in functionality here, just mirroring the same
function from kvm64.

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

diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
index 0f1e94c7b5..da08f7aab8 100644
--- a/target/arm/kvm32.c
+++ b/target/arm/kvm32.c
@@ -28,6 +28,14 @@ static inline void set_feature(uint64_t *features, int feature)
     *features |= 1ULL << feature;
 }
 
+static int read_sys_reg32(int fd, uint32_t *pret, uint64_t id)
+{
+    struct kvm_one_reg idreg = { .id = id, .addr = (uintptr_t)pret };
+
+    assert((id & KVM_REG_SIZE_MASK) == KVM_REG_SIZE_U32);
+    return ioctl(fd, KVM_GET_ONE_REG, &idreg);
+}
+
 bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
 {
     /* Identify the feature bits corresponding to the host CPU, and
@@ -35,9 +43,10 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
      * we have to create a scratch VM, create a single CPU inside it,
      * and then query that CPU for the relevant ID registers.
      */
-    int i, ret, fdarray[3];
+    int i, err = 0, fdarray[3];
     uint32_t midr, id_pfr0, mvfr1;
     uint64_t features = 0;
+
     /* Old kernels may not know about the PREFERRED_TARGET ioctl: however
      * we know these will only support creating one kind of guest CPU,
      * which is its preferred CPU type.
@@ -47,23 +56,6 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
         QEMU_KVM_ARM_TARGET_NONE
     };
     struct kvm_vcpu_init init;
-    struct kvm_one_reg idregs[] = {
-        {
-            .id = KVM_REG_ARM | KVM_REG_SIZE_U32
-            | ENCODE_CP_REG(15, 0, 0, 0, 0, 0, 0),
-            .addr = (uintptr_t)&midr,
-        },
-        {
-            .id = KVM_REG_ARM | KVM_REG_SIZE_U32
-            | ENCODE_CP_REG(15, 0, 0, 0, 1, 0, 0),
-            .addr = (uintptr_t)&id_pfr0,
-        },
-        {
-            .id = KVM_REG_ARM | KVM_REG_SIZE_U32
-            | KVM_REG_ARM_VFP | KVM_REG_ARM_VFP_MVFR1,
-            .addr = (uintptr_t)&mvfr1,
-        },
-    };
 
     if (!kvm_arm_create_scratch_host_vcpu(cpus_to_try, fdarray, &init)) {
         return false;
@@ -77,16 +69,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
      */
     ahcf->dtb_compatible = "arm,arm-v7";
 
-    for (i = 0; i < ARRAY_SIZE(idregs); i++) {
-        ret = ioctl(fdarray[2], KVM_GET_ONE_REG, &idregs[i]);
-        if (ret) {
-            break;
-        }
-    }
+    err |= read_sys_reg32(fdarray[2], &midr, ARM_CP15_REG32(0, 0, 0, 0));
+    err |= read_sys_reg32(fdarray[2], &id_pfr0, ARM_CP15_REG32(0, 0, 1, 0));
+    err |= read_sys_reg32(fdarray[2], &mvfr1,
+                          KVM_REG_ARM | KVM_REG_SIZE_U32 |
+                          KVM_REG_ARM_VFP | KVM_REG_ARM_VFP_MVFR1);
 
     kvm_arm_destroy_scratch_host_vcpu(fdarray);
 
-    if (ret) {
+    if (err < 0) {
         return false;
     }
 
-- 
2.17.2

On 10/24/18 12:37 PM, Richard Henderson wrote:
> -    int i, ret, fdarray[3];
> +    int i, err = 0, fdarray[3];

Bah.  The bit about having compile-tested for arm32 was fake news -- "i" is now
unused.  Ho hum.

> @@ -77,16 +69,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>       */
>      ahcf->dtb_compatible = "arm,arm-v7";
>  
> -    for (i = 0; i < ARRAY_SIZE(idregs); i++) {
> -        ret = ioctl(fdarray[2], KVM_GET_ONE_REG, &idregs[i]);
> -        if (ret) {
> -            break;
> -        }
> -    }
> +    err |= read_sys_reg32(fdarray[2], &midr, ARM_CP15_REG32(0, 0, 0, 0));
> +    err |= read_sys_reg32(fdarray[2], &id_pfr0, ARM_CP15_REG32(0, 0, 1, 0));
> +    err |= read_sys_reg32(fdarray[2], &mvfr1,
> +                          KVM_REG_ARM | KVM_REG_SIZE_U32 |
> +                          KVM_REG_ARM_VFP | KVM_REG_ARM_VFP_MVFR1);


r~

On 24 October 2018 at 12:37, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Assert that the value to be written is the correct size.
> No change in functionality here, just mirroring the same
> function from kvm64.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/kvm32.c | 41 ++++++++++++++++-------------------------
>  1 file changed, 16 insertions(+), 25 deletions(-)

> -    struct kvm_one_reg idregs[] = {
> -        {
> -            .id = KVM_REG_ARM | KVM_REG_SIZE_U32
> -            | ENCODE_CP_REG(15, 0, 0, 0, 0, 0, 0),
> -            .addr = (uintptr_t)&midr,
> -        },
> -        {
> -            .id = KVM_REG_ARM | KVM_REG_SIZE_U32
> -            | ENCODE_CP_REG(15, 0, 0, 0, 1, 0, 0),
> -            .addr = (uintptr_t)&id_pfr0,
> -        },
> -        {
> -            .id = KVM_REG_ARM | KVM_REG_SIZE_U32
> -            | KVM_REG_ARM_VFP | KVM_REG_ARM_VFP_MVFR1,
> -            .addr = (uintptr_t)&mvfr1,
> -        },
> -    };
>
>      if (!kvm_arm_create_scratch_host_vcpu(cpus_to_try, fdarray, &init)) {
>          return false;
> @@ -77,16 +69,15 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>       */
>      ahcf->dtb_compatible = "arm,arm-v7";
>
> -    for (i = 0; i < ARRAY_SIZE(idregs); i++) {
> -        ret = ioctl(fdarray[2], KVM_GET_ONE_REG, &idregs[i]);
> -        if (ret) {
> -            break;
> -        }
> -    }
> +    err |= read_sys_reg32(fdarray[2], &midr, ARM_CP15_REG32(0, 0, 0, 0));
> +    err |= read_sys_reg32(fdarray[2], &id_pfr0, ARM_CP15_REG32(0, 0, 1, 0));


Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
with the declaration of 'i' removed.

(Confusingly, ENCODE_CP_REG and ARM_CP15_REG32 take the
op1/crn/crm/op2 arguments in different orders.)

thanks
-- PMM

On 11/2/18 2:30 PM, Peter Maydell wrote:
> (Confusingly, ENCODE_CP_REG and ARM_CP15_REG32 take the
> op1/crn/crm/op2 arguments in different orders.)

As Shaggy said, "It wasn't me".  ;-)


r~