kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] KVM: x86: Disallow enable KVM_CAP_X86_DISABLE_EXITS capability after vCPUs have been created
@ 2023-03-30 12:35 Hou Wenlong
  2023-03-30 12:35 ` [PATCH 2/3] KVM: x86: Don't update KVM PV feature CPUID during vCPU running Hou Wenlong
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Hou Wenlong @ 2023-03-30 12:35 UTC (permalink / raw)
  To: kvm
  Cc: Paolo Bonzini, Jonathan Corbet, Sean Christopherson,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H. Peter Anvin, linux-doc, linux-kernel

Disable PAUSE/MWAIT/HLT exits after vCPUs have been created is useless,
because PAUSE/MWAIT/HLT intercepts configuration is not changed after
vCPU created.  And two vCPUs may have inconsistent configuration if
disable PAUSE/MWAIT/HLT exits between those two vCPUs creation. Since
it's a per-VM capability, all vCPUs should keep same configuration, so
disallow enable KVM_CAP_X86_DISABLE_EXITS capability after vCPUs have
been created.

Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 Documentation/virt/kvm/api.rst | 3 ++-
 arch/x86/kvm/x86.c             | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index a69e91088d76..95a683a27cf2 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -7179,7 +7179,8 @@ 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
+:Returns: 0 on success, -EINVAL when args[0] contains invalid exits or
+	  any vCPUs have been created.
 
 Valid bits in args[0] are::
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2c0ff40e5345..7e97595465fc 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6275,6 +6275,9 @@ 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;
 
@@ -6296,6 +6299,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];

base-commit: 27d6845d258b67f4eb3debe062b7dacc67e0c393
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/3] KVM: x86: Don't update KVM PV feature CPUID during vCPU running
  2023-03-30 12:35 [PATCH 1/3] KVM: x86: Disallow enable KVM_CAP_X86_DISABLE_EXITS capability after vCPUs have been created Hou Wenlong
@ 2023-03-30 12:35 ` Hou Wenlong
  2023-04-06  3:29   ` Sean Christopherson
  2023-03-30 12:35 ` [PATCH 3/3] KVM: x86: Detect KVM_CPUID_FEATURES base before KVM PV feature CPUID updating Hou Wenlong
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Hou Wenlong @ 2023-03-30 12:35 UTC (permalink / raw)
  To: kvm
  Cc: Sean Christopherson, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-kernel

__kvm_update_cpuid_runtime() may be called during vCPU running and KVM
PV feature CPUID is updated too. But the cached KVM PV feature bitmap is
not updated. Actually, KVM PV feature CPUID shouldn't be updated,
otherwise, KVM PV feature would be broken in guest. Currently, only
KVM_FEATURE_PV_UNHALT is updated, and it's impossible after disallow
disable HLT exits. However, KVM PV feature CPUID should be updated only
in KVM_SET_CPUID{,2} ioctl.

Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 arch/x86/kvm/cpuid.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 6972e0be60fa..af92d3422c79 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -222,6 +222,17 @@ static struct kvm_cpuid_entry2 *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcp
 					     vcpu->arch.cpuid_nent);
 }
 
+static void kvm_update_pv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries,
+				int nent)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
+	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
+		(best->eax & (1 << KVM_FEATURE_PV_UNHALT)))
+		best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);
+}
+
 void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best = kvm_find_kvm_cpuid_features(vcpu);
@@ -280,11 +291,6 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
 		     cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
 		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
 
