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

[Li, Xin3]

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