When VMX controls macros are used to set or clear a control bit, make
sure that this bit was checked in setup_vmcs_config() and thus is properly
reflected in vmcs_config.
No functional change intended.
Suggested-by: Sean Christopherson <[email protected]>
Reviewed-by: Maxim Levitsky <[email protected]>
Signed-off-by: Vitaly Kuznetsov <[email protected]>
---
arch/x86/kvm/vmx/vmx.c | 99 +++++++------------------------------
arch/x86/kvm/vmx/vmx.h | 109 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 127 insertions(+), 81 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 566be73c6509..93ca9ff8e641 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2448,7 +2448,6 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
struct vmx_capability *vmx_cap)
{
u32 vmx_msr_low, vmx_msr_high;
- u32 min, opt, min2, opt2;
u32 _pin_based_exec_control = 0;
u32 _cpu_based_exec_control = 0;
u32 _cpu_based_2nd_exec_control = 0;
@@ -2474,28 +2473,10 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
};
memset(vmcs_conf, 0, sizeof(*vmcs_conf));
- min = CPU_BASED_HLT_EXITING |
-#ifdef CONFIG_X86_64
- CPU_BASED_CR8_LOAD_EXITING |
- CPU_BASED_CR8_STORE_EXITING |
-#endif
- CPU_BASED_CR3_LOAD_EXITING |
- CPU_BASED_CR3_STORE_EXITING |
- CPU_BASED_UNCOND_IO_EXITING |
- CPU_BASED_MOV_DR_EXITING |
- CPU_BASED_USE_TSC_OFFSETTING |
- CPU_BASED_MWAIT_EXITING |
- CPU_BASED_MONITOR_EXITING |
- CPU_BASED_INVLPG_EXITING |
- CPU_BASED_RDPMC_EXITING |
- CPU_BASED_INTR_WINDOW_EXITING;
-
- opt = CPU_BASED_TPR_SHADOW |
- CPU_BASED_USE_MSR_BITMAPS |
- CPU_BASED_NMI_WINDOW_EXITING |
- CPU_BASED_ACTIVATE_SECONDARY_CONTROLS |
- CPU_BASED_ACTIVATE_TERTIARY_CONTROLS;
- if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_PROCBASED_CTLS,
+
+ if (adjust_vmx_controls(KVM_REQ_VMX_CPU_BASED_VM_EXEC_CONTROL,
+ KVM_OPT_VMX_CPU_BASED_VM_EXEC_CONTROL,
+ MSR_IA32_VMX_PROCBASED_CTLS,
&_cpu_based_exec_control) < 0)
return -EIO;
#ifdef CONFIG_X86_64
@@ -2504,34 +2485,8 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
~CPU_BASED_CR8_STORE_EXITING;
#endif
if (_cpu_based_exec_control & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS) {
- min2 = 0;
- opt2 = SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
- SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
- SECONDARY_EXEC_WBINVD_EXITING |
- SECONDARY_EXEC_ENABLE_VPID |
- SECONDARY_EXEC_ENABLE_EPT |
- SECONDARY_EXEC_UNRESTRICTED_GUEST |
- SECONDARY_EXEC_PAUSE_LOOP_EXITING |
- SECONDARY_EXEC_DESC |
- SECONDARY_EXEC_ENABLE_RDTSCP |
- SECONDARY_EXEC_ENABLE_INVPCID |
- SECONDARY_EXEC_APIC_REGISTER_VIRT |
- SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
- SECONDARY_EXEC_SHADOW_VMCS |
- SECONDARY_EXEC_XSAVES |
- SECONDARY_EXEC_RDSEED_EXITING |
- SECONDARY_EXEC_RDRAND_EXITING |
- SECONDARY_EXEC_ENABLE_PML |
- SECONDARY_EXEC_TSC_SCALING |
- SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE |
- SECONDARY_EXEC_PT_USE_GPA |
- SECONDARY_EXEC_PT_CONCEAL_VMX |
- SECONDARY_EXEC_ENABLE_VMFUNC |
- SECONDARY_EXEC_BUS_LOCK_DETECTION |
- SECONDARY_EXEC_NOTIFY_VM_EXITING |
- SECONDARY_EXEC_ENCLS_EXITING;
-
- if (adjust_vmx_controls(min2, opt2,
+ if (adjust_vmx_controls(KVM_REQ_VMX_SECONDARY_VM_EXEC_CONTROL,
+ KVM_OPT_VMX_SECONDARY_VM_EXEC_CONTROL,
MSR_IA32_VMX_PROCBASED_CTLS2,
&_cpu_based_2nd_exec_control) < 0)
return -EIO;
@@ -2581,30 +2536,20 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
_cpu_based_2nd_exec_control &= ~SECONDARY_EXEC_ENCLS_EXITING;
if (_cpu_based_exec_control & CPU_BASED_ACTIVATE_TERTIARY_CONTROLS) {
- u64 opt3 = TERTIARY_EXEC_IPI_VIRT;
-
- _cpu_based_3rd_exec_control = adjust_vmx_controls64(opt3,
- MSR_IA32_VMX_PROCBASED_CTLS3);
+ _cpu_based_3rd_exec_control =
+ adjust_vmx_controls64(KVM_OPT_VMX_TERTIARY_VM_EXEC_CONTROL,
+ MSR_IA32_VMX_PROCBASED_CTLS3);
}
- min = VM_EXIT_SAVE_DEBUG_CONTROLS | VM_EXIT_ACK_INTR_ON_EXIT;
-#ifdef CONFIG_X86_64
- min |= VM_EXIT_HOST_ADDR_SPACE_SIZE;
-#endif
- opt = VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL |
- VM_EXIT_LOAD_IA32_PAT |
- VM_EXIT_LOAD_IA32_EFER |
- VM_EXIT_CLEAR_BNDCFGS |
- VM_EXIT_PT_CONCEAL_PIP |
- VM_EXIT_CLEAR_IA32_RTIT_CTL;
- if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_EXIT_CTLS,
+ if (adjust_vmx_controls(KVM_REQ_VMX_VM_EXIT_CONTROLS,
+ KVM_OPT_VMX_VM_EXIT_CONTROLS,
+ MSR_IA32_VMX_EXIT_CTLS,
&_vmexit_control) < 0)
return -EIO;
- min = PIN_BASED_EXT_INTR_MASK | PIN_BASED_NMI_EXITING;
- opt = PIN_BASED_VIRTUAL_NMIS | PIN_BASED_POSTED_INTR |
- PIN_BASED_VMX_PREEMPTION_TIMER;
- if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_PINBASED_CTLS,
+ if (adjust_vmx_controls(KVM_REQ_VMX_PIN_BASED_VM_EXEC_CONTROL,
+ KVM_OPT_VMX_PIN_BASED_VM_EXEC_CONTROL,
+ MSR_IA32_VMX_PINBASED_CTLS,
&_pin_based_exec_control) < 0)
return -EIO;
@@ -2614,17 +2559,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY))
_pin_based_exec_control &= ~PIN_BASED_POSTED_INTR;
- min = VM_ENTRY_LOAD_DEBUG_CONTROLS;
-#ifdef CONFIG_X86_64
- min |= VM_ENTRY_IA32E_MODE;
-#endif
- opt = VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL |
- VM_ENTRY_LOAD_IA32_PAT |
- VM_ENTRY_LOAD_IA32_EFER |
- VM_ENTRY_LOAD_BNDCFGS |
- VM_ENTRY_PT_CONCEAL_PIP |
- VM_ENTRY_LOAD_IA32_RTIT_CTL;
- if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_ENTRY_CTLS,
+ if (adjust_vmx_controls(KVM_REQ_VMX_VM_ENTRY_CONTROLS,
+ KVM_OPT_VMX_VM_ENTRY_CONTROLS,
+ MSR_IA32_VMX_ENTRY_CTLS,
&_vmentry_control) < 0)
return -EIO;
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 286c88e285ea..89eaab3495a6 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -467,6 +467,113 @@ static inline u8 vmx_get_rvi(void)
return vmcs_read16(GUEST_INTR_STATUS) & 0xff;
}
+#define __KVM_REQ_VMX_VM_ENTRY_CONTROLS \
+ (VM_ENTRY_LOAD_DEBUG_CONTROLS)
+#ifdef CONFIG_X86_64
+ #define KVM_REQ_VMX_VM_ENTRY_CONTROLS \
+ (__KVM_REQ_VMX_VM_ENTRY_CONTROLS | \
+ VM_ENTRY_IA32E_MODE)
+#else
+ #define KVM_REQ_VMX_VM_ENTRY_CONTROLS \
+ __KVM_REQ_VMX_VM_ENTRY_CONTROLS
+#endif
+#define KVM_OPT_VMX_VM_ENTRY_CONTROLS \
+ (VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL | \
+ VM_ENTRY_LOAD_IA32_PAT | \
+ VM_ENTRY_LOAD_IA32_EFER | \
+ VM_ENTRY_LOAD_BNDCFGS | \
+ VM_ENTRY_PT_CONCEAL_PIP | \
+ VM_ENTRY_LOAD_IA32_RTIT_CTL)
+
+#define __KVM_REQ_VMX_VM_EXIT_CONTROLS \
+ (VM_EXIT_SAVE_DEBUG_CONTROLS | \
+ VM_EXIT_ACK_INTR_ON_EXIT)
+#ifdef CONFIG_X86_64
+ #define KVM_REQ_VMX_VM_EXIT_CONTROLS \
+ (__KVM_REQ_VMX_VM_EXIT_CONTROLS | \
+ VM_EXIT_HOST_ADDR_SPACE_SIZE)
+#else
+ #define KVM_REQ_VMX_VM_EXIT_CONTROLS \
+ __KVM_REQ_VMX_VM_EXIT_CONTROLS
+#endif
+#define KVM_OPT_VMX_VM_EXIT_CONTROLS \
+ (VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL | \
+ VM_EXIT_LOAD_IA32_PAT | \
+ VM_EXIT_LOAD_IA32_EFER | \
+ VM_EXIT_CLEAR_BNDCFGS | \
+ VM_EXIT_PT_CONCEAL_PIP | \
+ VM_EXIT_CLEAR_IA32_RTIT_CTL)
+
+#define KVM_REQ_VMX_PIN_BASED_VM_EXEC_CONTROL \
+ (PIN_BASED_EXT_INTR_MASK | \
+ PIN_BASED_NMI_EXITING)
+#define KVM_OPT_VMX_PIN_BASED_VM_EXEC_CONTROL \
+ (PIN_BASED_VIRTUAL_NMIS | \
+ PIN_BASED_POSTED_INTR | \
+ PIN_BASED_VMX_PREEMPTION_TIMER)
+
+#define __KVM_REQ_VMX_CPU_BASED_VM_EXEC_CONTROL \
+ (CPU_BASED_HLT_EXITING | \
+ CPU_BASED_CR3_LOAD_EXITING | \
+ CPU_BASED_CR3_STORE_EXITING | \
+ CPU_BASED_UNCOND_IO_EXITING | \
+ CPU_BASED_MOV_DR_EXITING | \
+ CPU_BASED_USE_TSC_OFFSETTING | \
+ CPU_BASED_MWAIT_EXITING | \
+ CPU_BASED_MONITOR_EXITING | \
+ CPU_BASED_INVLPG_EXITING | \
+ CPU_BASED_RDPMC_EXITING | \
+ CPU_BASED_INTR_WINDOW_EXITING)
+
+#ifdef CONFIG_X86_64
+ #define KVM_REQ_VMX_CPU_BASED_VM_EXEC_CONTROL \
+ (__KVM_REQ_VMX_CPU_BASED_VM_EXEC_CONTROL | \
+ CPU_BASED_CR8_LOAD_EXITING | \
+ CPU_BASED_CR8_STORE_EXITING)
+#else
+ #define KVM_REQ_VMX_CPU_BASED_VM_EXEC_CONTROL \
+ __KVM_REQ_VMX_CPU_BASED_VM_EXEC_CONTROL
+#endif
+
+#define KVM_OPT_VMX_CPU_BASED_VM_EXEC_CONTROL \
+ (CPU_BASED_TPR_SHADOW | \
+ CPU_BASED_USE_MSR_BITMAPS | \
+ CPU_BASED_NMI_WINDOW_EXITING | \
+ CPU_BASED_ACTIVATE_SECONDARY_CONTROLS | \
+ CPU_BASED_ACTIVATE_TERTIARY_CONTROLS)
+
+#define KVM_REQ_VMX_SECONDARY_VM_EXEC_CONTROL 0
+#define KVM_OPT_VMX_SECONDARY_VM_EXEC_CONTROL \
+ (SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES | \
+ SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE | \
+ SECONDARY_EXEC_WBINVD_EXITING | \
+ SECONDARY_EXEC_ENABLE_VPID | \
+ SECONDARY_EXEC_ENABLE_EPT | \
+ SECONDARY_EXEC_UNRESTRICTED_GUEST | \
+ SECONDARY_EXEC_PAUSE_LOOP_EXITING | \
+ SECONDARY_EXEC_DESC | \
+ SECONDARY_EXEC_ENABLE_RDTSCP | \
+ SECONDARY_EXEC_ENABLE_INVPCID | \
+ SECONDARY_EXEC_APIC_REGISTER_VIRT | \
+ SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY | \
+ SECONDARY_EXEC_SHADOW_VMCS | \
+ SECONDARY_EXEC_XSAVES | \
+ SECONDARY_EXEC_RDSEED_EXITING | \
+ SECONDARY_EXEC_RDRAND_EXITING | \
+ SECONDARY_EXEC_ENABLE_PML | \
+ SECONDARY_EXEC_TSC_SCALING | \
+ SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE | \
+ SECONDARY_EXEC_PT_USE_GPA | \
+ SECONDARY_EXEC_PT_CONCEAL_VMX | \
+ SECONDARY_EXEC_ENABLE_VMFUNC | \
+ SECONDARY_EXEC_BUS_LOCK_DETECTION | \
+ SECONDARY_EXEC_NOTIFY_VM_EXITING | \
+ SECONDARY_EXEC_ENCLS_EXITING)
+
+#define KVM_REQ_VMX_TERTIARY_VM_EXEC_CONTROL 0
+#define KVM_OPT_VMX_TERTIARY_VM_EXEC_CONTROL \
+ (TERTIARY_EXEC_IPI_VIRT)
+
#define BUILD_CONTROLS_SHADOW(lname, uname, bits) \
static inline void lname##_controls_set(struct vcpu_vmx *vmx, u##bits val) \
{ \
@@ -485,10 +592,12 @@ static inline u##bits lname##_controls_get(struct vcpu_vmx *vmx) \
} \
static inline void lname##_controls_setbit(struct vcpu_vmx *vmx, u##bits val) \
{ \
+ BUILD_BUG_ON(!(val & (KVM_REQ_VMX_##uname | KVM_OPT_VMX_##uname))); \
lname##_controls_set(vmx, lname##_controls_get(vmx) | val); \
} \
static inline void lname##_controls_clearbit(struct vcpu_vmx *vmx, u##bits val) \
{ \
+ BUILD_BUG_ON(!(val & (KVM_REQ_VMX_##uname | KVM_OPT_VMX_##uname))); \
lname##_controls_set(vmx, lname##_controls_get(vmx) & ~val); \
}
BUILD_CONTROLS_SHADOW(vm_entry, VM_ENTRY_CONTROLS, 32)
--
2.35.3
On Thu, Jul 14, 2022, Vitaly Kuznetsov wrote:
> @@ -2581,30 +2536,20 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
> _cpu_based_2nd_exec_control &= ~SECONDARY_EXEC_ENCLS_EXITING;
>
> if (_cpu_based_exec_control & CPU_BASED_ACTIVATE_TERTIARY_CONTROLS) {
Curly braces no longer necessary.
> - u64 opt3 = TERTIARY_EXEC_IPI_VIRT;
> -
> - _cpu_based_3rd_exec_control = adjust_vmx_controls64(opt3,
> - MSR_IA32_VMX_PROCBASED_CTLS3);
> + _cpu_based_3rd_exec_control =
> + adjust_vmx_controls64(KVM_OPT_VMX_TERTIARY_VM_EXEC_CONTROL,
> + MSR_IA32_VMX_PROCBASED_CTLS3);
Please align indentation.
if (_cpu_based_exec_control & CPU_BASED_ACTIVATE_TERTIARY_CONTROLS)
_cpu_based_3rd_exec_control =
adjust_vmx_controls64(KVM_OPT_VMX_TERTIARY_VM_EXEC_CONTROL,
MSR_IA32_VMX_PROCBASED_CTLS3);
> }
>
> - min = VM_EXIT_SAVE_DEBUG_CONTROLS | VM_EXIT_ACK_INTR_ON_EXIT;
> -#ifdef CONFIG_X86_64
> - min |= VM_EXIT_HOST_ADDR_SPACE_SIZE;
> -#endif
> - opt = VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL |
> - VM_EXIT_LOAD_IA32_PAT |
> - VM_EXIT_LOAD_IA32_EFER |
> - VM_EXIT_CLEAR_BNDCFGS |
> - VM_EXIT_PT_CONCEAL_PIP |
> - VM_EXIT_CLEAR_IA32_RTIT_CTL;
> - if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_EXIT_CTLS,
> + if (adjust_vmx_controls(KVM_REQ_VMX_VM_EXIT_CONTROLS,
I think we should spell out REQUIRED and OPTIONAL, almost all of the usage puts
them on their own lines, i.e. longer names doesn't change much. "OPT" is fine,
but "REQ" already means "REQUEST" in KVM, i.e. I mentally read this as
"KVM requested controls", which is quite different from "KVM required controls".
> + KVM_OPT_VMX_VM_EXIT_CONTROLS,
> + MSR_IA32_VMX_EXIT_CTLS,
> &_vmexit_control) < 0)
> return -EIO;
>
> - min = PIN_BASED_EXT_INTR_MASK | PIN_BASED_NMI_EXITING;
> - opt = PIN_BASED_VIRTUAL_NMIS | PIN_BASED_POSTED_INTR |
> - PIN_BASED_VMX_PREEMPTION_TIMER;
> - if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_PINBASED_CTLS,
> + if (adjust_vmx_controls(KVM_REQ_VMX_PIN_BASED_VM_EXEC_CONTROL,
> + KVM_OPT_VMX_PIN_BASED_VM_EXEC_CONTROL,
> + MSR_IA32_VMX_PINBASED_CTLS,
> &_pin_based_exec_control) < 0)
I vote to opportunistically (or in a prep patch) drop the silly "< 0" check, it's
pointless and makes the code unnecessarily difficult to follow.
And at that point, I also think it makes sense to move the pointer passing to the
same line as the MSR definition, even if one or two lines run a bit long:
if (adjust_vmx_controls(KVM_REQUIRED_VMX_VM_ENTRY_CONTROLS,
KVM_OPTIONAL_VMX_VM_ENTRY_CONTROLS,
MSR_IA32_VMX_ENTRY_CTLS, &_vmentry_control))
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 286c88e285ea..89eaab3495a6 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -467,6 +467,113 @@ static inline u8 vmx_get_rvi(void)
> return vmcs_read16(GUEST_INTR_STATUS) & 0xff;
> }
>
> +#define __KVM_REQ_VMX_VM_ENTRY_CONTROLS \
> + (VM_ENTRY_LOAD_DEBUG_CONTROLS)
> +#ifdef CONFIG_X86_64
> + #define KVM_REQ_VMX_VM_ENTRY_CONTROLS \
> + (__KVM_REQ_VMX_VM_ENTRY_CONTROLS | \
> + VM_ENTRY_IA32E_MODE)
Align indentation (more obvious below).
> +#else
> + #define KVM_REQ_VMX_VM_ENTRY_CONTROLS \
> + __KVM_REQ_VMX_VM_ENTRY_CONTROLS
> +#endif
> +#define KVM_OPT_VMX_VM_ENTRY_CONTROLS \
> + (VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL | \
> + VM_ENTRY_LOAD_IA32_PAT | \
> + VM_ENTRY_LOAD_IA32_EFER | \
> + VM_ENTRY_LOAD_BNDCFGS | \
> + VM_ENTRY_PT_CONCEAL_PIP | \
> + VM_ENTRY_LOAD_IA32_RTIT_CTL)
Align inside the paranthesis so that the control names all line up (goes for
everything in this file).
#define KVM_OPT_VMX_VM_ENTRY_CONTROLS \
(VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL | \
VM_ENTRY_LOAD_IA32_PAT | \
VM_ENTRY_LOAD_IA32_EFER | \
VM_ENTRY_LOAD_BNDCFGS | \
VM_ENTRY_PT_CONCEAL_PIP | \
VM_ENTRY_LOAD_IA32_RTIT_CTL)
> +#define KVM_REQ_VMX_TERTIARY_VM_EXEC_CONTROL 0
> +#define KVM_OPT_VMX_TERTIARY_VM_EXEC_CONTROL \
> + (TERTIARY_EXEC_IPI_VIRT)
> +
> #define BUILD_CONTROLS_SHADOW(lname, uname, bits) \
> static inline void lname##_controls_set(struct vcpu_vmx *vmx, u##bits val) \
> { \
> @@ -485,10 +592,12 @@ static inline u##bits lname##_controls_get(struct vcpu_vmx *vmx) \
> } \
> static inline void lname##_controls_setbit(struct vcpu_vmx *vmx, u##bits val) \
I suspect these need to be __always_inline, otherwise various sanitizers might
cause these to be out of line and break the build due to @val not being a
compile-time constant.
> { \
> + BUILD_BUG_ON(!(val & (KVM_REQ_VMX_##uname | KVM_OPT_VMX_##uname))); \
> lname##_controls_set(vmx, lname##_controls_get(vmx) | val); \
> } \
> static inline void lname##_controls_clearbit(struct vcpu_vmx *vmx, u##bits val) \
> { \
> + BUILD_BUG_ON(!(val & (KVM_REQ_VMX_##uname | KVM_OPT_VMX_##uname))); \
> lname##_controls_set(vmx, lname##_controls_get(vmx) & ~val); \
> }
> BUILD_CONTROLS_SHADOW(vm_entry, VM_ENTRY_CONTROLS, 32)
> --
> 2.35.3
>
On Thu, Jul 14, 2022, Vitaly Kuznetsov wrote:
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 286c88e285ea..89eaab3495a6 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -467,6 +467,113 @@ static inline u8 vmx_get_rvi(void)
> return vmcs_read16(GUEST_INTR_STATUS) & 0xff;
> }
>
> +#define __KVM_REQ_VMX_VM_ENTRY_CONTROLS \
> + (VM_ENTRY_LOAD_DEBUG_CONTROLS)
> +#ifdef CONFIG_X86_64
> + #define KVM_REQ_VMX_VM_ENTRY_CONTROLS \
> + (__KVM_REQ_VMX_VM_ENTRY_CONTROLS | \
> + VM_ENTRY_IA32E_MODE)
This breaks 32-bit builds, but at least we know the assert works!
vmx_set_efer() toggles VM_ENTRY_IA32E_MODE without a CONFIG_X86_64 guard. That
should be easy enough to fix since KVM should never allow EFER_LMA. Compile
tested patch at the bottom.
More problematic is that clang-13 doesn't like the new asserts, and even worse gives
a very cryptic error. I don't have bandwidth to look into this at the moment, and
probably won't next week either.
ERROR: modpost: "__compiletime_assert_533" [arch/x86/kvm/kvm-intel.ko] undefined!
ERROR: modpost: "__compiletime_assert_531" [arch/x86/kvm/kvm-intel.ko] undefined!
ERROR: modpost: "__compiletime_assert_532" [arch/x86/kvm/kvm-intel.ko] undefined!
ERROR: modpost: "__compiletime_assert_530" [arch/x86/kvm/kvm-intel.ko] undefined!
make[2]: *** [scripts/Makefile.modpost:128: modules-only.symvers] Error 1
make[1]: *** [Makefile:1753: modules] Error 2
make[1]: *** Waiting for unfinished jobs....
> +#else
> + #define KVM_REQ_VMX_VM_ENTRY_CONTROLS \
> + __KVM_REQ_VMX_VM_ENTRY_CONTROLS
> +#endif
EFER.LMA patch, compile tested only.
---
From: Sean Christopherson <[email protected]>
Date: Fri, 22 Jul 2022 18:26:21 +0000
Subject: [PATCH] KVM: VMX: Don't toggle VM_ENTRY_IA32E_MODE for 32-bit
kernels/KVM
Don't toggle VM_ENTRY_IA32E_MODE in 32-bit kernels/KVM and instead bug
the VM if KVM attempts to run the guest with EFER.LMA=1. KVM doesn't
support running 64-bit guests with 32-bit hosts.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/vmx/vmx.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index bff97babf381..8623607e596d 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2894,10 +2894,15 @@ int vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer)
return 0;
vcpu->arch.efer = efer;
+#ifdef CONFIG_X86_64
if (efer & EFER_LMA)
vm_entry_controls_setbit(vmx, VM_ENTRY_IA32E_MODE);
else
vm_entry_controls_clearbit(vmx, VM_ENTRY_IA32E_MODE);
+#else
+ if (KVM_BUG_ON(efer & EFER_LMA, vcpu->kvm))
+ return 1;
+#endif
vmx_setup_uret_msrs(vmx);
return 0;
base-commit: e22e2665637151a321433b2bb705f5c3b8da40bc
--
On Fri, Jul 22, 2022 at 06:33:27PM +0000, Sean Christopherson wrote:
> On Thu, Jul 14, 2022, Vitaly Kuznetsov wrote:
> > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > index 286c88e285ea..89eaab3495a6 100644
> > --- a/arch/x86/kvm/vmx/vmx.h
> > +++ b/arch/x86/kvm/vmx/vmx.h
> > @@ -467,6 +467,113 @@ static inline u8 vmx_get_rvi(void)
> > return vmcs_read16(GUEST_INTR_STATUS) & 0xff;
> > }
> >
> > +#define __KVM_REQ_VMX_VM_ENTRY_CONTROLS \
> > + (VM_ENTRY_LOAD_DEBUG_CONTROLS)
> > +#ifdef CONFIG_X86_64
> > + #define KVM_REQ_VMX_VM_ENTRY_CONTROLS \
> > + (__KVM_REQ_VMX_VM_ENTRY_CONTROLS | \
> > + VM_ENTRY_IA32E_MODE)
>
> This breaks 32-bit builds, but at least we know the assert works!
>
> vmx_set_efer() toggles VM_ENTRY_IA32E_MODE without a CONFIG_X86_64 guard. That
> should be easy enough to fix since KVM should never allow EFER_LMA. Compile
> tested patch at the bottom.
>
> More problematic is that clang-13 doesn't like the new asserts, and even worse gives
> a very cryptic error. I don't have bandwidth to look into this at the moment, and
> probably won't next week either.
>
> ERROR: modpost: "__compiletime_assert_533" [arch/x86/kvm/kvm-intel.ko] undefined!
> ERROR: modpost: "__compiletime_assert_531" [arch/x86/kvm/kvm-intel.ko] undefined!
> ERROR: modpost: "__compiletime_assert_532" [arch/x86/kvm/kvm-intel.ko] undefined!
> ERROR: modpost: "__compiletime_assert_530" [arch/x86/kvm/kvm-intel.ko] undefined!
> make[2]: *** [scripts/Makefile.modpost:128: modules-only.symvers] Error 1
> make[1]: *** [Makefile:1753: modules] Error 2
> make[1]: *** Waiting for unfinished jobs....
clang-14 added support for the error and warning attributes, which makes
the BUILD_BUG_ON failures look like GCC. With allmodconfig, this
becomes:
In file included from ../arch/x86/kvm/vmx/vmx.c:61:
In file included from ../arch/x86/kvm/vmx/nested.h:7:
../arch/x86/kvm/vmx/vmx.h:610:1: error: call to __compiletime_assert_1135 declared with 'error' attribute: BUILD_BUG_ON failed: !(val & (KVM_REQ_VMX_VM_ENTRY_CONTROLS | KVM_OPT_VMX_VM_ENTRY_CONTROLS))
BUILD_CONTROLS_SHADOW(vm_entry, VM_ENTRY_CONTROLS, 32)
^
../arch/x86/kvm/vmx/vmx.h:602:2: note: expanded from macro 'BUILD_CONTROLS_SHADOW'
BUILD_BUG_ON(!(val & (KVM_REQ_VMX_##uname | KVM_OPT_VMX_##uname))); \
^
../include/linux/build_bug.h:50:2: note: expanded from macro 'BUILD_BUG_ON'
BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
^
../include/linux/build_bug.h:39:37: note: expanded from macro 'BUILD_BUG_ON_MSG'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^
note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
../include/linux/compiler_types.h:340:2: note: expanded from macro '_compiletime_assert'
__compiletime_assert(condition, msg, prefix, suffix)
^
../include/linux/compiler_types.h:333:4: note: expanded from macro '__compiletime_assert'
prefix ## suffix(); \
^
<scratch space>:259:1: note: expanded from here
__compiletime_assert_1135
^
In file included from ../arch/x86/kvm/vmx/vmx.c:61:
In file included from ../arch/x86/kvm/vmx/nested.h:7:
../arch/x86/kvm/vmx/vmx.h:610:1: error: call to __compiletime_assert_1136 declared with 'error' attribute: BUILD_BUG_ON failed: !(val & (KVM_REQ_VMX_VM_ENTRY_CONTROLS | KVM_OPT_VMX_VM_ENTRY_CONTROLS))
../arch/x86/kvm/vmx/vmx.h:607:2: note: expanded from macro 'BUILD_CONTROLS_SHADOW'
BUILD_BUG_ON(!(val & (KVM_REQ_VMX_##uname | KVM_OPT_VMX_##uname))); \
^
../include/linux/build_bug.h:50:2: note: expanded from macro 'BUILD_BUG_ON'
BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
^
../include/linux/build_bug.h:39:37: note: expanded from macro 'BUILD_BUG_ON_MSG'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^
note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
../include/linux/compiler_types.h:340:2: note: expanded from macro '_compiletime_assert'
__compiletime_assert(condition, msg, prefix, suffix)
^
../include/linux/compiler_types.h:333:4: note: expanded from macro '__compiletime_assert'
prefix ## suffix(); \
^
<scratch space>:10:1: note: expanded from here
__compiletime_assert_1136
^
In file included from ../arch/x86/kvm/vmx/vmx.c:61:
In file included from ../arch/x86/kvm/vmx/nested.h:7:
../arch/x86/kvm/vmx/vmx.h:611:1: error: call to __compiletime_assert_1137 declared with 'error' attribute: BUILD_BUG_ON failed: !(val & (KVM_REQ_VMX_VM_EXIT_CONTROLS | KVM_OPT_VMX_VM_EXIT_CONTROLS))
BUILD_CONTROLS_SHADOW(vm_exit, VM_EXIT_CONTROLS, 32)
^
../arch/x86/kvm/vmx/vmx.h:602:2: note: expanded from macro 'BUILD_CONTROLS_SHADOW'
BUILD_BUG_ON(!(val & (KVM_REQ_VMX_##uname | KVM_OPT_VMX_##uname))); \
^
../include/linux/build_bug.h:50:2: note: expanded from macro 'BUILD_BUG_ON'
BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
^
../include/linux/build_bug.h:39:37: note: expanded from macro 'BUILD_BUG_ON_MSG'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^
note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
../include/linux/compiler_types.h:340:2: note: expanded from macro '_compiletime_assert'
__compiletime_assert(condition, msg, prefix, suffix)
^
../include/linux/compiler_types.h:333:4: note: expanded from macro '__compiletime_assert'
prefix ## suffix(); \
^
<scratch space>:30:1: note: expanded from here
__compiletime_assert_1137
^
In file included from ../arch/x86/kvm/vmx/vmx.c:61:
In file included from ../arch/x86/kvm/vmx/nested.h:7:
../arch/x86/kvm/vmx/vmx.h:611:1: error: call to __compiletime_assert_1138 declared with 'error' attribute: BUILD_BUG_ON failed: !(val & (KVM_REQ_VMX_VM_EXIT_CONTROLS | KVM_OPT_VMX_VM_EXIT_CONTROLS))
../arch/x86/kvm/vmx/vmx.h:607:2: note: expanded from macro 'BUILD_CONTROLS_SHADOW'
BUILD_BUG_ON(!(val & (KVM_REQ_VMX_##uname | KVM_OPT_VMX_##uname))); \
^
../include/linux/build_bug.h:50:2: note: expanded from macro 'BUILD_BUG_ON'
BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
^
../include/linux/build_bug.h:39:37: note: expanded from macro 'BUILD_BUG_ON_MSG'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^
note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
../include/linux/compiler_types.h:340:2: note: expanded from macro '_compiletime_assert'
__compiletime_assert(condition, msg, prefix, suffix)
^
../include/linux/compiler_types.h:333:4: note: expanded from macro '__compiletime_assert'
prefix ## suffix(); \
^
<scratch space>:40:1: note: expanded from here
__compiletime_assert_1138
^
4 errors generated.
As you mentioned in the other comment on this patch, the 'inline'
keyword should be '__always_inline' in the BUILD_CONTROLS_SHADOW macro
and a couple of other functions need it for BUILD_BUG_ON to see the
value all the way through the call chain. The following diff resolves
those errors for me, hopefully it is useful!
Cheers,
Nathan
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 4ce7ed835e06..b97ed63ece56 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -790,7 +790,7 @@ static bool msr_write_intercepted(struct vcpu_vmx *vmx, u32 msr)
MSR_IA32_SPEC_CTRL);
}
-static void clear_atomic_switch_msr_special(struct vcpu_vmx *vmx,
+static __always_inline void clear_atomic_switch_msr_special(struct vcpu_vmx *vmx,
unsigned long entry, unsigned long exit)
{
vm_entry_controls_clearbit(vmx, entry);
@@ -848,7 +848,7 @@ static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, m->host.nr);
}
-static void add_atomic_switch_msr_special(struct vcpu_vmx *vmx,
+static __always_inline void add_atomic_switch_msr_special(struct vcpu_vmx *vmx,
unsigned long entry, unsigned long exit,
unsigned long guest_val_vmcs, unsigned long host_val_vmcs,
u64 guest_val, u64 host_val)
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 758f80c41beb..acefa5b5e1b9 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -597,12 +597,12 @@ static inline u##bits lname##_controls_get(struct vcpu_vmx *vmx) \
{ \
return __##lname##_controls_get(vmx->loaded_vmcs); \
} \
-static inline void lname##_controls_setbit(struct vcpu_vmx *vmx, u##bits val) \
+static __always_inline void lname##_controls_setbit(struct vcpu_vmx *vmx, u##bits val) \
{ \
BUILD_BUG_ON(!(val & (KVM_REQ_VMX_##uname | KVM_OPT_VMX_##uname))); \
lname##_controls_set(vmx, lname##_controls_get(vmx) | val); \
} \
-static inline void lname##_controls_clearbit(struct vcpu_vmx *vmx, u##bits val) \
+static __always_inline void lname##_controls_clearbit(struct vcpu_vmx *vmx, u##bits val) \
{ \
BUILD_BUG_ON(!(val & (KVM_REQ_VMX_##uname | KVM_OPT_VMX_##uname))); \
lname##_controls_set(vmx, lname##_controls_get(vmx) & ~val); \
> > +#else
> > + #define KVM_REQ_VMX_VM_ENTRY_CONTROLS \
> > + __KVM_REQ_VMX_VM_ENTRY_CONTROLS
> > +#endif
>
> EFER.LMA patch, compile tested only.
>
> ---
> From: Sean Christopherson <[email protected]>
> Date: Fri, 22 Jul 2022 18:26:21 +0000
> Subject: [PATCH] KVM: VMX: Don't toggle VM_ENTRY_IA32E_MODE for 32-bit
> kernels/KVM
>
> Don't toggle VM_ENTRY_IA32E_MODE in 32-bit kernels/KVM and instead bug
> the VM if KVM attempts to run the guest with EFER.LMA=1. KVM doesn't
> support running 64-bit guests with 32-bit hosts.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/vmx/vmx.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index bff97babf381..8623607e596d 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2894,10 +2894,15 @@ int vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer)
> return 0;
>
> vcpu->arch.efer = efer;
> +#ifdef CONFIG_X86_64
> if (efer & EFER_LMA)
> vm_entry_controls_setbit(vmx, VM_ENTRY_IA32E_MODE);
> else
> vm_entry_controls_clearbit(vmx, VM_ENTRY_IA32E_MODE);
> +#else
> + if (KVM_BUG_ON(efer & EFER_LMA, vcpu->kvm))
> + return 1;
> +#endif
>
> vmx_setup_uret_msrs(vmx);
> return 0;
>
> base-commit: e22e2665637151a321433b2bb705f5c3b8da40bc
> --
>
On Fri, Jul 22, 2022, Nathan Chancellor wrote:
> On Fri, Jul 22, 2022 at 06:33:27PM +0000, Sean Christopherson wrote:
> > On Thu, Jul 14, 2022, Vitaly Kuznetsov wrote:
> > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > > index 286c88e285ea..89eaab3495a6 100644
> > > --- a/arch/x86/kvm/vmx/vmx.h
> > > +++ b/arch/x86/kvm/vmx/vmx.h
> > > @@ -467,6 +467,113 @@ static inline u8 vmx_get_rvi(void)
> > > return vmcs_read16(GUEST_INTR_STATUS) & 0xff;
> > > }
> > >
> > > +#define __KVM_REQ_VMX_VM_ENTRY_CONTROLS \
> > > + (VM_ENTRY_LOAD_DEBUG_CONTROLS)
> > > +#ifdef CONFIG_X86_64
> > > + #define KVM_REQ_VMX_VM_ENTRY_CONTROLS \
> > > + (__KVM_REQ_VMX_VM_ENTRY_CONTROLS | \
> > > + VM_ENTRY_IA32E_MODE)
> >
> > This breaks 32-bit builds, but at least we know the assert works!
> >
> > vmx_set_efer() toggles VM_ENTRY_IA32E_MODE without a CONFIG_X86_64 guard. That
> > should be easy enough to fix since KVM should never allow EFER_LMA. Compile
> > tested patch at the bottom.
> >
> > More problematic is that clang-13 doesn't like the new asserts, and even worse gives
> > a very cryptic error. I don't have bandwidth to look into this at the moment, and
> > probably won't next week either.
> >
> > ERROR: modpost: "__compiletime_assert_533" [arch/x86/kvm/kvm-intel.ko] undefined!
> > ERROR: modpost: "__compiletime_assert_531" [arch/x86/kvm/kvm-intel.ko] undefined!
> > ERROR: modpost: "__compiletime_assert_532" [arch/x86/kvm/kvm-intel.ko] undefined!
> > ERROR: modpost: "__compiletime_assert_530" [arch/x86/kvm/kvm-intel.ko] undefined!
> > make[2]: *** [scripts/Makefile.modpost:128: modules-only.symvers] Error 1
> > make[1]: *** [Makefile:1753: modules] Error 2
> > make[1]: *** Waiting for unfinished jobs....
>
> clang-14 added support for the error and warning attributes, which makes
> the BUILD_BUG_ON failures look like GCC. With allmodconfig, this
> becomes:
...
> As you mentioned in the other comment on this patch, the 'inline'
> keyword should be '__always_inline' in the BUILD_CONTROLS_SHADOW macro
> and a couple of other functions need it for BUILD_BUG_ON to see the
> value all the way through the call chain. The following diff resolves
> those errors for me, hopefully it is useful!
Thanks a ton! Y'all are like a benevolent Beetlejuice, one needs only to mention
"clang" and you show up and solve the problem :-)
> Cheers,
> Nathan
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 4ce7ed835e06..b97ed63ece56 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -790,7 +790,7 @@ static bool msr_write_intercepted(struct vcpu_vmx *vmx, u32 msr)
> MSR_IA32_SPEC_CTRL);
> }
>
> -static void clear_atomic_switch_msr_special(struct vcpu_vmx *vmx,
> +static __always_inline void clear_atomic_switch_msr_special(struct vcpu_vmx *vmx,
> unsigned long entry, unsigned long exit)
> {
> vm_entry_controls_clearbit(vmx, entry);
> @@ -848,7 +848,7 @@ static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
> vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, m->host.nr);
> }
>
> -static void add_atomic_switch_msr_special(struct vcpu_vmx *vmx,
> +static __always_inline void add_atomic_switch_msr_special(struct vcpu_vmx *vmx,
> unsigned long entry, unsigned long exit,
> unsigned long guest_val_vmcs, unsigned long host_val_vmcs,
> u64 guest_val, u64 host_val)
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 758f80c41beb..acefa5b5e1b9 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -597,12 +597,12 @@ static inline u##bits lname##_controls_get(struct vcpu_vmx *vmx) \
> { \
> return __##lname##_controls_get(vmx->loaded_vmcs); \
> } \
> -static inline void lname##_controls_setbit(struct vcpu_vmx *vmx, u##bits val) \
> +static __always_inline void lname##_controls_setbit(struct vcpu_vmx *vmx, u##bits val) \
> { \
> BUILD_BUG_ON(!(val & (KVM_REQ_VMX_##uname | KVM_OPT_VMX_##uname))); \
> lname##_controls_set(vmx, lname##_controls_get(vmx) | val); \
> } \
> -static inline void lname##_controls_clearbit(struct vcpu_vmx *vmx, u##bits val) \
> +static __always_inline void lname##_controls_clearbit(struct vcpu_vmx *vmx, u##bits val) \
> { \
> BUILD_BUG_ON(!(val & (KVM_REQ_VMX_##uname | KVM_OPT_VMX_##uname))); \
> lname##_controls_set(vmx, lname##_controls_get(vmx) & ~val); \
>
> > > +#else
> > > + #define KVM_REQ_VMX_VM_ENTRY_CONTROLS \
> > > + __KVM_REQ_VMX_VM_ENTRY_CONTROLS
> > > +#endif
On Fri, Jul 22, 2022 at 09:38:47PM +0000, Sean Christopherson wrote:
> On Fri, Jul 22, 2022, Nathan Chancellor wrote:
> > On Fri, Jul 22, 2022 at 06:33:27PM +0000, Sean Christopherson wrote:
> > > On Thu, Jul 14, 2022, Vitaly Kuznetsov wrote:
> > > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > > > index 286c88e285ea..89eaab3495a6 100644
> > > > --- a/arch/x86/kvm/vmx/vmx.h
> > > > +++ b/arch/x86/kvm/vmx/vmx.h
> > > > @@ -467,6 +467,113 @@ static inline u8 vmx_get_rvi(void)
> > > > return vmcs_read16(GUEST_INTR_STATUS) & 0xff;
> > > > }
> > > >
> > > > +#define __KVM_REQ_VMX_VM_ENTRY_CONTROLS \
> > > > + (VM_ENTRY_LOAD_DEBUG_CONTROLS)
> > > > +#ifdef CONFIG_X86_64
> > > > + #define KVM_REQ_VMX_VM_ENTRY_CONTROLS \
> > > > + (__KVM_REQ_VMX_VM_ENTRY_CONTROLS | \
> > > > + VM_ENTRY_IA32E_MODE)
> > >
> > > This breaks 32-bit builds, but at least we know the assert works!
> > >
> > > vmx_set_efer() toggles VM_ENTRY_IA32E_MODE without a CONFIG_X86_64 guard. That
> > > should be easy enough to fix since KVM should never allow EFER_LMA. Compile
> > > tested patch at the bottom.
> > >
> > > More problematic is that clang-13 doesn't like the new asserts, and even worse gives
> > > a very cryptic error. I don't have bandwidth to look into this at the moment, and
> > > probably won't next week either.
> > >
> > > ERROR: modpost: "__compiletime_assert_533" [arch/x86/kvm/kvm-intel.ko] undefined!
> > > ERROR: modpost: "__compiletime_assert_531" [arch/x86/kvm/kvm-intel.ko] undefined!
> > > ERROR: modpost: "__compiletime_assert_532" [arch/x86/kvm/kvm-intel.ko] undefined!
> > > ERROR: modpost: "__compiletime_assert_530" [arch/x86/kvm/kvm-intel.ko] undefined!
> > > make[2]: *** [scripts/Makefile.modpost:128: modules-only.symvers] Error 1
> > > make[1]: *** [Makefile:1753: modules] Error 2
> > > make[1]: *** Waiting for unfinished jobs....
> >
> > clang-14 added support for the error and warning attributes, which makes
> > the BUILD_BUG_ON failures look like GCC. With allmodconfig, this
> > becomes:
>
> ...
>
> > As you mentioned in the other comment on this patch, the 'inline'
> > keyword should be '__always_inline' in the BUILD_CONTROLS_SHADOW macro
> > and a couple of other functions need it for BUILD_BUG_ON to see the
> > value all the way through the call chain. The following diff resolves
> > those errors for me, hopefully it is useful!
>
> Thanks a ton! Y'all are like a benevolent Beetlejuice, one needs only to mention
> "clang" and you show up and solve the problem :-)
Praise be to the mightly lei and its filters :)
FWIW, if you ever have a question about clang's behavior or any errors,
please feel free to cc [email protected], we're always happy to look
into things so that clang stays well supported upstream (and thank you
for verifying KVM changes with it!).
Cheers,
Nathan
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 4ce7ed835e06..b97ed63ece56 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -790,7 +790,7 @@ static bool msr_write_intercepted(struct vcpu_vmx *vmx, u32 msr)
> > MSR_IA32_SPEC_CTRL);
> > }
> >
> > -static void clear_atomic_switch_msr_special(struct vcpu_vmx *vmx,
> > +static __always_inline void clear_atomic_switch_msr_special(struct vcpu_vmx *vmx,
> > unsigned long entry, unsigned long exit)
> > {
> > vm_entry_controls_clearbit(vmx, entry);
> > @@ -848,7 +848,7 @@ static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
> > vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, m->host.nr);
> > }
> >
> > -static void add_atomic_switch_msr_special(struct vcpu_vmx *vmx,
> > +static __always_inline void add_atomic_switch_msr_special(struct vcpu_vmx *vmx,
> > unsigned long entry, unsigned long exit,
> > unsigned long guest_val_vmcs, unsigned long host_val_vmcs,
> > u64 guest_val, u64 host_val)
> > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > index 758f80c41beb..acefa5b5e1b9 100644
> > --- a/arch/x86/kvm/vmx/vmx.h
> > +++ b/arch/x86/kvm/vmx/vmx.h
> > @@ -597,12 +597,12 @@ static inline u##bits lname##_controls_get(struct vcpu_vmx *vmx) \
> > { \
> > return __##lname##_controls_get(vmx->loaded_vmcs); \
> > } \
> > -static inline void lname##_controls_setbit(struct vcpu_vmx *vmx, u##bits val) \
> > +static __always_inline void lname##_controls_setbit(struct vcpu_vmx *vmx, u##bits val) \
> > { \
> > BUILD_BUG_ON(!(val & (KVM_REQ_VMX_##uname | KVM_OPT_VMX_##uname))); \
> > lname##_controls_set(vmx, lname##_controls_get(vmx) | val); \
> > } \
> > -static inline void lname##_controls_clearbit(struct vcpu_vmx *vmx, u##bits val) \
> > +static __always_inline void lname##_controls_clearbit(struct vcpu_vmx *vmx, u##bits val) \
> > { \
> > BUILD_BUG_ON(!(val & (KVM_REQ_VMX_##uname | KVM_OPT_VMX_##uname))); \
> > lname##_controls_set(vmx, lname##_controls_get(vmx) & ~val); \
> >
> > > > +#else
> > > > + #define KVM_REQ_VMX_VM_ENTRY_CONTROLS \
> > > > + __KVM_REQ_VMX_VM_ENTRY_CONTROLS
> > > > +#endif
On 7/22/22 20:33, Sean Christopherson wrote:
> ERROR: modpost: "__compiletime_assert_533" [arch/x86/kvm/kvm-intel.ko] undefined!
> ERROR: modpost: "__compiletime_assert_531" [arch/x86/kvm/kvm-intel.ko] undefined!
> ERROR: modpost: "__compiletime_assert_532" [arch/x86/kvm/kvm-intel.ko] undefined!
> ERROR: modpost: "__compiletime_assert_530" [arch/x86/kvm/kvm-intel.ko] undefined!
> make[2]: *** [scripts/Makefile.modpost:128: modules-only.symvers] Error 1
> make[1]: *** [Makefile:1753: modules] Error 2
> make[1]: *** Waiting for unfinished jobs....
I think it comes from
static void add_atomic_switch_msr_special(struct vcpu_vmx *vmx,
unsigned long entry, unsigned long exit,
unsigned long guest_val_vmcs, unsigned long host_val_vmcs,
u64 guest_val, u64 host_val)
{
vmcs_write64(guest_val_vmcs, guest_val);
if (host_val_vmcs != HOST_IA32_EFER)
vmcs_write64(host_val_vmcs, host_val);
vm_entry_controls_setbit(vmx, entry);
vm_exit_controls_setbit(vmx, exit);
}
and it can be fixed just with __always_inline.
Paolo