Re: [PATCH v6 06/15] cpufreq: Add Hardware P-State (HWP) driver

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.07.2023 21:49, Jason Andryuk wrote:
> On Mon, Jul 24, 2023 at 12:15 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 24.07.2023 14:58, Jason Andryuk wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
>>> @@ -0,0 +1,521 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * hwp.c cpufreq driver to run Intel Hardware P-States (HWP)
>>> + *
>>> + * Copyright (C) 2021 Jason Andryuk <jandryuk@xxxxxxxxx>
>>> + */
>>> +
>>> +#include <xen/cpumask.h>
>>> +#include <xen/init.h>
>>> +#include <xen/param.h>
>>> +#include <xen/xmalloc.h>
>>> +#include <asm/msr.h>
>>> +#include <acpi/cpufreq/cpufreq.h>
>>> +
>>> +static bool __ro_after_init feature_hwp_notification;
>>> +static bool __ro_after_init feature_hwp_activity_window;
>>> +
>>> +static bool __ro_after_init feature_hdc;
>>> +
>>> +static bool __ro_after_init opt_cpufreq_hdc = true;
>>> +
>>> +union hwp_request
>>> +{
>>> +    struct
>>> +    {
>>> +        unsigned int min_perf:8;
>>> +        unsigned int max_perf:8;
>>> +        unsigned int desired:8;
>>> +        unsigned int energy_perf:8;
>>> +        unsigned int activity_window:10;
>>> +        bool package_control:1;
>>> +        unsigned int :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;
>>> +    };
>>> +    uint64_t raw;
>>> +};
>>> +
>>> +struct hwp_drv_data
>>> +{
>>> +    union
>>> +    {
>>> +        uint64_t hwp_caps;
>>> +        struct
>>> +        {
>>> +            unsigned int highest:8;
>>> +            unsigned int guaranteed:8;
>>> +            unsigned int most_efficient:8;
>>> +            unsigned int lowest:8;
>>> +            unsigned int :32;
>>> +        } hw;
>>> +    };
>>> +    union hwp_request curr_req;
>>> +    int ret;
>>> +    uint16_t activity_window;
>>> +    uint8_t minimum;
>>> +    uint8_t maximum;
>>> +    uint8_t desired;
>>> +    uint8_t energy_perf;
>>> +};
>>> +static DEFINE_PER_CPU_READ_MOSTLY(struct hwp_drv_data *, hwp_drv_data);
>>> +
>>> +#define hwp_err(cpu, fmt, ...) \
>>> +    printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, ##__VA_ARGS__)
>>> +#define hwp_info(fmt, ...)    printk(XENLOG_INFO "HWP: " fmt, 
>>> ##__VA_ARGS__)
>>
>> I'm not convinced ", ##__VA_ARGS__" is a good construct to use. I notice
>> we have a few instances (mainly in code inherited from elsewhere), but I
>> think it ought to either be plain C99 style (without the ##) or purely
>> the gcc extension form (not using __VA_ARGS__).
> 
> Using plain __VA_ARGS__ doesn't work for the cases without arguments:
> arch/x86/acpi/cpufreq/hwp.c:78:53: error: expected expression before ‘)’ token
>    78 |         printk(XENLOG_DEBUG "HWP: " fmt, __VA_ARGS__);  \
>       |                                                     ^
> arch/x86/acpi/cpufreq/hwp.c:201:9: note: in expansion of macro ‘hwp_verbose’
>   201 |         hwp_verbose("disabled: No energy/performance
> preference available");
>       |         ^~~~~~~~~~~

Of course.

> I can use "__VA_OPT__(,) __VA_ARGS__" though.

__VA_OPT__ is yet newer than C99, so this is an option only if all
compilers we continue to support actually know of this. We have no
uses of it in the codebase so far, which suggests you might best go
with the longstanding gcc extension here.

>>> +static int cf_check hwp_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>> +{
>>> +    static union hwp_request initial_req;
>>> +    unsigned int cpu = policy->cpu;
>>> +    struct hwp_drv_data *data;
>>> +    bool first_run = false;
>>> +
>>> +    data = xzalloc(struct hwp_drv_data);
>>> +    if ( !data )
>>> +        return -ENOMEM;
>>> +
>>> +    policy->governor = &cpufreq_gov_hwp;
>>> +
>>> +    per_cpu(hwp_drv_data, cpu) = data;
>>> +
>>> +    on_selected_cpus(cpumask_of(cpu), hwp_init_msrs, policy, 1);
>>> +
>>> +    if ( data->curr_req.raw == -1 )
>>> +    {
>>> +        hwp_err(cpu, "Could not initialize HWP properly\n");
>>> +        per_cpu(hwp_drv_data, cpu) = NULL;
>>> +        xfree(data);
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    data->minimum = data->curr_req.min_perf;
>>> +    data->maximum = data->curr_req.max_perf;
>>> +    data->desired = data->curr_req.desired;
>>> +    data->energy_perf = data->curr_req.energy_perf;
>>> +    data->activity_window = data->curr_req.activity_window;
>>> +
>>> +    if ( initial_req.raw == 0 )
>>> +    {
>>> +        hwp_verbose("CPU%u: HWP_CAPABILITIES: %016lx\n", cpu, 
>>> data->hwp_caps);
>>> +        initial_req = data->curr_req;
>>> +        first_run = true;
>>> +    }
>>
>> What part of data->curr_req is guaranteed to be non-0 (for the condition
>> around ...
>>
>>> +    if ( first_run || data->curr_req.raw != initial_req.raw )
>>> +        hwp_verbose("CPU%u: rdmsr HWP_REQUEST %016lx\n", cpu,
>>> +                    data->curr_req.raw);
>>
>> ... this logging to be effective)?
> 
> Hmmm.  I think you are correct that there is no guarantee that
> data->curr_req will be non-zero.  i.e. a BIOS could zero the whole
> register.  In practice, I see 0x8000ff01 - energy_perf = 0x80, maximum
> = 0xff and minimum = 0x01.
> 
> Would you prefer that I make first_run a static variable to track if
> initial_req has been populated?

Well, I wouldn't call it "prefer", but I see no easy alternative.

Jan