Re: [Xen-devel] [RFC PATCH 00/15] RFC: SGX virtualization design and draft patches

2017-07-31 Thread Huang, Kai 

Hi Wei,

Thanks for your comments. Please see my reply below.

On 7/29/2017 1:40 AM, Wei Liu wrote:

On Tue, Jul 18, 2017 at 08:22:55PM +1200, Huang, Kai wrote:

Hi Wei,

Thank you very much for comments. Please see my reply below.

On 7/17/2017 9:16 PM, Wei Liu wrote:

Hi Kai

Thanks for this nice write-up.

Some comments and questions below.

On Sun, Jul 09, 2017 at 08:03:10PM +1200, Kai Huang wrote:

Hi all,


[...]

2. SGX Virtualization Design

2.1 High Level Toolstack Changes:

2.1.1 New 'epc' parameter

EPC is limited resource. In order to use EPC efficiently among all domains,
when creating guest, administrator should be able to specify domain's virtual
EPC size. And admin
alao should be able to get all domain's virtual EPC size.

For this purpose, a new 'epc = ' parameter is added to XL configuration
file. This parameter specifies guest's virtual EPC size. The EPC base address
will be calculated by toolstack internally, according to guest's memory size,
MMIO size, etc. 'epc' is MB in unit and any 1MB aligned value will be accepted.

2.1.2 New XL commands (?)

Administrator should be able to get physical EPC size, and all domain's virtual
EPC size. For this purpose, we can introduce 2 additional commands:

  # xl sgxinfo

Which will print out physical EPC size, and other SGX info (such as SGX1, SGX2,
etc) if necessary.

  # xl sgxlist 

Which will print out particular domain's virtual EPC size, or list all virtual
EPC sizes for all supported domains.

Alternatively, we can also extend existing XL commands by adding new option

  # xl info -sgx

Which will print out physical EPC size along with other physinfo. And

  # xl list  -sgx

Which will print out domain's virtual EPC size.

Comments?



Can a guest have multiple EPC? If so, the proposed parameter is not good
enough.


According to SDM a machine may have multiple EPC, but it may have doesn't
mean it must have. EPC is typically reserved by BIOS as Processor Reserved
Memory (PRM), and in my understanding, client machine  doesn't need to have
multiple EPC. Currently, I don't see why we need to expose multiple EPC to
guest. Even physical machine reports multiple EPC, exposing one EPC to guest
is enough. Currently SGX should not be supported with virtual NUMA
simultaneously for a single domain.



When you say "is enough", do you mean Intel doesn't recommend users to
use more than one? I don't think from reading this doc precludes using
more then one technically.


No I don't think Intel would make such recommendation. For real hardware 
yes it's possible there are multiple EPC sections, but for client or 
single socket server machine, typically there will be only one EPC. In 
case of VM, I don't see there's any benefit of exposing multiple EPCs to 
guest, except the vNUMA case. My thinking is although SDM doesn't 
preclude using more than one EPC but for VM there's no need to use more 
than one.






Can a guest with EPC enabled be migrated? The answer to this question
can lead to multiple other questions.


See the last section of my design. I saw you've already seen it. :)



Another question, is EPC going to be backed by normal memory? This is
related to memory accounting of the guest.


Although SDM says typically EPC is allocated by BIOS as PRM, but I think we
can just treat EPC as PRM, so I believe yes, physically EPC is backed by
normal memory. But EPC is reported as reserved memory in e820 table.



Is EPC going to be modeled as a device or another type of memory? This
is related to how we manage it in the toolstack.


I think we'd better to treat EPC as another type of memory. I am not sure
whether it should be modeled as device, as on real machine, EPC is also
exposed in ACPI table via "INT0E0C" device under \_SB (however it is not
modeled as PCIE device for sure).



Finally why do you not allow the users to specify the base address?


I don't see any reason why user needs to specify base address. If we do,
then specify what address? On real machine, BIOS set the base address, and
for VM, I think toolstack/Xen should do this.


We can expose an option for user to control that if they want to and at
the same time provide the logic to calculate the base address
internally. I'm not sure if that's going to be very useful, but I'm not
convinced it is entirely useless either.

Thinking a bit more we can always extend the syntax and API to support
that if need be, so I'm fine with not providing such mechanism at early
stage.


Yeah I think we can extend if needed in the future. Thanks Wei.

Thanks,
-Kai





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

Re: [Xen-devel] [PATCH 09/15] xen: vmx: handle SGX related MSRs

2017-07-21 Thread Huang, Kai 



On 7/21/2017 9:42 PM, Huang, Kai wrote:



On 7/20/2017 5:27 AM, Andrew Cooper wrote:

On 09/07/17 09:09, Kai Huang wrote:

This patch handles IA32_FEATURE_CONTROL and IA32_SGXLEPUBKEYHASHn MSRs.

For IA32_FEATURE_CONTROL, if SGX is exposed to domain, then 
SGX_ENABLE bit
is always set. If SGX launch control is also exposed to domain, and 
physical
IA32_SGXLEPUBKEYHASHn are writable, then SGX_LAUNCH_CONTROL_ENABLE 
bit is

also always set. Write to IA32_FEATURE_CONTROL is ignored.

For IA32_SGXLEPUBKEYHASHn, a new 'struct sgx_vcpu' is added for 
per-vcpu SGX
staff, and currently it has vcpu's virtual ia32_sgxlepubkeyhash[0-3]. 
Two
boolean 'readable' and 'writable' are also added to indicate whether 
virtual

IA32_SGXLEPUBKEYHASHn are readable and writable.

During vcpu is initialized, virtual ia32_sgxlepubkeyhash are also 
initialized.
If physical IA32_SGXLEPUBKEYHASHn are writable, then 
ia32_sgxlepubkeyhash are
set to Intel's default value, as for physical machine, those MSRs 
will have
Intel's default value. If physical MSRs are not writable (it is 
*locked* by
BIOS before handling to Xen), then we try to read those MSRs and use 
physical
values as defult value for virtual MSRs. One thing is rdmsr_safe is 
used, as
although SDM says if SGX is present, IA32_SGXLEPUBKEYHASHn are 
available for
read, but in reality, skylake client (at least some, depending on 
BIOS) doesn't
have those MSRs available, so we use rdmsr_safe and set readable to 
false if it

returns error code.

For IA32_SGXLEPUBKEYHASHn MSR read from guest, if physical MSRs are not
readable, guest is not allowed to read either, otherwise vcpu's 
virtual MSR

value is returned.

For IA32_SGXLEPUBKEYHASHn MSR write from guest, we allow guest to 
write if both

physical MSRs are writable and SGX launch control is exposed to domain,
otherwise error is injected.

To make EINIT run successfully in guest, vcpu's virtual 
IA32_SGXLEPUBKEYHASHn

will be update to physical MSRs when vcpu is scheduled in.

Signed-off-by: Kai Huang <kai.hu...@linux.intel.com>
---
  xen/arch/x86/hvm/vmx/sgx.c | 194 
+

  xen/arch/x86/hvm/vmx/vmx.c |  24 +
  xen/include/asm-x86/cpufeature.h   |   3 +
  xen/include/asm-x86/hvm/vmx/sgx.h  |  22 +
  xen/include/asm-x86/hvm/vmx/vmcs.h |   2 +
  xen/include/asm-x86/msr-index.h|   6 ++
  6 files changed, 251 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/sgx.c b/xen/arch/x86/hvm/vmx/sgx.c
index 14379151e8..4944e57aef 100644
--- a/xen/arch/x86/hvm/vmx/sgx.c
+++ b/xen/arch/x86/hvm/vmx/sgx.c
@@ -405,6 +405,200 @@ void hvm_destroy_epc(struct domain *d)
  hvm_reset_epc(d, true);
  }
+/* Whether IA32_SGXLEPUBKEYHASHn are physically *unlocked* by BIOS */
+bool_t sgx_ia32_sgxlepubkeyhash_writable(void)
+{
+uint64_t sgx_lc_enabled = IA32_FEATURE_CONTROL_SGX_ENABLE |
+  
IA32_FEATURE_CONTROL_SGX_LAUNCH_CONTROL_ENABLE |

+  IA32_FEATURE_CONTROL_LOCK;
+uint64_t val;
+
+rdmsrl(MSR_IA32_FEATURE_CONTROL, val);
+
+return (val & sgx_lc_enabled) == sgx_lc_enabled;
+}
+
+bool_t domain_has_sgx(struct domain *d)
+{
+/* hvm_epc_populated(d) implies CPUID has SGX */
+return hvm_epc_populated(d);
+}
+
+bool_t domain_has_sgx_launch_control(struct domain *d)
+{
+struct cpuid_policy *p = d->arch.cpuid;
+
+if ( !domain_has_sgx(d) )
+return false;
+
+/* Unnecessary but check anyway */
+if ( !cpu_has_sgx_launch_control )
+return false;
+
+return !!p->feat.sgx_launch_control;
+}


Both of these should be d->arch.cpuid->feat.{sgx,sgx_lc} only, and not
from having individual helpers.

The CPUID setup during host boot and domain construction should take
care of setting everything up properly, or hiding the features from the
guest.  The point of the work I've been doing is to prevent situations
where the guest can see SGX but something doesn't work because of Xen
using nested checks like this.


Thanks for comments. Will change to simple check against 
d->arch.cpuid->feat.{sgx,sgx_lc}.





