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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.01.2024 17:24, Andrew Cooper wrote:
> On 24/01/2024 3:27 pm, Jan Beulich wrote:
>> ... if available only, of course.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -406,9 +406,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);
>> +    }
>> +
> 
> I know you're re-using an existing condition, but I think it's more
> likely that it's Intel-only than common to VIA and Shanghai.

Then again when re-using the condition I questioned how likely it is
that people actually use Xen on CPUs of these two vendors, when the
respective code is only bit-rotting.

> Also, why is gated on verbose?
> 
> (I think I can see why this is rhetorical question, and I expect you can
> guess what the feedback will be.)

Hmm, no, I don't think I can guess that. The reason is simple: In
case the MSR doesn't exist, I'd like to avoid the respective (debug)
log message, emitted while recovering from the #GP, appearing twice.
(Which imo eliminates the only guess I might otherwise have: Don't
add complexity [the extra part of the condition] when it's not
needed.)

>> --- a/xen/arch/x86/nmi.c
>> +++ b/xen/arch/x86/nmi.c
>> @@ -589,9 +589,20 @@ static void cf_check do_nmi_stats(unsign
>>      unsigned int cpu;
>>      bool pend, mask;
>>  
>> -    printk("CPU\tNMI\n");
>> +    printk("CPU\tNMI%s\n", boot_cpu_has(X86_FEATURE_SMI_COUNT) ? "\tSMI" : 
>> "");
>>      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) )
>> +        {
>> +            unsigned int smi_count, dummy;
>> +
>> +            rdmsr(MSR_SMI_COUNT, smi_count, dummy);
>> +            printk("\t%3u\n", smi_count);
> 
> This reads MSR_SMI_COUNT repeatedly on the same CPU.
> 
> You'll need to IPI all CPUs to dump the count into a per-cpu variable.

Oh, how embarrassing.

Jan