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

["Huang, Kai" <kai.huang@linux.intel.com>]

On 7/21/2017 9:17 PM, Huang, Kai wrote:
> 
> 
> 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.

Sorry I forgot to response to this in my last reply. I looked at code 
again and yes I think we can make the code to common place. I will move 
current sgx.c to arch/x86/sgx.c. Thanks for comments.

>>
>>> ---
>>>   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.

Will do.

>>
>>> @@ -0,0 +1,208 @@
>>> +/*
>>> + * Intel Software Guard Extensions support
>>
>> Please include a GPLv2 header.

Yes will do.

Thanks,
-Kai
>>
>>> + *
>>> + * 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

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