* [PATCH v2] KVM: Optimize kvm_arch_vcpu_ioctl_run function
@ 2020-04-16 5:10 Tianjia Zhang
2020-04-16 7:03 ` Vitaly Kuznetsov
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Tianjia Zhang @ 2020-04-16 5:10 UTC (permalink / raw)
To: pbonzini, tsbogend, paulus, mpe, benh, borntraeger, frankja,
david, cohuck, heiko.carstens, gor, sean.j.christopherson,
vkuznets, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa,
maz, james.morse, julien.thierry.kdev, suzuki.poulose,
christoffer.dall, peterx, thuth
Cc: kvm, linux-arm-kernel, kvmarm, linux-mips, kvm-ppc, linuxppc-dev,
linux-s390, linux-kernel, tianjia.zhang
In earlier versions of kvm, 'kvm_run' is an independent structure
and is not included in the vcpu structure. At present, 'kvm_run'
is already included in the vcpu structure, so the parameter
'kvm_run' is redundant.
This patch simplify the function definition, removes the extra
'kvm_run' parameter, and extract it from the 'kvm_vcpu' structure
if necessary.
Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
---
v2 change:
remove 'kvm_run' parameter and extract it from 'kvm_vcpu'
arch/mips/kvm/mips.c | 3 ++-
arch/powerpc/kvm/powerpc.c | 3 ++-
arch/s390/kvm/kvm-s390.c | 3 ++-
arch/x86/kvm/x86.c | 11 ++++++-----
include/linux/kvm_host.h | 2 +-
virt/kvm/arm/arm.c | 6 +++---
virt/kvm/kvm_main.c | 2 +-
7 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
index 8f05dd0a0f4e..ec24adf4857e 100644
--- a/arch/mips/kvm/mips.c
+++ b/arch/mips/kvm/mips.c
@@ -439,8 +439,9 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
return -ENOIOCTLCMD;
}
-int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
+int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
{
+ struct kvm_run *run = vcpu->run;
int r = -EINTR;
vcpu_load(vcpu);
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index e15166b0a16d..7e24691e138a 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -1764,8 +1764,9 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
return r;
}
-int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
+int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
{
+ struct kvm_run *run = vcpu->run;
int r;
vcpu_load(vcpu);
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 19a81024fe16..443af3ead739 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -4333,8 +4333,9 @@ static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
store_regs_fmt2(vcpu, kvm_run);
}
-int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
+int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
{
+ struct kvm_run *kvm_run = vcpu->run;
int rc;
if (kvm_run->immediate_exit)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3bf2ecafd027..a0338e86c90f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8707,8 +8707,9 @@ static void kvm_put_guest_fpu(struct kvm_vcpu *vcpu)
trace_kvm_fpu(0);
}
-int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
+int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
{
+ struct kvm_run *kvm_run = vcpu->run;
int r;
vcpu_load(vcpu);
@@ -8726,18 +8727,18 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
r = -EAGAIN;
if (signal_pending(current)) {
r = -EINTR;
- vcpu->run->exit_reason = KVM_EXIT_INTR;
+ kvm_run->exit_reason = KVM_EXIT_INTR;
++vcpu->stat.signal_exits;
}
goto out;
}
- if (vcpu->run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) {
+ if (kvm_run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) {
r = -EINVAL;
goto out;
}
- if (vcpu->run->kvm_dirty_regs) {
+ if (kvm_run->kvm_dirty_regs) {
r = sync_regs(vcpu);
if (r != 0)
goto out;
@@ -8767,7 +8768,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
out:
kvm_put_guest_fpu(vcpu);
- if (vcpu->run->kvm_valid_regs)
+ if (kvm_run->kvm_valid_regs)
store_regs(vcpu);
post_kvm_run_save(vcpu);
kvm_sigset_deactivate(vcpu);
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 6d58beb65454..1e17ef719595 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -866,7 +866,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
struct kvm_mp_state *mp_state);
int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
struct kvm_guest_debug *dbg);
-int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run);
+int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu);
int kvm_arch_init(void *opaque);
void kvm_arch_exit(void);
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 48d0ec44ad77..f5390ac2165b 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -639,7 +639,6 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
/**
* kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute guest code
* @vcpu: The VCPU pointer
- * @run: The kvm_run structure pointer used for userspace state exchange
*
* This function is called through the VCPU_RUN ioctl called from user space. It
* will execute VM code in a loop until the time slice for the process is used
@@ -647,8 +646,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
* return with return value 0 and with the kvm_run structure filled in with the
* required data for the requested emulation.
*/
-int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
+int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
{
+ struct kvm_run *run = vcpu->run;
int ret;
if (unlikely(!kvm_vcpu_initialized(vcpu)))
@@ -659,7 +659,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
return ret;
if (run->exit_reason == KVM_EXIT_MMIO) {
- ret = kvm_handle_mmio_return(vcpu, vcpu->run);
+ ret = kvm_handle_mmio_return(vcpu, run);
if (ret)
return ret;
}
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 74bdb7bf3295..e18faea89146 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3135,7 +3135,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
synchronize_rcu();
put_pid(oldpid);
}
- r = kvm_arch_vcpu_ioctl_run(vcpu, vcpu->run);
+ r = kvm_arch_vcpu_ioctl_run(vcpu);
trace_kvm_userspace_exit(vcpu->run->exit_reason, r);
break;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2] KVM: Optimize kvm_arch_vcpu_ioctl_run function 2020-04-16 5:10 [PATCH v2] KVM: Optimize kvm_arch_vcpu_ioctl_run function Tianjia Zhang @ 2020-04-16 7:03 ` Vitaly Kuznetsov 2020-04-16 8:28 ` Marc Zyngier 2020-04-16 8:10 ` Cornelia Huck 2020-04-16 14:16 ` Paolo Bonzini 2 siblings, 1 reply; 9+ messages in thread From: Vitaly Kuznetsov @ 2020-04-16 7:03 UTC (permalink / raw) To: Tianjia Zhang Cc: kvm, linux-arm-kernel, kvmarm, linux-mips, kvm-ppc, linuxppc-dev, linux-s390, linux-kernel, tianjia.zhang, pbonzini, tsbogend, paulus, mpe, benh, borntraeger, frankja, david, cohuck, heiko.carstens, gor, sean.j.christopherson, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa, maz, james.morse, julien.thierry.kdev, suzuki.poulose, christoffer.dall, peterx, thuth Tianjia Zhang <tianjia.zhang@linux.alibaba.com> writes: > In earlier versions of kvm, 'kvm_run' is an independent structure > and is not included in the vcpu structure. At present, 'kvm_run' > is already included in the vcpu structure, so the parameter > 'kvm_run' is redundant. > > This patch simplify the function definition, removes the extra > 'kvm_run' parameter, and extract it from the 'kvm_vcpu' structure > if necessary. > > Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> > --- > > v2 change: > remove 'kvm_run' parameter and extract it from 'kvm_vcpu' > > arch/mips/kvm/mips.c | 3 ++- > arch/powerpc/kvm/powerpc.c | 3 ++- > arch/s390/kvm/kvm-s390.c | 3 ++- > arch/x86/kvm/x86.c | 11 ++++++----- > include/linux/kvm_host.h | 2 +- > virt/kvm/arm/arm.c | 6 +++--- > virt/kvm/kvm_main.c | 2 +- > 7 files changed, 17 insertions(+), 13 deletions(-) > > diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c > index 8f05dd0a0f4e..ec24adf4857e 100644 > --- a/arch/mips/kvm/mips.c > +++ b/arch/mips/kvm/mips.c > @@ -439,8 +439,9 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, > return -ENOIOCTLCMD; > } > > -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > { > + struct kvm_run *run = vcpu->run; > int r = -EINTR; > > vcpu_load(vcpu); > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c > index e15166b0a16d..7e24691e138a 100644 > --- a/arch/powerpc/kvm/powerpc.c > +++ b/arch/powerpc/kvm/powerpc.c > @@ -1764,8 +1764,9 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg) > return r; > } > > -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > { > + struct kvm_run *run = vcpu->run; > int r; > > vcpu_load(vcpu); > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 19a81024fe16..443af3ead739 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -4333,8 +4333,9 @@ static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > store_regs_fmt2(vcpu, kvm_run); > } > > -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > { > + struct kvm_run *kvm_run = vcpu->run; > int rc; > > if (kvm_run->immediate_exit) > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 3bf2ecafd027..a0338e86c90f 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -8707,8 +8707,9 @@ static void kvm_put_guest_fpu(struct kvm_vcpu *vcpu) > trace_kvm_fpu(0); > } > > -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > { > + struct kvm_run *kvm_run = vcpu->run; > int r; > > vcpu_load(vcpu); > @@ -8726,18 +8727,18 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > r = -EAGAIN; > if (signal_pending(current)) { > r = -EINTR; > - vcpu->run->exit_reason = KVM_EXIT_INTR; > + kvm_run->exit_reason = KVM_EXIT_INTR; > ++vcpu->stat.signal_exits; > } > goto out; > } > > - if (vcpu->run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { > + if (kvm_run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { > r = -EINVAL; > goto out; > } > > - if (vcpu->run->kvm_dirty_regs) { > + if (kvm_run->kvm_dirty_regs) { > r = sync_regs(vcpu); > if (r != 0) > goto out; > @@ -8767,7 +8768,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > > out: > kvm_put_guest_fpu(vcpu); > - if (vcpu->run->kvm_valid_regs) > + if (kvm_run->kvm_valid_regs) > store_regs(vcpu); > post_kvm_run_save(vcpu); > kvm_sigset_deactivate(vcpu); > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 6d58beb65454..1e17ef719595 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -866,7 +866,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu, > struct kvm_mp_state *mp_state); > int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, > struct kvm_guest_debug *dbg); > -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run); > +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu); > > int kvm_arch_init(void *opaque); > void kvm_arch_exit(void); > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > index 48d0ec44ad77..f5390ac2165b 100644 > --- a/virt/kvm/arm/arm.c > +++ b/virt/kvm/arm/arm.c > @@ -639,7 +639,6 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu) > /** > * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute guest code > * @vcpu: The VCPU pointer > - * @run: The kvm_run structure pointer used for userspace state exchange > * > * This function is called through the VCPU_RUN ioctl called from user space. It > * will execute VM code in a loop until the time slice for the process is used > @@ -647,8 +646,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu) > * return with return value 0 and with the kvm_run structure filled in with the > * required data for the requested emulation. > */ > -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > { > + struct kvm_run *run = vcpu->run; > int ret; > > if (unlikely(!kvm_vcpu_initialized(vcpu))) > @@ -659,7 +659,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > return ret; > > if (run->exit_reason == KVM_EXIT_MMIO) { > - ret = kvm_handle_mmio_return(vcpu, vcpu->run); > + ret = kvm_handle_mmio_return(vcpu, run); I don't know much about ARM but this also seems redundant, kvm_handle_mmio_return() is also able to extruct 'struct kvm_run' from' 'struct kvm_vcpu'. This likely deserves it's own patch though. > if (ret) > return ret; > } > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 74bdb7bf3295..e18faea89146 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -3135,7 +3135,7 @@ static long kvm_vcpu_ioctl(struct file *filp, > synchronize_rcu(); > put_pid(oldpid); > } > - r = kvm_arch_vcpu_ioctl_run(vcpu, vcpu->run); > + r = kvm_arch_vcpu_ioctl_run(vcpu); > trace_kvm_userspace_exit(vcpu->run->exit_reason, r); > break; > } Looked at non-x86 arches just briefly but there seems to be no controversy here, so Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> -- Vitaly ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] KVM: Optimize kvm_arch_vcpu_ioctl_run function 2020-04-16 7:03 ` Vitaly Kuznetsov @ 2020-04-16 8:28 ` Marc Zyngier 2020-04-16 8:45 ` Tianjia Zhang 0 siblings, 1 reply; 9+ messages in thread From: Marc Zyngier @ 2020-04-16 8:28 UTC (permalink / raw) To: Vitaly Kuznetsov Cc: Tianjia Zhang, kvm, linux-arm-kernel, kvmarm, linux-mips, kvm-ppc, linuxppc-dev, linux-s390, linux-kernel, pbonzini, tsbogend, paulus, mpe, benh, borntraeger, frankja, david, cohuck, heiko.carstens, gor, sean.j.christopherson, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa, james.morse, julien.thierry.kdev, suzuki.poulose, christoffer.dall, peterx, thuth On 2020-04-16 08:03, Vitaly Kuznetsov wrote: > Tianjia Zhang <tianjia.zhang@linux.alibaba.com> writes: > >> In earlier versions of kvm, 'kvm_run' is an independent structure >> and is not included in the vcpu structure. At present, 'kvm_run' >> is already included in the vcpu structure, so the parameter >> 'kvm_run' is redundant. >> >> This patch simplify the function definition, removes the extra >> 'kvm_run' parameter, and extract it from the 'kvm_vcpu' structure >> if necessary. >> >> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> >> --- >> >> v2 change: >> remove 'kvm_run' parameter and extract it from 'kvm_vcpu' >> >> arch/mips/kvm/mips.c | 3 ++- >> arch/powerpc/kvm/powerpc.c | 3 ++- >> arch/s390/kvm/kvm-s390.c | 3 ++- >> arch/x86/kvm/x86.c | 11 ++++++----- >> include/linux/kvm_host.h | 2 +- >> virt/kvm/arm/arm.c | 6 +++--- >> virt/kvm/kvm_main.c | 2 +- >> 7 files changed, 17 insertions(+), 13 deletions(-) >> >> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c >> index 8f05dd0a0f4e..ec24adf4857e 100644 >> --- a/arch/mips/kvm/mips.c >> +++ b/arch/mips/kvm/mips.c >> @@ -439,8 +439,9 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct >> kvm_vcpu *vcpu, >> return -ENOIOCTLCMD; >> } >> >> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run >> *run) >> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) >> { >> + struct kvm_run *run = vcpu->run; >> int r = -EINTR; >> >> vcpu_load(vcpu); >> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c >> index e15166b0a16d..7e24691e138a 100644 >> --- a/arch/powerpc/kvm/powerpc.c >> +++ b/arch/powerpc/kvm/powerpc.c >> @@ -1764,8 +1764,9 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu >> *vcpu, struct kvm_one_reg *reg) >> return r; >> } >> >> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run >> *run) >> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) >> { >> + struct kvm_run *run = vcpu->run; >> int r; >> >> vcpu_load(vcpu); >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index 19a81024fe16..443af3ead739 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -4333,8 +4333,9 @@ static void store_regs(struct kvm_vcpu *vcpu, >> struct kvm_run *kvm_run) >> store_regs_fmt2(vcpu, kvm_run); >> } >> >> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run >> *kvm_run) >> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) >> { >> + struct kvm_run *kvm_run = vcpu->run; >> int rc; >> >> if (kvm_run->immediate_exit) >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 3bf2ecafd027..a0338e86c90f 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -8707,8 +8707,9 @@ static void kvm_put_guest_fpu(struct kvm_vcpu >> *vcpu) >> trace_kvm_fpu(0); >> } >> >> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run >> *kvm_run) >> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) >> { >> + struct kvm_run *kvm_run = vcpu->run; >> int r; >> >> vcpu_load(vcpu); >> @@ -8726,18 +8727,18 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu >> *vcpu, struct kvm_run *kvm_run) >> r = -EAGAIN; >> if (signal_pending(current)) { >> r = -EINTR; >> - vcpu->run->exit_reason = KVM_EXIT_INTR; >> + kvm_run->exit_reason = KVM_EXIT_INTR; >> ++vcpu->stat.signal_exits; >> } >> goto out; >> } >> >> - if (vcpu->run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { >> + if (kvm_run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { >> r = -EINVAL; >> goto out; >> } >> >> - if (vcpu->run->kvm_dirty_regs) { >> + if (kvm_run->kvm_dirty_regs) { >> r = sync_regs(vcpu); >> if (r != 0) >> goto out; >> @@ -8767,7 +8768,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu >> *vcpu, struct kvm_run *kvm_run) >> >> out: >> kvm_put_guest_fpu(vcpu); >> - if (vcpu->run->kvm_valid_regs) >> + if (kvm_run->kvm_valid_regs) >> store_regs(vcpu); >> post_kvm_run_save(vcpu); >> kvm_sigset_deactivate(vcpu); >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >> index 6d58beb65454..1e17ef719595 100644 >> --- a/include/linux/kvm_host.h >> +++ b/include/linux/kvm_host.h >> @@ -866,7 +866,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct >> kvm_vcpu *vcpu, >> struct kvm_mp_state *mp_state); >> int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, >> struct kvm_guest_debug *dbg); >> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run >> *kvm_run); >> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu); >> >> int kvm_arch_init(void *opaque); >> void kvm_arch_exit(void); >> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c >> index 48d0ec44ad77..f5390ac2165b 100644 >> --- a/virt/kvm/arm/arm.c >> +++ b/virt/kvm/arm/arm.c >> @@ -639,7 +639,6 @@ static void check_vcpu_requests(struct kvm_vcpu >> *vcpu) >> /** >> * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute >> guest code >> * @vcpu: The VCPU pointer >> - * @run: The kvm_run structure pointer used for userspace state >> exchange >> * >> * This function is called through the VCPU_RUN ioctl called from >> user space. It >> * will execute VM code in a loop until the time slice for the >> process is used >> @@ -647,8 +646,9 @@ static void check_vcpu_requests(struct kvm_vcpu >> *vcpu) >> * return with return value 0 and with the kvm_run structure filled >> in with the >> * required data for the requested emulation. >> */ >> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run >> *run) >> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) >> { >> + struct kvm_run *run = vcpu->run; >> int ret; >> >> if (unlikely(!kvm_vcpu_initialized(vcpu))) >> @@ -659,7 +659,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, >> struct kvm_run *run) >> return ret; >> >> if (run->exit_reason == KVM_EXIT_MMIO) { >> - ret = kvm_handle_mmio_return(vcpu, vcpu->run); >> + ret = kvm_handle_mmio_return(vcpu, run); > > I don't know much about ARM but this also seems redundant, > kvm_handle_mmio_return() is also able to extruct 'struct kvm_run' from' > 'struct kvm_vcpu'. This likely deserves it's own patch though. Something like this (untested): diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 32c8a675e5a4..82978995bdd6 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -490,7 +490,7 @@ void handle_exit_early(struct kvm_vcpu *vcpu, struct kvm_run *run, void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long data); unsigned long kvm_mmio_read_buf(const void *buf, unsigned int len); -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run); +int kvm_handle_mmio_return(struct kvm_vcpu *vcpu); int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run, phys_addr_t fault_ipa); diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c index 4e0366759726..3b2c822b4859 100644 --- a/virt/kvm/arm/mmio.c +++ b/virt/kvm/arm/mmio.c @@ -77,9 +77,8 @@ unsigned long kvm_mmio_read_buf(const void *buf, unsigned int len) * or in-kernel IO emulation * * @vcpu: The VCPU pointer - * @run: The VCPU run struct containing the mmio data */ -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) +int kvm_handle_mmio_return(struct kvm_vcpu *vcpu) { unsigned long data; unsigned int len; @@ -93,7 +92,7 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) if (!kvm_vcpu_dabt_iswrite(vcpu)) { len = kvm_vcpu_dabt_get_as(vcpu); - data = kvm_mmio_read_buf(run->mmio.data, len); + data = kvm_mmio_read_buf(vcpu->run->mmio.data, len); if (kvm_vcpu_dabt_issext(vcpu) && len < sizeof(unsigned long)) { @@ -104,7 +103,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) if (!kvm_vcpu_dabt_issf(vcpu)) data = data & 0xffffffff; - trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr, + trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, + vcpu->run->mmio.phys_addr, &data); data = vcpu_data_host_to_guest(vcpu, data, len); vcpu_set_reg(vcpu, kvm_vcpu_dabt_get_rd(vcpu), data); Overall, there is a large set of cleanups to be done when both the vcpu and the run structures are passed as parameters at the same time. Just grepping the tree for kvm_run is pretty instructive. M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] KVM: Optimize kvm_arch_vcpu_ioctl_run function 2020-04-16 8:28 ` Marc Zyngier @ 2020-04-16 8:45 ` Tianjia Zhang 2020-04-16 8:50 ` Cornelia Huck 2020-04-16 8:58 ` Marc Zyngier 0 siblings, 2 replies; 9+ messages in thread From: Tianjia Zhang @ 2020-04-16 8:45 UTC (permalink / raw) To: Marc Zyngier, Vitaly Kuznetsov Cc: kvm, linux-arm-kernel, kvmarm, linux-mips, kvm-ppc, linuxppc-dev, linux-s390, linux-kernel, pbonzini, tsbogend, paulus, mpe, benh, borntraeger, frankja, david, cohuck, heiko.carstens, gor, sean.j.christopherson, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa, james.morse, julien.thierry.kdev, suzuki.poulose, christoffer.dall, peterx, thuth On 2020/4/16 16:28, Marc Zyngier wrote: > On 2020-04-16 08:03, Vitaly Kuznetsov wrote: >> Tianjia Zhang <tianjia.zhang@linux.alibaba.com> writes: >> >>> In earlier versions of kvm, 'kvm_run' is an independent structure >>> and is not included in the vcpu structure. At present, 'kvm_run' >>> is already included in the vcpu structure, so the parameter >>> 'kvm_run' is redundant. >>> >>> This patch simplify the function definition, removes the extra >>> 'kvm_run' parameter, and extract it from the 'kvm_vcpu' structure >>> if necessary. >>> >>> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> >>> --- >>> >>> v2 change: >>> remove 'kvm_run' parameter and extract it from 'kvm_vcpu' >>> >>> arch/mips/kvm/mips.c | 3 ++- >>> arch/powerpc/kvm/powerpc.c | 3 ++- >>> arch/s390/kvm/kvm-s390.c | 3 ++- >>> arch/x86/kvm/x86.c | 11 ++++++----- >>> include/linux/kvm_host.h | 2 +- >>> virt/kvm/arm/arm.c | 6 +++--- >>> virt/kvm/kvm_main.c | 2 +- >>> 7 files changed, 17 insertions(+), 13 deletions(-) >>> >>> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c >>> index 8f05dd0a0f4e..ec24adf4857e 100644 >>> --- a/arch/mips/kvm/mips.c >>> +++ b/arch/mips/kvm/mips.c >>> @@ -439,8 +439,9 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct >>> kvm_vcpu *vcpu, >>> return -ENOIOCTLCMD; >>> } >>> >>> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >>> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) >>> { >>> + struct kvm_run *run = vcpu->run; >>> int r = -EINTR; >>> >>> vcpu_load(vcpu); >>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c >>> index e15166b0a16d..7e24691e138a 100644 >>> --- a/arch/powerpc/kvm/powerpc.c >>> +++ b/arch/powerpc/kvm/powerpc.c >>> @@ -1764,8 +1764,9 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu >>> *vcpu, struct kvm_one_reg *reg) >>> return r; >>> } >>> >>> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >>> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) >>> { >>> + struct kvm_run *run = vcpu->run; >>> int r; >>> >>> vcpu_load(vcpu); >>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>> index 19a81024fe16..443af3ead739 100644 >>> --- a/arch/s390/kvm/kvm-s390.c >>> +++ b/arch/s390/kvm/kvm-s390.c >>> @@ -4333,8 +4333,9 @@ static void store_regs(struct kvm_vcpu *vcpu, >>> struct kvm_run *kvm_run) >>> store_regs_fmt2(vcpu, kvm_run); >>> } >>> >>> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run >>> *kvm_run) >>> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) >>> { >>> + struct kvm_run *kvm_run = vcpu->run; >>> int rc; >>> >>> if (kvm_run->immediate_exit) >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>> index 3bf2ecafd027..a0338e86c90f 100644 >>> --- a/arch/x86/kvm/x86.c >>> +++ b/arch/x86/kvm/x86.c >>> @@ -8707,8 +8707,9 @@ static void kvm_put_guest_fpu(struct kvm_vcpu >>> *vcpu) >>> trace_kvm_fpu(0); >>> } >>> >>> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run >>> *kvm_run) >>> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) >>> { >>> + struct kvm_run *kvm_run = vcpu->run; >>> int r; >>> >>> vcpu_load(vcpu); >>> @@ -8726,18 +8727,18 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu >>> *vcpu, struct kvm_run *kvm_run) >>> r = -EAGAIN; >>> if (signal_pending(current)) { >>> r = -EINTR; >>> - vcpu->run->exit_reason = KVM_EXIT_INTR; >>> + kvm_run->exit_reason = KVM_EXIT_INTR; >>> ++vcpu->stat.signal_exits; >>> } >>> goto out; >>> } >>> >>> - if (vcpu->run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { >>> + if (kvm_run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { >>> r = -EINVAL; >>> goto out; >>> } >>> >>> - if (vcpu->run->kvm_dirty_regs) { >>> + if (kvm_run->kvm_dirty_regs) { >>> r = sync_regs(vcpu); >>> if (r != 0) >>> goto out; >>> @@ -8767,7 +8768,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu >>> *vcpu, struct kvm_run *kvm_run) >>> >>> out: >>> kvm_put_guest_fpu(vcpu); >>> - if (vcpu->run->kvm_valid_regs) >>> + if (kvm_run->kvm_valid_regs) >>> store_regs(vcpu); >>> post_kvm_run_save(vcpu); >>> kvm_sigset_deactivate(vcpu); >>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >>> index 6d58beb65454..1e17ef719595 100644 >>> --- a/include/linux/kvm_host.h >>> +++ b/include/linux/kvm_host.h >>> @@ -866,7 +866,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct >>> kvm_vcpu *vcpu, >>> struct kvm_mp_state *mp_state); >>> int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, >>> struct kvm_guest_debug *dbg); >>> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run >>> *kvm_run); >>> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu); >>> >>> int kvm_arch_init(void *opaque); >>> void kvm_arch_exit(void); >>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c >>> index 48d0ec44ad77..f5390ac2165b 100644 >>> --- a/virt/kvm/arm/arm.c >>> +++ b/virt/kvm/arm/arm.c >>> @@ -639,7 +639,6 @@ static void check_vcpu_requests(struct kvm_vcpu >>> *vcpu) >>> /** >>> * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute >>> guest code >>> * @vcpu: The VCPU pointer >>> - * @run: The kvm_run structure pointer used for userspace state >>> exchange >>> * >>> * This function is called through the VCPU_RUN ioctl called from >>> user space. It >>> * will execute VM code in a loop until the time slice for the >>> process is used >>> @@ -647,8 +646,9 @@ static void check_vcpu_requests(struct kvm_vcpu >>> *vcpu) >>> * return with return value 0 and with the kvm_run structure filled >>> in with the >>> * required data for the requested emulation. >>> */ >>> -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >>> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) >>> { >>> + struct kvm_run *run = vcpu->run; >>> int ret; >>> >>> if (unlikely(!kvm_vcpu_initialized(vcpu))) >>> @@ -659,7 +659,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu >>> *vcpu, struct kvm_run *run) >>> return ret; >>> >>> if (run->exit_reason == KVM_EXIT_MMIO) { >>> - ret = kvm_handle_mmio_return(vcpu, vcpu->run); >>> + ret = kvm_handle_mmio_return(vcpu, run); >> >> I don't know much about ARM but this also seems redundant, >> kvm_handle_mmio_return() is also able to extruct 'struct kvm_run' from' >> 'struct kvm_vcpu'. This likely deserves it's own patch though. > > Something like this (untested): > > diff --git a/arch/arm64/include/asm/kvm_host.h > b/arch/arm64/include/asm/kvm_host.h > index 32c8a675e5a4..82978995bdd6 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -490,7 +490,7 @@ void handle_exit_early(struct kvm_vcpu *vcpu, struct > kvm_run *run, > void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long data); > unsigned long kvm_mmio_read_buf(const void *buf, unsigned int len); > > -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run); > +int kvm_handle_mmio_return(struct kvm_vcpu *vcpu); > int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run, > phys_addr_t fault_ipa); > > diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c > index 4e0366759726..3b2c822b4859 100644 > --- a/virt/kvm/arm/mmio.c > +++ b/virt/kvm/arm/mmio.c > @@ -77,9 +77,8 @@ unsigned long kvm_mmio_read_buf(const void *buf, > unsigned int len) > * or in-kernel IO emulation > * > * @vcpu: The VCPU pointer > - * @run: The VCPU run struct containing the mmio data > */ > -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run) > +int kvm_handle_mmio_return(struct kvm_vcpu *vcpu) > { > unsigned long data; > unsigned int len; > @@ -93,7 +92,7 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, > struct kvm_run *run) > > if (!kvm_vcpu_dabt_iswrite(vcpu)) { > len = kvm_vcpu_dabt_get_as(vcpu); > - data = kvm_mmio_read_buf(run->mmio.data, len); > + data = kvm_mmio_read_buf(vcpu->run->mmio.data, len); > > if (kvm_vcpu_dabt_issext(vcpu) && > len < sizeof(unsigned long)) { > @@ -104,7 +103,8 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, > struct kvm_run *run) > if (!kvm_vcpu_dabt_issf(vcpu)) > data = data & 0xffffffff; > > - trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr, > + trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, > + vcpu->run->mmio.phys_addr, > &data); > data = vcpu_data_host_to_guest(vcpu, data, len); > vcpu_set_reg(vcpu, kvm_vcpu_dabt_get_rd(vcpu), data); > > Overall, there is a large set of cleanups to be done when both the vcpu > and the run > structures are passed as parameters at the same time. Just grepping the > tree for > kvm_run is pretty instructive. > > M. Sorry, it's my mistake, I only compiled the x86 platform, I will submit patch again. Thanks, Tianjia ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] KVM: Optimize kvm_arch_vcpu_ioctl_run function 2020-04-16 8:45 ` Tianjia Zhang @ 2020-04-16 8:50 ` Cornelia Huck 2020-04-16 9:08 ` Tianjia Zhang 2020-04-16 8:58 ` Marc Zyngier 1 sibling, 1 reply; 9+ messages in thread From: Cornelia Huck @ 2020-04-16 8:50 UTC (permalink / raw) To: Tianjia Zhang Cc: Marc Zyngier, Vitaly Kuznetsov, kvm, linux-arm-kernel, kvmarm, linux-mips, kvm-ppc, linuxppc-dev, linux-s390, linux-kernel, pbonzini, tsbogend, paulus, mpe, benh, borntraeger, frankja, david, heiko.carstens, gor, sean.j.christopherson, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa, james.morse, julien.thierry.kdev, suzuki.poulose, christoffer.dall, peterx, thuth On Thu, 16 Apr 2020 16:45:33 +0800 Tianjia Zhang <tianjia.zhang@linux.alibaba.com> wrote: > On 2020/4/16 16:28, Marc Zyngier wrote: > > On 2020-04-16 08:03, Vitaly Kuznetsov wrote: > >> Tianjia Zhang <tianjia.zhang@linux.alibaba.com> writes: > >> > >>> In earlier versions of kvm, 'kvm_run' is an independent structure > >>> and is not included in the vcpu structure. At present, 'kvm_run' > >>> is already included in the vcpu structure, so the parameter > >>> 'kvm_run' is redundant. > >>> > >>> This patch simplify the function definition, removes the extra > >>> 'kvm_run' parameter, and extract it from the 'kvm_vcpu' structure > >>> if necessary. > >>> > >>> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> > >>> --- > >>> > >>> v2 change: > >>> remove 'kvm_run' parameter and extract it from 'kvm_vcpu' > >>> > >>> arch/mips/kvm/mips.c | 3 ++- > >>> arch/powerpc/kvm/powerpc.c | 3 ++- > >>> arch/s390/kvm/kvm-s390.c | 3 ++- > >>> arch/x86/kvm/x86.c | 11 ++++++----- > >>> include/linux/kvm_host.h | 2 +- > >>> virt/kvm/arm/arm.c | 6 +++--- > >>> virt/kvm/kvm_main.c | 2 +- > >>> 7 files changed, 17 insertions(+), 13 deletions(-) > > Overall, there is a large set of cleanups to be done when both the vcpu > > and the run > > structures are passed as parameters at the same time. Just grepping the > > tree for > > kvm_run is pretty instructive. > > > > M. > > Sorry, it's my mistake, I only compiled the x86 platform, I will submit > patch again. I think it's completely fine (and even preferable) to do cleanups like that on top. [FWIW, I compiled s390 here.] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] KVM: Optimize kvm_arch_vcpu_ioctl_run function 2020-04-16 8:50 ` Cornelia Huck @ 2020-04-16 9:08 ` Tianjia Zhang 0 siblings, 0 replies; 9+ messages in thread From: Tianjia Zhang @ 2020-04-16 9:08 UTC (permalink / raw) To: Cornelia Huck Cc: Marc Zyngier, Vitaly Kuznetsov, kvm, linux-arm-kernel, kvmarm, linux-mips, kvm-ppc, linuxppc-dev, linux-s390, linux-kernel, pbonzini, tsbogend, paulus, mpe, benh, borntraeger, frankja, david, heiko.carstens, gor, sean.j.christopherson, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa, james.morse, julien.thierry.kdev, suzuki.poulose, christoffer.dall, peterx, thuth On 2020/4/16 16:50, Cornelia Huck wrote: > On Thu, 16 Apr 2020 16:45:33 +0800 > Tianjia Zhang <tianjia.zhang@linux.alibaba.com> wrote: > >> On 2020/4/16 16:28, Marc Zyngier wrote: >>> On 2020-04-16 08:03, Vitaly Kuznetsov wrote: >>>> Tianjia Zhang <tianjia.zhang@linux.alibaba.com> writes: >>>> >>>>> In earlier versions of kvm, 'kvm_run' is an independent structure >>>>> and is not included in the vcpu structure. At present, 'kvm_run' >>>>> is already included in the vcpu structure, so the parameter >>>>> 'kvm_run' is redundant. >>>>> >>>>> This patch simplify the function definition, removes the extra >>>>> 'kvm_run' parameter, and extract it from the 'kvm_vcpu' structure >>>>> if necessary. >>>>> >>>>> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> >>>>> --- >>>>> >>>>> v2 change: >>>>> remove 'kvm_run' parameter and extract it from 'kvm_vcpu' >>>>> >>>>> arch/mips/kvm/mips.c | 3 ++- >>>>> arch/powerpc/kvm/powerpc.c | 3 ++- >>>>> arch/s390/kvm/kvm-s390.c | 3 ++- >>>>> arch/x86/kvm/x86.c | 11 ++++++----- >>>>> include/linux/kvm_host.h | 2 +- >>>>> virt/kvm/arm/arm.c | 6 +++--- >>>>> virt/kvm/kvm_main.c | 2 +- >>>>> 7 files changed, 17 insertions(+), 13 deletions(-) > >>> Overall, there is a large set of cleanups to be done when both the vcpu >>> and the run >>> structures are passed as parameters at the same time. Just grepping the >>> tree for >>> kvm_run is pretty instructive. >>> >>> M. >> >> Sorry, it's my mistake, I only compiled the x86 platform, I will submit >> patch again. > > I think it's completely fine (and even preferable) to do cleanups like > that on top. > > [FWIW, I compiled s390 here.] > Very good, I will do a comprehensive cleanup of this type of code. Thanks, Tianjia ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] KVM: Optimize kvm_arch_vcpu_ioctl_run function 2020-04-16 8:45 ` Tianjia Zhang 2020-04-16 8:50 ` Cornelia Huck @ 2020-04-16 8:58 ` Marc Zyngier 1 sibling, 0 replies; 9+ messages in thread From: Marc Zyngier @ 2020-04-16 8:58 UTC (permalink / raw) To: Tianjia Zhang Cc: Vitaly Kuznetsov, kvm, linux-arm-kernel, kvmarm, linux-mips, kvm-ppc, linuxppc-dev, linux-s390, linux-kernel, pbonzini, tsbogend, paulus, mpe, benh, borntraeger, frankja, david, cohuck, heiko.carstens, gor, sean.j.christopherson, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa, james.morse, julien.thierry.kdev, suzuki.poulose, christoffer.dall, peterx, thuth On 2020-04-16 09:45, Tianjia Zhang wrote: > On 2020/4/16 16:28, Marc Zyngier wrote: [...] >> Overall, there is a large set of cleanups to be done when both the >> vcpu and the run >> structures are passed as parameters at the same time. Just grepping >> the tree for >> kvm_run is pretty instructive. >> >> M. > > Sorry, it's my mistake, I only compiled the x86 platform, I will > submit patch again. Not a mistake. All I'm saying is that there is an opportunity for a larger series that cleans up the code base, rather than just doing a couple of localized changes. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] KVM: Optimize kvm_arch_vcpu_ioctl_run function 2020-04-16 5:10 [PATCH v2] KVM: Optimize kvm_arch_vcpu_ioctl_run function Tianjia Zhang 2020-04-16 7:03 ` Vitaly Kuznetsov @ 2020-04-16 8:10 ` Cornelia Huck 2020-04-16 14:16 ` Paolo Bonzini 2 siblings, 0 replies; 9+ messages in thread From: Cornelia Huck @ 2020-04-16 8:10 UTC (permalink / raw) To: Tianjia Zhang Cc: pbonzini, tsbogend, paulus, mpe, benh, borntraeger, frankja, david, heiko.carstens, gor, sean.j.christopherson, vkuznets, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa, maz, james.morse, julien.thierry.kdev, suzuki.poulose, christoffer.dall, peterx, thuth, kvm, linux-arm-kernel, kvmarm, linux-mips, kvm-ppc, linuxppc-dev, linux-s390, linux-kernel On Thu, 16 Apr 2020 13:10:57 +0800 Tianjia Zhang <tianjia.zhang@linux.alibaba.com> wrote: > In earlier versions of kvm, 'kvm_run' is an independent structure > and is not included in the vcpu structure. At present, 'kvm_run' > is already included in the vcpu structure, so the parameter > 'kvm_run' is redundant. > > This patch simplify the function definition, removes the extra s/simplify/simplifies/ > 'kvm_run' parameter, and extract it from the 'kvm_vcpu' structure s/extract/extracts/ > if necessary. > > Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> > --- > > v2 change: > remove 'kvm_run' parameter and extract it from 'kvm_vcpu' > > arch/mips/kvm/mips.c | 3 ++- > arch/powerpc/kvm/powerpc.c | 3 ++- > arch/s390/kvm/kvm-s390.c | 3 ++- > arch/x86/kvm/x86.c | 11 ++++++----- > include/linux/kvm_host.h | 2 +- > virt/kvm/arm/arm.c | 6 +++--- > virt/kvm/kvm_main.c | 2 +- > 7 files changed, 17 insertions(+), 13 deletions(-) > Reviewed-by: Cornelia Huck <cohuck@redhat.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] KVM: Optimize kvm_arch_vcpu_ioctl_run function 2020-04-16 5:10 [PATCH v2] KVM: Optimize kvm_arch_vcpu_ioctl_run function Tianjia Zhang 2020-04-16 7:03 ` Vitaly Kuznetsov 2020-04-16 8:10 ` Cornelia Huck @ 2020-04-16 14:16 ` Paolo Bonzini 2 siblings, 0 replies; 9+ messages in thread From: Paolo Bonzini @ 2020-04-16 14:16 UTC (permalink / raw) To: Tianjia Zhang, tsbogend, paulus, mpe, benh, borntraeger, frankja, david, cohuck, heiko.carstens, gor, sean.j.christopherson, vkuznets, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa, maz, james.morse, julien.thierry.kdev, suzuki.poulose, christoffer.dall, peterx, thuth Cc: kvm, linux-arm-kernel, kvmarm, linux-mips, kvm-ppc, linuxppc-dev, linux-s390, linux-kernel On 16/04/20 07:10, Tianjia Zhang wrote: > In earlier versions of kvm, 'kvm_run' is an independent structure > and is not included in the vcpu structure. At present, 'kvm_run' > is already included in the vcpu structure, so the parameter > 'kvm_run' is redundant. > > This patch simplify the function definition, removes the extra > 'kvm_run' parameter, and extract it from the 'kvm_vcpu' structure > if necessary. > > Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> > --- > > v2 change: > remove 'kvm_run' parameter and extract it from 'kvm_vcpu' > > arch/mips/kvm/mips.c | 3 ++- > arch/powerpc/kvm/powerpc.c | 3 ++- > arch/s390/kvm/kvm-s390.c | 3 ++- > arch/x86/kvm/x86.c | 11 ++++++----- > include/linux/kvm_host.h | 2 +- > virt/kvm/arm/arm.c | 6 +++--- > virt/kvm/kvm_main.c | 2 +- > 7 files changed, 17 insertions(+), 13 deletions(-) > > diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c > index 8f05dd0a0f4e..ec24adf4857e 100644 > --- a/arch/mips/kvm/mips.c > +++ b/arch/mips/kvm/mips.c > @@ -439,8 +439,9 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, > return -ENOIOCTLCMD; > } > > -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > { > + struct kvm_run *run = vcpu->run; > int r = -EINTR; > > vcpu_load(vcpu); > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c > index e15166b0a16d..7e24691e138a 100644 > --- a/arch/powerpc/kvm/powerpc.c > +++ b/arch/powerpc/kvm/powerpc.c > @@ -1764,8 +1764,9 @@ int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg) > return r; > } > > -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > { > + struct kvm_run *run = vcpu->run; > int r; > > vcpu_load(vcpu); > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 19a81024fe16..443af3ead739 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -4333,8 +4333,9 @@ static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > store_regs_fmt2(vcpu, kvm_run); > } > > -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > { > + struct kvm_run *kvm_run = vcpu->run; > int rc; > > if (kvm_run->immediate_exit) > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 3bf2ecafd027..a0338e86c90f 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -8707,8 +8707,9 @@ static void kvm_put_guest_fpu(struct kvm_vcpu *vcpu) > trace_kvm_fpu(0); > } > > -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > { > + struct kvm_run *kvm_run = vcpu->run; > int r; > > vcpu_load(vcpu); > @@ -8726,18 +8727,18 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > r = -EAGAIN; > if (signal_pending(current)) { > r = -EINTR; > - vcpu->run->exit_reason = KVM_EXIT_INTR; > + kvm_run->exit_reason = KVM_EXIT_INTR; > ++vcpu->stat.signal_exits; > } > goto out; > } > > - if (vcpu->run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { > + if (kvm_run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { > r = -EINVAL; > goto out; > } > > - if (vcpu->run->kvm_dirty_regs) { > + if (kvm_run->kvm_dirty_regs) { > r = sync_regs(vcpu); > if (r != 0) > goto out; > @@ -8767,7 +8768,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > > out: > kvm_put_guest_fpu(vcpu); > - if (vcpu->run->kvm_valid_regs) > + if (kvm_run->kvm_valid_regs) > store_regs(vcpu); > post_kvm_run_save(vcpu); > kvm_sigset_deactivate(vcpu); > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 6d58beb65454..1e17ef719595 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -866,7 +866,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu, > struct kvm_mp_state *mp_state); > int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu, > struct kvm_guest_debug *dbg); > -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run); > +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu); > > int kvm_arch_init(void *opaque); > void kvm_arch_exit(void); > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > index 48d0ec44ad77..f5390ac2165b 100644 > --- a/virt/kvm/arm/arm.c > +++ b/virt/kvm/arm/arm.c > @@ -639,7 +639,6 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu) > /** > * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute guest code > * @vcpu: The VCPU pointer > - * @run: The kvm_run structure pointer used for userspace state exchange > * > * This function is called through the VCPU_RUN ioctl called from user space. It > * will execute VM code in a loop until the time slice for the process is used > @@ -647,8 +646,9 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu) > * return with return value 0 and with the kvm_run structure filled in with the > * required data for the requested emulation. > */ > -int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) > { > + struct kvm_run *run = vcpu->run; > int ret; > > if (unlikely(!kvm_vcpu_initialized(vcpu))) > @@ -659,7 +659,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > return ret; > > if (run->exit_reason == KVM_EXIT_MMIO) { > - ret = kvm_handle_mmio_return(vcpu, vcpu->run); > + ret = kvm_handle_mmio_return(vcpu, run); > if (ret) > return ret; > } > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 74bdb7bf3295..e18faea89146 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -3135,7 +3135,7 @@ static long kvm_vcpu_ioctl(struct file *filp, > synchronize_rcu(); > put_pid(oldpid); > } > - r = kvm_arch_vcpu_ioctl_run(vcpu, vcpu->run); > + r = kvm_arch_vcpu_ioctl_run(vcpu); > trace_kvm_userspace_exit(vcpu->run->exit_reason, r); > break; > } > Queued, thanks. Paolo ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-04-16 14:36 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-16 5:10 [PATCH v2] KVM: Optimize kvm_arch_vcpu_ioctl_run function Tianjia Zhang 2020-04-16 7:03 ` Vitaly Kuznetsov 2020-04-16 8:28 ` Marc Zyngier 2020-04-16 8:45 ` Tianjia Zhang 2020-04-16 8:50 ` Cornelia Huck 2020-04-16 9:08 ` Tianjia Zhang 2020-04-16 8:58 ` Marc Zyngier 2020-04-16 8:10 ` Cornelia Huck 2020-04-16 14:16 ` Paolo Bonzini
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).