kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/3] KVM: x86: Disallow writes to feature MSRs post-KVM_RUN
@ 2022-08-05 17:29 Sean Christopherson
  2022-08-05 17:29 ` [RFC PATCH 1/3] KVM: x86: Add macros to track first...last VMX feature MSRs Sean Christopherson
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Sean Christopherson @ 2022-08-05 17:29 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Like Xu

Give feature MSRs that same treatment as CPUID and disallow changing said
MSRs after KVM_RUN.

RFC as this is lightly tested and should come with a selftests update to
verify it actually works.  Posting early to get feedback on the overall
idea, and on the VMX MSRs trickery (though I think patches 1-2 are a good
idea irrespective of trying to reduce the overhead of the new check).

Sean Christopherson (3):
  KVM: x86: Add macros to track first...last VMX feature MSRs
  KVM: x86: Generate set of VMX feature MSRs using first/last
    definitions
  KVM: x86: Disallow writes to immutable feature MSRs after KVM_RUN

 arch/x86/kvm/svm/svm.c |  2 +-
 arch/x86/kvm/vmx/vmx.c |  8 ++--
 arch/x86/kvm/x86.c     | 90 +++++++++++++++++++++++++++---------------
 arch/x86/kvm/x86.h     |  8 ++++
 4 files changed, 72 insertions(+), 36 deletions(-)


base-commit: 93472b79715378a2386598d6632c654a2223267b
-- 
2.37.1.559.g78731f0fdb-goog


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

* [RFC PATCH 1/3] KVM: x86: Add macros to track first...last VMX feature MSRs
  2022-08-05 17:29 [RFC PATCH 0/3] KVM: x86: Disallow writes to feature MSRs post-KVM_RUN Sean Christopherson
@ 2022-08-05 17:29 ` Sean Christopherson
  2022-08-05 17:29 ` [RFC PATCH 2/3] KVM: x86: Generate set of VMX feature MSRs using first/last definitions Sean Christopherson
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2022-08-05 17:29 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Like Xu

Add macros to track the range of VMX feature MSRs that are emulated by
KVM to reduce the maintenance cost of extending the set of emulated MSRs.

Note, KVM doesn't necessarily emulate all known/consumed VMX MSRs, e.g.
PROCBASED_CTLS3 is consumed by KVM to enable IPI virtualization, but is
not emulated as KVM doesn't emulate/virtualize IPI virtualization for
nested guests.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c | 2 +-
 arch/x86/kvm/vmx/vmx.c | 8 ++++----
 arch/x86/kvm/x86.h     | 8 ++++++++
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 38f873cb6f2c..0d7ad9a55a33 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4147,7 +4147,7 @@ static bool svm_has_emulated_msr(struct kvm *kvm, u32 index)
 {
 	switch (index) {
 	case MSR_IA32_MCG_EXT_CTL:
-	case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
+	case KVM_FIRST_EMULATED_VMX_MSR ... KVM_LAST_EMULATED_VMX_MSR:
 		return false;
 	case MSR_IA32_SMBASE:
 		/* SEV-ES guests do not support SMM, so report false */
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index d7f8331d6f7e..36732339c5f7 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1830,7 +1830,7 @@ static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu,
 static int vmx_get_msr_feature(struct kvm_msr_entry *msr)
 {
 	switch (msr->index) {
-	case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
+	case KVM_FIRST_EMULATED_VMX_MSR ... KVM_LAST_EMULATED_VMX_MSR:
 		if (!nested)
 			return 1;
 		return vmx_get_vmx_msr(&vmcs_config.nested, msr->index, &msr->data);
@@ -1918,7 +1918,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		msr_info->data = to_vmx(vcpu)->msr_ia32_sgxlepubkeyhash
 			[msr_info->index - MSR_IA32_SGXLEPUBKEYHASH0];
 		break;
-	case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
+	case KVM_FIRST_EMULATED_VMX_MSR ... KVM_LAST_EMULATED_VMX_MSR:
 		if (!nested_vmx_allowed(vcpu))
 			return 1;
 		if (vmx_get_vmx_msr(&vmx->nested.msrs, msr_info->index,
@@ -2254,7 +2254,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		vmx->msr_ia32_sgxlepubkeyhash
 			[msr_index - MSR_IA32_SGXLEPUBKEYHASH0] = data;
 		break;
-	case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
+	case KVM_FIRST_EMULATED_VMX_MSR ... KVM_LAST_EMULATED_VMX_MSR:
 		if (!msr_info->host_initiated)
 			return 1; /* they are read-only */
 		if (!nested_vmx_allowed(vcpu))
@@ -6854,7 +6854,7 @@ static bool vmx_has_emulated_msr(struct kvm *kvm, u32 index)
 		 * real mode.
 		 */
 		return enable_unrestricted_guest || emulate_invalid_guest_state;
-	case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
+	case KVM_FIRST_EMULATED_VMX_MSR ... KVM_LAST_EMULATED_VMX_MSR:
 		return nested;
 	case MSR_AMD64_VIRT_SPEC_CTRL:
 	case MSR_AMD64_TSC_RATIO:
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 1926d2cb8e79..ae151aea17c5 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -39,6 +39,14 @@ void kvm_spurious_fault(void);
 	failed;								\
 })
 
+/*
+ * The first...last VMX feature MSRs that are emulated by KVM.  This may or may
+ * not cover all known VMX MSRs, as KVM doesn't emulate an MSR until there's an
+ * associated feature that KVM supports for nested virtualization.
+ */
+#define KVM_FIRST_EMULATED_VMX_MSR	MSR_IA32_VMX_BASIC
+#define KVM_LAST_EMULATED_VMX_MSR	MSR_IA32_VMX_VMFUNC
+
 #define KVM_DEFAULT_PLE_GAP		128
 #define KVM_VMX_DEFAULT_PLE_WINDOW	4096
 #define KVM_DEFAULT_PLE_WINDOW_GROW	2
-- 
2.37.1.559.g78731f0fdb-goog


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

* [RFC PATCH 2/3] KVM: x86: Generate set of VMX feature MSRs using first/last definitions
  2022-08-05 17:29 [RFC PATCH 0/3] KVM: x86: Disallow writes to feature MSRs post-KVM_RUN Sean Christopherson
  2022-08-05 17:29 ` [RFC PATCH 1/3] KVM: x86: Add macros to track first...last VMX feature MSRs Sean Christopherson
