From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christian Borntraeger Date: Mon, 27 Sep 2021 06:54:28 +0000 Subject: Re: [PATCH 01/14] KVM: s390: Ensure kvm_arch_no_poll() is read once when blocking vCPU Message-Id: <01fa2462-6652-7206-5ef3-bacb3452b4c8@de.ibm.com> List-Id: References: <20210925005528.1145584-1-seanjc@google.com> <20210925005528.1145584-2-seanjc@google.com> In-Reply-To: <20210925005528.1145584-2-seanjc@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Sean Christopherson , Marc Zyngier , Huacai Chen , Aleksandar Markovic , Paul Mackerras , Janosch Frank , Paolo Bonzini Cc: James Morse , Alexandru Elisei , Suzuki K Poulose , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-mips@vger.kernel.org, kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, linux-kernel@vger.kernel.org, David Matlack , Jing Zhang Am 25.09.21 um 02:55 schrieb Sean Christopherson: > Wrap s390's halt_poll_max_steal with READ_ONCE and snapshot the result of > kvm_arch_no_poll() in kvm_vcpu_block() to avoid a mostly-theoretical, > largely benign bug on s390 where the result of kvm_arch_no_poll() could > change due to userspace modifying halt_poll_max_steal while the vCPU is > blocking. The bug is largely benign as it will either cause KVM to skip > updating halt-polling times (no_poll toggles false=>true) or to update > halt-polling times with a slightly flawed block_ns. > > Note, READ_ONCE is unnecessary in the current code, add it in case the > arch hook is ever inlined, and to provide a hint that userspace can > change the param at will. > > Fixes: 8b905d28ee17 ("KVM: s390: provide kvm_arch_no_poll function") > Cc: Christian Borntraeger > Signed-off-by: Sean Christopherson Reviewed-by: Christian Borntraeger > --- > arch/s390/kvm/kvm-s390.c | 2 +- > virt/kvm/kvm_main.c | 5 +++-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 6a6dd5e1daf6..7cabe6778b1b 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -3446,7 +3446,7 @@ bool kvm_arch_no_poll(struct kvm_vcpu *vcpu) > { > /* do not poll with more than halt_poll_max_steal percent of steal time */ > if (S390_lowcore.avg_steal_timer * 100 / (TICK_USEC << 12) >> - halt_poll_max_steal) { > + READ_ONCE(halt_poll_max_steal)) { > vcpu->stat.halt_no_poll_steal++; > return true; > } > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 191dac6b1bed..768a4cbb26a6 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -3213,6 +3213,7 @@ update_halt_poll_stats(struct kvm_vcpu *vcpu, u64 poll_ns, bool waited) > */ > void kvm_vcpu_block(struct kvm_vcpu *vcpu) > { > + bool halt_poll_allowed = !kvm_arch_no_poll(vcpu); > ktime_t start, cur, poll_end; > bool waited = false; > u64 block_ns; > @@ -3220,7 +3221,7 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu) > kvm_arch_vcpu_blocking(vcpu); > > start = cur = poll_end = ktime_get(); > - if (vcpu->halt_poll_ns && !kvm_arch_no_poll(vcpu)) { > + if (vcpu->halt_poll_ns && halt_poll_allowed) { > ktime_t stop = ktime_add_ns(ktime_get(), vcpu->halt_poll_ns); > > ++vcpu->stat.generic.halt_attempted_poll; > @@ -3275,7 +3276,7 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu) > update_halt_poll_stats( > vcpu, ktime_to_ns(ktime_sub(poll_end, start)), waited); > > - if (!kvm_arch_no_poll(vcpu)) { > + if (halt_poll_allowed) { > if (!vcpu_valid_wakeup(vcpu)) { > shrink_halt_poll_ns(vcpu); > } else if (vcpu->kvm->max_halt_poll_ns) { >