linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] KVM: x86: remove duplicated KVM_REQ_EVENT request
@ 2020-02-07  2:48 linmiaohe
  2020-02-07  9:05 ` Vitaly Kuznetsov
  0 siblings, 1 reply; 6+ messages in thread
From: linmiaohe @ 2020-02-07  2:48 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: kvm, linux-kernel, x86, pbonzini, rkrcmar, sean.j.christopherson,
	wanpengli, jmattson, joro, tglx, mingo, bp, hpa

Hi:
Vitaly Kuznetsov <vkuznets@redhat.com> writes:
> linmiaohe <linmiaohe@huawei.com> writes:
>> From: Miaohe Lin <linmiaohe@huawei.com>
>>
>> The KVM_REQ_EVENT request is already made in kvm_set_rflags(). We 
>> should not make it again.
>>  	kvm_rip_write(vcpu, ctxt->eip);
>>  	kvm_set_rflags(vcpu, ctxt->eflags);
>> -	kvm_make_request(KVM_REQ_EVENT, vcpu);
>
> I would've actually done it the other way around and removed
> kvm_make_request() from kvm_set_rflags() as it is not an obvious behavior (e.g. why kvm_rip_write() doens't do that and kvm_set_rflags() does ?) adding kvm_make_request() to all call sites.
>
>In case this looks like too big of a change with no particular gain I'd suggest you at least leave a comment above kvm_set_rflags(), something like
>
>"kvm_make_request() also requests KVM_REQ_EVENT"

I think adding kvm_make_request() to all call sites is too big without particular gain. And also leave a comment above
kvm_set_rflags() maybe isn't needed as rflags updates is an site that can trigger event injection. Please see commit
(3842d135ff24 KVM: Check for pending events before attempting injection) for detail.

What do you think? Thanks for your review anyway. :)

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

* Re: [PATCH] KVM: x86: remove duplicated KVM_REQ_EVENT request
  2020-02-07  2:48 [PATCH] KVM: x86: remove duplicated KVM_REQ_EVENT request linmiaohe
@ 2020-02-07  9:05 ` Vitaly Kuznetsov
  2020-02-12 11:49   ` Paolo Bonzini
  0 siblings, 1 reply; 6+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-07  9:05 UTC (permalink / raw)
  To: linmiaohe
  Cc: kvm, linux-kernel, x86, pbonzini, rkrcmar, sean.j.christopherson,
	wanpengli, jmattson, joro, tglx, mingo, bp, hpa

linmiaohe <linmiaohe@huawei.com> writes:

> Hi:
> Vitaly Kuznetsov <vkuznets@redhat.com> writes:
>> linmiaohe <linmiaohe@huawei.com> writes:
>>> From: Miaohe Lin <linmiaohe@huawei.com>
>>>
>>> The KVM_REQ_EVENT request is already made in kvm_set_rflags(). We 
>>> should not make it again.
>>>  	kvm_rip_write(vcpu, ctxt->eip);
>>>  	kvm_set_rflags(vcpu, ctxt->eflags);
>>> -	kvm_make_request(KVM_REQ_EVENT, vcpu);
>>
>> I would've actually done it the other way around and removed
>> kvm_make_request() from kvm_set_rflags() as it is not an obvious behavior (e.g. why kvm_rip_write() doens't do that and kvm_set_rflags() does ?) adding kvm_make_request() to all call sites.
>>
>>In case this looks like too big of a change with no particular gain I'd suggest you at least leave a comment above kvm_set_rflags(), something like
>>
>>"kvm_make_request() also requests KVM_REQ_EVENT"
>
> I think adding kvm_make_request() to all call sites is too big without particular gain. And also leave a comment above
> kvm_set_rflags() maybe isn't needed as rflags updates is an site that can trigger event injection. Please see commit
> (3842d135ff24 KVM: Check for pending events before attempting injection) for detail.
>
> What do you think?

I don't have a strong opinion on this and your change is correct so feel
free to throw my

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>


-- 
Vitaly


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

* Re: [PATCH] KVM: x86: remove duplicated KVM_REQ_EVENT request
  2020-02-07  9:05 ` Vitaly Kuznetsov
