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.coop...@citrix.com> > > Thanks! > > > >> --- > >> CC: Jan Beulich <jbeul...@suse.com> > >> CC: Roger Pau Monné <roger....@citrix.com> > >> CC: Wei Liu <w...@xen.org> > >> CC: Boris Ostrovsky <boris.ostrov...@oracle.com> > >> CC: Ian Jackson <i...@xenproject.org> > >> > >> 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....@citrix.com> Thanks.
