Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <13aaf1272737737c29ab1de22438695637944d24.camel@redhat.com>
Subject: Re: [PATCH v7 03/26] x86/fpu/xstate: Add CET supervisor mode state
 support
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Yang Weijiang <weijiang.yang@intel.com>, seanjc@google.com,
        pbonzini@redhat.com, dave.hansen@intel.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     peterz@infradead.org, chao.gao@intel.com,
        rick.p.edgecombe@intel.com, john.allen@amd.com
Date:   Thu, 30 Nov 2023 19:27:09 +0200
In-Reply-To: <20231124055330.138870-4-weijiang.yang@intel.com>
References: <20231124055330.138870-1-weijiang.yang@intel.com>
         <20231124055330.138870-4-weijiang.yang@intel.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote:
> Add supervisor mode state support within FPU xstate management framework.
> Although supervisor shadow stack is not enabled/used today in kernel,KVM
> requires the support because when KVM advertises shadow stack feature to
> guest, architecturally it claims the support for both user and supervisor
> modes for guest OSes(Linux or non-Linux).
> 
> CET supervisor states not only includes PL{0,1,2}_SSP but also IA32_S_CET
> MSR, but the latter is not xsave-managed. In virtualization world, guest
> IA32_S_CET is saved/stored into/from VM control structure. With supervisor
> xstate support, guest supervisor mode shadow stack state can be properly
> saved/restored when 1) guest/host FPU context is swapped 2) vCPU
> thread is sched out/in.
> 
> The alternative is to enable it in KVM domain, but KVM maintainers NAKed
> the solution. The external discussion can be found at [*], it ended up
> with adding the support in kernel instead of KVM domain.
> 
> Note, in KVM case, guest CET supervisor state i.e., IA32_PL{0,1,2}_MSRs,
> are preserved after VM-Exit until host/guest fpstates are swapped, but
> since host supervisor shadow stack is disabled, the preserved MSRs won't
> hurt host.
> 
> [*]: https://lore.kernel.org/all/806e26c2-8d21-9cc9-a0b7-7787dd231729@intel.com/
> 
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>  arch/x86/include/asm/fpu/types.h  | 14 ++++++++++++--
>  arch/x86/include/asm/fpu/xstate.h |  6 +++---
>  arch/x86/kernel/fpu/xstate.c      |  6 +++++-
>  3 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
> index eb810074f1e7..c6fd13a17205 100644
> --- a/arch/x86/include/asm/fpu/types.h
> +++ b/arch/x86/include/asm/fpu/types.h
> @@ -116,7 +116,7 @@ enum xfeature {
>  	XFEATURE_PKRU,
>  	XFEATURE_PASID,
>  	XFEATURE_CET_USER,
> -	XFEATURE_CET_KERNEL_UNUSED,
> +	XFEATURE_CET_KERNEL,
>  	XFEATURE_RSRVD_COMP_13,
>  	XFEATURE_RSRVD_COMP_14,
>  	XFEATURE_LBR,
> @@ -139,7 +139,7 @@ enum xfeature {
>  #define XFEATURE_MASK_PKRU		(1 << XFEATURE_PKRU)
>  #define XFEATURE_MASK_PASID		(1 << XFEATURE_PASID)
>  #define XFEATURE_MASK_CET_USER		(1 << XFEATURE_CET_USER)
> -#define XFEATURE_MASK_CET_KERNEL	(1 << XFEATURE_CET_KERNEL_UNUSED)
> +#define XFEATURE_MASK_CET_KERNEL	(1 << XFEATURE_CET_KERNEL)
>  #define XFEATURE_MASK_LBR		(1 << XFEATURE_LBR)
>  #define XFEATURE_MASK_XTILE_CFG		(1 << XFEATURE_XTILE_CFG)
>  #define XFEATURE_MASK_XTILE_DATA	(1 << XFEATURE_XTILE_DATA)
> @@ -264,6 +264,16 @@ struct cet_user_state {
>  	u64 user_ssp;
>  };
>  
> +/*
> + * State component 12 is Control-flow Enforcement supervisor states
> + */
> +struct cet_supervisor_state {
> +	/* supervisor ssp pointers  */
> +	u64 pl0_ssp;
> +	u64 pl1_ssp;
> +	u64 pl2_ssp;
> +};
> +
>  /*
>   * State component 15: Architectural LBR configuration state.
>   * The size of Arch LBR state depends on the number of LBRs (lbr_depth).
> diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
> index d4427b88ee12..3b4a038d3c57 100644
> --- a/arch/x86/include/asm/fpu/xstate.h
> +++ b/arch/x86/include/asm/fpu/xstate.h
> @@ -51,7 +51,8 @@
>  
>  /* All currently supported supervisor features */
>  #define XFEATURE_MASK_SUPERVISOR_SUPPORTED (XFEATURE_MASK_PASID | \
> -					    XFEATURE_MASK_CET_USER)
> +					    XFEATURE_MASK_CET_USER | \
> +					    XFEATURE_MASK_CET_KERNEL)
>  
>  /*
>   * A supervisor state component may not always contain valuable information,
> @@ -78,8 +79,7 @@
>   * Unsupported supervisor features. When a supervisor feature in this mask is
>   * supported in the future, move it to the supported supervisor feature mask.
>   */
> -#define XFEATURE_MASK_SUPERVISOR_UNSUPPORTED (XFEATURE_MASK_PT | \
> -					      XFEATURE_MASK_CET_KERNEL)
> +#define XFEATURE_MASK_SUPERVISOR_UNSUPPORTED (XFEATURE_MASK_PT)
>  
>  /* All supervisor states including supported and unsupported states. */
>  #define XFEATURE_MASK_SUPERVISOR_ALL (XFEATURE_MASK_SUPERVISOR_SUPPORTED | \
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 6e50a4251e2b..b57d909facca 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -51,7 +51,7 @@ static const char *xfeature_names[] =
>  	"Protection Keys User registers",
>  	"PASID state",
>  	"Control-flow User registers",
> -	"Control-flow Kernel registers (unused)",
> +	"Control-flow Kernel registers",
>  	"unknown xstate feature",
>  	"unknown xstate feature",
>  	"unknown xstate feature",
> @@ -73,6 +73,7 @@ static unsigned short xsave_cpuid_features[] __initdata = {
>  	[XFEATURE_PT_UNIMPLEMENTED_SO_FAR]	= X86_FEATURE_INTEL_PT,
>  	[XFEATURE_PKRU]				= X86_FEATURE_OSPKE,
>  	[XFEATURE_PASID]			= X86_FEATURE_ENQCMD,
> +	[XFEATURE_CET_KERNEL]			= X86_FEATURE_SHSTK,
>  	[XFEATURE_XTILE_CFG]			= X86_FEATURE_AMX_TILE,
>  	[XFEATURE_XTILE_DATA]			= X86_FEATURE_AMX_TILE,
>  };
> @@ -277,6 +278,7 @@ static void __init print_xstate_features(void)
>  	print_xstate_feature(XFEATURE_MASK_PKRU);
>  	print_xstate_feature(XFEATURE_MASK_PASID);
>  	print_xstate_feature(XFEATURE_MASK_CET_USER);
> +	print_xstate_feature(XFEATURE_MASK_CET_KERNEL);
>  	print_xstate_feature(XFEATURE_MASK_XTILE_CFG);
>  	print_xstate_feature(XFEATURE_MASK_XTILE_DATA);
>  }
> @@ -346,6 +348,7 @@ static __init void os_xrstor_booting(struct xregs_state *xstate)
>  	 XFEATURE_MASK_BNDCSR |			\
>  	 XFEATURE_MASK_PASID |			\
>  	 XFEATURE_MASK_CET_USER |		\
> +	 XFEATURE_MASK_CET_KERNEL |		\
>  	 XFEATURE_MASK_XTILE)
>  
>  /*
> @@ -546,6 +549,7 @@ static bool __init check_xstate_against_struct(int nr)
>  	case XFEATURE_PASID:	  return XCHECK_SZ(sz, nr, struct ia32_pasid_state);
>  	case XFEATURE_XTILE_CFG:  return XCHECK_SZ(sz, nr, struct xtile_cfg);
>  	case XFEATURE_CET_USER:	  return XCHECK_SZ(sz, nr, struct cet_user_state);
> +	case XFEATURE_CET_KERNEL: return XCHECK_SZ(sz, nr, struct cet_supervisor_state);
>  	case XFEATURE_XTILE_DATA: check_xtile_data_against_struct(sz); return true;
>  	default:
>  		XSTATE_WARN_ON(1, "No structure for xstate: %d\n", nr);

Any reason why my reviewed-by was not added to this patch?

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky