| Message ID | 1487588434-4359-5-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 20.02.17 at 12:00, <andrew.cooper3@citrix.com> wrote: > --- a/xen/arch/x86/cpuid.c > +++ b/xen/arch/x86/cpuid.c > @@ -163,6 +163,9 @@ static void recalculate_xstate(struct cpuid_policy *p) > */ > static void recalculate_misc(struct cpuid_policy *p) > { > + /* Leaves with subleaf unions. */ > + p->basic.raw[0x4] = p->basic.raw[0x7] = p->basic.raw[0xd] = EMPTY_LEAF; How come you play with leaves 7 and 0xd here? > @@ -244,6 +248,25 @@ static void __init calculate_raw_policy(void) > cpuid_leaf(i, &p->basic.raw[i]); > } > > + if ( p->basic.max_leaf >= 4 ) > + { > + for ( i = 0; i < ARRAY_SIZE(p->cache.raw); ++i ) > + { > + cpuid_count_leaf(4, i, &p->cache.raw[i]); > + > + if ( p->cache.subleaf[i].type == 0 ) > + break; > + } > + > + /* > + * The choice of CPUID_GUEST_NR_CACHE is arbitrary. It is expected > + * that it will eventually need increasing for future hardware. > + */ > + if ( i == ARRAY_SIZE(p->cache.raw) ) > + printk(XENLOG_WARNING > + "CPUID: Insufficient Leaf 4 space for this hardware\n"); > + } It probably doesn't hurt, but it's one off: There's no enough space only when the next (i-th) doesn't report type 0. > @@ -125,6 +126,15 @@ struct cpuid_policy > }; > } basic; > > + /* Structured cache leaf: 0x00000004[xx] */ > + union { > + struct cpuid_leaf raw[CPUID_GUEST_NR_CACHE]; > + struct { > + uint32_t type:4, According to the SDM version I'm looking at this is a 5 bit field. Jan
On 21/02/17 17:16, Jan Beulich wrote: >>>> On 20.02.17 at 12:00, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/arch/x86/cpuid.c >> +++ b/xen/arch/x86/cpuid.c >> @@ -163,6 +163,9 @@ static void recalculate_xstate(struct cpuid_policy *p) >> */ >> static void recalculate_misc(struct cpuid_policy *p) >> { >> + /* Leaves with subleaf unions. */ >> + p->basic.raw[0x4] = p->basic.raw[0x7] = p->basic.raw[0xd] = EMPTY_LEAF; > How come you play with leaves 7 and 0xd here? This particular piece of clobbering was something which has only just occurred to me now when implementing the leaf 4 union. Then again, there is no supported way of getting any values into those particular rows, or reading out of them, so I could just rely on no-one caring? > >> @@ -244,6 +248,25 @@ static void __init calculate_raw_policy(void) >> cpuid_leaf(i, &p->basic.raw[i]); >> } >> >> + if ( p->basic.max_leaf >= 4 ) >> + { >> + for ( i = 0; i < ARRAY_SIZE(p->cache.raw); ++i ) >> + { >> + cpuid_count_leaf(4, i, &p->cache.raw[i]); >> + >> + if ( p->cache.subleaf[i].type == 0 ) >> + break; >> + } >> + >> + /* >> + * The choice of CPUID_GUEST_NR_CACHE is arbitrary. It is expected >> + * that it will eventually need increasing for future hardware. >> + */ >> + if ( i == ARRAY_SIZE(p->cache.raw) ) >> + printk(XENLOG_WARNING >> + "CPUID: Insufficient Leaf 4 space for this hardware\n"); >> + } > It probably doesn't hurt, but it's one off: There's no enough space > only when the next (i-th) doesn't report type 0. This bit of logic is slightly awkward. We read into p->cache.raw[i] before looking to see whether p->cache.subleaf[i].type is the end of the list. As such we always read one-past-the-end. > >> @@ -125,6 +126,15 @@ struct cpuid_policy >> }; >> } basic; >> >> + /* Structured cache leaf: 0x00000004[xx] */ >> + union { >> + struct cpuid_leaf raw[CPUID_GUEST_NR_CACHE]; >> + struct { >> + uint32_t type:4, > According to the SDM version I'm looking at this is a 5 bit field. Right you are. I'd got confused by the "Bits 04 - 00". Will fix. ~Andrew
>>> On 21.02.17 at 18:35, <andrew.cooper3@citrix.com> wrote: > On 21/02/17 17:16, Jan Beulich wrote: >>>>> On 20.02.17 at 12:00, <andrew.cooper3@citrix.com> wrote: >>> --- a/xen/arch/x86/cpuid.c >>> +++ b/xen/arch/x86/cpuid.c >>> @@ -163,6 +163,9 @@ static void recalculate_xstate(struct cpuid_policy *p) >>> */ >>> static void recalculate_misc(struct cpuid_policy *p) >>> { >>> + /* Leaves with subleaf unions. */ >>> + p->basic.raw[0x4] = p->basic.raw[0x7] = p->basic.raw[0xd] = EMPTY_LEAF; >> How come you play with leaves 7 and 0xd here? > > This particular piece of clobbering was something which has only just > occurred to me now when implementing the leaf 4 union. > > Then again, there is no supported way of getting any values into those > particular rows, or reading out of them, so I could just rely on no-one > caring? Well, if they start out as EMPTY_LEAF and there's no way to get other values into them, why bother filling them here? The more with a line that doesn't allow neatly extending should one more such leaf need adding here. I'd say if you want to clobber the values here just in case, merge the assignments above (in numeric order) with the ones that are already there just below (visible in the original patch context). >>> @@ -244,6 +248,25 @@ static void __init calculate_raw_policy(void) >>> cpuid_leaf(i, &p->basic.raw[i]); >>> } >>> >>> + if ( p->basic.max_leaf >= 4 ) >>> + { >>> + for ( i = 0; i < ARRAY_SIZE(p->cache.raw); ++i ) >>> + { >>> + cpuid_count_leaf(4, i, &p->cache.raw[i]); >>> + >>> + if ( p->cache.subleaf[i].type == 0 ) >>> + break; >>> + } >>> + >>> + /* >>> + * The choice of CPUID_GUEST_NR_CACHE is arbitrary. It is expected >>> + * that it will eventually need increasing for future hardware. >>> + */ >>> + if ( i == ARRAY_SIZE(p->cache.raw) ) >>> + printk(XENLOG_WARNING >>> + "CPUID: Insufficient Leaf 4 space for this hardware\n"); >>> + } >> It probably doesn't hurt, but it's one off: There's no enough space >> only when the next (i-th) doesn't report type 0. > > This bit of logic is slightly awkward. We read into p->cache.raw[i] > before looking to see whether p->cache.subleaf[i].type is the end of the > list. As such we always read one-past-the-end. Sure. Issuing the message prematurely could of course be avoided nevertheless, by reading sub-leaf i (regardless of whether i == CPUID_GUEST_NR_CACHE) into a local variable and checking type there. But as said, it's not something I strictly ask for to be done, as I can also see upsides of seeing this warning earlier than absolutely needed. Jan
On 22/02/17 07:23, Jan Beulich wrote: >>>> On 21.02.17 at 18:35, <andrew.cooper3@citrix.com> wrote: >> On 21/02/17 17:16, Jan Beulich wrote: >>>>>> On 20.02.17 at 12:00, <andrew.cooper3@citrix.com> wrote: >>>> --- a/xen/arch/x86/cpuid.c >>>> +++ b/xen/arch/x86/cpuid.c >>>> @@ -163,6 +163,9 @@ static void recalculate_xstate(struct cpuid_policy *p) >>>> */ >>>> static void recalculate_misc(struct cpuid_policy *p) >>>> { >>>> + /* Leaves with subleaf unions. */ >>>> + p->basic.raw[0x4] = p->basic.raw[0x7] = p->basic.raw[0xd] = EMPTY_LEAF; >>> How come you play with leaves 7 and 0xd here? >> This particular piece of clobbering was something which has only just >> occurred to me now when implementing the leaf 4 union. >> >> Then again, there is no supported way of getting any values into those >> particular rows, or reading out of them, so I could just rely on no-one >> caring? > Well, if they start out as EMPTY_LEAF and there's no way to get > other values into them, why bother filling them here? The more > with a line that doesn't allow neatly extending should one more > such leaf need adding here. I'd say if you want to clobber the > values here just in case, merge the assignments above (in > numeric order) with the ones that are already there just below > (visible in the original patch context). I will just drop the clobbering. Even this bit of logic isn't going to survive to the end of eventual toolstack changes. > >>>> @@ -244,6 +248,25 @@ static void __init calculate_raw_policy(void) >>>> cpuid_leaf(i, &p->basic.raw[i]); >>>> } >>>> >>>> + if ( p->basic.max_leaf >= 4 ) >>>> + { >>>> + for ( i = 0; i < ARRAY_SIZE(p->cache.raw); ++i ) >>>> + { >>>> + cpuid_count_leaf(4, i, &p->cache.raw[i]); >>>> + >>>> + if ( p->cache.subleaf[i].type == 0 ) >>>> + break; >>>> + } >>>> + >>>> + /* >>>> + * The choice of CPUID_GUEST_NR_CACHE is arbitrary. It is expected >>>> + * that it will eventually need increasing for future hardware. >>>> + */ >>>> + if ( i == ARRAY_SIZE(p->cache.raw) ) >>>> + printk(XENLOG_WARNING >>>> + "CPUID: Insufficient Leaf 4 space for this hardware\n"); >>>> + } >>> It probably doesn't hurt, but it's one off: There's no enough space >>> only when the next (i-th) doesn't report type 0. >> This bit of logic is slightly awkward. We read into p->cache.raw[i] >> before looking to see whether p->cache.subleaf[i].type is the end of the >> list. As such we always read one-past-the-end. > Sure. Issuing the message prematurely could of course be avoided > nevertheless, by reading sub-leaf i (regardless of whether i == > CPUID_GUEST_NR_CACHE) into a local variable and checking type > there. But as said, it's not something I strictly ask for to be done, > as I can also see upsides of seeing this warning earlier than > absolutely needed. Ok. I will leave it as-is. ~Andrew
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c index 3ecb794..ca38d04 100644 --- a/xen/arch/x86/cpuid.c +++ b/xen/arch/x86/cpuid.c @@ -163,6 +163,9 @@ static void recalculate_xstate(struct cpuid_policy *p) */ static void recalculate_misc(struct cpuid_policy *p) { + /* Leaves with subleaf unions. */ + p->basic.raw[0x4] = p->basic.raw[0x7] = p->basic.raw[0xd] = EMPTY_LEAF; + p->basic.raw[0x8] = EMPTY_LEAF; p->basic.raw[0xc] = EMPTY_LEAF; @@ -201,6 +204,7 @@ static void recalculate_misc(struct cpuid_policy *p) p->basic.apic_id = 0; /* Dynamic. */ zero_leaves(p->basic.raw, 0x2, 0x3); + memset(p->cache.raw, 0, sizeof(p->cache.raw)); p->basic.raw[0x9] = EMPTY_LEAF; p->extd.vendor_ebx = p->basic.vendor_ebx; @@ -244,6 +248,25 @@ static void __init calculate_raw_policy(void) cpuid_leaf(i, &p->basic.raw[i]); } + if ( p->basic.max_leaf >= 4 ) + { + for ( i = 0; i < ARRAY_SIZE(p->cache.raw); ++i ) + { + cpuid_count_leaf(4, i, &p->cache.raw[i]); + + if ( p->cache.subleaf[i].type == 0 ) + break; + } + + /* + * The choice of CPUID_GUEST_NR_CACHE is arbitrary. It is expected + * that it will eventually need increasing for future hardware. + */ + if ( i == ARRAY_SIZE(p->cache.raw) ) + printk(XENLOG_WARNING + "CPUID: Insufficient Leaf 4 space for this hardware\n"); + } + if ( p->basic.max_leaf >= 7 ) { cpuid_count_leaf(7, 0, &p->feat.raw[0]); @@ -522,6 +545,23 @@ void recalculate_cpuid_policy(struct domain *d) recalculate_xstate(p); recalculate_misc(p); + for ( i = 0; i < ARRAY_SIZE(p->cache.raw); ++i ) + { + if ( p->cache.subleaf[i].type >= 1 && + p->cache.subleaf[i].type <= 3 ) + { + /* Subleaf has a valid cache type. Zero reserved fields. */ + p->cache.raw[i].a &= 0xffffc3ffu; + p->cache.raw[i].d &= 0x00000007u; + } + else + { + /* Subleaf is not valid. Zero the rest of the union. */ + zero_leaves(p->cache.raw, i, ARRAY_SIZE(p->cache.raw) - 1); + break; + } + } + if ( !p->extd.svm ) p->extd.raw[0xa] = EMPTY_LEAF; @@ -607,7 +647,7 @@ static void pv_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res) *res = EMPTY_LEAF; break; - case 0x0 ... 0x3: + case 0x0 ... 0x4: case 0x7 ... 0x9: case 0xc ... XSTATE_CPUID: case 0x80000000 ... 0xffffffff: @@ -642,7 +682,7 @@ static void hvm_cpuid(uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *res) res->a = (res->a & ~0xff) | 3; break; - case 0x0 ... 0x3: + case 0x0 ... 0x4: case 0x7 ... 0x9: case 0xc ... XSTATE_CPUID: case 0x80000000 ... 0xffffffff: @@ -676,6 +716,13 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf, switch ( leaf ) { + case 0x4: + if ( subleaf >= ARRAY_SIZE(p->cache.raw) ) + return; + + *res = p->cache.raw[subleaf]; + break; + case 0x7: ASSERT(p->feat.max_subleaf < ARRAY_SIZE(p->feat.raw)); if ( subleaf > min_t(uint32_t, p->feat.max_subleaf, diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index fc42cb1..aa0d914 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -101,6 +101,10 @@ static int update_domain_cpuid_info(struct domain *d, switch ( ctl->input[0] ) { case 0x00000000 ... ARRAY_SIZE(p->basic.raw) - 1: + if ( ctl->input[0] == 4 && + ctl->input[1] >= ARRAY_SIZE(p->cache.raw) ) + return 0; + if ( ctl->input[0] == 7 && ctl->input[1] >= ARRAY_SIZE(p->feat.raw) ) return 0; @@ -129,7 +133,9 @@ static int update_domain_cpuid_info(struct domain *d, switch ( ctl->input[0] ) { case 0x00000000 ... ARRAY_SIZE(p->basic.raw) - 1: - if ( ctl->input[0] == 7 ) + if ( ctl->input[0] == 4 ) + p->cache.raw[ctl->input[1]] = leaf; + else if ( ctl->input[0] == 7 ) p->feat.raw[ctl->input[1]] = leaf; else if ( ctl->input[0] == XSTATE_CPUID ) p->xstate.raw[ctl->input[1]] = leaf; diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h index 6d1990b..14fc95c 100644 --- a/xen/include/asm-x86/cpuid.h +++ b/xen/include/asm-x86/cpuid.h @@ -63,6 +63,7 @@ DECLARE_PER_CPU(bool, cpuid_faulting_enabled); #define CPUID_GUEST_NR_BASIC (0xdu + 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_EXTD_INTEL (0x8u + 1) #define CPUID_GUEST_NR_EXTD_AMD (0x1cu + 1) @@ -125,6 +126,15 @@ struct cpuid_policy }; } basic; + /* Structured cache leaf: 0x00000004[xx] */ + union { + struct cpuid_leaf raw[CPUID_GUEST_NR_CACHE]; + struct { + uint32_t type:4, + :28, :32, :32, :32; + } subleaf[CPUID_GUEST_NR_CACHE]; + } cache; + /* Structured feature leaf: 0x00000007[xx] */ union { struct cpuid_leaf raw[CPUID_GUEST_NR_FEAT];
Leaf 0x4 is reserved by AMD. For Intel, it is a multi-invocation leaf with ecx enumerating different cache details. Add a new union for it in struct cpuid_policy, collect it from hardware in calculate_raw_policy(), audit it in recalculate_cpuid_policy() and update guest_cpuid() and update_domain_cpuid_info() to properly insert/extract data. A lot of the data here will need further auditing/refinement when better topology support is introduced, but for now, this matches the existing toolstack behaviour. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> --- xen/arch/x86/cpuid.c | 51 +++++++++++++++++++++++++++++++++++++++++++-- xen/arch/x86/domctl.c | 8 ++++++- xen/include/asm-x86/cpuid.h | 10 +++++++++ 3 files changed, 66 insertions(+), 3 deletions(-)