[v15,14/23] x86/cpu/intel: Clear SGX_LC capability if not enabled in FEATURE_CONTROL

Commit Message

Jarkko Sakkinen Nov. 2, 2018, 11:11 p.m. UTC

From: Sean Christopherson <[e-mail]>

Like SGX itself, SGX Launch Control must be explicitly enabled via a
flag in IA32_FEATURE_CONTROL.  Clear the SGX_LC capability if Launch
Control is not fully enabled (or obviously if SGX itself is disabled).

Note that clearing X86_FEATURE_SGX_LC creates a bit of a conundrum
regarding the SGXLEPUBKEYHASH MSRs, as it may be desirable to read the
MSRs even if they are not writable, e.g. to query the configured key,
but clearing the capability leaves no breadcrum for discerning whether
or not the MSRs exist.  But, such usage will be rare (KVM is the only
known case at this time) and not performance critical, so it's not
unreasonable to require the use of rdmsr_safe().  Clearing the cap bit
eliminates the need for an additional flag to track whether or not
Launch Control is truly enabled, which is what we care about the vast
majority of the time.

Signed-off-by: Sean Christopherson <[e-mail]>
---
 arch/x86/kernel/cpu/intel.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Andy Shevchenko Nov. 3, 2018, 1:15 p.m. UTC | #1

On Sat, Nov 3, 2018 at 1:17 AM Jarkko Sakkinen
<[e-mail]> wrote:
>
> From: Sean Christopherson <[e-mail]>
>
> Like SGX itself, SGX Launch Control must be explicitly enabled via a
> flag in IA32_FEATURE_CONTROL.  Clear the SGX_LC capability if Launch
> Control is not fully enabled (or obviously if SGX itself is disabled).
>
> Note that clearing X86_FEATURE_SGX_LC creates a bit of a conundrum
> regarding the SGXLEPUBKEYHASH MSRs, as it may be desirable to read the
> MSRs even if they are not writable, e.g. to query the configured key,
> but clearing the capability leaves no breadcrum for discerning whether
> or not the MSRs exist.  But, such usage will be rare (KVM is the only
> known case at this time) and not performance critical, so it's not
> unreasonable to require the use of rdmsr_safe().  Clearing the cap bit
> eliminates the need for an additional flag to track whether or not
> Launch Control is truly enabled, which is what we care about the vast
> majority of the time.

> @@ -618,6 +618,8 @@ static void detect_sgx(struct cpuinfo_x86 *c)
>                 setup_clear_cpu_cap(X86_FEATURE_SGX1);
>                 setup_clear_cpu_cap(X86_FEATURE_SGX2);
>         }
> +       if (unsupported || !(fc & FEATURE_CONTROL_SGX_LE_WR))
> +               setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
>  }

A-ha, I see how you use this variable here (though it's still possible
to get rid of it, choose what is better for readability /
maintenance).

Jarkko Sakkinen Nov. 5, 2018, 2:37 p.m. UTC | #2

On Sat, Nov 03, 2018 at 03:15:15PM +0200, Andy Shevchenko wrote:
> > @@ -618,6 +618,8 @@ static void detect_sgx(struct cpuinfo_x86 *c)
> >                 setup_clear_cpu_cap(X86_FEATURE_SGX1);
> >                 setup_clear_cpu_cap(X86_FEATURE_SGX2);
> >         }
> > +       if (unsupported || !(fc & FEATURE_CONTROL_SGX_LE_WR))
> > +               setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
> >  }
> 
> A-ha, I see how you use this variable here (though it's still possible
> to get rid of it, choose what is better for readability /
> maintenance).

I would propose to squash this one to the earlier commit and refactor
it in a way that I proposed. Having this part as a separate commit in
my opinion is a bit confusing.

/Jarkko

Patch

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 9bf8fe2c04ac..bc52c52f7025 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -618,6 +618,8 @@  static void detect_sgx(struct cpuinfo_x86 *c)
 		setup_clear_cpu_cap(X86_FEATURE_SGX1);
 		setup_clear_cpu_cap(X86_FEATURE_SGX2);
 	}
+	if (unsupported || !(fc & FEATURE_CONTROL_SGX_LE_WR))
+		setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
 }
 
 static void init_intel_energy_perf(struct cpuinfo_x86 *c)