2020-04-13 05:49:39

by Tianjia Zhang

Subject: [PATCH] KVM: Optimize kvm_arch_vcpu_ioctl_run function

kvm_arch_vcpu_ioctl_run() is only called in the file kvm_main.c,
where vcpu->run is the kvm_run parameter, so it has been replaced.

Signed-off-by: Tianjia Zhang <[email protected]>
---
arch/x86/kvm/x86.c | 8 ++++----
virt/kvm/arm/arm.c | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3bf2ecafd027..70e3f4abbd4d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8726,18 +8726,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 +8767,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/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 48d0ec44ad77..ab9d7966a4c8 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -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;
}
--
2.17.1

2020-04-13 13:00:53

by Marc Zyngier

Subject: Re: [PATCH] KVM: Optimize kvm_arch_vcpu_ioctl_run function

Tianjia,

On 2020-04-13 04:45, Tianjia Zhang wrote:
> kvm_arch_vcpu_ioctl_run() is only called in the file kvm_main.c,
> where vcpu->run is the kvm_run parameter, so it has been replaced.
>
> Signed-off-by: Tianjia Zhang <[email protected]>
> ---
> arch/x86/kvm/x86.c | 8 ++++----
> virt/kvm/arm/arm.c | 2 +-
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3bf2ecafd027..70e3f4abbd4d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8726,18 +8726,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 +8767,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/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 48d0ec44ad77..ab9d7966a4c8 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -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;
> }

Do you have any number supporting the idea that you are optimizing
anything
here? Performance, code size, register pressure or any other relevant
metric?

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2020-04-13 22:07:45

by Tianjia Zhang

Subject: Re: [PATCH] KVM: Optimize kvm_arch_vcpu_ioctl_run function

On 2020/4/13 16:56, Marc Zyngier wrote:
> Tianjia,
>
> On 2020-04-13 04:45, Tianjia Zhang wrote:
>> kvm_arch_vcpu_ioctl_run() is only called in the file kvm_main.c,
>> where vcpu->run is the kvm_run parameter, so it has been replaced.
>>
>> Signed-off-by: Tianjia Zhang <[email protected]>
>> ---
>>  arch/x86/kvm/x86.c | 8 ++++----
>>  virt/kvm/arm/arm.c | 2 +-
>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 3bf2ecafd027..70e3f4abbd4d 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -8726,18 +8726,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 +8767,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/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index 48d0ec44ad77..ab9d7966a4c8 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -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;
>>      }
>
> Do you have any number supporting the idea that you are optimizing anything
> here? Performance, code size, register pressure or any other relevant
> metric?
>
> Thanks,
>
>         M.

This is only a simplified implementation of the function, the impact on
performance and register pressure can be ignored.

Thanks,
Tianjia

2020-04-14 16:13:48

by Vitaly Kuznetsov

Subject: Re: [PATCH] KVM: Optimize kvm_arch_vcpu_ioctl_run function

Tianjia Zhang <[email protected]> writes:

> kvm_arch_vcpu_ioctl_run() is only called in the file kvm_main.c,
> where vcpu->run is the kvm_run parameter, so it has been replaced.
>
> Signed-off-by: Tianjia Zhang <[email protected]>
> ---
> arch/x86/kvm/x86.c | 8 ++++----
> virt/kvm/arm/arm.c | 2 +-
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3bf2ecafd027..70e3f4abbd4d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8726,18 +8726,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;

I have a more generic question: why do we need to pass 'kvm_run' to
kvm_arch_vcpu_ioctl_run() if it can be extracted from 'struct kvm_vcpu'?
The only call site looks like

virt/kvm/kvm_main.c: r = kvm_arch_vcpu_ioctl_run(vcpu, vcpu->run);

> ++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 +8767,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/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 48d0ec44ad77..ab9d7966a4c8 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -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;
> }

--
Vitaly

2020-04-15 22:04:46

by Tianjia Zhang

Subject: Re: [PATCH] KVM: Optimize kvm_arch_vcpu_ioctl_run function