+
+/* Digest of Intel signing key. MSR's default value after reset. */
+#define SGX_INTEL_DEFAULT_LEPUBKEYHASH0 0xa6053e051270b7ac
+#define SGX_INTEL_DEFAULT_LEPUBKEYHASH1 0x6cfbe8ba8b3b413d
+#define SGX_INTEL_DEFAULT_LEPUBKEYHASH2 0xc4916d99f2b3735d
+#define SGX_INTEL_DEFAULT_LEPUBKEYHASH3 0xd4f8c05909f9bb3b
+
+void sgx_vcpu_init(struct vcpu *v)
+{
+struct sgx_vcpu *sgxv = to_sgx_vcpu(v);
+
+memset(sgxv, 0, sizeof (*sgxv));
+
+if ( sgx_ia32_sgxlepubkeyhash_writable() )
+{
+/*
+ * If physical MSRs are writable, set vcpu's default value 
to Intel's
+ * default value. For real machine, after reset, MSRs 
contain Intel's

+ * default value.
+ */
+sgxv->ia32_sgxlepubkeyhash[0] = 
SGX_INTEL_DEFAULT_LEPUBKEYHASH0;
+sgxv->ia32_sgxlepubkeyhash[1] = 
SGX_INTEL_DEFAULT_LEPUBKEYHASH1;
+

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

2017-07-21 Thread Huang, Kai 



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.hu...@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 00..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.hu...@linux.intel.com>
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+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 == _cpu_data )
 detect_sgx(_sgx_cpudata);
 else {
 struct sgx_cpuinfo tmp;
 detect_sgx();
 if ( memcmp(_sgx_cpudata, , 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

Re: [Xen-devel] [PATCH 09/15] xen: vmx: handle SGX related MSRs

2017-07-21 Thread Huang, Kai 



On 7/20/2017 5:27 AM, Andrew Cooper wrote:

On 09/07/17 09:09, Kai Huang wrote:

This patch handles IA32_FEATURE_CONTROL and IA32_SGXLEPUBKEYHASHn MSRs.

For IA32_FEATURE_CONTROL, if SGX is exposed to domain, then SGX_ENABLE bit
is always set. If SGX launch control is also exposed to domain, and physical
IA32_SGXLEPUBKEYHASHn are writable, then SGX_LAUNCH_CONTROL_ENABLE bit is
also always set. Write to IA32_FEATURE_CONTROL is ignored.

For IA32_SGXLEPUBKEYHASHn, a new 'struct sgx_vcpu' is added for per-vcpu SGX
staff, and currently it has vcpu's virtual ia32_sgxlepubkeyhash[0-3]. Two
boolean 'readable' and 'writable' are also added to indicate whether virtual
IA32_SGXLEPUBKEYHASHn are readable and writable.

During vcpu is initialized, virtual ia32_sgxlepubkeyhash are also initialized.
If physical IA32_SGXLEPUBKEYHASHn are writable, then ia32_sgxlepubkeyhash are
set to Intel's default value, as for physical machine, those MSRs will have
Intel's default value. If physical MSRs are not writable (it is *locked* by
BIOS before handling to Xen), then we try to read those MSRs and use physical
values as defult value for virtual MSRs. One thing is rdmsr_safe is used, as
although SDM says if SGX is present, IA32_SGXLEPUBKEYHASHn are available for
read, but in reality, skylake client (at least some, depending on BIOS) doesn't
have those MSRs available, so we use rdmsr_safe and set readable to false if it
returns error code.

For IA32_SGXLEPUBKEYHASHn MSR read from guest, if physical MSRs are not
readable, guest is not allowed to read either, otherwise vcpu's virtual MSR
value is returned.

For IA32_SGXLEPUBKEYHASHn MSR write from guest, we allow guest to write if both
physical MSRs are writable and SGX launch control is exposed to domain,
otherwise error is injected.

To make EINIT run successfully in guest, vcpu's virtual IA32_SGXLEPUBKEYHASHn
will be update to physical MSRs when vcpu is scheduled in.

Signed-off-by: Kai Huang 
---
  xen/arch/x86/hvm/vmx/sgx.c | 194 +
  xen/arch/x86/hvm/vmx/vmx.c |  24 +
  xen/include/asm-x86/cpufeature.h   |   3 +
  xen/include/asm-x86/hvm/vmx/sgx.h  |  22 +
  xen/include/asm-x86/hvm/vmx/vmcs.h |   2 +
  xen/include/asm-x86/msr-index.h|   6 ++
  6 files changed, 251 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/sgx.c b/xen/arch/x86/hvm/vmx/sgx.c
index 14379151e8..4944e57aef 100644
--- a/xen/arch/x86/hvm/vmx/sgx.c
+++ b/xen/arch/x86/hvm/vmx/sgx.c
@@ -405,6 +405,200 @@ void hvm_destroy_epc(struct domain *d)
  hvm_reset_epc(d, true);
  }
  
+/* Whether IA32_SGXLEPUBKEYHASHn are physically *unlocked* by BIOS */

+bool_t sgx_ia32_sgxlepubkeyhash_writable(void)
+{
+uint64_t sgx_lc_enabled = IA32_FEATURE_CONTROL_SGX_ENABLE |
+  IA32_FEATURE_CONTROL_SGX_LAUNCH_CONTROL_ENABLE |
+  IA32_FEATURE_CONTROL_LOCK;
+uint64_t val;
+
+rdmsrl(MSR_IA32_FEATURE_CONTROL, val);
+
+return (val & sgx_lc_enabled) == sgx_lc_enabled;
+}
+
+bool_t domain_has_sgx(struct domain *d)
+{
+/* hvm_epc_populated(d) implies CPUID has SGX */
+return hvm_epc_populated(d);
+}
+
+bool_t domain_has_sgx_launch_control(struct domain *d)
+{
+struct cpuid_policy *p = d->arch.cpuid;
+
+if ( !domain_has_sgx(d) )
+return false;
+
+/* Unnecessary but check anyway */
+if ( !cpu_has_sgx_launch_control )
+return false;
+
+return !!p->feat.sgx_launch_control;
+}


Both of these should be d->arch.cpuid->feat.{sgx,sgx_lc} only, and not
from having individual helpers.

The CPUID setup during host boot and domain construction should take
care of setting everything up properly, or hiding the features from the
guest.  The point of the work I've been doing is to prevent situations
where the guest can see SGX but something doesn't work because of Xen
using nested checks like this.


Thanks for comments. Will change to simple check against 
d->arch.cpuid->feat.{sgx,sgx_lc}.





+
+/* Digest of Intel signing key. MSR's default value after reset. */
+#define SGX_INTEL_DEFAULT_LEPUBKEYHASH0 0xa6053e051270b7ac
+#define SGX_INTEL_DEFAULT_LEPUBKEYHASH1 0x6cfbe8ba8b3b413d
+#define SGX_INTEL_DEFAULT_LEPUBKEYHASH2 0xc4916d99f2b3735d
+#define SGX_INTEL_DEFAULT_LEPUBKEYHASH3 0xd4f8c05909f9bb3b
+
+void sgx_vcpu_init(struct vcpu *v)
+{
+struct sgx_vcpu *sgxv = to_sgx_vcpu(v);
+
+memset(sgxv, 0, sizeof (*sgxv));
+
+if ( sgx_ia32_sgxlepubkeyhash_writable() )
+{
+/*
+ * If physical MSRs are writable, set vcpu's default value to Intel's
+ * default value. For real machine, after reset, MSRs contain Intel's
+ * default value.
+ */
+sgxv->ia32_sgxlepubkeyhash[0] = SGX_INTEL_DEFAULT_LEPUBKEYHASH0;
+sgxv->ia32_sgxlepubkeyhash[1] = SGX_INTEL_DEFAULT_LEPUBKEYHASH1;
+sgxv->ia32_sgxlepubkeyhash[2] = SGX_INTEL_DEFAULT_LEPUBKEYHASH2;
+sgxv->ia32_sgxlepubkeyhash[3] =

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

2017-07-21 Thread Huang, Kai 



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 


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 00..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 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+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 == _cpu_data )
detect_sgx(_sgx_cpudata);
else {
struct sgx_cpuinfo tmp;
detect_sgx();
if ( memcmp(_sgx_cpudata, , 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

Re: [Xen-devel] [RFC PATCH 00/15] RFC: SGX virtualization design and draft patches

2017-07-21 Thread Huang, Kai 



On 7/17/2017 6:08 PM, Huang, Kai wrote:

Hi Andrew,

Thank you very much for comments. Sorry for late reply, and please see 
my reply below.


On 7/12/2017 2:13 AM, Andrew Cooper wrote:

On 09/07/17 09:03, Kai Huang wrote:

Hi all,

This series is RFC Xen SGX virtualization support design and RFC 
draft patches.


Thankyou very much for this design doc.


2. SGX Virtualization Design

2.1 High Level Toolstack Changes:

2.1.1 New 'epc' parameter

EPC is limited resource. In order to use EPC efficiently among all 
domains,
when creating guest, administrator should be able to specify domain's 
virtual

EPC size. And admin
alao should be able to get all domain's virtual EPC size.

For this purpose, a new 'epc = ' parameter is added to XL 
configuration
file. This parameter specifies guest's virtual EPC size. The EPC base 
address
will be calculated by toolstack internally, according to guest's 
memory size,
MMIO size, etc. 'epc' is MB in unit and any 1MB aligned value will be 
accepted.


How will this interact with multi-package servers?  Even though its 
fine to implement the single-package support first, the design should 
be extensible to the multi-package case.


First of all, what are the implications of multi-package SGX?

(Somewhere) you mention changes to scheduling.  I presume this is 
because a guest with EPC mappings in EPT must be scheduled on the same 
package, or ENCLU[EENTER] will fail.  I presume also that each package 
will have separate, unrelated private keys?


The ENCLU[EENTE] will continue to work on multi-package server. Actually 
I was told all ISA existing behavior documented in SDM won't change for 
server, as otherwise this would be a bad design :)


Unfortunately I was told I cannot talk about MP server SGX a lot now. 
Basically I can only talk about staff already documented in SDM (sorry 
:( ). But I guess multiple EPC in CPUID is designed to cover MP server, 
at lease mainly (we can do reasonable guess).


In terms of the design, I think we can follow XL config file parameters 
for memory. 'epc' parameter will always specify totol EPC size that the 
domain has. And we can use existing NUMA related parameters, such as 
setting cpus='...' to physically pin vcpu to specific pCPUs, so that EPC 
will be mostly allocated from related node. If that node runs out of 
EPC, we can decide whether to allocate EPC from other node, or fail to 
create domain. I know Linux supports NUMA policy which can specify 
whether to allow allocating memory from other nodes, does Xen has such 
policy? Sorry I haven't checked this. If Xen has such policy, we need to 
choose whether to use memory policy, or introduce new policy for EPC.


If we are going to support vNUAM EPC in the future. We can also use 
similar way to config vNUMA EPC in XL config.


Sorry I mentioned scheduling. I should say *potentially* :). My thinking 
was as SGX is per-thread, then SGX info reported by different CPU 
package may be different (ex, whether SGX2 is supported), then we may 
need scheduler to be aware of SGX. But I think we don't have to consider 
this now.


What's your comments?



I presume there is no sensible way (even on native) for a single 
logical process to use multiple different enclaves?  By extension, 
does it make sense to try and offer parts of multiple enclaves to a 
single VM?


The native machine allows running multiple enclaves, even signed by 
multiple authors. SGX only has limit that before launching any other 
enclave, Launch Enclave (LE) must be launched. LE is the only enclave 
that doesn't require EINITTOKEN in EINIT. For LE, its signer 
(SHA256(sigstruct->modulus)) must be equal to the value in 
IA32_SGXLEPUBKEYHASHn MSRs. LE will generates EINITTOKEN for other 
enclaves (EINIT for other enclaves requires EINITTOKEN). For other 
enclaves, there's no such limitation that enclave's signer must match 
IA32_SGXLEPUBKEYHASHn so the signer can be anybody. But for other 
enclaves, before running EINIT, the LE's signer (which is equal to 
IA32_SGXLEPUBKEYHASHn as explained above) needs to be updated to 
IA32_SGXLEPUBKEYHASHn (MSRs can be changed, for example, when there's 
multiple LEs running in OS). This is because EINIT needs to perform 
EINITTOKEN integrity check (EINITTOKEN contains MAC info that calculated 
by LE, and EINIT needs LE's IA32_SGXLEPUBKEYHASHn to derive the key to 
verify MAC).


SGX in VM doesn't change those behaviors, so in VM, the enclaves can 
also be signed by anyone, but Xen needs to emulate IA32_SGXLEPUBKEYHASHn 
so that when one VM is running, the correct IA32_SGXLEPUBKEYHASHn are 
already in physical MSRs.





2.1.3 Notify domain's virtual EPC base and size to Xen

Xen needs to know guest's EPC base and size in order to populate EPC 
pages for
it. Toolstack notifies EPC base and size to Xen via 
XEN_DOMCTL_set_cpuid.


I am currently in the process of reworking the Xen/Toolstack interface 
when it comes to CPUID handling.  The latest design is available here: 
ht

Re: [Xen-devel] [PATCH 15/15] xen: tools: expose EPC in ACPI table

2017-07-18 Thread Huang, Kai 



On 7/18/2017 10:21 PM, Roger Pau Monné wrote:

On Tue, Jul 18, 2017 at 08:36:15PM +1200, Huang, Kai wrote:



On 7/17/2017 10:54 PM, Roger Pau Monné wrote:

On Sun, Jul 09, 2017 at 08:16:05PM +1200, Kai Huang wrote:

On physical machine EPC is exposed in ACPI table via "INT0E0C". Although EPC
can be discovered by CPUID but Windows driver requires EPC to be exposed in
ACPI table as well. This patch exposes EPC in ACPI table.

Signed-off-by: Kai Huang <kai.hu...@linux.intel.com>
---
   tools/firmware/hvmloader/util.c  | 23 +++
   tools/firmware/hvmloader/util.h  |  3 +++


Is there any reason this needs to be done in hvmloader instead of
libacpi? I'm mostly asking this because PVH guests can also get ACPI
tables, so it would be good to be able to expose EPC to them using
ACPI.


Hi Roger,

Thanks for comments. I didn't deliberately choose to do in hvmloader instead
of libacpi. It seems libxl only builds ACPI table when guest is HVM, and it
doesn't use any device model, and I think I have covered this part (see
changes to init_acpi_config). Is there anything that I missed?


dsdt.asl is only used for HVM guests, PVH guests basically get an
empty dsdt + dsdt_acpi_info + processor objects populated by make_dsdt
(see Makefile in libacpi), so they end up without the EPC Device
block.

It would be good if a new empty dsdt is created, that contains the
Device EPC block, or a ssdt is used, and it's added to both HVM/PVH
guests.

Alternatively you could also code the EPC Device block in mk_dsdt, but
that's going to be cumbersome IMHO.


Hi Roger,

I got your point. I think it's definitely better to cover PVH if we can. 
Let me see whether it is possible to do.


Thanks,
-Kai


Thanks, Roger.



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

Re: [Xen-devel] [PATCH 01/15] xen: x86: expose SGX to HVM domain in CPU featureset

2017-07-18 Thread Huang, Kai 



On 7/18/2017 10:12 PM, Andrew Cooper wrote:

On 09/07/17 09:04, Kai Huang wrote:

Expose SGX in CPU featureset for HVM domain. SGX will not be supported for
PV domain, as ENCLS (which SGX driver in guest essentially runs) must run
in ring 0, while PV kernel runs in ring 3. Theoretically we can support SGX
in PV domain via either emulating #GP caused by ENCLS running in ring 3, or
by PV ENCLS but it is really not necessary at this stage. And currently SGX
is only exposed to HAP HVM domain (we can add for shadow in the future).

SGX Launch Control is also exposed in CPU featureset for HVM domain. SGX
Launch Control depends on SGX.

Signed-off-by: Kai Huang 
---
  xen/include/public/arch-x86/cpufeatureset.h | 3 ++-
  xen/tools/gen-cpuid.py  | 3 +++
  2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/include/public/arch-x86/cpufeatureset.h 
b/xen/include/public/arch-x86/cpufeatureset.h
index 97dd3534c5..b6c54e654e 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -193,7 +193,7 @@ XEN_CPUFEATURE(XSAVES,4*32+ 3) /*S  XSAVES/XRSTORS 
instructions */
  /* Intel-defined CPU features, CPUID level 0x0007:0.ebx, word 5 */
  XEN_CPUFEATURE(FSGSBASE,  5*32+ 0) /*A  {RD,WR}{FS,GS}BASE instructions */
  XEN_CPUFEATURE(TSC_ADJUST,5*32+ 1) /*S  TSC_ADJUST MSR available */
-XEN_CPUFEATURE(SGX,   5*32+ 2) /*   Software Guard extensions */
+XEN_CPUFEATURE(SGX,   5*32+ 2) /*H  Intel Software Guard extensions */
  XEN_CPUFEATURE(BMI1,  5*32+ 3) /*A  1st bit manipulation extensions */
  XEN_CPUFEATURE(HLE,   5*32+ 4) /*A  Hardware Lock Elision */
  XEN_CPUFEATURE(AVX2,  5*32+ 5) /*A  AVX2 instructions */
@@ -229,6 +229,7 @@ XEN_CPUFEATURE(PKU,   6*32+ 3) /*H  Protection Keys 
for Userspace */
  XEN_CPUFEATURE(OSPKE, 6*32+ 4) /*!  OS Protection Keys Enable */
  XEN_CPUFEATURE(AVX512_VPOPCNTDQ, 6*32+14) /*A  POPCNT for vectors of DW/QW */
  XEN_CPUFEATURE(RDPID, 6*32+22) /*A  RDPID instruction */
+XEN_CPUFEATURE(SGX_LAUNCH_CONTROL, 6*32+30) /*H Intel SGX Launch Control */


Could we abbreviate this to SGX_LC ?  It is certainly rather shorter to
write, and appears to be used elsewhere.


Sure. Will do.

Thanks,
-Kai


~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

Re: [Xen-devel] [PATCH 15/15] xen: tools: expose EPC in ACPI table

2017-07-18 Thread Huang, Kai 



On 7/17/2017 10:54 PM, Roger Pau Monné wrote:

On Sun, Jul 09, 2017 at 08:16:05PM +1200, Kai Huang wrote:

On physical machine EPC is exposed in ACPI table via "INT0E0C". Although EPC
can be discovered by CPUID but Windows driver requires EPC to be exposed in
ACPI table as well. This patch exposes EPC in ACPI table.

Signed-off-by: Kai Huang 
---
  tools/firmware/hvmloader/util.c  | 23 +++
  tools/firmware/hvmloader/util.h  |  3 +++


Is there any reason this needs to be done in hvmloader instead of
libacpi? I'm mostly asking this because PVH guests can also get ACPI
tables, so it would be good to be able to expose EPC to them using
ACPI.


Hi Roger,

Thanks for comments. I didn't deliberately choose to do in hvmloader 
instead of libacpi. It seems libxl only builds ACPI table when guest is 
HVM, and it doesn't use any device model, and I think I have covered 
this part (see changes to init_acpi_config). Is there anything that I 
missed?


Thanks,
-Kai


Thanks, Roger.

___
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

Re: [Xen-devel] [RFC PATCH 00/15] RFC: SGX virtualization design and draft patches

2017-07-18 Thread Huang, Kai 

Hi Wei,

Thank you very much for comments. Please see my reply below.

On 7/17/2017 9:16 PM, Wei Liu wrote:

Hi Kai

Thanks for this nice write-up.

Some comments and questions below.

On Sun, Jul 09, 2017 at 08:03:10PM +1200, Kai Huang wrote:

Hi all,


[...]

2. SGX Virtualization Design

2.1 High Level Toolstack Changes:

2.1.1 New 'epc' parameter

EPC is limited resource. In order to use EPC efficiently among all domains,
when creating guest, administrator should be able to specify domain's virtual
EPC size. And admin
alao should be able to get all domain's virtual EPC size.

For this purpose, a new 'epc = ' parameter is added to XL configuration
file. This parameter specifies guest's virtual EPC size. The EPC base address
will be calculated by toolstack internally, according to guest's memory size,
MMIO size, etc. 'epc' is MB in unit and any 1MB aligned value will be accepted.

2.1.2 New XL commands (?)

Administrator should be able to get physical EPC size, and all domain's virtual
EPC size. For this purpose, we can introduce 2 additional commands:

 # xl sgxinfo

Which will print out physical EPC size, and other SGX info (such as SGX1, SGX2,
etc) if necessary.

 # xl sgxlist 

Which will print out particular domain's virtual EPC size, or list all virtual
EPC sizes for all supported domains.

Alternatively, we can also extend existing XL commands by adding new option

 # xl info -sgx

Which will print out physical EPC size along with other physinfo. And

 # xl list  -sgx

Which will print out domain's virtual EPC size.

Comments?



Can a guest have multiple EPC? If so, the proposed parameter is not good
enough.


According to SDM a machine may have multiple EPC, but it may have 
doesn't mean it must have. EPC is typically reserved by BIOS as 
Processor Reserved Memory (PRM), and in my understanding, client machine 
 doesn't need to have multiple EPC. Currently, I don't see why we need 
to expose multiple EPC to guest. Even physical machine reports multiple 
EPC, exposing one EPC to guest is enough. Currently SGX should not be 
supported with virtual NUMA simultaneously for a single domain.




Can a guest with EPC enabled be migrated? The answer to this question
can lead to multiple other questions.


See the last section of my design. I saw you've already seen it. :)



Another question, is EPC going to be backed by normal memory? This is
related to memory accounting of the guest.


Although SDM says typically EPC is allocated by BIOS as PRM, but I think 
we can just treat EPC as PRM, so I believe yes, physically EPC is backed 
by normal memory. But EPC is reported as reserved memory in e820 table.




Is EPC going to be modeled as a device or another type of memory? This
is related to how we manage it in the toolstack.


I think we'd better to treat EPC as another type of memory. I am not 
sure whether it should be modeled as device, as on real machine, EPC is 
also exposed in ACPI table via "INT0E0C" device under \_SB (however it 
is not modeled as PCIE device for sure).




Finally why do you not allow the users to specify the base address?


I don't see any reason why user needs to specify base address. If we do, 
then specify what address? On real machine, BIOS set the base address, 
and for VM, I think toolstack/Xen should do this.





In my RFC patches I didn't implement the commands as I don't know which
is better. In the github repo I mentioned at the beginning, there's an old
branch in which I implemented 'xl sgxinfo' and 'xl sgxlist', but they are
implemented via dedicated hypercall for SGX, which I am not sure whether is a
good option so I didn't include it in my RFC patches.

2.1.3 Notify domain's virtual EPC base and size to Xen

Xen needs to know guest's EPC base and size in order to populate EPC pages for
it. Toolstack notifies EPC base and size to Xen via XEN_DOMCTL_set_cpuid.

2.1.4 Launch Control Support (?)

[...]


But maybe integrating EPC to MM framework is more reasonable. Comments?

2.2.2 EPC Virtualization (?)

This part is how to populate EPC for guests. We have 3 choices:
 - Static Partitioning
 - Oversubscription
 - Ballooning



IMHO static partitioning is good enough as a starting point.

Ballooning is nice to have but please don't make it mandatory. Not all
guests have balloon driver -- imagine a unikernel style secure domain
running with EPC.


That's good point. Thanks.





2.3 Additional Point: Live Migration, Snapshot Support (?)



Oh, here it is. Nice.


Actually from hardware's point of view, SGX is not migratable. There are two
reasons:

 - SGX key architecture cannot be virtualized.

 For example, some keys are bound to CPU. For example, Sealing key, EREPORT
 key, etc. If VM is migrated to another machine, the same enclave will 
derive
 the different keys. Taking Sealing key as an example, Sealing key is
 typically used by enclave (enclave can get sealing key by EGETKEY) to 
*seal*
 its secrets to

Re: [Xen-devel] [PATCH 01/15] xen: x86: expose SGX to HVM domain in CPU featureset

2017-07-17 Thread Huang, Kai 



On 7/12/2017 11:09 PM, Andrew Cooper wrote:

On 09/07/17 10:04, Kai Huang wrote:
Expose SGX in CPU featureset for HVM domain. SGX will not be supported 
for

PV domain, as ENCLS (which SGX driver in guest essentially runs) must run
in ring 0, while PV kernel runs in ring 3. Theoretically we can 
support SGX
in PV domain via either emulating #GP caused by ENCLS running in ring 
3, or
by PV ENCLS but it is really not necessary at this stage. And 
currently SGX

is only exposed to HAP HVM domain (we can add for shadow in the future).

SGX Launch Control is also exposed in CPU featureset for HVM domain. SGX
Launch Control depends on SGX.

Signed-off-by: Kai Huang 


I think its perfectly reasonable to restrict to HVM guests to start 
with, although I don't see how shadow vs HAP has any impact at this 
stage?  All that matters is that the EPC pages appear in the guests p2m.


Hmm it seems I forgot replying this one. Sorry. Actually there's no 
difference between shadow and HAP SGX, as currently SGX functionality is 
not depending on EPT. I didn't expose SGX to shadow as I haven't got 
chance to implement and test shadow part. I will add shadow support in 
next version.


Thanks,
-Kai


~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

Re: [Xen-devel] [PATCH 08/15] xen: x86: add SGX cpuid handling support.

2017-07-17 Thread Huang, Kai 



On 7/14/2017 7:37 PM, Andrew Cooper wrote:

On 13/07/17 07:42, Huang, Kai wrote:

On 7/12/2017 10:56 PM, Andrew Cooper wrote:

On 09/07/17 10:10, Kai Huang wrote:

Why do we need this hide_epc parameter?  If we aren't providing any 
epc resource to the guest, the entire sgx union should be zero and 
the SGX feature bit should be hidden.


My intention was to hide physical EPC info for pv_max_policy and 
hvm_max_policy (recalculate_sgx is also called by 
calculate_pv_max_policy and calculate_hvm_max_policy), as they are for 
guest and don't need physical EPC info. But keeping physical EPC info 
in them does no harm so I think we can simply remove hide_epc.


It is my experience that providing half the information is worse than 
providing none or all of it, because developers are notorious for taking 
shortcuts when looking for features.


Patch 1 means that a PV guest will never have p->feat.sgx set. 
Therefore, we will hit the memset() below, and zero the whole of the SGX 
union.


Yes I'll remove hide_epc. It is not absolutely needed.





IMO we cannot check whether EPC is valid and zero sgx union in 
recalculate_sgx, as it is called for each CPUID. For example, it is 
called for SGX subleaf 0, and 1, and then 2, and when subleaf 0 and 1 
are called, the EPC resource is 0 (hasn't been configured).


recalculate_*() only get called when the toolstack makes updates to the 
policy.  It is an unfortunate side effect of the current implementation, 
but will be going away with my CPUID work.


The intended flow will be this:

At Xen boot:
* Calculates the raw, host and max policies (as we do today)

At domain create:
* Appropriate policy gets copied to make the default domain policy.
* Toolstack gets the whole policy at one with a new 
DOMCTL_get_cpuid_policy hypercall.
* Toolstack makes all adjustments (locally) that it wants to, based on 
configuration, etc.

* Toolstack makes a single DOMCTL_set_cpuid_policy hypercall.
* Xen audits the new policy proposed by the toolstack, resulting in a 
single yes/no decision.
** If not, the toolstack is told to try again.  This will likely result 
in xl asking the user to modify their .cfg file.

** If yes, the proposed policy becomes the actual policy.

This scheme will fix the current problem we have where the toolstack 
blindly proposes changes (one leaf at a time), and Xen has to zero the 
bits it doesn't like (because the toolstack has never traditionally 
checked the return value of the hypercall :( )


This is actually what I was looking for when implementing CPUID support 
for SGX. I think I'll wait for your work to be merged to Xen and then do 
my work above your work. :)


Thanks,
-Kai









+
+/* Subleaf 2. */
+uint32_t base_valid:1, :11, base_pfn_low:20;
+uint32_t base_pfn_high:20, :12;
+uint32_t size_valid:1, :11, npages_low:20;
+uint32_t npages_high:20, :12;
+};


Are the {base,size}_valid fields correct?  The manual says the are 
4-bit fields rather than single bit fields.


They are 4 bits in SDM but actually currently only bit 1 is valid 
(other values are reserved). I think for now bool base_valid should be 
enough. We can extend when new values come out. What's your suggestion?


Ok.  That can work for now.





I would also drop the _pfn from the base names.  The fields still 
need shifting to get a sensible value.


OK. Will do.


As a further thought, what about uint64_t base:40 and size:40?  That 
would reduce the complexity of calculating the values.


~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

Re: [Xen-devel] [PATCH 15/15] xen: tools: expose EPC in ACPI table

2017-07-17 Thread Huang, Kai 



On 7/14/2017 11:31 PM, Jan Beulich wrote:

On 09.07.17 at 10:16,  wrote:

--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -330,6 +330,15 @@ cpuid(uint32_t idx, uint32_t *eax, uint32_t *ebx, uint32_t 
*ecx, uint32_t *edx)
  : "0" (idx) );
  }
  
