diff mbox

[09/15] xen: vmx: handle SGX related MSRs

Message ID 0d4f023a709e7ca089821267e517fb1157437ab6.1499586046.git.kai.huang@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kai Huang July 9, 2017, 8:09 a.m. UTC
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.huang@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(+)

Comments

Andrew Cooper July 19, 2017, 5:27 p.m. UTC | #1
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.huang@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.

> +
> +/* 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] = SGX_INTEL_DEFAULT_LEPUBKEYHASH3;
> +
> +        sgxv->readable = 1;
> +        sgxv->writable = domain_has_sgx_launch_control(v->domain);
> +    }
> +    else
> +    {
> +        uint64_t v;
> +        /*
> +         * Although SDM says if SGX is present, then IA32_SGXLEPUBKEYHASHn are
> +         * available for read, but in reality for SKYLAKE client machines,
> +         * those MSRs are not available if SGX is present, so we cannot rely on
> +         * cpu_has_sgx to determine whether to we are able to read MSRs,
> +         * instead, we always use rdmsr_safe.

Talking with Jun at XenSummit, I got the impression that the
availability of these has MSRs is based on SGX_LC, not SGX.

Furthermore, that is my reading of 41.2.2 "Intel SGX Launch Control
Configuration", although the logic is expressed in terms of checking SGX
before SGX_LC.

> +         */
> +        sgxv->readable = rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH0, v) ? 0 : 1;
> +
> +        if ( !sgxv->readable )
> +            return;
> +
> +        rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH0, sgxv->ia32_sgxlepubkeyhash[0]);
> +        rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH1, sgxv->ia32_sgxlepubkeyhash[1]);
> +        rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH2, sgxv->ia32_sgxlepubkeyhash[2]);
> +        rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH3, sgxv->ia32_sgxlepubkeyhash[3]);
> +    }
> +}
> +
> +void sgx_ctxt_switch_to(struct vcpu *v)
> +{
> +    struct sgx_vcpu *sgxv = to_sgx_vcpu(v);
> +
> +    if ( sgxv->writable && sgx_ia32_sgxlepubkeyhash_writable() )

This causes a read of FEATURE_CONTROL on every context switch path,
which is inefficient.

Just like with CPUID policy, we will (eventually) have a generic MSR
policy for the guest to use.  In particular, I can forsee a usecase
where hardware has LC unlocked, but the host administrator wishes LC to
be locked from the guests point of view.

> +    {
> +        wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0, sgxv->ia32_sgxlepubkeyhash[0]);
> +        wrmsrl(MSR_IA32_SGXLEPUBKEYHASH1, sgxv->ia32_sgxlepubkeyhash[1]);
> +        wrmsrl(MSR_IA32_SGXLEPUBKEYHASH2, sgxv->ia32_sgxlepubkeyhash[2]);
> +        wrmsrl(MSR_IA32_SGXLEPUBKEYHASH3, sgxv->ia32_sgxlepubkeyhash[3]);
> +    }
> +}
> +
> +int sgx_msr_read_intercept(struct vcpu *v, unsigned int msr, u64 *msr_content)
> +{
> +    struct sgx_vcpu *sgxv = to_sgx_vcpu(v);
> +    u64 data;
> +    int r = 1;
> +
> +    if ( !domain_has_sgx(v->domain) )
> +        return 0;
> +
> +    switch ( msr )
> +    {
> +    case MSR_IA32_FEATURE_CONTROL:
> +        data = (IA32_FEATURE_CONTROL_LOCK |
> +                IA32_FEATURE_CONTROL_SGX_ENABLE);
> +        /*
> +         * If physical IA32_SGXLEPUBKEYHASHn are writable, then we always
> +         * allow guest to be able to change IA32_SGXLEPUBKEYHASHn at runtime.
> +         */
> +        if ( sgx_ia32_sgxlepubkeyhash_writable() &&
> +                domain_has_sgx_launch_control(v->domain) )
> +            data |= IA32_FEATURE_CONTROL_SGX_LAUNCH_CONTROL_ENABLE;
> +
> +        *msr_content = data;
> +
> +        break;

Newline here please.

> +    case MSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:

Spaces around ... please.  (it is only because of the #defines that this
isn't a syntax error).

> +        /*
> +         * SDM 35.1 Model-Specific Registers, table 35-2.
> +         *
> +         * IA32_SGXLEPUBKEYHASH[0..3]:
> +         *
> +         * Read permitted if CPUID.0x12.0:EAX[0] = 1.
> +         *
> +         * In reality, MSRs may not be readable even SGX is present, in which
> +         * case guest is not allowed to read either.
> +         */
> +        if ( !sgxv->readable )
> +        {
> +            r = 0;
> +            break;
> +        }
> +
> +        data = sgxv->ia32_sgxlepubkeyhash[msr - MSR_IA32_SGXLEPUBKEYHASH0];
> +
> +        *msr_content = data;
> +
> +        break;
> +    default:
> +        r = 0;
> +        break;
> +    }
> +
> +    return r;
> +}
> +
> +int sgx_msr_write_intercept(struct vcpu *v, unsigned int msr, u64 msr_content)
> +{
> +    struct sgx_vcpu *sgxv = to_sgx_vcpu(v);
> +    int r = 1;
> +
> +    if ( !domain_has_sgx(v->domain) )
> +        return 0;
> +
> +    switch ( msr )
> +    {
> +    case MSR_IA32_FEATURE_CONTROL:
> +        /* sliently drop */

Silently dropping is not ok.  This change needs rebasing over c/s
46c3acb308 where I have fixed up the writeability of FEATURE_CONTROL.

> +        break;
> +    case MSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:
> +        /*
> +         * SDM 35.1 Model-Specific Registers, table 35-2.
> +         *
> +         * IA32_SGXLEPUBKEYHASH[0..3]:
> +         *
> +         * - If CPUID.0x7.0:ECX[30] = 1, FEATURE_CONTROL[17] is available.
> +         * - Write permitted if CPUID.0x12.0:EAX[0] = 1 &&
> +         *      FEATURE_CONTROL[17] = 1 && FEATURE_CONTROL[0] = 1.
> +         *
> +         * sgxv->writable == 1 means sgx_ia32_sgxlepubkeyhash_writable() and
> +         * domain_has_sgx_launch_control(d) both are true.
> +         */
> +        if ( !sgxv->writable )
> +        {
> +            r = 0;
> +            break;
> +        }
> +
> +        sgxv->ia32_sgxlepubkeyhash[msr - MSR_IA32_SGXLEPUBKEYHASH0] =
> +            msr_content;
> +
> +        break;
> +    default:
> +        r = 0;
> +        break;
> +    }
> +
> +    return r;
> +}
> +
>  static bool_t sgx_enabled_in_bios(void)
>  {
>      uint64_t val, sgx_enabled = IA32_FEATURE_CONTROL_SGX_ENABLE |
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 243643111d..7ee5515bdc 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -470,6 +470,8 @@ static int vmx_vcpu_initialise(struct vcpu *v)
>      if ( v->vcpu_id == 0 )
>          v->arch.user_regs.rax = 1;
>  
> +    sgx_vcpu_init(v);
> +
>      return 0;
>  }
>  
> @@ -1048,6 +1050,9 @@ static void vmx_ctxt_switch_to(struct vcpu *v)
>  
>      if ( v->domain->arch.hvm_domain.pi_ops.switch_to )
>          v->domain->arch.hvm_domain.pi_ops.switch_to(v);
> +
> +    if ( domain_has_sgx(v->domain) )
> +        sgx_ctxt_switch_to(v);
>  }
>  
>  
> @@ -2876,10 +2881,20 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>          __vmread(GUEST_IA32_DEBUGCTL, msr_content);
>          break;
>      case MSR_IA32_FEATURE_CONTROL:
> +        /* If neither SGX nor nested is supported, this MSR should not be
> +         * touched */
> +        if ( !sgx_msr_read_intercept(current, msr, msr_content) &&
> +                !nvmx_msr_read_intercept(msr, msr_content) )
> +            goto gp_fault;

