* [PATCH] KVM: x86: fix kvm_vcpu_is_preempted
@ 2022-01-12 11:19 Li RongQing
2022-01-12 16:43 ` Sean Christopherson
2022-01-13 11:03 ` Paolo Bonzini
0 siblings, 2 replies; 6+ messages in thread
From: Li RongQing @ 2022-01-12 11:19 UTC (permalink / raw)
To: pbonzini, seanjc, vkuznets, wanpengli, jmattson, tglx, mingo, bp,
x86, hpa, rkrcmar, kvm, joro
After support paravirtualized TLB shootdowns, steal_time.preempted
includes not only KVM_VCPU_PREEMPTED, but also KVM_VCPU_FLUSH_TLB
and kvm_vcpu_is_preempted should test only with KVM_VCPU_PREEMPTED
Fixes: 858a43aae2367 ("KVM: X86: use paravirtualized TLB Shootdown")
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
arch/x86/kernel/kvm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 59abbda..a9202d9 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -1025,8 +1025,8 @@ asm(
".type __raw_callee_save___kvm_vcpu_is_preempted, @function;"
"__raw_callee_save___kvm_vcpu_is_preempted:"
"movq __per_cpu_offset(,%rdi,8), %rax;"
-"cmpb $0, " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rax);"
-"setne %al;"
+"movb " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rax), %al;"
+"andb $" __stringify(KVM_VCPU_PREEMPTED) ", %al;"
"ret;"
".size __raw_callee_save___kvm_vcpu_is_preempted, .-__raw_callee_save___kvm_vcpu_is_preempted;"
".popsection");
--
2.9.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: x86: fix kvm_vcpu_is_preempted 2022-01-12 11:19 [PATCH] KVM: x86: fix kvm_vcpu_is_preempted Li RongQing @ 2022-01-12 16:43 ` Sean Christopherson 2022-01-13 8:06 ` 答复: " Li,Rongqing 2022-01-13 10:00 ` Peter Zijlstra 2022-01-13 11:03 ` Paolo Bonzini 1 sibling, 2 replies; 6+ messages in thread From: Sean Christopherson @ 2022-01-12 16:43 UTC (permalink / raw) To: Li RongQing Cc: pbonzini, vkuznets, wanpengli, jmattson, tglx, mingo, bp, x86, hpa, rkrcmar, kvm, joro On Wed, Jan 12, 2022, Li RongQing wrote: > After support paravirtualized TLB shootdowns, steal_time.preempted > includes not only KVM_VCPU_PREEMPTED, but also KVM_VCPU_FLUSH_TLB > > and kvm_vcpu_is_preempted should test only with KVM_VCPU_PREEMPTED > > Fixes: 858a43aae2367 ("KVM: X86: use paravirtualized TLB Shootdown") > Signed-off-by: Li RongQing <lirongqing@baidu.com> > --- > arch/x86/kernel/kvm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c > index 59abbda..a9202d9 100644 > --- a/arch/x86/kernel/kvm.c > +++ b/arch/x86/kernel/kvm.c > @@ -1025,8 +1025,8 @@ asm( > ".type __raw_callee_save___kvm_vcpu_is_preempted, @function;" > "__raw_callee_save___kvm_vcpu_is_preempted:" > "movq __per_cpu_offset(,%rdi,8), %rax;" > -"cmpb $0, " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rax);" > -"setne %al;" > +"movb " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rax), %al;" > +"andb $" __stringify(KVM_VCPU_PREEMPTED) ", %al;" Eww, the existing code is sketchy. It relies on the compiler to store _Bool/bool in a single byte since %rax may be non-zero from the __per_cpu_offset(), and modifying %al doesn't zero %rax[63:8]. I doubt gcc or clang use anything but a single byte on x86-64, but "andl" is just as cheap so I don't see any harm in being paranoid. > "ret;" > ".size __raw_callee_save___kvm_vcpu_is_preempted, .-__raw_callee_save___kvm_vcpu_is_preempted;" > ".popsection"); > -- > 2.9.4 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* 答复: [PATCH] KVM: x86: fix kvm_vcpu_is_preempted 2022-01-12 16:43 ` Sean Christopherson @ 2022-01-13 8:06 ` Li,Rongqing 2022-01-13 10:00 ` Peter Zijlstra 1 sibling, 0 replies; 6+ messages in thread From: Li,Rongqing @ 2022-01-13 8:06 UTC (permalink / raw) To: Sean Christopherson Cc: pbonzini, vkuznets, wanpengli, jmattson, tglx, mingo, bp, x86, hpa, rkrcmar, kvm, joro > -----邮件原件----- > 发件人: Sean Christopherson <seanjc@google.com> > 发送时间: 2022年1月13日 0:44 > 收件人: Li,Rongqing <lirongqing@baidu.com> > 抄送: pbonzini@redhat.com; vkuznets@redhat.com; wanpengli@tencent.com; > jmattson@google.com; tglx@linutronix.de; mingo@redhat.com; bp@alien8.de; > x86@kernel.org; hpa@zytor.com; rkrcmar@redhat.com; kvm@vger.kernel.org; > joro@8bytes.org > 主题: Re: [PATCH] KVM: x86: fix kvm_vcpu_is_preempted > > On Wed, Jan 12, 2022, Li RongQing wrote: > > After support paravirtualized TLB shootdowns, steal_time.preempted > > includes not only KVM_VCPU_PREEMPTED, but also KVM_VCPU_FLUSH_TLB > > > > and kvm_vcpu_is_preempted should test only with KVM_VCPU_PREEMPTED > > > > Fixes: 858a43aae2367 ("KVM: X86: use paravirtualized TLB Shootdown") > > Signed-off-by: Li RongQing <lirongqing@baidu.com> > > --- > > arch/x86/kernel/kvm.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c index > > 59abbda..a9202d9 100644 > > --- a/arch/x86/kernel/kvm.c > > +++ b/arch/x86/kernel/kvm.c > > @@ -1025,8 +1025,8 @@ asm( > > ".type __raw_callee_save___kvm_vcpu_is_preempted, @function;" > > "__raw_callee_save___kvm_vcpu_is_preempted:" > > "movq __per_cpu_offset(,%rdi,8), %rax;" > > -"cmpb $0, " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rax);" > > -"setne %al;" > > +"movb " __stringify(KVM_STEAL_TIME_preempted) > "+steal_time(%rax), %al;" > > +"andb $" __stringify(KVM_VCPU_PREEMPTED) ", %al;" > > Eww, the existing code is sketchy. It relies on the compiler to store _Bool/bool > in a single byte since %rax may be non-zero from the __per_cpu_offset(), and > modifying %al doesn't zero %rax[63:8]. I doubt gcc or clang use anything but a > single byte on x86-64, but "andl" is just as cheap so I don't see any harm in being > paranoid. > Build with gcc (GCC) 7.3.0, disassembled as blow before: ffffffff8105bdc0 <__raw_callee_save___kvm_vcpu_is_preempted>: ffffffff8105bdc0: 48 8b 04 fd c0 16 1d mov -0x7de2e940(,%rdi,8),%rax ffffffff8105bdc7: 82 ffffffff8105bdc8: 80 b8 90 c0 02 00 00 cmpb $0x0,0x2c090(%rax) ffffffff8105bdcf: 0f 95 c0 setne %al ffffffff8105bdd2: c3 retq ffffffff8105bdd3: 0f 1f 00 nopl (%rax) ffffffff8105bdd6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1) ffffffff8105bddd: 00 00 00 after: ffffffff8105bdc0 <__raw_callee_save___kvm_vcpu_is_preempted>: ffffffff8105bdc0: 48 8b 04 fd c0 16 1d mov -0x7de2e940(,%rdi,8),%rax ffffffff8105bdc7: 82 ffffffff8105bdc8: 8a 80 90 c0 02 00 mov 0x2c090(%rax),%al ffffffff8105bdce: 24 01 and $0x1,%al ffffffff8105bdd0: c3 retq ffffffff8105bdd1: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1) ffffffff8105bdd6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1) ffffffff8105bddd: 00 00 00 Thanks -Li ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86: fix kvm_vcpu_is_preempted 2022-01-12 16:43 ` Sean Christopherson 2022-01-13 8:06 ` 答复: " Li,Rongqing @ 2022-01-13 10:00 ` Peter Zijlstra 1 sibling, 0 replies; 6+ messages in thread From: Peter Zijlstra @ 2022-01-13 10:00 UTC (permalink / raw) To: Sean Christopherson Cc: Li RongQing, pbonzini, vkuznets, wanpengli, jmattson, tglx, mingo, bp, x86, hpa, rkrcmar, kvm, joro On Wed, Jan 12, 2022 at 04:43:39PM +0000, Sean Christopherson wrote: > On Wed, Jan 12, 2022, Li RongQing wrote: > > After support paravirtualized TLB shootdowns, steal_time.preempted > > includes not only KVM_VCPU_PREEMPTED, but also KVM_VCPU_FLUSH_TLB > > > > and kvm_vcpu_is_preempted should test only with KVM_VCPU_PREEMPTED > > > > Fixes: 858a43aae2367 ("KVM: X86: use paravirtualized TLB Shootdown") > > Signed-off-by: Li RongQing <lirongqing@baidu.com> > > --- > > arch/x86/kernel/kvm.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c > > index 59abbda..a9202d9 100644 > > --- a/arch/x86/kernel/kvm.c > > +++ b/arch/x86/kernel/kvm.c > > @@ -1025,8 +1025,8 @@ asm( > > ".type __raw_callee_save___kvm_vcpu_is_preempted, @function;" > > "__raw_callee_save___kvm_vcpu_is_preempted:" > > "movq __per_cpu_offset(,%rdi,8), %rax;" > > -"cmpb $0, " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rax);" > > -"setne %al;" > > +"movb " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rax), %al;" > > +"andb $" __stringify(KVM_VCPU_PREEMPTED) ", %al;" > > Eww, the existing code is sketchy. It relies on the compiler to store _Bool/bool > in a single byte since %rax may be non-zero from the __per_cpu_offset(), and > modifying %al doesn't zero %rax[63:8]. I doubt gcc or clang use anything but a > single byte on x86-64, but "andl" is just as cheap so I don't see any harm in > being paranoid. Agreed, better to clear the rest of rax just to be safe. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86: fix kvm_vcpu_is_preempted 2022-01-12 11:19 [PATCH] KVM: x86: fix kvm_vcpu_is_preempted Li RongQing 2022-01-12 16:43 ` Sean Christopherson @ 2022-01-13 11:03 ` Paolo Bonzini 2022-01-14 9:49 ` 答复: " Li,Rongqing 1 sibling, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2022-01-13 11:03 UTC (permalink / raw) To: Li RongQing, seanjc, vkuznets, wanpengli, jmattson, tglx, mingo, bp, x86, hpa, rkrcmar, kvm, joro On 1/12/22 12:19, Li RongQing wrote: > After support paravirtualized TLB shootdowns, steal_time.preempted > includes not only KVM_VCPU_PREEMPTED, but also KVM_VCPU_FLUSH_TLB > > and kvm_vcpu_is_preempted should test only with KVM_VCPU_PREEMPTED The combination of PREEMPTED=0,FLUSH_TLB=1 is invalid and can only happens if the guest malfunctions (which it doesn't, it uses cmpxchg to set KVM_VCPU_PREEMPTED); the host only does an xchg with 0 as the new value. Since this is guest code, this patch does not change an actual error in the code, does it? Paolo > Fixes: 858a43aae2367 ("KVM: X86: use paravirtualized TLB Shootdown") > Signed-off-by: Li RongQing<lirongqing@baidu.com> > --- > arch/x86/kernel/kvm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c > index 59abbda..a9202d9 100644 > --- a/arch/x86/kernel/kvm.c > +++ b/arch/x86/kernel/kvm.c > @@ -1025,8 +1025,8 @@ asm( > ".type __raw_callee_save___kvm_vcpu_is_preempted, @function;" > "__raw_callee_save___kvm_vcpu_is_preempted:" > "movq __per_cpu_offset(,%rdi,8), %rax;" > -"cmpb $0, " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rax);" > -"setne %al;" > +"movb " __stringify(KVM_STEAL_TIME_preempted) "+steal_time(%rax), %al;" > +"andb $" __stringify(KVM_VCPU_PREEMPTED) ", %al;" > "ret;" > ".size __raw_callee_save___kvm_vcpu_is_preempted, .-__raw_callee_save___kvm_vcpu_is_preempted;" > ".popsection"); ^ permalink raw reply [flat|nested] 6+ messages in thread
* 答复: [PATCH] KVM: x86: fix kvm_vcpu_is_preempted 2022-01-13 11:03 ` Paolo Bonzini @ 2022-01-14 9:49 ` Li,Rongqing 0 siblings, 0 replies; 6+ messages in thread From: Li,Rongqing @ 2022-01-14 9:49 UTC (permalink / raw) To: Paolo Bonzini, seanjc, vkuznets, wanpengli, jmattson, tglx, mingo, bp, x86, hpa, rkrcmar, kvm, joro > -----邮件原件----- > The combination of PREEMPTED=0,FLUSH_TLB=1 is invalid and can only > happens if the guest malfunctions (which it doesn't, it uses cmpxchg to set > KVM_VCPU_PREEMPTED); the host only does an xchg with 0 as the new value. > Since this is guest code, this patch does not change an actual error in the code, > does it? > You are right, this is not a issue in practice Thanks -Li ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-14 9:49 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-12 11:19 [PATCH] KVM: x86: fix kvm_vcpu_is_preempted Li RongQing 2022-01-12 16:43 ` Sean Christopherson 2022-01-13 8:06 ` 答复: " Li,Rongqing 2022-01-13 10:00 ` Peter Zijlstra 2022-01-13 11:03 ` Paolo Bonzini 2022-01-14 9:49 ` 答复: " Li,Rongqing
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).