The 1st patch is to fix a comment.
The 2nd patch is to avoid running into many vmx_enable_intercept_for_msr() or
vmx_disable_intercept_for_msr() that may return immediately, when
vmx_msr_bitmap is not supported.
The 3rd patch is to simplify vmx_enable_intercept_for_msr() and
vmx_disable_intercept_for_msr(), to avoid calling
possible_passthrough_msr_slot() twice.
Thank you very much!
Dongli Zhang (3):
KVM: VMX: fix comment to add LBR to passthrough MSRs
KVM: VMX: return early if msr_bitmap is not supported
KVM: VMX: simplify MSR interception enable/disable
arch/x86/kvm/vmx/vmx.c | 60 ++++++++++++++++++++++++---------------------
1 file changed, 32 insertions(+), 28 deletions(-)
base-commit: 7455665a3521aa7b56245c0a2810f748adc5fdd4
The vmx_msr_filter_changed() may directly/indirectly calls only
vmx_enable_intercept_for_msr() or vmx_disable_intercept_for_msr(). Those
two functions may exit immediately if !cpu_has_vmx_msr_bitmap().
vmx_msr_filter_changed()
-> vmx_disable_intercept_for_msr()
-> pt_update_intercept_for_msr()
-> vmx_set_intercept_for_msr()
-> vmx_enable_intercept_for_msr()
-> vmx_disable_intercept_for_msr()
Therefore, we exit early if !cpu_has_vmx_msr_bitmap().
Signed-off-by: Dongli Zhang <[email protected]>
---
arch/x86/kvm/vmx/vmx.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 05319b74bd3d..5a866d3c2bc8 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4127,6 +4127,9 @@ static void vmx_msr_filter_changed(struct kvm_vcpu *vcpu)
struct vcpu_vmx *vmx = to_vmx(vcpu);
u32 i;
+ if (!cpu_has_vmx_msr_bitmap())
+ return;
+
/*
* Redo intercept permissions for MSRs that KVM is passing through to
* the guest. Disabling interception will check the new MSR filter and
--
2.34.1
According to the is_valid_passthrough_msr(), the LBR MSRs are also
passthrough MSRs, since the commit 1b5ac3226a1a ("KVM: vmx/pmu:
Pass-through LBR msrs when the guest LBR event is ACTIVE").
Signed-off-by: Dongli Zhang <[email protected]>
---
arch/x86/kvm/vmx/vmx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index d4e6625e0a9a..05319b74bd3d 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -159,7 +159,7 @@ module_param(allow_smaller_maxphyaddr, bool, S_IRUGO);
/*
* List of MSRs that can be directly passed to the guest.
- * In addition to these x2apic and PT MSRs are handled specially.
+ * In addition to these x2apic, PT and LBR MSRs are handled specially.
*/
static u32 vmx_possible_passthrough_msrs[MAX_POSSIBLE_PASSTHROUGH_MSRS] = {
MSR_IA32_SPEC_CTRL,
--
2.34.1
Currently, the is_valid_passthrough_msr() is only called by two sites:
vmx_enable_intercept_for_msr() and vmx_disable_intercept_for_msr(). The
is_valid_passthrough_msr() is called for two reasons.
1. Do WARN() if the input msr is neither x2APIC/PT/LBR passthrough MSRs,
nor the possible passthrough MSRs.
2. Return if the msr is a possible passthrough MSR.
While the is_valid_passthrough_msr() may traverse the
vmx_possible_passthrough_msrs[], the following
possible_passthrough_msr_slot() may traverse the save array again. There
is no need to call possible_passthrough_msr_slot() twice.
vmx_disable_intercept_for_msr()
-> is_valid_passthrough_msr()
-> possible_passthrough_msr_slot()
-> possible_passthrough_msr_slot()
Therefore, we merge the is_valid_passthrough_msr() and the following
possible_passthrough_msr_slot() into the same function:
- If the msr is not any passthrough MSR, WARN and return -ENOENT.
- Return VMX_OTHER_PASSTHROUGH if x2apic/PT/LBR.
- Return VMX_POSSIBLE_PASSTHROUGH and set possible_idx, if possible
passthrough MSRs.
Signed-off-by: Dongli Zhang <[email protected]>
---
arch/x86/kvm/vmx/vmx.c | 55 +++++++++++++++++++++---------------------
1 file changed, 28 insertions(+), 27 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 5a866d3c2bc8..76dff0e7d8bd 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -669,14 +669,18 @@ static int possible_passthrough_msr_slot(u32 msr)
return -ENOENT;
}
-static bool is_valid_passthrough_msr(u32 msr)
+#define VMX_POSSIBLE_PASSTHROUGH 1
+#define VMX_OTHER_PASSTHROUGH 2
+/*
+ * Vefify if the msr is the passthrough MSRs.
+ * Return the index in *possible_idx if it is a possible passthrough MSR.
+ */
+static int validate_passthrough_msr(u32 msr, int *possible_idx)
{
- bool r;
-
switch (msr) {
case 0x800 ... 0x8ff:
/* x2APIC MSRs. These are handled in vmx_update_msr_bitmap_x2apic() */
- return true;
+ return VMX_OTHER_PASSTHROUGH;
case MSR_IA32_RTIT_STATUS:
case MSR_IA32_RTIT_OUTPUT_BASE:
case MSR_IA32_RTIT_OUTPUT_MASK:
@@ -691,14 +695,17 @@ static bool is_valid_passthrough_msr(u32 msr)
case MSR_LBR_CORE_FROM ... MSR_LBR_CORE_FROM + 8:
case MSR_LBR_CORE_TO ... MSR_LBR_CORE_TO + 8:
/* LBR MSRs. These are handled in vmx_update_intercept_for_lbr_msrs() */
- return true;
+ return VMX_OTHER_PASSTHROUGH;
}
- r = possible_passthrough_msr_slot(msr) != -ENOENT;
+ *possible_idx = possible_passthrough_msr_slot(msr);
+ WARN(*possible_idx == -ENOENT,
+ "Invalid MSR %x, please adapt vmx_possible_passthrough_msrs[]", msr);
- WARN(!r, "Invalid MSR %x, please adapt vmx_possible_passthrough_msrs[]", msr);
+ if (*possible_idx >= 0)
+ return VMX_POSSIBLE_PASSTHROUGH;
- return r;
+ return -ENOENT;
}
struct vmx_uret_msr *vmx_find_uret_msr(struct vcpu_vmx *vmx, u32 msr)
@@ -3954,6 +3961,7 @@ void vmx_disable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
unsigned long *msr_bitmap = vmx->vmcs01.msr_bitmap;
+ int idx;
if (!cpu_has_vmx_msr_bitmap())
return;
@@ -3963,16 +3971,12 @@ void vmx_disable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
/*
* Mark the desired intercept state in shadow bitmap, this is needed
* for resync when the MSR filters change.
- */
- if (is_valid_passthrough_msr(msr)) {
- int idx = possible_passthrough_msr_slot(msr);
-
- if (idx != -ENOENT) {
- if (type & MSR_TYPE_R)
- clear_bit(idx, vmx->shadow_msr_intercept.read);
- if (type & MSR_TYPE_W)
- clear_bit(idx, vmx->shadow_msr_intercept.write);
- }
+ */
+ if (validate_passthrough_msr(msr, &idx) == VMX_POSSIBLE_PASSTHROUGH) {
+ if (type & MSR_TYPE_R)
+ clear_bit(idx, vmx->shadow_msr_intercept.read);
+ if (type & MSR_TYPE_W)
+ clear_bit(idx, vmx->shadow_msr_intercept.write);
}
if ((type & MSR_TYPE_R) &&
@@ -3998,6 +4002,7 @@ void vmx_enable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
unsigned long *msr_bitmap = vmx->vmcs01.msr_bitmap;
+ int idx;
if (!cpu_has_vmx_msr_bitmap())
return;
@@ -4008,15 +4013,11 @@ void vmx_enable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
* Mark the desired intercept state in shadow bitmap, this is needed
* for resync when the MSR filter changes.
*/
- if (is_valid_passthrough_msr(msr)) {
- int idx = possible_passthrough_msr_slot(msr);
-
- if (idx != -ENOENT) {
- if (type & MSR_TYPE_R)
- set_bit(idx, vmx->shadow_msr_intercept.read);
- if (type & MSR_TYPE_W)
- set_bit(idx, vmx->shadow_msr_intercept.write);
- }
+ if (validate_passthrough_msr(msr, &idx) == VMX_POSSIBLE_PASSTHROUGH) {
+ if (type & MSR_TYPE_R)
+ set_bit(idx, vmx->shadow_msr_intercept.read);
+ if (type & MSR_TYPE_W)
+ set_bit(idx, vmx->shadow_msr_intercept.write);
}
if (type & MSR_TYPE_R)
--
2.34.1
On Fri, Feb 16, 2024, Dongli Zhang wrote:
> ---
> arch/x86/kvm/vmx/vmx.c | 55 +++++++++++++++++++++---------------------
> 1 file changed, 28 insertions(+), 27 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 5a866d3c2bc8..76dff0e7d8bd 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -669,14 +669,18 @@ static int possible_passthrough_msr_slot(u32 msr)
> return -ENOENT;
> }
>
> -static bool is_valid_passthrough_msr(u32 msr)
> +#define VMX_POSSIBLE_PASSTHROUGH 1
> +#define VMX_OTHER_PASSTHROUGH 2
> +/*
> + * Vefify if the msr is the passthrough MSRs.
> + * Return the index in *possible_idx if it is a possible passthrough MSR.
> + */
> +static int validate_passthrough_msr(u32 msr, int *possible_idx)
There's no need for a custom tri-state return value or an out-param, just return
the slot/-ENOENT. Not fully tested yet, but this should do the trick.
From: Sean Christopherson <[email protected]>
Date: Mon, 19 Feb 2024 07:58:10 -0800
Subject: [PATCH] KVM: VMX: Combine "check" and "get" APIs for passthrough MSR
lookups
Combine possible_passthrough_msr_slot() and is_valid_passthrough_msr()
into a single function, vmx_get_passthrough_msr_slot(), and have the
combined helper return the slot on success, using a negative value to
indiciate "failure".
Combining the operations avoids iterating over the array of passthrough
MSRs twice for relevant MSRs.
Suggested-by: Dongli Zhang <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/vmx/vmx.c | 63 +++++++++++++++++-------------------------
1 file changed, 25 insertions(+), 38 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 014cf47dc66b..969fd3aa0da3 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -658,25 +658,14 @@ static inline bool cpu_need_virtualize_apic_accesses(struct kvm_vcpu *vcpu)
return flexpriority_enabled && lapic_in_kernel(vcpu);
}
-static int possible_passthrough_msr_slot(u32 msr)
+static int vmx_get_passthrough_msr_slot(u32 msr)
{
- u32 i;
-
- for (i = 0; i < ARRAY_SIZE(vmx_possible_passthrough_msrs); i++)
- if (vmx_possible_passthrough_msrs[i] == msr)
- return i;
-
- return -ENOENT;
-}
-
-static bool is_valid_passthrough_msr(u32 msr)
-{
- bool r;
+ int i;
switch (msr) {
case 0x800 ... 0x8ff:
/* x2APIC MSRs. These are handled in vmx_update_msr_bitmap_x2apic() */
- return true;
+ return -ENOENT;
case MSR_IA32_RTIT_STATUS:
case MSR_IA32_RTIT_OUTPUT_BASE:
case MSR_IA32_RTIT_OUTPUT_MASK:
@@ -691,14 +680,16 @@ static bool is_valid_passthrough_msr(u32 msr)
case MSR_LBR_CORE_FROM ... MSR_LBR_CORE_FROM + 8:
case MSR_LBR_CORE_TO ... MSR_LBR_CORE_TO + 8:
/* LBR MSRs. These are handled in vmx_update_intercept_for_lbr_msrs() */
- return true;
+ return -ENOENT;
}
- r = possible_passthrough_msr_slot(msr) != -ENOENT;
-
- WARN(!r, "Invalid MSR %x, please adapt vmx_possible_passthrough_msrs[]", msr);
+ for (i = 0; i < ARRAY_SIZE(vmx_possible_passthrough_msrs); i++) {
+ if (vmx_possible_passthrough_msrs[i] == msr)
+ return i;
+ }
- return r;
+ WARN(1, "Invalid MSR %x, please adapt vmx_possible_passthrough_msrs[]", msr);
+ return -ENOENT;
}
struct vmx_uret_msr *vmx_find_uret_msr(struct vcpu_vmx *vmx, u32 msr)
@@ -3954,6 +3945,7 @@ void vmx_disable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
unsigned long *msr_bitmap = vmx->vmcs01.msr_bitmap;
+ int idx;
if (!cpu_has_vmx_msr_bitmap())
return;
@@ -3963,16 +3955,13 @@ void vmx_disable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
/*
* Mark the desired intercept state in shadow bitmap, this is needed
* for resync when the MSR filters change.
- */
- if (is_valid_passthrough_msr(msr)) {
- int idx = possible_passthrough_msr_slot(msr);
-
- if (idx != -ENOENT) {
- if (type & MSR_TYPE_R)
- clear_bit(idx, vmx->shadow_msr_intercept.read);
- if (type & MSR_TYPE_W)
- clear_bit(idx, vmx->shadow_msr_intercept.write);
- }
+ */
+ idx = vmx_get_passthrough_msr_slot(msr);
+ if (idx >= 0) {
+ if (type & MSR_TYPE_R)
+ clear_bit(idx, vmx->shadow_msr_intercept.read);
+ if (type & MSR_TYPE_W)
+ clear_bit(idx, vmx->shadow_msr_intercept.write);
}
if ((type & MSR_TYPE_R) &&
@@ -3998,6 +3987,7 @@ void vmx_enable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
unsigned long *msr_bitmap = vmx->vmcs01.msr_bitmap;
+ int idx;
if (!cpu_has_vmx_msr_bitmap())
return;
@@ -4008,15 +3998,12 @@ void vmx_enable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
* Mark the desired intercept state in shadow bitmap, this is needed
* for resync when the MSR filter changes.
*/
- if (is_valid_passthrough_msr(msr)) {
- int idx = possible_passthrough_msr_slot(msr);
-
- if (idx != -ENOENT) {
- if (type & MSR_TYPE_R)
- set_bit(idx, vmx->shadow_msr_intercept.read);
- if (type & MSR_TYPE_W)
- set_bit(idx, vmx->shadow_msr_intercept.write);
- }
+ idx = vmx_get_passthrough_msr_slot(msr);
+ if (idx >= 0) {
+ if (type & MSR_TYPE_R)
+ set_bit(idx, vmx->shadow_msr_intercept.read);
+ if (type & MSR_TYPE_W)
+ set_bit(idx, vmx->shadow_msr_intercept.write);
}
if (type & MSR_TYPE_R)
base-commit: 342c6dfc2a0ae893394a6f894acd1d1728c009f2
--
Hi Sean,
On 2/19/24 14:33, Sean Christopherson wrote:
> On Fri, Feb 16, 2024, Dongli Zhang wrote:
>> ---
>> arch/x86/kvm/vmx/vmx.c | 55 +++++++++++++++++++++---------------------
>> 1 file changed, 28 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 5a866d3c2bc8..76dff0e7d8bd 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -669,14 +669,18 @@ static int possible_passthrough_msr_slot(u32 msr)
>> return -ENOENT;
>> }
>>
>> -static bool is_valid_passthrough_msr(u32 msr)
>> +#define VMX_POSSIBLE_PASSTHROUGH 1
>> +#define VMX_OTHER_PASSTHROUGH 2
>> +/*
>> + * Vefify if the msr is the passthrough MSRs.
>> + * Return the index in *possible_idx if it is a possible passthrough MSR.
>> + */
>> +static int validate_passthrough_msr(u32 msr, int *possible_idx)
>
> There's no need for a custom tri-state return value or an out-param, just return
> the slot/-ENOENT. Not fully tested yet, but this should do the trick.
The new patch looks good to me, from functionality's perspective.
Just that the new patched function looks confusing. That's why I was adding the
out-param initially to differentiate from different cases.
The new vmx_get_passthrough_msr_slot() is just doing the trick by combining many
jobs together:
1. Get the possible passthrough msr slot index.
2. For x2APIC/PT/LBR msr, return -ENOENT.
3. For other msr, return the same -ENOENT, with a WARN.
The semantics of the function look confusing.
If the objective is to return passthrough msr slot, why return -ENOENT for
x2APIC/PT/LBR.
Why both x2APIC/PT/LBR and other MSRs return the same -ENOENT, while the other
MSRs may trigger WARN. (I know this is because the other MSRs do not belong to
any passthrough MSRs).
661 static int vmx_get_passthrough_msr_slot(u32 msr)
662 {
663 int i;
664
665 switch (msr) {
666 case 0x800 ... 0x8ff:
667 /* x2APIC MSRs. These are handled in
vmx_update_msr_bitmap_x2apic() */
668 return -ENOENT;
669 case MSR_IA32_RTIT_STATUS:
670 case MSR_IA32_RTIT_OUTPUT_BASE:
671 case MSR_IA32_RTIT_OUTPUT_MASK:
672 case MSR_IA32_RTIT_CR3_MATCH:
673 case MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B:
674 /* PT MSRs. These are handled in
pt_update_intercept_for_msr() */
675 case MSR_LBR_SELECT:
676 case MSR_LBR_TOS:
677 case MSR_LBR_INFO_0 ... MSR_LBR_INFO_0 + 31:
678 case MSR_LBR_NHM_FROM ... MSR_LBR_NHM_FROM + 31:
679 case MSR_LBR_NHM_TO ... MSR_LBR_NHM_TO + 31:
680 case MSR_LBR_CORE_FROM ... MSR_LBR_CORE_FROM + 8:
681 case MSR_LBR_CORE_TO ... MSR_LBR_CORE_TO + 8:
682 /* LBR MSRs. These are handled in
vmx_update_intercept_for_lbr_msrs() */
683 return -ENOENT;
684 }
685
686 for (i = 0; i < ARRAY_SIZE(vmx_possible_passthrough_msrs); i++) {
687 if (vmx_possible_passthrough_msrs[i] == msr)
688 return i;
689 }
690
691 WARN(1, "Invalid MSR %x, please adapt
vmx_possible_passthrough_msrs[]", msr);
692 return -ENOENT;
693 }
The patch looks good to me.
Thank you very much!
Dongli Zhang
>
> From: Sean Christopherson <[email protected]>
> Date: Mon, 19 Feb 2024 07:58:10 -0800
> Subject: [PATCH] KVM: VMX: Combine "check" and "get" APIs for passthrough MSR
> lookups
>
> Combine possible_passthrough_msr_slot() and is_valid_passthrough_msr()
> into a single function, vmx_get_passthrough_msr_slot(), and have the
> combined helper return the slot on success, using a negative value to
> indiciate "failure".
>
> Combining the operations avoids iterating over the array of passthrough
> MSRs twice for relevant MSRs.
>
> Suggested-by: Dongli Zhang <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/vmx/vmx.c | 63 +++++++++++++++++-------------------------
> 1 file changed, 25 insertions(+), 38 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 014cf47dc66b..969fd3aa0da3 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -658,25 +658,14 @@ static inline bool cpu_need_virtualize_apic_accesses(struct kvm_vcpu *vcpu)
> return flexpriority_enabled && lapic_in_kernel(vcpu);
> }
>
> -static int possible_passthrough_msr_slot(u32 msr)
> +static int vmx_get_passthrough_msr_slot(u32 msr)
> {
> - u32 i;
> -
> - for (i = 0; i < ARRAY_SIZE(vmx_possible_passthrough_msrs); i++)
> - if (vmx_possible_passthrough_msrs[i] == msr)
> - return i;
> -
> - return -ENOENT;
> -}
> -
> -static bool is_valid_passthrough_msr(u32 msr)
> -{
> - bool r;
> + int i;
>
> switch (msr) {
> case 0x800 ... 0x8ff:
> /* x2APIC MSRs. These are handled in vmx_update_msr_bitmap_x2apic() */
> - return true;
> + return -ENOENT;
> case MSR_IA32_RTIT_STATUS:
> case MSR_IA32_RTIT_OUTPUT_BASE:
> case MSR_IA32_RTIT_OUTPUT_MASK:
> @@ -691,14 +680,16 @@ static bool is_valid_passthrough_msr(u32 msr)
> case MSR_LBR_CORE_FROM ... MSR_LBR_CORE_FROM + 8:
> case MSR_LBR_CORE_TO ... MSR_LBR_CORE_TO + 8:
> /* LBR MSRs. These are handled in vmx_update_intercept_for_lbr_msrs() */
> - return true;
> + return -ENOENT;
> }
>
> - r = possible_passthrough_msr_slot(msr) != -ENOENT;
> -
> - WARN(!r, "Invalid MSR %x, please adapt vmx_possible_passthrough_msrs[]", msr);
> + for (i = 0; i < ARRAY_SIZE(vmx_possible_passthrough_msrs); i++) {
> + if (vmx_possible_passthrough_msrs[i] == msr)
> + return i;
> + }
>
> - return r;
> + WARN(1, "Invalid MSR %x, please adapt vmx_possible_passthrough_msrs[]", msr);
> + return -ENOENT;
> }
>
> struct vmx_uret_msr *vmx_find_uret_msr(struct vcpu_vmx *vmx, u32 msr)
> @@ -3954,6 +3945,7 @@ void vmx_disable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> unsigned long *msr_bitmap = vmx->vmcs01.msr_bitmap;
> + int idx;
>
> if (!cpu_has_vmx_msr_bitmap())
> return;
> @@ -3963,16 +3955,13 @@ void vmx_disable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
> /*
> * Mark the desired intercept state in shadow bitmap, this is needed
> * for resync when the MSR filters change.
> - */
> - if (is_valid_passthrough_msr(msr)) {
> - int idx = possible_passthrough_msr_slot(msr);
> -
> - if (idx != -ENOENT) {
> - if (type & MSR_TYPE_R)
> - clear_bit(idx, vmx->shadow_msr_intercept.read);
> - if (type & MSR_TYPE_W)
> - clear_bit(idx, vmx->shadow_msr_intercept.write);
> - }
> + */
> + idx = vmx_get_passthrough_msr_slot(msr);
> + if (idx >= 0) {
> + if (type & MSR_TYPE_R)
> + clear_bit(idx, vmx->shadow_msr_intercept.read);
> + if (type & MSR_TYPE_W)
> + clear_bit(idx, vmx->shadow_msr_intercept.write);
> }
>
> if ((type & MSR_TYPE_R) &&
> @@ -3998,6 +3987,7 @@ void vmx_enable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> unsigned long *msr_bitmap = vmx->vmcs01.msr_bitmap;
> + int idx;
>
> if (!cpu_has_vmx_msr_bitmap())
> return;
> @@ -4008,15 +3998,12 @@ void vmx_enable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
> * Mark the desired intercept state in shadow bitmap, this is needed
> * for resync when the MSR filter changes.
> */
> - if (is_valid_passthrough_msr(msr)) {
> - int idx = possible_passthrough_msr_slot(msr);
> -
> - if (idx != -ENOENT) {
> - if (type & MSR_TYPE_R)
> - set_bit(idx, vmx->shadow_msr_intercept.read);
> - if (type & MSR_TYPE_W)
> - set_bit(idx, vmx->shadow_msr_intercept.write);
> - }
> + idx = vmx_get_passthrough_msr_slot(msr);
> + if (idx >= 0) {
> + if (type & MSR_TYPE_R)
> + set_bit(idx, vmx->shadow_msr_intercept.read);
> + if (type & MSR_TYPE_W)
> + set_bit(idx, vmx->shadow_msr_intercept.write);
> }
>
> if (type & MSR_TYPE_R)
>
> base-commit: 342c6dfc2a0ae893394a6f894acd1d1728c009f2
On Tue, Feb 20, 2024, Dongli Zhang wrote:
> Hi Sean,
>
> On 2/19/24 14:33, Sean Christopherson wrote:
> > On Fri, Feb 16, 2024, Dongli Zhang wrote:
> >> ---
> >> arch/x86/kvm/vmx/vmx.c | 55 +++++++++++++++++++++---------------------
> >> 1 file changed, 28 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> >> index 5a866d3c2bc8..76dff0e7d8bd 100644
> >> --- a/arch/x86/kvm/vmx/vmx.c
> >> +++ b/arch/x86/kvm/vmx/vmx.c
> >> @@ -669,14 +669,18 @@ static int possible_passthrough_msr_slot(u32 msr)
> >> return -ENOENT;
> >> }
> >>
> >> -static bool is_valid_passthrough_msr(u32 msr)
> >> +#define VMX_POSSIBLE_PASSTHROUGH 1
> >> +#define VMX_OTHER_PASSTHROUGH 2
> >> +/*
> >> + * Vefify if the msr is the passthrough MSRs.
> >> + * Return the index in *possible_idx if it is a possible passthrough MSR.
> >> + */
> >> +static int validate_passthrough_msr(u32 msr, int *possible_idx)
> >
> > There's no need for a custom tri-state return value or an out-param, just return
> > the slot/-ENOENT. Not fully tested yet, but this should do the trick.
>
> The new patch looks good to me, from functionality's perspective.
>
> Just that the new patched function looks confusing. That's why I was adding the
> out-param initially to differentiate from different cases.
>
> The new vmx_get_passthrough_msr_slot() is just doing the trick by combining many
> jobs together:
>
> 1. Get the possible passthrough msr slot index.
>
> 2. For x2APIC/PT/LBR msr, return -ENOENT.
>
> 3. For other msr, return the same -ENOENT, with a WARN.
>
> The semantics of the function look confusing.
>
> If the objective is to return passthrough msr slot, why return -ENOENT for
> x2APIC/PT/LBR.
Because there is no "slot" for them in vmx_possible_passthrough_msrs, and the
main purpose of the helpers is to get that slot in order to efficiently update
the MSR bitmaps in response to userspace MSR filter changes. The WARN is an extra
sanity check to ensure that KVM doesn't start passing through an MSR without
adding the MSR to vmx_possible_passthrough_msrs (or special casing it a la XAPIC,
PT, and LBR MSRS).
> Why both x2APIC/PT/LBR and other MSRs return the same -ENOENT, while the other
> MSRs may trigger WARN. (I know this is because the other MSRs do not belong to
> any passthrough MSRs).
The x2APIC/PT/LBR MSRs are given special treatment: KVM may pass them through to
the guest, but unlike the "regular" passthrough MSRs, userspace is NOT allowed to
override that behavior via MSR filters.
And so as mentioned above, they don't have a slot in vmx_possible_passthrough_msrs.
On 2/21/24 07:43, Sean Christopherson wrote:
> On Tue, Feb 20, 2024, Dongli Zhang wrote:
>> Hi Sean,
>>
>> On 2/19/24 14:33, Sean Christopherson wrote:
>>> On Fri, Feb 16, 2024, Dongli Zhang wrote:
>>>> ---
>>>> arch/x86/kvm/vmx/vmx.c | 55 +++++++++++++++++++++---------------------
>>>> 1 file changed, 28 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>>>> index 5a866d3c2bc8..76dff0e7d8bd 100644
>>>> --- a/arch/x86/kvm/vmx/vmx.c
>>>> +++ b/arch/x86/kvm/vmx/vmx.c
>>>> @@ -669,14 +669,18 @@ static int possible_passthrough_msr_slot(u32 msr)
>>>> return -ENOENT;
>>>> }
>>>>
>>>> -static bool is_valid_passthrough_msr(u32 msr)
>>>> +#define VMX_POSSIBLE_PASSTHROUGH 1
>>>> +#define VMX_OTHER_PASSTHROUGH 2
>>>> +/*
>>>> + * Vefify if the msr is the passthrough MSRs.
>>>> + * Return the index in *possible_idx if it is a possible passthrough MSR.
>>>> + */
>>>> +static int validate_passthrough_msr(u32 msr, int *possible_idx)
>>>
>>> There's no need for a custom tri-state return value or an out-param, just return
>>> the slot/-ENOENT. Not fully tested yet, but this should do the trick.
>>
>> The new patch looks good to me, from functionality's perspective.
>>
>> Just that the new patched function looks confusing. That's why I was adding the
>> out-param initially to differentiate from different cases.
>>
>> The new vmx_get_passthrough_msr_slot() is just doing the trick by combining many
>> jobs together:
>>
>> 1. Get the possible passthrough msr slot index.
>>
>> 2. For x2APIC/PT/LBR msr, return -ENOENT.
>>
>> 3. For other msr, return the same -ENOENT, with a WARN.
>>
>> The semantics of the function look confusing.
>>
>> If the objective is to return passthrough msr slot, why return -ENOENT for
>> x2APIC/PT/LBR.
>
> Because there is no "slot" for them in vmx_possible_passthrough_msrs, and the
> main purpose of the helpers is to get that slot in order to efficiently update
> the MSR bitmaps in response to userspace MSR filter changes. The WARN is an extra
> sanity check to ensure that KVM doesn't start passing through an MSR without
> adding the MSR to vmx_possible_passthrough_msrs (or special casing it a la XAPIC,
> PT, and LBR MSRS).
>
>> Why both x2APIC/PT/LBR and other MSRs return the same -ENOENT, while the other
>> MSRs may trigger WARN. (I know this is because the other MSRs do not belong to
>> any passthrough MSRs).
>
> The x2APIC/PT/LBR MSRs are given special treatment: KVM may pass them through to
> the guest, but unlike the "regular" passthrough MSRs, userspace is NOT allowed to
> override that behavior via MSR filters.
>
> And so as mentioned above, they don't have a slot in vmx_possible_passthrough_msrs.
Thank you very much for the explanation! This looks good to me.
Dongli Zhang