@ 2020-02-12 11:49   ` Paolo Bonzini
  0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2020-02-12 11:49 UTC (permalink / raw)
  To: Vitaly Kuznetsov, linmiaohe
  Cc: kvm, linux-kernel, x86, rkrcmar, sean.j.christopherson,
	wanpengli, jmattson, joro, tglx, mingo, bp, hpa

On 07/02/20 10:05, Vitaly Kuznetsov wrote:
> kvm_make_request() from kvm_set_rflags() as it is not an obvious
> behavior (e.g. why kvm_rip_write() doens't do that and
> kvm_set_rflags() does ?)

Because writing RFLAGS can change IF and therefore cause an interrupt to
be injected.

Paolo


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

* Re: [PATCH] KVM: x86: remove duplicated KVM_REQ_EVENT request
@ 2020-02-12 12:40 linmiaohe
  0 siblings, 0 replies; 6+ messages in thread
From: linmiaohe @ 2020-02-12 12:40 UTC (permalink / raw)
  To: Paolo Bonzini, Vitaly Kuznetsov
  Cc: kvm, linux-kernel, x86, rkrcmar, sean.j.christopherson,
	wanpengli, jmattson, joro, tglx, mingo, bp, hpa

Paolo Bonzini <pbonzini@redhat.com> write:
> On 07/02/20 10:05, Vitaly Kuznetsov wrote:
>> kvm_make_request() from kvm_set_rflags() as it is not an obvious 
>> behavior (e.g. why kvm_rip_write() doens't do that and
>> kvm_set_rflags() does ?)
>
>Because writing RFLAGS can change IF and therefore cause an interrupt to be injected.
>

Many thanks for your explanation. :) I thought it was because of Trap Flag.


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

* Re: [PATCH] KVM: x86: remove duplicated KVM_REQ_EVENT request
  2020-02-06  1:45 linmiaohe
@ 2020-02-06 10:41 ` Vitaly Kuznetsov
  0 siblings, 0 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-06 10:41 UTC (permalink / raw)
  To: linmiaohe
  Cc: kvm, linux-kernel, x86, pbonzini, rkrcmar, sean.j.christopherson,
	wanpengli, jmattson, joro, tglx, mingo, bp, hpa

linmiaohe <linmiaohe@huawei.com> writes:

> From: Miaohe Lin <linmiaohe@huawei.com>
>
> The KVM_REQ_EVENT request is already made in kvm_set_rflags(). We should
> not make it again.
>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  arch/x86/kvm/x86.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fbabb2f06273..212282c6fb76 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8942,7 +8942,6 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
>  
>  	kvm_rip_write(vcpu, ctxt->eip);
>  	kvm_set_rflags(vcpu, ctxt->eflags);
> -	kvm_make_request(KVM_REQ_EVENT, vcpu);

I would've actually done it the other way around and removed
kvm_make_request() from kvm_set_rflags() as it is not an obvious
behavior (e.g. why kvm_rip_write() doens't do that and kvm_set_rflags()
does ?) adding kvm_make_request() to all call sites.

In case this looks like too big of a change with no particular gain I'd
suggest you at least leave a comment above kvm_set_rflags(), something
like

"kvm_make_request() also requests KVM_REQ_EVENT"

>  	return 1;
>  }
>  EXPORT_SYMBOL_GPL(kvm_task_switch);

-- 
Vitaly


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

* [PATCH] KVM: x86: remove duplicated KVM_REQ_EVENT request
@ 2020-02-06  1:45 linmiaohe
  2020-02-06 10:41 ` Vitaly Kuznetsov
  0 siblings, 1 reply; 6+ messages in thread
From: linmiaohe @ 2020-02-06  1:45 UTC (permalink / raw)
  To: pbonzini, rkrcmar, sean.j.christopherson, vkuznets, wanpengli,
	jmattson, joro, tglx, mingo, bp, hpa
  Cc: linmiaohe, kvm, linux-kernel, x86

From: Miaohe Lin <linmiaohe@huawei.com>

The KVM_REQ_EVENT request is already made in kvm_set_rflags(). We should
not make it again.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 arch/x86/kvm/x86.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fbabb2f06273..212282c6fb76 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8942,7 +8942,6 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
 
 	kvm_rip_write(vcpu, ctxt->eip);
 	kvm_set_rflags(vcpu, ctxt->eflags);
-	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	return 1;
 }
 EXPORT_SYMBOL_GPL(kvm_task_switch);
-- 
2.19.1


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

end of thread, other threads:[~2020-02-12 12:40 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-07  2:48 [PATCH] KVM: x86: remove duplicated KVM_REQ_EVENT request linmiaohe
2020-02-07  9:05 ` Vitaly Kuznetsov
2020-02-12 11:49   ` Paolo Bonzini
  -- strict thread matches above, loose matches on Subject: below --
2020-02-12 12:40 linmiaohe
2020-02-06  1:45 linmiaohe
2020-02-06 10:41 ` Vitaly Kuznetsov

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