diff mbox series

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

Message ID a6c0b0d2632a6c603e68d9bdc81f564290ff04ad.1610935432.git.kai.huang@intel.com (mailing list archive)
State New, archived
Headers show
Series KVM SGX virtualization support | expand

Commit Message

Kai Huang Jan. 18, 2021, 3:27 a.m. UTC 
From: Sean Christopherson <sean.j.christopherson@intel.com>

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 <sean.j.christopherson@intel.com>
Co-developed-by: Kai Huang <kai.huang@intel.com>
Signed-off-by: Kai Huang <kai.huang@intel.com>
---
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 | 64 +++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 13 deletions(-)

Comments

Dave Hansen Jan. 20, 2021, 9:02 p.m. UTC | #1 
On 1/17/21 7:27 PM, Kai Huang wrote:
> -	enable_sgx = cpu_has(c, X86_FEATURE_SGX) &&
> -		     cpu_has(c, X86_FEATURE_SGX_LC) &&
> -		     IS_ENABLED(CONFIG_X86_SGX);
> +	enable_sgx_driver = cpu_has(c, X86_FEATURE_SGX) &&
> +			    cpu_has(c, X86_FEATURE_SGX1) &&
> +			    IS_ENABLED(CONFIG_X86_SGX) &&
> +			    cpu_has(c, X86_FEATURE_SGX_LC);
> +	enable_sgx_virt = cpu_has(c, X86_FEATURE_SGX) &&
> +			  cpu_has(c, X86_FEATURE_SGX1) &&
> +			  IS_ENABLED(CONFIG_X86_SGX) &&
> +			  IS_ENABLED(CONFIG_X86_SGX_VIRTUALIZATION) &&
> +			  enable_vmx;

Would it be too much to ask that the SGX/SGX1 checks not be duplicated?
 Perhaps:

	enable_sgx_any = cpu_feature_enabled(CONFIG_X86_SGX) &&
			 cpu_feature_enabled(CONFIG_X86_SGX1);

	enable_sgx_driver = enable_sgx_any &&
			    cpu_has(c, X86_FEATURE_SGX_LC);

	enable_sgx_virt = enable_sgx_any &&
			  enable_vmx &&
		     IS_ENABLED(CONFIG_X86_SGX_VIRTUALIZATION)

BTW, CONFIG_X86_SGX_VIRTUALIZATION is a pretty porky name.  Maybe just
CONFIG_X86_SGX_VIRT?
Sean Christopherson Jan. 20, 2021, 10:36 p.m. UTC | #2 
On Wed, Jan 20, 2021, Dave Hansen wrote:
> BTW, CONFIG_X86_SGX_VIRTUALIZATION is a pretty porky name.  Maybe just
> CONFIG_X86_SGX_VIRT?

Mmm, bacon.  I used the full "virtualization" to avoid any possible confusion
with virtual memory.  The existing sgx_get_epc_virt_addr() in particular gave me
pause.

I agree it's long and not consistent since other code in this series uses "virt".
My thinking was that most shortand versions, e.g. virt_epc, would be used only
in contexts that are already fairly obvious to be KVM/virtualization related,
whereas the porcine Kconfig would help establish that context.
Dave Hansen Jan. 20, 2021, 11:27 p.m. UTC | #3 
On 1/20/21 2:36 PM, Sean Christopherson wrote:
> On Wed, Jan 20, 2021, Dave Hansen wrote:
>> BTW, CONFIG_X86_SGX_VIRTUALIZATION is a pretty porky name.  Maybe just
>> CONFIG_X86_SGX_VIRT?
> Mmm, bacon.  I used the full "virtualization" to avoid any possible confusion
> with virtual memory.  The existing sgx_get_epc_virt_addr() in particular gave me
> pause.
> 
> I agree it's long and not consistent since other code in this series uses "virt".
> My thinking was that most shortand versions, e.g. virt_epc, would be used only
> in contexts that are already fairly obvious to be KVM/virtualization related,
> whereas the porcine Kconfig would help establish that context.

Not a big deal either way.  I agree that "virt" can be confusing.

Considering that:

+config X86_SGX_VIRTUALIZATION
+	depends on ... KVM_INTEL

Calling it X86_SGX_KVM doesn't seem horrible either.
Kai Huang Jan. 20, 2021, 11:48 p.m. UTC | #4 
On Wed, 20 Jan 2021 15:27:27 -0800 Dave Hansen wrote:
> On 1/20/21 2:36 PM, Sean Christopherson wrote:
> > On Wed, Jan 20, 2021, Dave Hansen wrote:
> >> BTW, CONFIG_X86_SGX_VIRTUALIZATION is a pretty porky name.  Maybe just
> >> CONFIG_X86_SGX_VIRT?
> > Mmm, bacon.  I used the full "virtualization" to avoid any possible confusion
> > with virtual memory.  The existing sgx_get_epc_virt_addr() in particular gave me
> > pause.
> > 
> > I agree it's long and not consistent since other code in this series uses "virt".
> > My thinking was that most shortand versions, e.g. virt_epc, would be used only
> > in contexts that are already fairly obvious to be KVM/virtualization related,
> > whereas the porcine Kconfig would help establish that context.
> 
> Not a big deal either way.  I agree that "virt" can be confusing.
> 
> Considering that:
> 
> +config X86_SGX_VIRTUALIZATION
> +	depends on ... KVM_INTEL

It is already in patch 3: Introduce virtual EPC for use by KVM guests:

+config X86_SGX_VIRTUALIZATION
+	bool "Software Guard eXtensions (SGX) Virtualization"
+	depends on X86_SGX && KVM_INTEL

> 
> Calling it X86_SGX_KVM doesn't seem horrible either.
Kai Huang Jan. 20, 2021, 11:50 p.m. UTC | #5 
On Wed, 20 Jan 2021 13:02:15 -0800 Dave Hansen wrote:
> On 1/17/21 7:27 PM, Kai Huang wrote:
> > -	enable_sgx = cpu_has(c, X86_FEATURE_SGX) &&
> > -		     cpu_has(c, X86_FEATURE_SGX_LC) &&
> > -		     IS_ENABLED(CONFIG_X86_SGX);
> > +	enable_sgx_driver = cpu_has(c, X86_FEATURE_SGX) &&
> > +			    cpu_has(c, X86_FEATURE_SGX1) &&
> > +			    IS_ENABLED(CONFIG_X86_SGX) &&
> > +			    cpu_has(c, X86_FEATURE_SGX_LC);
> > +	enable_sgx_virt = cpu_has(c, X86_FEATURE_SGX) &&
> > +			  cpu_has(c, X86_FEATURE_SGX1) &&
> > +			  IS_ENABLED(CONFIG_X86_SGX) &&
> > +			  IS_ENABLED(CONFIG_X86_SGX_VIRTUALIZATION) &&
> > +			  enable_vmx;
> 
> Would it be too much to ask that the SGX/SGX1 checks not be duplicated?
>  Perhaps:
> 
> 	enable_sgx_any = cpu_feature_enabled(CONFIG_X86_SGX) &&
> 			 cpu_feature_enabled(CONFIG_X86_SGX1);
> 
> 	enable_sgx_driver = enable_sgx_any &&
> 			    cpu_has(c, X86_FEATURE_SGX_LC);
> 
> 	enable_sgx_virt = enable_sgx_any &&
> 			  enable_vmx &&
> 		     IS_ENABLED(CONFIG_X86_SGX_VIRTUALIZATION)
> 

I am happy to do it. Thanks.
Dave Hansen Jan. 20, 2021, 11:51 p.m. UTC | #6 
On 1/20/21 3:48 PM, Kai Huang wrote:
>> Not a big deal either way.  I agree that "virt" can be confusing.
>>
>> Considering that:
>>
>> +config X86_SGX_VIRTUALIZATION
>> +	depends on ... KVM_INTEL
> It is already in patch 3: Introduce virtual EPC for use by KVM guests:
> 
> +config X86_SGX_VIRTUALIZATION
> +	bool "Software Guard eXtensions (SGX) Virtualization"
> +	depends on X86_SGX && KVM_INTEL

Right, I'm suggesting that patch 3 should call it:

	X86_SGX_KVM

instead of:

	X86_SGX_VIRTUALIZATION
