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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.11.2020 11:32, Andrew Cooper wrote:
> On 10/11/2020 08:03, Jan Beulich wrote:
>> On 09.11.2020 18:38, Andrew Cooper wrote:
>>> --- 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,

Not the most obvious place for it to live at, but
xen/include/acpi/cpufreq/cpufreq.h ?

Jan