On Wed, 6 Jan 2021 15:19:41 -0800 Sean Christopherson wrote:
> On Thu, Jan 07, 2021, Kai Huang wrote:
> > On Wed, 6 Jan 2021 14:21:39 -0800 Dave Hansen wrote:
> > > On 1/6/21 2:12 PM, Kai Huang wrote:
> > > > On Wed, 6 Jan 2021 11:39:46 -0800 Dave Hansen wrote:
> > > >> On 1/5/21 5:55 PM, Kai Huang wrote:
> > > >>> --- a/arch/x86/kernel/cpu/feat_ctl.c
> > > >>> +++ b/arch/x86/kernel/cpu/feat_ctl.c
> > > >>> @@ -97,6 +97,8 @@ static void clear_sgx_caps(void)
> > > >>> {
> > > >>> setup_clear_cpu_cap(X86_FEATURE_SGX);
> > > >>> setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
> > > >>> + setup_clear_cpu_cap(X86_FEATURE_SGX1);
> > > >>> + setup_clear_cpu_cap(X86_FEATURE_SGX2);
> > > >>> }
> > > >> Logically, I think you want this *after* the "Allow SGX virtualization
> > > >> without Launch Control support" patch. As it stands, this will totally
> > > >> disable SGX (including virtualization) if launch control is unavailable.
> > > >>
> > > > To me it is better to be here, since clear_sgx_caps(), which disables SGX
> > > > totally, should logically clear all SGX feature bits, no matter later patch's
> > > > behavior. So when new SGX bits are introduced, clear_sgx_caps() should clear
> > > > them too. Otherwise the logic of this patch (adding new SGX feature bits) is
> > > > not complete IMHO.
> > > >
> > > > And actually in later patch "Allow SGX virtualization without Launch Control
> > > > support", a new clear_sgx_lc() is added, and is called when LC is not
> > > > available but SGX virtualization is enabled, to make sure only SGX_LC bit is
> > > > cleared in this case. I don't quite understand why we need to clear SGX1 and
> > > > SGX2 in clear_sgx_caps() after the later patch.
> > >
> > > I was talking about patch ordering. It could be argued that this goes
> > > after the content of patch 05/23. Please _consider_ changing the ordering.
> > >
> > > If that doesn't work for some reason, please at least call out in the
> > > changelog that it leaves a temporarily funky situation.
> > >
> >
> > The later patch currently uses SGX1 bit, which is the reason that this patch
> > needs be before later patch.
> >
> > Sean,
> >
> > I think it is OK to remove SGX1 bit check in later patch, since I have
> > never seen a machine with SGX bit in CPUID, but w/o SGX1.
>
> The SGX1 check is "needed" to handle the case where SGX is supported but was
> soft-disabled, e.g. because software disable a machine check bank by writing an
> MCi_CTL MSR.
>
> > If we remove SGX1 bit check in later, we can put this patch after the later
> > patch.
> >
> > Do you have comment here? If you are OK, I'll remove SGX1 bit check in later
> > patch and reorder the patch.
>
> Hmm, I'm not sure why the SGX driver was merged without explicitly checking for
> SGX1 support. I'm pretty sure we had an explicit SGX1 check in the driver path
> at some point. My guess is that the SGX1 change ended up in the KVM series
> through a mishandled rebase.
>
> Moving the check later won't break anything that's not already broken. But,
> arguably checking SGX1 is a bug fix of sorts, e.g. to guard against broken
> firmware, and should go in as a standalone patch destined for stable. The
> kernel can't prevent SGX from being soft-disabled after boot, but IMO it should
> cleanly handle the case where SGX was soft-disabled _before_ boot.
It seems I need to dig some history. Thanks Sean for the info!
