diff mbox series

[v15,05/23] x86/cpu/intel: Detect SGX support and update caps appropriately

Message ID 20181102231320.29164-6-jarkko.sakkinen@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Intel SGX1 | expand

Commit Message

Jarkko Sakkinen Nov. 2, 2018, 11:11 p.m. UTC 
From: Sean Christopherson <sean.j.christopherson@intel.com>

Similar to other large Intel features such as VMX and TXT, SGX must be
explicitly enabled in IA32_FEATURE_CONTROL MSR to be truly usable.
Clear all SGX related capabilities if SGX is not fully enabled in
IA32_FEATURE_CONTROL or if the SGX1 instruction set isn't supported
(impossible on bare metal, theoretically possible in a VM if the VMM
is doing something weird).

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/intel.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Andy Shevchenko Nov. 3, 2018, 1:05 p.m. UTC | #1 
On Sat, Nov 3, 2018 at 1:16 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> From: Sean Christopherson <sean.j.christopherson@intel.com>
>
> Similar to other large Intel features such as VMX and TXT, SGX must be
> explicitly enabled in IA32_FEATURE_CONTROL MSR to be truly usable.
> Clear all SGX related capabilities if SGX is not fully enabled in
> IA32_FEATURE_CONTROL or if the SGX1 instruction set isn't supported
> (impossible on bare metal, theoretically possible in a VM if the VMM
> is doing something weird).
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kernel/cpu/intel.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index fc3c07fe7df5..9bf8fe2c04ac 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -596,6 +596,30 @@ static void detect_tme(struct cpuinfo_x86 *c)
>         c->x86_phys_bits -= keyid_bits;
>  }
>
> +static void detect_sgx(struct cpuinfo_x86 *c)
> +{
> +       bool unsupported = false;
> +       unsigned long long fc;
> +
> +       rdmsrl(MSR_IA32_FEATURE_CONTROL, fc);
> +       if (!(fc & FEATURE_CONTROL_LOCKED)) {
> +               pr_err_once("sgx: IA32_FEATURE_CONTROL MSR is not locked\n");
> +               unsupported = true;
> +       } else if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) {
> +               pr_err_once("sgx: not enabled in IA32_FEATURE_CONTROL MSR\n");
> +               unsupported = true;
> +       } else if (!cpu_has(c, X86_FEATURE_SGX1)) {
> +               pr_err_once("sgx: SGX1 instruction set not supported\n");
> +               unsupported = true;
> +       }

If you do

} else {
 /* Supported */
 return;
}

here, you may get rid of temporary variable.

(Up to you)

> +
> +       if (unsupported) {
> +               setup_clear_cpu_cap(X86_FEATURE_SGX);
> +               setup_clear_cpu_cap(X86_FEATURE_SGX1);
> +               setup_clear_cpu_cap(X86_FEATURE_SGX2);
> +       }
> +}
> +
>  static void init_intel_energy_perf(struct cpuinfo_x86 *c)
>  {
>         u64 epb;
> @@ -763,6 +787,9 @@ static void init_intel(struct cpuinfo_x86 *c)
>         if (cpu_has(c, X86_FEATURE_TME))
>                 detect_tme(c);
>
> +       if (cpu_has(c, X86_FEATURE_SGX))
> +               detect_sgx(c);
> +
>         init_intel_energy_perf(c);
>
>         init_intel_misc_features(c);
> --
> 2.19.1
>
Jarkko Sakkinen Nov. 5, 2018, 2:09 p.m. UTC | #2 
On Sat, Nov 03, 2018 at 03:05:39PM +0200, Andy Shevchenko wrote:
> > +static void detect_sgx(struct cpuinfo_x86 *c)
> > +{
> > +       bool unsupported = false;
> > +       unsigned long long fc;
> > +
> > +       rdmsrl(MSR_IA32_FEATURE_CONTROL, fc);
> > +       if (!(fc & FEATURE_CONTROL_LOCKED)) {
> > +               pr_err_once("sgx: IA32_FEATURE_CONTROL MSR is not locked\n");
> > +               unsupported = true;
> > +       } else if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) {
> > +               pr_err_once("sgx: not enabled in IA32_FEATURE_CONTROL MSR\n");
> > +               unsupported = true;
> > +       } else if (!cpu_has(c, X86_FEATURE_SGX1)) {
> > +               pr_err_once("sgx: SGX1 instruction set not supported\n");
> > +               unsupported = true;
> > +       }
> 
> If you do
> 
> } else {
>  /* Supported */
>  return;
> }

Agree. Would this be a more clean flow in the attached patch?