@ 2022-08-05 17:29 ` Sean Christopherson
  2022-08-10 12:52   ` Paolo Bonzini
  2022-08-05 17:29 ` [RFC PATCH 3/3] KVM: x86: Disallow writes to immutable feature MSRs after KVM_RUN Sean Christopherson
  2022-08-10 12:52 ` [RFC PATCH 0/3] KVM: x86: Disallow writes to feature MSRs post-KVM_RUN Paolo Bonzini
  3 siblings, 1 reply; 10+ messages in thread
From: Sean Christopherson @ 2022-08-05 17:29 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Like Xu

Add VMX MSRs to the runtime list of feature MSRs by iterating over the
range of emulated MSRs instead of manually defining each MSR in the "all"
list.  Using the range definition reduces the cost of emulating a new VMX
MSR, e.g. prevents forgetting to add an MSR to the list.

Extracting the VMX MSRs from the "all" list, which is a compile-time
constant, also shrinks the list to the point where the compiler can
heavily optimize code that iterates over the list.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 53 +++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 31 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 33560bfa0cac..a1c65b77fb16 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1526,36 +1526,19 @@ static u32 emulated_msrs[ARRAY_SIZE(emulated_msrs_all)];
 static unsigned num_emulated_msrs;
 
 /*
- * List of msr numbers which are used to expose MSR-based features that
- * can be used by a hypervisor to validate requested CPU features.
+ * List of MSRs that control the existence of MSR-based features, i.e. MSRs
+ * that are effectively CPUID leafs.  VMX MSRs are also included in the set of
+ * feature MSRs, but are handled separately to allow expedited lookups.
  */
