| Message ID | 20220513103500.3671-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/cpuid: expose MCDT_NO | expand |
On 13/05/2022 11:35, Roger Pau Monne wrote: > Some CPU models don't exhibit MCDT behavior, but also don't expose > MCDT_NO. Set the MCDT_NO bit for CPUs known to not exhibit the > behavior, so guests can get this information, as using > family/model/stepping detection when running virtualized is not to be > relied. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/arch/x86/cpu/intel.c | 70 ++++++++++++++++++++++++++++++++++++++++ > xen/arch/x86/cpuid.c | 10 ++++++ > 2 files changed, 80 insertions(+) > > diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c > index dc6a0c7807..d821f460ae 100644 > --- a/xen/arch/x86/cpu/intel.c > +++ b/xen/arch/x86/cpu/intel.c > @@ -518,6 +518,73 @@ static void intel_log_freq(const struct cpuinfo_x86 *c) > printk("%u MHz\n", (factor * max_ratio + 50) / 100); > } > > +void update_mcdt_no(struct cpuinfo_x86 *c) > +{ > +#define FAM6_MODEL(m, s, c) { 6, m, s, c } > + /* > + * List of models that do not exhibit MCDT behavior, but might not > + * advertise MCDT_NO on CPUID. > + */ > + static const struct { > + uint8_t family; > + uint8_t model; > + uint8_t stepping; > + bool check_stepping; > + } mcdt_no[] = { > + /* Haswell Server EP, EP4S. */ > + FAM6_MODEL(0x3f, 2, true), > + /* Elkhart Lake. */ > + FAM6_MODEL(0x3f, 4, true), > + /* Cherryview. */ > + FAM6_MODEL(0x4c, 0, false), > + /* Broadwell Server E, EP, EP4S, EX. */ > + FAM6_MODEL(0x4f, 0, false), > + /* Broadwell DE V2, V3. */ > + FAM6_MODEL(0x56, 3, true), > + /* Broadwell DE Y0. */ > + FAM6_MODEL(0x56, 4, true), > + /* Broadwell DE A1, Hewitt Lake. */ > + FAM6_MODEL(0x56, 5, true), > + /* Anniedale. */ > + FAM6_MODEL(0x5a, 0, false), > + /* Apollo Lake. */ > + FAM6_MODEL(0x5c, 9, true), > + FAM6_MODEL(0x5c, 0xa, true), > + /* Denverton. */ > + FAM6_MODEL(0x5f, 1, true), > + /* XMM7272. */ > + FAM6_MODEL(0x65, 0, false), > + /* Cougar Mountain. */ > + FAM6_MODEL(0x6e, 0, false), > + /* Butter. */ > + FAM6_MODEL(0x75, 0, false), > + /* Gemini Lake. */ > + FAM6_MODEL(0x7a, 1, true), > + FAM6_MODEL(0x7a, 8, true), > + /* Snowridge. */ > + FAM6_MODEL(0x86, 4, true), > + FAM6_MODEL(0x86, 5, true), > + FAM6_MODEL(0x86, 7, true), > + /* Lakefield B-step. */ > + FAM6_MODEL(0x8a, 1, true), > + /* Elkhart Lake. */ > + FAM6_MODEL(0x96, 1, true), > + /* Jasper Lake. */ > + FAM6_MODEL(0x9c, 0, true), > + { } > + }; > +#undef FAM6_MODEL > + const typeof(mcdt_no[0]) *m; > + > + for (m = mcdt_no; m->family | m->model | m->stepping; m++) > + if ( c->x86 == m->family && c->x86_model == m->model && > + (!m->check_stepping || c->x86_mask == m->stepping) ) > + { > + __set_bit(X86_FEATURE_MCDT_NO, c->x86_capability); > + break; > + } > +} Please could we see about using x86_match_cpu() rather than basically opencoding it? Linux's bug.c has some fairly comprehensive examples of how to do tables like this with it. Also, can we use our shiny new intel-family.h names? The stepping checks guidance seems suspect. Lemme ping some people about that. I suspect that means "we checked the production CPUs but didn't look at the pre-prod hardware" which in practice means we don't care about steppings listed. ~Andrew
On Fri, May 13, 2022 at 11:18:47AM +0000, Andrew Cooper wrote: > On 13/05/2022 11:35, Roger Pau Monne wrote: > > Some CPU models don't exhibit MCDT behavior, but also don't expose > > MCDT_NO. Set the MCDT_NO bit for CPUs known to not exhibit the > > behavior, so guests can get this information, as using > > family/model/stepping detection when running virtualized is not to be > > relied. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > xen/arch/x86/cpu/intel.c | 70 ++++++++++++++++++++++++++++++++++++++++ > > xen/arch/x86/cpuid.c | 10 ++++++ > > 2 files changed, 80 insertions(+) > > > > diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c > > index dc6a0c7807..d821f460ae 100644 > > --- a/xen/arch/x86/cpu/intel.c > > +++ b/xen/arch/x86/cpu/intel.c > > @@ -518,6 +518,73 @@ static void intel_log_freq(const struct cpuinfo_x86 *c) > > printk("%u MHz\n", (factor * max_ratio + 50) / 100); > > } > > > > +void update_mcdt_no(struct cpuinfo_x86 *c) > > +{ > > +#define FAM6_MODEL(m, s, c) { 6, m, s, c } > > + /* > > + * List of models that do not exhibit MCDT behavior, but might not > > + * advertise MCDT_NO on CPUID. > > + */ > > + static const struct { > > + uint8_t family; > > + uint8_t model; > > + uint8_t stepping; > > + bool check_stepping; > > + } mcdt_no[] = { > > + /* Haswell Server EP, EP4S. */ > > + FAM6_MODEL(0x3f, 2, true), > > + /* Elkhart Lake. */ > > + FAM6_MODEL(0x3f, 4, true), > > + /* Cherryview. */ > > + FAM6_MODEL(0x4c, 0, false), > > + /* Broadwell Server E, EP, EP4S, EX. */ > > + FAM6_MODEL(0x4f, 0, false), > > + /* Broadwell DE V2, V3. */ > > + FAM6_MODEL(0x56, 3, true), > > + /* Broadwell DE Y0. */ > > + FAM6_MODEL(0x56, 4, true), > > + /* Broadwell DE A1, Hewitt Lake. */ > > + FAM6_MODEL(0x56, 5, true), > > + /* Anniedale. */ > > + FAM6_MODEL(0x5a, 0, false), > > + /* Apollo Lake. */ > > + FAM6_MODEL(0x5c, 9, true), > > + FAM6_MODEL(0x5c, 0xa, true), > > + /* Denverton. */ > > + FAM6_MODEL(0x5f, 1, true), > > + /* XMM7272. */ > > + FAM6_MODEL(0x65, 0, false), > > + /* Cougar Mountain. */ > > + FAM6_MODEL(0x6e, 0, false), > > + /* Butter. */ > > + FAM6_MODEL(0x75, 0, false), > > + /* Gemini Lake. */ > > + FAM6_MODEL(0x7a, 1, true), > > + FAM6_MODEL(0x7a, 8, true), > > + /* Snowridge. */ > > + FAM6_MODEL(0x86, 4, true), > > + FAM6_MODEL(0x86, 5, true), > > + FAM6_MODEL(0x86, 7, true), > > + /* Lakefield B-step. */ > > + FAM6_MODEL(0x8a, 1, true), > > + /* Elkhart Lake. */ > > + FAM6_MODEL(0x96, 1, true), > > + /* Jasper Lake. */ > > + FAM6_MODEL(0x9c, 0, true), > > + { } > > + }; > > +#undef FAM6_MODEL > > + const typeof(mcdt_no[0]) *m; > > + > > + for (m = mcdt_no; m->family | m->model | m->stepping; m++) > > + if ( c->x86 == m->family && c->x86_model == m->model && > > + (!m->check_stepping || c->x86_mask == m->stepping) ) > > + { > > + __set_bit(X86_FEATURE_MCDT_NO, c->x86_capability); > > + break; > > + } > > +} > > Please could we see about using x86_match_cpu() rather than basically > opencoding it? Linux's bug.c has some fairly comprehensive examples of > how to do tables like this with it. Yes, I know about x86_match_cpu(). I've used this open-coded form because of the conditional extra checking of the stepping which is not handled by x86_match_cpu(). I didn't feel like extending struct x86_cpu_id and x86_match_cpu() just for this use-case, but I could do it. > Also, can we use our shiny new intel-family.h names? > > The stepping checks guidance seems suspect. Lemme ping some people > about that. I suspect that means "we checked the production CPUs but > didn't look at the pre-prod hardware" which in practice means we don't > care about steppings listed. OK, that would help quite a lot, as then I could just use plain x86_match_cpu(). Thanks, Roger.
Hi, It seems that this patch has been stale for nearly 2 months, with the first patch in series already merged and some discussions between maintainers and author in the Patch #2 thread. So I am sending this email as a gentle reminder. Kind regards, Henry > -----Original Message----- > Subject: [PATCH 2/2] x86/cpuid: set MCDT_NO for non-affected models > > Some CPU models don't exhibit MCDT behavior, but also don't expose > MCDT_NO. Set the MCDT_NO bit for CPUs known to not exhibit the > behavior, so guests can get this information, as using > family/model/stepping detection when running virtualized is not to be > relied. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/arch/x86/cpu/intel.c | 70 > ++++++++++++++++++++++++++++++++++++++++ > xen/arch/x86/cpuid.c | 10 ++++++ > 2 files changed, 80 insertions(+) > > diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c > index dc6a0c7807..d821f460ae 100644 > --- a/xen/arch/x86/cpu/intel.c > +++ b/xen/arch/x86/cpu/intel.c > @@ -518,6 +518,73 @@ static void intel_log_freq(const struct cpuinfo_x86 > *c) > printk("%u MHz\n", (factor * max_ratio + 50) / 100); > } > > +void update_mcdt_no(struct cpuinfo_x86 *c) > +{ > +#define FAM6_MODEL(m, s, c) { 6, m, s, c } > + /* > + * List of models that do not exhibit MCDT behavior, but might not > + * advertise MCDT_NO on CPUID. > + */ > + static const struct { > + uint8_t family; > + uint8_t model; > + uint8_t stepping; > + bool check_stepping; > + } mcdt_no[] = { > + /* Haswell Server EP, EP4S. */ > + FAM6_MODEL(0x3f, 2, true), > + /* Elkhart Lake. */ > + FAM6_MODEL(0x3f, 4, true), > + /* Cherryview. */ > + FAM6_MODEL(0x4c, 0, false), > + /* Broadwell Server E, EP, EP4S, EX. */ > + FAM6_MODEL(0x4f, 0, false), > + /* Broadwell DE V2, V3. */ > + FAM6_MODEL(0x56, 3, true), > + /* Broadwell DE Y0. */ > + FAM6_MODEL(0x56, 4, true), > + /* Broadwell DE A1, Hewitt Lake. */ > + FAM6_MODEL(0x56, 5, true), > + /* Anniedale. */ > + FAM6_MODEL(0x5a, 0, false), > + /* Apollo Lake. */ > + FAM6_MODEL(0x5c, 9, true), > + FAM6_MODEL(0x5c, 0xa, true), > + /* Denverton. */ > + FAM6_MODEL(0x5f, 1, true), > + /* XMM7272. */ > + FAM6_MODEL(0x65, 0, false), > + /* Cougar Mountain. */ > + FAM6_MODEL(0x6e, 0, false), > + /* Butter. */ > + FAM6_MODEL(0x75, 0, false), > + /* Gemini Lake. */ > + FAM6_MODEL(0x7a, 1, true), > + FAM6_MODEL(0x7a, 8, true), > + /* Snowridge. */ > + FAM6_MODEL(0x86, 4, true), > + FAM6_MODEL(0x86, 5, true), > + FAM6_MODEL(0x86, 7, true), > + /* Lakefield B-step. */ > + FAM6_MODEL(0x8a, 1, true), > + /* Elkhart Lake. */ > + FAM6_MODEL(0x96, 1, true), > + /* Jasper Lake. */ > + FAM6_MODEL(0x9c, 0, true), > + { } > + }; > +#undef FAM6_MODEL > + const typeof(mcdt_no[0]) *m; > + > + for (m = mcdt_no; m->family | m->model | m->stepping; m++) > + if ( c->x86 == m->family && c->x86_model == m->model && > + (!m->check_stepping || c->x86_mask == m->stepping) ) > + { > + __set_bit(X86_FEATURE_MCDT_NO, c->x86_capability); > + break; > + } > +} > + > static void cf_check init_intel(struct cpuinfo_x86 *c) > { > /* Detect the extended topology information if available */ > @@ -556,6 +623,9 @@ static void cf_check init_intel(struct cpuinfo_x86 *c) > if ((opt_cpu_info && !(c->apicid & (c->x86_num_siblings - 1))) || > c == &boot_cpu_data ) > intel_log_freq(c); > + > + if (!cpu_has(c, X86_FEATURE_MCDT_NO)) > + update_mcdt_no(c); > } > > const struct cpu_dev intel_cpu_dev = { > diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c > index 66be1a8015..ca2ed44149 100644 > --- a/xen/arch/x86/cpuid.c > +++ b/xen/arch/x86/cpuid.c > @@ -360,6 +360,16 @@ static void __init calculate_host_policy(void) > > p->basic.max_leaf = > min_t(uint32_t, p->basic.max_leaf, ARRAY_SIZE(p->basic.raw) - 1); > + > + /* > + * For Intel hardware MCDT_NO might be set by Xen for models that don't > + * exhibit MCDT behavior but also don't have the MCDT_NO bit set in > + * CPUID. Extend feat.max_subleaf beyond what hardware supports to > include > + * the feature leaf containing this information. > + */ > + if ( boot_cpu_has(X86_FEATURE_MCDT_NO) ) > + p->feat.max_subleaf = max(p->feat.max_subleaf, 2u); > + > p->feat.max_subleaf = > min_t(uint32_t, p->feat.max_subleaf, ARRAY_SIZE(p->feat.raw) - 1); > > -- > 2.36.0 >
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c index dc6a0c7807..d821f460ae 100644 --- a/xen/arch/x86/cpu/intel.c +++ b/xen/arch/x86/cpu/intel.c @@ -518,6 +518,73 @@ static void intel_log_freq(const struct cpuinfo_x86 *c) printk("%u MHz\n", (factor * max_ratio + 50) / 100); } +void update_mcdt_no(struct cpuinfo_x86 *c) +{ +#define FAM6_MODEL(m, s, c) { 6, m, s, c } + /* + * List of models that do not exhibit MCDT behavior, but might not + * advertise MCDT_NO on CPUID. + */ + static const struct { + uint8_t family; + uint8_t model; + uint8_t stepping; + bool check_stepping; + } mcdt_no[] = { + /* Haswell Server EP, EP4S. */ + FAM6_MODEL(0x3f, 2, true), + /* Elkhart Lake. */ + FAM6_MODEL(0x3f, 4, true), + /* Cherryview. */ + FAM6_MODEL(0x4c, 0, false), + /* Broadwell Server E, EP, EP4S, EX. */ + FAM6_MODEL(0x4f, 0, false), + /* Broadwell DE V2, V3. */ + FAM6_MODEL(0x56, 3, true), + /* Broadwell DE Y0. */ + FAM6_MODEL(0x56, 4, true), + /* Broadwell DE A1, Hewitt Lake. */ + FAM6_MODEL(0x56, 5, true), + /* Anniedale. */ + FAM6_MODEL(0x5a, 0, false), + /* Apollo Lake. */ + FAM6_MODEL(0x5c, 9, true), + FAM6_MODEL(0x5c, 0xa, true), + /* Denverton. */ + FAM6_MODEL(0x5f, 1, true), + /* XMM7272. */ + FAM6_MODEL(0x65, 0, false), + /* Cougar Mountain. */ + FAM6_MODEL(0x6e, 0, false), + /* Butter. */ + FAM6_MODEL(0x75, 0, false), + /* Gemini Lake. */ + FAM6_MODEL(0x7a, 1, true), + FAM6_MODEL(0x7a, 8, true), + /* Snowridge. */ + FAM6_MODEL(0x86, 4, true), + FAM6_MODEL(0x86, 5, true), + FAM6_MODEL(0x86, 7, true), + /* Lakefield B-step. */ + FAM6_MODEL(0x8a, 1, true), + /* Elkhart Lake. */ + FAM6_MODEL(0x96, 1, true), + /* Jasper Lake. */ + FAM6_MODEL(0x9c, 0, true), + { } + }; +#undef FAM6_MODEL + const typeof(mcdt_no[0]) *m; + + for (m = mcdt_no; m->family | m->model | m->stepping; m++) + if ( c->x86 == m->family && c->x86_model == m->model && + (!m->check_stepping || c->x86_mask == m->stepping) ) + { + __set_bit(X86_FEATURE_MCDT_NO, c->x86_capability); + break; + } +} + static void cf_check init_intel(struct cpuinfo_x86 *c) { /* Detect the extended topology information if available */ @@ -556,6 +623,9 @@ static void cf_check init_intel(struct cpuinfo_x86 *c) if ((opt_cpu_info && !(c->apicid & (c->x86_num_siblings - 1))) || c == &boot_cpu_data ) intel_log_freq(c); + + if (!cpu_has(c, X86_FEATURE_MCDT_NO)) + update_mcdt_no(c); } const struct cpu_dev intel_cpu_dev = { diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c index 66be1a8015..ca2ed44149 100644 --- a/xen/arch/x86/cpuid.c +++ b/xen/arch/x86/cpuid.c @@ -360,6 +360,16 @@ static void __init calculate_host_policy(void) p->basic.max_leaf = min_t(uint32_t, p->basic.max_leaf, ARRAY_SIZE(p->basic.raw) - 1); + + /* + * For Intel hardware MCDT_NO might be set by Xen for models that don't + * exhibit MCDT behavior but also don't have the MCDT_NO bit set in + * CPUID. Extend feat.max_subleaf beyond what hardware supports to include + * the feature leaf containing this information. + */ + if ( boot_cpu_has(X86_FEATURE_MCDT_NO) ) + p->feat.max_subleaf = max(p->feat.max_subleaf, 2u); + p->feat.max_subleaf = min_t(uint32_t, p->feat.max_subleaf, ARRAY_SIZE(p->feat.raw) - 1);
Some CPU models don't exhibit MCDT behavior, but also don't expose MCDT_NO. Set the MCDT_NO bit for CPUs known to not exhibit the behavior, so guests can get this information, as using family/model/stepping detection when running virtualized is not to be relied. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/cpu/intel.c | 70 ++++++++++++++++++++++++++++++++++++++++ xen/arch/x86/cpuid.c | 10 ++++++ 2 files changed, 80 insertions(+)