From: Xin Li <xin3.li@intel.com>
Handle FRED MSR access requests, allowing FRED context to be set/get
from both host and guest.
During VM save/restore and live migration, FRED context needs to be
saved/restored, which requires FRED MSRs to be accessed from userspace,
e.g., Qemu.
Note, handling of MSR_IA32_FRED_SSP0, i.e., MSR_IA32_PL0_SSP, is not
added yet, which is done in the KVM CET patch set.
Signed-off-by: Xin Li <xin3.li@intel.com>
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
Tested-by: Shan Kang <shan.kang@intel.com>
---
Changes since v2:
* Add a helper to convert FRED MSR index to VMCS field encoding to
make the code more compact (Chao Gao).
* Get rid of the "host_initiated" check because userspace has to set
CPUID before MSRs (Chao Gao & Sean Christopherson).
* Address a few cleanup comments (Sean Christopherson).
Changes since v1:
* Use kvm_cpu_cap_has() instead of cpu_feature_enabled() (Chao Gao).
* Fail host requested FRED MSRs access if KVM cannot virtualize FRED
(Chao Gao).
* Handle the case FRED MSRs are valid but KVM cannot virtualize FRED
(Chao Gao).
* Add sanity checks when writing to FRED MSRs.
---
arch/x86/kvm/vmx/vmx.c | 48 ++++++++++++++++++++++++++++++++++++++++++
arch/x86/kvm/x86.c | 28 ++++++++++++++++++++++++
2 files changed, 76 insertions(+)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 1fd32aa255f9..ae9712624413 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1426,6 +1426,24 @@ static void vmx_write_guest_kernel_gs_base(struct vcpu_vmx *vmx, u64 data)
preempt_enable();
vmx->msr_guest_kernel_gs_base = data;
}
+
+static u64 vmx_read_guest_fred_rsp0(struct vcpu_vmx *vmx)
+{
+ preempt_disable();
+ if (vmx->guest_state_loaded)
+ vmx->msr_guest_fred_rsp0 = read_msr(MSR_IA32_FRED_RSP0);
+ preempt_enable();
+ return vmx->msr_guest_fred_rsp0;
+}
+
+static void vmx_write_guest_fred_rsp0(struct vcpu_vmx *vmx, u64 data)
+{
+ preempt_disable();
+ if (vmx->guest_state_loaded)
+ wrmsrns(MSR_IA32_FRED_RSP0, data);
+ preempt_enable();
+ vmx->msr_guest_fred_rsp0 = data;
+}
#endif
static void grow_ple_window(struct kvm_vcpu *vcpu)
@@ -2039,6 +2057,24 @@ int vmx_get_feature_msr(u32 msr, u64 *data)
}
}
+#ifdef CONFIG_X86_64
+static u32 fred_msr_vmcs_fields[] = {
+ GUEST_IA32_FRED_RSP1,
+ GUEST_IA32_FRED_RSP2,
+ GUEST_IA32_FRED_RSP3,
+ GUEST_IA32_FRED_STKLVLS,
+ GUEST_IA32_FRED_SSP1,
+ GUEST_IA32_FRED_SSP2,
+ GUEST_IA32_FRED_SSP3,
+ GUEST_IA32_FRED_CONFIG,
+};
+
+static u32 fred_msr_to_vmcs(u32 msr)
+{
+ return fred_msr_vmcs_fields[msr - MSR_IA32_FRED_RSP1];
+}
+#endif
+
/*
* Reads an msr value (of 'msr_info->index') into 'msr_info->data'.
* Returns 0 on success, non-0 otherwise.
@@ -2061,6 +2097,12 @@ int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
case MSR_KERNEL_GS_BASE:
msr_info->data = vmx_read_guest_kernel_gs_base(vmx);
break;
+ case MSR_IA32_FRED_RSP0:
+ msr_info->data = vmx_read_guest_fred_rsp0(vmx);
+ break;
+ case MSR_IA32_FRED_RSP1 ... MSR_IA32_FRED_CONFIG:
+ msr_info->data = vmcs_read64(fred_msr_to_vmcs(msr_info->index));
+ break;
#endif
case MSR_EFER:
return kvm_get_msr_common(vcpu, msr_info);
@@ -2268,6 +2310,12 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
vmx_update_exception_bitmap(vcpu);
}
break;
+ case MSR_IA32_FRED_RSP0:
+ vmx_write_guest_fred_rsp0(vmx, data);
+ break;
+ case MSR_IA32_FRED_RSP1 ... MSR_IA32_FRED_CONFIG:
+ vmcs_write64(fred_msr_to_vmcs(msr_index), data);
+ break;
#endif
case MSR_IA32_SYSENTER_CS:
if (is_guest_mode(vcpu))
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c841817a914a..007577143337 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -318,6 +318,9 @@ static const u32 msrs_to_save_base[] = {
MSR_STAR,
#ifdef CONFIG_X86_64
MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
+ MSR_IA32_FRED_RSP0, MSR_IA32_FRED_RSP1, MSR_IA32_FRED_RSP2,
+ MSR_IA32_FRED_RSP3, MSR_IA32_FRED_STKLVLS, MSR_IA32_FRED_SSP1,
+ MSR_IA32_FRED_SSP2, MSR_IA32_FRED_SSP3, MSR_IA32_FRED_CONFIG,
#endif
MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
MSR_IA32_FEAT_CTL, MSR_IA32_BNDCFGS, MSR_TSC_AUX,
@@ -1849,6 +1852,23 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data,
data = (u32)data;
break;
+ case MSR_IA32_FRED_RSP0 ... MSR_IA32_FRED_CONFIG:
+ if (!guest_cpu_cap_has(vcpu, X86_FEATURE_FRED))
+ return 1;
+
+ /* Bit 11, bits 5:4, and bit 2 of the IA32_FRED_CONFIG must be zero */
+ if (index == MSR_IA32_FRED_CONFIG && data & (BIT_ULL(11) | GENMASK_ULL(5, 4) | BIT_ULL(2)))
+ return 1;
+ if (index != MSR_IA32_FRED_STKLVLS && is_noncanonical_msr_address(data, vcpu))
+ return 1;
+ if ((index >= MSR_IA32_FRED_RSP0 && index <= MSR_IA32_FRED_RSP3) &&
+ (data & GENMASK_ULL(5, 0)))
+ return 1;
+ if ((index >= MSR_IA32_FRED_SSP1 && index <= MSR_IA32_FRED_SSP3) &&
+ (data & GENMASK_ULL(2, 0)))
+ return 1;
+
+ break;
}
msr.data = data;
@@ -1893,6 +1913,10 @@ int __kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data,
!guest_cpu_cap_has(vcpu, X86_FEATURE_RDPID))
return 1;
break;
+ case MSR_IA32_FRED_RSP0 ... MSR_IA32_FRED_CONFIG:
+ if (!guest_cpu_cap_has(vcpu, X86_FEATURE_FRED))
+ return 1;
+ break;
}
msr.index = index;
@@ -7455,6 +7479,10 @@ static void kvm_probe_msr_to_save(u32 msr_index)
if (!(kvm_get_arch_capabilities() & ARCH_CAP_TSX_CTRL_MSR))
return;
break;
+ case MSR_IA32_FRED_RSP0 ... MSR_IA32_FRED_CONFIG:
+ if (!kvm_cpu_cap_has(X86_FEATURE_FRED))
+ return;
+ break;
default:
break;
}
--
2.48.1On Fri, Mar 28, 2025, Xin Li (Intel) wrote:
> From: Xin Li <xin3.li@intel.com>
>
> Handle FRED MSR access requests, allowing FRED context to be set/get
> from both host and guest.
>
> During VM save/restore and live migration, FRED context needs to be
> saved/restored, which requires FRED MSRs to be accessed from userspace,
> e.g., Qemu.
>
> Note, handling of MSR_IA32_FRED_SSP0, i.e., MSR_IA32_PL0_SSP, is not
> added yet, which is done in the KVM CET patch set.
>
> Signed-off-by: Xin Li <xin3.li@intel.com>
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> Tested-by: Shan Kang <shan.kang@intel.com>
> ---
>
> Changes since v2:
> * Add a helper to convert FRED MSR index to VMCS field encoding to
> make the code more compact (Chao Gao).
> * Get rid of the "host_initiated" check because userspace has to set
> CPUID before MSRs (Chao Gao & Sean Christopherson).
> * Address a few cleanup comments (Sean Christopherson).
>
> Changes since v1:
> * Use kvm_cpu_cap_has() instead of cpu_feature_enabled() (Chao Gao).
> * Fail host requested FRED MSRs access if KVM cannot virtualize FRED
> (Chao Gao).
> * Handle the case FRED MSRs are valid but KVM cannot virtualize FRED
> (Chao Gao).
> * Add sanity checks when writing to FRED MSRs.
> ---
> arch/x86/kvm/vmx/vmx.c | 48 ++++++++++++++++++++++++++++++++++++++++++
> arch/x86/kvm/x86.c | 28 ++++++++++++++++++++++++
> 2 files changed, 76 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 1fd32aa255f9..ae9712624413 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1426,6 +1426,24 @@ static void vmx_write_guest_kernel_gs_base(struct vcpu_vmx *vmx, u64 data)
> preempt_enable();
> vmx->msr_guest_kernel_gs_base = data;
> }
> +
> +static u64 vmx_read_guest_fred_rsp0(struct vcpu_vmx *vmx)
> +{
> + preempt_disable();
> + if (vmx->guest_state_loaded)
> + vmx->msr_guest_fred_rsp0 = read_msr(MSR_IA32_FRED_RSP0);
> + preempt_enable();
> + return vmx->msr_guest_fred_rsp0;
> +}
> +
> +static void vmx_write_guest_fred_rsp0(struct vcpu_vmx *vmx, u64 data)
> +{
> + preempt_disable();
> + if (vmx->guest_state_loaded)
> + wrmsrns(MSR_IA32_FRED_RSP0, data);
> + preempt_enable();
> + vmx->msr_guest_fred_rsp0 = data;
> +}
> #endif
Maybe add helpers to deal with the preemption stuff? Oh, never mind, FRED
uses WRMSRNS. Hmm, actually, can't these all be non-serializing? KVM is
progating *guest* values to hardware, so a VM-Enter is guaranteed before the
CPU value can be consumed.
#ifdef CONFIG_X86_64
static u64 vmx_read_guest_host_msr(struct vcpu_vmx *vmx, u32 msr, u64 *cache)
{
preempt_disable();
if (vmx->guest_state_loaded)
*cache = read_msr(msr);
preempt_enable();
return *cache;
}
static u64 vmx_write_guest_host_msr(struct vcpu_vmx *vmx, u32 msr, u64 data,
u64 *cache)
{
preempt_disable();
if (vmx->guest_state_loaded)
wrmsrns(MSR_KERNEL_GS_BASE, data);
preempt_enable();
*cache = data;
}
static u64 vmx_read_guest_kernel_gs_base(struct vcpu_vmx *vmx)
{
return vmx_read_guest_host_msr(vmx, MSR_KERNEL_GS_BASE,
&vmx->msr_guest_kernel_gs_base);
}
static void vmx_write_guest_kernel_gs_base(struct vcpu_vmx *vmx, u64 data)
{
vmx_write_guest_host_msr(vmx, MSR_KERNEL_GS_BASE, data,
&vmx->msr_guest_kernel_gs_base);
}
static u64 vmx_read_guest_fred_rsp0(struct vcpu_vmx *vmx)
{
return vmx_read_guest_host_msr(vmx, MSR_IA32_FRED_RSP0,
&vmx->msr_guest_fred_rsp0);
}
static void vmx_write_guest_fred_rsp0(struct vcpu_vmx *vmx, u64 data)
{
return vmx_write_guest_host_msr(vmx, MSR_IA32_FRED_RSP0, data,
&vmx->msr_guest_fred_rsp0);
}
#endif
> static void grow_ple_window(struct kvm_vcpu *vcpu)
> @@ -2039,6 +2057,24 @@ int vmx_get_feature_msr(u32 msr, u64 *data)
> }
> }
>
> +#ifdef CONFIG_X86_64
> +static u32 fred_msr_vmcs_fields[] = {
This should be const.
> + GUEST_IA32_FRED_RSP1,
> + GUEST_IA32_FRED_RSP2,
> + GUEST_IA32_FRED_RSP3,
> + GUEST_IA32_FRED_STKLVLS,
> + GUEST_IA32_FRED_SSP1,
> + GUEST_IA32_FRED_SSP2,
> + GUEST_IA32_FRED_SSP3,
> + GUEST_IA32_FRED_CONFIG,
> +};
I think it also makes sense to add a static_assert() here, more so to help
readers follow along than anything else.
static_assert(MSR_IA32_FRED_CONFIG - MSR_IA32_FRED_RSP1 ==
ARRAY_SIZE(fred_msr_vmcs_fields) - 1);
> +
> +static u32 fred_msr_to_vmcs(u32 msr)
> +{
> + return fred_msr_vmcs_fields[msr - MSR_IA32_FRED_RSP1];
> +}
> +#endif
> +
> /*
> * Reads an msr value (of 'msr_info->index') into 'msr_info->data'.
> * Returns 0 on success, non-0 otherwise.
> @@ -2061,6 +2097,12 @@ int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> case MSR_KERNEL_GS_BASE:
> msr_info->data = vmx_read_guest_kernel_gs_base(vmx);
> break;
> + case MSR_IA32_FRED_RSP0:
> + msr_info->data = vmx_read_guest_fred_rsp0(vmx);
> + break;
> + case MSR_IA32_FRED_RSP1 ... MSR_IA32_FRED_CONFIG:
> + msr_info->data = vmcs_read64(fred_msr_to_vmcs(msr_info->index));
> + break;
> #endif
> case MSR_EFER:
> return kvm_get_msr_common(vcpu, msr_info);
> @@ -2268,6 +2310,12 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> vmx_update_exception_bitmap(vcpu);
> }
> break;
> + case MSR_IA32_FRED_RSP0:
> + vmx_write_guest_fred_rsp0(vmx, data);
> + break;
> + case MSR_IA32_FRED_RSP1 ... MSR_IA32_FRED_CONFIG:
> + vmcs_write64(fred_msr_to_vmcs(msr_index), data);
> + break;
> #endif
> case MSR_IA32_SYSENTER_CS:
> if (is_guest_mode(vcpu))
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c841817a914a..007577143337 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -318,6 +318,9 @@ static const u32 msrs_to_save_base[] = {
> MSR_STAR,
> #ifdef CONFIG_X86_64
> MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
> + MSR_IA32_FRED_RSP0, MSR_IA32_FRED_RSP1, MSR_IA32_FRED_RSP2,
> + MSR_IA32_FRED_RSP3, MSR_IA32_FRED_STKLVLS, MSR_IA32_FRED_SSP1,
> + MSR_IA32_FRED_SSP2, MSR_IA32_FRED_SSP3, MSR_IA32_FRED_CONFIG,
> #endif
> MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
> MSR_IA32_FEAT_CTL, MSR_IA32_BNDCFGS, MSR_TSC_AUX,
> @@ -1849,6 +1852,23 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data,
>
> data = (u32)data;
> break;
> + case MSR_IA32_FRED_RSP0 ... MSR_IA32_FRED_CONFIG:
> + if (!guest_cpu_cap_has(vcpu, X86_FEATURE_FRED))
> + return 1;
Yeesh, this is a bit of a no-win situation. Having to re-check the MSR index is
no fun, but the amount of overlap between MSRs is significant, i.e. I see why you
bundled everything together. Ugh, and MSR_IA32_FRED_STKLVLS is buried smack dab
in the middle of everything.
> +
> + /* Bit 11, bits 5:4, and bit 2 of the IA32_FRED_CONFIG must be zero */
Eh, the comment isn't helping much. If we want to add more documentation, add
#defines. But I think we can documented the reserved behavior while also tidying
up the code a bit.
After much fiddling, how about this?
case MSR_IA32_FRED_STKLVLS:
if (!guest_cpu_cap_has(vcpu, X86_FEATURE_FRED))
return 1;
break;
case MSR_IA32_FRED_RSP0 ... MSR_IA32_FRED_RSP3:
case MSR_IA32_FRED_SSP1 ... MSR_IA32_FRED_CONFIG: {
u64 reserved_bits;
if (!guest_cpu_cap_has(vcpu, X86_FEATURE_FRED))
return 1;
if (is_noncanonical_msr_address(data, vcpu))
return 1;
switch (index) {
case MSR_IA32_FRED_CONFIG:
reserved_bits = BIT_ULL(11) | GENMASK_ULL(5, 4) | BIT_ULL(2);
break;
case MSR_IA32_FRED_RSP0 ... MSR_IA32_FRED_RSP3:
reserved_bits = GENMASK_ULL(5, 0);
break;
case MSR_IA32_FRED_SSP1 ... MSR_IA32_FRED_SSP3:
reserved_bits = GENMASK_ULL(2, 0);
break;
default:
WARN_ON_ONCE(1);
return 1;
}
if (data & reserved_bits)
return 1;
break;
}
> @@ -1893,6 +1913,10 @@ int __kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data,
> !guest_cpu_cap_has(vcpu, X86_FEATURE_RDPID))
> return 1;
> break;
> + case MSR_IA32_FRED_RSP0 ... MSR_IA32_FRED_CONFIG:
> + if (!guest_cpu_cap_has(vcpu, X86_FEATURE_FRED))
> + return 1;
> + break;
> }
>
> msr.index = index;
> @@ -7455,6 +7479,10 @@ static void kvm_probe_msr_to_save(u32 msr_index)
> if (!(kvm_get_arch_capabilities() & ARCH_CAP_TSX_CTRL_MSR))
> return;
> break;
> + case MSR_IA32_FRED_RSP0 ... MSR_IA32_FRED_CONFIG:
> + if (!kvm_cpu_cap_has(X86_FEATURE_FRED))
> + return;
> + break;
> default:
> break;
> }
> --
> 2.48.1
>
On 6/24/2025 9:27 AM, Sean Christopherson wrote:
>> +
>> +static u64 vmx_read_guest_fred_rsp0(struct vcpu_vmx *vmx)
>> +{
>> + preempt_disable();
>> + if (vmx->guest_state_loaded)
>> + vmx->msr_guest_fred_rsp0 = read_msr(MSR_IA32_FRED_RSP0);
>> + preempt_enable();
>> + return vmx->msr_guest_fred_rsp0;
>> +}
>> +
>> +static void vmx_write_guest_fred_rsp0(struct vcpu_vmx *vmx, u64 data)
>> +{
>> + preempt_disable();
>> + if (vmx->guest_state_loaded)
>> + wrmsrns(MSR_IA32_FRED_RSP0, data);
>> + preempt_enable();
>> + vmx->msr_guest_fred_rsp0 = data;
>> +}
>> #endif
>
> Maybe add helpers to deal with the preemption stuff? Oh, never mind, FRED
This is a good idea.
Do you want to upstream the following patch?
So I can rebase this patch on top of it in the next iteration.
> uses WRMSRNS. Hmm, actually, can't these all be non-serializing? KVM is
> progating *guest* values to hardware, so a VM-Enter is guaranteed before the
> CPU value can be consumed.
I see your point. It seems that only a new MSR write instruction could
achieve this: consistently performing a non-serializing write to a MSR
with the assumption that the target is a guest MSR. So software needs
to explicitly specify the type, host or guest, of the target MSR to the CPU.
(WRMSRNS writes to an MSR in either a serializing or non-serializing
manner, only based on its index.)
>
> #ifdef CONFIG_X86_64
> static u64 vmx_read_guest_host_msr(struct vcpu_vmx *vmx, u32 msr, u64 *cache)
> {
> preempt_disable();
> if (vmx->guest_state_loaded)
> *cache = read_msr(msr);
> preempt_enable();
> return *cache;
> }
>
> static u64 vmx_write_guest_host_msr(struct vcpu_vmx *vmx, u32 msr, u64 data,
> u64 *cache)
> {
> preempt_disable();
> if (vmx->guest_state_loaded)
> wrmsrns(MSR_KERNEL_GS_BASE, data);
> preempt_enable();
> *cache = data;
> }
>
> static u64 vmx_read_guest_kernel_gs_base(struct vcpu_vmx *vmx)
> {
> return vmx_read_guest_host_msr(vmx, MSR_KERNEL_GS_BASE,
> &vmx->msr_guest_kernel_gs_base);
> }
>
> static void vmx_write_guest_kernel_gs_base(struct vcpu_vmx *vmx, u64 data)
> {
> vmx_write_guest_host_msr(vmx, MSR_KERNEL_GS_BASE, data,
> &vmx->msr_guest_kernel_gs_base);
> }
>
> static u64 vmx_read_guest_fred_rsp0(struct vcpu_vmx *vmx)
> {
> return vmx_read_guest_host_msr(vmx, MSR_IA32_FRED_RSP0,
> &vmx->msr_guest_fred_rsp0);
> }
>
> static void vmx_write_guest_fred_rsp0(struct vcpu_vmx *vmx, u64 data)
> {
> return vmx_write_guest_host_msr(vmx, MSR_IA32_FRED_RSP0, data,
> &vmx->msr_guest_fred_rsp0);
> }
> #endif
>
>> +#ifdef CONFIG_X86_64
>> +static u32 fred_msr_vmcs_fields[] = {
>
> This should be const.
Will add.
>
>> + GUEST_IA32_FRED_RSP1,
>> + GUEST_IA32_FRED_RSP2,
>> + GUEST_IA32_FRED_RSP3,
>> + GUEST_IA32_FRED_STKLVLS,
>> + GUEST_IA32_FRED_SSP1,
>> + GUEST_IA32_FRED_SSP2,
>> + GUEST_IA32_FRED_SSP3,
>> + GUEST_IA32_FRED_CONFIG,
>> +};
>
> I think it also makes sense to add a static_assert() here, more so to help
> readers follow along than anything else.
>
> static_assert(MSR_IA32_FRED_CONFIG - MSR_IA32_FRED_RSP1 ==
> ARRAY_SIZE(fred_msr_vmcs_fields) - 1);
Good idea!
I tried to make fred_msr_to_vmcs() fail at build time, but couldn’t get
it to work.
>
>> +
>> +static u32 fred_msr_to_vmcs(u32 msr)
>> +{
>> + return fred_msr_vmcs_fields[msr - MSR_IA32_FRED_RSP1];
>> +}
>> +#endif
>> +
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> @@ -1849,6 +1852,23 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data,
>>
>> data = (u32)data;
>> break;
>> + case MSR_IA32_FRED_RSP0 ... MSR_IA32_FRED_CONFIG:
>> + if (!guest_cpu_cap_has(vcpu, X86_FEATURE_FRED))
>> + return 1;
>
> Yeesh, this is a bit of a no-win situation. Having to re-check the MSR index is
> no fun, but the amount of overlap between MSRs is significant, i.e. I see why you
> bundled everything together. Ugh, and MSR_IA32_FRED_STKLVLS is buried smack dab
> in the middle of everything.
>
>> +
>> + /* Bit 11, bits 5:4, and bit 2 of the IA32_FRED_CONFIG must be zero */
>
> Eh, the comment isn't helping much. If we want to add more documentation, add
> #defines. But I think we can documented the reserved behavior while also tidying
> up the code a bit.
>
> After much fiddling, how about this?
>
> case MSR_IA32_FRED_STKLVLS:
> if (!guest_cpu_cap_has(vcpu, X86_FEATURE_FRED))
> return 1;
> break;
>
> case MSR_IA32_FRED_RSP0 ... MSR_IA32_FRED_RSP3:
> case MSR_IA32_FRED_SSP1 ... MSR_IA32_FRED_CONFIG: {
> u64 reserved_bits;
>
> if (!guest_cpu_cap_has(vcpu, X86_FEATURE_FRED))
> return 1;
>
> if (is_noncanonical_msr_address(data, vcpu))
> return 1;
>
> switch (index) {
> case MSR_IA32_FRED_CONFIG:
> reserved_bits = BIT_ULL(11) | GENMASK_ULL(5, 4) | BIT_ULL(2);
> break;
> case MSR_IA32_FRED_RSP0 ... MSR_IA32_FRED_RSP3:
> reserved_bits = GENMASK_ULL(5, 0);
> break;
> case MSR_IA32_FRED_SSP1 ... MSR_IA32_FRED_SSP3:
> reserved_bits = GENMASK_ULL(2, 0);
> break;
> default:
> WARN_ON_ONCE(1);
> return 1;
> }
> if (data & reserved_bits)
> return 1;
> break;
> }
>
Easier to read, I will use it :)
Thanks!
Xin
On 6/25/2025 10:18 AM, Xin Li wrote: >> >> Maybe add helpers to deal with the preemption stuff? Oh, never mind, >> FRED > > This is a good idea. > > Do you want to upstream the following patch? As I have almost done addressing your comments in my local repo, just sent out the patch. It's based on the latest kvm-x86/vmx branch.
On Thu, Jun 26, 2025, Xin Li wrote: > On 6/25/2025 10:18 AM, Xin Li wrote: > > > > > > Maybe add helpers to deal with the preemption stuff? Oh, never > > > mind, FRED > > > > This is a good idea. > > > > Do you want to upstream the following patch? > > As I have almost done addressing your comments in my local repo, just > sent out the patch. Saw it, and the LKGS patch. I'm OOO for a week, so I probably won't get them applied for a couple weeks. Thanks!
© 2016 - 2025 Red Hat, Inc.