On 04.05.2023 18:56, Jason Andryuk wrote:
> On Thu, May 4, 2023 at 9:11 AM Jan Beulich <jbeul...@suse.com> wrote:
>> On 01.05.2023 21:30, Jason Andryuk wrote:
>>> --- a/docs/misc/xen-command-line.pandoc
>>> +++ b/docs/misc/xen-command-line.pandoc
>>> @@ -499,7 +499,7 @@ If set, force use of the performance counters for
>>> oprofile, rather than detectin
>>> available support.
>>>
>>> ### cpufreq
>>> -> `= none | {{ <boolean> | xen }
>>> [:[powersave|performance|ondemand|userspace][,<maxfreq>][,[<minfreq>][,[verbose]]]]}
>>> | dom0-kernel`
>>> +> `= none | {{ <boolean> | xen }
>>> [:[powersave|performance|ondemand|userspace][,<hdc>][,[<hwp>]][,[<maxfreq>]][,[<minfreq>]][,[verbose]]]}
>>> | dom0-kernel`
>>
>> Considering you use a special internal governor, the 4 governor alternatives
>> are
>> meaningless for hwp. Hence at the command line level recognizing "hwp" as if
>> it
>> was another governor name would seem better to me. This would then also get
>> rid
>> of one of the two special "no-" prefix parsing cases (which I'm not overly
>> happy about).
>>
>> Even if not done that way I'm puzzled by the way you spell out the
>> interaction
>> of "hwp" and "hdc": As you say in the description, "hdc" is meaningful only
>> when
>> "hwp" was specified, so even if not merged with the governors group "hwp"
>> should
>> come first, and "hdc" ought to be rejected if "hwp" wasn't first specified.
>> (The
>> way you've spelled it out it actually looks to be kind of the other way
>> around.)
>
> I placed them in alphabetical order, but, yes, it doesn't make sense.
>
>> Strictly speaking "maxfreq" and "minfreq" also should be objected to when
>> "hwp"
>> was specified.
>>
>> Overall I'm getting the impression that beyond your "verbose" related
>> adjustment
>> more is needed, if you're meaning to get things closer to how we parse the
>> option (splitting across multiple lines to help see what I mean):
>>
>> `= none
>> | {{ <boolean> | xen } [:{powersave|performance|ondemand|userspace}
>>
>> [{,hwp[,hdc]|[,maxfreq=<maxfreq>[,minfreq=<minfreq>]}]
>> [,verbose]]}
>> | dom0-kernel`
>>
>> (We're still parsing in a more relaxed way, e.g. minfreq may come ahead of
>> maxfreq, but better be more tight in the doc than too relaxed.)
>>
>> Furthermore while max/min freq don't apply directly, there are still two MSRs
>> controlling bounds at the package and logical processor levels.
>
> Well, we only program the logical processor level MSRs because we
> don't have a good idea of the packages to know when we can skip
> writing an MSR.
>
> How about this:
> `= none
> | {{ <boolean> | xen } {
> [:{powersave|performance|ondemand|userspace}[,maxfreq=<maxfreq>[,minfreq=<minfreq>]]
> | [:hwp[,hdc]] }
> [,verbose]]}
> | dom0-kernel`
Looks right, yes.
>>> --- /dev/null
>>> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
>>> @@ -0,0 +1,506 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * hwp.c cpufreq driver to run Intel Hardware P-States (HWP)
>>> + *
>>> + * Copyright (C) 2021 Jason Andryuk <jandr...@gmail.com>
>>> + */
>>> +
>>> +#include <xen/cpumask.h>
>>> +#include <xen/init.h>
>>> +#include <xen/param.h>
>>> +#include <xen/xmalloc.h>
>>> +#include <asm/io.h>
>>> +#include <asm/msr.h>
>>> +#include <acpi/cpufreq/cpufreq.h>
>>> +
>>> +static bool feature_hwp;
>>> +static bool feature_hwp_notification;
>>> +static bool feature_hwp_activity_window;
>>> +static bool feature_hwp_energy_perf;
>>> +static bool feature_hwp_pkg_level_ctl;
>>> +static bool feature_hwp_peci;
>>> +
>>> +static bool feature_hdc;
>>
>> Most (all?) of these want to be __ro_after_init, I expect.
>
> I think you are correct. (This pre-dates __ro_after_init and I didn't
> update it.)
Yet even then they should have used __read_mostly.
>>> +union hwp_request
>>> +{
>>> + struct
>>> + {
>>> + uint64_t min_perf:8;
>>> + uint64_t max_perf:8;
>>> + uint64_t desired:8;
>>> + uint64_t energy_perf:8;
>>> + uint64_t activity_window:10;
>>> + uint64_t package_control:1;
>>> + uint64_t reserved:16;
>>> + uint64_t activity_window_valid:1;
>>> + uint64_t energy_perf_valid:1;
>>> + uint64_t desired_valid:1;
>>> + uint64_t max_perf_valid:1;
>>> + uint64_t min_perf_valid:1;
>>
>> The boolean fields here would probably better be of type "bool". I also
>> don't see the need for using uint64_t for any of the other fields -
>> unsigned int will be quite fine, I think. Only ...
>
> This is the hardware MSR format, so it seemed natural to use uint64_t
> and the bit fields. To me, uint64_t foo:$bits; better shows that we
> are dividing up a single hardware register using bit fields.
> Honestly, I'm unfamiliar with the finer points of laying out bitfields
> with bool. And the 10 bits of activity window throws off aligning to
> standard types.
>
> This seems to have the correct layout:
> struct
> {
> unsigned char min_perf;
> unsigned char max_perf;
> unsigned char desired;
> unsigned char energy_perf;
> unsigned int activity_window:10;
> bool package_control:1;
> unsigned int reserved:16;
> bool activity_window_valid:1;
> bool energy_perf_valid:1;
> bool desired_valid:1;
> bool max_perf_valid:1;
> bool min_perf_valid:1;
> } ;
>
> Or would you prefer the first 8 bit ones to be unsigned int
> min_perf:8?
Personally I think using bitfields uniformly would be better. What you
definitely cannot use if not using a bitfield is "unsigned char", it
ought to by uint8_t then. If using a bitfield, as said, I think it's
best to stick to unsigned int and bool, unless field width goes
beyond 32 bits or fields cross a 32-bit boundary.
> The bools seem to need :1, which doesn't seem to be
> gaining us much, IMO. I'd strongly prefer just keeping it as I have
> it, but I will change it however you like.
It's not so much how I like it, but to follow (a) existing practice
(for the boolean fields) and (b) ./CODING_STYLE (for the selection of
types).
>>> + };
>>> + uint64_t raw;
>>
>> ... this wants to keep this type. (Same again below then.)
>
> For "below", do you want:
>
> struct
> {
> unsigned char highest;
> unsigned char guaranteed;
> unsigned char most_efficient;
> unsigned char lowest;
> unsigned int reserved;
> } hw;
> ?
No - it can only be bitfields or fixed-width types here.
>>> +bool __init hwp_available(void)
>>> +{
>>> + unsigned int eax, ecx, unused;
>>> + bool use_hwp;
>>> +
>>> + if ( boot_cpu_data.cpuid_level < CPUID_PM_LEAF )
>>> + {
>>> + hwp_verbose("cpuid_level (%#x) lacks HWP support\n",
>>> + boot_cpu_data.cpuid_level);
>>> + return false;
>>> + }
>>> +
>>> + if ( boot_cpu_data.cpuid_level < 0x16 )
>>> + {
>>> + hwp_info("HWP disabled: cpuid_level %#x < 0x16 lacks CPU freq
>>> info\n",
>>> + boot_cpu_data.cpuid_level);
>>> + return false;
>>> + }
>>> +
>>> + cpuid(CPUID_PM_LEAF, &eax, &unused, &ecx, &unused);
>>> +
>>> + if ( !(eax & CPUID6_EAX_HWP_ENERGY_PERFORMANCE_PREFERENCE) &&
>>> + !(ecx & CPUID6_ECX_IA32_ENERGY_PERF_BIAS) )
>>> + {
>>> + hwp_verbose("HWP disabled: No energy/performance preference
>>> available");
>>> + return false;
>>> + }
>>> +
>>> + feature_hwp = eax & CPUID6_EAX_HWP;
>>> + feature_hwp_notification = eax & CPUID6_EAX_HWP_NOTIFICATION;
>>> + feature_hwp_activity_window = eax & CPUID6_EAX_HWP_ACTIVITY_WINDOW;
>>> + feature_hwp_energy_perf =
>>> + eax & CPUID6_EAX_HWP_ENERGY_PERFORMANCE_PREFERENCE;
>>> + feature_hwp_pkg_level_ctl = eax &
>>> CPUID6_EAX_HWP_PACKAGE_LEVEL_REQUEST;
>>> + feature_hwp_peci = eax & CPUID6_EAX_HWP_PECI;
>>> +
>>> + hwp_verbose("HWP: %d notify: %d act-window: %d energy-perf: %d
>>> pkg-level: %d peci: %d\n",
>>> + feature_hwp, feature_hwp_notification,
>>> + feature_hwp_activity_window, feature_hwp_energy_perf,
>>> + feature_hwp_pkg_level_ctl, feature_hwp_peci);
>>> +
>>> + if ( !feature_hwp )
>>> + return false;
>>> +
>>> + feature_hdc = eax & CPUID6_EAX_HDC;
>>> +
>>> + hwp_verbose("HWP: Hardware Duty Cycling (HDC) %ssupported%s\n",
>>> + feature_hdc ? "" : "not ",
>>> + feature_hdc ? opt_cpufreq_hdc ? ", enabled" : ", disabled"
>>> + : "");
>>> +
>>> + feature_hdc = feature_hdc && opt_cpufreq_hdc;
>>> +
>>> + hwp_verbose("HWP: HW_FEEDBACK %ssupported\n",
>>> + (eax & CPUID6_EAX_HW_FEEDBACK) ? "" : "not ");
>>
>> You report this, but you don't really use it?
>
> Correct. I needed to know what capabilities my processors have.
>
> feature_hwp_pkg_level_ctl and feature_hwp_peci can also be dropped
> since they aren't used beyond printing their values. I'd still lean
> toward keeping their printing under verbose since otherwise there
> isn't a convenient way to know if they are available without
> recompiling.
That's fine, but wants mentioning in the description. Also respective
variables would want to be __initdata then, be local to the function,
or be dropped altogether. Plus you'd want to be consistent - either
you use a helper variable for all print-only features, or you don't.
>>> +static void hwp_get_cpu_speeds(struct cpufreq_policy *policy)
>>> +{
>>> + uint32_t base_khz, max_khz, bus_khz, edx;
>>> +
>>> + cpuid(0x16, &base_khz, &max_khz, &bus_khz, &edx);
>>> +
>>> + /* aperf/mperf scales base. */
>>> + policy->cpuinfo.perf_freq = base_khz * 1000;
>>> + policy->cpuinfo.min_freq = base_khz * 1000;
>>> + policy->cpuinfo.max_freq = max_khz * 1000;
>>> + policy->min = base_khz * 1000;
>>> + policy->max = max_khz * 1000;
>>> + policy->cur = 0;
>>
>> What is the comment intended to be telling me here?
>
> When I was surprised to discover that I needed to pass in the base
> frequency for proper aperf/mperf scaling, it seemed relevant at the
> time as it's the opposite of ACPI cpufreq. It can be dropped now.
Well, I'm not insisting on dropping the comment. It could also be left,
but then extended so it can be understood what is meant.
>>> +static void cf_check hwp_init_msrs(void *info)
>>> +{
>>> + struct cpufreq_policy *policy = info;
>>> + struct hwp_drv_data *data = this_cpu(hwp_drv_data);
>>> + uint64_t val;
>>> +
>>> + /*
>>> + * Package level MSR, but we don't have a good idea of packages here,
>>> so
>>> + * just do it everytime.
>>> + */
>>> + if ( rdmsr_safe(MSR_IA32_PM_ENABLE, val) )
>>> + {
>>> + hwp_err("CPU%u: error rdmsr_safe(MSR_IA32_PM_ENABLE)\n",
>>> policy->cpu);
>>> + data->curr_req.raw = -1;
>>> + return;
>>> + }
>>> +
>>> + /* Ensure we don't generate interrupts */
>>> + if ( feature_hwp_notification )
>>> + wrmsr_safe(MSR_IA32_HWP_INTERRUPT, 0);
>>> +
>>> + hwp_verbose("CPU%u: MSR_IA32_PM_ENABLE: %016lx\n", policy->cpu, val);
>>> + if ( !(val & IA32_PM_ENABLE_HWP_ENABLE) )
>>> + {
>>> + val |= IA32_PM_ENABLE_HWP_ENABLE;
>>> + if ( wrmsr_safe(MSR_IA32_PM_ENABLE, val) )
>>> + {
>>> + hwp_err("CPU%u: error wrmsr_safe(MSR_IA32_PM_ENABLE, %lx)\n",
>>> + policy->cpu, val);
>>> + data->curr_req.raw = -1;
>>> + return;
>>> + }
>>> + }
>>> +
>>> + if ( rdmsr_safe(MSR_IA32_HWP_CAPABILITIES, data->hwp_caps) )
>>> + {
>>> + hwp_err("CPU%u: error rdmsr_safe(MSR_IA32_HWP_CAPABILITIES)\n",
>>> + policy->cpu);
>>> + data->curr_req.raw = -1;
>>> + return;
>>> + }
>>> +
>>> + if ( rdmsr_safe(MSR_IA32_HWP_REQUEST, data->curr_req.raw) )
>>> + {
>>> + hwp_err("CPU%u: error rdmsr_safe(MSR_IA32_HWP_REQUEST)\n",
>>> policy->cpu);
>>> + data->curr_req.raw = -1;
>>> + return;
>>> + }
>>> +
>>> + if ( !feature_hwp_energy_perf ) {
>>
>> Nit: Brace placement.
>>
>>> + if ( rdmsr_safe(MSR_IA32_ENERGY_PERF_BIAS, val) )
>>> + {
>>> + hwp_err("error rdmsr_safe(MSR_IA32_ENERGY_PERF_BIAS)\n");
>>> + data->curr_req.raw = -1;
>>> +
>>> + return;
>>> + }
>>> +
>>> + data->energy_perf = val & IA32_ENERGY_BIAS_MASK;
>>> + }
>>
>> In order to not need to undo the "enable" you've already done, maybe that
>> should move down here?
>
> HWP needs to be enabled before the Capabilities and Request MSRs can
> be read.
I must have missed this aspect in the SDM. Do you have a pointer?
> Reading them shouldn't fail, but it seems safer to use
> rdmsr_safe in case something goes wrong.
Sure. But then the "enable" will need undoing in the unlikely event of
failure.
>>> --- a/xen/arch/x86/include/asm/cpufeature.h
>>> +++ b/xen/arch/x86/include/asm/cpufeature.h
>>> @@ -46,8 +46,17 @@ extern struct cpuinfo_x86 boot_cpu_data;
>>> #define cpu_has(c, bit) test_bit(bit, (c)->x86_capability)
>>> #define boot_cpu_has(bit) test_bit(bit, boot_cpu_data.x86_capability)
>>>
>>> -#define CPUID_PM_LEAF 6
>>> -#define CPUID6_ECX_APERFMPERF_CAPABILITY 0x1
>>> +#define CPUID_PM_LEAF 6
>>> +#define CPUID6_EAX_HWP (_AC(1, U) << 7)
>>> +#define CPUID6_EAX_HWP_NOTIFICATION (_AC(1, U) << 8)
>>> +#define CPUID6_EAX_HWP_ACTIVITY_WINDOW (_AC(1, U) << 9)
>>> +#define CPUID6_EAX_HWP_ENERGY_PERFORMANCE_PREFERENCE (_AC(1, U) << 10)
>>> +#define CPUID6_EAX_HWP_PACKAGE_LEVEL_REQUEST (_AC(1, U) << 11)
>>> +#define CPUID6_EAX_HDC (_AC(1, U) << 13)
>>> +#define CPUID6_EAX_HWP_PECI (_AC(1, U) << 16)
>>> +#define CPUID6_EAX_HW_FEEDBACK (_AC(1, U) << 19)
>>
>> Perhaps better without open-coding BIT()?
>
> Ok.
>
>> I also find it a little odd that e.g. bit 17 is left out here despite you
>> declaring the 5 "valid" bits in union hwp_request (which are qualified by
>> this CPUID bit afaict).
>
> Well, I thought I wasn't supposed to introduce unused defines, so I
> didn't add one for 17. For union hwp_request, the "valid" bits are
> part of the register structure, so it makes sense to include them
> instead of an incomplete definition. IIRC, at some point I set the
> "valid" bits when I wasn't supposed to, and they caused the wrmsr
> calls to fail. That might have been because my test machines don't
> have package-level HWP.
>
> (I was confused when the CPUID section stated "Bit 17: Flexible HWP is
> supported if set.", but there are no further references to "Flexible
> HWP" in the SDM.)
A not uncommon issue with the SDM. At least there is a place where bit
17's purpose is described in the HWP section.
>>> @@ -165,6 +172,11 @@
>>> #define PASID_PASID_MASK 0x000fffff
>>> #define PASID_VALID (_AC(1, ULL) << 31)
>>>
>>> +#define MSR_IA32_PKG_HDC_CTL 0x00000db0
>>> +#define IA32_PKG_HDC_CTL_HDC_PKG_ENABLE (_AC(1, ULL) << 0)
>>
>> The name has two redundant infixes, which looks odd, but then I can't
>> suggest any better without going too much out of sync with the SDM.
>
> Yes, it's not a good name, but I was trying to keep close to the SDM.
> FAOD, these should drop IA32_ to become:
> MSR_PKG_HDC_CTL
> PKG_HDC_CTL_HDC_PKG_ENABLE
> ?
Right.
> Thank you for taking the time to review this.
Well, it has taken me awfully long to get back to this.
Jan