/Jarkko
From 3b863a7db00cefffc15df918a5132c35ea313c27 Mon Sep 17 00:00:00 2001
From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Date: Mon, 5 Nov 2018 16:06:06 +0200
Subject: [PATCH] x86/cpu/intel: clean up detect_sgx() flow

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/intel.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index bc52c52f7025..8a20a193d399 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -598,28 +598,36 @@ static void detect_tme(struct cpuinfo_x86 *c)
 
 static void detect_sgx(struct cpuinfo_x86 *c)
 {
-	bool unsupported = false;
 	unsigned long long fc;
 
 	rdmsrl(MSR_IA32_FEATURE_CONTROL, fc);
 	if (!(fc & FEATURE_CONTROL_LOCKED)) {
 		pr_err_once("sgx: IA32_FEATURE_CONTROL MSR is not locked\n");
-		unsupported = true;
-	} else if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) {
+		goto out_unsupported;
+	}
+
+	if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) {
 		pr_err_once("sgx: not enabled in IA32_FEATURE_CONTROL MSR\n");
-		unsupported = true;
-	} else if (!cpu_has(c, X86_FEATURE_SGX1)) {
+		goto out_unsupported;
+	}
+
+	if (!cpu_has(c, X86_FEATURE_SGX1)) {
 		pr_err_once("sgx: SGX1 instruction set not supported\n");
-		unsupported = true;
+		goto out_unsupported;
 	}
 
-	if (unsupported) {
-		setup_clear_cpu_cap(X86_FEATURE_SGX);
-		setup_clear_cpu_cap(X86_FEATURE_SGX1);
-		setup_clear_cpu_cap(X86_FEATURE_SGX2);
+	if (!(fc & FEATURE_CONTROL_SGX_LE_WR)) {
+		pr_info_once("sgx: launch control MSRs are not writable\n");
+		goto out_msrs_rdonly;
 	}
-	if (unsupported || !(fc & FEATURE_CONTROL_SGX_LE_WR))
-		setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
+
+	return;
+out_unsupported:
+	setup_clear_cpu_cap(X86_FEATURE_SGX);
+	setup_clear_cpu_cap(X86_FEATURE_SGX1);
+	setup_clear_cpu_cap(X86_FEATURE_SGX2);
+out_msrs_rdonly:
+	setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
 }
 
 static void init_intel_energy_perf(struct cpuinfo_x86 *c)
Jarkko Sakkinen Nov. 5, 2018, 2:11 p.m. UTC | #3 
On Mon, Nov 05, 2018 at 04:09:33PM +0200, Jarkko Sakkinen wrote:
> On Sat, Nov 03, 2018 at 03:05:39PM +0200, Andy Shevchenko wrote:
> > > +static void detect_sgx(struct cpuinfo_x86 *c)
> > > +{
> > > +       bool unsupported = false;
> > > +       unsigned long long fc;
> > > +
> > > +       rdmsrl(MSR_IA32_FEATURE_CONTROL, fc);
> > > +       if (!(fc & FEATURE_CONTROL_LOCKED)) {
> > > +               pr_err_once("sgx: IA32_FEATURE_CONTROL MSR is not locked\n");
> > > +               unsupported = true;
> > > +       } else if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) {
> > > +               pr_err_once("sgx: not enabled in IA32_FEATURE_CONTROL MSR\n");
> > > +               unsupported = true;
> > > +       } else if (!cpu_has(c, X86_FEATURE_SGX1)) {
> > > +               pr_err_once("sgx: SGX1 instruction set not supported\n");
> > > +               unsupported = true;
> > > +       }
> > 
> > If you do
> > 
> > } else {
> >  /* Supported */
> >  return;
> > }
> 
> Agree. Would this be a more clean flow in the attached patch?

Actually I'll paste the whole function for clarity because it is not too
long:

static void detect_sgx(struct cpuinfo_x86 *c)
{
	unsigned long long fc;

	rdmsrl(MSR_IA32_FEATURE_CONTROL, fc);
	if (!(fc & FEATURE_CONTROL_LOCKED)) {
		pr_err_once("sgx: IA32_FEATURE_CONTROL MSR is not locked\n");
		goto out_unsupported;
	}

	if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) {
		pr_err_once("sgx: not enabled in IA32_FEATURE_CONTROL MSR\n");
		goto out_unsupported;
	}

	if (!cpu_has(c, X86_FEATURE_SGX1)) {
		pr_err_once("sgx: SGX1 instruction set not supported\n");
		goto out_unsupported;
	}

	if (!(fc & FEATURE_CONTROL_SGX_LE_WR)) {
		pr_info_once("sgx: launch control MSRs are not writable\n");
		goto out_msrs_rdonly;
	}

	return;
out_unsupported:
	setup_clear_cpu_cap(X86_FEATURE_SGX);
	setup_clear_cpu_cap(X86_FEATURE_SGX1);
	setup_clear_cpu_cap(X86_FEATURE_SGX2);
