| Message ID | 20210925005528.1145584-2-seanjc@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: Halt-polling fixes, cleanups and a new stat | expand |
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 <borntraeger@de.ibm.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > 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) { >
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) {
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 <borntraeger@de.ibm.com> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/s390/kvm/kvm-s390.c | 2 +- virt/kvm/kvm_main.c | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-)