Unfortunately, this logic is broken.  In the case that both SMX and VMX
are configured, the VMX handler will clobber the values set up by the
SGX handler.  Sergey has a VMX-policy series (v1 posted, v2 in the
works) to start addressing some of the issues on the VMX side, but
fundamentally, all reads like this need serving out of a single policy,
rather than having different subsystems fighting for control of the
values.  (The Xen MSR code is terrible for this at the moment.)

> +        break;
>      case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_VMFUNC:
>          if ( !nvmx_msr_read_intercept(msr, msr_content) )
>              goto gp_fault;
>          break;
> +    case MSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:
> +        if ( !sgx_msr_read_intercept(current, msr, msr_content) )
> +            goto gp_fault;
> +        break;
>      case MSR_IA32_MISC_ENABLE:
>          rdmsrl(MSR_IA32_MISC_ENABLE, *msr_content);
>          /* Debug Trace Store is not supported. */
> @@ -3119,10 +3134,19 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>          break;
>      }
>      case MSR_IA32_FEATURE_CONTROL:
> +        /* See vmx_msr_read_intercept */
> +        if ( !sgx_msr_write_intercept(current, msr, msr_content) &&
> +                !nvmx_msr_write_intercept(msr, msr_content) )

Definitely needs a rebase.  nvmx_msr_write_intercept() has been removed.

~Andrew

