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

["Huang, Kai" <kai.huang@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxxxx>
> > > + */
> > > +
> > > +#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@xxxxxxxxxxxxx
> > https://lists.xen.org/xen-devel
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel