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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

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.

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

Thanks.