Jarkko Sakkinen Jan. 21, 2021, 1:11 a.m. UTC | #7 
On Wed, Jan 20, 2021 at 01:02:15PM -0800, Dave Hansen wrote:
> On 1/17/21 7:27 PM, Kai Huang wrote:
> > -	enable_sgx = cpu_has(c, X86_FEATURE_SGX) &&
> > -		     cpu_has(c, X86_FEATURE_SGX_LC) &&
> > -		     IS_ENABLED(CONFIG_X86_SGX);
> > +	enable_sgx_driver = cpu_has(c, X86_FEATURE_SGX) &&
> > +			    cpu_has(c, X86_FEATURE_SGX1) &&
> > +			    IS_ENABLED(CONFIG_X86_SGX) &&
> > +			    cpu_has(c, X86_FEATURE_SGX_LC);
> > +	enable_sgx_virt = cpu_has(c, X86_FEATURE_SGX) &&
> > +			  cpu_has(c, X86_FEATURE_SGX1) &&
> > +			  IS_ENABLED(CONFIG_X86_SGX) &&
> > +			  IS_ENABLED(CONFIG_X86_SGX_VIRTUALIZATION) &&
> > +			  enable_vmx;
> 
> Would it be too much to ask that the SGX/SGX1 checks not be duplicated?
>  Perhaps:
> 
> 	enable_sgx_any = cpu_feature_enabled(CONFIG_X86_SGX) &&
> 			 cpu_feature_enabled(CONFIG_X86_SGX1);
> 
> 	enable_sgx_driver = enable_sgx_any &&
> 			    cpu_has(c, X86_FEATURE_SGX_LC);
> 
> 	enable_sgx_virt = enable_sgx_any &&
> 			  enable_vmx &&
> 		     IS_ENABLED(CONFIG_X86_SGX_VIRTUALIZATION)
> 
> BTW, CONFIG_X86_SGX_VIRTUALIZATION is a pretty porky name.  Maybe just
> CONFIG_X86_SGX_VIRT?

If my /dev/sgx_vepc naming gets acceptance, then IMHO the best name
ought to be CONFIG_X86_VEPC.

/Jarkko
Jarkko Sakkinen Jan. 21, 2021, 1:12 a.m. UTC | #8 
On Wed, Jan 20, 2021 at 03:27:27PM -0800, Dave Hansen wrote:
> On 1/20/21 2:36 PM, Sean Christopherson wrote:
> > On Wed, Jan 20, 2021, Dave Hansen wrote:
> >> BTW, CONFIG_X86_SGX_VIRTUALIZATION is a pretty porky name.  Maybe just
> >> CONFIG_X86_SGX_VIRT?
> > Mmm, bacon.  I used the full "virtualization" to avoid any possible confusion
> > with virtual memory.  The existing sgx_get_epc_virt_addr() in particular gave me
> > pause.
> > 
> > I agree it's long and not consistent since other code in this series uses "virt".
> > My thinking was that most shortand versions, e.g. virt_epc, would be used only
> > in contexts that are already fairly obvious to be KVM/virtualization related,
> > whereas the porcine Kconfig would help establish that context.
> 
> Not a big deal either way.  I agree that "virt" can be confusing.
> 
> Considering that:
> 
> +config X86_SGX_VIRTUALIZATION
> +	depends on ... KVM_INTEL
> 
> Calling it X86_SGX_KVM doesn't seem horrible either.

This is something that I could cope just as well as with my proposal
as this is KVM tied feature.

/Jarkko
Kai Huang Jan. 21, 2021, 1:53 a.m. UTC | #9 
On Wed, 20 Jan 2021 15:51:44 -0800 Dave Hansen wrote:
> On 1/20/21 3:48 PM, Kai Huang wrote:
> >> Not a big deal either way.  I agree that "virt" can be confusing.
> >>
> >> Considering that:
> >>
> >> +config X86_SGX_VIRTUALIZATION
> >> +	depends on ... KVM_INTEL
> > It is already in patch 3: Introduce virtual EPC for use by KVM guests:
> > 
> > +config X86_SGX_VIRTUALIZATION
> > +	bool "Software Guard eXtensions (SGX) Virtualization"
> > +	depends on X86_SGX && KVM_INTEL
> 
> Right, I'm suggesting that patch 3 should call it:
> 
> 	X86_SGX_KVM
> 
> instead of:
> 
> 	X86_SGX_VIRTUALIZATION

In case we want to change to X86_SGX_KVM, is it more reasonable to put it to
arch/x86/kvm/Kconfig (maybe change to X86_KVM_SGX)?

Jarkko also mentioned X86_SGX_VEPC, in which case still putting it to
arch/x86/Kconfig looks a better fit.

Sean, Paolo,

