Re: [RFC PATCH v3 07/27] x86/cpu/intel: Allow SGX virtualization without Launch Control support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jan 27, 2021, Kai Huang wrote:
> On Tue, 26 Jan 2021 16:18:31 -0800 Dave Hansen wrote:
> > On 1/26/21 3:56 PM, Kai Huang wrote:
> > > On Tue, 26 Jan 2021 08:26:21 -0800 Dave Hansen wrote:
> > >> On 1/26/21 1:30 AM, Kai Huang wrote:
> > >>> --- 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_vmx;
> > >>> +	bool enable_sgx_any, enable_sgx_kvm, enable_sgx_driver;
> > >>>  	u64 msr;
> > >>>  
> > >>>  	if (rdmsrl_safe(MSR_IA32_FEAT_CTL, &msr)) {
> > >>> @@ -114,13 +115,22 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
> > >>>  		return;
> > >>>  	}
> > >>>  
> > >>> +	enable_vmx = cpu_has(c, X86_FEATURE_VMX) &&
> > >>> +		     IS_ENABLED(CONFIG_KVM_INTEL);
> > >>
> > >> The reason it's called 'enable_sgx' below is because this code is
> > >> actually going to "enable sgx".  This code does not "enable vmx".  That
> > >> makes this a badly-named variable.  "vmx_enabled" or "vmx_available"
> > >> would be better.
> > > 
> > > It will also try to enable VMX if feature control MSR is not locked by BIOS.
> > > Please see below code:
> > 
> > Ahh, I forgot this is non-SGX code.  It's mucking with all kinds of
> > other stuff in the same MSR.  Oh, well, I guess that's what you get for
> > dumping a bunch of refactoring in the same patch as the new code.
> > 
> > 
> > >>> -	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) &&
> > >>> +			 cpu_has(c, X86_FEATURE_SGX1) &&
> > >>> +			 IS_ENABLED(CONFIG_X86_SGX);
> > >>
> > >> The X86_FEATURE_SGX1 check seems to have snuck in here.  Why?
> > > 
> > > Please see my reply to Sean's reply.
> > 
> > ... yes, so you're breaking out the fix into a separate patch,.
> 
> For the separate patch to fix SGX1 check, if I understand correctly, SGX driver
> should be changed too. I feel I am not the best person to do it. Jarkko or Sean
> is. 

SGX driver doesn't need to be changed, just this core feat_ctl.c code.

> So I'll remove SGX1 here in the next version, but I won't include another
> patch to fix the SGX1 logic. If Jarkko or Sean sent out that patch, and it is
> merged quickly, I can rebase on top of that.
> 
> Does this make sense?

Yep, adding a check on SGX1 is definitely not mandatory for this series.



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux