[PATCH v2 0/3] arm/kvm: use kvm_{get,set}_one

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* [PATCH v2 0/3] arm/kvm: use kvm_{get,set}_one_reg
@ 2023-10-10 14:24 Cornelia Huck
  2023-10-10 14:24 ` [PATCH v2 1/3] arm/kvm: convert to kvm_set_one_reg Cornelia Huck
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Cornelia Huck @ 2023-10-10 14:24 UTC (permalink / raw)
  To: Peter Maydell, Paolo Bonzini
  Cc: Gavin Shan, qemu-arm, qemu-devel, kvm, Cornelia Huck

I sent this cleanup first... in mid-July (ugh). But better late than never, I guess.

From v1:
- fix buglets (thanks Gavin)
- add patch 3 on top

The kvm_{get,set}_one_reg functions have been around for a very long
time, and using them instead of open-coding the ioctl invocations
saves lines of code, and gives us a tracepoint as well. They cannot
be used by invocations of the ioctl not acting on a CPUState, but
that still leaves a lot of conversions in the target/arm code.

target/mips and target/ppc also have some potential for conversions,
but as I cannot test either (and they are both in 'Odd fixes' anyway),
I left them alone.

Survives some testing on a Mt. Snow.

Cornelia Huck (3):
  arm/kvm: convert to kvm_set_one_reg
  arm/kvm: convert to kvm_get_one_reg
  arm/kvm: convert to read_sys_reg64

 target/arm/kvm.c   |  28 +++-------
 target/arm/kvm64.c | 129 ++++++++++++---------------------------------
 2 files changed, 40 insertions(+), 117 deletions(-)

-- 
2.41.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/3] arm/kvm: convert to kvm_set_one_reg
  2023-10-10 14:24 [PATCH v2 0/3] arm/kvm: use kvm_{get,set}_one_reg Cornelia Huck
@ 2023-10-10 14:24 ` Cornelia Huck
  2023-10-11  2:44   ` Philippe Mathieu-Daudé
  2023-10-10 14:24 ` [PATCH v2 2/3] arm/kvm: convert to kvm_get_one_reg Cornelia Huck
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Cornelia Huck @ 2023-10-10 14:24 UTC (permalink / raw)
  To: Peter Maydell, Paolo Bonzini
  Cc: Gavin Shan, qemu-arm, qemu-devel, kvm, Cornelia Huck

We can neaten the code by switching to the kvm_set_one_reg function.

Reviewed-by: Gavin Shan <gshan@redhat.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 target/arm/kvm.c   | 13 +++------
 target/arm/kvm64.c | 66 +++++++++++++---------------------------------
 2 files changed, 21 insertions(+), 58 deletions(-)

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index b66b936a9586..1a8084c4601c 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -589,7 +589,6 @@ bool write_list_to_kvmstate(ARMCPU *cpu, int level)
     bool ok = true;
 
     for (i = 0; i < cpu->cpreg_array_len; i++) {
-        struct kvm_one_reg r;
         uint64_t regidx = cpu->cpreg_indexes[i];
         uint32_t v32;
         int ret;
@@ -598,19 +597,17 @@ bool write_list_to_kvmstate(ARMCPU *cpu, int level)
             continue;
         }
 
-        r.id = regidx;
         switch (regidx & KVM_REG_SIZE_MASK) {
         case KVM_REG_SIZE_U32:
             v32 = cpu->cpreg_values[i];
-            r.addr = (uintptr_t)&v32;
+            ret = kvm_set_one_reg(cs, regidx, &v32);
             break;
         case KVM_REG_SIZE_U64:
-            r.addr = (uintptr_t)(cpu->cpreg_values + i);
+            ret = kvm_set_one_reg(cs, regidx, cpu->cpreg_values + i);
             break;
         default:
             g_assert_not_reached();
         }
-        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &r);
         if (ret) {
             /* We might fail for "unknown register" and also for
              * "you tried to set a register which is constant with
@@ -731,17 +728,13 @@ void kvm_arm_get_virtual_time(CPUState *cs)
 void kvm_arm_put_virtual_time(CPUState *cs)
 {
     ARMCPU *cpu = ARM_CPU(cs);
-    struct kvm_one_reg reg = {
-        .id = KVM_REG_ARM_TIMER_CNT,
-        .addr = (uintptr_t)&cpu->kvm_vtime,
-    };
     int ret;
 
     if (!cpu->kvm_vtime_dirty) {
         return;
     }
 
-    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    ret = kvm_set_one_reg(cs, KVM_REG_ARM_TIMER_CNT, &cpu->kvm_vtime);
     if (ret) {
         error_report("Failed to set KVM_REG_ARM_TIMER_CNT");
         abort();
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 5e95c496bb9f..047b269a7918 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -540,14 +540,10 @@ static int kvm_arm_sve_set_vls(CPUState *cs)
 {
     ARMCPU *cpu = ARM_CPU(cs);
     uint64_t vls[KVM_ARM64_SVE_VLS_WORDS] = { cpu->sve_vq.map };
-    struct kvm_one_reg reg = {
-        .id = KVM_REG_ARM64_SVE_VLS,
-        .addr = (uint64_t)&vls[0],
-    };
 
     assert(cpu->sve_max_vq <= KVM_ARM64_SVE_VQ_MAX);
 
-    return kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    return kvm_set_one_reg(cs, KVM_REG_ARM64_SVE_VLS, &vls[0]);
 }
 
 #define ARM_CPU_ID_MPIDR       3, 0, 0, 0, 5
@@ -726,19 +722,17 @@ static void kvm_inject_arm_sea(CPUState *c)
 static int kvm_arch_put_fpsimd(CPUState *cs)
 {
     CPUARMState *env = &ARM_CPU(cs)->env;
-    struct kvm_one_reg reg;
     int i, ret;
 
     for (i = 0; i < 32; i++) {
         uint64_t *q = aa64_vfp_qreg(env, i);
 #if HOST_BIG_ENDIAN
         uint64_t fp_val[2] = { q[1], q[0] };
-        reg.addr = (uintptr_t)fp_val;
+        ret = kvm_set_one_reg(cs, AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]),
+                                                        fp_val);
 #else
-        reg.addr = (uintptr_t)q;
+        ret = kvm_set_one_reg(cs, AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]), q);
 #endif
-        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
-        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
         if (ret) {
             return ret;
         }
@@ -759,14 +753,11 @@ static int kvm_arch_put_sve(CPUState *cs)
     CPUARMState *env = &cpu->env;
     uint64_t tmp[ARM_MAX_VQ * 2];
     uint64_t *r;
-    struct kvm_one_reg reg;
     int n, ret;
 
     for (n = 0; n < KVM_ARM64_SVE_NUM_ZREGS; ++n) {
         r = sve_bswap64(tmp, &env->vfp.zregs[n].d[0], cpu->sve_max_vq * 2);
-        reg.addr = (uintptr_t)r;
-        reg.id = KVM_REG_ARM64_SVE_ZREG(n, 0);
-        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+        ret = kvm_set_one_reg(cs, KVM_REG_ARM64_SVE_ZREG(n, 0), r);
         if (ret) {
             return ret;
         }
@@ -775,9 +766,7 @@ static int kvm_arch_put_sve(CPUState *cs)
     for (n = 0; n < KVM_ARM64_SVE_NUM_PREGS; ++n) {
         r = sve_bswap64(tmp, r = &env->vfp.pregs[n].p[0],
                         DIV_ROUND_UP(cpu->sve_max_vq * 2, 8));
-        reg.addr = (uintptr_t)r;
-        reg.id = KVM_REG_ARM64_SVE_PREG(n, 0);
-        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+        ret = kvm_set_one_reg(cs, KVM_REG_ARM64_SVE_PREG(n, 0), r);
         if (ret) {
             return ret;
         }
@@ -785,9 +774,7 @@ static int kvm_arch_put_sve(CPUState *cs)
 
     r = sve_bswap64(tmp, &env->vfp.pregs[FFR_PRED_NUM].p[0],
                     DIV_ROUND_UP(cpu->sve_max_vq * 2, 8));
-    reg.addr = (uintptr_t)r;
-    reg.id = KVM_REG_ARM64_SVE_FFR(0);
-    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    ret = kvm_set_one_reg(cs, KVM_REG_ARM64_SVE_FFR(0), r);
     if (ret) {
         return ret;
     }
@@ -797,7 +784,6 @@ static int kvm_arch_put_sve(CPUState *cs)
 
 int kvm_arch_put_registers(CPUState *cs, int level)
 {
-    struct kvm_one_reg reg;
     uint64_t val;
     uint32_t fpr;
     int i, ret;
@@ -814,9 +800,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
     }
 
     for (i = 0; i < 31; i++) {
-        reg.id = AARCH64_CORE_REG(regs.regs[i]);
-        reg.addr = (uintptr_t) &env->xregs[i];
-        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+        ret = kvm_set_one_reg(cs, AARCH64_CORE_REG(regs.regs[i]),
+                              &env->xregs[i]);
         if (ret) {
             return ret;
         }
@@ -827,16 +812,12 @@ int kvm_arch_put_registers(CPUState *cs, int level)
      */
     aarch64_save_sp(env, 1);
 
-    reg.id = AARCH64_CORE_REG(regs.sp);
-    reg.addr = (uintptr_t) &env->sp_el[0];
-    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    ret = kvm_set_one_reg(cs, AARCH64_CORE_REG(regs.sp), &env->sp_el[0]);
     if (ret) {
         return ret;
     }
 
-    reg.id = AARCH64_CORE_REG(sp_el1);
-    reg.addr = (uintptr_t) &env->sp_el[1];
-    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    ret = kvm_set_one_reg(cs, AARCH64_CORE_REG(sp_el1), &env->sp_el[1]);
     if (ret) {
         return ret;
     }
@@ -847,23 +828,17 @@ int kvm_arch_put_registers(CPUState *cs, int level)
     } else {
         val = cpsr_read(env);
     }