Do you have comment here?
Jarkko Sakkinen Jan. 21, 2021, 2:35 p.m. UTC | #10 
On Thu, Jan 21, 2021 at 02:53:23PM +1300, Kai Huang wrote:
> On Wed, 20 Jan 2021 15:51:44 -0800 Dave Hansen wrote:
> > On 1/20/21 3:48 PM, Kai Huang wrote:
> > >> Not a big deal either way.  I agree that "virt" can be confusing.
> > >>
> > >> Considering that:
> > >>
> > >> +config X86_SGX_VIRTUALIZATION
> > >> +	depends on ... KVM_INTEL
> > > It is already in patch 3: Introduce virtual EPC for use by KVM guests:
> > > 
> > > +config X86_SGX_VIRTUALIZATION
> > > +	bool "Software Guard eXtensions (SGX) Virtualization"
> > > +	depends on X86_SGX && KVM_INTEL
> > 
> > Right, I'm suggesting that patch 3 should call it:
> > 
> > 	X86_SGX_KVM
> > 
> > instead of:
> > 
> > 	X86_SGX_VIRTUALIZATION
> 
> In case we want to change to X86_SGX_KVM, is it more reasonable to put it to
> arch/x86/kvm/Kconfig (maybe change to X86_KVM_SGX)?
> 
> Jarkko also mentioned X86_SGX_VEPC, in which case still putting it to
> arch/x86/Kconfig looks a better fit.

I disagree with myself on that now :-) I think the other
suggestions are better. I'm only pursuing 'vepc' for the
device name because it follows the pattern used in the
other devices.

> 
> Sean, Paolo,
> 
> Do you have comment here?


/Jarkko
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/feat_ctl.c b/arch/x86/kernel/cpu/feat_ctl.c
index 7937a315f8cf..7bd8c57c62fa 100644
--- a/arch/x86/kernel/cpu/feat_ctl.c
+++ b/arch/x86/kernel/cpu/feat_ctl.c
@@ -98,6 +98,11 @@  static void clear_sgx_caps(void)
 	setup_clear_cpu_cap(X86_FEATURE_SGX);
 }
 
+static void clear_sgx_lc(void)
+{
+	setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
+}
+
 static int __init nosgx(char *str)
 {
 	clear_sgx_caps();
@@ -110,7 +115,7 @@  early_param("nosgx", nosgx);
 void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
 {
 	bool tboot = tboot_enabled();
-	bool enable_sgx;
+	bool enable_vmx, enable_sgx_virt, enable_sgx_driver;
 	u64 msr;
 
 	if (rdmsrl_safe(MSR_IA32_FEAT_CTL, &msr)) {
@@ -119,13 +124,24 @@  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.
+	 * Enable SGX if and only if the kernel supports SGX.  Require Launch
+	 * Control support if SGX virtualization is *not* supported, i.e.
+	 * disable SGX if the LE hash MSRs can't be written and SGX can't be
+	 * exposed to a KVM guest (which might support non-LC configurations).
 	 */
-	enable_sgx = cpu_has(c, X86_FEATURE_SGX) &&
-		     cpu_has(c, X86_FEATURE_SGX_LC) &&
-		     IS_ENABLED(CONFIG_X86_SGX);
+	enable_sgx_driver = cpu_has(c, X86_FEATURE_SGX) &&
+			    cpu_has(c, X86_FEATURE_SGX1) &&
+			    IS_ENABLED(CONFIG_X86_SGX) &&
+			    cpu_has(c, X86_FEATURE_SGX_LC);
+	enable_sgx_virt = cpu_has(c, X86_FEATURE_SGX) &&
+			  cpu_has(c, X86_FEATURE_SGX1) &&
+			  IS_ENABLED(CONFIG_X86_SGX) &&
+			  IS_ENABLED(CONFIG_X86_SGX_VIRTUALIZATION) &&
+			  enable_vmx;
 
 	if (msr & FEAT_CTL_LOCKED)
 		goto update_caps;
@@ -141,15 +157,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_driver || enable_sgx_virt) {
+		msr |= FEAT_CTL_SGX_ENABLED;
+		if (enable_sgx_driver)
+			msr |= FEAT_CTL_SGX_LC_ENABLED;
+	}
 
 	wrmsrl(MSR_IA32_FEAT_CTL, msr);
 
@@ -172,10 +191,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_driver || enable_sgx_virt)
+			pr_err_once("SGX disabled by BIOS.\n");
 		clear_sgx_caps();
+		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_virt) {
+		pr_err_once("SGX virtualization disabled due to lack of VMX.\n");
+		enable_sgx_virt = 0;
+	}
+
+	if (!(msr & FEAT_CTL_SGX_LC_ENABLED) && enable_sgx_driver) {
+		if (!enable_sgx_virt) {
+			pr_err_once("SGX Launch Control is locked. Disable SGX.\n");
+			clear_sgx_caps();
+		} else {
+			pr_err_once("SGX Launch Control is locked. Support SGX virtualization only.\n");
+			clear_sgx_lc();
+		}
 	}
 }