+void cpuid_count(uint32_t idx, uint32_t count, uint32_t *eax,


Please name the first two leaf and subleaf.


Sure will do.




@@ -888,6 +897,18 @@ static uint8_t acpi_lapic_id(unsigned cpu)
  return LAPIC_ID(cpu);
  }
  
+static void get_epc_info(struct acpi_config *config)

+{
+uint32_t eax, ebx, ecx, edx;
+
+cpuid_count(0x12, 0x2, , , , );
+
+config->epc_base = (((uint64_t)(ebx & 0xf)) << 32) |
+   (uint64_t)(eax & 0xf000);


Pointless cast.


+config->epc_size = (((uint64_t)(edx & 0xf)) << 32) |
+   (uint64_t)(ecx & 0xf000);


Again.


Will do.




--- a/tools/libacpi/dsdt.asl
+++ b/tools/libacpi/dsdt.asl
@@ -441,6 +441,55 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "Xen", "HVM", 0)
  }
  }
  }
+
+Device (EPC)
+{
+Name (_HID, EisaId ("INT0E0C"))
+Name (_STR, Unicode ("Enclave Page Cache 1.5"))
+Name (_MLS, Package (0x01)
+{
+Package (0x02)
+{
+"en",
+Unicode ("Enclave Page Cache 1.5")
+}
+})
+Name (RBUF, ResourceTemplate ()
+{
+QWordMemory (ResourceConsumer, PosDecode, MinFixed, MaxFixed,
+Cacheable, ReadWrite,
+0x, // Granularity
+0x, // Range Minimum
+0x, // Range Maximum
+0x, // Translation Offset
+0x0001, // Length
+,, _Y03,
+AddressRangeMemory, TypeStatic)
+})
+
+Method(_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
+{
+CreateQwordField (RBUF, \_SB.EPC._Y03._MIN, EMIN) // _MIN: 
Minimuum Base Address
+CreateQwordField (RBUF, \_SB.EPC._Y03._MAX, EMAX) // _MIN: 
Maximum Base Address
+CreateQwordField (RBUF, \_SB.EPC._Y03._LEN, ELEN) // _LEN: 
Length


Please see the comment in _SB.PCI0._CRS regarding operations
on qword fields. Even if we may not formally support the named
Windows versions anymore, we should continue to be careful
here. You could have noticed this by seeing that ...


@@ -21,6 +21,8 @@
 LMIN, 32,
 HMIN, 32,
 LLEN, 32,
-   HLEN, 32
+   HLEN, 32,
+   EMIN, 64,
+   ELEN, 64,
 }


... there have been no 64-bit fields here so far.


Thank you for pointing this out. I'll take a look.




@@ -156,6 +156,9 @@ static int init_acpi_config(libxl__gc *gc,
  config->lapic_id = acpi_lapic_id;
  config->acpi_revision = 5;
  
+config->epc_base = b_info->u.hvm.sgx.epcbase;

+config->epc_size = (b_info->u.hvm.sgx.epckb << 10);


Pointless parentheses. Plus I guess the field names could do with
an underscore separator in the middle - it took me a moment to
realize this is a kB value (explaining the shift by 10).


Sure. will change to epc_kb and epc_base :)

Thanks,
-Kai


Jan


___
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

Re: [Xen-devel] [RFC PATCH 00/15] RFC: SGX virtualization design and draft patches

2017-07-17 Thread Huang, Kai 

Hi Andrew,

Thank you very much for comments. Sorry for late reply, and please see 
my reply below.


On 7/12/2017 2:13 AM, Andrew Cooper wrote:

On 09/07/17 09:03, Kai Huang wrote:

Hi all,

This series is RFC Xen SGX virtualization support design and RFC draft 
patches.


Thankyou very much for this design doc.


2. SGX Virtualization Design

2.1 High Level Toolstack Changes:

2.1.1 New 'epc' parameter

EPC is limited resource. In order to use EPC efficiently among all 
domains,
when creating guest, administrator should be able to specify domain's 
virtual

EPC size. And admin
alao should be able to get all domain's virtual EPC size.

For this purpose, a new 'epc = ' parameter is added to XL 
configuration
file. This parameter specifies guest's virtual EPC size. The EPC base 
address
will be calculated by toolstack internally, according to guest's 
memory size,
MMIO size, etc. 'epc' is MB in unit and any 1MB aligned value will be 
accepted.


How will this interact with multi-package servers?  Even though its fine 
to implement the single-package support first, the design should be 
extensible to the multi-package case.


First of all, what are the implications of multi-package SGX?

(Somewhere) you mention changes to scheduling.  I presume this is 
because a guest with EPC mappings in EPT must be scheduled on the same 
package, or ENCLU[EENTER] will fail.  I presume also that each package 
will have separate, unrelated private keys?


The ENCLU[EENTE] will continue to work on multi-package server. Actually 
I was told all ISA existing behavior documented in SDM won't change for 
server, as otherwise this would be a bad design :)


Unfortunately I was told I cannot talk about MP server SGX a lot now. 
Basically I can only talk about staff already documented in SDM (sorry 
:( ). But I guess multiple EPC in CPUID is designed to cover MP server, 
at lease mainly (we can do reasonable guess).


In terms of the design, I think we can follow XL config file parameters 
for memory. 'epc' parameter will always specify totol EPC size that the 
domain has. And we can use existing NUMA related parameters, such as 
setting cpus='...' to physically pin vcpu to specific pCPUs, so that EPC 
will be mostly allocated from related node. If that node runs out of 
EPC, we can decide whether to allocate EPC from other node, or fail to 
create domain. I know Linux supports NUMA policy which can specify 
whether to allow allocating memory from other nodes, does Xen has such 
policy? Sorry I haven't checked this. If Xen has such policy, we need to 
choose whether to use memory policy, or introduce new policy for EPC.


If we are going to support vNUAM EPC in the future. We can also use 
similar way to config vNUMA EPC in XL config.


Sorry I mentioned scheduling. I should say *potentially* :). My thinking 
was as SGX is per-thread, then SGX info reported by different CPU 
package may be different (ex, whether SGX2 is supported), then we may 
need scheduler to be aware of SGX. But I think we don't have to consider 
this now.


What's your comments?



I presume there is no sensible way (even on native) for a single logical 
process to use multiple different enclaves?  By extension, does it make 
sense to try and offer parts of multiple enclaves to a single VM?


The native machine allows running multiple enclaves, even signed by 
multiple authors. SGX only has limit that before launching any other 
enclave, Launch Enclave (LE) must be launched. LE is the only enclave 
that doesn't require EINITTOKEN in EINIT. For LE, its signer 
(SHA256(sigstruct->modulus)) must be equal to the value in 
IA32_SGXLEPUBKEYHASHn MSRs. LE will generates EINITTOKEN for other 
enclaves (EINIT for other enclaves requires EINITTOKEN). For other 
enclaves, there's no such limitation that enclave's signer must match 
IA32_SGXLEPUBKEYHASHn so the signer can be anybody. But for other 
enclaves, before running EINIT, the LE's signer (which is equal to 
IA32_SGXLEPUBKEYHASHn as explained above) needs to be updated to 
IA32_SGXLEPUBKEYHASHn (MSRs can be changed, for example, when there's 
multiple LEs running in OS). This is because EINIT needs to perform 
EINITTOKEN integrity check (EINITTOKEN contains MAC info that calculated 
by LE, and EINIT needs LE's IA32_SGXLEPUBKEYHASHn to derive the key to 
verify MAC).


SGX in VM doesn't change those behaviors, so in VM, the enclaves can 
also be signed by anyone, but Xen needs to emulate IA32_SGXLEPUBKEYHASHn 
so that when one VM is running, the correct IA32_SGXLEPUBKEYHASHn are 
already in physical MSRs.





2.1.3 Notify domain's virtual EPC base and size to Xen

Xen needs to know guest's EPC base and size in order to populate EPC 
pages for

it. Toolstack notifies EPC base and size to Xen via XEN_DOMCTL_set_cpuid.


I am currently in the process of reworking the Xen/Toolstack interface 
when it comes to CPUID handling.  The latest design is available here:

Re: [Xen-devel] [PATCH 15/15] xen: tools: expose EPC in ACPI table

2017-07-13 Thread Huang, Kai 



On 7/12/2017 11:05 PM, Andrew Cooper wrote:

On 09/07/17 10:16, Kai Huang wrote:
On physical machine EPC is exposed in ACPI table via "INT0E0C". 
Although EPC
can be discovered by CPUID but Windows driver requires EPC to be 
exposed in

ACPI table as well. This patch exposes EPC in ACPI table.


:(


diff --git a/tools/libacpi/dsdt.asl b/tools/libacpi/dsdt.asl
index fa8ff317b2..25ce196028 100644
--- a/tools/libacpi/dsdt.asl
+++ b/tools/libacpi/dsdt.asl
@@ -441,6 +441,55 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "Xen", 
"HVM", 0)

  }
  }
  }
