* [PATCH for-8.2 0/2] arm/kvm: use kvm_{get,set}_one_reg
@ 2023-07-18 11:14 Cornelia Huck
2023-07-18 11:14 ` [PATCH for-8.2 1/2] arm/kvm: convert to kvm_set_one_reg Cornelia Huck
2023-07-18 11:14 ` [PATCH for-8.2 2/2] arm/kvm: convert to kvm_get_one_reg Cornelia Huck
0 siblings, 2 replies; 11+ messages in thread
From: Cornelia Huck @ 2023-07-18 11:14 UTC (permalink / raw)
To: Peter Maydell, Paolo Bonzini; +Cc: qemu-arm, qemu-devel, kvm, Cornelia Huck
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 (2):
arm/kvm: convert to kvm_set_one_reg
arm/kvm: convert to kvm_get_one_reg
target/arm/kvm.c | 28 +++--------
target/arm/kvm64.c | 123 ++++++++++++---------------------------------
2 files changed, 39 insertions(+), 112 deletions(-)
--
2.41.0
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH for-8.2 1/2] arm/kvm: convert to kvm_set_one_reg 2023-07-18 11:14 [PATCH for-8.2 0/2] arm/kvm: use kvm_{get,set}_one_reg Cornelia Huck @ 2023-07-18 11:14 ` Cornelia Huck 2023-07-24 2:26 ` Gavin Shan 2023-07-18 11:14 ` [PATCH for-8.2 2/2] arm/kvm: convert to kvm_get_one_reg Cornelia Huck 1 sibling, 1 reply; 11+ messages in thread From: Cornelia Huck @ 2023-07-18 11:14 UTC (permalink / raw) To: Peter Maydell, Paolo Bonzini; +Cc: qemu-arm, qemu-devel, kvm, Cornelia Huck We can neaten the code by switching to the kvm_set_one_reg function. 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 b4c7654f4980..cdbffc3c6e0d 100644 --- a/target/arm/kvm.c +++ b/target/arm/kvm.c @@ -561,7 +561,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; @@ -570,19 +569,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 @@ -703,17 +700,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, ®); + 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 94bbd9661fd3..b4d02dff5381 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, ®); + return kvm_set_one_reg(cs, KVM_REG_ARM64_SVE_VLS, &vls[0]); } #define ARM_CPU_ID_MPIDR 3, 0, 0, 0, 5 @@ -725,19 +721,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, ®); if (ret) { return ret; } @@ -758,14 +752,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, ®); + ret = kvm_set_one_reg(cs, KVM_REG_ARM64_SVE_ZREG(n, 0), r); if (ret) { return ret; } @@ -774,9 +765,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, ®); + ret = kvm_set_one_reg(cs, KVM_REG_ARM64_SVE_PREG(n, 0), r); if (ret) { return ret; } @@ -784,9 +773,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, ®); + ret = kvm_set_one_reg(cs, KVM_REG_ARM64_SVE_FFR(0), r); if (ret) { return ret; } @@ -796,7 +783,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; @@ -813,9 +799,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, ®); + ret = kvm_set_one_reg(cs, AARCH64_CORE_REG(regs.regs[i]), + &env->xregs[i]); if (ret) { return ret; } @@ -826,16 +811,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, ®); + 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, ®); + ret = kvm_set_one_reg(cs, AARCH64_CORE_REG(sp_el1), &env->sp_el[1]); if (ret) { return ret; } @@ -846,23 +827,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, ®); + 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, ®); + 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, ®); + ret = kvm_set_one_reg(cs, AARCH64_CORE_REG(elr_el1), &env->elr_el[1]); if (ret) { return ret; } @@ -881,9 +856,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, ®); + ret = kvm_set_one_reg(cs, AARCH64_CORE_REG(spsr[i]), + &env->banked_spsr[i + 1]); if (ret) { return ret; } @@ -898,18 +872,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, ®); + 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, ®); + 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] 11+ messages in thread
* Re: [PATCH for-8.2 1/2] arm/kvm: convert to kvm_set_one_reg 2023-07-18 11:14 ` [PATCH for-8.2 1/2] arm/kvm: convert to kvm_set_one_reg Cornelia Huck @ 2023-07-24 2:26 ` Gavin Shan 2023-07-24 8:47 ` Cornelia Huck 0 siblings, 1 reply; 11+ messages in thread From: Gavin Shan @ 2023-07-24 2:26 UTC (permalink / raw) To: Cornelia Huck, Peter Maydell, Paolo Bonzini; +Cc: qemu-arm, qemu-devel, kvm Hi Connie, On 7/18/23 21:14, Cornelia Huck wrote: > We can neaten the code by switching to the kvm_set_one_reg function. > > 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(-) > Some wrong replacements to be fixed in kvm_arch_put_fpsimd() as below. Apart from that, LGTM: Reviewed-by: Gavin Shan <gshan@redhat.com> > diff --git a/target/arm/kvm.c b/target/arm/kvm.c > index b4c7654f4980..cdbffc3c6e0d 100644 > --- a/target/arm/kvm.c > +++ b/target/arm/kvm.c > @@ -561,7 +561,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; > @@ -570,19 +569,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 > @@ -703,17 +700,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, ®); > + 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 94bbd9661fd3..b4d02dff5381 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, ®); > + return kvm_set_one_reg(cs, KVM_REG_ARM64_SVE_VLS, &vls[0]); > } > > #define ARM_CPU_ID_MPIDR 3, 0, 0, 0, 5 > @@ -725,19 +721,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); ^^^^^^^ s/&fp_val/fp_val > #else > - reg.addr = (uintptr_t)q; > + ret = kvm_set_one_reg(cs, AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]), &q); ^^^ s/&q/q > #endif > - reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]); > - ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); > if (ret) { > return ret; > } > @@ -758,14 +752,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, ®); > + ret = kvm_set_one_reg(cs, KVM_REG_ARM64_SVE_ZREG(n, 0), r); > if (ret) { > return ret; > } > @@ -774,9 +765,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, ®); > + ret = kvm_set_one_reg(cs, KVM_REG_ARM64_SVE_PREG(n, 0), r); > if (ret) { > return ret; > } > @@ -784,9 +773,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, ®); > + ret = kvm_set_one_reg(cs, KVM_REG_ARM64_SVE_FFR(0), r); > if (ret) { > return ret; > } > @@ -796,7 +783,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; > @@ -813,9 +799,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, ®); > + ret = kvm_set_one_reg(cs, AARCH64_CORE_REG(regs.regs[i]), > + &env->xregs[i]); > if (ret) { > return ret; > } > @@ -826,16 +811,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, ®); > + 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, ®); > + ret = kvm_set_one_reg(cs, AARCH64_CORE_REG(sp_el1), &env->sp_el[1]); > if (ret) { > return ret; > } > @@ -846,23 +827,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, ®); > + 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, ®); > + 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, ®); > + ret = kvm_set_one_reg(cs, AARCH64_CORE_REG(elr_el1), &env->elr_el[1]); > if (ret) { > return ret; > } > @@ -881,9 +856,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, ®); > + ret = kvm_set_one_reg(cs, AARCH64_CORE_REG(spsr[i]), > + &env->banked_spsr[i + 1]); > if (ret) { > return ret; > } > @@ -898,18 +872,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, ®); > + 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, ®); > + ret = kvm_set_one_reg(cs, AARCH64_SIMD_CTRL_REG(fp_regs.fpcr), &fpr); > if (ret) { > return ret; > } Thanks, Gavin ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-8.2 1/2] arm/kvm: convert to kvm_set_one_reg 2023-07-24 2:26 ` Gavin Shan @ 2023-07-24 8:47 ` Cornelia Huck 0 siblings, 0 replies; 11+ messages in thread From: Cornelia Huck @ 2023-07-24 8:47 UTC (permalink / raw) To: Gavin Shan, Peter Maydell, Paolo Bonzini; +Cc: qemu-arm, qemu-devel, kvm On Mon, Jul 24 2023, Gavin Shan <gshan@redhat.com> wrote: > Hi Connie, > > On 7/18/23 21:14, Cornelia Huck wrote: >> We can neaten the code by switching to the kvm_set_one_reg function. >> >> 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(-) >> > > Some wrong replacements to be fixed in kvm_arch_put_fpsimd() as below. > Apart from that, LGTM: > > Reviewed-by: Gavin Shan <gshan@redhat.com> > @@ -725,19 +721,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); > ^^^^^^^ > s/&fp_val/fp_val >> #else >> - reg.addr = (uintptr_t)q; >> + ret = kvm_set_one_reg(cs, AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]), &q); > ^^^ > s/&q/q > >> #endif Whoops, I thought I had double-checked these... ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH for-8.2 2/2] arm/kvm: convert to kvm_get_one_reg 2023-07-18 11:14 [PATCH for-8.2 0/2] arm/kvm: use kvm_{get,set}_one_reg Cornelia Huck 2023-07-18 11:14 ` [PATCH for-8.2 1/2] arm/kvm: convert to kvm_set_one_reg Cornelia Huck @ 2023-07-18 11:14 ` Cornelia Huck 2023-07-24 2:35 ` Gavin Shan 2023-07-31 7:15 ` Gavin Shan 1 sibling, 2 replies; 11+ messages in thread From: Cornelia Huck @ 2023-07-18 11:14 UTC (permalink / raw) To: Peter Maydell, Paolo Bonzini; +Cc: 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. 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 cdbffc3c6e0d..4123f6dc9d72 100644 --- a/target/arm/kvm.c +++ b/target/arm/kvm.c @@ -525,24 +525,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(); @@ -678,17 +673,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, ®); + 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 b4d02dff5381..66b52d6f8d23 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -908,14 +908,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, ®); + ret = kvm_get_one_reg(cs, AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]), q); if (ret) { return ret; } else { @@ -939,15 +936,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, ®); + ret = kvm_get_one_reg(cs, KVM_REG_ARM64_SVE_ZREG(n, 0), r); if (ret) { return ret; } @@ -956,9 +950,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, ®); + ret = kvm_get_one_reg(cs, KVM_REG_ARM64_SVE_PREG(n, 0), r); if (ret) { return ret; } @@ -966,9 +958,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, ®); + ret = kvm_get_one_reg(cs, KVM_REG_ARM64_SVE_FFR(0), r); if (ret) { return ret; } @@ -979,7 +969,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; @@ -989,31 +978,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, ®); + 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, ®); + 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, ®); + 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, ®); + ret = kvm_get_one_reg(cs, AARCH64_CORE_REG(regs.pstate), &val); if (ret) { return ret; } @@ -1030,9 +1012,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, ®); + ret = kvm_get_one_reg(cs, AARCH64_CORE_REG(regs.pc), &env->pc); if (ret) { return ret; } @@ -1046,9 +1026,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, ®); + ret = kvm_get_one_reg(cs, AARCH64_CORE_REG(elr_el1), &env->elr_el[1]); if (ret) { return ret; } @@ -1058,9 +1036,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, ®); + ret = kvm_get_one_reg(cs, AARCH64_CORE_REG(spsr[i]), + &env->banked_spsr[i + 1]); if (ret) { return ret; } @@ -1081,17 +1058,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, ®); + 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, ®); + 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] 11+ messages in thread
* Re: [PATCH for-8.2 2/2] arm/kvm: convert to kvm_get_one_reg 2023-07-18 11:14 ` [PATCH for-8.2 2/2] arm/kvm: convert to kvm_get_one_reg Cornelia Huck @ 2023-07-24 2:35 ` Gavin Shan 2023-07-24 8:48 ` Cornelia Huck 2023-07-31 7:15 ` Gavin Shan 1 sibling, 1 reply; 11+ messages in thread From: Gavin Shan @ 2023-07-24 2:35 UTC (permalink / raw) To: Cornelia Huck, Peter Maydell, Paolo Bonzini; +Cc: qemu-arm, qemu-devel, kvm Hi Connie, On 7/18/23 21:14, Cornelia Huck wrote: > We can neaten the code by switching the callers that work on a > CPUstate to the kvm_get_one_reg function. > > 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(-) > The replacements look good to me. However, I guess it's worty to apply the same replacements for target/arm/kvm64.c since we're here? [gshan@gshan arm]$ pwd /home/gshan/sandbox/q/target/arm [gshan@gshan arm]$ git grep KVM_GET_ONE_REG kvm64.c: err = ioctl(fd, KVM_GET_ONE_REG, &idreg); kvm64.c: return ioctl(fd, KVM_GET_ONE_REG, &idreg); kvm64.c: ret = ioctl(fdarray[2], KVM_GET_ONE_REG, ®); Thanks, Gavin > diff --git a/target/arm/kvm.c b/target/arm/kvm.c > index cdbffc3c6e0d..4123f6dc9d72 100644 > --- a/target/arm/kvm.c > +++ b/target/arm/kvm.c > @@ -525,24 +525,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(); > @@ -678,17 +673,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, ®); > + 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 b4d02dff5381..66b52d6f8d23 100644 > --- a/target/arm/kvm64.c > +++ b/target/arm/kvm64.c > @@ -908,14 +908,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, ®); > + ret = kvm_get_one_reg(cs, AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]), q); > if (ret) { > return ret; > } else { > @@ -939,15 +936,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, ®); > + ret = kvm_get_one_reg(cs, KVM_REG_ARM64_SVE_ZREG(n, 0), r); > if (ret) { > return ret; > } > @@ -956,9 +950,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, ®); > + ret = kvm_get_one_reg(cs, KVM_REG_ARM64_SVE_PREG(n, 0), r); > if (ret) { > return ret; > } > @@ -966,9 +958,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, ®); > + ret = kvm_get_one_reg(cs, KVM_REG_ARM64_SVE_FFR(0), r); > if (ret) { > return ret; > } > @@ -979,7 +969,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; > @@ -989,31 +978,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, ®); > + 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, ®); > + 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, ®); > + 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, ®); > + ret = kvm_get_one_reg(cs, AARCH64_CORE_REG(regs.pstate), &val); > if (ret) { > return ret; > } > @@ -1030,9 +1012,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, ®); > + ret = kvm_get_one_reg(cs, AARCH64_CORE_REG(regs.pc), &env->pc); > if (ret) { > return ret; > } > @@ -1046,9 +1026,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, ®); > + ret = kvm_get_one_reg(cs, AARCH64_CORE_REG(elr_el1), &env->elr_el[1]); > if (ret) { > return ret; > } > @@ -1058,9 +1036,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, ®); > + ret = kvm_get_one_reg(cs, AARCH64_CORE_REG(spsr[i]), > + &env->banked_spsr[i + 1]); > if (ret) { > return ret; > } > @@ -1081,17 +1058,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, ®); > + 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, ®); > + ret = kvm_get_one_reg(cs, AARCH64_SIMD_CTRL_REG(fp_regs.fpcr), &fpr); > if (ret) { > return ret; > } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-8.2 2/2] arm/kvm: convert to kvm_get_one_reg 2023-07-24 2:35 ` Gavin Shan @ 2023-07-24 8:48 ` Cornelia Huck 2023-07-25 0:01 ` Gavin Shan 0 siblings, 1 reply; 11+ messages in thread From: Cornelia Huck @ 2023-07-24 8:48 UTC (permalink / raw) To: Gavin Shan, Peter Maydell, Paolo Bonzini; +Cc: qemu-arm, qemu-devel, kvm On Mon, Jul 24 2023, Gavin Shan <gshan@redhat.com> wrote: > Hi Connie, > > On 7/18/23 21:14, Cornelia Huck wrote: >> We can neaten the code by switching the callers that work on a >> CPUstate to the kvm_get_one_reg function. >> >> 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(-) >> > > The replacements look good to me. However, I guess it's worty to apply > the same replacements for target/arm/kvm64.c since we're here? > > [gshan@gshan arm]$ pwd > /home/gshan/sandbox/q/target/arm > [gshan@gshan arm]$ git grep KVM_GET_ONE_REG > kvm64.c: err = ioctl(fd, KVM_GET_ONE_REG, &idreg); > kvm64.c: return ioctl(fd, KVM_GET_ONE_REG, &idreg); > kvm64.c: ret = ioctl(fdarray[2], KVM_GET_ONE_REG, ®); These are the callers that don't work on a CPUState (all in initial feature discovery IIRC), so they need to stay that way. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-8.2 2/2] arm/kvm: convert to kvm_get_one_reg 2023-07-24 8:48 ` Cornelia Huck @ 2023-07-25 0:01 ` Gavin Shan 2023-07-27 9:55 ` Cornelia Huck 0 siblings, 1 reply; 11+ messages in thread From: Gavin Shan @ 2023-07-25 0:01 UTC (permalink / raw) To: Cornelia Huck, Peter Maydell, Paolo Bonzini; +Cc: qemu-arm, qemu-devel, kvm On 7/24/23 18:48, Cornelia Huck wrote: > On Mon, Jul 24 2023, Gavin Shan <gshan@redhat.com> wrote: >> >> On 7/18/23 21:14, Cornelia Huck wrote: >>> We can neaten the code by switching the callers that work on a >>> CPUstate to the kvm_get_one_reg function. >>> >>> 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(-) >>> >> >> The replacements look good to me. However, I guess it's worty to apply >> the same replacements for target/arm/kvm64.c since we're here? >> >> [gshan@gshan arm]$ pwd >> /home/gshan/sandbox/q/target/arm >> [gshan@gshan arm]$ git grep KVM_GET_ONE_REG >> kvm64.c: err = ioctl(fd, KVM_GET_ONE_REG, &idreg); >> kvm64.c: return ioctl(fd, KVM_GET_ONE_REG, &idreg); >> kvm64.c: ret = ioctl(fdarray[2], KVM_GET_ONE_REG, ®); > > These are the callers that don't work on a CPUState (all in initial > feature discovery IIRC), so they need to stay that way. > Right, All these ioctl commands are issued when CPUState isn't around. However, there are two wrappers read_sys_{reg32, reg64}(). The ioctl call in kvm_arm_sve_get_vls() can be replaced by read_sys_reg64(). I guess it'd better to do this in a separate patch if you agree. Thanks, Gavin ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-8.2 2/2] arm/kvm: convert to kvm_get_one_reg 2023-07-25 0:01 ` Gavin Shan @ 2023-07-27 9:55 ` Cornelia Huck 2023-10-10 10:09 ` Cornelia Huck 0 siblings, 1 reply; 11+ messages in thread From: Cornelia Huck @ 2023-07-27 9:55 UTC (permalink / raw) To: Gavin Shan, Peter Maydell, Paolo Bonzini; +Cc: qemu-arm, qemu-devel, kvm On Tue, Jul 25 2023, Gavin Shan <gshan@redhat.com> wrote: > On 7/24/23 18:48, Cornelia Huck wrote: >> On Mon, Jul 24 2023, Gavin Shan <gshan@redhat.com> wrote: >>> >>> On 7/18/23 21:14, Cornelia Huck wrote: >>>> We can neaten the code by switching the callers that work on a >>>> CPUstate to the kvm_get_one_reg function. >>>> >>>> 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(-) >>>> >>> >>> The replacements look good to me. However, I guess it's worty to apply >>> the same replacements for target/arm/kvm64.c since we're here? >>> >>> [gshan@gshan arm]$ pwd >>> /home/gshan/sandbox/q/target/arm >>> [gshan@gshan arm]$ git grep KVM_GET_ONE_REG >>> kvm64.c: err = ioctl(fd, KVM_GET_ONE_REG, &idreg); >>> kvm64.c: return ioctl(fd, KVM_GET_ONE_REG, &idreg); >>> kvm64.c: ret = ioctl(fdarray[2], KVM_GET_ONE_REG, ®); >> >> These are the callers that don't work on a CPUState (all in initial >> feature discovery IIRC), so they need to stay that way. >> > > Right, All these ioctl commands are issued when CPUState isn't around. However, there > are two wrappers read_sys_{reg32, reg64}(). The ioctl call in kvm_arm_sve_get_vls() > can be replaced by read_sys_reg64(). I guess it'd better to do this in a separate > patch if you agree. Yes, we could do that, but I'm not sure how much it adds to the code... in any case, I agree that this would be a separate patch. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-8.2 2/2] arm/kvm: convert to kvm_get_one_reg 2023-07-27 9:55 ` Cornelia Huck @ 2023-10-10 10:09 ` Cornelia Huck 0 siblings, 0 replies; 11+ messages in thread From: Cornelia Huck @ 2023-10-10 10:09 UTC (permalink / raw) To: Gavin Shan, Peter Maydell, Paolo Bonzini; +Cc: qemu-arm, qemu-devel, kvm [spooky season is coming up, so time for some thread necromancy!] On Thu, Jul 27 2023, Cornelia Huck <cohuck@redhat.com> wrote: > On Tue, Jul 25 2023, Gavin Shan <gshan@redhat.com> wrote: > >> On 7/24/23 18:48, Cornelia Huck wrote: >>> On Mon, Jul 24 2023, Gavin Shan <gshan@redhat.com> wrote: >>>> >>>> On 7/18/23 21:14, Cornelia Huck wrote: >>>>> We can neaten the code by switching the callers that work on a >>>>> CPUstate to the kvm_get_one_reg function. >>>>> >>>>> 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(-) >>>>> >>>> >>>> The replacements look good to me. However, I guess it's worty to apply >>>> the same replacements for target/arm/kvm64.c since we're here? >>>> >>>> [gshan@gshan arm]$ pwd >>>> /home/gshan/sandbox/q/target/arm >>>> [gshan@gshan arm]$ git grep KVM_GET_ONE_REG >>>> kvm64.c: err = ioctl(fd, KVM_GET_ONE_REG, &idreg); >>>> kvm64.c: return ioctl(fd, KVM_GET_ONE_REG, &idreg); >>>> kvm64.c: ret = ioctl(fdarray[2], KVM_GET_ONE_REG, ®); >>> >>> These are the callers that don't work on a CPUState (all in initial >>> feature discovery IIRC), so they need to stay that way. >>> >> >> Right, All these ioctl commands are issued when CPUState isn't around. However, there >> are two wrappers read_sys_{reg32, reg64}(). The ioctl call in kvm_arm_sve_get_vls() >> can be replaced by read_sys_reg64(). I guess it'd better to do this in a separate >> patch if you agree. > > Yes, we could do that, but I'm not sure how much it adds to the > code... in any case, I agree that this would be a separate patch. This series has managed to bubble up to the top of my todo list again, and I think I'll just go ahead and include that as a separate change on top. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-8.2 2/2] arm/kvm: convert to kvm_get_one_reg 2023-07-18 11:14 ` [PATCH for-8.2 2/2] arm/kvm: convert to kvm_get_one_reg Cornelia Huck 2023-07-24 2:35 ` Gavin Shan @ 2023-07-31 7:15 ` Gavin Shan 1 sibling, 0 replies; 11+ messages in thread From: Gavin Shan @ 2023-07-31 7:15 UTC (permalink / raw) To: Cornelia Huck, Peter Maydell, Paolo Bonzini; +Cc: qemu-arm, qemu-devel, kvm On 7/18/23 21:14, Cornelia Huck wrote: > We can neaten the code by switching the callers that work on a > CPUstate to the kvm_get_one_reg function. > > 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: Gavin Shan <gshan@redhat.com> Thanks, Gavin ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-10-10 10:11 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-18 11:14 [PATCH for-8.2 0/2] arm/kvm: use kvm_{get,set}_one_reg Cornelia Huck
2023-07-18 11:14 ` [PATCH for-8.2 1/2] arm/kvm: convert to kvm_set_one_reg Cornelia Huck
2023-07-24 2:26 ` Gavin Shan
2023-07-24 8:47 ` Cornelia Huck
2023-07-18 11:14 ` [PATCH for-8.2 2/2] arm/kvm: convert to kvm_get_one_reg Cornelia Huck
2023-07-24 2:35 ` Gavin Shan
2023-07-24 8:48 ` Cornelia Huck
2023-07-25 0:01 ` Gavin Shan
2023-07-27 9:55 ` Cornelia Huck
2023-10-10 10:09 ` Cornelia Huck
2023-07-31 7:15 ` Gavin Shan
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).