Re: [PATCH] KVM: nVMX: Disallow access to vmcs12 fields that aren't supported by "hardware"

[Chao Gao <chao.gao@intel.com>]

>+static __init bool cpu_has_vmcs12_field(unsigned int idx)
>+{
>+	switch (VMCS12_IDX_TO_ENC(idx)) {
>+	case VIRTUAL_PROCESSOR_ID: return cpu_has_vmx_vpid();
>+	case POSTED_INTR_NV: return cpu_has_vmx_posted_intr();
>+	VMCS12_CASE64(TSC_MULTIPLIER): return cpu_has_vmx_tsc_scaling();
>+	VMCS12_CASE64(VIRTUAL_APIC_PAGE_ADDR): return cpu_has_vmx_tpr_shadow();
>+	VMCS12_CASE64(APIC_ACCESS_ADDR): return cpu_has_vmx_virtualize_apic_accesses();
>+	VMCS12_CASE64(POSTED_INTR_DESC_ADDR): return cpu_has_vmx_posted_intr();
>+	VMCS12_CASE64(VM_FUNCTION_CONTROL): return cpu_has_vmx_vmfunc();
>+	VMCS12_CASE64(EPT_POINTER): return cpu_has_vmx_ept();
>+	VMCS12_CASE64(EPTP_LIST_ADDRESS): return cpu_has_vmx_vmfunc();
>+	VMCS12_CASE64(XSS_EXIT_BITMAP): return cpu_has_vmx_xsaves();
>+	VMCS12_CASE64(ENCLS_EXITING_BITMAP): return cpu_has_vmx_encls_vmexit();
>+	VMCS12_CASE64(GUEST_IA32_PERF_GLOBAL_CTRL): return cpu_has_load_perf_global_ctrl();
>+	VMCS12_CASE64(HOST_IA32_PERF_GLOBAL_CTRL): return cpu_has_load_perf_global_ctrl();
>+	case TPR_THRESHOLD: return cpu_has_vmx_tpr_shadow();
>+	case SECONDARY_VM_EXEC_CONTROL: return cpu_has_secondary_exec_ctrls();
>+	case GUEST_S_CET: return cpu_has_load_cet_ctrl();
>+	case GUEST_SSP: return cpu_has_load_cet_ctrl();
>+	case GUEST_INTR_SSP_TABLE: return cpu_has_load_cet_ctrl();
>+	case HOST_S_CET: return cpu_has_load_cet_ctrl();
>+	case HOST_SSP: return cpu_has_load_cet_ctrl();
>+	case HOST_INTR_SSP_TABLE: return cpu_has_load_cet_ctrl();

Most fields here are not shadowed, e.g., CET-related fields. So, the plan is
that new fields should be added here regardless of whether they are shadowed or
not, right?

And GUEST_INTR_STATUS is missing here. It depends on APICv and is handled
explicitly in init_vmcs_shadow_fields().

>+
>+	/* KVM always emulates PML and the VMX preemption timer in software. */
>+	case GUEST_PML_INDEX:
>+	case VMX_PREEMPTION_TIMER_VALUE:
>+	default:
>+		return true;
>+	}
>+}
>+
>+void __init nested_vmx_setup_vmcs12_fields(void)
>+{
>+	unsigned int i;
>+
>+	for (i = 0; i < ARRAY_SIZE(supported_vmcs12_field_offsets); i++) {
>+		if (!supported_vmcs12_field_offsets[i] ||
>+		    !cpu_has_vmcs12_field(i))
>+			continue;
>+
>+		vmcs12_field_offsets[i] = supported_vmcs12_field_offsets[i];
>+		nr_vmcs12_fields = i + 1;
>+	}
>+}
>diff --git a/arch/x86/kvm/vmx/vmcs12.h b/arch/x86/kvm/vmx/vmcs12.h
>index 4ad6b16525b9..e5905ba0bb42 100644
>--- a/arch/x86/kvm/vmx/vmcs12.h
>+++ b/arch/x86/kvm/vmx/vmcs12.h
>@@ -374,8 +374,12 @@ static inline void vmx_check_vmcs12_offsets(void)
> 	CHECK_OFFSET(guest_pml_index, 996);
> }
> 
>-extern const unsigned short vmcs12_field_offsets[];
>-extern const unsigned int nr_vmcs12_fields;
>+extern const __initconst u16 supported_vmcs12_field_offsets[];

No need to extern supported_vmcs12_field_offsets since it's only used in
vmcs12.c.

>+
>+extern u16 vmcs12_field_offsets[] __ro_after_init;
>+extern unsigned int nr_vmcs12_fields __ro_after_init;
>+
>+void __init nested_vmx_setup_vmcs12_fields(void);
> 
> static inline short get_vmcs12_field_offset(unsigned long field)
> {
>diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>index 6b96f7aea20b..e5ad3853f51d 100644
>--- a/arch/x86/kvm/vmx/vmx.c
>+++ b/arch/x86/kvm/vmx/vmx.c
>@@ -8670,6 +8670,8 @@ __init int vmx_hardware_setup(void)
> 	 * can hide/show features based on kvm_cpu_cap_has().
> 	 */
> 	if (nested) {
>+		nested_vmx_setup_vmcs12_fields();
>+
> 		nested_vmx_setup_ctls_msrs(&vmcs_config, vmx_capability.ept);
> 
> 		r = nested_vmx_hardware_setup(kvm_vmx_exit_handlers);
>
>base-commit: 58e10b63777d0aebee2cf4e6c67e1a83e7edbe0f
>-- 
>2.52.0.239.gd5f0c6e74e-goog
>
>