+
+Device (EPC)


Would it not be better to put this into an SSDT, and only expose it to 
the guest if SGX is advertised?


You mean to create dedicated ssdt_epc.asl? I thought about this, but I 
am not quite sure if we can, because new EPC device will need to refer 
\_SB.EMIN, and \_SB.ELEN, which are in acpi_info, to get EPC base and 
size. Can we refer acpi_info in dedicated ssdt_epc.asl?


Thanks,
-Kai



~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

Re: [Xen-devel] [PATCH 05/15] xen: p2m: new 'p2m_epc' type for EPC mapping

2017-07-12 Thread Huang, Kai 



On 7/13/2017 12:21 AM, George Dunlap wrote:



On Jul 12, 2017, at 1:01 PM, Andrew Cooper  wrote:

On 09/07/17 10:12, Kai Huang wrote:

A new 'p2m_epc' type is added for EPC mapping type. Two wrapper functions
set_epc_p2m_entry and clear_epc_p2m_entry are also added for further use.


Other groups in Intel have been looking to reduce the number of p2m types we 
have, so we can use more hardware defined bits in the EPT pagetable entries.

If we need a new type then we will certainly add one, but it is not clear why 
this type is needed.


Does the hypervisor need to know which pages of a domain’s p2m 1) have valid 
config set up, but 2) aren’t accessible to itself or any other domain?


Hi Andrew, George,

