Re: [PATCH v9 07/22] KVM: VMX: Initialize VMCS FRED fields

[Xin Li]

> On Mar 4, 2026, at 8:23 AM, Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> 
> On Wed, Jan 21, 2026, Binbin Wu wrote:
>> On 10/27/2025 4:18 AM, Xin Li (Intel) wrote:
>>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>>> index fcfa99160018..c8b5359123bf 100644
>>> --- a/arch/x86/kvm/vmx/vmx.c
>>> +++ b/arch/x86/kvm/vmx/vmx.c
>>> @@ -1459,6 +1459,15 @@ void vmx_vcpu_load_vmcs(struct kvm_vcpu *vcpu, int cpu)
>>>    (unsigned long)(cpu_entry_stack(cpu) + 1));
>>> }
>>> 
>>> + /* Per-CPU FRED MSRs */
> 
> Meh, this is pretty self-explanatory code.

I want to remind that this is “Per-CPU”.

On the other side, FRED_CONFIG and FRED_STKLVLS are typically the same on
all CPUs, and they don’t need to be updated during vCPU migration.

But anyway vCPU migration only cares per-CPU MSRs, so this is redundant.

It often bothers me whether to explain the code a bit more or not, with a
short brief comment or a lengthy one.

> 
>>> + if (kvm_cpu_cap_has(X86_FEATURE_FRED)) {
>>> +#ifdef CONFIG_X86_64
>> 
>> Nit:
>> 
>> Is this needed?
>> 
>> FRED is initialized by X86_64_F(), if CONFIG_X86_64 is not enabled, this
>> path is not reachable.
>> There should be no compilation issue without #ifdef CONFIG_X86_64 / #endif.
>> 
>> There are several similar patterns in this patch, using  #ifdef CONFIG_X86_64 / 
>> #endif or not seems not consistent. E.g. __vmx_vcpu_reset() and init_vmcs()
>> doesn't check the config, but here does.
>> 
>>> + vmcs_write64(HOST_IA32_FRED_RSP1, __this_cpu_ist_top_va(ESTACK_DB));
>>> + vmcs_write64(HOST_IA32_FRED_RSP2, __this_cpu_ist_top_va(ESTACK_NMI));
>>> + vmcs_write64(HOST_IA32_FRED_RSP3, __this_cpu_ist_top_va(ESTACK_DF));
> 
> IMO, this is flawed for other reasons.  KVM shouldn't be relying on kernel
> implementation details with respect to what FRED stack handles what event.
> 
> The simplest approach would be to read the actual MSR.  _If_ using a per-CPU read
> provides meaningful performance benefits over RDMSR (or RDMSR w/ immediate?  I
> don't see an API for that...), then have the kernel provide a dedicated accessor.

I think you asked for it:

https://lore.kernel.org/kvm/ZmoWB_XtA0wR2K4Q@xxxxxxxxxx/

I assume fetching through per-CPU cache is fast, but I might have misunderstood
your suggestion.


> 
> Then the accessor can be a non-inlined functions, and this code can be e.g.:
> 
> if (IS_ENABLED(CONFIG_X86_64) && kvm_cpu_cap_has(X86_FEATURE_FRED)) {
> vmcs_write64(HOST_IA32_FRED_RSP1, fred_rsp(MSR_IA32_FRED_RSP1));
> vmcs_write64(HOST_IA32_FRED_RSP2, fred_rsp(MSR_IA32_FRED_RSP2));
> vmcs_write64(HOST_IA32_FRED_RSP3, fred_rsp(MSR_IA32_FRED_RSP2));
> }
> 
> where fred_rsp() is _declared_ unconditionally, but implemented only for 64-bit.
> That way the compiler will be happy, and the actual usage will be dropped before
> linking via dead-code elimination.

If KVM can’t rely on kernel side implementation details, fred_rsp() has to read
directly from the corresponding MSRs, right?


> 
> Actually, we can probably do one better?
> 
> if (cpu_feature_enabled(X86_FEATURE_FRED) && kvm_cpu_cap_has(X86_FEATURE_FRED)) {

I think KVM now forces X86_FEATURE_FRED=y, no?