[RFC,v5,06/26] x86/cpu/intel: Allow SGX virtualization without Launch Control support

Commit Message

Huang, Kai Feb. 13, 2021, 1:29 p.m. UTC

From: Sean Christopherson <[e-mail]>

The kernel will currently disable all SGX support if the hardware does
not support launch control.  Make it more permissive to allow SGX
virtualization on systems without Launch Control support.  This will
allow KVM to expose SGX to guests that have less-strict requirements on
the availability of flexible launch control.

Improve error message to distinguish between three cases.  There are two
cases where SGX support is completely disabled:
1) SGX has been disabled completely by the BIOS
2) SGX LC is locked by the BIOS.  Bare-metal support is disabled because
   of LC unavailability.  SGX virtualization is unavailable (because of
   Kconfig).
One where it is partially available:
3) SGX LC is locked by the BIOS.  Bare-metal support is disabled because
   of LC unavailability.  SGX virtualization is supported.

Signed-off-by: Sean Christopherson <[e-mail]>
Co-developed-by: Kai Huang <[e-mail]>
Signed-off-by: Kai Huang <[e-mail]>
---
v4->v5:

 - No code change.

v3->v4:

 - Removed cpu_has(X86_FEATURE_SGX1) check in enable_sgx_any, since it logically
   is not related to KVM SGX series, per Sean.
 - Changed declaration of variables to be in reverse-christmas tree style, per
   Jarkko.

v2->v3:

 - Added to use 'enable_sgx_any', per Dave.
 - Changed to call clear_cpu_cap() directly, rather than using clear_sgx_caps()
   and clear_sgx_lc().
 - Changed to use CONFIG_X86_SGX_KVM, instead of CONFIG_X86_SGX_VIRTUALIZATION.

v1->v2:

 - Refined commit message per Dave's comments.
 - Added check to only enable SGX virtualization when VMX is supported, per
   Dave's comment.
 - Refined error msg print to explicitly call out SGX virtualization will be
   supported when LC is locked by BIOS, per Dave's comment.

---
 arch/x86/kernel/cpu/feat_ctl.c | 57 ++++++++++++++++++++++++++--------
 1 file changed, 44 insertions(+), 13 deletions(-)

Comments

Jarkko Sakkinen Feb. 16, 2021, 2:15 a.m. UTC | #1

