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.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.
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....@citrix.com> Thanks. ~Andrew
