Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 26 Mar 2019 07:27:19 -0700
From:   Sean Christopherson <sean.j.christopherson@intel.com>
To:     "Huang, Kai" <kai.huang@intel.com>
Cc:     "jarkko.sakkinen@linux.intel.com" <jarkko.sakkinen@linux.intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
        "x86@kernel.org" <x86@kernel.org>,
        "Svahn, Kai" <kai.svahn@intel.com>,
        "nhorman@redhat.com" <nhorman@redhat.com>,
        "josh@joshtriplett.org" <josh@joshtriplett.org>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "Ayoun, Serge" <serge.ayoun@intel.com>,
        "Huang, Haitao" <haitao.huang@intel.com>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "npmccallum@redhat.com" <npmccallum@redhat.com>,
        "rientjes@google.com" <rientjes@google.com>,
        "luto@kernel.org" <luto@kernel.org>,
        "Katz-zamir, Shay" <shay.katz-zamir@intel.com>,
        "Hansen, Dave" <dave.hansen@intel.com>,
        "bp@alien8.de" <bp@alien8.de>,
        "andriy.shevchenko@linux.intel.com" 
        <andriy.shevchenko@linux.intel.com>
Subject: Re: [PATCH v19,RESEND 08/27] x86/cpu/intel: Detect SGX support and
 update caps appropriately
Message-ID: <20190326142719.GC3757@linux.intel.com>
References: <20190320162119.4469-1-jarkko.sakkinen@linux.intel.com>
 <20190320162119.4469-9-jarkko.sakkinen@linux.intel.com>
 <1553602654.17255.15.camel@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1553602654.17255.15.camel@intel.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Mar 26, 2019 at 05:17:40AM -0700, Huang, Kai wrote:
> On Wed, 2019-03-20 at 18:21 +0200, Jarkko Sakkinen 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).
> > 
> > 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.
> 
> [Resend. Somehow my last reply doesn't show up in my mailbox so not sure whether I sent it
> successfully or not. Sorry if you receving duplicated mails.]
> 
> However this is not consistent with HW behavior. If LC feature flag is not present, then MSRs should
> have hash of Intel's key, which is not always the case here, when you expose SGX to KVM. Enclave in
> KVM guest will get unexpected EINIT error when launing Intel enclave, if on HW MSRs are configured
> to 3rd party value but locked to readonly.

Intel doesn't have a singular key.  The internal reset value of the LE
pubkey hash MSRs is micro-architectural, i.e. can change without warning
on any given processor.  All current processors with SGX support may use
the same reset value, but it's not something that customers should/can
rely on.

That being said, this in no way impacts KVM's ability to virtualize SGX,
e.g. KVM can directly do CPUID and {RD,WR}MSR to probe the capabilities
of the platform as needed.

> My opition is we already have enough cases that violates HW behavior in
> SGX virtualization, let's not have one more.

What are the other cases?

> Besides, why do we "need an additional flag to track whether or not
> Launch Control is truly enabled"? Doesn't driver only need to know whether
> MSRs are writable?

Yes, and that's why we're overloading X86_FEATURE_SGX_LC to be set if
and only if SGX_LC is supported *and* enabled, e.g. so that the kernel
can simply check X86_FEATURE_SGX_LC without having to also probe the MSRs.