| Message ID | 1ca2e437e21b0684518ff32dad292aa6080dcb11.1499586046.git.kai.huang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <kai.huang@linux.intel.com> > --- > 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 <asm/paging.h> > #include <asm/processor.h> > #include <asm/xstate.h> > +#include <asm/hvm/vmx/sgx.h> > > 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. 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. > +{ > + if ( !p->feat.sgx ) > + { > + memset(&p->sgx, 0, sizeof (p->sgx)); > + return; > + } > + > + if ( !p->sgx.sgx1 ) > + { > + memset(&p->sgx, 0, sizeof (p->sgx)); > + return; > + } These two clauses can be combined. > + > + /* > + * 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. > + > + if ( hide_epc ) > + { > + memset(&p->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 multiple EPC sections (meaning subleaf 3, > + * 4, ... may also be valid), but as the policy is for guest so we only > + * need one EPC section (subleaf 2). > + */ > + cpuid_count_leaf(SGX_CPUID, 0, &p->sgx.raw[0]); > + cpuid_count_leaf(SGX_CPUID, 0, &p->sgx.raw[0]); > + cpuid_count_leaf(SGX_CPUID, 0, &p->sgx.raw[0]); Copy & paste error? I presume you meant to use leaves 1 and 2 here, rather than leaf 0 each time? > + } > + > /* Extended leaves. */ > cpuid_leaf(0x80000000, &p->extd.raw[0]); > for ( i = 1; i < min(ARRAY_SIZE(p->extd.raw), > @@ -324,6 +376,8 @@ static void __init calculate_host_policy(void) > cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p); > recalculate_xstate(p); > recalculate_misc(p); > + /* For host policy we report physical EPC */ > + recalculate_sgx(p, 0); > > if ( p->extd.svm ) > { > @@ -357,6 +411,11 @@ static void __init calculate_pv_max_policy(void) > sanitise_featureset(pv_featureset); > cpuid_featureset_to_policy(pv_featureset, p); > recalculate_xstate(p); > + /* > + * For PV policy we don't report physical EPC. Actually for PV policy > + * currently SGX will be disabled. > + */ > + recalculate_sgx(p, 1); > > p->extd.raw[0xa] = EMPTY_LEAF; /* No SVM for PV guests. */ > } > @@ -413,6 +472,13 @@ static void __init calculate_hvm_max_policy(void) > sanitise_featureset(hvm_featureset); > cpuid_featureset_to_policy(hvm_featureset, p); > recalculate_xstate(p); > + /* > + * For HVM policy we don't report physical EPC. Actually cpuid policy > + * should report VM's virtual EPC base and size. However VM's virtual > + * EPC info will come from toolstack, and only after Xen is notified > + * VM's cpuid policy should report invalid EPC. > + */ > + recalculate_sgx(p, 1); > } > > void __init init_guest_cpuid(void) > @@ -528,6 +594,12 @@ void recalculate_cpuid_policy(struct domain *d) > if ( p->basic.max_leaf < XSTATE_CPUID ) > __clear_bit(X86_FEATURE_XSAVE, fs); > > + if ( p->basic.max_leaf < SGX_CPUID ) > + { > + __clear_bit(X86_FEATURE_SGX, fs); > + __clear_bit(X86_FEATURE_SGX_LAUNCH_CONTROL, fs); Because you filled in the feature dependency graph for SGX_LAUNCH depending on SGX, this second clear bit isn't necessary. Clearing SGX will cause sanitise_featureset() to automatically clear SGX_LAUNCH (and any future feature bits). > + } > + > sanitise_featureset(fs); > > /* Fold host's FDP_EXCP_ONLY and NO_FPU_SEL into guest's view. */ > @@ -550,6 +622,12 @@ void recalculate_cpuid_policy(struct domain *d) > > recalculate_xstate(p); > recalculate_misc(p); > + /* > + * recalculate_cpuid_policy is also called for domain's cpuid policy, > + * which is from toolstack via XEN_DOMCTL_set_cpuid, therefore we cannot > + * hide domain's virtual EPC from toolstack. > + */ > + recalculate_sgx(p, 0); > > for ( i = 0; i < ARRAY_SIZE(p->cache.raw); ++i ) > { > @@ -645,6 +723,13 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, > *res = p->xstate.raw[subleaf]; > break; > > + case SGX_CPUID: > + if ( !p->feat.sgx ) > + return; || subleaf >= ARRAY_SIZE(p->sgx.raw) Otherwise, a guest CPUID query can walk read off the end of raw[]. > + > + *res = p->sgx.raw[subleaf]; > + break; > + > default: > *res = p->basic.raw[leaf]; > break; > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c > index f40e989fd8..7d49947a3e 100644 > --- a/xen/arch/x86/domctl.c > +++ b/xen/arch/x86/domctl.c > @@ -53,6 +53,7 @@ static int update_domain_cpuid_info(struct domain *d, > struct cpuid_policy *p = d->arch.cpuid; > const struct cpuid_leaf leaf = { ctl->eax, ctl->ebx, ctl->ecx, ctl->edx }; > int old_vendor = p->x86_vendor; > + int ret = 0; > > /* > * Skip update for leaves we don't care about. This avoids the overhead > @@ -74,6 +75,7 @@ static int update_domain_cpuid_info(struct domain *d, > if ( ctl->input[0] == XSTATE_CPUID && > ctl->input[1] != 1 ) /* Everything else automatically calculated. */ > return 0; > + > break; > > case 0x40000000: case 0x40000100: > @@ -104,6 +106,10 @@ static int update_domain_cpuid_info(struct domain *d, > p->xstate.raw[ctl->input[1]] = leaf; > break; > > + case SGX_CPUID: > + p->sgx.raw[ctl->input[1]] = leaf; > + break; You also need to modify the higher switch statement so the toolstack can't cause Xen to write beyond the end of .raw[]. > + > default: > p->basic.raw[ctl->input[0]] = leaf; > break; > @@ -255,6 +261,45 @@ static int update_domain_cpuid_info(struct domain *d, > } > break; > > + case 0x12: > + { > + uint64_t base_pfn, npages; > + > + if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ) > + break; > + > + if ( ctl->input[1] != 2 ) > + break; > + > + /* SGX has not enabled */ > + if ( !p->feat.sgx || !p->sgx.sgx1 ) > + break; > + > + /* > + * If SGX is enabled in CPUID, then we are expecting valid EPC resource > + * in sub-leaf 0x2. Return -EFAULT to notify toolstack that there's > + * something wrong. > + */ > + if ( !p->sgx.base_valid || !p->sgx.size_valid ) Is there any plausible usecase where only one of these is valid? If not, why are they split? > + { > + ret = -EINVAL; > + break; > + } > + > + base_pfn = (((uint64_t)(p->sgx.base_pfn_high)) << 20) | > + (uint64_t)p->sgx.base_pfn_low; > + npages = (((uint64_t)(p->sgx.npages_high)) << 20) | > + (uint64_t)p->sgx.npages_low; > + > + if ( !hvm_epc_populated(d) ) > + ret = hvm_populate_epc(d, base_pfn, npages); > + else > + if ( base_pfn != to_sgx(d)->epc_base_pfn || > + npages != to_sgx(d)->epc_npages ) > + ret = -EINVAL; > + > + break; > + } > case 0x80000001: > if ( is_pv_domain(d) && ((levelling_caps & LCAP_e1cd) == LCAP_e1cd) ) > { > @@ -299,7 +344,7 @@ static int update_domain_cpuid_info(struct domain *d, > break; > } > > - return 0; > + return ret; > } > > void arch_get_domain_info(const struct domain *d, > diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h > index ac25908eca..326f267263 100644 > --- a/xen/include/asm-x86/cpuid.h > +++ b/xen/include/asm-x86/cpuid.h > @@ -61,10 +61,11 @@ extern struct cpuidmasks cpuidmask_defaults; > /* Whether or not cpuid faulting is available for the current domain. */ > DECLARE_PER_CPU(bool, cpuid_faulting_enabled); > > -#define CPUID_GUEST_NR_BASIC (0xdu + 1) > +#define CPUID_GUEST_NR_BASIC (0x12u + 1) > #define CPUID_GUEST_NR_FEAT (0u + 1) > #define CPUID_GUEST_NR_CACHE (5u + 1) > #define CPUID_GUEST_NR_XSTATE (62u + 1) > +#define CPUID_GUEST_NR_SGX (0x2u + 1) > #define CPUID_GUEST_NR_EXTD_INTEL (0x8u + 1) > #define CPUID_GUEST_NR_EXTD_AMD (0x1cu + 1) > #define CPUID_GUEST_NR_EXTD MAX(CPUID_GUEST_NR_EXTD_INTEL, \ > @@ -169,6 +170,29 @@ struct cpuid_policy > } comp[CPUID_GUEST_NR_XSTATE]; > } xstate; > > + union { > + struct cpuid_leaf raw[CPUID_GUEST_NR_SGX]; > + > + struct { > + /* Subleaf 0. */ > + uint32_t sgx1:1, sgx2:1, :30; Please use bool bitfields for these. Something like: bool sgx1:1 sgx2:2; uint32_t :30; should be fine. > + uint32_t miscselect, /* c */ :32; > + uint32_t maxenclavesize_n64:8, maxenclavesize_64:8, :16; uint8_t for these please, rather than an 8 bit bitfield. Can we use clearer names, such as maxsize_legacy and maxsize_long? They will be accessed via p->sgx. anyway, so the "enclave" bit of context is already present. > + > + /* Subleaf 1. */ > + uint32_t init:1, debug:1, mode64:1, /*reserve*/:1, provisionkey:1, > + einittokenkey:1, :26; bools as well please here. > + uint32_t /* reserve */:32; > + uint32_t xfrm_low, xfrm_high; uint64_t xfrm ? The XSAVE words are apart because they are not adjacent in the architectural layout, but these are. > + > + /* 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. I would also drop the _pfn from the base names. The fields still need shifting to get a sensible value. ~Andrew > + } sgx; > + > /* Extended leaves: 0x800000xx */ > union { > struct cpuid_leaf raw[CPUID_GUEST_NR_EXTD];
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 <kai.huang@linux.intel.com> >> --- >> 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 <asm/paging.h> >> #include <asm/processor.h> >> #include <asm/xstate.h> >> +#include <asm/hvm/vmx/sgx.h> >> 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(&p->sgx, 0, sizeof (p->sgx)); >> + return; >> + } >> + >> + if ( !p->sgx.sgx1 ) >> + { >> + memset(&p->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(&p->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 multiple EPC sections (meaning >> subleaf 3, >> + * 4, ... may also be valid), but as the policy is for guest >> so we only >> + * need one EPC section (subleaf 2). >> + */ >> + cpuid_count_leaf(SGX_CPUID, 0, &p->sgx.raw[0]); >> + cpuid_count_leaf(SGX_CPUID, 0, &p->sgx.raw[0]); >> + cpuid_count_leaf(SGX_CPUID, 0, &p->sgx.raw[0]); > > Copy & paste error? I presume you meant to use leaves 1 and 2 here, > rather than leaf 0 each time? Oh sorry. Yes indeed I meant zero out subleaf 1 and 2. > >> + } >> + >> /* Extended leaves. */ >> cpuid_leaf(0x80000000, &p->extd.raw[0]); >> for ( i = 1; i < min(ARRAY_SIZE(p->extd.raw), >> @@ -324,6 +376,8 @@ static void __init calculate_host_policy(void) >> cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p); >> recalculate_xstate(p); >> recalculate_misc(p); >> + /* For host policy we report physical EPC */ >> + recalculate_sgx(p, 0); >> if ( p->extd.svm ) >> { >> @@ -357,6 +411,11 @@ static void __init calculate_pv_max_policy(void) >> sanitise_featureset(pv_featureset); >> cpuid_featureset_to_policy(pv_featureset, p); >> recalculate_xstate(p); >> + /* >> + * For PV policy we don't report physical EPC. Actually for PV >> policy >> + * currently SGX will be disabled. >> + */ >> + recalculate_sgx(p, 1); >> p->extd.raw[0xa] = EMPTY_LEAF; /* No SVM for PV guests. */ >> } >> @@ -413,6 +472,13 @@ static void __init calculate_hvm_max_policy(void) >> sanitise_featureset(hvm_featureset); >> cpuid_featureset_to_policy(hvm_featureset, p); >> recalculate_xstate(p); >> + /* >> + * For HVM policy we don't report physical EPC. Actually cpuid >> policy >> + * should report VM's virtual EPC base and size. However VM's >> virtual >> + * EPC info will come from toolstack, and only after Xen is notified >> + * VM's cpuid policy should report invalid EPC. >> + */ >> + recalculate_sgx(p, 1); >> } >> void __init init_guest_cpuid(void) >> @@ -528,6 +594,12 @@ void recalculate_cpuid_policy(struct domain *d) >> if ( p->basic.max_leaf < XSTATE_CPUID ) >> __clear_bit(X86_FEATURE_XSAVE, fs); >> + if ( p->basic.max_leaf < SGX_CPUID ) >> + { >> + __clear_bit(X86_FEATURE_SGX, fs); >> + __clear_bit(X86_FEATURE_SGX_LAUNCH_CONTROL, fs); > > Because you filled in the feature dependency graph for SGX_LAUNCH > depending on SGX, this second clear bit isn't necessary. Clearing SGX > will cause sanitise_featureset() to automatically clear SGX_LAUNCH (and > any future feature bits). Yes you are right. I'll just clear SGX in next version. > >> + } >> + >> sanitise_featureset(fs); >> /* Fold host's FDP_EXCP_ONLY and NO_FPU_SEL into guest's view. */ >> @@ -550,6 +622,12 @@ void recalculate_cpuid_policy(struct domain *d) >> recalculate_xstate(p); >> recalculate_misc(p); >> + /* >> + * recalculate_cpuid_policy is also called for domain's cpuid >> policy, >> + * which is from toolstack via XEN_DOMCTL_set_cpuid, therefore we >> cannot >> + * hide domain's virtual EPC from toolstack. >> + */ >> + recalculate_sgx(p, 0); >> for ( i = 0; i < ARRAY_SIZE(p->cache.raw); ++i ) >> { >> @@ -645,6 +723,13 @@ void guest_cpuid(const struct vcpu *v, uint32_t >> leaf, >> *res = p->xstate.raw[subleaf]; >> break; >> + case SGX_CPUID: >> + if ( !p->feat.sgx ) >> + return; > > || subleaf >= ARRAY_SIZE(p->sgx.raw) > > Otherwise, a guest CPUID query can walk read off the end of raw[]. Oh yes. Thanks for pointing out. > >> + >> + *res = p->sgx.raw[subleaf]; >> + break; >> + >> default: >> *res = p->basic.raw[leaf]; >> break; >> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c >> index f40e989fd8..7d49947a3e 100644 >> --- a/xen/arch/x86/domctl.c >> +++ b/xen/arch/x86/domctl.c >> @@ -53,6 +53,7 @@ static int update_domain_cpuid_info(struct domain *d, >> struct cpuid_policy *p = d->arch.cpuid; >> const struct cpuid_leaf leaf = { ctl->eax, ctl->ebx, ctl->ecx, >> ctl->edx }; >> int old_vendor = p->x86_vendor; >> + int ret = 0; >> /* >> * Skip update for leaves we don't care about. This avoids the >> overhead >> @@ -74,6 +75,7 @@ static int update_domain_cpuid_info(struct domain *d, >> if ( ctl->input[0] == XSTATE_CPUID && >> ctl->input[1] != 1 ) /* Everything else automatically >> calculated. */ >> return 0; >> + >> break; >> case 0x40000000: case 0x40000100: >> @@ -104,6 +106,10 @@ static int update_domain_cpuid_info(struct domain >> *d, >> p->xstate.raw[ctl->input[1]] = leaf; >> break; >> + case SGX_CPUID: >> + p->sgx.raw[ctl->input[1]] = leaf; >> + break; > > You also need to modify the higher switch statement so the toolstack > can't cause Xen to write beyond the end of .raw[]. Yes I understand your point now. Will do. > >> + >> default: >> p->basic.raw[ctl->input[0]] = leaf; >> break; >> @@ -255,6 +261,45 @@ static int update_domain_cpuid_info(struct domain >> *d, >> } >> break; >> + case 0x12: >> + { >> + uint64_t base_pfn, npages; >> + >> + if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ) >> + break; >> + >> + if ( ctl->input[1] != 2 ) >> + break; >> + >> + /* SGX has not enabled */ >> + if ( !p->feat.sgx || !p->sgx.sgx1 ) >> + break; >> + >> + /* >> + * If SGX is enabled in CPUID, then we are expecting valid >> EPC resource >> + * in sub-leaf 0x2. Return -EFAULT to notify toolstack that >> there's >> + * something wrong. >> + */ >> + if ( !p->sgx.base_valid || !p->sgx.size_valid ) > > Is there any plausible usecase where only one of these is valid? If > not, why are they split? You mean why are they split in SDM? I don't think there's any usecase where only one is valid. In reality, either they both valid, or they both invalid, otherwise there's bug in either CPU ucode or BIOS. This is just the definition in SDM. > >> + { >> + ret = -EINVAL; >> + break; >> + } >> + >> + base_pfn = (((uint64_t)(p->sgx.base_pfn_high)) << 20) | >> + (uint64_t)p->sgx.base_pfn_low; >> + npages = (((uint64_t)(p->sgx.npages_high)) << 20) | >> + (uint64_t)p->sgx.npages_low; >> + >> + if ( !hvm_epc_populated(d) ) >> + ret = hvm_populate_epc(d, base_pfn, npages); >> + else >> + if ( base_pfn != to_sgx(d)->epc_base_pfn || >> + npages != to_sgx(d)->epc_npages ) >> + ret = -EINVAL; >> + >> + break; >> + } >> case 0x80000001: >> if ( is_pv_domain(d) && ((levelling_caps & LCAP_e1cd) == >> LCAP_e1cd) ) >> { >> @@ -299,7 +344,7 @@ static int update_domain_cpuid_info(struct domain *d, >> break; >> } >> - return 0; >> + return ret; >> } >> void arch_get_domain_info(const struct domain *d, >> diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h >> index ac25908eca..326f267263 100644 >> --- a/xen/include/asm-x86/cpuid.h >> +++ b/xen/include/asm-x86/cpuid.h >> @@ -61,10 +61,11 @@ extern struct cpuidmasks cpuidmask_defaults; >> /* Whether or not cpuid faulting is available for the current >> domain. */ >> DECLARE_PER_CPU(bool, cpuid_faulting_enabled); >> -#define CPUID_GUEST_NR_BASIC (0xdu + 1) >> +#define CPUID_GUEST_NR_BASIC (0x12u + 1) >> #define CPUID_GUEST_NR_FEAT (0u + 1) >> #define CPUID_GUEST_NR_CACHE (5u + 1) >> #define CPUID_GUEST_NR_XSTATE (62u + 1) >> +#define CPUID_GUEST_NR_SGX (0x2u + 1) >> #define CPUID_GUEST_NR_EXTD_INTEL (0x8u + 1) >> #define CPUID_GUEST_NR_EXTD_AMD (0x1cu + 1) >> #define CPUID_GUEST_NR_EXTD MAX(CPUID_GUEST_NR_EXTD_INTEL, \ >> @@ -169,6 +170,29 @@ struct cpuid_policy >> } comp[CPUID_GUEST_NR_XSTATE]; >> } xstate; >> + union { >> + struct cpuid_leaf raw[CPUID_GUEST_NR_SGX]; >> + >> + struct { >> + /* Subleaf 0. */ >> + uint32_t sgx1:1, sgx2:1, :30; > > Please use bool bitfields for these. > > Something like: > > bool sgx1:1 sgx2:2; > uint32_t :30; > > should be fine. > OK. Will do. Thanks. >> + uint32_t miscselect, /* c */ :32; >> + uint32_t maxenclavesize_n64:8, maxenclavesize_64:8, :16; > > uint8_t for these please, rather than an 8 bit bitfield. > > Can we use clearer names, such as maxsize_legacy and maxsize_long? They > will be accessed via p->sgx. anyway, so the "enclave" bit of context is > already present. Sure. I'll change to the name you suggested. > >> + >> + /* Subleaf 1. */ >> + uint32_t init:1, debug:1, mode64:1, /*reserve*/:1, >> provisionkey:1, >> + einittokenkey:1, :26; > > bools as well please here. Will do. > >> + uint32_t /* reserve */:32; >> + uint32_t xfrm_low, xfrm_high; > > uint64_t xfrm ? > > The XSAVE words are apart because they are not adjacent in the > architectural layout, but these are. I think the reason I chose xfrm_low and xfrm_high is in recalculate_sgx, I need to reference them: if ( p->basic.xsave ) { p->sgx.xfrm_low |= p->xstate.xcr0_low; p->sgx.xfrm_high |= p->xstate.xcr0_high; } But I have no problem change to xfrm. > >> + >> + /* 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? > > I would also drop the _pfn from the base names. The fields still need > shifting to get a sensible value. OK. Will do. > > ~Andrew > >> + } sgx; >> + >> /* Extended leaves: 0x800000xx */ >> union { >> struct cpuid_leaf raw[CPUID_GUEST_NR_EXTD]; > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
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. > > 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 :( ) > > >> >>> + >>> + /* 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
>>> On 14.07.17 at 09:37, <andrew.cooper3@citrix.com> 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: >>>> + /* 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. But that may not really be portable. I've just checked the Intel compiler (on Windows, admittedly), and it then starts the base and size fields each on an 8-byte boundary. Hence all other fields would then better also be uint64_t. Jan
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
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 <asm/paging.h> #include <asm/processor.h> #include <asm/xstate.h> +#include <asm/hvm/vmx/sgx.h> 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) +{ + if ( !p->feat.sgx ) + { + memset(&p->sgx, 0, sizeof (p->sgx)); + return; + } + + if ( !p->sgx.sgx1 ) + { + memset(&p->sgx, 0, sizeof (p->sgx)); + return; + } + + /* + * 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; + } + + if ( hide_epc ) + { + memset(&p->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 multiple EPC sections (meaning subleaf 3, + * 4, ... may also be valid), but as the policy is for guest so we only + * need one EPC section (subleaf 2). + */ + cpuid_count_leaf(SGX_CPUID, 0, &p->sgx.raw[0]); + cpuid_count_leaf(SGX_CPUID, 0, &p->sgx.raw[0]); + cpuid_count_leaf(SGX_CPUID, 0, &p->sgx.raw[0]); + } + /* Extended leaves. */ cpuid_leaf(0x80000000, &p->extd.raw[0]); for ( i = 1; i < min(ARRAY_SIZE(p->extd.raw), @@ -324,6 +376,8 @@ static void __init calculate_host_policy(void) cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p); recalculate_xstate(p); recalculate_misc(p); + /* For host policy we report physical EPC */ + recalculate_sgx(p, 0); if ( p->extd.svm ) { @@ -357,6 +411,11 @@ static void __init calculate_pv_max_policy(void) sanitise_featureset(pv_featureset); cpuid_featureset_to_policy(pv_featureset, p); recalculate_xstate(p); + /* + * For PV policy we don't report physical EPC. Actually for PV policy + * currently SGX will be disabled. + */ + recalculate_sgx(p, 1); p->extd.raw[0xa] = EMPTY_LEAF; /* No SVM for PV guests. */ } @@ -413,6 +472,13 @@ static void __init calculate_hvm_max_policy(void) sanitise_featureset(hvm_featureset); cpuid_featureset_to_policy(hvm_featureset, p); recalculate_xstate(p); + /* + * For HVM policy we don't report physical EPC. Actually cpuid policy + * should report VM's virtual EPC base and size. However VM's virtual + * EPC info will come from toolstack, and only after Xen is notified + * VM's cpuid policy should report invalid EPC. + */ + recalculate_sgx(p, 1); } void __init init_guest_cpuid(void) @@ -528,6 +594,12 @@ void recalculate_cpuid_policy(struct domain *d) if ( p->basic.max_leaf < XSTATE_CPUID ) __clear_bit(X86_FEATURE_XSAVE, fs); + if ( p->basic.max_leaf < SGX_CPUID ) + { + __clear_bit(X86_FEATURE_SGX, fs); + __clear_bit(X86_FEATURE_SGX_LAUNCH_CONTROL, fs); + } + sanitise_featureset(fs); /* Fold host's FDP_EXCP_ONLY and NO_FPU_SEL into guest's view. */ @@ -550,6 +622,12 @@ void recalculate_cpuid_policy(struct domain *d) recalculate_xstate(p); recalculate_misc(p); + /* + * recalculate_cpuid_policy is also called for domain's cpuid policy, + * which is from toolstack via XEN_DOMCTL_set_cpuid, therefore we cannot + * hide domain's virtual EPC from toolstack. + */ + recalculate_sgx(p, 0); for ( i = 0; i < ARRAY_SIZE(p->cache.raw); ++i ) { @@ -645,6 +723,13 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, *res = p->xstate.raw[subleaf]; break; + case SGX_CPUID: + if ( !p->feat.sgx ) + return; + + *res = p->sgx.raw[subleaf]; + break; + default: *res = p->basic.raw[leaf]; break; diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index f40e989fd8..7d49947a3e 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -53,6 +53,7 @@ static int update_domain_cpuid_info(struct domain *d, struct cpuid_policy *p = d->arch.cpuid; const struct cpuid_leaf leaf = { ctl->eax, ctl->ebx, ctl->ecx, ctl->edx }; int old_vendor = p->x86_vendor; + int ret = 0; /* * Skip update for leaves we don't care about. This avoids the overhead @@ -74,6 +75,7 @@ static int update_domain_cpuid_info(struct domain *d, if ( ctl->input[0] == XSTATE_CPUID && ctl->input[1] != 1 ) /* Everything else automatically calculated. */ return 0; + break; case 0x40000000: case 0x40000100: @@ -104,6 +106,10 @@ static int update_domain_cpuid_info(struct domain *d, p->xstate.raw[ctl->input[1]] = leaf; break; + case SGX_CPUID: + p->sgx.raw[ctl->input[1]] = leaf; + break; + default: p->basic.raw[ctl->input[0]] = leaf; break; @@ -255,6 +261,45 @@ static int update_domain_cpuid_info(struct domain *d, } break; + case 0x12: + { + uint64_t base_pfn, npages; + + if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ) + break; + + if ( ctl->input[1] != 2 ) + break; + + /* SGX has not enabled */ + if ( !p->feat.sgx || !p->sgx.sgx1 ) + break; + + /* + * If SGX is enabled in CPUID, then we are expecting valid EPC resource + * in sub-leaf 0x2. Return -EFAULT to notify toolstack that there's + * something wrong. + */ + if ( !p->sgx.base_valid || !p->sgx.size_valid ) + { + ret = -EINVAL; + break; + } + + base_pfn = (((uint64_t)(p->sgx.base_pfn_high)) << 20) | + (uint64_t)p->sgx.base_pfn_low; + npages = (((uint64_t)(p->sgx.npages_high)) << 20) | + (uint64_t)p->sgx.npages_low; + + if ( !hvm_epc_populated(d) ) + ret = hvm_populate_epc(d, base_pfn, npages); + else + if ( base_pfn != to_sgx(d)->epc_base_pfn || + npages != to_sgx(d)->epc_npages ) + ret = -EINVAL; + + break; + } case 0x80000001: if ( is_pv_domain(d) && ((levelling_caps & LCAP_e1cd) == LCAP_e1cd) ) { @@ -299,7 +344,7 @@ static int update_domain_cpuid_info(struct domain *d, break; } - return 0; + return ret; } void arch_get_domain_info(const struct domain *d, diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h index ac25908eca..326f267263 100644 --- a/xen/include/asm-x86/cpuid.h +++ b/xen/include/asm-x86/cpuid.h @@ -61,10 +61,11 @@ extern struct cpuidmasks cpuidmask_defaults; /* Whether or not cpuid faulting is available for the current domain. */ DECLARE_PER_CPU(bool, cpuid_faulting_enabled); -#define CPUID_GUEST_NR_BASIC (0xdu + 1) +#define CPUID_GUEST_NR_BASIC (0x12u + 1) #define CPUID_GUEST_NR_FEAT (0u + 1) #define CPUID_GUEST_NR_CACHE (5u + 1) #define CPUID_GUEST_NR_XSTATE (62u + 1) +#define CPUID_GUEST_NR_SGX (0x2u + 1) #define CPUID_GUEST_NR_EXTD_INTEL (0x8u + 1) #define CPUID_GUEST_NR_EXTD_AMD (0x1cu + 1) #define CPUID_GUEST_NR_EXTD MAX(CPUID_GUEST_NR_EXTD_INTEL, \ @@ -169,6 +170,29 @@ struct cpuid_policy } comp[CPUID_GUEST_NR_XSTATE]; } xstate; + union { + struct cpuid_leaf raw[CPUID_GUEST_NR_SGX]; + + struct { + /* Subleaf 0. */ + uint32_t sgx1:1, sgx2:1, :30; + uint32_t miscselect, /* c */ :32; + uint32_t maxenclavesize_n64:8, maxenclavesize_64:8, :16; + + /* Subleaf 1. */ + uint32_t init:1, debug:1, mode64:1, /*reserve*/:1, provisionkey:1, + einittokenkey:1, :26; + uint32_t /* reserve */:32; + uint32_t xfrm_low, xfrm_high; + + /* 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; + }; + } sgx; + /* Extended leaves: 0x800000xx */ union { struct cpuid_leaf raw[CPUID_GUEST_NR_EXTD];
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 <kai.huang@linux.intel.com> --- 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(-)