Re: [PATCH v2] x86/msr: fix handling of MSR_IA32_PERF_{STATUS/CTL}

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 10/11/2020 08:03, Jan Beulich wrote:
> On 09.11.2020 18:38, Andrew Cooper wrote:
>> From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>
>> Currently a PV hardware domain can also be given control over the CPU
>> frequency, and such guest is allowed to write to MSR_IA32_PERF_CTL.
>> However since commit 322ec7c89f6 the default behavior has been changed
>> to reject accesses to not explicitly handled MSRs, preventing PV
>> guests that manage CPU frequency from reading
>> MSR_IA32_PERF_{STATUS/CTL}.
>>
>> Additionally some HVM guests (Windows at least) will attempt to read
>> MSR_IA32_PERF_CTL and will panic if given back a #GP fault:
>>
>>   vmx.c:3035:d8v0 RDMSR 0x00000199 unimplemented
>>   d8v0 VIRIDIAN CRASH: 3b c0000096 fffff806871c1651 ffffda0253683720 0
>>
>> Move the handling of MSR_IA32_PERF_{STATUS/CTL} to the common MSR
>> handling shared between HVM and PV guests, and add an explicit case
>> for reads to MSR_IA32_PERF_{STATUS/CTL}.
>>
>> Restore previous behavior and allow PV guests with the required
>> permissions to read the contents of the mentioned MSRs. Non privileged
>> guests will get 0 when trying to read those registers, as writes to
>> MSR_IA32_PERF_CTL by such guest will already be silently dropped.
>>
>> Fixes: 322ec7c89f6 ('x86/pv: disallow access to unknown MSRs')
>> Fixes: 84e848fd7a1 ('x86/hvm: disallow access to unknown MSRs')
>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks,

> with a nit, a minor adjustment request, and a question:
>
>> @@ -448,6 +467,21 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t 
>> val)
>>              goto gp_fault;
>>          break;
>>  
>> +        /*
>> +         * This MSR are not enumerated in CPUID.  It has been around since 
>> the
> s/are/is/

Oops, yes.

>
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -1069,6 +1069,23 @@ extern enum cpufreq_controller {
>>      FREQCTL_none, FREQCTL_dom0_kernel, FREQCTL_xen
>>  } cpufreq_controller;
>>  
>> +static always_inline bool is_cpufreq_controller(const struct domain *d)
>> +{
>> +    /*
>> +     * A PV dom0 can be nominated as the cpufreq controller, instead of 
>> using
>> +     * Xen's cpufreq driver, at which point dom0 gets direct access to 
>> certain
>> +     * MSRs.
>> +     *
>> +     * This interface only works when dom0 is identity pinned and has the 
>> same
>> +     * number of vCPUs as pCPUs on the system.
>> +     *
>> +     * It would be far better to paravirtualise the interface.
>> +     */
>> +    return (IS_ENABLED(CONFIG_PV) &&
>> +            (cpufreq_controller == FREQCTL_dom0_kernel) &&
>> +            is_pv_domain(d) && is_hardware_domain(d));
>> +}
> IS_ENABLED(CONFIG_PV) is already part of is_pv_domain() and hence
> imo shouldn't be used explicitly here.

Ah yes.  Will drop.

> Also, this being an x86 concept, is it really a good idea to put
> in xen/sched.h? I guess this builds on Arm just because of DCE
> from the IS_ENABLED(CONFIG_PV) (where afaict the one in
> is_pv_domain() will still do). (But yes, I do realize that
> cpufreq_controller itself gets declared in this file, so maybe
> better to do some subsequent cleanup.)

I can't spot anywhere obviously better for it to live.  We don't have a
common cpufreq.h, and I'm not sure that cpuidle.h is an appropriate
place to live either.

Thanks,

~Andrew