Re: [PATCH v2 01/16] x86/msr: Implement rdmsr_safe() in C

[Jan Beulich <jbeulich@xxxxxxxx>]

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.cooper3@xxxxxxxxxx>
>>> In principle
>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>> 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