Re: [PATCH for-4.20 v2] x86/intel: Fix PERF_GLOBAL fixup when virtualised

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.01.2025 18:07, Andrew Cooper wrote:
> On 21/01/2025 5:04 pm, Jan Beulich wrote:
>> On 21.01.2025 17:56, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/cpu/intel.c
>>> +++ b/xen/arch/x86/cpu/intel.c
>>> @@ -535,39 +535,49 @@ static void intel_log_freq(const struct cpuinfo_x86 
>>> *c)
>>>      printk("%u MHz\n", (factor * max_ratio + 50) / 100);
>>>  }
>>>  
>>> +static void init_intel_perf(struct cpuinfo_x86 *c)
>>> +{
>>> +    uint64_t val;
>>> +    unsigned int eax, ver, nr_cnt;
>>> +
>>> +    if ( c->cpuid_level <= 9 ||
>>> +         ({  rdmsrl(MSR_IA32_MISC_ENABLE, val);
>> Just curious (not an objection or anything): Is there a reason you have
>> two padding blanks here instead of just one?
> 
> Alignment with the next line.

Yet that's then merely a matter of removing a blank from that line too,
isn't it?

>> (Really we may want to gain
>> a function-like form to invoke RDMSR, but that's orthogonal to the change
>> here.)
> 
> Indeed.
> 
> * def0701b5373 - (xen-nj-msr) switch rdmsrl => rdmsr (30 hours ago)
> <Andrew Cooper>
> * 1a3f92abccf1 - rdmsr (30 hours ago) <Andrew Cooper>
> * 01c9ec7d9482 - rdmsr_safe (30 hours ago) <Andrew Cooper>
> * 7ec72a0379b2 - fix error printing in write_msr() (30 hours ago)
> <Andrew Cooper>
> * 3ff3d60835a5 - drop wrmsrl (30 hours ago) <Andrew Cooper>
> * 136128799b4a - wrmsr cleanup (30 hours ago) <Andrew Cooper>
> * b2ed78d2e7e3 - x86/msr: Move MSR_FEATURE_CONTROL handling to the new
> MSR infrastructure (30 hours ago) <Andrew Cooper>
> * 7691edea3d67 - x86/msr: Clean up the MSR_DEBUGCTL constants (30 hours
> ago) <Andrew Cooper>
> * 77ba2827a955 - x86/msr: Clean up the MSR_MISC_ENABLE constants (30
> hours ago) <Andrew Cooper>
> * 7f2768cfc4b3 - ---upstream--- (30 hours ago) <Andrew Cooper>
> * 8b2e048fdd14 - x86/msr: Introduce msr_{set,clear}_bits() helpers (30
> hours ago) <Andrew Cooper>
> * 562f88503342 - x86/msr: Clean up the MSR_FEATURE_CONTROL constants (30
> hours ago) <Andrew Cooper>
> * 199888c9e2f8 - x86/msr: Clean up the
> MSR_{PLATFORM_INFO,MISC_FEATURES_ENABLES} constants (30 hours ago)
> <Andrew Cooper>
> * c3f5d1bb40b5 - (tag: 4.20.0-rc2, xenbits/staging, xenbits/master,
> upstream/staging, upstream/master, origin/staging, origin/master,
> origin/HEAD, staging, pending, master) automation/cirrus-ci: introduce
> FreeBSD randconfig builds (4 days ago) <Roger Pau Monne>
> 
> That was work I did while sat in an airport unable to leave XenSummit in
> Nanjing...
> 
> It's blocked on arguments over naming.

Is it? I couldn't find any replies of mine in my outbox (and a quick
attempt to search by subjects on the web also didn't reveal anything),
but then Nanjing also was quite some time ago.

I fear you will not like this, but from a more general perspective:
When you say "blocked" for your own patches, it's usually the case that
the ball is in your court. Interestingly other people's patches (say
Roger's or mine) are, when they are "blocked", also often lacking
feedback from you. While it's apparently close to impossible to get
you to reply on such threads rooting in other people's patches,
shouldn't it at least be entirely in your own interest to keep the ball
rolling when it comes to your own ones? That is, make an attempt (or
perhaps repeated ones) to come to some form of agreement?

Plus for anything you deem blocked, you know where the list of pending
x86 patches is that you could add yours to. Since in that table we
record last posting dates, finding the patches is then much easier as
well.

Jan