Re: [PATCH v2] x86: amend 'n' debug-key output with SMI count

[Jan Beulich <jbeulich@xxxxxxxx>]

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