Re: [PATCH 03/15] xen: x86: add early stage SGX feature detection

From: "Huang, Kai" <kai.huang@linux.intel.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>,
	Kai Huang <kaih.linux@gmail.com>,
	xen-devel@lists.xen.org
Cc: kevin.tian@intel.com, jbeulich@suse.com
Subject: Re: [PATCH 03/15] xen: x86: add early stage SGX feature detection
Date: Fri, 21 Jul 2017 21:17:44 +1200	[thread overview]
Message-ID: <deb9a06c-722b-b1e1-e649-86a8d46951c1@linux.intel.com> (raw)
In-Reply-To: <5931d3d6-e5b6-2fb3-c443-63899db090fb@citrix.com>

On 7/20/2017 2:23 AM, Andrew Cooper wrote:
> On 09/07/17 09:09, Kai Huang wrote:
>> This patch adds early stage SGX feature detection via SGX CPUID 0x12. Function
>> detect_sgx is added to detect SGX info on each CPU (called from vmx_cpu_up).
>> SDM says SGX info returned by CPUID is per-thread, and we cannot assume all
>> threads will return the same SGX info, so we have to detect SGX for each CPU.
>> For simplicity, currently SGX is only supported when all CPUs reports the same
>> SGX info.
>>
>> SDM also says it's possible to have multiple EPC sections but this is only for
>> multiple-socket server, which we don't support now (there are other things
>> need to be done, ex, NUMA EPC, scheduling, etc, as well), so currently only
>> one EPC is supported.
>>
>> Dedicated files sgx.c and sgx.h are added (under vmx directory as SGX is Intel
>> specific) for bulk of above SGX detection code detection code, and for further
>> SGX code as well.
>>
>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
> 
> I am not sure putting this under hvm/ is a sensible move.  Almost
> everything in this patch is currently common, and I can forsee us
> wanting to introduce PV support, so it would be good to introduce this
> in a guest-neutral location to begin with.
> 
>> ---
>>   xen/arch/x86/hvm/vmx/Makefile     |   1 +
>>   xen/arch/x86/hvm/vmx/sgx.c        | 208 ++++++++++++++++++++++++++++++++++++++
>>   xen/arch/x86/hvm/vmx/vmcs.c       |   4 +
>>   xen/include/asm-x86/cpufeature.h  |   1 +
>>   xen/include/asm-x86/hvm/vmx/sgx.h |  45 +++++++++
>>   5 files changed, 259 insertions(+)
>>   create mode 100644 xen/arch/x86/hvm/vmx/sgx.c
>>   create mode 100644 xen/include/asm-x86/hvm/vmx/sgx.h
>>
>> diff --git a/xen/arch/x86/hvm/vmx/Makefile b/xen/arch/x86/hvm/vmx/Makefile
>> index 04a29ce59d..f6bcf0d143 100644
>> --- a/xen/arch/x86/hvm/vmx/Makefile
>> +++ b/xen/arch/x86/hvm/vmx/Makefile
>> @@ -4,3 +4,4 @@ obj-y += realmode.o
>>   obj-y += vmcs.o
>>   obj-y += vmx.o
>>   obj-y += vvmx.o
>> +obj-y += sgx.o
>> diff --git a/xen/arch/x86/hvm/vmx/sgx.c b/xen/arch/x86/hvm/vmx/sgx.c
>> new file mode 100644
>> index 0000000000..6b41469371
>> --- /dev/null
>> +++ b/xen/arch/x86/hvm/vmx/sgx.c
> 
> This file looks like it should be arch/x86/sgx.c, given its current content.
> 
>> @@ -0,0 +1,208 @@
>> +/*
>> + * Intel Software Guard Extensions support
> 
> Please include a GPLv2 header.
> 
>> + *
>> + * Author: Kai Huang <kai.huang@linux.intel.com>
>> + */
>> +
>> +#include <asm/cpufeature.h>
>> +#include <asm/msr-index.h>
>> +#include <asm/msr.h>
>> +#include <asm/hvm/vmx/sgx.h>
>> +#include <asm/hvm/vmx/vmcs.h>
>> +
>> +static struct sgx_cpuinfo __read_mostly sgx_cpudata[NR_CPUS];
>> +static struct sgx_cpuinfo __read_mostly boot_sgx_cpudata;
> 
> I don't think any of this is necessary.  The description says that all
> EPCs across the server will be reported in CPUID subleaves, and our
> implementation gives up if the data are non-identical across CPUs.
> 
> Therefore, we only need to keep one copy of the data, and check check
> APs against the master copy.

Right. boot_sgx_cpudata is what we need. Currently detect_sgx is called 
from vmx_cpu_up. How about changing to calling it from identify_cpu, and 
something like below ?

	if ( c == &boot_cpu_data )
		detect_sgx(&boot_sgx_cpudata);
	else {
		struct sgx_cpuinfo tmp;
		detect_sgx(&tmp);
		if ( memcmp(&boot_sgx_cpudata, &tmp, sizeof (tmp)) )
			//disable SGX
	}

Thanks,
-Kai
> 
> 
> Let me see about splitting up a few bits of the existing CPUID
> infrastructure, so we can use the host cpuid policy more effectively for
> Xen related things.
> 
> ~Andrew
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel