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

["Zhang, Cathy" <cathy.zhang@intel.com>]

Hi Dave,

Thanks for your comments and suggestions!

> -----Original Message-----
> From: Hansen, Dave <dave.hansen@intel.com>
> Sent: Friday, April 1, 2022 11:42 PM
> To: Zhang, Cathy <cathy.zhang@intel.com>; linux-sgx@vger.kernel.org;
> x86@kernel.org
> Cc: jarkko@kernel.org; Chatre, Reinette <reinette.chatre@intel.com>; Raj,
> Ashok <ashok.raj@intel.com>
> Subject: Re: [RFC PATCH v3 09/10] x86/cpu: Call ENCLS[EUPDATESVN]
> procedure in microcode update
> 
> 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.

Moved to sgx.h and added "sgx_".

> 
> 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.

Yes, I see. The update comment is shown in my reply to Boris. Please take a look.