Actually I haven't thought this thoroughly, but my first glance is 
there's no existing p2m_type that can be reasonably used for EPC. 
Probably p2m_ram_rw or p2m_mmio_direct are two potential candidates. For 
EPC, for *static partitioning* Xen hypervisor just needs to setup 
mappings and then leave it until guest is destroyed. But for p2m_ram_rw 
and p2m_mmio_direct there are additional logic when Xen learns about the 
two types. To me adding 'p2m_epc' is more straightforward and safe. 
Maybe we can change to a more generic name such as 'p2m_ram_encrypted'? 
But again I am not sure other encryption technology can also be applied 
to EPC.




  -George
___
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

Re: [Xen-devel] [PATCH 08/15] xen: x86: add SGX cpuid handling support.

2017-07-12 Thread Huang, Kai 



On 7/12/2017 10:56 PM, Andrew Cooper wrote:

On 09/07/17 10:10, Kai Huang wrote:

This patch adds SGX to cpuid handling support. In init_guest_cpuid, for
raw_policy and host_policy, physical EPC info is reported, but for 
pv_max_policy
and hvm_max_policy EPC is hidden, as for particular domain, it's EPC 
base and
size are from tookstack, and it is meaningless to contain physical EPC 
info in
them. Before domain's EPC base and size are properly configured, 
guest's SGX
cpuid should report invalid EPC, which is also consistent with HW 
behavior.


Currently all EPC pages are fully populated for domain when it is 
created.
Xen gets domain's EPC base and size from toolstack via 
XEN_DOMCTL_set_cpuid,

so domain's EPC pages are also populated in XEN_DOMCTL_set_cpuid, after
receiving valid EPC base and size. Failure to populate EPC (such as 
there's

no enough free EPC pages) results in domain creation failure by making
XEN_DOMCTL_set_cpuid return error.

Signed-off-by: Kai Huang 
---
  xen/arch/x86/cpuid.c| 87 
-

  xen/arch/x86/domctl.c   | 47 +++-
  xen/include/asm-x86/cpuid.h | 26 +-
  3 files changed, 157 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index d359e090f3..db896be2e8 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -9,6 +9,7 @@
  #include 
  #include 
  #include 
+#include 
  const uint32_t known_features[] = INIT_KNOWN_FEATURES;
  const uint32_t special_features[] = INIT_SPECIAL_FEATURES;
@@ -158,6 +159,44 @@ static void recalculate_xstate(struct 
cpuid_policy *p)

  }
  }
+static void recalculate_sgx(struct cpuid_policy *p, bool_t hide_epc)


Across the entire series, please use bool rather than bool_t.

Hi Andrew,

Thank you very much for comments.

Will do.



Why do we need this hide_epc parameter?  If we aren't providing any epc 
resource to the guest, the entire sgx union should be zero and the SGX 
feature bit should be hidden.


My intention was to hide physical EPC info for pv_max_policy and 
hvm_max_policy (recalculate_sgx is also called by 
calculate_pv_max_policy and calculate_hvm_max_policy), as they are for 
guest and don't need physical EPC info. But keeping physical EPC info in 
them does no harm so I think we can simply remove hide_epc.


IMO we cannot check whether EPC is valid and zero sgx union in 
recalculate_sgx, as it is called for each CPUID. For example, it is 
called for SGX subleaf 0, and 1, and then 2, and when subleaf 0 and 1 
are called, the EPC resource is 0 (hasn't been configured).





+{
+if ( !p->feat.sgx )
+{
+memset(>sgx, 0, sizeof (p->sgx));
+return;
+}
+
+if ( !p->sgx.sgx1 )
+{
+memset(>sgx, 0, sizeof (p->sgx));
+return;
+}


These two clauses can be combined.


Will do.




+
+/*
+ * SDM 42.7.2.1 SECS.ATTRIBUTE.XFRM:
+ *
+ * Legal value for SECS.ATTRIBUTE.XFRM conform to these 
requirements:

+ *  - XFRM[1:0] must be set to 0x3;
+ *  - If processor does not support XSAVE, or if the system 
software has not

+ *enabled XSAVE, then XFRM[63:2] must be 0.
+ *  - If the processor does support XSAVE, XFRM must contain a 
value that

+ *would be legal if loaded into XCR0.
+ */
+p->sgx.xfrm_low = 0x3;
+p->sgx.xfrm_high = 0;
+if ( p->basic.xsave )
+{
+p->sgx.xfrm_low |= p->xstate.xcr0_low;
+p->sgx.xfrm_high |= p->xstate.xcr0_high;
+}


There is a bug here, but it will disappear with my CPUID work.  At the 
moment, the job of this function is to sanitise values handed by the 
toolstack, which includes zeroing all the reserved bits.  This is 
because there is currently no way to signal a failure.


When I fix the toolstack interface, the toolstack will propose a new 
CPUID policy, and Xen will have a function to check it against the 
architectural requirements.  At that point, we will be applying checks, 
but not modifying the contents.


I think I need to look at your design first and then I should be able to 
understand your comment. :)





+
+if ( hide_epc )
+{
+memset(>sgx.raw[0x2], 0, sizeof (struct cpuid_leaf));
+}
+}
+
  /*
   * Misc adjustments to the policy.  Mostly clobbering reserved 
fields and
   * duplicating shared fields.  Intentionally hidden fields are 
annotated.

@@ -239,7 +278,7 @@ static void __init calculate_raw_policy(void)
  {
  switch ( i )
  {
-case 0x4: case 0x7: case 0xd:
+case 0x4: case 0x7: case 0xd: case 0x12:
  /* Multi-invocation leaves.  Deferred. */
  continue;
  }
@@ -299,6 +338,19 @@ static void __init calculate_raw_policy(void)
  }
  }