-    reg.id = AARCH64_CORE_REG(regs.pstate);
-    reg.addr = (uintptr_t) &val;
-    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    ret = kvm_set_one_reg(cs, AARCH64_CORE_REG(regs.pstate), &val);
     if (ret) {
         return ret;
     }
 
-    reg.id = AARCH64_CORE_REG(regs.pc);
-    reg.addr = (uintptr_t) &env->pc;
-    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    ret = kvm_set_one_reg(cs, AARCH64_CORE_REG(regs.pc), &env->pc);
     if (ret) {
         return ret;
     }
 
-    reg.id = AARCH64_CORE_REG(elr_el1);
-    reg.addr = (uintptr_t) &env->elr_el[1];
-    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    ret = kvm_set_one_reg(cs, AARCH64_CORE_REG(elr_el1), &env->elr_el[1]);
     if (ret) {
         return ret;
     }
@@ -882,9 +857,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
 
     /* KVM 0-4 map to QEMU banks 1-5 */
     for (i = 0; i < KVM_NR_SPSR; i++) {
-        reg.id = AARCH64_CORE_REG(spsr[i]);
-        reg.addr = (uintptr_t) &env->banked_spsr[i + 1];
-        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+        ret = kvm_set_one_reg(cs, AARCH64_CORE_REG(spsr[i]),
+                              &env->banked_spsr[i + 1]);
         if (ret) {
             return ret;
         }
@@ -899,18 +873,14 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         return ret;
     }
 
-    reg.addr = (uintptr_t)(&fpr);
     fpr = vfp_get_fpsr(env);
-    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
-    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    ret = kvm_set_one_reg(cs, AARCH64_SIMD_CTRL_REG(fp_regs.fpsr), &fpr);
     if (ret) {
         return ret;
     }
 
-    reg.addr = (uintptr_t)(&fpr);
     fpr = vfp_get_fpcr(env);
-    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr);
-    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    ret = kvm_set_one_reg(cs, AARCH64_SIMD_CTRL_REG(fp_regs.fpcr), &fpr);
     if (ret) {
         return ret;
     }
-- 
2.41.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] arm/kvm: convert to kvm_set_one_reg
  2023-10-10 14:24 ` [PATCH v2 1/3] arm/kvm: convert to kvm_set_one_reg Cornelia Huck
@ 2023-10-11  2:44   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-11  2:44 UTC (permalink / raw)
  To: Cornelia Huck, Peter Maydell, Paolo Bonzini
  Cc: Gavin Shan, qemu-arm, qemu-devel, kvm

On 10/10/23 16:24, Cornelia Huck wrote:
> We can neaten the code by switching to the kvm_set_one_reg function.
> 
> Reviewed-by: Gavin Shan <gshan@redhat.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>   target/arm/kvm.c   | 13 +++------
>   target/arm/kvm64.c | 66 +++++++++++++---------------------------------
>   2 files changed, 21 insertions(+), 58 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 2/3] arm/kvm: convert to kvm_get_one_reg
  2023-10-10 14:24 [PATCH v2 0/3] arm/kvm: use kvm_{get,set}_one_reg Cornelia Huck
  2023-10-10 14:24 ` [PATCH v2 1/3] arm/kvm: convert to kvm_set_one_reg Cornelia Huck