-static const u32 msr_based_features_all[] = {
-	MSR_IA32_VMX_BASIC,
-	MSR_IA32_VMX_TRUE_PINBASED_CTLS,
-	MSR_IA32_VMX_PINBASED_CTLS,
-	MSR_IA32_VMX_TRUE_PROCBASED_CTLS,
-	MSR_IA32_VMX_PROCBASED_CTLS,
-	MSR_IA32_VMX_TRUE_EXIT_CTLS,
-	MSR_IA32_VMX_EXIT_CTLS,
-	MSR_IA32_VMX_TRUE_ENTRY_CTLS,
-	MSR_IA32_VMX_ENTRY_CTLS,
-	MSR_IA32_VMX_MISC,
-	MSR_IA32_VMX_CR0_FIXED0,
-	MSR_IA32_VMX_CR0_FIXED1,
-	MSR_IA32_VMX_CR4_FIXED0,
-	MSR_IA32_VMX_CR4_FIXED1,
-	MSR_IA32_VMX_VMCS_ENUM,
-	MSR_IA32_VMX_PROCBASED_CTLS2,
-	MSR_IA32_VMX_EPT_VPID_CAP,
-	MSR_IA32_VMX_VMFUNC,
-
+static const u32 msr_based_features_all_except_vmx[] = {
 	MSR_F10H_DECFG,
 	MSR_IA32_UCODE_REV,
 	MSR_IA32_ARCH_CAPABILITIES,
 	MSR_IA32_PERF_CAPABILITIES,
 };
 
-static u32 msr_based_features[ARRAY_SIZE(msr_based_features_all)];
+static u32 msr_based_features[ARRAY_SIZE(msr_based_features_all_except_vmx) +
+			      (KVM_LAST_EMULATED_VMX_MSR - KVM_FIRST_EMULATED_VMX_MSR + 1)];
 static unsigned int num_msr_based_features;
 
 static u64 kvm_get_arch_capabilities(void)
@@ -6868,6 +6851,18 @@ long kvm_arch_vm_ioctl(struct file *filp,
 	return r;
 }
 
+static void kvm_proble_feature_msr(u32 msr_index)
+{
+	struct kvm_msr_entry msr = {
+		.index = msr_index,
+	};
+
+	if (kvm_get_msr_feature(&msr))
+		return;
+
+	msr_based_features[num_msr_based_features++] = msr_index;
+}
+
 static void kvm_init_msr_list(void)
 {
 	u32 dummy[2];
@@ -6954,15 +6949,11 @@ static void kvm_init_msr_list(void)
 		emulated_msrs[num_emulated_msrs++] = emulated_msrs_all[i];
 	}
 
-	for (i = 0; i < ARRAY_SIZE(msr_based_features_all); i++) {
-		struct kvm_msr_entry msr;
+	for (i = KVM_FIRST_EMULATED_VMX_MSR; i <= KVM_LAST_EMULATED_VMX_MSR; i++)
+		kvm_proble_feature_msr(i);
 
-		msr.index = msr_based_features_all[i];
-		if (kvm_get_msr_feature(&msr))
-			continue;
-
-		msr_based_features[num_msr_based_features++] = msr_based_features_all[i];
-	}
+	for (i = 0; i < ARRAY_SIZE(msr_based_features_all_except_vmx); i++)
+		kvm_proble_feature_msr(msr_based_features_all_except_vmx[i]);
 }
 
 static int vcpu_mmio_write(struct kvm_vcpu *vcpu, gpa_t addr, int len,
-- 
2.37.1.559.g78731f0fdb-goog


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

* [RFC PATCH 3/3] KVM: x86: Disallow writes to immutable feature MSRs after KVM_RUN
  2022-08-05 17:29 [RFC PATCH 0/3] KVM: x86: Disallow writes to feature MSRs post-KVM_RUN Sean Christopherson
  2022-08-05 17:29 ` [RFC PATCH 1/3] KVM: x86: Add macros to track first...last VMX feature MSRs Sean Christopherson
  2022-08-05 17:29 ` [RFC PATCH 2/3] KVM: x86: Generate set of VMX feature MSRs using first/last definitions Sean Christopherson
@ 2022-08-05 17:29 ` Sean Christopherson
  2022-08-10  6:29   ` Xiaoyao Li
  2022-08-10 12:52 ` [RFC PATCH 0/3] KVM: x86: Disallow writes to feature MSRs post-KVM_RUN Paolo Bonzini
  3 siblings, 1 reply; 10+ messages in thread
From: Sean Christopherson @ 2022-08-05 17:29 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Like Xu

Disallow writes to feature MSRs after KVM_RUN to prevent userspace from
changing the vCPU model after running the vCPU.  Similar to guest CPUID,
KVM uses feature MSRs to configure intercepts, determine what operations
are/aren't allowed, etc.  Changing the capabilities while the vCPU is
active will at best yield unpredictable guest behavior, and at worst
could be dangerous to KVM.

Allow writing the current value, e.g. so that userspace can blindly set
all MSRs when emulating RESET, and unconditionally allow writes to
MSR_IA32_UCODE_REV so that userspace can emulate patch loads.

Special case the VMX MSRs to keep the generic list small, i.e. so that
KVM can do a linear walk of the generic list without incurring meaningful
overhead.

Cc: Like Xu <like.xu.linux@gmail.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a1c65b77fb16..4da26a1f14c1 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1541,6 +1541,26 @@ static u32 msr_based_features[ARRAY_SIZE(msr_based_features_all_except_vmx) +
 			      (KVM_LAST_EMULATED_VMX_MSR - KVM_FIRST_EMULATED_VMX_MSR + 1)];
 static unsigned int num_msr_based_features;
 
+/*
+ * All feature MSRs except uCode revID, which tracks the currently loaded uCode
+ * patch, are immutable once the vCPU model is defined.
+ */
+static bool kvm_is_immutable_feature_msr(u32 msr)
+{
+	int i;
+
+	if (msr >= KVM_FIRST_EMULATED_VMX_MSR && msr <= KVM_LAST_EMULATED_VMX_MSR)
+		return true;
+
+	for (i = 0; i < ARRAY_SIZE(msr_based_features_all_except_vmx); i++) {
+		if (msr == msr_based_features_all_except_vmx[i])
+			return msr != MSR_IA32_UCODE_REV;
+	}
+
+	return false;
+}
+
+
 static u64 kvm_get_arch_capabilities(void)
 {
 	u64 data = 0;
@@ -2136,6 +2156,23 @@ static int do_get_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
 
 static int do_set_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
 {
+	u64 val;
+
+	/*
+	 * Disallow writes to immutable feature MSRs after KVM_RUN.  KVM does
+	 * not support modifying the guest vCPU model on the fly, e.g. changing
+	 * the nVMX capabilities while L2 is running is nonsensical.  Ignore
+	 * writes of the same value, e.g. to allow userspace to blindly stuff
+	 * all MSRs when emulating RESET.
+	 */
+	if (vcpu->arch.last_vmentry_cpu != -1 &&
+	    kvm_is_immutable_feature_msr(index)) {
+		if (do_get_msr(vcpu, index, &val) || *data != val)
+			return -EINVAL;
+
+		return 0;
+	}
+
 	return kvm_set_msr_ignored_check(vcpu, index, *data, true);
 }
 
-- 
2.37.1.559.g78731f0fdb-goog


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

* Re: [RFC PATCH 3/3] KVM: x86: Disallow writes to immutable feature MSRs after KVM_RUN
  2022-08-05 17:29 ` [RFC PATCH 3/3] KVM: x86: Disallow writes to immutable feature MSRs after KVM_RUN Sean Christopherson
@ 2022-08-10  6:29   ` Xiaoyao Li
  2022-08-10 14:45     ` Sean Christopherson
  0 siblings, 1 reply; 10+ messages in thread
From: Xiaoyao Li @ 2022-08-10  6:29 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Like Xu

On 8/6/2022 1:29 AM, Sean Christopherson wrote:
> Disallow writes to feature MSRs after KVM_RUN to prevent userspace from
> changing the vCPU model after running the vCPU.  Similar to guest CPUID,
> KVM uses feature MSRs to configure intercepts, determine what operations
> are/aren't allowed, etc.  Changing the capabilities while the vCPU is
> active will at best yield unpredictable guest behavior, and at worst
> could be dangerous to KVM.
> 
> Allow writing the current value, e.g. so that userspace can blindly set
> all MSRs when emulating RESET, and unconditionally allow writes to
> MSR_IA32_UCODE_REV so that userspace can emulate patch loads.
> 
> Special case the VMX MSRs to keep the generic list small, i.e. so that
> KVM can do a linear walk of the generic list without incurring meaningful
> overhead.
> 
> Cc: Like Xu <like.xu.linux@gmail.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/x86.c | 37 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 37 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a1c65b77fb16..4da26a1f14c1 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1541,6 +1541,26 @@ static u32 msr_based_features[ARRAY_SIZE(msr_based_features_all_except_vmx) +
>   			      (KVM_LAST_EMULATED_VMX_MSR - KVM_FIRST_EMULATED_VMX_MSR + 1)];
>   static unsigned int num_msr_based_features;
>   
> +/*
> + * All feature MSRs except uCode revID, which tracks the currently loaded uCode
> + * patch, are immutable once the vCPU model is defined.
> + */
> +static bool kvm_is_immutable_feature_msr(u32 msr)
> +{
> +	int i;
> +
> +	if (msr >= KVM_FIRST_EMULATED_VMX_MSR && msr <= KVM_LAST_EMULATED_VMX_MSR)
> +		return true;
> +
> +	for (i = 0; i < ARRAY_SIZE(msr_based_features_all_except_vmx); i++) {
> +		if (msr == msr_based_features_all_except_vmx[i])
> +			return msr != MSR_IA32_UCODE_REV;
> +	}
> +
> +	return false;
> +}
> +
> +
>   static u64 kvm_get_arch_capabilities(void)
>   {
>   	u64 data = 0;
> @@ -2136,6 +2156,23 @@ static int do_get_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
>   
>   static int do_set_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
>   {
> +	u64 val;
> +
> +	/*
> +	 * Disallow writes to immutable feature MSRs after KVM_RUN.  KVM does
> +	 * not support modifying the guest vCPU model on the fly, e.g. changing
> +	 * the nVMX capabilities while L2 is running is nonsensical.  Ignore
> +	 * writes of the same value, e.g. to allow userspace to blindly stuff
> +	 * all MSRs when emulating RESET.
> +	 */
> +	if (vcpu->arch.last_vmentry_cpu != -1 &&

can we extract "vcpu->arch.last_vmentry_cpu != -1" into a function like 
kvm_vcpu_has_runned() ?

> +	    kvm_is_immutable_feature_msr(index)) {
> +		if (do_get_msr(vcpu, index, &val) || *data != val)
> +			return -EINVAL;
> +
> +		return 0;
> +	}
> +
>   	return kvm_set_msr_ignored_check(vcpu, index, *data, true);
>   }
>   


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

* Re: [RFC PATCH 2/3] KVM: x86: Generate set of VMX feature MSRs using first/last definitions
  2022-08-05 17:29 ` [RFC PATCH 2/3] KVM: x86: Generate set of VMX feature MSRs using first/last definitions Sean Christopherson
@ 2022-08-10 12:52   ` Paolo Bonzini
  2022-08-10 14:40     ` Sean Christopherson
  0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2022-08-10 12:52 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm, linux-kernel, Like Xu

On 8/5/22 19:29, Sean Christopherson wrote:
>   
> +static void kvm_proble_feature_msr(u32 msr_index)
> +{
> +	struct kvm_msr_entry msr = {
> +		.index = msr_index,
> +	};
> +
> +	if (kvm_get_msr_feature(&msr))
> +		return;
> +
> +	msr_based_features[num_msr_based_features++] = msr_index;
> +}
> +
>   static void kvm_init_msr_list(void)
>   {
>   	u32 dummy[2];
> @@ -6954,15 +6949,11 @@ static void kvm_init_msr_list(void)
>   		emulated_msrs[num_emulated_msrs++] = emulated_msrs_all[i];
>   	}
>   
> -	for (i = 0; i < ARRAY_SIZE(msr_based_features_all); i++) {
> -		struct kvm_msr_entry msr;
> +	for (i = KVM_FIRST_EMULATED_VMX_MSR; i <= KVM_LAST_EMULATED_VMX_MSR; i++)
> +		kvm_proble_feature_msr(i);
>   
> -		msr.index = msr_based_features_all[i];
> -		if (kvm_get_msr_feature(&msr))
> -			continue;
> -
> -		msr_based_features[num_msr_based_features++] = msr_based_features_all[i];
> -	}
> +	for (i = 0; i < ARRAY_SIZE(msr_based_features_all_except_vmx); i++)
> +		kvm_proble_feature_msr(msr_based_features_all_except_vmx[i]);

I'd rather move all the code to a new function 
kvm_init_feature_msr_list() instead, and call it from 
kvm_arch_hardware_setup().

Thanks,

paolo


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

* Re: [RFC PATCH 0/3] KVM: x86: Disallow writes to feature MSRs post-KVM_RUN
  2022-08-05 17:29 [RFC PATCH 0/3] KVM: x86: Disallow writes to feature MSRs post-KVM_RUN Sean Christopherson
                   ` (2 preceding siblings ...)
  2022-08-05 17:29 ` [RFC PATCH 3/3] KVM: x86: Disallow writes to immutable feature MSRs after KVM_RUN Sean Christopherson
@ 2022-08-10 12:52 ` Paolo Bonzini
  3 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2022-08-10 12:52 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm, linux-kernel, Like Xu

On 8/5/22 19:29, Sean Christopherson wrote:
> Give feature MSRs that same treatment as CPUID and disallow changing said
> MSRs after KVM_RUN.
> 
> RFC as this is lightly tested and should come with a selftests update to
> verify it actually works.  Posting early to get feedback on the overall
> idea, and on the VMX MSRs trickery (though I think patches 1-2 are a good
> idea irrespective of trying to reduce the overhead of the new check).

They are good, just a small style remark on patch 2.

Paolo


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

* Re: [RFC PATCH 2/3] KVM: x86: Generate set of VMX feature MSRs using first/last definitions
  2022-08-10 12:52   ` Paolo Bonzini
@ 2022-08-10 14:40     ` Sean Christopherson
  2022-08-10 19:16       ` Paolo Bonzini
  0 siblings, 1 reply; 10+ messages in thread
From: Sean Christopherson @ 2022-08-10 14:40 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: kvm, linux-kernel, Like Xu

On Wed, Aug 10, 2022, Paolo Bonzini wrote:
> On 8/5/22 19:29, Sean Christopherson wrote:
> > +static void kvm_proble_feature_msr(u32 msr_index)
> > +{
> > +	struct kvm_msr_entry msr = {
> > +		.index = msr_index,
> > +	};
> > +
> > +	if (kvm_get_msr_feature(&msr))
> > +		return;
> > +
> > +	msr_based_features[num_msr_based_features++] = msr_index;
> > +}
> > +
> >   static void kvm_init_msr_list(void)
> >   {
> >   	u32 dummy[2];
> > @@ -6954,15 +6949,11 @@ static void kvm_init_msr_list(void)
> >   		emulated_msrs[num_emulated_msrs++] = emulated_msrs_all[i];
> >   	}
> > -	for (i = 0; i < ARRAY_SIZE(msr_based_features_all); i++) {
> > -		struct kvm_msr_entry msr;
> > +	for (i = KVM_FIRST_EMULATED_VMX_MSR; i <= KVM_LAST_EMULATED_VMX_MSR; i++)
> > +		kvm_proble_feature_msr(i);
> > -		msr.index = msr_based_features_all[i];
> > -		if (kvm_get_msr_feature(&msr))
> > -			continue;
> > -
> > -		msr_based_features[num_msr_based_features++] = msr_based_features_all[i];
> > -	}
> > +	for (i = 0; i < ARRAY_SIZE(msr_based_features_all_except_vmx); i++)
> > +		kvm_proble_feature_msr(msr_based_features_all_except_vmx[i]);
> 
> I'd rather move all the code to a new function kvm_init_feature_msr_list()
> instead, and call it from kvm_arch_hardware_setup().

Would it make sense to also split out kvm_init_emulated_msr_list()?  Hmm, and
rename this to kvm_init_virtualized_msr_list()?  I can't tell if that would be
helpful or confusing.

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

* Re: [RFC PATCH 3/3] KVM: x86: Disallow writes to immutable feature MSRs after KVM_RUN
  2022-08-10  6:29   ` Xiaoyao Li
@ 2022-08-10 14:45     ` Sean Christopherson
  0 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2022-08-10 14:45 UTC (permalink / raw)
  To: Xiaoyao Li; +Cc: Paolo Bonzini, kvm, linux-kernel, Like Xu

On Wed, Aug 10, 2022, Xiaoyao Li wrote:
> On 8/6/2022 1:29 AM, Sean Christopherson wrote:
> > @@ -2136,6 +2156,23 @@ static int do_get_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
> >   static int do_set_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
> >   {
> > +	u64 val;
> > +
> > +	/*
> > +	 * Disallow writes to immutable feature MSRs after KVM_RUN.  KVM does
> > +	 * not support modifying the guest vCPU model on the fly, e.g. changing
> > +	 * the nVMX capabilities while L2 is running is nonsensical.  Ignore
> > +	 * writes of the same value, e.g. to allow userspace to blindly stuff
> > +	 * all MSRs when emulating RESET.
> > +	 */
> > +	if (vcpu->arch.last_vmentry_cpu != -1 &&
> 
> can we extract "vcpu->arch.last_vmentry_cpu != -1" into a function like
> kvm_vcpu_has_runned() ?

Ya, a helper is in order.  I'll add a patch in the next version.

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

* Re: [RFC PATCH 2/3] KVM: x86: Generate set of VMX feature MSRs using first/last definitions
  2022-08-10 14:40     ` Sean Christopherson
@ 2022-08-10 19:16       ` Paolo Bonzini
  0 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2022-08-10 19:16 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm, linux-kernel, Like Xu

On 8/10/22 16:40, Sean Christopherson wrote:
>> I'd rather move all the code to a new function kvm_init_feature_msr_list()
>> instead, and call it from kvm_arch_hardware_setup().
> 
> Would it make sense to also split out kvm_init_emulated_msr_list()?  Hmm, and
> rename this to kvm_init_virtualized_msr_list()?  I can't tell if that would be
> helpful or confusing.

I thought of feature MSRs because it's a different ioctl altogether, but 
this is not an objection; whatever seems less confusing to you.

Paolo


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

end of thread, other threads:[~2022-08-10 19:16 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-05 17:29 [RFC PATCH 0/3] KVM: x86: Disallow writes to feature MSRs post-KVM_RUN Sean Christopherson
2022-08-05 17:29 ` [RFC PATCH 1/3] KVM: x86: Add macros to track first...last VMX feature MSRs Sean Christopherson
2022-08-05 17:29 ` [RFC PATCH 2/3] KVM: x86: Generate set of VMX feature MSRs using first/last definitions Sean Christopherson
2022-08-10 12:52   ` Paolo Bonzini
2022-08-10 14:40     ` Sean Christopherson
2022-08-10 19:16       ` Paolo Bonzini
2022-08-05 17:29 ` [RFC PATCH 3/3] KVM: x86: Disallow writes to immutable feature MSRs after KVM_RUN Sean Christopherson
2022-08-10  6:29   ` Xiaoyao Li
2022-08-10 14:45     ` Sean Christopherson
2022-08-10 12:52 ` [RFC PATCH 0/3] KVM: x86: Disallow writes to feature MSRs post-KVM_RUN Paolo Bonzini

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