Re: [RFC PATCH v3 09/10] x86/cpu: Call ENCLS[EUPDATESVN] procedure in microcode update

[Dave Hansen <dave.hansen@intel.com>]

On 4/1/22 07:24, Cathy Zhang wrote:
> +#ifndef update_cpusvn_intel
> +static inline void update_cpusvn_intel(void) {}
> +#endif
> +
>  #endif /* _ASM_X86_MICROCODE_H */
> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> index 74bcb6841a4b..5867f5a78d13 100644
> --- a/arch/x86/include/asm/sgx.h
> +++ b/arch/x86/include/asm/sgx.h
> @@ -409,4 +409,9 @@ int sgx_virt_einit(void __user *sigstruct, void __user *token,
>  int sgx_set_attribute(unsigned long *allowed_attributes,
>  		      unsigned int attribute_fd);
>  
> +#ifdef CONFIG_X86_SGX
> +void update_cpusvn_intel(void);
> +#define update_cpusvn_intel update_cpusvn_intel
> +#endif
> +
>  #endif /* _ASM_X86_SGX_H */
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 7b8382c11788..f53fd877ba0d 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -59,6 +59,7 @@
>  #include <asm/cpu_device_id.h>
>  #include <asm/uv/uv.h>
>  #include <asm/sigframe.h>
> +#include <asm/sgx.h>
>  
>  #include "cpu.h"
>  
> @@ -2086,6 +2087,14 @@ void microcode_check(void)
>  
>  	perf_check_microcode();
>  
> +	/*
> +	 * SGX related microcode update requires EUPDATESVN to update CPUSVN, which
> +	 * will destroy all enclaves to ensure EPC is not in use. If SGX is configured
> +	 * and EUPDATESVN is supported, call the EUPDATESVN procecure.
> +	 */
> +	if (IS_ENABLED(CONFIG_X86_SGX) && (cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN))
> +		update_cpusvn_intel();

In addition to what Borislav said, these #ifdefs are not how we do things.

The update_cpusvn_intel() shouldn't be declared in two different
headers.  Just imagine what happens if some code happens to include the
microcode header *then* the sgx.h header.

Please define both the 'static inline' stub *and* the declaration in
sgx.h.  Then you won't even need the #ifndef trickery.  Please also add
an "sgx_" to its name prefix while you're at it.

That comment is also not great.  A reader will likely have *ZERO* idea
what EUPDATESVN is or what a CPUSVN is.  Try to focus on what the code
*means* rather than just repeat the if() conditions in the comment.