@ 2023-10-10 14:24 ` Cornelia Huck
  2023-10-11  2:44   ` Philippe Mathieu-Daudé
  2023-10-10 14:24 ` [PATCH v2 3/3] arm/kvm: convert to read_sys_reg64 Cornelia Huck
  2023-10-17 12:15 ` [PATCH v2 0/3] arm/kvm: use kvm_{get,set}_one_reg Peter Maydell
  3 siblings, 1 reply; 10+ messages in thread
From: Cornelia Huck @ 2023-10-10 14:24 UTC (permalink / raw)
  To: Peter Maydell, Paolo Bonzini
  Cc: Gavin Shan, qemu-arm, qemu-devel, kvm, Cornelia Huck

We can neaten the code by switching the callers that work on a
CPUstate to the kvm_get_one_reg function.

Reviewed-by: Gavin Shan <gshan@redhat.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 target/arm/kvm.c   | 15 +++---------
 target/arm/kvm64.c | 57 ++++++++++++----------------------------------
 2 files changed, 18 insertions(+), 54 deletions(-)

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 1a8084c4601c..7903e2ddde1b 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -553,24 +553,19 @@ bool write_kvmstate_to_list(ARMCPU *cpu)
     bool ok = true;
 
     for (i = 0; i < cpu->cpreg_array_len; i++) {
-        struct kvm_one_reg r;
         uint64_t regidx = cpu->cpreg_indexes[i];
         uint32_t v32;
         int ret;
 
-        r.id = regidx;
-
         switch (regidx & KVM_REG_SIZE_MASK) {
         case KVM_REG_SIZE_U32:
-            r.addr = (uintptr_t)&v32;
-            ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &r);
+            ret = kvm_get_one_reg(cs, regidx, &v32);
             if (!ret) {
                 cpu->cpreg_values[i] = v32;
             }
             break;
         case KVM_REG_SIZE_U64:
-            r.addr = (uintptr_t)(cpu->cpreg_values + i);
-            ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &r);
+            ret = kvm_get_one_reg(cs, regidx, cpu->cpreg_values + i);
             break;
         default:
             g_assert_not_reached();
@@ -706,17 +701,13 @@ int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu)
 void kvm_arm_get_virtual_time(CPUState *cs)
 {
     ARMCPU *cpu = ARM_CPU(cs);
-    struct kvm_one_reg reg = {
-        .id = KVM_REG_ARM_TIMER_CNT,
-        .addr = (uintptr_t)&cpu->kvm_vtime,
-    };
     int ret;
 
     if (cpu->kvm_vtime_dirty) {
         return;
     }
 
-    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    ret = kvm_get_one_reg(cs, KVM_REG_ARM_TIMER_CNT, &cpu->kvm_vtime);
     if (ret) {
         error_report("Failed to get KVM_REG_ARM_TIMER_CNT");
         abort();
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 047b269a7918..558c0b88dd69 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -909,14 +909,11 @@ int kvm_arch_put_registers(CPUState *cs, int level)
 static int kvm_arch_get_fpsimd(CPUState *cs)
 {
     CPUARMState *env = &ARM_CPU(cs)->env;
-    struct kvm_one_reg reg;
     int i, ret;
 
     for (i = 0; i < 32; i++) {
         uint64_t *q = aa64_vfp_qreg(env, i);
-        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
-        reg.addr = (uintptr_t)q;
-        ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+        ret = kvm_get_one_reg(cs, AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]), q);
         if (ret) {
             return ret;
         } else {
@@ -940,15 +937,12 @@ static int kvm_arch_get_sve(CPUState *cs)
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
-    struct kvm_one_reg reg;
     uint64_t *r;
     int n, ret;
 
     for (n = 0; n < KVM_ARM64_SVE_NUM_ZREGS; ++n) {
         r = &env->vfp.zregs[n].d[0];
-        reg.addr = (uintptr_t)r;
-        reg.id = KVM_REG_ARM64_SVE_ZREG(n, 0);
-        ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+        ret = kvm_get_one_reg(cs, KVM_REG_ARM64_SVE_ZREG(n, 0), r);
         if (ret) {
             return ret;
         }
@@ -957,9 +951,7 @@ static int kvm_arch_get_sve(CPUState *cs)
 
     for (n = 0; n < KVM_ARM64_SVE_NUM_PREGS; ++n) {
         r = &env->vfp.pregs[n].p[0];
-        reg.addr = (uintptr_t)r;
-        reg.id = KVM_REG_ARM64_SVE_PREG(n, 0);
-        ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+        ret = kvm_get_one_reg(cs, KVM_REG_ARM64_SVE_PREG(n, 0), r);
         if (ret) {
             return ret;
         }
@@ -967,9 +959,7 @@ static int kvm_arch_get_sve(CPUState *cs)
     }
 
     r = &env->vfp.pregs[FFR_PRED_NUM].p[0];
-    reg.addr = (uintptr_t)r;
-    reg.id = KVM_REG_ARM64_SVE_FFR(0);
-    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    ret = kvm_get_one_reg(cs, KVM_REG_ARM64_SVE_FFR(0), r);
     if (ret) {
         return ret;
     }
@@ -980,7 +970,6 @@ static int kvm_arch_get_sve(CPUState *cs)
 
 int kvm_arch_get_registers(CPUState *cs)
 {
-    struct kvm_one_reg reg;
     uint64_t val;
     unsigned int el;
     uint32_t fpr;
@@ -990,31 +979,24 @@ int kvm_arch_get_registers(CPUState *cs)
     CPUARMState *env = &cpu->env;
 
     for (i = 0; i < 31; i++) {
-        reg.id = AARCH64_CORE_REG(regs.regs[i]);
-        reg.addr = (uintptr_t) &env->xregs[i];
-        ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+        ret = kvm_get_one_reg(cs, AARCH64_CORE_REG(regs.regs[i]),
+                              &env->xregs[i]);
         if (ret) {
             return ret;
         }
     }
 
-    reg.id = AARCH64_CORE_REG(regs.sp);
-    reg.addr = (uintptr_t) &env->sp_el[0];
-    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    ret = kvm_get_one_reg(cs, AARCH64_CORE_REG(regs.sp), &env->sp_el[0]);
     if (ret) {
         return ret;
     }
 
-    reg.id = AARCH64_CORE_REG(sp_el1);
-    reg.addr = (uintptr_t) &env->sp_el[1];
-    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    ret = kvm_get_one_reg(cs, AARCH64_CORE_REG(sp_el1), &env->sp_el[1]);
     if (ret) {
         return ret;
     }
 
-    reg.id = AARCH64_CORE_REG(regs.pstate);
-    reg.addr = (uintptr_t) &val;
-    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    ret = kvm_get_one_reg(cs, AARCH64_CORE_REG(regs.pstate), &val);
     if (ret) {
         return ret;
     }
@@ -1031,9 +1013,7 @@ int kvm_arch_get_registers(CPUState *cs)
      */
     aarch64_restore_sp(env, 1);
 
-    reg.id = AARCH64_CORE_REG(regs.pc);
-    reg.addr = (uintptr_t) &env->pc;
-    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    ret = kvm_get_one_reg(cs, AARCH64_CORE_REG(regs.pc), &env->pc);
     if (ret) {
         return ret;
     }
@@ -1047,9 +1027,7 @@ int kvm_arch_get_registers(CPUState *cs)
         aarch64_sync_64_to_32(env);
     }
 
-    reg.id = AARCH64_CORE_REG(elr_el1);
-    reg.addr = (uintptr_t) &env->elr_el[1];
-    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    ret = kvm_get_one_reg(cs, AARCH64_CORE_REG(elr_el1), &env->elr_el[1]);
     if (ret) {
         return ret;
     }
@@ -1059,9 +1037,8 @@ int kvm_arch_get_registers(CPUState *cs)
      * KVM SPSRs 0-4 map to QEMU banks 1-5
      */
     for (i = 0; i < KVM_NR_SPSR; i++) {
-        reg.id = AARCH64_CORE_REG(spsr[i]);
-        reg.addr = (uintptr_t) &env->banked_spsr[i + 1];
-        ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+        ret = kvm_get_one_reg(cs, AARCH64_CORE_REG(spsr[i]),
+                              &env->banked_spsr[i + 1]);
         if (ret) {
             return ret;
         }
@@ -1082,17 +1059,13 @@ int kvm_arch_get_registers(CPUState *cs)
         return ret;
     }
 
-    reg.addr = (uintptr_t)(&fpr);
-    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
-    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    ret = kvm_get_one_reg(cs, AARCH64_SIMD_CTRL_REG(fp_regs.fpsr), &fpr);
     if (ret) {
         return ret;
     }
     vfp_set_fpsr(env, fpr);
 
-    reg.addr = (uintptr_t)(&fpr);
-    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr);
-    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    ret = kvm_get_one_reg(cs, AARCH64_SIMD_CTRL_REG(fp_regs.fpcr), &fpr);
     if (ret) {
         return ret;
     }
-- 
2.41.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/3] arm/kvm: convert to kvm_get_one_reg
  2023-10-10 14:24 ` [PATCH v2 2/3] arm/kvm: convert to kvm_get_one_reg Cornelia Huck
@ 2023-10-11  2:44   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-11  2:44 UTC (permalink / raw)
  To: Cornelia Huck, Peter Maydell, Paolo Bonzini
  Cc: Gavin Shan, qemu-arm, qemu-devel, kvm

On 10/10/23 16:24, Cornelia Huck wrote:
> We can neaten the code by switching the callers that work on a
> CPUstate to the kvm_get_one_reg function.
> 
> Reviewed-by: Gavin Shan <gshan@redhat.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>   target/arm/kvm.c   | 15 +++---------
>   target/arm/kvm64.c | 57 ++++++++++++----------------------------------
>   2 files changed, 18 insertions(+), 54 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 3/3] arm/kvm: convert to read_sys_reg64
  2023-10-10 14:24 [PATCH v2 0/3] arm/kvm: use kvm_{get,set}_one_reg Cornelia Huck
  2023-10-10 14:24 ` [PATCH v2 1/3] arm/kvm: convert to kvm_set_one_reg Cornelia Huck
  2023-10-10 14:24 ` [PATCH v2 2/3] arm/kvm: convert to kvm_get_one_reg Cornelia Huck
@ 2023-10-10 14:24 ` Cornelia Huck
  2023-10-11  0:02   ` Gavin Shan
  2023-10-17 12:09   ` Peter Maydell
  2023-10-17 12:15 ` [PATCH v2 0/3] arm/kvm: use kvm_{get,set}_one_reg Peter Maydell
  3 siblings, 2 replies; 10+ messages in thread
From: Cornelia Huck @ 2023-10-10 14:24 UTC (permalink / raw)
  To: Peter Maydell, Paolo Bonzini
  Cc: Gavin Shan, qemu-arm, qemu-devel, kvm, Cornelia Huck

We can use read_sys_reg64 to get the SVE_VLS register instead of
calling GET_ONE_REG directly.

Suggested-by: Gavin Shan <gshan@redhat.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 target/arm/kvm64.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 558c0b88dd69..d40c89a84752 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -500,10 +500,6 @@ uint32_t kvm_arm_sve_get_vls(CPUState *cs)
             .target = -1,
             .features[0] = (1 << KVM_ARM_VCPU_SVE),
         };
-        struct kvm_one_reg reg = {
-            .id = KVM_REG_ARM64_SVE_VLS,
-            .addr = (uint64_t)&vls[0],
-        };
         int fdarray[3], ret;
 
         probed = true;
@@ -512,7 +508,7 @@ uint32_t kvm_arm_sve_get_vls(CPUState *cs)
             error_report("failed to create scratch VCPU with SVE enabled");
             abort();
         }
-        ret = ioctl(fdarray[2], KVM_GET_ONE_REG, &reg);
+        ret = read_sys_reg64(fdarray[2], &vls[0], KVM_REG_ARM64_SVE_VLS);
         kvm_arm_destroy_scratch_host_vcpu(fdarray);
         if (ret) {
             error_report("failed to get KVM_REG_ARM64_SVE_VLS: %s",
-- 
2.41.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] arm/kvm: convert to read_sys_reg64
  2023-10-10 14:24 ` [PATCH v2 3/3] arm/kvm: convert to read_sys_reg64 Cornelia Huck
@ 2023-10-11  0:02   ` Gavin Shan
  2023-10-17 12:09   ` Peter Maydell
  1 sibling, 0 replies; 10+ messages in thread
From: Gavin Shan @ 2023-10-11  0:02 UTC (permalink / raw)
  To: Cornelia Huck, Peter Maydell, Paolo Bonzini; +Cc: qemu-arm, qemu-devel, kvm

On 10/11/23 00:24, Cornelia Huck wrote:
> We can use read_sys_reg64 to get the SVE_VLS register instead of
> calling GET_ONE_REG directly.
> 
> Suggested-by: Gavin Shan <gshan@redhat.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>   target/arm/kvm64.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 558c0b88dd69..d40c89a84752 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -500,10 +500,6 @@ uint32_t kvm_arm_sve_get_vls(CPUState *cs)
>               .target = -1,
>               .features[0] = (1 << KVM_ARM_VCPU_SVE),
>           };
> -        struct kvm_one_reg reg = {
> -            .id = KVM_REG_ARM64_SVE_VLS,
> -            .addr = (uint64_t)&vls[0],
> -        };
>           int fdarray[3], ret;
>   
>           probed = true;
> @@ -512,7 +508,7 @@ uint32_t kvm_arm_sve_get_vls(CPUState *cs)
>               error_report("failed to create scratch VCPU with SVE enabled");
>               abort();
>           }
> -        ret = ioctl(fdarray[2], KVM_GET_ONE_REG, &reg);
> +        ret = read_sys_reg64(fdarray[2], &vls[0], KVM_REG_ARM64_SVE_VLS);
>           kvm_arm_destroy_scratch_host_vcpu(fdarray);
>           if (ret) {
>               error_report("failed to get KVM_REG_ARM64_SVE_VLS: %s",



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] arm/kvm: convert to read_sys_reg64
  2023-10-10 14:24 ` [PATCH v2 3/3] arm/kvm: convert to read_sys_reg64 Cornelia Huck
  2023-10-11  0:02   ` Gavin Shan
@ 2023-10-17 12:09   ` Peter Maydell
  2023-10-17 15:28     ` Cornelia Huck
  1 sibling, 1 reply; 10+ messages in thread
From: Peter Maydell @ 2023-10-17 12:09 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: Paolo Bonzini, Gavin Shan, qemu-arm, qemu-devel, kvm

On Tue, 10 Oct 2023 at 15:25, Cornelia Huck <cohuck@redhat.com> wrote:
>
> We can use read_sys_reg64 to get the SVE_VLS register instead of
> calling GET_ONE_REG directly.
>
> Suggested-by: Gavin Shan <gshan@redhat.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  target/arm/kvm64.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 558c0b88dd69..d40c89a84752 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -500,10 +500,6 @@ uint32_t kvm_arm_sve_get_vls(CPUState *cs)
>              .target = -1,
>              .features[0] = (1 << KVM_ARM_VCPU_SVE),
>          };
> -        struct kvm_one_reg reg = {
> -            .id = KVM_REG_ARM64_SVE_VLS,
> -            .addr = (uint64_t)&vls[0],
> -        };
>          int fdarray[3], ret;
>
>          probed = true;
> @@ -512,7 +508,7 @@ uint32_t kvm_arm_sve_get_vls(CPUState *cs)
>              error_report("failed to create scratch VCPU with SVE enabled");
>              abort();
>          }
> -        ret = ioctl(fdarray[2], KVM_GET_ONE_REG, &reg);
> +        ret = read_sys_reg64(fdarray[2], &vls[0], KVM_REG_ARM64_SVE_VLS);
>          kvm_arm_destroy_scratch_host_vcpu(fdarray);
>          if (ret) {
>              error_report("failed to get KVM_REG_ARM64_SVE_VLS: %s",

read_sys_reg64() asserts that the register you're trying to
read is 64 bits, but KVM_REG_ARM64_SVE_VLS is not, it's 512 bits:

#define KVM_REG_ARM64_SVE_VLS           (KVM_REG_ARM64 | KVM_REG_ARM64_SVE | \
                                         KVM_REG_SIZE_U512 | 0xffff)

So this change would trip the assert on a host where SVE
is supported and enabled.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] arm/kvm: convert to read_sys_reg64
  2023-10-17 12:09   ` Peter Maydell
@ 2023-10-17 15:28     ` Cornelia Huck
  0 siblings, 0 replies; 10+ messages in thread
