linux-kernel - Re: [PATCH v3 05/15] KVM: VMX: Rename XSAVES control to follow KVM's preferred "ENABLE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87y1ib5sta.fsf@redhat.com>
Date:   Wed, 16 Aug 2023 09:50:57 +0200
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Zeng Guang <guang.zeng@...el.com>,
        Yuan Yao <yuan.yao@...el.com>
Subject: Re: [PATCH v3 05/15] KVM: VMX: Rename XSAVES control to follow
 KVM's preferred "ENABLE_XYZ"

Sean Christopherson <seanjc@...gle.com> writes:

> Rename the XSAVES secondary execution control to follow KVM's preferred
> style so that XSAVES related logic can use common macros that depend on
> KVM's preferred style.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/include/asm/vmx.h      | 2 +-
>  arch/x86/kvm/vmx/capabilities.h | 2 +-
>  arch/x86/kvm/vmx/hyperv.c       | 2 +-

Reviewed-by: Vitaly Kuznetsov <vkuznets@...hat.com>

with a minor comment

>  arch/x86/kvm/vmx/nested.c       | 6 +++---
>  arch/x86/kvm/vmx/nested.h       | 2 +-
>  arch/x86/kvm/vmx/vmx.c          | 2 +-
>  arch/x86/kvm/vmx/vmx.h          | 2 +-
>  7 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index 0d02c4aafa6f..0e73616b82f3 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -71,7 +71,7 @@
>  #define SECONDARY_EXEC_RDSEED_EXITING		VMCS_CONTROL_BIT(RDSEED_EXITING)
>  #define SECONDARY_EXEC_ENABLE_PML               VMCS_CONTROL_BIT(PAGE_MOD_LOGGING)
>  #define SECONDARY_EXEC_PT_CONCEAL_VMX		VMCS_CONTROL_BIT(PT_CONCEAL_VMX)
> -#define SECONDARY_EXEC_XSAVES			VMCS_CONTROL_BIT(XSAVES)
> +#define SECONDARY_EXEC_ENABLE_XSAVES		VMCS_CONTROL_BIT(XSAVES)
>  #define SECONDARY_EXEC_MODE_BASED_EPT_EXEC	VMCS_CONTROL_BIT(MODE_BASED_EPT_EXEC)
>  #define SECONDARY_EXEC_PT_USE_GPA		VMCS_CONTROL_BIT(PT_USE_GPA)
>  #define SECONDARY_EXEC_TSC_SCALING              VMCS_CONTROL_BIT(TSC_SCALING)

To avoid the need to make up these names in KVM we can probably just
stick to SDM; that would make it easier to make a connection between KVM
and Intel docs if needed. E.g. SDM uses "Use TSC scaling" so this
could've been "SECONDARY_EXEC_USE_TSC_SCALING" for consistency.

Unfortunatelly, SDM itself is not very consistent in the naming,
e.g. compare "WBINVD exiting"/"RDSEED exiting" with "Enable ENCLS
exiting"/"Enable ENCLV exiting" but I guess we won't be able to do
significantly better in KVM anyways..

> diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
> index d0abee35d7ba..41a4533f9989 100644
> --- a/arch/x86/kvm/vmx/capabilities.h
> +++ b/arch/x86/kvm/vmx/capabilities.h
> @@ -252,7 +252,7 @@ static inline bool cpu_has_vmx_pml(void)
>  static inline bool cpu_has_vmx_xsaves(void)
>  {
>  	return vmcs_config.cpu_based_2nd_exec_ctrl &
> -		SECONDARY_EXEC_XSAVES;
> +		SECONDARY_EXEC_ENABLE_XSAVES;
>  }
>  
>  static inline bool cpu_has_vmx_waitpkg(void)
> diff --git a/arch/x86/kvm/vmx/hyperv.c b/arch/x86/kvm/vmx/hyperv.c
> index 79450e1ed7cf..313b8bb5b8a7 100644
> --- a/arch/x86/kvm/vmx/hyperv.c
> +++ b/arch/x86/kvm/vmx/hyperv.c
> @@ -78,7 +78,7 @@
>  	 SECONDARY_EXEC_DESC |						\
>  	 SECONDARY_EXEC_ENABLE_RDTSCP |					\
>  	 SECONDARY_EXEC_ENABLE_INVPCID |				\
> -	 SECONDARY_EXEC_XSAVES |					\
> +	 SECONDARY_EXEC_ENABLE_XSAVES |					\
>  	 SECONDARY_EXEC_RDSEED_EXITING |				\
>  	 SECONDARY_EXEC_RDRAND_EXITING |				\
>  	 SECONDARY_EXEC_TSC_SCALING |					\
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 516391cc0d64..22e08d30baef 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2307,7 +2307,7 @@ static void prepare_vmcs02_early(struct vcpu_vmx *vmx, struct loaded_vmcs *vmcs0
>  				  SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
>  				  SECONDARY_EXEC_ENABLE_INVPCID |
>  				  SECONDARY_EXEC_ENABLE_RDTSCP |
> -				  SECONDARY_EXEC_XSAVES |
> +				  SECONDARY_EXEC_ENABLE_XSAVES |
>  				  SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE |
>  				  SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
>  				  SECONDARY_EXEC_APIC_REGISTER_VIRT |
> @@ -6331,7 +6331,7 @@ static bool nested_vmx_l1_wants_exit(struct kvm_vcpu *vcpu,
>  		 * If if it were, XSS would have to be checked against
>  		 * the XSS exit bitmap in vmcs12.
>  		 */
> -		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
> +		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_XSAVES);
>  	case EXIT_REASON_UMWAIT:
>  	case EXIT_REASON_TPAUSE:
>  		return nested_cpu_has2(vmcs12,
> @@ -6874,7 +6874,7 @@ static void nested_vmx_setup_secondary_ctls(u32 ept_caps,
>  		SECONDARY_EXEC_ENABLE_INVPCID |
>  		SECONDARY_EXEC_ENABLE_VMFUNC |
>  		SECONDARY_EXEC_RDSEED_EXITING |
> -		SECONDARY_EXEC_XSAVES |
> +		SECONDARY_EXEC_ENABLE_XSAVES |
>  		SECONDARY_EXEC_TSC_SCALING |
>  		SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE;
>  
> diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
> index 96952263b029..b4b9d51438c6 100644
> --- a/arch/x86/kvm/vmx/nested.h
> +++ b/arch/x86/kvm/vmx/nested.h
> @@ -168,7 +168,7 @@ static inline int nested_cpu_has_ept(struct vmcs12 *vmcs12)
>  
>  static inline bool nested_cpu_has_xsaves(struct vmcs12 *vmcs12)
>  {
> -	return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
> +	return nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_XSAVES);
>  }
>  
>  static inline bool nested_cpu_has_pml(struct vmcs12 *vmcs12)
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 78f292b7e2c5..22975cc949b7 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4614,7 +4614,7 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
>  
>  	if (cpu_has_vmx_xsaves())
>  		vmx_adjust_secondary_exec_control(vmx, &exec_control,
> -						  SECONDARY_EXEC_XSAVES,
> +						  SECONDARY_EXEC_ENABLE_XSAVES,
>  						  vcpu->arch.xsaves_enabled, false);
>  
>  	/*
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 32384ba38499..cde902b44d97 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -562,7 +562,7 @@ static inline u8 vmx_get_rvi(void)
>  	 SECONDARY_EXEC_APIC_REGISTER_VIRT |				\
>  	 SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |				\
>  	 SECONDARY_EXEC_SHADOW_VMCS |					\
> -	 SECONDARY_EXEC_XSAVES |					\
> +	 SECONDARY_EXEC_ENABLE_XSAVES |					\
>  	 SECONDARY_EXEC_RDSEED_EXITING |				\
>  	 SECONDARY_EXEC_RDRAND_EXITING |				\
>  	 SECONDARY_EXEC_ENABLE_PML |					\



-- 
Vitaly