Re: [PATCH 3/3] x86/msr: Fix Solaris and turbostat following XSA-351

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/03/2021 08:32, Roger Pau Monné wrote:
> On Tue, Mar 16, 2021 at 09:20:10PM +0000, Andrew Cooper wrote:
>> On 16/03/2021 16:56, Roger Pau Monné wrote:
>>> On Tue, Mar 16, 2021 at 04:18:44PM +0000, Andrew Cooper wrote:
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> Thanks!
>>>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>> CC: Wei Liu <wl@xxxxxxx>
>>>> CC: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
>>>> CC: Ian Jackson <iwj@xxxxxxxxxxxxxx>
>>>>
>>>> For 4.15 This wants backporting to all security trees, as it is a fix to a
>>>> regression introduced in XSA-351.
>>>>
>>>> Also it means that users don't need msr_relaxed=1 to unbreak Solaris 
>>>> guests,
>>>> which is a strict useability improvement.
>>>> ---
>>>>  xen/arch/x86/msr.c | 13 ++++++++++++-
>>>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
>>>> index 5927b6811b..a83a1d7fba 100644
>>>> --- a/xen/arch/x86/msr.c
>>>> +++ b/xen/arch/x86/msr.c
>>>> @@ -188,7 +188,6 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t 
>>>> *val)
>>>>      case MSR_TSX_CTRL:
>>>>      case MSR_MCU_OPT_CTRL:
>>>>      case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
>>>> -    case MSR_RAPL_POWER_UNIT:
>>>>      case MSR_PKG_POWER_LIMIT  ... MSR_PKG_POWER_INFO:
>>>>      case MSR_DRAM_POWER_LIMIT ... MSR_DRAM_POWER_INFO:
>>>>      case MSR_PP0_POWER_LIMIT  ... MSR_PP0_POLICY:
>>>> @@ -284,6 +283,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, 
>>>> uint64_t *val)
>>>>              goto gp_fault;
>>>>          break;
>>>>  
>>>> +    case MSR_RAPL_POWER_UNIT:
>>>> +        /*
>>>> +         * This MSR is non-architectural.  However, some versions of 
>>>> Solaris
>>>> +         * blindly reads it without a #GP guard, and some versions of
>>>> +         * turbostat crash after expecting a read of /proc/cpu/0/msr not 
>>>> to
>>>> +         * fail.  Read as zero on Intel hardware.
>>>> +         */
>>>> +        if ( !(cp->x86_vendor & X86_VENDOR_INTEL) )
>>>> +            goto gp_fault;
>>> AFAICT from Linux usage this is Intel specific (not present in any of
>>> the clones).
>> Indeed.
>>
>>>> +        *val = 0;
>>>> +        break;
>>> Do we also need to care about MSR_AMD_RAPL_POWER_UNIT (0xc0010299) for
>>> Solaris?
>> AMD has a CPUID bit for this interface, 0x80000007.EDX[14].
> Right, I see now on the manual. I guess I was confused because I don't
> seem to see Linux checking this CPUID bit in:
>
> https://elixir.bootlin.com/linux/latest/source/arch/x86/events/rapl.c#L773
>
> And instead it seems to attach the RAPL driver based on CPU model
> information. That's fine on Linux because accesses are using the _safe
> variants.

Borislav also wants a bugfix for that, seeing as there is a CPUID bit.

>
> The patch looks good to me, I wonder whether you should move the
> "users don't need msr_relaxed=1..." to the commit log, but that might
> be weird if the patch is backported, because it won't make sense for
> any older Xen version.

Will do.

> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

~Andrew