From: Cornelia Huck @ 2023-10-17 15:28 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Paolo Bonzini, Gavin Shan, qemu-arm, qemu-devel, kvm

On Tue, Oct 17 2023, Peter Maydell <peter.maydell@linaro.org> wrote:

> On Tue, 10 Oct 2023 at 15:25, Cornelia Huck <cohuck@redhat.com> wrote:
>>
>> We can use read_sys_reg64 to get the SVE_VLS register instead of
>> calling GET_ONE_REG directly.
>>
>> Suggested-by: Gavin Shan <gshan@redhat.com>
>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>  target/arm/kvm64.c | 6 +-----
>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
>> index 558c0b88dd69..d40c89a84752 100644
>> --- a/target/arm/kvm64.c
>> +++ b/target/arm/kvm64.c
>> @@ -500,10 +500,6 @@ uint32_t kvm_arm_sve_get_vls(CPUState *cs)
>>              .target = -1,
>>              .features[0] = (1 << KVM_ARM_VCPU_SVE),
>>          };
>> -        struct kvm_one_reg reg = {
>> -            .id = KVM_REG_ARM64_SVE_VLS,
>> -            .addr = (uint64_t)&vls[0],
>> -        };
>>          int fdarray[3], ret;
>>
>>          probed = true;
>> @@ -512,7 +508,7 @@ uint32_t kvm_arm_sve_get_vls(CPUState *cs)
>>              error_report("failed to create scratch VCPU with SVE enabled");
>>              abort();
>>          }
>> -        ret = ioctl(fdarray[2], KVM_GET_ONE_REG, &reg);
>> +        ret = read_sys_reg64(fdarray[2], &vls[0], KVM_REG_ARM64_SVE_VLS);
>>          kvm_arm_destroy_scratch_host_vcpu(fdarray);
>>          if (ret) {
>>              error_report("failed to get KVM_REG_ARM64_SVE_VLS: %s",
>
> read_sys_reg64() asserts that the register you're trying to
> read is 64 bits, but KVM_REG_ARM64_SVE_VLS is not, it's 512 bits:
>
> #define KVM_REG_ARM64_SVE_VLS           (KVM_REG_ARM64 | KVM_REG_ARM64_SVE | \
>                                          KVM_REG_SIZE_U512 | 0xffff)
>
> So this change would trip the assert on a host where SVE
> is supported and enabled.

Whoops, it seems that I misread this. (And my test environment didn't
have that enabled...)



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/3] arm/kvm: use kvm_{get,set}_one_reg
  2023-10-10 14:24 [PATCH v2 0/3] arm/kvm: use kvm_{get,set}_one_reg Cornelia Huck
                   ` (2 preceding siblings ...)
  2023-10-10 14:24 ` [PATCH v2 3/3] arm/kvm: convert to read_sys_reg64 Cornelia Huck