out_msrs_rdonly:
	setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
}

/Jarkko
Andy Shevchenko Nov. 5, 2018, 2:31 p.m. UTC | #4 
On Mon, Nov 05, 2018 at 04:09:33PM +0200, Jarkko Sakkinen wrote:
> On Sat, Nov 03, 2018 at 03:05:39PM +0200, Andy Shevchenko wrote:

> Agree. Would this be a more clean flow in the attached patch?

LGTM.

> From 3b863a7db00cefffc15df918a5132c35ea313c27 Mon Sep 17 00:00:00 2001
> From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Date: Mon, 5 Nov 2018 16:06:06 +0200
> Subject: [PATCH] x86/cpu/intel: clean up detect_sgx() flow
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/intel.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index bc52c52f7025..8a20a193d399 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -598,28 +598,36 @@ static void detect_tme(struct cpuinfo_x86 *c)
>  
>  static void detect_sgx(struct cpuinfo_x86 *c)
>  {
> -	bool unsupported = false;
>  	unsigned long long fc;
>  
>  	rdmsrl(MSR_IA32_FEATURE_CONTROL, fc);
>  	if (!(fc & FEATURE_CONTROL_LOCKED)) {
>  		pr_err_once("sgx: IA32_FEATURE_CONTROL MSR is not locked\n");
> -		unsupported = true;
> -	} else if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) {
> +		goto out_unsupported;
> +	}
> +
> +	if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) {
>  		pr_err_once("sgx: not enabled in IA32_FEATURE_CONTROL MSR\n");
> -		unsupported = true;
> -	} else if (!cpu_has(c, X86_FEATURE_SGX1)) {
> +		goto out_unsupported;
> +	}
> +
> +	if (!cpu_has(c, X86_FEATURE_SGX1)) {
>  		pr_err_once("sgx: SGX1 instruction set not supported\n");
> -		unsupported = true;
> +		goto out_unsupported;
>  	}
>  
> -	if (unsupported) {
> -		setup_clear_cpu_cap(X86_FEATURE_SGX);
> -		setup_clear_cpu_cap(X86_FEATURE_SGX1);
> -		setup_clear_cpu_cap(X86_FEATURE_SGX2);
> +	if (!(fc & FEATURE_CONTROL_SGX_LE_WR)) {
> +		pr_info_once("sgx: launch control MSRs are not writable\n");
> +		goto out_msrs_rdonly;
>  	}
> -	if (unsupported || !(fc & FEATURE_CONTROL_SGX_LE_WR))
> -		setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
> +
> +	return;
> +out_unsupported:
> +	setup_clear_cpu_cap(X86_FEATURE_SGX);
> +	setup_clear_cpu_cap(X86_FEATURE_SGX1);
> +	setup_clear_cpu_cap(X86_FEATURE_SGX2);
> +out_msrs_rdonly:
> +	setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
>  }
>  
>  static void init_intel_energy_perf(struct cpuinfo_x86 *c)
> -- 
> 2.19.1
>
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index fc3c07fe7df5..9bf8fe2c04ac 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -596,6 +596,30 @@  static void detect_tme(struct cpuinfo_x86 *c)
 	c->x86_phys_bits -= keyid_bits;
 }
 
+static void detect_sgx(struct cpuinfo_x86 *c)
+{
+	bool unsupported = false;
+	unsigned long long fc;
+
+	rdmsrl(MSR_IA32_FEATURE_CONTROL, fc);
+	if (!(fc & FEATURE_CONTROL_LOCKED)) {
+		pr_err_once("sgx: IA32_FEATURE_CONTROL MSR is not locked\n");
+		unsupported = true;
+	} else if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) {
+		pr_err_once("sgx: not enabled in IA32_FEATURE_CONTROL MSR\n");
+		unsupported = true;
+	} else if (!cpu_has(c, X86_FEATURE_SGX1)) {
+		pr_err_once("sgx: SGX1 instruction set not supported\n");
+		unsupported = true;
+	}
+
+	if (unsupported) {
+		setup_clear_cpu_cap(X86_FEATURE_SGX);
+		setup_clear_cpu_cap(X86_FEATURE_SGX1);
+		setup_clear_cpu_cap(X86_FEATURE_SGX2);
+	}
+}
+
 static void init_intel_energy_perf(struct cpuinfo_x86 *c)
 {
 	u64 epb;
@@ -763,6 +787,9 @@  static void init_intel(struct cpuinfo_x86 *c)
 	if (cpu_has(c, X86_FEATURE_TME))
 		detect_tme(c);
 
+	if (cpu_has(c, X86_FEATURE_SGX))
+		detect_sgx(c);
+
 	init_intel_energy_perf(c);
 
 	init_intel_misc_features(c);