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

Dave Hansen
2022-04-01T16:19:35Z

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.