+if ( p->basic.max_leaf >= SGX_CPUID )
+{
+/*
+ * For raw policy we just report native CPUID. For EPC on 
native it's
+ * possible that we will have

Re: [Xen-devel] [PATCH 04/15] xen: mm: add ioremap_cache

2017-07-12 Thread Huang, Kai 



On 7/12/2017 7:13 PM, Julien Grall wrote:



On 07/12/2017 02:52 AM, Huang, Kai wrote:

Hi Julien,


Hello Kai,

Please avoid top-posting.


Sorry. Will avoid in the future :)




Thanks for pointing out. I'll move to x86 specific.

I've cc-ed all maintainers reported by ./scripts/get_maintainer.pl, 
looks this script doesn't report all maintainers. Sorry. I'll add ARM 
maintainers next time. 


I would always double check the result of scripts/get_maintainer.pl. I 
am aware of a bug in scripts/get_maintainers.pl where only maintainer of 
the specific component (here x86) are listed, even when you touch common 
code.


In this case, I didn't ask to CC ARM maintainers, but CC "THE REST" 
group (see MAINTAINERS).


Understood. I'll follow in the future.

Thanks,
-Kai


Cheers,



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

Re: [Xen-devel] [PATCH 04/15] xen: mm: add ioremap_cache

2017-07-12 Thread Huang, Kai 



On 7/12/2017 6:17 PM, Jan Beulich wrote:

Julien Grall  07/11/17 10:15 PM >>>

On 07/09/2017 09:10 AM, Kai Huang wrote:

Currently Xen only has non-cacheable version of ioremap. Although EPC is
reported as reserved memory in e820 but it can be mapped as cacheable. This
patch adds ioremap_cache (cacheable version of ioremap).

Signed-off-by: Kai Huang 
---
   xen/arch/x86/mm.c  | 15 +--
   xen/include/xen/vmap.h |  1 +


First of all, this is common code and the "REST" maintainers should have
been CCed for this include.

But xen/include/xen/vmap.h is common code and going to break ARM. We
already have an inline implementation of ioremap_nocache. You should
move the definition in x86 specific headers.


Indeed, plus the ARM implementation actually shows how this would better
be done: Have a function allowing more than just true/false to be passed in,
to eventually also allow having ioremap_wc() and alike as wrappers. As long
as it's x86-specific I'd then also suggest calling the new wrapper function
ioremap_wb() (as "cache" may also mean WT).


Hi Jan,

Thanks for comments. I'll do as you suggested.

Thanks,
-Kai


Jan


___
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

Re: [Xen-devel] [PATCH 02/15] xen: vmx: detect ENCLS VMEXIT

2017-07-12 Thread Huang, Kai 



On 7/13/2017 6:54 AM, Jan Beulich wrote:

Andrew Cooper  07/12/17 1:12 PM >>>

On 09/07/17 10:09, Kai Huang wrote:

If ENCLS VMEXIT is not present then we cannot support SGX virtualization.
This patch detects presence of ENCLS VMEXIT. A Xen boot boolean parameter
'sgx' is also added to manually enable/disable SGX.

Signed-off-by: Kai Huang 


At a minimum, you also need to modify calculate_hvm_max_policy() to hide
SGX if we don't have ENCLS intercept support.


Actually IMO this is not needed, as I added an __initcall(sgx_init) (see 
patch 0003), where I will call setup_clear_cpu_cap(X86_FEATURE_SGX) if 
for any reason boot_sgx_cpuidata (which contains common SGX cpuid info 
shared by all cores) doesn't have valid SGX info. if ENCLS VMEXIT is not 
present, then detect_sgx won't be called for any core so that 
X86_FEATURE_SGX will be cleared in boot_cpu_data. As init_guest_cpuid is 
called after all __initcalls are called, so if ENCLS VMEXIT is not 
present, or sgx is disabled via boot parameter, then even for 
host_policy, it won't have SGX.


Of course if we changed the implementation of __initcall(sgx_init), we 
probably need to explicitly clear SGX here. Anyway clearing SGX here 
doesn't have any harm, so I am completely fine to do it if you think it 
is necessary.




Additionally I think the command line option should default to off initially
and it needs an entry in the command line option doc.


Sure. I'll change default to 0 and change the doc as well.

Thanks,
-Kai


Jan


___
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

Re: [Xen-devel] [PATCH 04/15] xen: mm: add ioremap_cache

2017-07-11 Thread Huang, Kai 

Hi Julien,

Thanks for pointing out. I'll move to x86 specific.

I've cc-ed all maintainers reported by ./scripts/get_maintainer.pl, 
looks this script doesn't report all maintainers. Sorry. I'll add ARM 
maintainers next time.


Thanks,
-Kai

On 7/12/2017 8:14 AM, Julien Grall wrote:

Hi,

On 07/09/2017 09:10 AM, Kai Huang wrote:

Currently Xen only has non-cacheable version of ioremap. Although EPC is
reported as reserved memory in e820 but it can be mapped as cacheable. 
This

patch adds ioremap_cache (cacheable version of ioremap).

Signed-off-by: Kai Huang 
---
  xen/arch/x86/mm.c  | 15 +--
  xen/include/xen/vmap.h |  1 +


First of all, this is common code and the "REST" maintainers should have 
been CCed for this include.


But xen/include/xen/vmap.h is common code and going to break ARM. We 
already have an inline implementation of ioremap_nocache. You should 
move the definition in x86 specific headers.


Please make sure to at least build test ARM when touching common code.

Cheers,


  2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 101ab33193..d0b6b3a247 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -6284,9 +6284,10 @@ void *__init arch_vmap_virt_end(void)
  return (void *)fix_to_virt(__end_of_fixed_addresses);
  }
-void __iomem *ioremap(paddr_t pa, size_t len)
+static void __iomem *__ioremap(paddr_t pa, size_t len, bool_t cache)
  {
  mfn_t mfn = _mfn(PFN_DOWN(pa));
+unsigned int flags = cache ? PAGE_HYPERVISOR : 
PAGE_HYPERVISOR_NOCACHE;

  void *va;
  WARN_ON(page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL));
@@ -6299,12 +6300,22 @@ void __iomem *ioremap(paddr_t pa, size_t len)
  unsigned int offs = pa & (PAGE_SIZE - 1);
  unsigned int nr = PFN_UP(offs + len);
-va = __vmap(, nr, 1, 1, PAGE_HYPERVISOR_NOCACHE, 
VMAP_DEFAULT) + offs;

+va = __vmap(, nr, 1, 1, flags, VMAP_DEFAULT) + offs;
  }
  return (void __force __iomem *)va;
  }
+void __iomem *ioremap(paddr_t pa, size_t len)
+{
+return __ioremap(pa, len, false);
+}
+
+void __iomem *ioremap_cache(paddr_t pa, size_t len)
+{
+return __ioremap(pa, len, true);
+}
+
  int create_perdomain_mapping(struct domain *d, unsigned long va,
   unsigned int nr, l1_pgentry_t **pl1tab,
   struct page_info **ppg)
diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index 369560e620..f6037e368c 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -24,6 +24,7 @@ void *vzalloc(size_t size);
  void vfree(void *va);
  void __iomem *ioremap(paddr_t, size_t);
+void __iomem *ioremap_cache(paddr_t, size_t);
  static inline void iounmap(void __iomem *va)
  {





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

Re: [Xen-devel] [PATCH v2] xen: x86: remove duplicated IA32_FEATURE_CONTROL MSR macro

2016-06-28 Thread Huang, Kai 



On 6/28/2016 8:37 PM, Jan Beulich wrote:

On 28.06.16 at 10:12,  wrote:

From: Kai Huang 


On the 24th I had asked you privately to please follow Xen patch
submission rules: Patches get sent _to_ the list, and maintainers
get _cc_-ed. People receiving mails may have rules in place in their
mail systems to pre-sort incoming traffic accordingly.


Oh sorry. I checked my mailbox but looks I couldn't find your email. 
Maybe something wrong happened. Will follow this rule in the future. 
Thanks for reminding.


Thanks,
-Kai


Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel



___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] xen: x86: remove duplicated IA32_FEATURE_CONTROL MSR macro

2016-06-24 Thread Huang, Kai 

Hi Kevin, Jan,

Thanks for comments.

On 6/24/2016 11:31 PM, Jan Beulich wrote:

On 24.06.16 at 12:56,  wrote:

 From: kaih.li...@gmail.com [mailto:kaih.li...@gmail.com]
Sent: Friday, June 24, 2016 6:45 PM
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -133,12 +133,13 @@
 #define MSR_IA32_VMX_TRUE_EXIT_CTLS 0x48f
 #define MSR_IA32_VMX_TRUE_ENTRY_CTLS0x490
 #define MSR_IA32_VMX_VMFUNC 0x491
-#define IA32_FEATURE_CONTROL_MSR0x3a
+#define MSR_IA32_FEATURE_CONTROL0x3a


Instead of moving MSR definition up here, better move all related lines
down since original place is more sorted regarding to 0x3a.


I agree.


Sure. I'll move this macro down, along with the bit macros.




 #define IA32_FEATURE_CONTROL_MSR_LOCK 0x0001
 #define IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_INSIDE_SMX  0x0002
 #define IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON_OUTSIDE_SMX 0x0004
 #define IA32_FEATURE_CONTROL_MSR_SENTER_PARAM_CTL 0x7f00
 #define IA32_FEATURE_CONTROL_MSR_ENABLE_SENTER0x8000
+#define IA32_FEATURE_CONTROL_MSR_SGX_ENABLE   0x4


suppose above macros better be changed in same style? Or is it
really meaningful to keep whole MSR name in every bit definition?
Is it clearly enough to just keep strings after _MSR_?


I partly agree. The _MSR_ infix is clearly pointless. I wouldn't,
however, like to see the IA32_FEATURE_CONTROL_ prefix
dropped, as it helps associating the bits with their MSR.


Sure. I think we can have consensus on just removing the _MSR_ infix, so 
the bit macros will be like IA32_FEATURE_CONTROL_LOCK, 
IA32_FEATURE_CONTROL_SGX_ENABLE, etc?


Thanks,
-Kai


Jan




___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] xen: x86: remove duplicated MSR_IA32_FEATURE_CONTROL definition

2016-06-24 Thread Huang, Kai 



On 6/22/2016 11:44 PM, Jan Beulich wrote:

On 22.06.16 at 12:17,  wrote:

@@ -288,7 +289,6 @@
 #define MSR_IA32_PLATFORM_ID   0x0017
 #define MSR_IA32_EBL_CR_POWERON0x002a
 #define MSR_IA32_EBC_FREQUENCY_ID  0x002c
-#define MSR_IA32_FEATURE_CONTROL   0x003a
 #define MSR_IA32_TSC_ADJUST0x003b


The latest when removing the definition here you should have noticed
that this variant follows the conventional naming, so if you want to
consolidate things, it should be the other way around imo. While not
the main reason, it'll also make sure mwait-idle.c won't needlessly
diverge from its Linux original.


You are right. I'll sent out another patch removing the old one.

Thanks,
-Kai


Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel



___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel