On 19/08/2025 2:28 pm, Andrew Cooper wrote:
> On 18/08/2025 12:23 pm, Jan Beulich wrote:
>> On 15.08.2025 22:41, Andrew Cooper wrote:
>>> ... in preparation to be able to use asm goto.
>>>
>>> Notably this mean that the value parameter must be taken by pointer rather
>>> than by value.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
>> In principle
>> Reviewed-by: Jan Beulich <jbeul...@suse.com>
> Thanks.
>
>> However, having looked at patch 2 first, ...
>>
>>> @@ -879,14 +879,14 @@ static void intel_init_ppin(const struct cpuinfo_x86
>>> *c)
>>> case 0x8f: /* Sapphire Rapids X */
>>>
>>> if ( (c != &boot_cpu_data && !ppin_msr) ||
>>> - rdmsr_safe(MSR_PPIN_CTL, val) )
>>> + rdmsr_safe(MSR_PPIN_CTL, &val) )
>>> return;
>> ... with this, wouldn't we be better off using ...
>>
>>> /* If PPIN is disabled, but not locked, try to enable. */
>>> if ( !(val & (PPIN_ENABLE | PPIN_LOCKOUT)) )
>>> {
>>> wrmsr_safe(MSR_PPIN_CTL, val | PPIN_ENABLE);
>>> - rdmsr_safe(MSR_PPIN_CTL, val);
>>> + rdmsr_safe(MSR_PPIN_CTL, &val);
>> ... plain rdmsr() here, thus not leaving it open to the behavioral change
>> patch 2 comes with?
> Yeah, probably. At the point we've read it once, and written to it, a
> subsequent read is not going fail.
>
> I'll adjust, although it will have to be a rdmsrl().
No. That would introduce a bug, because this path ignores a write error
too.
There isn't actually a problem even with patch 2, because of the how the
logic is currently laid out.
~Andrew