On Sun, Feb 14, 2021 at 02:29:05AM +1300, Kai Huang wrote:
> From: Sean Christopherson <[e-mail]>
> 
> The kernel will currently disable all SGX support if the hardware does
> not support launch control.  Make it more permissive to allow SGX
> virtualization on systems without Launch Control support.  This will
> allow KVM to expose SGX to guests that have less-strict requirements on
> the availability of flexible launch control.
> 
> Improve error message to distinguish between three cases.  There are two
> cases where SGX support is completely disabled:
> 1) SGX has been disabled completely by the BIOS
> 2) SGX LC is locked by the BIOS.  Bare-metal support is disabled because
>    of LC unavailability.  SGX virtualization is unavailable (because of
>    Kconfig).
> One where it is partially available:
> 3) SGX LC is locked by the BIOS.  Bare-metal support is disabled because
>    of LC unavailability.  SGX virtualization is supported.
> 
> Signed-off-by: Sean Christopherson <[e-mail]>
> Co-developed-by: Kai Huang <[e-mail]>
> Signed-off-by: Kai Huang <[e-mail]>
> ---
> v4->v5:
> 
>  - No code change.
> 
> v3->v4:
> 
>  - Removed cpu_has(X86_FEATURE_SGX1) check in enable_sgx_any, since it logically
>    is not related to KVM SGX series, per Sean.
>  - Changed declaration of variables to be in reverse-christmas tree style, per
>    Jarkko.
> 
> v2->v3:
> 
>  - Added to use 'enable_sgx_any', per Dave.
>  - Changed to call clear_cpu_cap() directly, rather than using clear_sgx_caps()
>    and clear_sgx_lc().
>  - Changed to use CONFIG_X86_SGX_KVM, instead of CONFIG_X86_SGX_VIRTUALIZATION.
> 
> v1->v2:
> 
>  - Refined commit message per Dave's comments.
>  - Added check to only enable SGX virtualization when VMX is supported, per
>    Dave's comment.
>  - Refined error msg print to explicitly call out SGX virtualization will be
>    supported when LC is locked by BIOS, per Dave's comment.
> 
> ---
>  arch/x86/kernel/cpu/feat_ctl.c | 57 ++++++++++++++++++++++++++--------
>  1 file changed, 44 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/feat_ctl.c b/arch/x86/kernel/cpu/feat_ctl.c
> index 27533a6e04fa..96c370284913 100644
> --- a/arch/x86/kernel/cpu/feat_ctl.c
> +++ b/arch/x86/kernel/cpu/feat_ctl.c
> @@ -105,7 +105,8 @@ early_param("nosgx", nosgx);
>  void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
>  {
>  	bool tboot = tboot_enabled();
> -	bool enable_sgx;
> +	bool enable_sgx_any, enable_sgx_kvm, enable_sgx_driver;
> +	bool enable_vmx;
>  	u64 msr;
>  
>  	if (rdmsrl_safe(MSR_IA32_FEAT_CTL, &msr)) {
> @@ -114,13 +115,21 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
>  		return;
>  	}
>  
> +	enable_vmx = cpu_has(c, X86_FEATURE_VMX) &&
> +		     IS_ENABLED(CONFIG_KVM_INTEL);

It's less than 100 characters:

        enable_vmx = cpu_has(c, X86_FEATURE_VMX) && IS_ENABLED(CONFIG_KVM_INTEL);

This is better:

        enable_vmx = IS_ENABLED(CONFIG_KVM_INTEL) && cpu_has(c, X86_FEATURE_VMX);

You only want to evaluate cpu_has() if COHNFIG_KVM_INTEL is enabled.

> +
>  	/*
> -	 * Enable SGX if and only if the kernel supports SGX and Launch Control
> -	 * is supported, i.e. disable SGX if the LE hash MSRs can't be written.
> +	 * Separate out SGX driver enabling from KVM.  This allows KVM
> +	 * guests to use SGX even if the kernel SGX driver refuses to
> +	 * use it.  This happens if flexible Faunch Control is not
> +	 * available.
>  	 */
> -	enable_sgx = cpu_has(c, X86_FEATURE_SGX) &&
> -		     cpu_has(c, X86_FEATURE_SGX_LC) &&
> -		     IS_ENABLED(CONFIG_X86_SGX);
> +	enable_sgx_any = cpu_has(c, X86_FEATURE_SGX) &&
> +			 IS_ENABLED(CONFIG_X86_SGX);
> +	enable_sgx_driver = enable_sgx_any &&
> +			    cpu_has(c, X86_FEATURE_SGX_LC);
> +	enable_sgx_kvm = enable_sgx_any && enable_vmx &&
> +			  IS_ENABLED(CONFIG_X86_SGX_KVM);
>  
>  	if (msr & FEAT_CTL_LOCKED)
>  		goto update_caps;
> @@ -136,15 +145,18 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
>  	 * i.e. KVM is enabled, to avoid unnecessarily adding an attack vector
>  	 * for the kernel, e.g. using VMX to hide malicious code.
>  	 */
> -	if (cpu_has(c, X86_FEATURE_VMX) && IS_ENABLED(CONFIG_KVM_INTEL)) {
> +	if (enable_vmx) {
>  		msr |= FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX;
>  
>  		if (tboot)
>  			msr |= FEAT_CTL_VMX_ENABLED_INSIDE_SMX;
>  	}
>  
> -	if (enable_sgx)
> -		msr |= FEAT_CTL_SGX_ENABLED | FEAT_CTL_SGX_LC_ENABLED;
> +	if (enable_sgx_kvm || enable_sgx_driver) {
> +		msr |= FEAT_CTL_SGX_ENABLED;
> +		if (enable_sgx_driver)
> +			msr |= FEAT_CTL_SGX_LC_ENABLED;
> +	}
>  
>  	wrmsrl(MSR_IA32_FEAT_CTL, msr);
>  
> @@ -167,10 +179,29 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
>  	}
>  
>  update_sgx:
> -	if (!(msr & FEAT_CTL_SGX_ENABLED) ||
> -	    !(msr & FEAT_CTL_SGX_LC_ENABLED) || !enable_sgx) {
> -		if (enable_sgx)
> -			pr_err_once("SGX disabled by BIOS\n");
> +	if (!(msr & FEAT_CTL_SGX_ENABLED)) {
> +		if (enable_sgx_kvm || enable_sgx_driver)
> +			pr_err_once("SGX disabled by BIOS.\n");
>  		clear_cpu_cap(c, X86_FEATURE_SGX);

Empty line before return statement.

> +		return;
> +	}
> +
> +	/*
> +	 * VMX feature bit may be cleared due to being disabled in BIOS,
> +	 * in which case SGX virtualization cannot be supported either.
> +	 */
> +	if (!cpu_has(c, X86_FEATURE_VMX) && enable_sgx_kvm) {
> +		pr_err_once("SGX virtualization disabled due to lack of VMX.\n");
> +		enable_sgx_kvm = 0;
> +	}
> +
> +	if (!(msr & FEAT_CTL_SGX_LC_ENABLED) && enable_sgx_driver) {
> +		if (!enable_sgx_kvm) {
> +			pr_err_once("SGX Launch Control is locked. Disable SGX.\n");
> +			clear_cpu_cap(c, X86_FEATURE_SGX);
> +		} else {
> +			pr_err_once("SGX Launch Control is locked. Support SGX virtualization only.\n");
> +			clear_cpu_cap(c, X86_FEATURE_SGX_LC);
> +		}
>  	}
>  }
> -- 
> 2.29.2
> 
> 

/Jarkko

Huang, Kai Feb. 16, 2021, 5:03 a.m. UTC | #2

> 
> On Sun, Feb 14, 2021 at 02:29:05AM +1300, Kai Huang wrote:
> > From: Sean Christopherson <[e-mail]>
> >
> > The kernel will currently disable all SGX support if the hardware does
> > not support launch control.  Make it more permissive to allow SGX
> > virtualization on systems without Launch Control support.  This will
> > allow KVM to expose SGX to guests that have less-strict requirements
> > on the availability of flexible launch control.
> >
> > Improve error message to distinguish between three cases.  There are
> > two cases where SGX support is completely disabled:
> > 1) SGX has been disabled completely by the BIOS
> > 2) SGX LC is locked by the BIOS.  Bare-metal support is disabled because
> >    of LC unavailability.  SGX virtualization is unavailable (because of
> >    Kconfig).
> > One where it is partially available:
> > 3) SGX LC is locked by the BIOS.  Bare-metal support is disabled because
> >    of LC unavailability.  SGX virtualization is supported.
> >
> > Signed-off-by: Sean Christopherson <[e-mail]>
> > Co-developed-by: Kai Huang <[e-mail]>
> > Signed-off-by: Kai Huang <[e-mail]>
> > ---
> > v4->v5:
> >
> >  - No code change.
> >
> > v3->v4:
> >
> >  - Removed cpu_has(X86_FEATURE_SGX1) check in enable_sgx_any, since it
> logically
> >    is not related to KVM SGX series, per Sean.
> >  - Changed declaration of variables to be in reverse-christmas tree style, per
> >    Jarkko.
> >
> > v2->v3:
> >
> >  - Added to use 'enable_sgx_any', per Dave.
> >  - Changed to call clear_cpu_cap() directly, rather than using clear_sgx_caps()
> >    and clear_sgx_lc().
> >  - Changed to use CONFIG_X86_SGX_KVM, instead of
> CONFIG_X86_SGX_VIRTUALIZATION.
> >
> > v1->v2:
> >
> >  - Refined commit message per Dave's comments.
> >  - Added check to only enable SGX virtualization when VMX is supported, per
> >    Dave's comment.
> >  - Refined error msg print to explicitly call out SGX virtualization will be
> >    supported when LC is locked by BIOS, per Dave's comment.
> >
> > ---
> >  arch/x86/kernel/cpu/feat_ctl.c | 57
> > ++++++++++++++++++++++++++--------
> >  1 file changed, 44 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/feat_ctl.c
> > b/arch/x86/kernel/cpu/feat_ctl.c index 27533a6e04fa..96c370284913
> > 100644
> > --- a/arch/x86/kernel/cpu/feat_ctl.c
> > +++ b/arch/x86/kernel/cpu/feat_ctl.c
> > @@ -105,7 +105,8 @@ early_param("nosgx", nosgx);  void
> > init_ia32_feat_ctl(struct cpuinfo_x86 *c)  {
> >  	bool tboot = tboot_enabled();
> > -	bool enable_sgx;
> > +	bool enable_sgx_any, enable_sgx_kvm, enable_sgx_driver;
> > +	bool enable_vmx;
> >  	u64 msr;
> >
> >  	if (rdmsrl_safe(MSR_IA32_FEAT_CTL, &msr)) { @@ -114,13 +115,21
> @@
> > void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
> >  		return;
> >  	}
> >
> > +	enable_vmx = cpu_has(c, X86_FEATURE_VMX) &&
> > +		     IS_ENABLED(CONFIG_KVM_INTEL);
> 
> It's less than 100 characters:

Just carious, shouldn't be 80 characters to wrap a new line, instead of 100?

> 
>         enable_vmx = cpu_has(c, X86_FEATURE_VMX) &&
> IS_ENABLED(CONFIG_KVM_INTEL);
> 
> This is better:
> 
>         enable_vmx = IS_ENABLED(CONFIG_KVM_INTEL) && cpu_has(c,
> X86_FEATURE_VMX);
> 
> You only want to evaluate cpu_has() if COHNFIG_KVM_INTEL is enabled.

If you look at the original code, cpu_has() comes first. It's just one-time booting time code, and I don't think it matters.

Btw, are you also suggesting IS_ENABLED(CONFIG_X86_SGX) should come before cpu_has() for SGX in below code? 

That being said, cpu_has() comes first for both VMX and SGX in the original code. I don't know why I need to change the sequence in this patch.

> 
> > +
> >  	/*
> > -	 * Enable SGX if and only if the kernel supports SGX and Launch Control
> > -	 * is supported, i.e. disable SGX if the LE hash MSRs can't be written.
> > +	 * Separate out SGX driver enabling from KVM.  This allows KVM
> > +	 * guests to use SGX even if the kernel SGX driver refuses to
> > +	 * use it.  This happens if flexible Faunch Control is not
> > +	 * available.
> >  	 */
> > -	enable_sgx = cpu_has(c, X86_FEATURE_SGX) &&
> > -		     cpu_has(c, X86_FEATURE_SGX_LC) &&
> > -		     IS_ENABLED(CONFIG_X86_SGX);
> > +	enable_sgx_any = cpu_has(c, X86_FEATURE_SGX) &&
> > +			 IS_ENABLED(CONFIG_X86_SGX);
> > +	enable_sgx_driver = enable_sgx_any &&
> > +			    cpu_has(c, X86_FEATURE_SGX_LC);
> > +	enable_sgx_kvm = enable_sgx_any && enable_vmx &&
> > +			  IS_ENABLED(CONFIG_X86_SGX_KVM);
> >
> >  	if (msr & FEAT_CTL_LOCKED)
> >  		goto update_caps;
> > @@ -136,15 +145,18 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
> >  	 * i.e. KVM is enabled, to avoid unnecessarily adding an attack vector
> >  	 * for the kernel, e.g. using VMX to hide malicious code.
> >  	 */
> > -	if (cpu_has(c, X86_FEATURE_VMX) &&
> IS_ENABLED(CONFIG_KVM_INTEL)) {
> > +	if (enable_vmx) {
> >  		msr |= FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX;
> >
> >  		if (tboot)
> >  			msr |= FEAT_CTL_VMX_ENABLED_INSIDE_SMX;
> >  	}
> >
> > -	if (enable_sgx)
> > -		msr |= FEAT_CTL_SGX_ENABLED |
> FEAT_CTL_SGX_LC_ENABLED;
> > +	if (enable_sgx_kvm || enable_sgx_driver) {
> > +		msr |= FEAT_CTL_SGX_ENABLED;
> > +		if (enable_sgx_driver)
> > +			msr |= FEAT_CTL_SGX_LC_ENABLED;
> > +	}
> >
> >  	wrmsrl(MSR_IA32_FEAT_CTL, msr);
> >
> > @@ -167,10 +179,29 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
> >  	}
> >
> >  update_sgx:
> > -	if (!(msr & FEAT_CTL_SGX_ENABLED) ||
> > -	    !(msr & FEAT_CTL_SGX_LC_ENABLED) || !enable_sgx) {
> > -		if (enable_sgx)
> > -			pr_err_once("SGX disabled by BIOS\n");
> > +	if (!(msr & FEAT_CTL_SGX_ENABLED)) {
> > +		if (enable_sgx_kvm || enable_sgx_driver)
> > +			pr_err_once("SGX disabled by BIOS.\n");
> >  		clear_cpu_cap(c, X86_FEATURE_SGX);
> 
> Empty line before return statement.

It's just two statements inside the if() {} statement. Putting a new line here is too sparse IMHO.

I'd like to hear more.

Dave, do you have any comment?

> 
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * VMX feature bit may be cleared due to being disabled in BIOS,
> > +	 * in which case SGX virtualization cannot be supported either.
> > +	 */
> > +	if (!cpu_has(c, X86_FEATURE_VMX) && enable_sgx_kvm) {
> > +		pr_err_once("SGX virtualization disabled due to lack of VMX.\n");
> > +		enable_sgx_kvm = 0;
> > +	}
> > +
> > +	if (!(msr & FEAT_CTL_SGX_LC_ENABLED) && enable_sgx_driver) {
> > +		if (!enable_sgx_kvm) {
> > +			pr_err_once("SGX Launch Control is locked. Disable
> SGX.\n");
> > +			clear_cpu_cap(c, X86_FEATURE_SGX);
> > +		} else {
> > +			pr_err_once("SGX Launch Control is locked. Support
> SGX virtualization only.\n");
> > +			clear_cpu_cap(c, X86_FEATURE_SGX_LC);
> > +		}
> >  	}
> >  }
> > --
> > 2.29.2
> >
> >
> 
> /Jarkko

Jarkko Sakkinen Feb. 16, 2021, 8:36 a.m. UTC | #3

On Tue, Feb 16, 2021 at 05:03:26AM +0000, Huang, Kai wrote:
> > 
> > On Sun, Feb 14, 2021 at 02:29:05AM +1300, Kai Huang wrote:
> > > From: Sean Christopherson <[e-mail]>
> > >
> > > The kernel will currently disable all SGX support if the hardware does
> > > not support launch control.  Make it more permissive to allow SGX
> > > virtualization on systems without Launch Control support.  This will
> > > allow KVM to expose SGX to guests that have less-strict requirements
> > > on the availability of flexible launch control.
> > >
> > > Improve error message to distinguish between three cases.  There are
> > > two cases where SGX support is completely disabled:
> > > 1) SGX has been disabled completely by the BIOS
> > > 2) SGX LC is locked by the BIOS.  Bare-metal support is disabled because
> > >    of LC unavailability.  SGX virtualization is unavailable (because of
> > >    Kconfig).
> > > One where it is partially available:
> > > 3) SGX LC is locked by the BIOS.  Bare-metal support is disabled because
> > >    of LC unavailability.  SGX virtualization is supported.
> > >
> > > Signed-off-by: Sean Christopherson <[e-mail]>
> > > Co-developed-by: Kai Huang <[e-mail]>
> > > Signed-off-by: Kai Huang <[e-mail]>
> > > ---
> > > v4->v5:
> > >
> > >  - No code change.
> > >
> > > v3->v4:
> > >
> > >  - Removed cpu_has(X86_FEATURE_SGX1) check in enable_sgx_any, since it
> > logically
> > >    is not related to KVM SGX series, per Sean.
> > >  - Changed declaration of variables to be in reverse-christmas tree style, per
> > >    Jarkko.
> > >
> > > v2->v3:
> > >
> > >  - Added to use 'enable_sgx_any', per Dave.
> > >  - Changed to call clear_cpu_cap() directly, rather than using clear_sgx_caps()
> > >    and clear_sgx_lc().
> > >  - Changed to use CONFIG_X86_SGX_KVM, instead of
> > CONFIG_X86_SGX_VIRTUALIZATION.
> > >
> > > v1->v2:
> > >
> > >  - Refined commit message per Dave's comments.
> > >  - Added check to only enable SGX virtualization when VMX is supported, per
> > >    Dave's comment.
> > >  - Refined error msg print to explicitly call out SGX virtualization will be
> > >    supported when LC is locked by BIOS, per Dave's comment.
> > >
> > > ---
> > >  arch/x86/kernel/cpu/feat_ctl.c | 57
> > > ++++++++++++++++++++++++++--------
> > >  1 file changed, 44 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/arch/x86/kernel/cpu/feat_ctl.c
> > > b/arch/x86/kernel/cpu/feat_ctl.c index 27533a6e04fa..96c370284913
> > > 100644
> > > --- a/arch/x86/kernel/cpu/feat_ctl.c
> > > +++ b/arch/x86/kernel/cpu/feat_ctl.c
> > > @@ -105,7 +105,8 @@ early_param("nosgx", nosgx);  void
> > > init_ia32_feat_ctl(struct cpuinfo_x86 *c)  {
> > >  	bool tboot = tboot_enabled();
> > > -	bool enable_sgx;
> > > +	bool enable_sgx_any, enable_sgx_kvm, enable_sgx_driver;
> > > +	bool enable_vmx;
> > >  	u64 msr;
> > >
> > >  	if (rdmsrl_safe(MSR_IA32_FEAT_CTL, &msr)) { @@ -114,13 +115,21
> > @@
> > > void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
> > >  		return;
> > >  	}
> > >
> > > +	enable_vmx = cpu_has(c, X86_FEATURE_VMX) &&
> > > +		     IS_ENABLED(CONFIG_KVM_INTEL);
> > 
> > It's less than 100 characters:
> 
> Just carious, shouldn't be 80 characters to wrap a new line, instead of 100?

Try with checkpatch.pl.

> 
> > 
> >         enable_vmx = cpu_has(c, X86_FEATURE_VMX) &&
> > IS_ENABLED(CONFIG_KVM_INTEL);
> > 
> > This is better:
> > 
> >         enable_vmx = IS_ENABLED(CONFIG_KVM_INTEL) && cpu_has(c,
> > X86_FEATURE_VMX);
> > 
> > You only want to evaluate cpu_has() if COHNFIG_KVM_INTEL is enabled.
> 
> If you look at the original code, cpu_has() comes first. It's just one-time booting time code, and I don't think it matters.
> 
> Btw, are you also suggesting IS_ENABLED(CONFIG_X86_SGX) should come before cpu_has() for SGX in below code? 
> 
> That being said, cpu_has() comes first for both VMX and SGX in the original code. I don't know why I need to change the sequence in this patch.
> 
> > 
> > > +
> > >  	/*
> > > -	 * Enable SGX if and only if the kernel supports SGX and Launch Control
> > > -	 * is supported, i.e. disable SGX if the LE hash MSRs can't be written.
> > > +	 * Separate out SGX driver enabling from KVM.  This allows KVM
> > > +	 * guests to use SGX even if the kernel SGX driver refuses to
> > > +	 * use it.  This happens if flexible Faunch Control is not
> > > +	 * available.
> > >  	 */
> > > -	enable_sgx = cpu_has(c, X86_FEATURE_SGX) &&
> > > -		     cpu_has(c, X86_FEATURE_SGX_LC) &&
> > > -		     IS_ENABLED(CONFIG_X86_SGX);
> > > +	enable_sgx_any = cpu_has(c, X86_FEATURE_SGX) &&
> > > +			 IS_ENABLED(CONFIG_X86_SGX);
> > > +	enable_sgx_driver = enable_sgx_any &&
> > > +			    cpu_has(c, X86_FEATURE_SGX_LC);
> > > +	enable_sgx_kvm = enable_sgx_any && enable_vmx &&
> > > +			  IS_ENABLED(CONFIG_X86_SGX_KVM);
> > >
> > >  	if (msr & FEAT_CTL_LOCKED)
> > >  		goto update_caps;
> > > @@ -136,15 +145,18 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
> > >  	 * i.e. KVM is enabled, to avoid unnecessarily adding an attack vector
> > >  	 * for the kernel, e.g. using VMX to hide malicious code.
> > >  	 */
> > > -	if (cpu_has(c, X86_FEATURE_VMX) &&
> > IS_ENABLED(CONFIG_KVM_INTEL)) {
> > > +	if (enable_vmx) {
> > >  		msr |= FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX;
> > >
> > >  		if (tboot)
> > >  			msr |= FEAT_CTL_VMX_ENABLED_INSIDE_SMX;
> > >  	}
> > >
> > > -	if (enable_sgx)
> > > -		msr |= FEAT_CTL_SGX_ENABLED |
> > FEAT_CTL_SGX_LC_ENABLED;
> > > +	if (enable_sgx_kvm || enable_sgx_driver) {
> > > +		msr |= FEAT_CTL_SGX_ENABLED;
> > > +		if (enable_sgx_driver)
> > > +			msr |= FEAT_CTL_SGX_LC_ENABLED;
> > > +	}
> > >
> > >  	wrmsrl(MSR_IA32_FEAT_CTL, msr);
> > >
> > > @@ -167,10 +179,29 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
> > >  	}
> > >
> > >  update_sgx:
> > > -	if (!(msr & FEAT_CTL_SGX_ENABLED) ||
> > > -	    !(msr & FEAT_CTL_SGX_LC_ENABLED) || !enable_sgx) {
> > > -		if (enable_sgx)
> > > -			pr_err_once("SGX disabled by BIOS\n");
> > > +	if (!(msr & FEAT_CTL_SGX_ENABLED)) {
> > > +		if (enable_sgx_kvm || enable_sgx_driver)
> > > +			pr_err_once("SGX disabled by BIOS.\n");
> > >  		clear_cpu_cap(c, X86_FEATURE_SGX);
> > 
> > Empty line before return statement.
> 
> It's just two statements inside the if() {} statement. Putting a new line here is too sparse IMHO.
> 
> I'd like to hear more.

This was a common review comment in original SGX series, so I'm
sticking to the pattern.

> 
> Dave, do you have any comment?
> 
> > 
> > > +		return;
> > > +	}
> > > +
> > > +	/*
> > > +	 * VMX feature bit may be cleared due to being disabled in BIOS,
> > > +	 * in which case SGX virtualization cannot be supported either.
> > > +	 */
> > > +	if (!cpu_has(c, X86_FEATURE_VMX) && enable_sgx_kvm) {
> > > +		pr_err_once("SGX virtualization disabled due to lack of VMX.\n");
> > > +		enable_sgx_kvm = 0;
> > > +	}
> > > +
> > > +	if (!(msr & FEAT_CTL_SGX_LC_ENABLED) && enable_sgx_driver) {
> > > +		if (!enable_sgx_kvm) {
> > > +			pr_err_once("SGX Launch Control is locked. Disable
> > SGX.\n");
> > > +			clear_cpu_cap(c, X86_FEATURE_SGX);
> > > +		} else {
> > > +			pr_err_once("SGX Launch Control is locked. Support
> > SGX virtualization only.\n");
> > > +			clear_cpu_cap(c, X86_FEATURE_SGX_LC);
> > > +		}
> > >  	}
> > >  }
> > > --
> > > 2.29.2
> > >
> > >
> > 
> > /Jarkko
> 

/Jarkko

Huang, Kai Feb. 16, 2021, 10:24 a.m. UTC | #4

> > > >
> > > > +	enable_vmx = cpu_has(c, X86_FEATURE_VMX) &&
> > > > +		     IS_ENABLED(CONFIG_KVM_INTEL);
> > >
> > > It's less than 100 characters:
> >
> > Just carious, shouldn't be 80 characters to wrap a new line, instead of 100?
> 
> Try with checkpatch.pl.

Checkpatch.pl has default value 100, but it can be overwritten. I found below document explicitly said 80 should be the length:

https://www.kernel.org/doc/html/v5.11-rc7/process/coding-style.html

2) Breaking long lines and strings

Coding style is all about readability and maintainability using commonly available tools.

The preferred limit on the length of a single line is 80 columns.

Statements longer than 80 columns should be broken into sensible chunks, unless exceeding 80 columns significantly increases readability and does not hide information.

[...]

> > > >  update_sgx:
> > > > -	if (!(msr & FEAT_CTL_SGX_ENABLED) ||
> > > > -	    !(msr & FEAT_CTL_SGX_LC_ENABLED) || !enable_sgx) {
> > > > -		if (enable_sgx)
> > > > -			pr_err_once("SGX disabled by BIOS\n");
> > > > +	if (!(msr & FEAT_CTL_SGX_ENABLED)) {
> > > > +		if (enable_sgx_kvm || enable_sgx_driver)
> > > > +			pr_err_once("SGX disabled by BIOS.\n");
> > > >  		clear_cpu_cap(c, X86_FEATURE_SGX);
> > >
> > > Empty line before return statement.
> >
> > It's just two statements inside the if() {} statement. Putting a new line here is
> too sparse IMHO.
> >
> > I'd like to hear more.
> 
> This was a common review comment in original SGX series, so I'm sticking to the
> pattern.

Well if you insist, I can do that. 

But I am not that convinced. In fact, I also believe that in most cases, having empty line before 'return' is good practice, for instance, when 'return' is the very last statement in the function.

I am also glad to do it if it is a x86 patch convention that we even need to put a new empty line when there are only very few statements inside if() {}. However it seems it is not the case.

For example, I just did a search in SGX driver code, and below examples all DONOT have empty line before return (and I don't think I captured them all):

sgx/driver.c:

static int sgx_release(struct inode *inode, struct file *file)
{
        ......
        kref_put(&encl->refcount, sgx_encl_release);
        return 0;
}

sgx/ioctl.c: 

static struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl)
{
        ......
                va_page->epc_page = sgx_alloc_va_page();
                if (IS_ERR(va_page->epc_page)) {
                        err = ERR_CAST(va_page->epc_page);
                        kfree(va_page);
                        return err;
                }

                WARN_ON_ONCE(encl->page_cnt % SGX_VA_SLOT_COUNT);
        }
        encl->page_cnt++;
        return va_page;
}

static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg)
{
        ......
        kfree(secs);
        return ret;
}

static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
                             unsigned long offset, struct sgx_secinfo *secinfo,
                             unsigned long flags)
{
        ......
        epc_page = sgx_alloc_epc_page(encl_page, true);
        if (IS_ERR(epc_page)) {
                kfree(encl_page);
                return PTR_ERR(epc_page);
        }
        ......
        sgx_mark_page_reclaimable(encl_page->epc_page);
        mutex_unlock(&encl->lock);
        mmap_read_unlock(current->mm);
        return ret;

        ......
}

And in cpu/feat_ctl.c:

void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
{
        ......
        if (rdmsrl_safe(MSR_IA32_FEAT_CTL, &msr)) {
                clear_cpu_cap(c, X86_FEATURE_VMX);
                clear_sgx_caps();
                return;
        }
        ......
}

Dave Hansen Feb. 16, 2021, 6:40 p.m. UTC | #5

On 2/13/21 5:29 AM, Kai Huang wrote:
> From: Sean Christopherson <[e-mail]>
> 
> The kernel will currently disable all SGX support if the hardware does
> not support launch control.  Make it more permissive to allow SGX
> virtualization on systems without Launch Control support.  This will
> allow KVM to expose SGX to guests that have less-strict requirements on
> the availability of flexible launch control.
...

Acked-by: Dave Hansen <[e-mail]>

Huang, Kai Feb. 16, 2021, 8:42 p.m. UTC | #6

> 
> On 2/13/21 5:29 AM, Kai Huang wrote:
> > From: Sean Christopherson <[e-mail]>
> >
> > The kernel will currently disable all SGX support if the hardware does
> > not support launch control.  Make it more permissive to allow SGX
> > virtualization on systems without Launch Control support.  This will
> > allow KVM to expose SGX to guests that have less-strict requirements
> > on the availability of flexible launch control.
> ...
> 
> Acked-by: Dave Hansen <[e-mail]>

Thanks!

Patch

diff --git a/arch/x86/kernel/cpu/feat_ctl.c b/arch/x86/kernel/cpu/feat_ctl.c
index 27533a6e04fa..96c370284913 100644
--- a/arch/x86/kernel/cpu/feat_ctl.c
+++ b/arch/x86/kernel/cpu/feat_ctl.c
@@ -105,7 +105,8 @@  early_param("nosgx", nosgx);
 void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
 {
 	bool tboot = tboot_enabled();
-	bool enable_sgx;
+	bool enable_sgx_any, enable_sgx_kvm, enable_sgx_driver;
+	bool enable_vmx;
 	u64 msr;
 
 	if (rdmsrl_safe(MSR_IA32_FEAT_CTL, &msr)) {
@@ -114,13 +115,21 @@  void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
 		return;
 	}
 
+	enable_vmx = cpu_has(c, X86_FEATURE_VMX) &&
+		     IS_ENABLED(CONFIG_KVM_INTEL);
+
 	/*
-	 * Enable SGX if and only if the kernel supports SGX and Launch Control
-	 * is supported, i.e. disable SGX if the LE hash MSRs can't be written.
+	 * Separate out SGX driver enabling from KVM.  This allows KVM
+	 * guests to use SGX even if the kernel SGX driver refuses to
+	 * use it.  This happens if flexible Faunch Control is not
+	 * available.
 	 */
-	enable_sgx = cpu_has(c, X86_FEATURE_SGX) &&
-		     cpu_has(c, X86_FEATURE_SGX_LC) &&
-		     IS_ENABLED(CONFIG_X86_SGX);
+	enable_sgx_any = cpu_has(c, X86_FEATURE_SGX) &&
+			 IS_ENABLED(CONFIG_X86_SGX);
+	enable_sgx_driver = enable_sgx_any &&
+			    cpu_has(c, X86_FEATURE_SGX_LC);
+	enable_sgx_kvm = enable_sgx_any && enable_vmx &&
+			  IS_ENABLED(CONFIG_X86_SGX_KVM);
 
 	if (msr & FEAT_CTL_LOCKED)
 		goto update_caps;
@@ -136,15 +145,18 @@  void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
 	 * i.e. KVM is enabled, to avoid unnecessarily adding an attack vector
 	 * for the kernel, e.g. using VMX to hide malicious code.
 	 */
-	if (cpu_has(c, X86_FEATURE_VMX) && IS_ENABLED(CONFIG_KVM_INTEL)) {
+	if (enable_vmx) {
 		msr |= FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX;
 
 		if (tboot)
 			msr |= FEAT_CTL_VMX_ENABLED_INSIDE_SMX;
 	}
 
-	if (enable_sgx)
-		msr |= FEAT_CTL_SGX_ENABLED | FEAT_CTL_SGX_LC_ENABLED;
+	if (enable_sgx_kvm || enable_sgx_driver) {
+		msr |= FEAT_CTL_SGX_ENABLED;
+		if (enable_sgx_driver)
+			msr |= FEAT_CTL_SGX_LC_ENABLED;
+	}
 
 	wrmsrl(MSR_IA32_FEAT_CTL, msr);
 
@@ -167,10 +179,29 @@  void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
 	}
 
 update_sgx:
-	if (!(msr & FEAT_CTL_SGX_ENABLED) ||
-	    !(msr & FEAT_CTL_SGX_LC_ENABLED) || !enable_sgx) {
-		if (enable_sgx)
-			pr_err_once("SGX disabled by BIOS\n");
+	if (!(msr & FEAT_CTL_SGX_ENABLED)) {
+		if (enable_sgx_kvm || enable_sgx_driver)
+			pr_err_once("SGX disabled by BIOS.\n");
 		clear_cpu_cap(c, X86_FEATURE_SGX);
+		return;
+	}
+
+	/*
+	 * VMX feature bit may be cleared due to being disabled in BIOS,
+	 * in which case SGX virtualization cannot be supported either.
+	 */
+	if (!cpu_has(c, X86_FEATURE_VMX) && enable_sgx_kvm) {
+		pr_err_once("SGX virtualization disabled due to lack of VMX.\n");
+		enable_sgx_kvm = 0;
+	}
+
+	if (!(msr & FEAT_CTL_SGX_LC_ENABLED) && enable_sgx_driver) {
+		if (!enable_sgx_kvm) {
+			pr_err_once("SGX Launch Control is locked. Disable SGX.\n");
+			clear_cpu_cap(c, X86_FEATURE_SGX);
+		} else {
+			pr_err_once("SGX Launch Control is locked. Support SGX virtualization only.\n");
+			clear_cpu_cap(c, X86_FEATURE_SGX_LC);
+		}
 	}
 }