Re: [PATCH v4 12/15] tools/xenpm: Print CPPC parameters for amd-cppc driver

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.05.2025 08:36, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Wednesday, April 30, 2025 9:55 PM
>>
>> On 14.04.2025 09:40, Penny Zheng wrote:
>>> HWP, amd-cppc, amd-cppc-epp are all the implementation of ACPI CPPC
>>> (Collaborative Processor Performace Control), so we introduce
>>> cppc_mode flag to print CPPC-related para.
>>>
>>> And HWP and amd-cppc-epp are both governor-less driver, so we
>>> introduce hw_auto flag to bypass governor-related print.
>>
>> But in the EPP driver you use the information which governor is active.
> 
> We want to have a one-one mapping between governor and epp value, such as,
> If users choose performance governor, no matter via "xenpm" or cmdline, users 
> want maximum performance,
> We set epp with 0 to meet the expectation.
> And if users choose powersave governor, users want the least power 
> consumption, then we shall set
> epp with 255 to meet the expectation.

That's all fine, but completely misses the point of my question: If the
governor is relevant, why would you bypass respective printing?

> Ondemand is a tricky part, hmmmm, I don't know which value is suitable for 
> it, the medium one? So I neglect it in the first place

Medium one may be okay-ish, but it's not really an appropriate fit. We may
want to at least consider rejecting the use of ondemand with the EPP driver.
That, however, heavily depends on how hardware would behave when using the
medium value.

>>> --- a/tools/misc/xenpm.c
>>> +++ b/tools/misc/xenpm.c
>>> @@ -790,9 +790,18 @@ static unsigned int
>>> calculate_activity_window(const xc_cppc_para_t *cppc,
>>>  /* print out parameters about cpu frequency */  static void
>>> print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)
>>> {
>>> -    bool hwp = strcmp(p_cpufreq->scaling_driver, XEN_HWP_DRIVER_NAME)
>> == 0;
>>> +    bool cppc_mode = false, hw_auto = false;
>>>      int i;
>>>
>>> +    if ( !strcmp(p_cpufreq->scaling_driver, XEN_HWP_DRIVER_NAME) ||
>>> +         !strcmp(p_cpufreq->scaling_driver, XEN_AMD_CPPC_DRIVER_NAME) ||
>>> +         !strcmp(p_cpufreq->scaling_driver,
>> XEN_AMD_CPPC_EPP_DRIVER_NAME) )
>>> +        cppc_mode = true;
>>> +
>>> +    if ( !strcmp(p_cpufreq->scaling_driver, XEN_HWP_DRIVER_NAME) ||
>>> +         !strcmp(p_cpufreq->scaling_driver,
>> XEN_AMD_CPPC_EPP_DRIVER_NAME) )
>>> +        hw_auto = true;
>>
>> Please avoid doing the same strcmp()s twice. There are several ways how to, 
>> so
>> I'm not going to make a particular suggestion.
> 
> Maybe we shall use switch-case() to replace the same strcmp()s
> Since it's not easy to switch-case() string value, I had a draft idea to 
> include an new entry in "struct xen_cppc_para",
> See:
> ```
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index fa431fd983..b872f1b66a 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -308,6 +308,10 @@ struct xen_ondemand {
> 
>  struct xen_cppc_para {
>      /* OUT */
> +#define XEN_SYSCTL_CPPC_VENDOR_HWP      1
> +#define XEN_SYSCTL_CPPC_VENDOR_AMD      2
> +#define XEN_SYSCTL_CPPC_VENDOR_AMD_EPP  3
> +    uint8_t vendor;
>      /* activity_window supported if set */
>  #define XEN_SYSCTL_CPPC_FEAT_ACT_WINDOW  (1 << 0)
>      uint32_t features; /* bit flags for features */
> 
> ```
> A new vendor filed in struct xen_cppc_para could help us differ the 
> underlying implementation.
> Or any better suggestions?

Well, if you set hw_auto first, then you can use that variable plus one
more strcmp() to set cppc_mode.

Jan