| Message ID | 20221117001657.1067231-4-dmatlack@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Restore original behavior of kvm.halt_poll_ns | expand |
Hi David, On 2022/11/17 8:16, David Matlack wrote: > Obey kvm.halt_poll_ns in VMs not using KVM_CAP_HALT_POLL on every halt, > rather than just sampling the module parameter when the VM is first s/first/firstly > created. This restore the original behavior of kvm.halt_poll_ns for VMs s/restore/restores > that have not opted into KVM_CAP_HALT_POLL. > > Notably, this change restores the ability for admins to disable or > change the maximum halt-polling time system wide for VMs not using > KVM_CAP_HALT_POLL. Should we add more detailed comments about relationship between KVM_CAP_HALT_POLL and kvm.halt_poll_ns in Documentation/virt/kvm/api.rst? Something like: "once KVM_CAP_HALT_POLL is used for a target VM, it will completely ignores any future changes to kvm.halt_poll_ns..." > Reported-by: Christian Borntraeger <borntraeger@de.ibm.com> > Fixes: acd05785e48c ("kvm: add capability for halt polling") > Signed-off-by: David Matlack <dmatlack@google.com> > --- > include/linux/kvm_host.h | 1 + > virt/kvm/kvm_main.c | 27 ++++++++++++++++++++++++--- > 2 files changed, 25 insertions(+), 3 deletions(-) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index e6e66c5e56f2..253ad055b6ad 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -788,6 +788,7 @@ struct kvm { > struct srcu_struct srcu; > struct srcu_struct irq_srcu; > pid_t userspace_pid; > + bool override_halt_poll_ns; > unsigned int max_halt_poll_ns; > u32 dirty_ring_size; > bool vm_bugged; > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 78caf19608eb..7f73ce99bd0e 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -1198,8 +1198,6 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname) > goto out_err_no_arch_destroy_vm; > } > > - kvm->max_halt_poll_ns = halt_poll_ns; > - > r = kvm_arch_init_vm(kvm, type); > if (r) > goto out_err_no_arch_destroy_vm; > @@ -3490,7 +3488,20 @@ static inline void update_halt_poll_stats(struct kvm_vcpu *vcpu, ktime_t start, > > static unsigned int kvm_vcpu_max_halt_poll_ns(struct kvm_vcpu *vcpu) > { > - return READ_ONCE(vcpu->kvm->max_halt_poll_ns); > + struct kvm *kvm = vcpu->kvm; > + > + if (kvm->override_halt_poll_ns) { > + /* > + * Ensure kvm->max_halt_poll_ns is not read before > + * kvm->override_halt_poll_ns. > + * > + * Pairs with the smp_wmb() when enabling KVM_CAP_HALT_POLL. > + */ > + smp_rmb(); > + return READ_ONCE(kvm->max_halt_poll_ns); > + } > + > + return READ_ONCE(halt_poll_ns); > } > > /* > @@ -4600,6 +4611,16 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm, > return -EINVAL; > > kvm->max_halt_poll_ns = cap->args[0]; > + > + /* > + * Ensure kvm->override_halt_poll_ns does not become visible > + * before kvm->max_halt_poll_ns. > + * > + * Pairs with the smp_rmb() in kvm_vcpu_max_halt_poll_ns(). > + */ > + smp_wmb(); > + kvm->override_halt_poll_ns = true; > + > return 0; > } > case KVM_CAP_DIRTY_LOG_RING: Looks good to me: Reviewed-by: Yanan Wang <wangyanan55@huawei.com> Thanks, Yanan
On Fri, Nov 18, 2022 at 12:28 AM wangyanan (Y) <wangyanan55@huawei.com> wrote: > > Hi David, > > On 2022/11/17 8:16, David Matlack wrote: > > Obey kvm.halt_poll_ns in VMs not using KVM_CAP_HALT_POLL on every halt, > > rather than just sampling the module parameter when the VM is first > s/first/firstly > > created. This restore the original behavior of kvm.halt_poll_ns for VMs > s/restore/restores > > that have not opted into KVM_CAP_HALT_POLL. > > > > Notably, this change restores the ability for admins to disable or > > change the maximum halt-polling time system wide for VMs not using > > KVM_CAP_HALT_POLL. > Should we add more detailed comments about relationship > between KVM_CAP_HALT_POLL and kvm.halt_poll_ns in > Documentation/virt/kvm/api.rst? Something like: > "once KVM_CAP_HALT_POLL is used for a target VM, it will > completely ignores any future changes to kvm.halt_poll_ns..." Yes we should. I will do some testing on this series today and then resend the series as a non-RFC with the Documentation changes. Thanks for the reviews. > > Reported-by: Christian Borntraeger <borntraeger@de.ibm.com> > > Fixes: acd05785e48c ("kvm: add capability for halt polling") > > Signed-off-by: David Matlack <dmatlack@google.com> > > --- > > include/linux/kvm_host.h | 1 + > > virt/kvm/kvm_main.c | 27 ++++++++++++++++++++++++--- > > 2 files changed, 25 insertions(+), 3 deletions(-) > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > index e6e66c5e56f2..253ad055b6ad 100644 > > --- a/include/linux/kvm_host.h > > +++ b/include/linux/kvm_host.h > > @@ -788,6 +788,7 @@ struct kvm { > > struct srcu_struct srcu; > > struct srcu_struct irq_srcu; > > pid_t userspace_pid; > > + bool override_halt_poll_ns; > > unsigned int max_halt_poll_ns; > > u32 dirty_ring_size; > > bool vm_bugged; > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 78caf19608eb..7f73ce99bd0e 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -1198,8 +1198,6 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname) > > goto out_err_no_arch_destroy_vm; > > } > > > > - kvm->max_halt_poll_ns = halt_poll_ns; > > - > > r = kvm_arch_init_vm(kvm, type); > > if (r) > > goto out_err_no_arch_destroy_vm; > > @@ -3490,7 +3488,20 @@ static inline void update_halt_poll_stats(struct kvm_vcpu *vcpu, ktime_t start, > > > > static unsigned int kvm_vcpu_max_halt_poll_ns(struct kvm_vcpu *vcpu) > > { > > - return READ_ONCE(vcpu->kvm->max_halt_poll_ns); > > + struct kvm *kvm = vcpu->kvm; > > + > > + if (kvm->override_halt_poll_ns) { > > + /* > > + * Ensure kvm->max_halt_poll_ns is not read before > > + * kvm->override_halt_poll_ns. > > + * > > + * Pairs with the smp_wmb() when enabling KVM_CAP_HALT_POLL. > > + */ > > + smp_rmb(); > > + return READ_ONCE(kvm->max_halt_poll_ns); > > + } > > + > > + return READ_ONCE(halt_poll_ns); > > } > > > > /* > > @@ -4600,6 +4611,16 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm, > > return -EINVAL; > > > > kvm->max_halt_poll_ns = cap->args[0]; > > + > > + /* > > + * Ensure kvm->override_halt_poll_ns does not become visible > > + * before kvm->max_halt_poll_ns. > > + * > > + * Pairs with the smp_rmb() in kvm_vcpu_max_halt_poll_ns(). > > + */ > > + smp_wmb(); > > + kvm->override_halt_poll_ns = true; > > + > > return 0; > > } > > case KVM_CAP_DIRTY_LOG_RING: > Looks good to me: > Reviewed-by: Yanan Wang <wangyanan55@huawei.com> > > Thanks, > Yanan
On Fri, Nov 18, 2022 at 8:58 AM David Matlack <dmatlack@google.com> wrote: > > On Fri, Nov 18, 2022 at 12:28 AM wangyanan (Y) <wangyanan55@huawei.com> wrote: > > > > Hi David, > > > > On 2022/11/17 8:16, David Matlack wrote: > > > Obey kvm.halt_poll_ns in VMs not using KVM_CAP_HALT_POLL on every halt, > > > rather than just sampling the module parameter when the VM is first > > s/first/firstly > > > created. This restore the original behavior of kvm.halt_poll_ns for VMs > > s/restore/restores > > > that have not opted into KVM_CAP_HALT_POLL. > > > > > > Notably, this change restores the ability for admins to disable or > > > change the maximum halt-polling time system wide for VMs not using > > > KVM_CAP_HALT_POLL. > > Should we add more detailed comments about relationship > > between KVM_CAP_HALT_POLL and kvm.halt_poll_ns in > > Documentation/virt/kvm/api.rst? Something like: > > "once KVM_CAP_HALT_POLL is used for a target VM, it will > > completely ignores any future changes to kvm.halt_poll_ns..." > > Yes we should. > > I will do some testing on this series today and then resend the series > as a non-RFC with the Documentation changes. > > Thanks for the reviews. Initial testing looks good but I did not have time to finish due to a bug in how our VMM is currently using KVM_CAP_HALT_POLL. I will be out all next week so I'll pick this up the week after. > > > > Reported-by: Christian Borntraeger <borntraeger@de.ibm.com> > > > Fixes: acd05785e48c ("kvm: add capability for halt polling") > > > Signed-off-by: David Matlack <dmatlack@google.com> > > > --- > > > include/linux/kvm_host.h | 1 + > > > virt/kvm/kvm_main.c | 27 ++++++++++++++++++++++++--- > > > 2 files changed, 25 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > > index e6e66c5e56f2..253ad055b6ad 100644 > > > --- a/include/linux/kvm_host.h > > > +++ b/include/linux/kvm_host.h > > > @@ -788,6 +788,7 @@ struct kvm { > > > struct srcu_struct srcu; > > > struct srcu_struct irq_srcu; > > > pid_t userspace_pid; > > > + bool override_halt_poll_ns; > > > unsigned int max_halt_poll_ns; > > > u32 dirty_ring_size; > > > bool vm_bugged; > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > > index 78caf19608eb..7f73ce99bd0e 100644 > > > --- a/virt/kvm/kvm_main.c > > > +++ b/virt/kvm/kvm_main.c > > > @@ -1198,8 +1198,6 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname) > > > goto out_err_no_arch_destroy_vm; > > > } > > > > > > - kvm->max_halt_poll_ns = halt_poll_ns; > > > - > > > r = kvm_arch_init_vm(kvm, type); > > > if (r) > > > goto out_err_no_arch_destroy_vm; > > > @@ -3490,7 +3488,20 @@ static inline void update_halt_poll_stats(struct kvm_vcpu *vcpu, ktime_t start, > > > > > > static unsigned int kvm_vcpu_max_halt_poll_ns(struct kvm_vcpu *vcpu) > > > { > > > - return READ_ONCE(vcpu->kvm->max_halt_poll_ns); > > > + struct kvm *kvm = vcpu->kvm; > > > + > > > + if (kvm->override_halt_poll_ns) { > > > + /* > > > + * Ensure kvm->max_halt_poll_ns is not read before > > > + * kvm->override_halt_poll_ns. > > > + * > > > + * Pairs with the smp_wmb() when enabling KVM_CAP_HALT_POLL. > > > + */ > > > + smp_rmb(); > > > + return READ_ONCE(kvm->max_halt_poll_ns); > > > + } > > > + > > > + return READ_ONCE(halt_poll_ns); > > > } > > > > > > /* > > > @@ -4600,6 +4611,16 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm, > > > return -EINVAL; > > > > > > kvm->max_halt_poll_ns = cap->args[0]; > > > + > > > + /* > > > + * Ensure kvm->override_halt_poll_ns does not become visible > > > + * before kvm->max_halt_poll_ns. > > > + * > > > + * Pairs with the smp_rmb() in kvm_vcpu_max_halt_poll_ns(). > > > + */ > > > + smp_wmb(); > > > + kvm->override_halt_poll_ns = true; > > > + > > > return 0; > > > } > > > case KVM_CAP_DIRTY_LOG_RING: > > Looks good to me: > > Reviewed-by: Yanan Wang <wangyanan55@huawei.com> > > > > Thanks, > > Yanan
On 2022/11/19 9:25, David Matlack wrote: > On Fri, Nov 18, 2022 at 8:58 AM David Matlack <dmatlack@google.com> wrote: >> On Fri, Nov 18, 2022 at 12:28 AM wangyanan (Y) <wangyanan55@huawei.com> wrote: >>> Hi David, >>> >>> On 2022/11/17 8:16, David Matlack wrote: >>>> Obey kvm.halt_poll_ns in VMs not using KVM_CAP_HALT_POLL on every halt, >>>> rather than just sampling the module parameter when the VM is first >>> s/first/firstly >>>> created. This restore the original behavior of kvm.halt_poll_ns for VMs >>> s/restore/restores >>>> that have not opted into KVM_CAP_HALT_POLL. >>>> >>>> Notably, this change restores the ability for admins to disable or >>>> change the maximum halt-polling time system wide for VMs not using >>>> KVM_CAP_HALT_POLL. >>> Should we add more detailed comments about relationship >>> between KVM_CAP_HALT_POLL and kvm.halt_poll_ns in >>> Documentation/virt/kvm/api.rst? Something like: >>> "once KVM_CAP_HALT_POLL is used for a target VM, it will >>> completely ignores any future changes to kvm.halt_poll_ns..." >> Yes we should. >> >> I will do some testing on this series today and then resend the series >> as a non-RFC with the Documentation changes. >> >> Thanks for the reviews. > Initial testing looks good but I did not have time to finish due to a > bug in how our VMM is currently using KVM_CAP_HALT_POLL. I will be out > all next week so I'll pick this up the week after. OK, thanks. Yanan >>>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>> Fixes: acd05785e48c ("kvm: add capability for halt polling") >>>> Signed-off-by: David Matlack <dmatlack@google.com> >>>> --- >>>> include/linux/kvm_host.h | 1 + >>>> virt/kvm/kvm_main.c | 27 ++++++++++++++++++++++++--- >>>> 2 files changed, 25 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >>>> index e6e66c5e56f2..253ad055b6ad 100644 >>>> --- a/include/linux/kvm_host.h >>>> +++ b/include/linux/kvm_host.h >>>> @@ -788,6 +788,7 @@ struct kvm { >>>> struct srcu_struct srcu; >>>> struct srcu_struct irq_srcu; >>>> pid_t userspace_pid; >>>> + bool override_halt_poll_ns; >>>> unsigned int max_halt_poll_ns; >>>> u32 dirty_ring_size; >>>> bool vm_bugged; >>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >>>> index 78caf19608eb..7f73ce99bd0e 100644 >>>> --- a/virt/kvm/kvm_main.c >>>> +++ b/virt/kvm/kvm_main.c >>>> @@ -1198,8 +1198,6 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname) >>>> goto out_err_no_arch_destroy_vm; >>>> } >>>> >>>> - kvm->max_halt_poll_ns = halt_poll_ns; >>>> - >>>> r = kvm_arch_init_vm(kvm, type); >>>> if (r) >>>> goto out_err_no_arch_destroy_vm; >>>> @@ -3490,7 +3488,20 @@ static inline void update_halt_poll_stats(struct kvm_vcpu *vcpu, ktime_t start, >>>> >>>> static unsigned int kvm_vcpu_max_halt_poll_ns(struct kvm_vcpu *vcpu) >>>> { >>>> - return READ_ONCE(vcpu->kvm->max_halt_poll_ns); >>>> + struct kvm *kvm = vcpu->kvm; >>>> + >>>> + if (kvm->override_halt_poll_ns) { >>>> + /* >>>> + * Ensure kvm->max_halt_poll_ns is not read before >>>> + * kvm->override_halt_poll_ns. >>>> + * >>>> + * Pairs with the smp_wmb() when enabling KVM_CAP_HALT_POLL. >>>> + */ >>>> + smp_rmb(); >>>> + return READ_ONCE(kvm->max_halt_poll_ns); >>>> + } >>>> + >>>> + return READ_ONCE(halt_poll_ns); >>>> } >>>> >>>> /* >>>> @@ -4600,6 +4611,16 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm, >>>> return -EINVAL; >>>> >>>> kvm->max_halt_poll_ns = cap->args[0]; >>>> + >>>> + /* >>>> + * Ensure kvm->override_halt_poll_ns does not become visible >>>> + * before kvm->max_halt_poll_ns. >>>> + * >>>> + * Pairs with the smp_rmb() in kvm_vcpu_max_halt_poll_ns(). >>>> + */ >>>> + smp_wmb(); >>>> + kvm->override_halt_poll_ns = true; >>>> + >>>> return 0; >>>> } >>>> case KVM_CAP_DIRTY_LOG_RING: >>> Looks good to me: >>> Reviewed-by: Yanan Wang <wangyanan55@huawei.com> >>> >>> Thanks, >>> Yanan > .
Am 17.11.22 um 01:16 schrieb David Matlack: > Obey kvm.halt_poll_ns in VMs not using KVM_CAP_HALT_POLL on every halt, > rather than just sampling the module parameter when the VM is first > created. This restore the original behavior of kvm.halt_poll_ns for VMs > that have not opted into KVM_CAP_HALT_POLL. > > Notably, this change restores the ability for admins to disable or > change the maximum halt-polling time system wide for VMs not using > KVM_CAP_HALT_POLL. > > Reported-by: Christian Borntraeger <borntraeger@de.ibm.com> > Fixes: acd05785e48c ("kvm: add capability for halt polling") > Signed-off-by: David Matlack <dmatlack@google.com> Tested-by: Christian Borntraeger <borntraeger@linux.ibm.com> Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com> One thing. This does not apply without the other 2 patches. Not sure if we want to redo this somehow to allow for stable backports? > --- > include/linux/kvm_host.h | 1 + > virt/kvm/kvm_main.c | 27 ++++++++++++++++++++++++--- > 2 files changed, 25 insertions(+), 3 deletions(-) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index e6e66c5e56f2..253ad055b6ad 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -788,6 +788,7 @@ struct kvm { > struct srcu_struct srcu; > struct srcu_struct irq_srcu; > pid_t userspace_pid; > + bool override_halt_poll_ns; > unsigned int max_halt_poll_ns; > u32 dirty_ring_size; > bool vm_bugged; > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 78caf19608eb..7f73ce99bd0e 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -1198,8 +1198,6 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname) > goto out_err_no_arch_destroy_vm; > } > > - kvm->max_halt_poll_ns = halt_poll_ns; > - > r = kvm_arch_init_vm(kvm, type); > if (r) > goto out_err_no_arch_destroy_vm; > @@ -3490,7 +3488,20 @@ static inline void update_halt_poll_stats(struct kvm_vcpu *vcpu, ktime_t start, > > static unsigned int kvm_vcpu_max_halt_poll_ns(struct kvm_vcpu *vcpu) > { > - return READ_ONCE(vcpu->kvm->max_halt_poll_ns); > + struct kvm *kvm = vcpu->kvm; > + > + if (kvm->override_halt_poll_ns) { > + /* > + * Ensure kvm->max_halt_poll_ns is not read before > + * kvm->override_halt_poll_ns. > + * > + * Pairs with the smp_wmb() when enabling KVM_CAP_HALT_POLL. > + */ > + smp_rmb(); > + return READ_ONCE(kvm->max_halt_poll_ns); > + } > + > + return READ_ONCE(halt_poll_ns); > } > > /* > @@ -4600,6 +4611,16 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm, > return -EINVAL; > > kvm->max_halt_poll_ns = cap->args[0]; > + > + /* > + * Ensure kvm->override_halt_poll_ns does not become visible > + * before kvm->max_halt_poll_ns. > + * > + * Pairs with the smp_rmb() in kvm_vcpu_max_halt_poll_ns(). > + */ > + smp_wmb(); > + kvm->override_halt_poll_ns = true; > + > return 0; > } > case KVM_CAP_DIRTY_LOG_RING:
Am 21.11.22 um 10:22 schrieb Christian Borntraeger: > Am 17.11.22 um 01:16 schrieb David Matlack: >> Obey kvm.halt_poll_ns in VMs not using KVM_CAP_HALT_POLL on every halt, >> rather than just sampling the module parameter when the VM is first >> created. This restore the original behavior of kvm.halt_poll_ns for VMs >> that have not opted into KVM_CAP_HALT_POLL. >> >> Notably, this change restores the ability for admins to disable or >> change the maximum halt-polling time system wide for VMs not using >> KVM_CAP_HALT_POLL. >> >> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com> >> Fixes: acd05785e48c ("kvm: add capability for halt polling") >> Signed-off-by: David Matlack <dmatlack@google.com> > > Tested-by: Christian Borntraeger <borntraeger@linux.ibm.com> > Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com> > > One thing. This does not apply without the other 2 patches. Not sure > if we want to redo this somehow to allow for stable backports? One option would be that Paolo, when applying uses the stable syntax as outlined in Documentation/process/stable-kernel-rules.rst for dependencies, e.g. Cc: <stable@vger.kernel.org> # 3.3.x: a1f84a3: sched: Check for idle Cc: <stable@vger.kernel.org> # 3.3.x: 1b9508f: sched: Rate-limit newidle Cc: <stable@vger.kernel.org> # 3.3.x: fd21073: sched: Fix affinity logic
On Sun, Nov 20, 2022 at 5:42 PM wangyanan (Y) <wangyanan55@huawei.com> wrote: > > On 2022/11/19 9:25, David Matlack wrote: > > On Fri, Nov 18, 2022 at 8:58 AM David Matlack <dmatlack@google.com> wrote: > >> On Fri, Nov 18, 2022 at 12:28 AM wangyanan (Y) <wangyanan55@huawei.com> wrote: > >>> Hi David, > >>> > >>> On 2022/11/17 8:16, David Matlack wrote: > >>>> Obey kvm.halt_poll_ns in VMs not using KVM_CAP_HALT_POLL on every halt, > >>>> rather than just sampling the module parameter when the VM is first > >>> s/first/firstly > >>>> created. This restore the original behavior of kvm.halt_poll_ns for VMs > >>> s/restore/restores > >>>> that have not opted into KVM_CAP_HALT_POLL. > >>>> > >>>> Notably, this change restores the ability for admins to disable or > >>>> change the maximum halt-polling time system wide for VMs not using > >>>> KVM_CAP_HALT_POLL. > >>> Should we add more detailed comments about relationship > >>> between KVM_CAP_HALT_POLL and kvm.halt_poll_ns in > >>> Documentation/virt/kvm/api.rst? Something like: > >>> "once KVM_CAP_HALT_POLL is used for a target VM, it will > >>> completely ignores any future changes to kvm.halt_poll_ns..." > >> Yes we should. > >> > >> I will do some testing on this series today and then resend the series > >> as a non-RFC with the Documentation changes. > >> > >> Thanks for the reviews. > > Initial testing looks good but I did not have time to finish due to a > > bug in how our VMM is currently using KVM_CAP_HALT_POLL. I will be out > > all next week so I'll pick this up the week after. > OK, thanks. I see that Linus already merged these patches into 6.1-rc7, so I've sent the Documentation changes as a separate series. https://lore.kernel.org/kvm/20221201195249.3369720-1-dmatlack@google.com/
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index e6e66c5e56f2..253ad055b6ad 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -788,6 +788,7 @@ struct kvm { struct srcu_struct srcu; struct srcu_struct irq_srcu; pid_t userspace_pid; + bool override_halt_poll_ns; unsigned int max_halt_poll_ns; u32 dirty_ring_size; bool vm_bugged; diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 78caf19608eb..7f73ce99bd0e 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1198,8 +1198,6 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname) goto out_err_no_arch_destroy_vm; } - kvm->max_halt_poll_ns = halt_poll_ns; - r = kvm_arch_init_vm(kvm, type); if (r) goto out_err_no_arch_destroy_vm; @@ -3490,7 +3488,20 @@ static inline void update_halt_poll_stats(struct kvm_vcpu *vcpu, ktime_t start, static unsigned int kvm_vcpu_max_halt_poll_ns(struct kvm_vcpu *vcpu) { - return READ_ONCE(vcpu->kvm->max_halt_poll_ns); + struct kvm *kvm = vcpu->kvm; + + if (kvm->override_halt_poll_ns) { + /* + * Ensure kvm->max_halt_poll_ns is not read before + * kvm->override_halt_poll_ns. + * + * Pairs with the smp_wmb() when enabling KVM_CAP_HALT_POLL. + */ + smp_rmb(); + return READ_ONCE(kvm->max_halt_poll_ns); + } + + return READ_ONCE(halt_poll_ns); } /* @@ -4600,6 +4611,16 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm, return -EINVAL; kvm->max_halt_poll_ns = cap->args[0]; + + /* + * Ensure kvm->override_halt_poll_ns does not become visible + * before kvm->max_halt_poll_ns. + * + * Pairs with the smp_rmb() in kvm_vcpu_max_halt_poll_ns(). + */ + smp_wmb(); + kvm->override_halt_poll_ns = true; + return 0; } case KVM_CAP_DIRTY_LOG_RING:
Obey kvm.halt_poll_ns in VMs not using KVM_CAP_HALT_POLL on every halt, rather than just sampling the module parameter when the VM is first created. This restore the original behavior of kvm.halt_poll_ns for VMs that have not opted into KVM_CAP_HALT_POLL. Notably, this change restores the ability for admins to disable or change the maximum halt-polling time system wide for VMs not using KVM_CAP_HALT_POLL. Reported-by: Christian Borntraeger <borntraeger@de.ibm.com> Fixes: acd05785e48c ("kvm: add capability for halt polling") Signed-off-by: David Matlack <dmatlack@google.com> --- include/linux/kvm_host.h | 1 + virt/kvm/kvm_main.c | 27 ++++++++++++++++++++++++--- 2 files changed, 25 insertions(+), 3 deletions(-)