On 2020/4/14 22:26, Vitaly Kuznetsov wrote:
> Tianjia Zhang <[email protected]> writes:
>
>> kvm_arch_vcpu_ioctl_run() is only called in the file kvm_main.c,
>> where vcpu->run is the kvm_run parameter, so it has been replaced.
>>
>> Signed-off-by: Tianjia Zhang <[email protected]>
>> ---
>> arch/x86/kvm/x86.c | 8 ++++----
>> virt/kvm/arm/arm.c | 2 +-
>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 3bf2ecafd027..70e3f4abbd4d 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -8726,18 +8726,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;
>
> I have a more generic question: why do we need to pass 'kvm_run' to
> kvm_arch_vcpu_ioctl_run() if it can be extracted from 'struct kvm_vcpu'?
> The only call site looks like
>
> virt/kvm/kvm_main.c: r = kvm_arch_vcpu_ioctl_run(vcpu, vcpu->run);
>

In the earlier version, kvm_run is used to pass parameters with user
mode and is not included in the vcpu structure, so it has been retained
until now.

Thanks,
Tianjia

>> ++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 +8767,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/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index 48d0ec44ad77..ab9d7966a4c8 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -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;
>> }
>

2020-04-15 22:44:37

by Vitaly Kuznetsov

Subject: Re: [PATCH] KVM: Optimize kvm_arch_vcpu_ioctl_run function

Tianjia Zhang <[email protected]> writes:

> On 2020/4/14 22:26, Vitaly Kuznetsov wrote:
>> Tianjia Zhang <[email protected]> writes:
>>
>>> kvm_arch_vcpu_ioctl_run() is only called in the file kvm_main.c,
>>> where vcpu->run is the kvm_run parameter, so it has been replaced.
>>>
>>> Signed-off-by: Tianjia Zhang <[email protected]>
>>> ---
>>> arch/x86/kvm/x86.c | 8 ++++----
>>> virt/kvm/arm/arm.c | 2 +-
>>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 3bf2ecafd027..70e3f4abbd4d 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -8726,18 +8726,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;
>>
>> I have a more generic question: why do we need to pass 'kvm_run' to
>> kvm_arch_vcpu_ioctl_run() if it can be extracted from 'struct kvm_vcpu'?
>> The only call site looks like
>>
>> virt/kvm/kvm_main.c: r = kvm_arch_vcpu_ioctl_run(vcpu, vcpu->run);
>>
>
> In the earlier version, kvm_run is used to pass parameters with user
> mode and is not included in the vcpu structure, so it has been retained
> until now.
>

In case this is no longer needed I'd suggest we drop 'kvm_run' parameter
and extract it from 'struct kvm_vcpu' when needed. This looks like a
natural add-on to your cleanup patch.

--
Vitaly

2020-04-16 00:14:13

by Paolo Bonzini

Subject: Re: [PATCH] KVM: Optimize kvm_arch_vcpu_ioctl_run function

On 15/04/20 11:07, Vitaly Kuznetsov wrote:
> In case this is no longer needed I'd suggest we drop 'kvm_run' parameter
> and extract it from 'struct kvm_vcpu' when needed. This looks like a
> natural add-on to your cleanup patch.

I agree, though I think it should be _instead_ of Tianjia's patch rather
than on top.

Paolo

2020-04-16 02:02:24

by Tianjia Zhang

Subject: Re: [PATCH] KVM: Optimize kvm_arch_vcpu_ioctl_run function

On 2020/4/15 22:53, Paolo Bonzini wrote:
> On 15/04/20 11:07, Vitaly Kuznetsov wrote:
>> In case this is no longer needed I'd suggest we drop 'kvm_run' parameter
>> and extract it from 'struct kvm_vcpu' when needed. This looks like a
>> natural add-on to your cleanup patch.
>
> I agree, though I think it should be _instead_ of Tianjia's patch rather
> than on top.
>
> Paolo
>

Thank you very much for the comments of Vitaly and Paolo, I will make a
v2 patch.

Thanks and best,
Tianjia