* [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 related [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86: remove duplicated KVM_REQ_EVENT request
2020-02-06 1:45 [PATCH] KVM: x86: remove duplicated KVM_REQ_EVENT request 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
* 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-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-07 2:48 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 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
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-06 1:45 [PATCH] KVM: x86: remove duplicated KVM_REQ_EVENT request linmiaohe
2020-02-06 10:41 ` Vitaly Kuznetsov
2020-02-07 2:48 linmiaohe
2020-02-07 9:05 ` Vitaly Kuznetsov
2020-02-12 11:49 ` Paolo Bonzini
2020-02-12 12:40 linmiaohe
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).