> +            goto gp_fault;
> +        break;
>      case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_TRUE_ENTRY_CTLS:
>          if ( !nvmx_msr_write_intercept(msr, msr_content) )
>              goto gp_fault;
>          break;
> +    case MSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:
> +        if ( !sgx_msr_write_intercept(current, msr, msr_content) )
> +            goto gp_fault;
> +        break;
>      case MSR_P6_PERFCTR(0)...MSR_P6_PERFCTR(7):
>      case MSR_P6_EVNTSEL(0)...MSR_P6_EVNTSEL(7):
>      case MSR_CORE_PERF_FIXED_CTR0...MSR_CORE_PERF_FIXED_CTR2:
> diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
> index 9793f8c1c5..dfb17c4bd8 100644
> --- a/xen/include/asm-x86/cpufeature.h
> +++ b/xen/include/asm-x86/cpufeature.h
> @@ -98,6 +98,9 @@
>  #define cpu_has_smap            boot_cpu_has(X86_FEATURE_SMAP)
>  #define cpu_has_sha             boot_cpu_has(X86_FEATURE_SHA)
>  
> +/* CPUID level 0x00000007:0.ecx */
> +#define cpu_has_sgx_launch_control  boot_cpu_has(X86_FEATURE_SGX_LAUNCH_CONTROL)
> +
>  /* CPUID level 0x80000007.edx */
>  #define cpu_has_itsc            boot_cpu_has(X86_FEATURE_ITSC)
>  
> diff --git a/xen/include/asm-x86/hvm/vmx/sgx.h b/xen/include/asm-x86/hvm/vmx/sgx.h
> index 40f860662a..c460f61e5e 100644
> --- a/xen/include/asm-x86/hvm/vmx/sgx.h
> +++ b/xen/include/asm-x86/hvm/vmx/sgx.h
> @@ -75,4 +75,26 @@ int hvm_populate_epc(struct domain *d, unsigned long epc_base_pfn,
>  int hvm_reset_epc(struct domain *d, bool_t free_epc);
>  void hvm_destroy_epc(struct domain *d);
>  
> +/* Per-vcpu SGX structure */
> +struct sgx_vcpu {
> +    uint64_t ia32_sgxlepubkeyhash[4];
> +    /*
> +     * Although SDM says if SGX is present, then IA32_SGXLEPUBKEYHASHn are
> +     * available for read, but in reality for SKYLAKE client machines, those
> +     * those MSRs are not available if SGX is present.
> +     */
> +    bool_t readable;
> +    bool_t writable;
> +};
> +#define to_sgx_vcpu(v)  (&(v->arch.hvm_vmx.sgx))
> +
> +bool_t sgx_ia32_sgxlepubkeyhash_writable(void);
> +bool_t domain_has_sgx(struct domain *d);
> +bool_t domain_has_sgx_launch_control(struct domain *d);
> +
> +void sgx_vcpu_init(struct vcpu *v);
> +void sgx_ctxt_switch_to(struct vcpu *v);
> +int sgx_msr_read_intercept(struct vcpu *v, unsigned int msr, u64 *msr_content);
> +int sgx_msr_write_intercept(struct vcpu *v, unsigned int msr, u64 msr_content);
> +
>  #endif  /* __ASM_X86_HVM_VMX_SGX_H__ */
> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
> index 6cfa5c3310..fc0b9d85fd 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -160,6 +160,8 @@ struct arch_vmx_struct {
>       * pCPU and wakeup the related vCPU.
>       */
>      struct pi_blocking_vcpu pi_blocking;
> +
> +    struct sgx_vcpu sgx;
>  };
>  
>  int vmx_create_vmcs(struct vcpu *v);
> diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
> index 771e7500af..16206a11b7 100644
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -296,6 +296,12 @@
>  #define IA32_FEATURE_CONTROL_SENTER_PARAM_CTL         0x7f00
>  #define IA32_FEATURE_CONTROL_ENABLE_SENTER            0x8000
>  #define IA32_FEATURE_CONTROL_SGX_ENABLE               0x40000
> +#define IA32_FEATURE_CONTROL_SGX_LAUNCH_CONTROL_ENABLE  0x20000
> +
> +#define MSR_IA32_SGXLEPUBKEYHASH0   0x0000008c
> +#define MSR_IA32_SGXLEPUBKEYHASH1   0x0000008d
> +#define MSR_IA32_SGXLEPUBKEYHASH2   0x0000008e
> +#define MSR_IA32_SGXLEPUBKEYHASH3   0x0000008f
>  
>  #define MSR_IA32_TSC_ADJUST		0x0000003b
>
Kai Huang July 21, 2017, 9:42 a.m. UTC | #2
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.huang@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;
>> +        sgxv->ia32_sgxlepubkeyhash[2] = SGX_INTEL_DEFAULT_LEPUBKEYHASH2;
>> +        sgxv->ia32_sgxlepubkeyhash[3] = SGX_INTEL_DEFAULT_LEPUBKEYHASH3;
>> +
>> +        sgxv->readable = 1;
>> +        sgxv->writable = domain_has_sgx_launch_control(v->domain);
>> +    }
>> +    else
>> +    {
>> +        uint64_t v;
>> +        /*
>> +         * Although SDM says if SGX is present, then IA32_SGXLEPUBKEYHASHn are
>> +         * available for read, but in reality for SKYLAKE client machines,
>> +         * those MSRs are not available if SGX is present, so we cannot rely on
>> +         * cpu_has_sgx to determine whether to we are able to read MSRs,
>> +         * instead, we always use rdmsr_safe.
> 
> Talking with Jun at XenSummit, I got the impression that the
> availability of these has MSRs is based on SGX_LC, not SGX.
> 
> Furthermore, that is my reading of 41.2.2 "Intel SGX Launch Control
> Configuration", although the logic is expressed in terms of checking SGX
> before SGX_LC.

Yes you are correct indeed. When I was writing the code I was reading 
the old SDM, which has bug and doesn't mention SGX_LC in CPUID as a 
condition. Please see my reply to your question whether this is erratum.

We should add cpu_has_sgx_lc as additional check, and I think we can use 
rdmsr if both SGX and SGX_LC are present (probably using rdmsr_safe is 
still better?).

> 
>> +         */
>> +        sgxv->readable = rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH0, v) ? 0 : 1;
>> +
>> +        if ( !sgxv->readable )
>> +            return;
>> +
>> +        rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH0, sgxv->ia32_sgxlepubkeyhash[0]);
>> +        rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH1, sgxv->ia32_sgxlepubkeyhash[1]);
>> +        rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH2, sgxv->ia32_sgxlepubkeyhash[2]);
>> +        rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH3, sgxv->ia32_sgxlepubkeyhash[3]);
>> +    }
>> +}
>> +
>> +void sgx_ctxt_switch_to(struct vcpu *v)
>> +{
>> +    struct sgx_vcpu *sgxv = to_sgx_vcpu(v);
>> +
>> +    if ( sgxv->writable && sgx_ia32_sgxlepubkeyhash_writable() )
> 
> This causes a read of FEATURE_CONTROL on every context switch path,
> which is inefficient.
> 
> Just like with CPUID policy, we will (eventually) have a generic MSR
> policy for the guest to use.  In particular, I can forsee a usecase
> where hardware has LC unlocked, but the host administrator wishes LC to
> be locked from the guests point of view.

We can remove sgx_ia32_sgxlepubkeyhash_writable, as if sgxv->writable is 
true, then sgx_ia32_sgxlepubkeyhash_writable is always true. I am not 
sure whether we should leave guest in locked mode in most cases but I 
think we can add 'lewr' XL parameter to explicitly set guest to unlocked 
mode (otherwise guest is locked). Please see my latest reply to design 
of Launch Control.

> 
>> +    {
>> +        wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0, sgxv->ia32_sgxlepubkeyhash[0]);
>> +        wrmsrl(MSR_IA32_SGXLEPUBKEYHASH1, sgxv->ia32_sgxlepubkeyhash[1]);
>> +        wrmsrl(MSR_IA32_SGXLEPUBKEYHASH2, sgxv->ia32_sgxlepubkeyhash[2]);
>> +        wrmsrl(MSR_IA32_SGXLEPUBKEYHASH3, sgxv->ia32_sgxlepubkeyhash[3]);
>> +    }
>> +}
>> +
>> +int sgx_msr_read_intercept(struct vcpu *v, unsigned int msr, u64 *msr_content)
>> +{
>> +    struct sgx_vcpu *sgxv = to_sgx_vcpu(v);
>> +    u64 data;
>> +    int r = 1;
>> +
>> +    if ( !domain_has_sgx(v->domain) )
>> +        return 0;
>> +
>> +    switch ( msr )
>> +    {
>> +    case MSR_IA32_FEATURE_CONTROL:
>> +        data = (IA32_FEATURE_CONTROL_LOCK |
>> +                IA32_FEATURE_CONTROL_SGX_ENABLE);
>> +        /*
>> +         * If physical IA32_SGXLEPUBKEYHASHn are writable, then we always
>> +         * allow guest to be able to change IA32_SGXLEPUBKEYHASHn at runtime.
>> +         */
>> +        if ( sgx_ia32_sgxlepubkeyhash_writable() &&
>> +                domain_has_sgx_launch_control(v->domain) )
>> +            data |= IA32_FEATURE_CONTROL_SGX_LAUNCH_CONTROL_ENABLE;
>> +
>> +        *msr_content = data;
>> +
>> +        break;
> 
> Newline here please.

Sure.

> 
>> +    case MSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:
> 
> Spaces around ... please.  (it is only because of the #defines that this
> isn't a syntax error).

Will do.

> 
>> +        /*
>> +         * SDM 35.1 Model-Specific Registers, table 35-2.
>> +         *
>> +         * IA32_SGXLEPUBKEYHASH[0..3]:
>> +         *
>> +         * Read permitted if CPUID.0x12.0:EAX[0] = 1.
>> +         *
>> +         * In reality, MSRs may not be readable even SGX is present, in which
>> +         * case guest is not allowed to read either.
>> +         */
>> +        if ( !sgxv->readable )
>> +        {
>> +            r = 0;
>> +            break;
>> +        }
>> +
>> +        data = sgxv->ia32_sgxlepubkeyhash[msr - MSR_IA32_SGXLEPUBKEYHASH0];
>> +
>> +        *msr_content = data;
>> +
>> +        break;
>> +    default:
>> +        r = 0;
>> +        break;
>> +    }
>> +
>> +    return r;
>> +}
>> +
>> +int sgx_msr_write_intercept(struct vcpu *v, unsigned int msr, u64 msr_content)
>> +{
>> +    struct sgx_vcpu *sgxv = to_sgx_vcpu(v);
>> +    int r = 1;
>> +
>> +    if ( !domain_has_sgx(v->domain) )
>> +        return 0;
>> +
>> +    switch ( msr )
>> +    {
>> +    case MSR_IA32_FEATURE_CONTROL:
>> +        /* sliently drop */
> 
> Silently dropping is not ok.  This change needs rebasing over c/s
> 46c3acb308 where I have fixed up the writeability of FEATURE_CONTROL.

Thanks. I'll take a look and change accordingly.

> 
>> +        break;
>> +    case MSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:
>> +        /*
>> +         * SDM 35.1 Model-Specific Registers, table 35-2.
>> +         *
>> +         * IA32_SGXLEPUBKEYHASH[0..3]:
>> +         *
>> +         * - If CPUID.0x7.0:ECX[30] = 1, FEATURE_CONTROL[17] is available.
>> +         * - Write permitted if CPUID.0x12.0:EAX[0] = 1 &&
>> +         *      FEATURE_CONTROL[17] = 1 && FEATURE_CONTROL[0] = 1.
>> +         *
>> +         * sgxv->writable == 1 means sgx_ia32_sgxlepubkeyhash_writable() and
>> +         * domain_has_sgx_launch_control(d) both are true.
>> +         */
>> +        if ( !sgxv->writable )
>> +        {
>> +            r = 0;
>> +            break;
>> +        }
>> +
>> +        sgxv->ia32_sgxlepubkeyhash[msr - MSR_IA32_SGXLEPUBKEYHASH0] =
>> +            msr_content;
>> +
>> +        break;
>> +    default:
>> +        r = 0;
>> +        break;
>> +    }
>> +
>> +    return r;
>> +}
>> +
>>   static bool_t sgx_enabled_in_bios(void)
>>   {
>>       uint64_t val, sgx_enabled = IA32_FEATURE_CONTROL_SGX_ENABLE |
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index 243643111d..7ee5515bdc 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -470,6 +470,8 @@ static int vmx_vcpu_initialise(struct vcpu *v)
>>       if ( v->vcpu_id == 0 )
>>           v->arch.user_regs.rax = 1;
>>   
>> +    sgx_vcpu_init(v);
>> +
>>       return 0;
>>   }
>>   
>> @@ -1048,6 +1050,9 @@ static void vmx_ctxt_switch_to(struct vcpu *v)
>>   
>>       if ( v->domain->arch.hvm_domain.pi_ops.switch_to )
>>           v->domain->arch.hvm_domain.pi_ops.switch_to(v);
>> +
>> +    if ( domain_has_sgx(v->domain) )
>> +        sgx_ctxt_switch_to(v);
>>   }
>>   
>>   
>> @@ -2876,10 +2881,20 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>>           __vmread(GUEST_IA32_DEBUGCTL, msr_content);
>>           break;
>>       case MSR_IA32_FEATURE_CONTROL:
>> +        /* If neither SGX nor nested is supported, this MSR should not be
>> +         * touched */
>> +        if ( !sgx_msr_read_intercept(current, msr, msr_content) &&
>> +                !nvmx_msr_read_intercept(msr, msr_content) )
>> +            goto gp_fault;
> 
> Unfortunately, this logic is broken.  In the case that both SMX and VMX
> are configured, the VMX handler will clobber the values set up by the
> SGX handler.  Sergey has a VMX-policy series (v1 posted, v2 in the
> works) to start addressing some of the issues on the VMX side, but
> fundamentally, all reads like this need serving out of a single policy,
> rather than having different subsystems fighting for control of the
> values.  (The Xen MSR code is terrible for this at the moment.)