-	best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
-	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
-		(best->eax & (1 << KVM_FEATURE_PV_UNHALT)))
-		best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);
-
 	if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT)) {
 		best = cpuid_entry2_find(entries, nent, 0x1, KVM_CPUID_INDEX_NOT_SIGNIFICANT);
 		if (best)
@@ -402,6 +408,7 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
 	int r;
 
 	__kvm_update_cpuid_runtime(vcpu, e2, nent);
+	kvm_update_pv_cpuid(vcpu, e2, nent);
 
 	/*
 	 * KVM does not correctly handle changing guest CPUID after KVM_RUN, as
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/3] KVM: x86: Detect KVM_CPUID_FEATURES base before KVM PV feature CPUID updating
  2023-03-30 12:35 [PATCH 1/3] KVM: x86: Disallow enable KVM_CAP_X86_DISABLE_EXITS capability after vCPUs have been created Hou Wenlong
  2023-03-30 12:35 ` [PATCH 2/3] KVM: x86: Don't update KVM PV feature CPUID during vCPU running Hou Wenlong
@ 2023-03-30 12:35 ` Hou Wenlong
  2023-04-06  3:18 ` [PATCH 1/3] KVM: x86: Disallow enable KVM_CAP_X86_DISABLE_EXITS capability after vCPUs have been created Sean Christopherson
  2023-04-07  2:01 ` Xiaoyao Li
  3 siblings, 0 replies; 7+ messages in thread
From: Hou Wenlong @ 2023-03-30 12:35 UTC (permalink / raw)
  To: kvm
  Cc: Sean Christopherson, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
	Vitaly Kuznetsov, linux-kernel

KVM_CPUID_FEATURES is detected after CPUID updating in kvm_set_cpuid(),
then 'vcpu->arch.kvm_cpuid_base' is NULL and KVM PV feature CPUID
updating wouldn't work. So detect it early before CPUID updating in
kvm_set_cpuid().

Fixes: ee3a5f9e3d9b ("KVM: x86: Do runtime CPUID update before updating vcpu->arch.cpuid_entries")
Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 arch/x86/kvm/cpuid.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index af92d3422c79..0c8658fdb6c1 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -176,15 +176,15 @@ static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2
 	return 0;
 }
 
-static struct kvm_hypervisor_cpuid kvm_get_hypervisor_cpuid(struct kvm_vcpu *vcpu,
-							    const char *sig)
+static struct kvm_hypervisor_cpuid __kvm_get_hypervisor_cpuid(struct kvm_cpuid_entry2 *entries,
+							      int nent, const char *sig)
 {
 	struct kvm_hypervisor_cpuid cpuid = {};
 	struct kvm_cpuid_entry2 *entry;
 	u32 base;
 
 	for_each_possible_hypervisor_cpuid_base(base) {
-		entry = kvm_find_cpuid_entry(vcpu, base);
+		entry = cpuid_entry2_find(entries, nent, base, KVM_CPUID_INDEX_NOT_SIGNIFICANT);
 
 		if (entry) {
 			u32 signature[3];
@@ -202,6 +202,14 @@ static struct kvm_hypervisor_cpuid kvm_get_hypervisor_cpuid(struct kvm_vcpu *vcp
 	}
 
 	return cpuid;
+
+}
+
+static struct kvm_hypervisor_cpuid kvm_get_hypervisor_cpuid(struct kvm_vcpu *vcpu,
+							    const char *sig)
+{
+	return __kvm_get_hypervisor_cpuid(vcpu->arch.cpuid_entries,
+					  vcpu->arch.cpuid_nent, sig);
 }
 
 static struct kvm_cpuid_entry2 *__kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcpu,
@@ -406,9 +414,12 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
                         int nent)
 {
 	int r;
+	struct kvm_hypervisor_cpuid kvm_cpuid;
 
 	__kvm_update_cpuid_runtime(vcpu, e2, nent);
-	kvm_update_pv_cpuid(vcpu, e2, nent);
+	kvm_cpuid = __kvm_get_hypervisor_cpuid(e2, nent, KVM_SIGNATURE);
+	if (kvm_cpuid.base)
+		kvm_update_pv_cpuid(vcpu, e2, nent);
 
 	/*
 	 * KVM does not correctly handle changing guest CPUID after KVM_RUN, as
@@ -444,7 +455,7 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
 	vcpu->arch.cpuid_entries = e2;
 	vcpu->arch.cpuid_nent = nent;
 
-	vcpu->arch.kvm_cpuid = kvm_get_hypervisor_cpuid(vcpu, KVM_SIGNATURE);
+	vcpu->arch.kvm_cpuid = kvm_cpuid;
 	vcpu->arch.xen.cpuid = kvm_get_hypervisor_cpuid(vcpu, XEN_SIGNATURE);
 	kvm_vcpu_after_set_cpuid(vcpu);
 
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] KVM: x86: Disallow enable KVM_CAP_X86_DISABLE_EXITS capability after vCPUs have been created
  2023-03-30 12:35 [PATCH 1/3] KVM: x86: Disallow enable KVM_CAP_X86_DISABLE_EXITS capability after vCPUs have been created Hou Wenlong
  2023-03-30 12:35 ` [PATCH 2/3] KVM: x86: Don't update KVM PV feature CPUID during vCPU running Hou Wenlong
  2023-03-30 12:35 ` [PATCH 3/3] KVM: x86: Detect KVM_CPUID_FEATURES base before KVM PV feature CPUID updating Hou Wenlong
@ 2023-04-06  3:18 ` Sean Christopherson
  2023-04-07  2:01 ` Xiaoyao Li
  3 siblings, 0 replies; 7+ messages in thread
From: Sean Christopherson @ 2023-04-06  3:18 UTC (permalink / raw)
  To: Hou Wenlong
  Cc: kvm, Paolo Bonzini, Jonathan Corbet, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-doc,
	linux-kernel

On Thu, Mar 30, 2023, Hou Wenlong wrote:
> Disable PAUSE/MWAIT/HLT exits after vCPUs have been created is useless,
> because PAUSE/MWAIT/HLT intercepts configuration is not changed after
> vCPU created.  And two vCPUs may have inconsistent configuration if
> disable PAUSE/MWAIT/HLT exits between those two vCPUs creation. Since
> it's a per-VM capability, all vCPUs should keep same configuration, so
> disallow enable KVM_CAP_X86_DISABLE_EXITS capability after vCPUs have
> been created.
> 
> Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> ---
>  Documentation/virt/kvm/api.rst | 3 ++-
>  arch/x86/kvm/x86.c             | 5 +++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index a69e91088d76..95a683a27cf2 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -7179,7 +7179,8 @@ 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
> +:Returns: 0 on success, -EINVAL when args[0] contains invalid exits or
> +	  any vCPUs have been created.
>  
>  Valid bits in args[0] are::
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2c0ff40e5345..7e97595465fc 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6275,6 +6275,9 @@ 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;
>  
> @@ -6296,6 +6299,8 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>  		}
>  
>  		r = 0;
> +disable_exits_unlock:

Hah!  I thought this looked familiar[*] :-)  

[*] https://lkml.kernel.org/r/DM6PR12MB35000D46146BA68EE294953BCACB9%40DM6PR12MB3500.namprd12.prod.outlook.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/3] KVM: x86: Don't update KVM PV feature CPUID during vCPU running
  2023-03-30 12:35 ` [PATCH 2/3] KVM: x86: Don't update KVM PV feature CPUID during vCPU running Hou Wenlong
@ 2023-04-06  3:29   ` Sean Christopherson
  2023-04-06 18:30     ` Kechen Lu
  0 siblings, 1 reply; 7+ messages in thread
From: Sean Christopherson @ 2023-04-06  3:29 UTC (permalink / raw)
  To: Hou Wenlong
  Cc: kvm, Paolo Bonzini, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H. Peter Anvin, linux-kernel, Kechen Lu

+Kechen

On Thu, Mar 30, 2023, Hou Wenlong wrote:
> __kvm_update_cpuid_runtime() may be called during vCPU running and KVM
> PV feature CPUID is updated too. But the cached KVM PV feature bitmap is
> not updated. Actually, KVM PV feature CPUID shouldn't be updated,
> otherwise, KVM PV feature would be broken in guest. Currently, only
> KVM_FEATURE_PV_UNHALT is updated, and it's impossible after disallow
> disable HLT exits. However, KVM PV feature CPUID should be updated only
> in KVM_SET_CPUID{,2} ioctl.
> 
> Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> ---
>  arch/x86/kvm/cpuid.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 6972e0be60fa..af92d3422c79 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -222,6 +222,17 @@ static struct kvm_cpuid_entry2 *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcp
>  					     vcpu->arch.cpuid_nent);
>  }
>  
> +static void kvm_update_pv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries,
> +				int nent)
> +{
> +	struct kvm_cpuid_entry2 *best;
> +
> +	best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
> +	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
> +		(best->eax & (1 << KVM_FEATURE_PV_UNHALT)))
> +		best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);
> +}
> +
>  void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_cpuid_entry2 *best = kvm_find_kvm_cpuid_features(vcpu);
> @@ -280,11 +291,6 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
>  		     cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
>  		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
>  
> -	best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
> -	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
> -		(best->eax & (1 << KVM_FEATURE_PV_UNHALT)))
> -		best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);
> -
>  	if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT)) {
>  		best = cpuid_entry2_find(entries, nent, 0x1, KVM_CPUID_INDEX_NOT_SIGNIFICANT);
>  		if (best)
> @@ -402,6 +408,7 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
>  	int r;
>  
>  	__kvm_update_cpuid_runtime(vcpu, e2, nent);
> +	kvm_update_pv_cpuid(vcpu, e2, nent);

Hrm, this will silently conflict with the proposed per-vCPU controls[*].  Though
arguably that patch is buggy and "needs" to toggle PV_UNHALT when userspace
messes with HLT passthrough.  But that doesn't really make sense either because
no guest will react kindly to KVM_FEATURE_PV_UNHALT disappearing.

I really wish this code didn't exist, i.e. that KVM let/forced userspace deal
with correctly defining guest CPUID.

Kechen, is it feasible for your userspace to clear PV_UNHALT when it (might) use
the per-vCPU control?  I.e. can KVM do as this series proposes and update guest
CPUID only on KVM_SET_CPUID{2}?  Dropping the behavior for the per-VM control
is probably not an option as I gotta assume that'd break userspace, but I would
really like to avoid carrying that over to the per-vCPU control, which would get
quite messy and probably can't work anyways.

[*] https://lkml.kernel.org/r/20230121020738.2973-6-kechenl%40nvidia.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH 2/3] KVM: x86: Don't update KVM PV feature CPUID during vCPU running
  2023-04-06  3:29   ` Sean Christopherson
@ 2023-04-06 18:30     ` Kechen Lu
  0 siblings, 0 replies; 7+ messages in thread
From: Kechen Lu @ 2023-04-06 18:30 UTC (permalink / raw)
  To: Sean Christopherson, Hou Wenlong
  Cc: kvm@vger.kernel.org, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Dave Hansen, x86@kernel.org, H. Peter Anvin,
	linux-kernel@vger.kernel.org

Hi Sean,

> -----Original Message-----
> From: Sean Christopherson <seanjc@google.com>
> Sent: Wednesday, April 5, 2023 8:29 PM
> To: Hou Wenlong <houwenlong.hwl@antgroup.com>
> Cc: kvm@vger.kernel.org; Paolo Bonzini <pbonzini@redhat.com>; Thomas
> Gleixner <tglx@linutronix.de>; Ingo Molnar <mingo@redhat.com>; Borislav
> Petkov <bp@alien8.de>; Dave Hansen <dave.hansen@linux.intel.com>;
> x86@kernel.org; H. Peter Anvin <hpa@zytor.com>; linux-
> kernel@vger.kernel.org; Kechen Lu <kechenl@nvidia.com>
> Subject: Re: [PATCH 2/3] KVM: x86: Don't update KVM PV feature CPUID
> during vCPU running
> 
> External email: Use caution opening links or attachments
> 
> 
> +Kechen
> 
> On Thu, Mar 30, 2023, Hou Wenlong wrote:
> > __kvm_update_cpuid_runtime() may be called during vCPU running and
> KVM
> > PV feature CPUID is updated too. But the cached KVM PV feature bitmap
> > is not updated. Actually, KVM PV feature CPUID shouldn't be updated,
> > otherwise, KVM PV feature would be broken in guest. Currently, only
> > KVM_FEATURE_PV_UNHALT is updated, and it's impossible after disallow
> > disable HLT exits. However, KVM PV feature CPUID should be updated
> > only in KVM_SET_CPUID{,2} ioctl.
> >
> > Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> > ---
> >  arch/x86/kvm/cpuid.c | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index
> > 6972e0be60fa..af92d3422c79 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -222,6 +222,17 @@ static struct kvm_cpuid_entry2
> *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcp
> >                                            vcpu->arch.cpuid_nent);  }
> >
> > +static void kvm_update_pv_cpuid(struct kvm_vcpu *vcpu, struct
> kvm_cpuid_entry2 *entries,
> > +                             int nent) {
> > +     struct kvm_cpuid_entry2 *best;
> > +
> > +     best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
> > +     if (kvm_hlt_in_guest(vcpu->kvm) && best &&
> > +             (best->eax & (1 << KVM_FEATURE_PV_UNHALT)))
> > +             best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT); }
> > +
> >  void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)  {
> >       struct kvm_cpuid_entry2 *best =
> > kvm_find_kvm_cpuid_features(vcpu);
> > @@ -280,11 +291,6 @@ static void __kvm_update_cpuid_runtime(struct
> kvm_vcpu *vcpu, struct kvm_cpuid_e
> >                    cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
> >               best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
> >
> > -     best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
> > -     if (kvm_hlt_in_guest(vcpu->kvm) && best &&
> > -             (best->eax & (1 << KVM_FEATURE_PV_UNHALT)))
> > -             best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);
> > -
> >       if (!kvm_check_has_quirk(vcpu->kvm,
> KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT)) {
> >               best = cpuid_entry2_find(entries, nent, 0x1,
> KVM_CPUID_INDEX_NOT_SIGNIFICANT);
> >               if (best)
> > @@ -402,6 +408,7 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu,
> struct kvm_cpuid_entry2 *e2,
> >       int r;
> >
> >       __kvm_update_cpuid_runtime(vcpu, e2, nent);
> > +     kvm_update_pv_cpuid(vcpu, e2, nent);
> 
> Hrm, this will silently conflict with the proposed per-vCPU controls[*].
> Though arguably that patch is buggy and "needs" to toggle PV_UNHALT
> when userspace messes with HLT passthrough.  But that doesn't really make
> sense either because no guest will react kindly to
> KVM_FEATURE_PV_UNHALT disappearing.

Yes agree, toggling PV_UNHALT with per-vCPU control also sounds not making 
sense to me. And as pv feature is per VM bases, if current per-vCPU control 
touches the pv feature toggling, that would probably cause a lot of messes.

> 
> I really wish this code didn't exist, i.e. that KVM let/forced userspace deal
> with correctly defining guest CPUID.
> 
> Kechen, is it feasible for your userspace to clear PV_UNHALT when it (might)
> use the per-vCPU control?  I.e. can KVM do as this series proposes and
> update guest CPUID only on KVM_SET_CPUID{2}?  Dropping the behavior for
> the per-VM control is probably not an option as I gotta assume that'd break
> userspace, but I would really like to avoid carrying that over to the per-vCPU
> control, which would get quite messy and probably can't work anyways.

Yes, in our use cases, it's feasible to clear PV_UNHALT while using the 
per-vCPU control. I think it makes sense on userspace responsibility to clear 
the PV_UNHALT bits while trying to use the per-vCPU control for hlt passthrough.
We may add notes/requirement after this line of doc 
Documentation/virt/kvm/api.rst:
"Do not enable KVM_FEATURE_PV_UNHALT if you disable HLT exits."

Best Regards,
Kechen

> 
> [*] https://lkml.kernel.org/r/20230121020738.2973-6-kechenl%40nvidia.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] KVM: x86: Disallow enable KVM_CAP_X86_DISABLE_EXITS capability after vCPUs have been created
  2023-03-30 12:35 [PATCH 1/3] KVM: x86: Disallow enable KVM_CAP_X86_DISABLE_EXITS capability after vCPUs have been created Hou Wenlong
                   ` (2 preceding siblings ...)
  2023-04-06  3:18 ` [PATCH 1/3] KVM: x86: Disallow enable KVM_CAP_X86_DISABLE_EXITS capability after vCPUs have been created Sean Christopherson
@ 2023-04-07  2:01 ` Xiaoyao Li
  3 siblings, 0 replies; 7+ messages in thread
From: Xiaoyao Li @ 2023-04-07  2:01 UTC (permalink / raw)
  To: Hou Wenlong, kvm
  Cc: Paolo Bonzini, Jonathan Corbet, Sean Christopherson,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H. Peter Anvin, linux-doc, linux-kernel

On 3/30/2023 8:35 PM, Hou Wenlong wrote:
> Disable PAUSE/MWAIT/HLT exits after vCPUs have been created is useless,
> because PAUSE/MWAIT/HLT intercepts configuration is not changed after
> vCPU created.  

PAUSE intercepts can be updated via 
SET_CPUID->vmx_vcpu_after_set_cpuid() after vCPU is created.

Aside it, this patch looks good to me.

> And two vCPUs may have inconsistent configuration if
> disable PAUSE/MWAIT/HLT exits between those two vCPUs creation. Since
> it's a per-VM capability, all vCPUs should keep same configuration, so
> disallow enable KVM_CAP_X86_DISABLE_EXITS capability after vCPUs have
> been created.
> 
> Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> ---
>   Documentation/virt/kvm/api.rst | 3 ++-
>   arch/x86/kvm/x86.c             | 5 +++++
>   2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index a69e91088d76..95a683a27cf2 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -7179,7 +7179,8 @@ 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
> +:Returns: 0 on success, -EINVAL when args[0] contains invalid exits or
> +	  any vCPUs have been created.
>   
>   Valid bits in args[0] are::
>   
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2c0ff40e5345..7e97595465fc 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6275,6 +6275,9 @@ 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;
>   
> @@ -6296,6 +6299,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];
> 
> base-commit: 27d6845d258b67f4eb3debe062b7dacc67e0c393


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-04-07  2:06 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-30 12:35 [PATCH 1/3] KVM: x86: Disallow enable KVM_CAP_X86_DISABLE_EXITS capability after vCPUs have been created Hou Wenlong
2023-03-30 12:35 ` [PATCH 2/3] KVM: x86: Don't update KVM PV feature CPUID during vCPU running Hou Wenlong
2023-04-06  3:29   ` Sean Christopherson
2023-04-06 18:30     ` Kechen Lu
2023-03-30 12:35 ` [PATCH 3/3] KVM: x86: Detect KVM_CPUID_FEATURES base before KVM PV feature CPUID updating Hou Wenlong
2023-04-06  3:18 ` [PATCH 1/3] KVM: x86: Disallow enable KVM_CAP_X86_DISABLE_EXITS capability after vCPUs have been created Sean Christopherson
2023-04-07  2:01 ` Xiaoyao Li

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).