| Message ID | 1483533584-8015-23-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 04.01.17 at 13:39, <andrew.cooper3@citrix.com> wrote: > @@ -306,6 +310,9 @@ void recalculate_cpuid_policy(struct domain *d) > if ( !d->disable_migrate && !d->arch.vtsc ) > __clear_bit(X86_FEATURE_ITSC, fs); > > + if ( p->basic.max_leaf < 0xd ) XSTATE_CPUID > @@ -333,21 +340,50 @@ void guest_cpuid(const struct vcpu *v, unsigned int leaf, > unsigned int subleaf, struct cpuid_leaf *res) > { > const struct domain *d = v->domain; > + const struct cpuid_policy *p = d->arch.cpuid; > > *res = EMPTY_LEAF; > > /* > * First pass: > * - Dispatch the virtualised leaves to their respective handlers. > + * - Perform max_leaf/subleaf calculations, maybe returning early. > */ > switch ( leaf ) > { > + case 0x0 ... 0x6: > + case 0x8 ... 0xc: > +#if 0 /* For when CPUID_GUEST_NR_BASIC isn't 0xd */ > + case 0xe ... CPUID_GUEST_NR_BASIC - 1: > +#endif Perhaps have a BUILD_BUG_ON() in an #else here? > + if ( leaf > p->basic.max_leaf ) > + return; > + break; > + > + case 0x7: > + if ( subleaf > p->feat.max_subleaf ) > + return; > + break; > + > + case 0xd: XSTATE_CPUID again, which raises the question whether switch() really is the best way to deal with things here. > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -3305,27 +3305,6 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, > if ( !edx ) > edx = &dummy; > > - if ( input & 0x7fffffff ) > - { > - /* > - * Requests outside the supported leaf ranges return zero on AMD > - * and the highest basic leaf output on Intel. Uniformly follow > - * the AMD model as the more sane one. > - */ I think this comment would better be moved instead of deleted. Jan
On 05/01/17 13:51, Jan Beulich wrote: > >> @@ -333,21 +340,50 @@ void guest_cpuid(const struct vcpu *v, unsigned int leaf, >> unsigned int subleaf, struct cpuid_leaf *res) >> { >> const struct domain *d = v->domain; >> + const struct cpuid_policy *p = d->arch.cpuid; >> >> *res = EMPTY_LEAF; >> >> /* >> * First pass: >> * - Dispatch the virtualised leaves to their respective handlers. >> + * - Perform max_leaf/subleaf calculations, maybe returning early. >> */ >> switch ( leaf ) >> { >> + case 0x0 ... 0x6: >> + case 0x8 ... 0xc: >> +#if 0 /* For when CPUID_GUEST_NR_BASIC isn't 0xd */ >> + case 0xe ... CPUID_GUEST_NR_BASIC - 1: >> +#endif > Perhaps have a BUILD_BUG_ON() in an #else here? The presence of this was to be a reminder to whomever tries upping max_leaf beyond 0xd. Then again, there is a reasonable chance it will be me. I am half tempted to leave it out. > >> + if ( leaf > p->basic.max_leaf ) >> + return; >> + break; >> + >> + case 0x7: >> + if ( subleaf > p->feat.max_subleaf ) >> + return; >> + break; >> + >> + case 0xd: > XSTATE_CPUID again, I considered this, but having a mix of named an numbered leaves is worse than having them uniformly numbered, especially when visually checking the conditions around the #if 0 case above. I had considered making a cpuid-index.h for leaf names, but most leaves are more commonly referred to by number than name, so I am really not sure if that would be helpful or hindering in the long run. > which raises the question whether switch() really is the best way to deal with things here. What else would you suggest? One way or another (better shown in the context of the following patch), we need one block per union{} to apply max_leaf calculations and read the base data from p->$FOO.raw[$IDX]. > >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -3305,27 +3305,6 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, >> if ( !edx ) >> edx = &dummy; >> >> - if ( input & 0x7fffffff ) >> - { >> - /* >> - * Requests outside the supported leaf ranges return zero on AMD >> - * and the highest basic leaf output on Intel. Uniformly follow >> - * the AMD model as the more sane one. >> - */ > I think this comment would better be moved instead of deleted. Where would you like it? It doesn't have an easy logical place to live in guest_cpuid(). The best I can think of is probably as an extension of the "First Pass" comment. ~Andrew
>>> On 05.01.17 at 15:28, <andrew.cooper3@citrix.com> wrote: > On 05/01/17 13:51, Jan Beulich wrote: >>> @@ -333,21 +340,50 @@ void guest_cpuid(const struct vcpu *v, unsigned int leaf, >>> unsigned int subleaf, struct cpuid_leaf *res) >>> { >>> const struct domain *d = v->domain; >>> + const struct cpuid_policy *p = d->arch.cpuid; >>> >>> *res = EMPTY_LEAF; >>> >>> /* >>> * First pass: >>> * - Dispatch the virtualised leaves to their respective handlers. >>> + * - Perform max_leaf/subleaf calculations, maybe returning early. >>> */ >>> switch ( leaf ) >>> { >>> + case 0x0 ... 0x6: >>> + case 0x8 ... 0xc: >>> +#if 0 /* For when CPUID_GUEST_NR_BASIC isn't 0xd */ >>> + case 0xe ... CPUID_GUEST_NR_BASIC - 1: >>> +#endif >> Perhaps have a BUILD_BUG_ON() in an #else here? > > The presence of this was to be a reminder to whomever tries upping > max_leaf beyond 0xd. Then again, there is a reasonable chance it will > be me. Well, that's why the recommendation to add a BUILD_BUG_ON() - that's a reminder to that "whoever". >>> + if ( leaf > p->basic.max_leaf ) >>> + return; >>> + break; >>> + >>> + case 0x7: >>> + if ( subleaf > p->feat.max_subleaf ) >>> + return; >>> + break; >>> + >>> + case 0xd: >> XSTATE_CPUID again, > > I considered this, but having a mix of named an numbered leaves is worse > than having them uniformly numbered, especially when visually checking > the conditions around the #if 0 case above. > > I had considered making a cpuid-index.h for leaf names, but most leaves > are more commonly referred to by number than name, so I am really not > sure if that would be helpful or hindering in the long run. > >> which raises the question whether switch() really is the best way to deal > with things here. > > What else would you suggest? One way or another (better shown in the > context of the following patch), we need one block per union{} to apply > max_leaf calculations and read the base data from p->$FOO.raw[$IDX]. Actually, perhaps a mixture: Inside the default case have if ( leaf == 0x7 ) { if ( subleaf > p->feat.max_subleaf ) return; } else if ( leaf == 0xd) { if ( subleaf > ARRAY_SIZE(p->xstate.raw) ) return; } if ( leaf > p->basic.max_leaf ) return; Which (by making the last one if rather than else-if) also fixes an issue I've spotted only now: So far you exclude leaves 7 and 0xd from the basic.max_leaf checking. (And this way that check could also go first.) >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -3305,27 +3305,6 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, >>> if ( !edx ) >>> edx = &dummy; >>> >>> - if ( input & 0x7fffffff ) >>> - { >>> - /* >>> - * Requests outside the supported leaf ranges return zero on AMD >>> - * and the highest basic leaf output on Intel. Uniformly follow >>> - * the AMD model as the more sane one. >>> - */ >> I think this comment would better be moved instead of deleted. > > Where would you like it? It doesn't have an easy logical place to live > in guest_cpuid(). The best I can think of is probably as an extension > of the "First Pass" comment. Right there, yes, as an extension to the line you're already adding. Jan
On 05/01/17 14:52, Jan Beulich wrote: >>>> On 05.01.17 at 15:28, <andrew.cooper3@citrix.com> wrote: >> On 05/01/17 13:51, Jan Beulich wrote: >>>> @@ -333,21 +340,50 @@ void guest_cpuid(const struct vcpu *v, unsigned int leaf, >>>> unsigned int subleaf, struct cpuid_leaf *res) >>>> { >>>> const struct domain *d = v->domain; >>>> + const struct cpuid_policy *p = d->arch.cpuid; >>>> >>>> *res = EMPTY_LEAF; >>>> >>>> /* >>>> * First pass: >>>> * - Dispatch the virtualised leaves to their respective handlers. >>>> + * - Perform max_leaf/subleaf calculations, maybe returning early. >>>> */ >>>> switch ( leaf ) >>>> { >>>> + case 0x0 ... 0x6: >>>> + case 0x8 ... 0xc: >>>> +#if 0 /* For when CPUID_GUEST_NR_BASIC isn't 0xd */ >>>> + case 0xe ... CPUID_GUEST_NR_BASIC - 1: >>>> +#endif >>> Perhaps have a BUILD_BUG_ON() in an #else here? >> The presence of this was to be a reminder to whomever tries upping >> max_leaf beyond 0xd. Then again, there is a reasonable chance it will >> be me. > Well, that's why the recommendation to add a BUILD_BUG_ON() - > that's a reminder to that "whoever". Ok. > >>>> + if ( leaf > p->basic.max_leaf ) >>>> + return; >>>> + break; >>>> + >>>> + case 0x7: >>>> + if ( subleaf > p->feat.max_subleaf ) >>>> + return; >>>> + break; >>>> + >>>> + case 0xd: >>> XSTATE_CPUID again, >> I considered this, but having a mix of named an numbered leaves is worse >> than having them uniformly numbered, especially when visually checking >> the conditions around the #if 0 case above. >> >> I had considered making a cpuid-index.h for leaf names, but most leaves >> are more commonly referred to by number than name, so I am really not >> sure if that would be helpful or hindering in the long run. >> >>> which raises the question whether switch() really is the best way to deal >> with things here. >> >> What else would you suggest? One way or another (better shown in the >> context of the following patch), we need one block per union{} to apply >> max_leaf calculations and read the base data from p->$FOO.raw[$IDX]. > Actually, perhaps a mixture: Inside the default case have > > if ( leaf == 0x7 ) > { > if ( subleaf > p->feat.max_subleaf ) > return; > } > else if ( leaf == 0xd) > { > if ( subleaf > ARRAY_SIZE(p->xstate.raw) ) > return; > } > if ( leaf > p->basic.max_leaf ) > return; > > Which (by making the last one if rather than else-if) also fixes an > issue I've spotted only now: So far you exclude leaves 7 and 0xd > from the basic.max_leaf checking. (And this way that check could > also go first.) Very good point, although I still think I'd still prefer a logic block in this form inside a case 0 ... 0x3fffffff to avoid potential leakage if other logic changes. ~Andrew
>>> On 05.01.17 at 16:02, <andrew.cooper3@citrix.com> wrote: > On 05/01/17 14:52, Jan Beulich wrote: >>>>> On 05.01.17 at 15:28, <andrew.cooper3@citrix.com> wrote: >>> What else would you suggest? One way or another (better shown in the >>> context of the following patch), we need one block per union{} to apply >>> max_leaf calculations and read the base data from p->$FOO.raw[$IDX]. >> Actually, perhaps a mixture: Inside the default case have >> >> if ( leaf == 0x7 ) >> { >> if ( subleaf > p->feat.max_subleaf ) >> return; >> } >> else if ( leaf == 0xd) >> { >> if ( subleaf > ARRAY_SIZE(p->xstate.raw) ) >> return; >> } >> if ( leaf > p->basic.max_leaf ) >> return; >> >> Which (by making the last one if rather than else-if) also fixes an >> issue I've spotted only now: So far you exclude leaves 7 and 0xd >> from the basic.max_leaf checking. (And this way that check could >> also go first.) > > Very good point, although I still think I'd still prefer a logic block > in this form inside a case 0 ... 0x3fffffff to avoid potential leakage > if other logic changes. Well, that's certainly fine with me. Jan
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c index 01bb906..d140482 100644 --- a/xen/arch/x86/cpuid.c +++ b/xen/arch/x86/cpuid.c @@ -279,6 +279,10 @@ void recalculate_cpuid_policy(struct domain *d) uint32_t fs[FSCAPINTS], max_fs[FSCAPINTS]; unsigned int i; + p->basic.max_leaf = min(p->basic.max_leaf, max->basic.max_leaf); + p->feat.max_subleaf = min(p->feat.max_subleaf, max->feat.max_subleaf); + p->extd.max_leaf = min(p->extd.max_leaf, max->extd.max_leaf); + cpuid_policy_to_featureset(p, fs); cpuid_policy_to_featureset(max, max_fs); @@ -306,6 +310,9 @@ void recalculate_cpuid_policy(struct domain *d) if ( !d->disable_migrate && !d->arch.vtsc ) __clear_bit(X86_FEATURE_ITSC, fs); + if ( p->basic.max_leaf < 0xd ) + __clear_bit(X86_FEATURE_XSAVE, fs); + /* Fold host's FDP_EXCP_ONLY and NO_FPU_SEL into guest's view. */ fs[FEATURESET_7b0] &= ~special_features[FEATURESET_7b0]; fs[FEATURESET_7b0] |= (host_policy.feat._7b0 & @@ -333,21 +340,50 @@ void guest_cpuid(const struct vcpu *v, unsigned int leaf, unsigned int subleaf, struct cpuid_leaf *res) { const struct domain *d = v->domain; + const struct cpuid_policy *p = d->arch.cpuid; *res = EMPTY_LEAF; /* * First pass: * - Dispatch the virtualised leaves to their respective handlers. + * - Perform max_leaf/subleaf calculations, maybe returning early. */ switch ( leaf ) { + case 0x0 ... 0x6: + case 0x8 ... 0xc: +#if 0 /* For when CPUID_GUEST_NR_BASIC isn't 0xd */ + case 0xe ... CPUID_GUEST_NR_BASIC - 1: +#endif + if ( leaf > p->basic.max_leaf ) + return; + break; + + case 0x7: + if ( subleaf > p->feat.max_subleaf ) + return; + break; + + case 0xd: + if ( subleaf > ARRAY_SIZE(p->xstate.raw) ) + return; + break; + case 0x40000000 ... 0x400000ff: if ( is_viridian_domain(d) ) return cpuid_viridian_leaves(v, leaf, subleaf, res); /* Fallthrough. */ case 0x40000100 ... 0x4fffffff: return cpuid_hypervisor_leaves(v, leaf, subleaf, res); + + case 0x80000000 ... 0x80000000 + CPUID_GUEST_NR_EXTD - 1: + if ( leaf > p->extd.max_leaf ) + return; + break; + + default: + return; } /* {pv,hvm}_cpuid() have this expectation. */ diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 7cda53f..1dd92e3 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3305,27 +3305,6 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, if ( !edx ) edx = &dummy; - if ( input & 0x7fffffff ) - { - /* - * Requests outside the supported leaf ranges return zero on AMD - * and the highest basic leaf output on Intel. Uniformly follow - * the AMD model as the more sane one. - */ - unsigned int limit; - - domain_cpuid(d, (input >> 16) != 0x8000 ? 0 : 0x80000000, 0, - &limit, &dummy, &dummy, &dummy); - if ( input > limit ) - { - *eax = 0; - *ebx = 0; - *ecx = 0; - *edx = 0; - return; - } - } - domain_cpuid(d, input, count, eax, ebx, ecx, edx); switch ( input ) diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 1c384cf..aed96c3 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -1032,29 +1032,6 @@ void pv_cpuid(struct cpu_user_regs *regs) subleaf = c = regs->_ecx; d = regs->_edx; - if ( leaf & 0x7fffffff ) - { - /* - * Requests outside the supported leaf ranges return zero on AMD - * and the highest basic leaf output on Intel. Uniformly follow - * the AMD model as the more sane one. - */ - unsigned int limit = (leaf >> 16) != 0x8000 ? 0 : 0x80000000, dummy; - - if ( !is_control_domain(currd) && !is_hardware_domain(currd) ) - domain_cpuid(currd, limit, 0, &limit, &dummy, &dummy, &dummy); - else - limit = cpuid_eax(limit); - if ( leaf > limit ) - { - regs->rax = 0; - regs->rbx = 0; - regs->rcx = 0; - regs->rdx = 0; - return; - } - } - if ( !is_control_domain(currd) && !is_hardware_domain(currd) ) domain_cpuid(currd, leaf, subleaf, &a, &b, &c, &d); else diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h index c621a6f..7363263 100644 --- a/xen/include/asm-x86/cpuid.h +++ b/xen/include/asm-x86/cpuid.h @@ -87,6 +87,7 @@ struct cpuid_policy * Per-domain objects: * * - Guest accurate: + * - max_{,sub}leaf * - All FEATURESET_* words * * Everything else should be considered inaccurate, and not necesserily 0.
Clamp the toolstack-providied max_leaf values in recalculate_cpuid_policy(), causing the per-domain policy to have guest-accurate data. Have guest_cpuid() exit early if a requested leaf is out of range, rather than falling into the legacy path. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> --- xen/arch/x86/cpuid.c | 36 ++++++++++++++++++++++++++++++++++++ xen/arch/x86/hvm/hvm.c | 21 --------------------- xen/arch/x86/traps.c | 23 ----------------------- xen/include/asm-x86/cpuid.h | 1 + 4 files changed, 37 insertions(+), 44 deletions(-)