On 19.08.2025 15:35, Andrew Cooper wrote:
> 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.
Why would there be a bug? Irrespective of the WRMSR, we know the earlier
RDMSR worked.
> There isn't actually a problem even with patch 2, because of the how the
> logic is currently laid out.
Yes, the logic would still be correct, just less obviously so.
Jan