@ 2023-10-17 12:15 ` Peter Maydell
  3 siblings, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2023-10-17 12:15 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: Paolo Bonzini, Gavin Shan, qemu-arm, qemu-devel, kvm

On Tue, 10 Oct 2023 at 15:25, Cornelia Huck <cohuck@redhat.com> wrote:
>
> I sent this cleanup first... in mid-July (ugh). But better late than never, I guess.
>
> From v1:
> - fix buglets (thanks Gavin)
> - add patch 3 on top
>
> The kvm_{get,set}_one_reg functions have been around for a very long
> time, and using them instead of open-coding the ioctl invocations
> saves lines of code, and gives us a tracepoint as well. They cannot
> be used by invocations of the ioctl not acting on a CPUState, but
> that still leaves a lot of conversions in the target/arm code.

I've applied patches 1 and 2 to target-arm.next.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-10-17 15:28 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-10 14:24 [PATCH v2 0/3] arm/kvm: use kvm_{get,set}_one_reg Cornelia Huck
2023-10-10 14:24 ` [PATCH v2 1/3] arm/kvm: convert to kvm_set_one_reg Cornelia Huck
2023-10-11  2:44   ` Philippe Mathieu-Daudé
2023-10-10 14:24 ` [PATCH v2 2/3] arm/kvm: convert to kvm_get_one_reg Cornelia Huck
2023-10-11  2:44   ` Philippe Mathieu-Daudé
2023-10-10 14:24 ` [PATCH v2 3/3] arm/kvm: convert to read_sys_reg64 Cornelia Huck
2023-10-11  0:02   ` Gavin Shan
2023-10-17 12:09   ` Peter Maydell
2023-10-17 15:28     ` Cornelia Huck
2023-10-17 12:15 ` [PATCH v2 0/3] arm/kvm: use kvm_{get,set}_one_reg Peter Maydell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).