Re: [PATCH 2/3] x86/msr: Forward port XSA-351 changes from 4.14

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/03/2021 08:52, Roger Pau Monné wrote:
> On Tue, Mar 16, 2021 at 04:18:43PM +0000, Andrew Cooper wrote:
>> staging was not impacted by XSA-351 at the time of release, due to c/s
>> 322ec7c89f and 84e848fd7a which disallows read access by default.
>>
>> Forward port the XSA-351 changes to make the code structure consistent 
>> between
>> 4.14 and 4.15.
>>
>> This removes logspew for guests probing for the RAPL interface.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> 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>
>>
>> Technically this breaks Solaris/turbostat insofar as you can no longer use
>> msr_relaxed to "fix" the guest.  The subsequent patch will unbreak it
>> differently.
>>
>> For 4.15.  Restoring behaviour closer to 4.14, and prereq for a bugfix 
>> needing
>> backporting.
>> ---
>>  xen/arch/x86/msr.c              | 19 +++++++++++++++++++
>>  xen/include/asm-x86/msr-index.h | 39 +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 58 insertions(+)
>>
>> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
>> index c3a988bd11..5927b6811b 100644
>> --- a/xen/arch/x86/msr.c
>> +++ b/xen/arch/x86/msr.c
>> @@ -188,6 +188,13 @@ 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:
>> +    case MSR_PP1_POWER_LIMIT  ... MSR_PP1_POLICY:
>> +    case MSR_PLATFORM_ENERGY_COUNTER:
>> +    case MSR_PLATFORM_POWER_LIMIT:
>>      case MSR_U_CET:
>>      case MSR_S_CET:
>>      case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
>> @@ -195,6 +202,8 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t 
>> *val)
>>      case MSR_AMD64_LWP_CBADDR:
>>      case MSR_PPIN_CTL:
>>      case MSR_PPIN:
>> +    case MSR_F15H_CU_POWER ... MSR_F15H_CU_MAX_POWER:
>> +    case MSR_AMD_RAPL_POWER_UNIT ... MSR_AMD_PKG_ENERGY_STATUS:
>>      case MSR_AMD_PPIN_CTL:
>>      case MSR_AMD_PPIN:
>>          goto gp_fault;
>> @@ -412,6 +421,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t 
>> val)
>>      case MSR_INTEL_CORE_THREAD_COUNT:
>>      case MSR_INTEL_PLATFORM_INFO:
>>      case MSR_ARCH_CAPABILITIES:
>> +    case MSR_IA32_PERF_STATUS:
> Should the MSR_IA32_PERF_STATUS addition maybe be part of the previous
> commit, as it's not related to the XSA-351 content?

It is XSA-351.  We sent out two patches in the end.

c/s 3059178798a (the PERF_STATUS/CTL fix in 4.15) was backported to 4.14
as one half of XSA-351, and gained the above hunk because it went
backwards over the #GP-default change.

In light of patch 1, it now needs reintroducing.

It doesn't really matter if this hunk is in patch 1 or 2, but it needs
to be present, and fits better here IMO.

>
> The rest LGTM:
>
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>
> I wonder whether we could squash this with 3/3 for staging commit, and
> then only backport 3/3 for older branches. But it's likely too much
> work just to prevent breaking msr_relaxed for Solaris for a single
> commit time span.

I did consider the bisectability, but the reality is that it has only
been a week or so with msr_relaxed working at all.

Splitting them apart is simpler to review.

~Andrew