Hi Dave,
Thanks for your comments and suggestions!
> -----Original Message-----
> From: Hansen, Dave <dave.hansen@xxxxxxxxx>
> Sent: Friday, April 1, 2022 11:42 PM
> To: Zhang, Cathy <cathy.zhang@xxxxxxxxx>; linux-sgx@xxxxxxxxxxxxxxx;
> x86@xxxxxxxxxx
> Cc: jarkko@xxxxxxxxxx; Chatre, Reinette <reinette.chatre@xxxxxxxxx>; Raj,
> Ashok <ashok.raj@xxxxxxxxx>
> 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.
