We extract cppc info from "struct xen_get_cpufreq_para", where it acts as
a member of union, and share the space with governor info.
However, it may fail in amd-cppc passive mode, in which governor info and
CPPC info could co-exist, and both need to be printed together via xenpm tool.
If we tried to still put it in "struct xen_get_cpufreq_para" (e.g. just move
out of union), "struct xen_get_cpufreq_para" will enlarge too much to further
make xen_sysctl.u exceed 128 bytes.
So we introduce a new sub-field GET_CPUFREQ_CPPC to dedicatedly acquire
CPPC-related para, and make get-cpufreq-para invoke GET_CPUFREQ_CPPC
if available.
New helpers print_cppc_para() and get_cpufreq_cppc() are introduced to
extract CPPC-related parameters process from cpufreq para.
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v4 -> v5:
- new commit
---
v5 -> v6:
- remove the changes for get-cpufreq-para
---
v6 -> v7:
- make get-cpufreq-para invoke GET_CPUFREQ_CPPC
---
tools/include/xenctrl.h | 3 +-
tools/libs/ctrl/xc_pm.c | 28 ++++++++++++-
tools/misc/xenpm.c | 78 ++++++++++++++++++++++++-------------
xen/drivers/acpi/pm-op.c | 19 +++++++--
xen/include/public/sysctl.h | 3 +-
5 files changed, 98 insertions(+), 33 deletions(-)
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 965d3b585a..e5103453a9 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1938,7 +1938,6 @@ struct xc_get_cpufreq_para {
xc_ondemand_t ondemand;
} u;
} s;
- xc_cppc_para_t cppc_para;
} u;
int32_t turbo_enabled;
@@ -1953,6 +1952,8 @@ int xc_set_cpufreq_para(xc_interface *xch, int cpuid,
int ctrl_type, int ctrl_value);
int xc_set_cpufreq_cppc(xc_interface *xch, int cpuid,
xc_set_cppc_para_t *set_cppc);
+int xc_get_cppc_para(xc_interface *xch, unsigned int cpuid,
+ xc_cppc_para_t *cppc_para);
int xc_get_cpufreq_avgfreq(xc_interface *xch, int cpuid, int *avg_freq);
int xc_set_sched_opt_smt(xc_interface *xch, uint32_t value);
diff --git a/tools/libs/ctrl/xc_pm.c b/tools/libs/ctrl/xc_pm.c
index 6fda973f1f..446ac0b911 100644
--- a/tools/libs/ctrl/xc_pm.c
+++ b/tools/libs/ctrl/xc_pm.c
@@ -288,7 +288,6 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
CHK_FIELD(s.scaling_min_freq);
CHK_FIELD(s.u.userspace);
CHK_FIELD(s.u.ondemand);
- CHK_FIELD(cppc_para);
#undef CHK_FIELD
@@ -366,6 +365,33 @@ int xc_set_cpufreq_cppc(xc_interface *xch, int cpuid,
return ret;
}
+int xc_get_cppc_para(xc_interface *xch, unsigned int cpuid,
+ xc_cppc_para_t *cppc_para)
+{
+ int ret;
+ struct xen_sysctl sysctl = {};
+ struct xen_get_cppc_para *sys_cppc_para = &sysctl.u.pm_op.u.get_cppc;
+
+ if ( !xch || !cppc_para )
+ {
+ errno = EINVAL;
+ return -1;
+ }
+
+ sysctl.cmd = XEN_SYSCTL_pm_op;
+ sysctl.u.pm_op.cmd = GET_CPUFREQ_CPPC;
+ sysctl.u.pm_op.cpuid = cpuid;
+
+ ret = xc_sysctl(xch, &sysctl);
+ if ( ret )
+ return ret;
+
+ BUILD_BUG_ON(sizeof(*cppc_para) != sizeof(*sys_cppc_para));
+ memcpy(cppc_para, sys_cppc_para, sizeof(*sys_cppc_para));
+
+ return ret;
+}
+
int xc_get_cpufreq_avgfreq(xc_interface *xch, int cpuid, int *avg_freq)
{
int ret = 0;
diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
index 6b054b10a4..8fc1d7cc65 100644
--- a/tools/misc/xenpm.c
+++ b/tools/misc/xenpm.c
@@ -801,6 +801,34 @@ static unsigned int calculate_activity_window(const xc_cppc_para_t *cppc,
return mantissa * multiplier;
}
+/* print out parameters about cpu cppc */
+static void print_cppc_para(unsigned int cpuid,
+ const xc_cppc_para_t *cppc)
+{
+ printf("cppc variables :\n");
+ printf(" hardware limits : lowest [%"PRIu32"] lowest nonlinear [%"PRIu32"]\n",
+ cppc->lowest, cppc->lowest_nonlinear);
+ printf(" : nominal [%"PRIu32"] highest [%"PRIu32"]\n",
+ cppc->nominal, cppc->highest);
+ printf(" configured limits : min [%"PRIu32"] max [%"PRIu32"] energy perf [%"PRIu32"]\n",
+ cppc->minimum, cppc->maximum, cppc->energy_perf);
+
+ if ( cppc->features & XEN_SYSCTL_CPPC_FEAT_ACT_WINDOW )
+ {
+ unsigned int activity_window;
+ const char *units;
+
+ activity_window = calculate_activity_window(cppc, &units);
+ printf(" : activity_window [%"PRIu32" %s]\n",
+ activity_window, units);
+ }
+
+ printf(" : desired [%"PRIu32"%s]\n",
+ cppc->desired,
+ cppc->desired ? "" : " hw autonomous");
+ printf("\n");
+}
+
/* print out parameters about cpu frequency */
static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)
{
@@ -826,33 +854,7 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)
printf("scaling_driver : %s\n", p_cpufreq->scaling_driver);
- if ( hwp )
- {
- const xc_cppc_para_t *cppc = &p_cpufreq->u.cppc_para;
-
- printf("cppc variables :\n");
- printf(" hardware limits : lowest [%"PRIu32"] lowest nonlinear [%"PRIu32"]\n",
- cppc->lowest, cppc->lowest_nonlinear);
- printf(" : nominal [%"PRIu32"] highest [%"PRIu32"]\n",
- cppc->nominal, cppc->highest);
- printf(" configured limits : min [%"PRIu32"] max [%"PRIu32"] energy perf [%"PRIu32"]\n",
- cppc->minimum, cppc->maximum, cppc->energy_perf);
-
- if ( cppc->features & XEN_SYSCTL_CPPC_FEAT_ACT_WINDOW )
- {
- unsigned int activity_window;
- const char *units;
-
- activity_window = calculate_activity_window(cppc, &units);
- printf(" : activity_window [%"PRIu32" %s]\n",
- activity_window, units);
- }
-
- printf(" : desired [%"PRIu32"%s]\n",
- cppc->desired,
- cppc->desired ? "" : " hw autonomous");
- }
- else
+ if ( !hwp )
{
if ( p_cpufreq->gov_num )
printf("scaling_avail_gov : %s\n",
@@ -898,6 +900,23 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)
printf("\n");
}
+/* show cpu cppc parameters information on CPU cpuid */
+static int show_cppc_para_by_cpuid(xc_interface *xc_handle, unsigned int cpuid)
+{
+ int ret;
+ xc_cppc_para_t cppc_para;
+
+ ret = xc_get_cppc_para(xc_handle, cpuid, &cppc_para);
+ if ( !ret )
+ print_cppc_para(cpuid, &cppc_para);
+ else if ( errno == ENODEV )
+ ret = 0; /* Ignore unsupported platform */
+ else
+ fprintf(stderr, "[CPU%u] failed to get cppc parameter\n", cpuid);
+
+ return ret;
+}
+
/* show cpu frequency parameters information on CPU cpuid */
static int show_cpufreq_para_by_cpuid(xc_interface *xc_handle, int cpuid)
{
@@ -957,7 +976,12 @@ static int show_cpufreq_para_by_cpuid(xc_interface *xc_handle, int cpuid)
} while ( ret && errno == EAGAIN );
if ( ret == 0 )
+ {
print_cpufreq_para(cpuid, p_cpufreq);
+
+ /* Show CPPC parameters if available */
+ ret = show_cppc_para_by_cpuid(xc_handle, cpuid);
+ }
else if ( errno == ENODEV )
{
ret = -ENODEV;
diff --git a/xen/drivers/acpi/pm-op.c b/xen/drivers/acpi/pm-op.c
index 6991616c1d..bf4638927f 100644
--- a/xen/drivers/acpi/pm-op.c
+++ b/xen/drivers/acpi/pm-op.c
@@ -77,6 +77,17 @@ static int read_scaling_available_governors(char *scaling_available_governors,
return 0;
}
+static int get_cpufreq_cppc(unsigned int cpu,
+ struct xen_get_cppc_para *cppc_para)
+{
+ int ret = -ENODEV;
+
+ if ( hwp_active() )
+ ret = get_hwp_para(cpu, cppc_para);
+
+ return ret;
+}
+
static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
{
uint32_t ret = 0;
@@ -142,9 +153,7 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
else
strlcpy(op->u.get_para.scaling_driver, "Unknown", CPUFREQ_NAME_LEN);
- if ( hwp_active() )
- ret = get_hwp_para(policy->cpu, &op->u.get_para.u.cppc_para);
- else
+ if ( !hwp_active() )
{
if ( !(scaling_available_governors =
xzalloc_array(char, gov_num * CPUFREQ_NAME_LEN)) )
@@ -381,6 +390,10 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
ret = set_cpufreq_para(op);
break;
+ case GET_CPUFREQ_CPPC:
+ ret = get_cpufreq_cppc(op->cpuid, &op->u.get_cppc);
+ break;
+
case SET_CPUFREQ_CPPC:
ret = set_cpufreq_cppc(op);
break;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index eb3a23b038..3f654f98ab 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -492,7 +492,6 @@ struct xen_get_cpufreq_para {
struct xen_ondemand ondemand;
} u;
} s;
- struct xen_get_cppc_para cppc_para;
} u;
int32_t turbo_enabled;
@@ -523,6 +522,7 @@ struct xen_sysctl_pm_op {
#define SET_CPUFREQ_PARA (CPUFREQ_PARA | 0x03)
#define GET_CPUFREQ_AVGFREQ (CPUFREQ_PARA | 0x04)
#define SET_CPUFREQ_CPPC (CPUFREQ_PARA | 0x05)
+ #define GET_CPUFREQ_CPPC (CPUFREQ_PARA | 0x06)
/* set/reset scheduler power saving option */
#define XEN_SYSCTL_pm_op_set_sched_opt_smt 0x21
@@ -547,6 +547,7 @@ struct xen_sysctl_pm_op {
uint32_t cpuid;
union {
struct xen_get_cpufreq_para get_para;
+ struct xen_get_cppc_para get_cppc;
struct xen_set_cpufreq_gov set_gov;
struct xen_set_cpufreq_para set_para;
struct xen_set_cppc_para set_cppc;
--
2.34.1On Fri, Aug 22, 2025 at 06:52:16PM +0800, Penny Zheng wrote:
> diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
> index 6b054b10a4..8fc1d7cc65 100644
> --- a/tools/misc/xenpm.c
> +++ b/tools/misc/xenpm.c
> @@ -898,6 +900,23 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)
> printf("\n");
> }
>
> +/* show cpu cppc parameters information on CPU cpuid */
> +static int show_cppc_para_by_cpuid(xc_interface *xc_handle, unsigned int cpuid)
> +{
> + int ret;
> + xc_cppc_para_t cppc_para;
> +
> + ret = xc_get_cppc_para(xc_handle, cpuid, &cppc_para);
> + if ( !ret )
> + print_cppc_para(cpuid, &cppc_para);
> + else if ( errno == ENODEV )
> + ret = 0; /* Ignore unsupported platform */
> + else
> + fprintf(stderr, "[CPU%u] failed to get cppc parameter\n", cpuid);
You might want to add ": %s" strerror(errno) to the error printed, which
could help figure out why we failed to get the parameters.
The rest of the tool side of the patch, with Jan suggestion, looks good
to me, so Acked-by: Anthony PERARD <anthony.perard@vates.tech> for the
next round.
Thanks,
--
Anthony PERARD
[Public]
> -----Original Message-----
> From: Anthony PERARD <anthony@xenproject.org>
> Sent: Wednesday, August 27, 2025 11:22 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: xen-devel@lists.xenproject.org; Huang, Ray <Ray.Huang@amd.com>; Anthony
> PERARD <anthony.perard@vates.tech>; Juergen Gross <jgross@suse.com>;
> Andrew Cooper <andrew.cooper3@citrix.com>; Orzel, Michal
> <Michal.Orzel@amd.com>; Jan Beulich <jbeulich@suse.com>; Julien Grall
> <julien@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>
> Subject: Re: [PATCH v7 11/13] tools/cpufreq: extract CPPC para from cpufreq para
>
> On Fri, Aug 22, 2025 at 06:52:16PM +0800, Penny Zheng wrote:
> > diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c index
> > 6b054b10a4..8fc1d7cc65 100644
> > --- a/tools/misc/xenpm.c
> > +++ b/tools/misc/xenpm.c
> > @@ -898,6 +900,23 @@ static void print_cpufreq_para(int cpuid, struct
> xc_get_cpufreq_para *p_cpufreq)
> > printf("\n");
> > }
> >
> > +/* show cpu cppc parameters information on CPU cpuid */ static int
> > +show_cppc_para_by_cpuid(xc_interface *xc_handle, unsigned int cpuid)
> > +{
> > + int ret;
> > + xc_cppc_para_t cppc_para;
> > +
> > + ret = xc_get_cppc_para(xc_handle, cpuid, &cppc_para);
> > + if ( !ret )
> > + print_cppc_para(cpuid, &cppc_para);
> > + else if ( errno == ENODEV )
> > + ret = 0; /* Ignore unsupported platform */
> > + else
> > + fprintf(stderr, "[CPU%u] failed to get cppc parameter\n",
> > + cpuid);
>
> You might want to add ": %s" strerror(errno) to the error printed, which could help
> figure out why we failed to get the parameters.
>
Ack
>
> The rest of the tool side of the patch, with Jan suggestion, looks good to me, so
> Acked-by: Anthony PERARD <anthony.perard@vates.tech> for the next round.
>
Thanks
> Thanks,
>
> --
> Anthony PERARD
On 22.08.2025 12:52, Penny Zheng wrote:
> We extract cppc info from "struct xen_get_cpufreq_para", where it acts as
> a member of union, and share the space with governor info.
> However, it may fail in amd-cppc passive mode, in which governor info and
> CPPC info could co-exist, and both need to be printed together via xenpm tool.
> If we tried to still put it in "struct xen_get_cpufreq_para" (e.g. just move
> out of union), "struct xen_get_cpufreq_para" will enlarge too much to further
> make xen_sysctl.u exceed 128 bytes.
>
> So we introduce a new sub-field GET_CPUFREQ_CPPC to dedicatedly acquire
> CPPC-related para, and make get-cpufreq-para invoke GET_CPUFREQ_CPPC
> if available.
> New helpers print_cppc_para() and get_cpufreq_cppc() are introduced to
> extract CPPC-related parameters process from cpufreq para.
>
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
Acked-by: Jan Beulich <jbeulich@suse.com> # hypervisor
> --- a/tools/libs/ctrl/xc_pm.c
> +++ b/tools/libs/ctrl/xc_pm.c
> @@ -288,7 +288,6 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
> CHK_FIELD(s.scaling_min_freq);
> CHK_FIELD(s.u.userspace);
> CHK_FIELD(s.u.ondemand);
> - CHK_FIELD(cppc_para);
>
> #undef CHK_FIELD
What is done here is already less than what could be done; I think ...
> @@ -366,6 +365,33 @@ int xc_set_cpufreq_cppc(xc_interface *xch, int cpuid,
> return ret;
> }
>
> +int xc_get_cppc_para(xc_interface *xch, unsigned int cpuid,
> + xc_cppc_para_t *cppc_para)
> +{
> + int ret;
> + struct xen_sysctl sysctl = {};
> + struct xen_get_cppc_para *sys_cppc_para = &sysctl.u.pm_op.u.get_cppc;
> +
> + if ( !xch || !cppc_para )
> + {
> + errno = EINVAL;
> + return -1;
> + }
> +
> + sysctl.cmd = XEN_SYSCTL_pm_op;
> + sysctl.u.pm_op.cmd = GET_CPUFREQ_CPPC;
> + sysctl.u.pm_op.cpuid = cpuid;
> +
> + ret = xc_sysctl(xch, &sysctl);
> + if ( ret )
> + return ret;
> +
> + BUILD_BUG_ON(sizeof(*cppc_para) != sizeof(*sys_cppc_para));
> + memcpy(cppc_para, sys_cppc_para, sizeof(*sys_cppc_para));
... you minimally want to apply as much checking here.
Jan
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, August 25, 2025 11:37 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; Anthony PERARD
> <anthony.perard@vates.tech>; Juergen Gross <jgross@suse.com>; Andrew
> Cooper <andrew.cooper3@citrix.com>; Orzel, Michal <Michal.Orzel@amd.com>;
> Julien Grall <julien@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; Stefano
> Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v7 11/13] tools/cpufreq: extract CPPC para from cpufreq para
>
> On 22.08.2025 12:52, Penny Zheng wrote:
> > We extract cppc info from "struct xen_get_cpufreq_para", where it acts
> > as a member of union, and share the space with governor info.
> > However, it may fail in amd-cppc passive mode, in which governor info
> > and CPPC info could co-exist, and both need to be printed together via xenpm
> tool.
> > If we tried to still put it in "struct xen_get_cpufreq_para" (e.g.
> > just move out of union), "struct xen_get_cpufreq_para" will enlarge
> > too much to further make xen_sysctl.u exceed 128 bytes.
> >
> > So we introduce a new sub-field GET_CPUFREQ_CPPC to dedicatedly
> > acquire CPPC-related para, and make get-cpufreq-para invoke
> > GET_CPUFREQ_CPPC if available.
> > New helpers print_cppc_para() and get_cpufreq_cppc() are introduced to
> > extract CPPC-related parameters process from cpufreq para.
> >
> > Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
>
> Acked-by: Jan Beulich <jbeulich@suse.com> # hypervisor
>
Thx
> > --- a/tools/libs/ctrl/xc_pm.c
> > +++ b/tools/libs/ctrl/xc_pm.c
> > @@ -288,7 +288,6 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
> > CHK_FIELD(s.scaling_min_freq);
> > CHK_FIELD(s.u.userspace);
> > CHK_FIELD(s.u.ondemand);
> > - CHK_FIELD(cppc_para);
> >
> > #undef CHK_FIELD
>
> What is done here is already less than what could be done; I think ...
>
Emm, maybe because we define two different cpufreq para structures for user space and sysctl, struct xc_get_cpufreq_para and struct xen_get_cppc_para.
But for cppc para, it is an alias:
typedef struct xen_get_cppc_para xc_cppc_para_t;
So ...
> > @@ -366,6 +365,33 @@ int xc_set_cpufreq_cppc(xc_interface *xch, int cpuid,
> > return ret;
> > }
> >
> > +int xc_get_cppc_para(xc_interface *xch, unsigned int cpuid,
> > + xc_cppc_para_t *cppc_para) {
> > + int ret;
> > + struct xen_sysctl sysctl = {};
> > + struct xen_get_cppc_para *sys_cppc_para =
> > +&sysctl.u.pm_op.u.get_cppc;
> > +
> > + if ( !xch || !cppc_para )
> > + {
> > + errno = EINVAL;
> > + return -1;
> > + }
> > +
> > + sysctl.cmd = XEN_SYSCTL_pm_op;
> > + sysctl.u.pm_op.cmd = GET_CPUFREQ_CPPC;
> > + sysctl.u.pm_op.cpuid = cpuid;
> > +
> > + ret = xc_sysctl(xch, &sysctl);
> > + if ( ret )
> > + return ret;
> > +
> > + BUILD_BUG_ON(sizeof(*cppc_para) != sizeof(*sys_cppc_para));
... maybe whole structure size checking is enough?
> > + memcpy(cppc_para, sys_cppc_para, sizeof(*sys_cppc_para));
>
> ... you minimally want to apply as much checking here.
>
> Jan
On 26.08.2025 10:21, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Monday, August 25, 2025 11:37 PM
>>
>> On 22.08.2025 12:52, Penny Zheng wrote:
>>> --- a/tools/libs/ctrl/xc_pm.c
>>> +++ b/tools/libs/ctrl/xc_pm.c
>>> @@ -288,7 +288,6 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
>>> CHK_FIELD(s.scaling_min_freq);
>>> CHK_FIELD(s.u.userspace);
>>> CHK_FIELD(s.u.ondemand);
>>> - CHK_FIELD(cppc_para);
>>>
>>> #undef CHK_FIELD
>>
>> What is done here is already less than what could be done; I think ...
>>
>
> Emm, maybe because we define two different cpufreq para structures for user space and sysctl, struct xc_get_cpufreq_para and struct xen_get_cppc_para.
> But for cppc para, it is an alias:
> typedef struct xen_get_cppc_para xc_cppc_para_t;
Oh. Then ...
> So ...
>
>>> @@ -366,6 +365,33 @@ int xc_set_cpufreq_cppc(xc_interface *xch, int cpuid,
>>> return ret;
>>> }
>>>
>>> +int xc_get_cppc_para(xc_interface *xch, unsigned int cpuid,
>>> + xc_cppc_para_t *cppc_para) {
>>> + int ret;
>>> + struct xen_sysctl sysctl = {};
>>> + struct xen_get_cppc_para *sys_cppc_para =
>>> +&sysctl.u.pm_op.u.get_cppc;
>>> +
>>> + if ( !xch || !cppc_para )
>>> + {
>>> + errno = EINVAL;
>>> + return -1;
>>> + }
>>> +
>>> + sysctl.cmd = XEN_SYSCTL_pm_op;
>>> + sysctl.u.pm_op.cmd = GET_CPUFREQ_CPPC;
>>> + sysctl.u.pm_op.cpuid = cpuid;
>>> +
>>> + ret = xc_sysctl(xch, &sysctl);
>>> + if ( ret )
>>> + return ret;
>>> +
>>> + BUILD_BUG_ON(sizeof(*cppc_para) != sizeof(*sys_cppc_para));
... why is this here, when ...
>>> + memcpy(cppc_para, sys_cppc_para, sizeof(*sys_cppc_para));
>>
>> ... you minimally want to apply as much checking here.
... a better effect can be had by
cppc_para = sys_cppc_para;
?
Jan
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, August 26, 2025 4:33 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; Anthony PERARD
> <anthony.perard@vates.tech>; Juergen Gross <jgross@suse.com>; Andrew
> Cooper <andrew.cooper3@citrix.com>; Orzel, Michal <Michal.Orzel@amd.com>;
> Julien Grall <julien@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; Stefano
> Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v7 11/13] tools/cpufreq: extract CPPC para from cpufreq para
>
> On 26.08.2025 10:21, Penny, Zheng wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Monday, August 25, 2025 11:37 PM
> >>
> >> On 22.08.2025 12:52, Penny Zheng wrote:
> >>> --- a/tools/libs/ctrl/xc_pm.c
> >>> +++ b/tools/libs/ctrl/xc_pm.c
> >>> @@ -288,7 +288,6 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
> >>> CHK_FIELD(s.scaling_min_freq);
> >>> CHK_FIELD(s.u.userspace);
> >>> CHK_FIELD(s.u.ondemand);
> >>> - CHK_FIELD(cppc_para);
> >>>
> >>> #undef CHK_FIELD
> >>
> >> What is done here is already less than what could be done; I think ...
> >>
> >
> > Emm, maybe because we define two different cpufreq para structures for user
> space and sysctl, struct xc_get_cpufreq_para and struct xen_get_cppc_para.
> > But for cppc para, it is an alias:
> > typedef struct xen_get_cppc_para xc_cppc_para_t;
>
> Oh. Then ...
>
> > So ...
> >
> >>> @@ -366,6 +365,33 @@ int xc_set_cpufreq_cppc(xc_interface *xch, int
> cpuid,
> >>> return ret;
> >>> }
> >>>
> >>> +int xc_get_cppc_para(xc_interface *xch, unsigned int cpuid,
> >>> + xc_cppc_para_t *cppc_para) {
> >>> + int ret;
> >>> + struct xen_sysctl sysctl = {};
> >>> + struct xen_get_cppc_para *sys_cppc_para =
> >>> +&sysctl.u.pm_op.u.get_cppc;
> >>> +
> >>> + if ( !xch || !cppc_para )
> >>> + {
> >>> + errno = EINVAL;
> >>> + return -1;
> >>> + }
> >>> +
> >>> + sysctl.cmd = XEN_SYSCTL_pm_op;
> >>> + sysctl.u.pm_op.cmd = GET_CPUFREQ_CPPC;
> >>> + sysctl.u.pm_op.cpuid = cpuid;
> >>> +
> >>> + ret = xc_sysctl(xch, &sysctl);
> >>> + if ( ret )
> >>> + return ret;
> >>> +
> >>> + BUILD_BUG_ON(sizeof(*cppc_para) != sizeof(*sys_cppc_para));
>
> ... why is this here, when ...
>
> >>> + memcpy(cppc_para, sys_cppc_para, sizeof(*sys_cppc_para));
> >>
> >> ... you minimally want to apply as much checking here.
>
> ... a better effect can be had by
>
> cppc_para = sys_cppc_para;
>
> ?
>
True, no need to do memory copy then if it is an alias
> Jan
© 2016 - 2025 Red Hat, Inc.