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
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
>
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.
© 2016 - 2026 Red Hat, Inc.