| Message ID | 20240517173926.965351-12-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: CPUID overhaul, fixes, and caching | expand |
On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote: > Reject KVM_CAP_X86_DISABLE_EXITS if vCPUs have been created, as disabling > PAUSE/MWAIT/HLT exits after vCPUs have been created is broken and useless, > e.g. except for PAUSE on SVM, the relevant intercepts aren't updated after > vCPU creation. vCPUs may also end up with an inconsistent configuration > if exits are disabled between creation of multiple vCPUs. Hi, I am not sure that PAUSE intercepts are updated either, I wasn't able to find a code that does this. I agree with this change, but note that there was some talk on the mailing list to allow to selectively disable VM exits (e.g PAUSE, MWAIT, ...) only on some vCPUs, based on the claim that some vCPUs might run RT tasks, while some might be housekeeping. I haven't followed those discussions closely. Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky > > Cc: Hou Wenlong <houwenlong.hwl@antgroup.com> > Link: https://lore.kernel.org/all/9227068821b275ac547eb2ede09ec65d2281fe07.1680179693.git.houwenlong.hwl@antgroup.com > Link: https://lore.kernel.org/all/20230121020738.2973-2-kechenl@nvidia.com > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > Documentation/virt/kvm/api.rst | 1 + > arch/x86/kvm/x86.c | 6 ++++++ > 2 files changed, 7 insertions(+) > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > index 6ab8b5b7c64e..884846282d06 100644 > --- a/Documentation/virt/kvm/api.rst > +++ b/Documentation/virt/kvm/api.rst > @@ -7645,6 +7645,7 @@ branch to guests' 0x200 interrupt vector. > :Architectures: x86 > :Parameters: args[0] defines which exits are disabled > :Returns: 0 on success, -EINVAL when args[0] contains invalid exits > + or if any vCPUs have already been created > > Valid bits in args[0] are:: > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index bb34891d2f0a..4cb0c150a2f8 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6568,6 +6568,10 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, > if (cap->args[0] & ~KVM_X86_DISABLE_VALID_EXITS) > break; > > + mutex_lock(&kvm->lock); > + if (kvm->created_vcpus) > + goto disable_exits_unlock; > + > if (cap->args[0] & KVM_X86_DISABLE_EXITS_PAUSE) > kvm->arch.pause_in_guest = true; > > @@ -6589,6 +6593,8 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, > } > > r = 0; > +disable_exits_unlock: > + mutex_unlock(&kvm->lock); > break; > case KVM_CAP_MSR_PLATFORM_INFO: > kvm->arch.guest_can_read_msr_platform_info = cap->args[0];
On Thu, Jul 04, 2024, Maxim Levitsky wrote: > On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote: > > Reject KVM_CAP_X86_DISABLE_EXITS if vCPUs have been created, as disabling > > PAUSE/MWAIT/HLT exits after vCPUs have been created is broken and useless, > > e.g. except for PAUSE on SVM, the relevant intercepts aren't updated after > > vCPU creation. vCPUs may also end up with an inconsistent configuration > > if exits are disabled between creation of multiple vCPUs. > > Hi, > > I am not sure that PAUSE intercepts are updated either, I wasn't able to find a code > that does this. > > I agree with this change, but note that there was some talk on the mailing > list to allow to selectively disable VM exits (e.g PAUSE, MWAIT, ...) only on > some vCPUs, based on the claim that some vCPUs might run RT tasks, while some > might be housekeeping. I haven't followed those discussions closely. This change is actually pulled from that series[*]. IIRC, v1 of that series didn't close the VM-scoped hole, and the overall code was much more complex as a result. [*] https://lore.kernel.org/all/20230121020738.2973-2-kechenl@nvidia.com
On 5/18/2024 1:38 AM, Sean Christopherson wrote: > Reject KVM_CAP_X86_DISABLE_EXITS if vCPUs have been created, as disabling > PAUSE/MWAIT/HLT exits after vCPUs have been created is broken and useless, > e.g. except for PAUSE on SVM, the relevant intercepts aren't updated after > vCPU creation. vCPUs may also end up with an inconsistent configuration > if exits are disabled between creation of multiple vCPUs. > > Cc: Hou Wenlong <houwenlong.hwl@antgroup.com> > Link: https://lore.kernel.org/all/9227068821b275ac547eb2ede09ec65d2281fe07.1680179693.git.houwenlong.hwl@antgroup.com > Link: https://lore.kernel.org/all/20230121020738.2973-2-kechenl@nvidia.com > Signed-off-by: Sean Christopherson <seanjc@google.com> Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com> > --- > Documentation/virt/kvm/api.rst | 1 + > arch/x86/kvm/x86.c | 6 ++++++ > 2 files changed, 7 insertions(+) > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > index 6ab8b5b7c64e..884846282d06 100644 > --- a/Documentation/virt/kvm/api.rst > +++ b/Documentation/virt/kvm/api.rst > @@ -7645,6 +7645,7 @@ branch to guests' 0x200 interrupt vector. > :Architectures: x86 > :Parameters: args[0] defines which exits are disabled > :Returns: 0 on success, -EINVAL when args[0] contains invalid exits > + or if any vCPUs have already been created > > Valid bits in args[0] are:: > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index bb34891d2f0a..4cb0c150a2f8 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6568,6 +6568,10 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, > if (cap->args[0] & ~KVM_X86_DISABLE_VALID_EXITS) > break; > > + mutex_lock(&kvm->lock); > + if (kvm->created_vcpus) > + goto disable_exits_unlock; > + > if (cap->args[0] & KVM_X86_DISABLE_EXITS_PAUSE) > kvm->arch.pause_in_guest = true; > > @@ -6589,6 +6593,8 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, > } > > r = 0; > +disable_exits_unlock: > + mutex_unlock(&kvm->lock); > break; > case KVM_CAP_MSR_PLATFORM_INFO: > kvm->arch.guest_can_read_msr_platform_info = cap->args[0];
On Mon, 2024-07-08 at 19:43 +0000, Sean Christopherson wrote: > On Thu, Jul 04, 2024, Maxim Levitsky wrote: > > On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote: > > > Reject KVM_CAP_X86_DISABLE_EXITS if vCPUs have been created, as disabling > > > PAUSE/MWAIT/HLT exits after vCPUs have been created is broken and useless, > > > e.g. except for PAUSE on SVM, the relevant intercepts aren't updated after > > > vCPU creation. vCPUs may also end up with an inconsistent configuration > > > if exits are disabled between creation of multiple vCPUs. > > > > Hi, > > > > I am not sure that PAUSE intercepts are updated either, I wasn't able to find a code > > that does this. > > > > I agree with this change, but note that there was some talk on the mailing > > list to allow to selectively disable VM exits (e.g PAUSE, MWAIT, ...) only on > > some vCPUs, based on the claim that some vCPUs might run RT tasks, while some > > might be housekeeping. I haven't followed those discussions closely. > > This change is actually pulled from that series[*]. IIRC, v1 of that series > didn't close the VM-scoped hole, and the overall code was much more complex as > a result. > > [*] https://lore.kernel.org/all/20230121020738.2973-2-kechenl@nvidia.com > Hi, Thanks for the pointer, I searched for this patch series in many places but I couldn't find it. Any idea what happened with this patch series btw? Best regards, Maxim Levitsky
On Wed, Jul 24, 2024, Maxim Levitsky wrote: > On Mon, 2024-07-08 at 19:43 +0000, Sean Christopherson wrote: > > On Thu, Jul 04, 2024, Maxim Levitsky wrote: > > > On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote: > > > > Reject KVM_CAP_X86_DISABLE_EXITS if vCPUs have been created, as disabling > > > > PAUSE/MWAIT/HLT exits after vCPUs have been created is broken and useless, > > > > e.g. except for PAUSE on SVM, the relevant intercepts aren't updated after > > > > vCPU creation. vCPUs may also end up with an inconsistent configuration > > > > if exits are disabled between creation of multiple vCPUs. > > > > > > Hi, > > > > > > I am not sure that PAUSE intercepts are updated either, I wasn't able to find a code > > > that does this. > > > > > > I agree with this change, but note that there was some talk on the mailing > > > list to allow to selectively disable VM exits (e.g PAUSE, MWAIT, ...) only on > > > some vCPUs, based on the claim that some vCPUs might run RT tasks, while some > > > might be housekeeping. I haven't followed those discussions closely. > > > > This change is actually pulled from that series[*]. IIRC, v1 of that series > > didn't close the VM-scoped hole, and the overall code was much more complex as > > a result. > > > > [*] https://lore.kernel.org/all/20230121020738.2973-2-kechenl@nvidia.com > > > > Hi, > Thanks for the pointer, I searched for this patch series in many places but I > couldn't find it. > Any idea what happened with this patch series btw? Nope. IIRC, v6 was close to being ready and only had a few cosmetic issues, but the author never posted a v7.
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index 6ab8b5b7c64e..884846282d06 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -7645,6 +7645,7 @@ branch to guests' 0x200 interrupt vector. :Architectures: x86 :Parameters: args[0] defines which exits are disabled :Returns: 0 on success, -EINVAL when args[0] contains invalid exits + or if any vCPUs have already been created Valid bits in args[0] are:: diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index bb34891d2f0a..4cb0c150a2f8 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -6568,6 +6568,10 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, if (cap->args[0] & ~KVM_X86_DISABLE_VALID_EXITS) break; + mutex_lock(&kvm->lock); + if (kvm->created_vcpus) + goto disable_exits_unlock; + if (cap->args[0] & KVM_X86_DISABLE_EXITS_PAUSE) kvm->arch.pause_in_guest = true; @@ -6589,6 +6593,8 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, } r = 0; +disable_exits_unlock: + mutex_unlock(&kvm->lock); break; case KVM_CAP_MSR_PLATFORM_INFO: kvm->arch.guest_can_read_msr_platform_info = cap->args[0];
Reject KVM_CAP_X86_DISABLE_EXITS if vCPUs have been created, as disabling PAUSE/MWAIT/HLT exits after vCPUs have been created is broken and useless, e.g. except for PAUSE on SVM, the relevant intercepts aren't updated after vCPU creation. vCPUs may also end up with an inconsistent configuration if exits are disabled between creation of multiple vCPUs. Cc: Hou Wenlong <houwenlong.hwl@antgroup.com> Link: https://lore.kernel.org/all/9227068821b275ac547eb2ede09ec65d2281fe07.1680179693.git.houwenlong.hwl@antgroup.com Link: https://lore.kernel.org/all/20230121020738.2973-2-kechenl@nvidia.com Signed-off-by: Sean Christopherson <seanjc@google.com> --- Documentation/virt/kvm/api.rst | 1 + arch/x86/kvm/x86.c | 6 ++++++ 2 files changed, 7 insertions(+)