Thanks for pointing out. I have located vmx-policy series. I'll look 
into it to see how should I change this logic.

> 
>> +        break;
>>       case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_VMFUNC:
>>           if ( !nvmx_msr_read_intercept(msr, msr_content) )
>>               goto gp_fault;
>>           break;
>> +    case MSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:
>> +        if ( !sgx_msr_read_intercept(current, msr, msr_content) )
>> +            goto gp_fault;
>> +        break;
>>       case MSR_IA32_MISC_ENABLE:
>>           rdmsrl(MSR_IA32_MISC_ENABLE, *msr_content);
>>           /* Debug Trace Store is not supported. */
>> @@ -3119,10 +3134,19 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>>           break;
>>       }
>>       case MSR_IA32_FEATURE_CONTROL:
>> +        /* See vmx_msr_read_intercept */
>> +        if ( !sgx_msr_write_intercept(current, msr, msr_content) &&
>> +                !nvmx_msr_write_intercept(msr, msr_content) )
> 
> Definitely needs a rebase.  nvmx_msr_write_intercept() has been removed.

Yeah. The code base of this series is about 3-4 weeks ago unfortunately. 
Will do.

Thanks,
-Kai

> 
> ~Andrew
> 
>> +            goto gp_fault;
>> +        break;
>>       case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_TRUE_ENTRY_CTLS:
>>           if ( !nvmx_msr_write_intercept(msr, msr_content) )
>>               goto gp_fault;
>>           break;
>> +    case MSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:
>> +        if ( !sgx_msr_write_intercept(current, msr, msr_content) )
>> +            goto gp_fault;
>> +        break;
>>       case MSR_P6_PERFCTR(0)...MSR_P6_PERFCTR(7):
>>       case MSR_P6_EVNTSEL(0)...MSR_P6_EVNTSEL(7):
>>       case MSR_CORE_PERF_FIXED_CTR0...MSR_CORE_PERF_FIXED_CTR2:
>> diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
>> index 9793f8c1c5..dfb17c4bd8 100644
>> --- a/xen/include/asm-x86/cpufeature.h
>> +++ b/xen/include/asm-x86/cpufeature.h
>> @@ -98,6 +98,9 @@
>>   #define cpu_has_smap            boot_cpu_has(X86_FEATURE_SMAP)
>>   #define cpu_has_sha             boot_cpu_has(X86_FEATURE_SHA)
>>   
>> +/* CPUID level 0x00000007:0.ecx */
>> +#define cpu_has_sgx_launch_control  boot_cpu_has(X86_FEATURE_SGX_LAUNCH_CONTROL)
>> +
>>   /* CPUID level 0x80000007.edx */
>>   #define cpu_has_itsc            boot_cpu_has(X86_FEATURE_ITSC)
>>   
>> diff --git a/xen/include/asm-x86/hvm/vmx/sgx.h b/xen/include/asm-x86/hvm/vmx/sgx.h
>> index 40f860662a..c460f61e5e 100644
>> --- a/xen/include/asm-x86/hvm/vmx/sgx.h
>> +++ b/xen/include/asm-x86/hvm/vmx/sgx.h
>> @@ -75,4 +75,26 @@ int hvm_populate_epc(struct domain *d, unsigned long epc_base_pfn,
>>   int hvm_reset_epc(struct domain *d, bool_t free_epc);
>>   void hvm_destroy_epc(struct domain *d);
>>   
>> +/* Per-vcpu SGX structure */
>> +struct sgx_vcpu {
>> +    uint64_t ia32_sgxlepubkeyhash[4];
>> +    /*
>> +     * Although SDM says if SGX is present, then IA32_SGXLEPUBKEYHASHn are
>> +     * available for read, but in reality for SKYLAKE client machines, those
>> +     * those MSRs are not available if SGX is present.
>> +     */
>> +    bool_t readable;
>> +    bool_t writable;
>> +};
>> +#define to_sgx_vcpu(v)  (&(v->arch.hvm_vmx.sgx))
>> +
>> +bool_t sgx_ia32_sgxlepubkeyhash_writable(void);
>> +bool_t domain_has_sgx(struct domain *d);
>> +bool_t domain_has_sgx_launch_control(struct domain *d);
>> +
>> +void sgx_vcpu_init(struct vcpu *v);
>> +void sgx_ctxt_switch_to(struct vcpu *v);
>> +int sgx_msr_read_intercept(struct vcpu *v, unsigned int msr, u64 *msr_content);
>> +int sgx_msr_write_intercept(struct vcpu *v, unsigned int msr, u64 msr_content);
>> +
>>   #endif  /* __ASM_X86_HVM_VMX_SGX_H__ */
>> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
>> index 6cfa5c3310..fc0b9d85fd 100644
>> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
>> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
>> @@ -160,6 +160,8 @@ struct arch_vmx_struct {
>>        * pCPU and wakeup the related vCPU.
>>        */
>>       struct pi_blocking_vcpu pi_blocking;
>> +
>> +    struct sgx_vcpu sgx;
>>   };
>>   
>>   int vmx_create_vmcs(struct vcpu *v);
>> diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
>> index 771e7500af..16206a11b7 100644
>> --- a/xen/include/asm-x86/msr-index.h
>> +++ b/xen/include/asm-x86/msr-index.h
>> @@ -296,6 +296,12 @@
>>   #define IA32_FEATURE_CONTROL_SENTER_PARAM_CTL         0x7f00
>>   #define IA32_FEATURE_CONTROL_ENABLE_SENTER            0x8000
>>   #define IA32_FEATURE_CONTROL_SGX_ENABLE               0x40000
>> +#define IA32_FEATURE_CONTROL_SGX_LAUNCH_CONTROL_ENABLE  0x20000
>> +
>> +#define MSR_IA32_SGXLEPUBKEYHASH0   0x0000008c
>> +#define MSR_IA32_SGXLEPUBKEYHASH1   0x0000008d
>> +#define MSR_IA32_SGXLEPUBKEYHASH2   0x0000008e
>> +#define MSR_IA32_SGXLEPUBKEYHASH3   0x0000008f
>>   
>>   #define MSR_IA32_TSC_ADJUST		0x0000003b
>>   
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
>
Kai Huang July 22, 2017, 1:37 a.m. UTC | #3
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.huang@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;
>>> +        sgxv->ia32_sgxlepubkeyhash[2] = 
>>> SGX_INTEL_DEFAULT_LEPUBKEYHASH2;
>>> +        sgxv->ia32_sgxlepubkeyhash[3] = 
>>> SGX_INTEL_DEFAULT_LEPUBKEYHASH3;
>>> +
>>> +        sgxv->readable = 1;
>>> +        sgxv->writable = domain_has_sgx_launch_control(v->domain);
>>> +    }
>>> +    else
>>> +    {
>>> +        uint64_t v;
>>> +        /*
>>> +         * Although SDM says if SGX is present, then 
>>> IA32_SGXLEPUBKEYHASHn are
>>> +         * available for read, but in reality for SKYLAKE client 
>>> machines,
>>> +         * those MSRs are not available if SGX is present, so we 
>>> cannot rely on
>>> +         * cpu_has_sgx to determine whether to we are able to read 
>>> MSRs,
>>> +         * instead, we always use rdmsr_safe.
>>
>> Talking with Jun at XenSummit, I got the impression that the
>> availability of these has MSRs is based on SGX_LC, not SGX.
>>
>> Furthermore, that is my reading of 41.2.2 "Intel SGX Launch Control
>> Configuration", although the logic is expressed in terms of checking SGX
>> before SGX_LC.
> 
> Yes you are correct indeed. When I was writing the code I was reading 
> the old SDM, which has bug and doesn't mention SGX_LC in CPUID as a 
> condition. Please see my reply to your question whether this is erratum.
> 
> We should add cpu_has_sgx_lc as additional check, and I think we can use 
> rdmsr if both SGX and SGX_LC are present (probably using rdmsr_safe is 
> still better?).
> 
>>
>>> +         */
>>> +        sgxv->readable = rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH0, v) ? 
>>> 0 : 1;
>>> +
>>> +        if ( !sgxv->readable )
>>> +            return;
>>> +
>>> +        rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH0, 
>>> sgxv->ia32_sgxlepubkeyhash[0]);
>>> +        rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH1, 
>>> sgxv->ia32_sgxlepubkeyhash[1]);
>>> +        rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH2, 
>>> sgxv->ia32_sgxlepubkeyhash[2]);
>>> +        rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH3, 
>>> sgxv->ia32_sgxlepubkeyhash[3]);
>>> +    }
>>> +}
>>> +
>>> +void sgx_ctxt_switch_to(struct vcpu *v)
>>> +{
>>> +    struct sgx_vcpu *sgxv = to_sgx_vcpu(v);
>>> +
>>> +    if ( sgxv->writable && sgx_ia32_sgxlepubkeyhash_writable() )
>>
>> This causes a read of FEATURE_CONTROL on every context switch path,
>> which is inefficient.
>>
>> Just like with CPUID policy, we will (eventually) have a generic MSR
>> policy for the guest to use.  In particular, I can forsee a usecase
>> where hardware has LC unlocked, but the host administrator wishes LC to
>> be locked from the guests point of view.
> 
> We can remove sgx_ia32_sgxlepubkeyhash_writable, as if sgxv->writable is 
> true, then sgx_ia32_sgxlepubkeyhash_writable is always true. I am not 
> sure whether we should leave guest in locked mode in most cases but I 
> think we can add 'lewr' XL parameter to explicitly set guest to unlocked 
> mode (otherwise guest is locked). Please see my latest reply to design 
> of Launch Control.

Hi Andrew,

I'd like to add something regarding to performance optimization of 
updating the MSRs. There are two optimizations that we can do:

- We can add per_cpu variable for the MSRs, and keep the per_vcpu 
variable always being equal to physical MSRs, then we only need to 
update the MSRs when the value we want to update is not equal to the 
value of per_vcpu variable. This can reduce some physical MSR write.

- Thanks to current SGX implementation, IA32_SGXLEPUBKEYHASHn MSRs are 
only used by EINIT. Once EINIT is done, EGETKEY won't depend on 
IA32_SGXLEPUBKEYHASHn. So we can also trap guest's EINIT, and update 
MSRs in EINIT VMEXIT. However if we trap EINIT, Xen needs to run EINIT 
on behalf of guest, meaning Xen needs to remap guest's EINIT parameters 
(we probably don't need to reconstruct EINIT parameters in Xen as both 
SIGSTRUCT and EINITTOKEN don't contain guest's virtual address 
internally that needs to be remapped by Xen), run EINIT, and emulate 
EINIT return value to guest.

The first optimization is pretty straightforward, and I think we should 
do it (and I'll do in next version). The second optimization requires 
trapping EINIT from guest (thus more complicated implementation) but can 
eliminate unnecessary MSR updates during context switch. Do you think we 
should do both the optimizations?

Thanks,
-Kai
> 
>>
>>> +    {
>>> +        wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0, 
>>> sgxv->ia32_sgxlepubkeyhash[0]);
>>> +        wrmsrl(MSR_IA32_SGXLEPUBKEYHASH1, 
>>> sgxv->ia32_sgxlepubkeyhash[1]);
>>> +        wrmsrl(MSR_IA32_SGXLEPUBKEYHASH2, 
>>> sgxv->ia32_sgxlepubkeyhash[2]);
>>> +        wrmsrl(MSR_IA32_SGXLEPUBKEYHASH3, 
>>> sgxv->ia32_sgxlepubkeyhash[3]);
>>> +    }
>>> +}
>>> +
>>> +int sgx_msr_read_intercept(struct vcpu *v, unsigned int msr, u64 
>>> *msr_content)
>>> +{
>>> +    struct sgx_vcpu *sgxv = to_sgx_vcpu(v);
>>> +    u64 data;
>>> +    int r = 1;
>>> +
>>> +    if ( !domain_has_sgx(v->domain) )
>>> +        return 0;
>>> +
>>> +    switch ( msr )
>>> +    {
>>> +    case MSR_IA32_FEATURE_CONTROL:
>>> +        data = (IA32_FEATURE_CONTROL_LOCK |
>>> +                IA32_FEATURE_CONTROL_SGX_ENABLE);
>>> +        /*
>>> +         * If physical IA32_SGXLEPUBKEYHASHn are writable, then we 
>>> always
>>> +         * allow guest to be able to change IA32_SGXLEPUBKEYHASHn at 
>>> runtime.
>>> +         */
>>> +        if ( sgx_ia32_sgxlepubkeyhash_writable() &&
>>> +                domain_has_sgx_launch_control(v->domain) )
>>> +            data |= IA32_FEATURE_CONTROL_SGX_LAUNCH_CONTROL_ENABLE;
>>> +
>>> +        *msr_content = data;
>>> +
>>> +        break;
>>
>> Newline here please.
> 
> Sure.
> 
>>
>>> +    case MSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:
>>
>> Spaces around ... please.  (it is only because of the #defines that this
>> isn't a syntax error).
> 
> Will do.
> 
>>
>>> +        /*
>>> +         * SDM 35.1 Model-Specific Registers, table 35-2.
>>> +         *
>>> +         * IA32_SGXLEPUBKEYHASH[0..3]:
>>> +         *
>>> +         * Read permitted if CPUID.0x12.0:EAX[0] = 1.
>>> +         *
>>> +         * In reality, MSRs may not be readable even SGX is present, 
>>> in which
>>> +         * case guest is not allowed to read either.
>>> +         */
>>> +        if ( !sgxv->readable )
>>> +        {
>>> +            r = 0;
>>> +            break;
>>> +        }
>>> +
>>> +        data = sgxv->ia32_sgxlepubkeyhash[msr - 
>>> MSR_IA32_SGXLEPUBKEYHASH0];
>>> +
>>> +        *msr_content = data;
>>> +
>>> +        break;
>>> +    default:
>>> +        r = 0;
>>> +        break;
>>> +    }
>>> +
>>> +    return r;
>>> +}
>>> +
>>> +int sgx_msr_write_intercept(struct vcpu *v, unsigned int msr, u64 
>>> msr_content)
>>> +{
>>> +    struct sgx_vcpu *sgxv = to_sgx_vcpu(v);
>>> +    int r = 1;
>>> +
>>> +    if ( !domain_has_sgx(v->domain) )
>>> +        return 0;
>>> +
>>> +    switch ( msr )
>>> +    {
>>> +    case MSR_IA32_FEATURE_CONTROL:
>>> +        /* sliently drop */
>>
>> Silently dropping is not ok.  This change needs rebasing over c/s
>> 46c3acb308 where I have fixed up the writeability of FEATURE_CONTROL.
> 
> Thanks. I'll take a look and change accordingly.
> 
>>
>>> +        break;
>>> +    case MSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:
>>> +        /*
>>> +         * SDM 35.1 Model-Specific Registers, table 35-2.
>>> +         *
>>> +         * IA32_SGXLEPUBKEYHASH[0..3]:
>>> +         *
>>> +         * - If CPUID.0x7.0:ECX[30] = 1, FEATURE_CONTROL[17] is 
>>> available.
>>> +         * - Write permitted if CPUID.0x12.0:EAX[0] = 1 &&
>>> +         *      FEATURE_CONTROL[17] = 1 && FEATURE_CONTROL[0] = 1.
>>> +         *
>>> +         * sgxv->writable == 1 means 
>>> sgx_ia32_sgxlepubkeyhash_writable() and
>>> +         * domain_has_sgx_launch_control(d) both are true.
>>> +         */
>>> +        if ( !sgxv->writable )
>>> +        {
>>> +            r = 0;
>>> +            break;
>>> +        }
>>> +
>>> +        sgxv->ia32_sgxlepubkeyhash[msr - MSR_IA32_SGXLEPUBKEYHASH0] =
>>> +            msr_content;
>>> +
>>> +        break;
>>> +    default:
>>> +        r = 0;
>>> +        break;
>>> +    }
>>> +
>>> +    return r;
>>> +}
>>> +
>>>   static bool_t sgx_enabled_in_bios(void)
>>>   {
>>>       uint64_t val, sgx_enabled = IA32_FEATURE_CONTROL_SGX_ENABLE |
>>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>>> index 243643111d..7ee5515bdc 100644
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -470,6 +470,8 @@ static int vmx_vcpu_initialise(struct vcpu *v)
>>>       if ( v->vcpu_id == 0 )
>>>           v->arch.user_regs.rax = 1;
>>> +    sgx_vcpu_init(v);
>>> +
>>>       return 0;
>>>   }
>>> @@ -1048,6 +1050,9 @@ static void vmx_ctxt_switch_to(struct vcpu *v)
>>>       if ( v->domain->arch.hvm_domain.pi_ops.switch_to )
>>>           v->domain->arch.hvm_domain.pi_ops.switch_to(v);
>>> +
>>> +    if ( domain_has_sgx(v->domain) )
>>> +        sgx_ctxt_switch_to(v);
>>>   }
>>> @@ -2876,10 +2881,20 @@ static int vmx_msr_read_intercept(unsigned 
>>> int msr, uint64_t *msr_content)
>>>           __vmread(GUEST_IA32_DEBUGCTL, msr_content);
>>>           break;
>>>       case MSR_IA32_FEATURE_CONTROL:
>>> +        /* If neither SGX nor nested is supported, this MSR should 
>>> not be
>>> +         * touched */
>>> +        if ( !sgx_msr_read_intercept(current, msr, msr_content) &&
>>> +                !nvmx_msr_read_intercept(msr, msr_content) )
>>> +            goto gp_fault;
>>
>> Unfortunately, this logic is broken.  In the case that both SMX and VMX
>> are configured, the VMX handler will clobber the values set up by the
>> SGX handler.  Sergey has a VMX-policy series (v1 posted, v2 in the
>> works) to start addressing some of the issues on the VMX side, but
>> fundamentally, all reads like this need serving out of a single policy,
>> rather than having different subsystems fighting for control of the
>> values.  (The Xen MSR code is terrible for this at the moment.)
> 
> Thanks for pointing out. I have located vmx-policy series. I'll look 
> into it to see how should I change this logic.
> 
>>
>>> +        break;
>>>       case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_VMFUNC:
>>>           if ( !nvmx_msr_read_intercept(msr, msr_content) )
>>>               goto gp_fault;
>>>           break;
>>> +    case MSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:
>>> +        if ( !sgx_msr_read_intercept(current, msr, msr_content) )
>>> +            goto gp_fault;
>>> +        break;
>>>       case MSR_IA32_MISC_ENABLE:
>>>           rdmsrl(MSR_IA32_MISC_ENABLE, *msr_content);
>>>           /* Debug Trace Store is not supported. */
>>> @@ -3119,10 +3134,19 @@ static int vmx_msr_write_intercept(unsigned 
>>> int msr, uint64_t msr_content)
>>>           break;
>>>       }
>>>       case MSR_IA32_FEATURE_CONTROL:
>>> +        /* See vmx_msr_read_intercept */
>>> +        if ( !sgx_msr_write_intercept(current, msr, msr_content) &&
>>> +                !nvmx_msr_write_intercept(msr, msr_content) )
>>
>> Definitely needs a rebase.  nvmx_msr_write_intercept() has been removed.
> 
> Yeah. The code base of this series is about 3-4 weeks ago unfortunately. 
> Will do.
> 
> Thanks,
> -Kai
> 
>>
>> ~Andrew
>>
>>> +            goto gp_fault;
>>> +        break;
>>>       case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_TRUE_ENTRY_CTLS:
>>>           if ( !nvmx_msr_write_intercept(msr, msr_content) )
>>>               goto gp_fault;
>>>           break;
>>> +    case MSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:
>>> +        if ( !sgx_msr_write_intercept(current, msr, msr_content) )
>>> +            goto gp_fault;
>>> +        break;
>>>       case MSR_P6_PERFCTR(0)...MSR_P6_PERFCTR(7):
>>>       case MSR_P6_EVNTSEL(0)...MSR_P6_EVNTSEL(7):
>>>       case MSR_CORE_PERF_FIXED_CTR0...MSR_CORE_PERF_FIXED_CTR2:
>>> diff --git a/xen/include/asm-x86/cpufeature.h 
>>> b/xen/include/asm-x86/cpufeature.h
>>> index 9793f8c1c5..dfb17c4bd8 100644
>>> --- a/xen/include/asm-x86/cpufeature.h
>>> +++ b/xen/include/asm-x86/cpufeature.h
>>> @@ -98,6 +98,9 @@
>>>   #define cpu_has_smap            boot_cpu_has(X86_FEATURE_SMAP)
>>>   #define cpu_has_sha             boot_cpu_has(X86_FEATURE_SHA)
>>> +/* CPUID level 0x00000007:0.ecx */
>>> +#define cpu_has_sgx_launch_control  
>>> boot_cpu_has(X86_FEATURE_SGX_LAUNCH_CONTROL)
>>> +
>>>   /* CPUID level 0x80000007.edx */
>>>   #define cpu_has_itsc            boot_cpu_has(X86_FEATURE_ITSC)
>>> diff --git a/xen/include/asm-x86/hvm/vmx/sgx.h 
>>> b/xen/include/asm-x86/hvm/vmx/sgx.h
>>> index 40f860662a..c460f61e5e 100644
>>> --- a/xen/include/asm-x86/hvm/vmx/sgx.h
>>> +++ b/xen/include/asm-x86/hvm/vmx/sgx.h
>>> @@ -75,4 +75,26 @@ int hvm_populate_epc(struct domain *d, unsigned 
>>> long epc_base_pfn,
>>>   int hvm_reset_epc(struct domain *d, bool_t free_epc);
>>>   void hvm_destroy_epc(struct domain *d);
>>> +/* Per-vcpu SGX structure */
>>> +struct sgx_vcpu {
>>> +    uint64_t ia32_sgxlepubkeyhash[4];
>>> +    /*
>>> +     * Although SDM says if SGX is present, then 
>>> IA32_SGXLEPUBKEYHASHn are
>>> +     * available for read, but in reality for SKYLAKE client 
>>> machines, those
>>> +     * those MSRs are not available if SGX is present.
>>> +     */
>>> +    bool_t readable;
>>> +    bool_t writable;
>>> +};
>>> +#define to_sgx_vcpu(v)  (&(v->arch.hvm_vmx.sgx))
>>> +
>>> +bool_t sgx_ia32_sgxlepubkeyhash_writable(void);
>>> +bool_t domain_has_sgx(struct domain *d);
>>> +bool_t domain_has_sgx_launch_control(struct domain *d);
>>> +
>>> +void sgx_vcpu_init(struct vcpu *v);
>>> +void sgx_ctxt_switch_to(struct vcpu *v);
>>> +int sgx_msr_read_intercept(struct vcpu *v, unsigned int msr, u64 
>>> *msr_content);
>>> +int sgx_msr_write_intercept(struct vcpu *v, unsigned int msr, u64 
>>> msr_content);
>>> +
>>>   #endif  /* __ASM_X86_HVM_VMX_SGX_H__ */
>>> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h 
>>> b/xen/include/asm-x86/hvm/vmx/vmcs.h
>>> index 6cfa5c3310..fc0b9d85fd 100644
>>> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
>>> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
>>> @@ -160,6 +160,8 @@ struct arch_vmx_struct {
>>>        * pCPU and wakeup the related vCPU.
>>>        */
>>>       struct pi_blocking_vcpu pi_blocking;
>>> +
>>> +    struct sgx_vcpu sgx;
>>>   };
>>>   int vmx_create_vmcs(struct vcpu *v);
>>> diff --git a/xen/include/asm-x86/msr-index.h 
>>> b/xen/include/asm-x86/msr-index.h
>>> index 771e7500af..16206a11b7 100644
>>> --- a/xen/include/asm-x86/msr-index.h
>>> +++ b/xen/include/asm-x86/msr-index.h
>>> @@ -296,6 +296,12 @@
>>>   #define IA32_FEATURE_CONTROL_SENTER_PARAM_CTL         0x7f00
>>>   #define IA32_FEATURE_CONTROL_ENABLE_SENTER            0x8000
>>>   #define IA32_FEATURE_CONTROL_SGX_ENABLE               0x40000
>>> +#define IA32_FEATURE_CONTROL_SGX_LAUNCH_CONTROL_ENABLE  0x20000
>>> +
>>> +#define MSR_IA32_SGXLEPUBKEYHASH0   0x0000008c
>>> +#define MSR_IA32_SGXLEPUBKEYHASH1   0x0000008d
>>> +#define MSR_IA32_SGXLEPUBKEYHASH2   0x0000008e
>>> +#define MSR_IA32_SGXLEPUBKEYHASH3   0x0000008f
>>>   #define MSR_IA32_TSC_ADJUST        0x0000003b
>>
>>
>> _______________________________________________
>> 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
diff mbox

Patch

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;
+}
+
+/* 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] = SGX_INTEL_DEFAULT_LEPUBKEYHASH3;
+
+        sgxv->readable = 1;
+        sgxv->writable = domain_has_sgx_launch_control(v->domain);
+    }
+    else
+    {
+        uint64_t v;
+        /*
+         * Although SDM says if SGX is present, then IA32_SGXLEPUBKEYHASHn are
+         * available for read, but in reality for SKYLAKE client machines,
+         * those MSRs are not available if SGX is present, so we cannot rely on
+         * cpu_has_sgx to determine whether to we are able to read MSRs,
+         * instead, we always use rdmsr_safe.
+         */
+        sgxv->readable = rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH0, v) ? 0 : 1;
+
+        if ( !sgxv->readable )
+            return;
+
+        rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH0, sgxv->ia32_sgxlepubkeyhash[0]);
+        rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH1, sgxv->ia32_sgxlepubkeyhash[1]);
+        rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH2, sgxv->ia32_sgxlepubkeyhash[2]);
+        rdmsr_safe(MSR_IA32_SGXLEPUBKEYHASH3, sgxv->ia32_sgxlepubkeyhash[3]);
+    }
+}
+
+void sgx_ctxt_switch_to(struct vcpu *v)
+{
+    struct sgx_vcpu *sgxv = to_sgx_vcpu(v);
+
+    if ( sgxv->writable && sgx_ia32_sgxlepubkeyhash_writable() )
+    {
+        wrmsrl(MSR_IA32_SGXLEPUBKEYHASH0, sgxv->ia32_sgxlepubkeyhash[0]);
+        wrmsrl(MSR_IA32_SGXLEPUBKEYHASH1, sgxv->ia32_sgxlepubkeyhash[1]);
+        wrmsrl(MSR_IA32_SGXLEPUBKEYHASH2, sgxv->ia32_sgxlepubkeyhash[2]);
+        wrmsrl(MSR_IA32_SGXLEPUBKEYHASH3, sgxv->ia32_sgxlepubkeyhash[3]);
+    }
+}
+
+int sgx_msr_read_intercept(struct vcpu *v, unsigned int msr, u64 *msr_content)
+{
+    struct sgx_vcpu *sgxv = to_sgx_vcpu(v);
+    u64 data;
+    int r = 1;
+
+    if ( !domain_has_sgx(v->domain) )
+        return 0;
+
+    switch ( msr )
+    {
+    case MSR_IA32_FEATURE_CONTROL:
+        data = (IA32_FEATURE_CONTROL_LOCK |
+                IA32_FEATURE_CONTROL_SGX_ENABLE);
+        /*
+         * If physical IA32_SGXLEPUBKEYHASHn are writable, then we always
+         * allow guest to be able to change IA32_SGXLEPUBKEYHASHn at runtime.
+         */
+        if ( sgx_ia32_sgxlepubkeyhash_writable() &&
+                domain_has_sgx_launch_control(v->domain) )
+            data |= IA32_FEATURE_CONTROL_SGX_LAUNCH_CONTROL_ENABLE;
+
+        *msr_content = data;
+
+        break;
+    case MSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:
+        /*
+         * SDM 35.1 Model-Specific Registers, table 35-2.
+         *
+         * IA32_SGXLEPUBKEYHASH[0..3]:
+         *
+         * Read permitted if CPUID.0x12.0:EAX[0] = 1.
+         *
+         * In reality, MSRs may not be readable even SGX is present, in which
+         * case guest is not allowed to read either.
+         */
+        if ( !sgxv->readable )
+        {
+            r = 0;
+            break;
+        }
+
+        data = sgxv->ia32_sgxlepubkeyhash[msr - MSR_IA32_SGXLEPUBKEYHASH0];
+
+        *msr_content = data;
+
+        break;
+    default:
+        r = 0;
+        break;
+    }
+
+    return r;
+}
+
+int sgx_msr_write_intercept(struct vcpu *v, unsigned int msr, u64 msr_content)
+{
+    struct sgx_vcpu *sgxv = to_sgx_vcpu(v);
+    int r = 1;
+
+    if ( !domain_has_sgx(v->domain) )
+        return 0;
+
+    switch ( msr )
+    {
+    case MSR_IA32_FEATURE_CONTROL:
+        /* sliently drop */
+        break;
+    case MSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:
+        /*
+         * SDM 35.1 Model-Specific Registers, table 35-2.
+         *
+         * IA32_SGXLEPUBKEYHASH[0..3]:
+         *
+         * - If CPUID.0x7.0:ECX[30] = 1, FEATURE_CONTROL[17] is available.
+         * - Write permitted if CPUID.0x12.0:EAX[0] = 1 &&
+         *      FEATURE_CONTROL[17] = 1 && FEATURE_CONTROL[0] = 1.
+         *
+         * sgxv->writable == 1 means sgx_ia32_sgxlepubkeyhash_writable() and
+         * domain_has_sgx_launch_control(d) both are true.
+         */
+        if ( !sgxv->writable )
+        {
+            r = 0;
+            break;
+        }
+
+        sgxv->ia32_sgxlepubkeyhash[msr - MSR_IA32_SGXLEPUBKEYHASH0] =
+            msr_content;
+
+        break;
+    default:
+        r = 0;
+        break;
+    }
+
+    return r;
+}
+
 static bool_t sgx_enabled_in_bios(void)
 {
     uint64_t val, sgx_enabled = IA32_FEATURE_CONTROL_SGX_ENABLE |
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 243643111d..7ee5515bdc 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -470,6 +470,8 @@  static int vmx_vcpu_initialise(struct vcpu *v)
     if ( v->vcpu_id == 0 )
         v->arch.user_regs.rax = 1;
 
+    sgx_vcpu_init(v);
+
     return 0;
 }
 
@@ -1048,6 +1050,9 @@  static void vmx_ctxt_switch_to(struct vcpu *v)
 
     if ( v->domain->arch.hvm_domain.pi_ops.switch_to )
         v->domain->arch.hvm_domain.pi_ops.switch_to(v);
+
+    if ( domain_has_sgx(v->domain) )
+        sgx_ctxt_switch_to(v);
 }
 
 
@@ -2876,10 +2881,20 @@  static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
         __vmread(GUEST_IA32_DEBUGCTL, msr_content);
         break;
     case MSR_IA32_FEATURE_CONTROL:
+        /* If neither SGX nor nested is supported, this MSR should not be
+         * touched */
+        if ( !sgx_msr_read_intercept(current, msr, msr_content) &&
+                !nvmx_msr_read_intercept(msr, msr_content) )
+            goto gp_fault;
+        break;
     case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_VMFUNC:
         if ( !nvmx_msr_read_intercept(msr, msr_content) )
             goto gp_fault;
         break;
+    case MSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:
+        if ( !sgx_msr_read_intercept(current, msr, msr_content) )
+            goto gp_fault;
+        break;
     case MSR_IA32_MISC_ENABLE:
         rdmsrl(MSR_IA32_MISC_ENABLE, *msr_content);
         /* Debug Trace Store is not supported. */
@@ -3119,10 +3134,19 @@  static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
         break;
     }
     case MSR_IA32_FEATURE_CONTROL:
+        /* See vmx_msr_read_intercept */
+        if ( !sgx_msr_write_intercept(current, msr, msr_content) &&
+                !nvmx_msr_write_intercept(msr, msr_content) )
+            goto gp_fault;
+        break;
     case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_TRUE_ENTRY_CTLS:
         if ( !nvmx_msr_write_intercept(msr, msr_content) )
             goto gp_fault;
         break;
+    case MSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:
+        if ( !sgx_msr_write_intercept(current, msr, msr_content) )
+            goto gp_fault;
+        break;
     case MSR_P6_PERFCTR(0)...MSR_P6_PERFCTR(7):
     case MSR_P6_EVNTSEL(0)...MSR_P6_EVNTSEL(7):
     case MSR_CORE_PERF_FIXED_CTR0...MSR_CORE_PERF_FIXED_CTR2:
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 9793f8c1c5..dfb17c4bd8 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -98,6 +98,9 @@ 
 #define cpu_has_smap            boot_cpu_has(X86_FEATURE_SMAP)
 #define cpu_has_sha             boot_cpu_has(X86_FEATURE_SHA)
 
+/* CPUID level 0x00000007:0.ecx */
+#define cpu_has_sgx_launch_control  boot_cpu_has(X86_FEATURE_SGX_LAUNCH_CONTROL)
+
 /* CPUID level 0x80000007.edx */
 #define cpu_has_itsc            boot_cpu_has(X86_FEATURE_ITSC)
 
diff --git a/xen/include/asm-x86/hvm/vmx/sgx.h b/xen/include/asm-x86/hvm/vmx/sgx.h
index 40f860662a..c460f61e5e 100644
--- a/xen/include/asm-x86/hvm/vmx/sgx.h
+++ b/xen/include/asm-x86/hvm/vmx/sgx.h
@@ -75,4 +75,26 @@  int hvm_populate_epc(struct domain *d, unsigned long epc_base_pfn,
 int hvm_reset_epc(struct domain *d, bool_t free_epc);
 void hvm_destroy_epc(struct domain *d);
 
+/* Per-vcpu SGX structure */
+struct sgx_vcpu {
+    uint64_t ia32_sgxlepubkeyhash[4];
+    /*
+     * Although SDM says if SGX is present, then IA32_SGXLEPUBKEYHASHn are
+     * available for read, but in reality for SKYLAKE client machines, those
+     * those MSRs are not available if SGX is present.
+     */
+    bool_t readable;
+    bool_t writable;
+};
+#define to_sgx_vcpu(v)  (&(v->arch.hvm_vmx.sgx))
+
+bool_t sgx_ia32_sgxlepubkeyhash_writable(void);
+bool_t domain_has_sgx(struct domain *d);
+bool_t domain_has_sgx_launch_control(struct domain *d);
+
+void sgx_vcpu_init(struct vcpu *v);
+void sgx_ctxt_switch_to(struct vcpu *v);
+int sgx_msr_read_intercept(struct vcpu *v, unsigned int msr, u64 *msr_content);
+int sgx_msr_write_intercept(struct vcpu *v, unsigned int msr, u64 msr_content);
+
 #endif  /* __ASM_X86_HVM_VMX_SGX_H__ */
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index 6cfa5c3310..fc0b9d85fd 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -160,6 +160,8 @@  struct arch_vmx_struct {
      * pCPU and wakeup the related vCPU.
      */
     struct pi_blocking_vcpu pi_blocking;
+
+    struct sgx_vcpu sgx;
 };
 
 int vmx_create_vmcs(struct vcpu *v);
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index 771e7500af..16206a11b7 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -296,6 +296,12 @@ 
 #define IA32_FEATURE_CONTROL_SENTER_PARAM_CTL         0x7f00
 #define IA32_FEATURE_CONTROL_ENABLE_SENTER            0x8000
 #define IA32_FEATURE_CONTROL_SGX_ENABLE               0x40000
+#define IA32_FEATURE_CONTROL_SGX_LAUNCH_CONTROL_ENABLE  0x20000
+
+#define MSR_IA32_SGXLEPUBKEYHASH0   0x0000008c
+#define MSR_IA32_SGXLEPUBKEYHASH1   0x0000008d
+#define MSR_IA32_SGXLEPUBKEYHASH2   0x0000008e
+#define MSR_IA32_SGXLEPUBKEYHASH3   0x0000008f
 
 #define MSR_IA32_TSC_ADJUST		0x0000003b