Re: [PATCH v4 15/25] KVM: VMX: Extend VMX controls macro shenanigans

From: Sean Christopherson <seanjc@google.com>
To: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
	Anirudh Rayabharam <anrayabh@linux.microsoft.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>,
	Maxim Levitsky <mlevitsk@redhat.com>,
	linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 15/25] KVM: VMX: Extend VMX controls macro shenanigans
Date: Thu, 21 Jul 2022 22:28:05 +0000	[thread overview]
Message-ID: <YtnS9fH4cFpADvzu@google.com> (raw)
In-Reply-To: <20220714091327.1085353-16-vkuznets@redhat.com>

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
> 