[Public]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Wednesday, April 30, 2025 9:55 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Huang, Ray <ray.hu...@amd.com>; Anthony PERARD
> <anthony.per...@vates.tech>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v4 12/15] tools/xenpm: Print CPPC parameters for amd-cppc
> driver
>
> 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.
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
I'll add above explanation in commit which introduces
CPUFREQ_POLICY_POWERSAVE/PERFORMANCE
> > --- 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?
> > @@ -800,7 +809,7 @@ static void print_cpufreq_para(int cpuid, struct
> xc_get_cpufreq_para *p_cpufreq)
> > printf(" %d", p_cpufreq->affected_cpus[i]);
> > printf("\n");
> >
> > - if ( hwp )
> > + if ( hw_auto )
> > printf("cpuinfo frequency : base [%"PRIu32"] max [%"PRIu32"]\n",
> > p_cpufreq->cpuinfo_min_freq,
> > p_cpufreq->cpuinfo_max_freq);
> > --- a/xen/drivers/acpi/pmstat.c
> > +++ b/xen/drivers/acpi/pmstat.c
> > @@ -201,7 +201,7 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op
> *op)
> > pmpt = processor_pminfo[op->cpuid];
> > policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
> >
> > - if ( !pmpt || !pmpt->perf.states ||
> > + if ( !pmpt || ((pmpt->init & XEN_PX_INIT) && !pmpt->perf.states)
> > + ||
> > !policy || !policy->governor )
> > return -EINVAL;
>
> This looks questionable all on its own. Where is it that ->perf.states
> allocation is
> being avoided? I first thought it might be patch 06 which is related, but
> that doesn't
> look to be it. In any event further down from here there is
>
->perf.states is allocated in set_px_pminfo()
It is a px-specific function.
> for ( i = 0; i < op->u.get_para.freq_num; i++ )
> data[i] = pmpt->perf.states[i].core_frequency * 1000;
>
> i.e. an access to the array solely based on hypercall input.
>
I'll guard it with pmpt->init & XEN_PX_INIT too
> Both this and ...
>
> > @@ -461,9 +461,10 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
> > switch ( op->cmd & PM_PARA_CATEGORY_MASK )
> > {
> > case CPUFREQ_PARA:
> > - if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_PX) )
> > + if ( !(xen_processor_pmbits & (XEN_PROCESSOR_PM_PX |
> > + XEN_PROCESSOR_PM_CPPC)) )
> > return -ENODEV;
> > - if ( !pmpt || !(pmpt->init & XEN_PX_INIT) )
> > + if ( !pmpt || !(pmpt->init & (XEN_PX_INIT | XEN_CPPC_INIT)) )
> > return -EINVAL;
> > break;
> > }
>
> ... this hunk also look as if they would belong (partly?) in maybe patch 03?
> Even more so as per the title this is solely a tool stack (xenpm) change.
>
True, I shall move them to 03, to let this commit being solely a tool stack
(xenpm) change
> Jan
