* [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
* [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 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 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 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
* 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-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
* 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
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).