From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752796AbdK1ScT (ORCPT ); Tue, 28 Nov 2017 13:32:19 -0500 Received: from mga03.intel.com ([134.134.136.65]:57313 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751796AbdK1ScR (ORCPT ); Tue, 28 Nov 2017 13:32:17 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,468,1505804400"; d="scan'208";a="7285925" Message-ID: <1511893683.9392.100.camel@intel.com> Subject: Re: [PATCH v6 04/11] x86: define IA32_FEATUE_CONTROL.SGX_LC From: Sean Christopherson To: Jarkko Sakkinen , platform-driver-x86@vger.kernel.org, x86@kernel.org Cc: linux-kernel@vger.kernel.org, Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Borislav Petkov , Janakarajan Natarajan , Paolo Bonzini , "Kirill A. Shutemov" , Kyle Huey , Vikas Shivappa , Piotr Luc , Grzegorz Andrejczuk Date: Tue, 28 Nov 2017 10:28:03 -0800 In-Reply-To: <1511889371.9392.58.camel@intel.com> References: <20171125193132.24321-1-jarkko.sakkinen@linux.intel.com> <20171125193132.24321-5-jarkko.sakkinen@linux.intel.com> <1511889371.9392.58.camel@intel.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.5.2-0ubuntu3.2 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2017-11-28 at 09:16 -0800, Sean Christopherson wrote: > On Sat, 2017-11-25 at 21:29 +0200, Jarkko Sakkinen wrote: > > > > When IA32_FEATURE_CONTROL.SGX_LC identifies that the root key for > > enclave signatures can be configured either by the OS or pre-boot > > firmware. > > > > If this the case, IA32_SGXLEPUBKEYHASHn MSRs (0 < n < 4) can be used > > to > > set the SHA256 of the root key. IA32_FEATURE_CONTROL bit 17 controls > > whether the MSRs are writable by the OS. The pre-boot firmware can > > decided whether to  set this bit before setting > > IA32_FEATURE_CONTROL.LOCK. > The commit message (feature control bit) doesn't match the patch (CPUID > bit). Also, assuming this message is destined for the commit that adds SGX_LC to feature control, I think it should first and foremost describe the hardware behavior.  The firmware vs OS interaction and use cases are valuable to document but IMO should come after the hardware description. And though it's not documented in the SDM, I think it's worthwhile to describe the SGX activation sequence and its relationship with the SGX MSRs, e.g. the LE hash MSRs are writable prior to SGX activation. Without that information, it's unclear as to how the LE hash MSRs could be different than Intel's reset value. So, maybe something like this?     After SGX is activated[1] the IA32_SGXLEPUBKEYHASHn MSRs are writable     if and only if SGX_LC is set in the IA32_FEATURE_CONTROL MSR and the     IA32_FEATURE_CONTROL MSR is locked, otherwise they are read-only.     For example, firmware can allow the OS to change the launch enclave     root key by setting IA32_FEATURE_CONTROL.SGX_LC, and thus give the     OS complete control over the enclaves it runs.  Alternatively,     firmware can clear IA32_FEATURE_CONTROL.SGX_LC to lock down the root     key and restrict the OS to running enclaves signed with the root key     or whitelisted/trusted by a launch enclave (which must be signed with     the root key).     [1] SGX related bits in IA32_FEATURE_CONTROL cannot be set until SGX         is activated, e.g. by firmware.  SGX activation is triggered by         setting bit 0 in MSR 0x7a.  Until SGX is activated, the LE hash         MSRs are writable, e.g. to allow firmware to lock down the LE         root key with a non-Intel value. > > > > > > Signed-off-by: Jarkko Sakkinen > > --- > >  arch/x86/include/asm/cpufeatures.h | 1 + > >  1 file changed, 1 insertion(+) > > > > diff --git a/arch/x86/include/asm/cpufeatures.h > > b/arch/x86/include/asm/cpufeatures.h > > index 31a7d1c0f204..43130f3c18a1 100644 > > --- a/arch/x86/include/asm/cpufeatures.h > > +++ b/arch/x86/include/asm/cpufeatures.h > > @@ -298,6 +298,7 @@ > >  #define X86_FEATURE_AVX512_VPOPCNTDQ (16*32+14) /* POPCNT for vectors > > of DW/QW */ > >  #define X86_FEATURE_LA57 (16*32+16) /* 5-level page tables */ > >  #define X86_FEATURE_RDPID (16*32+22) /* RDPID instruction */ > > +#define X86_FEATURE_SGX_LC (16*32+30) /* supports SGX launch > > configuration */ > >   > >  /* AMD-defined CPU features, CPUID level 0x80000007 (ebx), word 17 */ > >  #define X86_FEATURE_OVERFLOW_RECOV (17*32+0) /* MCA overflow recovery > > support */