On 16.02.2024 10:11, Roger Pau Monné wrote:
> On Wed, Feb 14, 2024 at 11:15:51AM +0100, Jan Beulich wrote:
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -407,9 +407,15 @@ void __init early_cpu_init(bool verbose)
>> paddr_bits -= (ebx >> 6) & 0x3f;
>> }
>>
>> - if (!(c->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)))
>> + if (!(c->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON))) {
>> + uint64_t smi_count;
>> +
>> park_offline_cpus = opt_mce;
>>
>> + if (!verbose && !rdmsr_safe(MSR_SMI_COUNT, smi_count))
>> + setup_force_cpu_cap(X86_FEATURE_SMI_COUNT);
>
> Why make it dependent on !verbose? The call with !verbose is tied to
> part of the ucode loading being half-functional (for example
> MCU_CONTROL_DIS_MCU_LOAD not being set) but I don't see that as a
> signal that SMI count shouldn't be used.
>
> does it need to be part of the early cpu initialization instead of
> being in the (later) Intel specific init code part of the
> identify_cpu()?
Yes, the condition was inverted. It could likely also be dropped
altogether; not sure which one's better: On one hand avoiding multiple
setup_force_cpu_cap() seems desirable (albeit not strictly necessary),
while otoh the code would be simpler without.
>> --- a/xen/arch/x86/nmi.c
>> +++ b/xen/arch/x86/nmi.c
>> @@ -585,15 +585,34 @@ static void cf_check do_nmi_trigger(unsi
>> self_nmi();
>> }
>>
>> +static DEFINE_PER_CPU(unsigned int, smi_count);
>> +
>> +static void cf_check read_smi_count(void *unused)
>> +{
>> + unsigned int dummy;
>> +
>> + rdmsr(MSR_SMI_COUNT, this_cpu(smi_count), dummy);
>> +}
>> +
>> static void cf_check do_nmi_stats(unsigned char key)
>> {
>> const struct vcpu *v;
>> unsigned int cpu;
>> bool pend, mask;
>>
>> - printk("CPU\tNMI\n");
>> + printk("CPU\tNMI%s\n", boot_cpu_has(X86_FEATURE_SMI_COUNT) ? "\tSMI" :
>> "");
>> +
>> + if ( boot_cpu_has(X86_FEATURE_SMI_COUNT) )
>> + on_each_cpu(read_smi_count, NULL, 1);
>> +
>> for_each_online_cpu ( cpu )
>> - printk("%3u\t%3u\n", cpu, per_cpu(nmi_count, cpu));
>> + {
>> + printk("%3u\t%3u", cpu, per_cpu(nmi_count, cpu));
>> + if ( boot_cpu_has(X86_FEATURE_SMI_COUNT) )
>> + printk("\t%3u\n", per_cpu(smi_count, cpu));
>> + else
>> + printk("\n");
>> + }
>>
>> if ( !hardware_domain || !(v = domain_vcpu(hardware_domain, 0)) )
>> return;
>
> Could you also amend the debug-key help text to mention SMI?
Hmm, I had considered that and decided against. I'm uncertain, nevertheless,
so could be talked into amending that help text. Just that I can't make it
"NMI and SMI statistics" as whether SMI data is available is conditional.
Yet "NMI (and maybe SMI) statistics" looks a little clumsy to me ...
Jan
