RE: [PATCH v1 08/23] KVM: VMX: Initialize VMCS FRED fields

Li, Xin3
2023-11-14T06:02:58Z

> >@@ -1477,6 +1477,18 @@ void vmx_vcpu_load_vmcs(struct kvm_vcpu *vcpu, int
> cpu,
> > (unsigned long)(cpu_entry_stack(cpu) + 1));
> > }
> >
> >+#ifdef CONFIG_X86_64
> >+ /* Per-CPU FRED MSRs */
> >+ if (cpu_feature_enabled(X86_FEATURE_FRED)) {
>
> how about kvm_cpu_cap_has()? to decouple KVM's capability to virtualize a feature
> and host's enabling a feature.

Very likely I guess.

> >+ vmcs_write64(HOST_IA32_FRED_RSP1,
> read_msr(MSR_IA32_FRED_RSP1));
> >+ vmcs_write64(HOST_IA32_FRED_RSP2,
> read_msr(MSR_IA32_FRED_RSP2));
> >+ vmcs_write64(HOST_IA32_FRED_RSP3,
> read_msr(MSR_IA32_FRED_RSP3));
> >+ vmcs_write64(HOST_IA32_FRED_SSP1,
> read_msr(MSR_IA32_FRED_SSP1));
> >+ vmcs_write64(HOST_IA32_FRED_SSP2,
> read_msr(MSR_IA32_FRED_SSP2));
> >+ vmcs_write64(HOST_IA32_FRED_SSP3,
> read_msr(MSR_IA32_FRED_SSP3));
> >+ }
> >+#endif
>
> why is this hunk enclosed in #ifdef CONFIG_X86_64 while the one below isn't?

As if the compiler doesn't complain, I should NOT add it.

>
> >+ if (cpu_feature_enabled(X86_FEATURE_FRED)) {
> >+ vmcs_write64(GUEST_IA32_FRED_CONFIG, 0);
> >+ vmcs_write64(GUEST_IA32_FRED_RSP1, 0);
> >+ vmcs_write64(GUEST_IA32_FRED_RSP2, 0);
> >+ vmcs_write64(GUEST_IA32_FRED_RSP3, 0);
> >+ vmcs_write64(GUEST_IA32_FRED_STKLVLS, 0);
> >+ vmcs_write64(GUEST_IA32_FRED_SSP1, 0);
> >+ vmcs_write64(GUEST_IA32_FRED_SSP2, 0);
> >+ vmcs_write64(GUEST_IA32_FRED_SSP3, 0);
> >+ }
> >+
>
> move this hunk to __vmx_vcpu_reset() because FRED spec says
>
> "INIT does not change the value of the new MSRs."
>

Yeah, will do.