[PATCH v7 04/15] xen/sysctl: Nest cpufreq scaling options

Jason Andryuk posted 15 patches 1 year, 4 months ago
There is a newer version of this series
[PATCH v7 04/15] xen/sysctl: Nest cpufreq scaling options
Posted by Jason Andryuk 1 year, 4 months ago
Add a union and struct so that most of the scaling variables of struct
xen_get_cpufreq_para are within in a binary-compatible layout.  This
allows cppc_para to live in the larger union and use uint32_ts - struct
xen_cppc_para will be 10 uint32_t's.

The new scaling struct is 3 * uint32_t + 16 bytes CPUFREQ_NAME_LEN + 4 *
uint32_t for xen_ondemand = 11 uint32_t.  That means the old size is
retained, int32_t turbo_enabled doesn't move and it's binary compatible.

The out-of-context memcpy() in xc_get_cpufreq_para() now handles the
copying of the fields removed there.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
NOTE: Jan would like a toolside review / ack because:
    Nevertheless I continue to be uncertain about all of this: Parts of
    the struct can apparently go out of sync with the sysctl struct, but
    other parts have to remain in sync without there being an
    appropriate build-time check (checking merely sizes clearly isn't
    enough). Therefore I'd really like to have a toolstack side review /
    ack here as well.

v6:
Add Jan's Reviewed-by

v5:
Expand commit message
Change comment to driver/governor
---
 tools/include/xenctrl.h     | 22 +++++++++++++---------
 tools/libs/ctrl/xc_pm.c     |  7 +------
 tools/misc/xenpm.c          | 24 ++++++++++++------------
 xen/drivers/acpi/pmstat.c   | 27 ++++++++++++++-------------
 xen/include/public/sysctl.h | 22 +++++++++++++---------
 5 files changed, 53 insertions(+), 49 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index dba33d5d0f..8aedb952a0 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1909,16 +1909,20 @@ struct xc_get_cpufreq_para {
     uint32_t cpuinfo_cur_freq;
     uint32_t cpuinfo_max_freq;
     uint32_t cpuinfo_min_freq;
-    uint32_t scaling_cur_freq;
-
-    char scaling_governor[CPUFREQ_NAME_LEN];
-    uint32_t scaling_max_freq;
-    uint32_t scaling_min_freq;
-
-    /* for specific governor */
     union {
-        xc_userspace_t userspace;
-        xc_ondemand_t ondemand;
+        struct {
+            uint32_t scaling_cur_freq;
+
+            char scaling_governor[CPUFREQ_NAME_LEN];
+            uint32_t scaling_max_freq;
+            uint32_t scaling_min_freq;
+
+            /* for specific governor */
+            union {
+                xc_userspace_t userspace;
+                xc_ondemand_t ondemand;
+            } u;
+        } s;
     } u;
 
     int32_t turbo_enabled;
diff --git a/tools/libs/ctrl/xc_pm.c b/tools/libs/ctrl/xc_pm.c
index c3a9864bf7..6e751e242f 100644
--- a/tools/libs/ctrl/xc_pm.c
+++ b/tools/libs/ctrl/xc_pm.c
@@ -265,17 +265,12 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
         user_para->cpuinfo_cur_freq = sys_para->cpuinfo_cur_freq;
         user_para->cpuinfo_max_freq = sys_para->cpuinfo_max_freq;
         user_para->cpuinfo_min_freq = sys_para->cpuinfo_min_freq;
-        user_para->scaling_cur_freq = sys_para->scaling_cur_freq;
-        user_para->scaling_max_freq = sys_para->scaling_max_freq;
-        user_para->scaling_min_freq = sys_para->scaling_min_freq;
         user_para->turbo_enabled    = sys_para->turbo_enabled;
 
         memcpy(user_para->scaling_driver,
                 sys_para->scaling_driver, CPUFREQ_NAME_LEN);
-        memcpy(user_para->scaling_governor,
-                sys_para->scaling_governor, CPUFREQ_NAME_LEN);
 
-        /* copy to user_para no matter what cpufreq governor */
+        /* copy to user_para no matter what cpufreq driver/governor */
         BUILD_BUG_ON(sizeof(((struct xc_get_cpufreq_para *)0)->u) !=
 		     sizeof(((struct xen_get_cpufreq_para *)0)->u));
 
diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
index 1bb6187e56..ee8ce5d5f2 100644
--- a/tools/misc/xenpm.c
+++ b/tools/misc/xenpm.c
@@ -730,39 +730,39 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)
     printf("scaling_avail_gov    : %s\n",
            p_cpufreq->scaling_available_governors);
 
-    printf("current_governor     : %s\n", p_cpufreq->scaling_governor);
-    if ( !strncmp(p_cpufreq->scaling_governor,
+    printf("current_governor     : %s\n", p_cpufreq->u.s.scaling_governor);
+    if ( !strncmp(p_cpufreq->u.s.scaling_governor,
                   "userspace", CPUFREQ_NAME_LEN) )
     {
         printf("  userspace specific :\n");
         printf("    scaling_setspeed : %u\n",
-               p_cpufreq->u.userspace.scaling_setspeed);
+               p_cpufreq->u.s.u.userspace.scaling_setspeed);
     }
-    else if ( !strncmp(p_cpufreq->scaling_governor,
+    else if ( !strncmp(p_cpufreq->u.s.scaling_governor,
                        "ondemand", CPUFREQ_NAME_LEN) )
     {
         printf("  ondemand specific  :\n");
         printf("    sampling_rate    : max [%u] min [%u] cur [%u]\n",
-               p_cpufreq->u.ondemand.sampling_rate_max,
-               p_cpufreq->u.ondemand.sampling_rate_min,
-               p_cpufreq->u.ondemand.sampling_rate);
+               p_cpufreq->u.s.u.ondemand.sampling_rate_max,
+               p_cpufreq->u.s.u.ondemand.sampling_rate_min,
+               p_cpufreq->u.s.u.ondemand.sampling_rate);
         printf("    up_threshold     : %u\n",
-               p_cpufreq->u.ondemand.up_threshold);
+               p_cpufreq->u.s.u.ondemand.up_threshold);
     }
 
     printf("scaling_avail_freq   :");
     for ( i = 0; i < p_cpufreq->freq_num; i++ )
         if ( p_cpufreq->scaling_available_frequencies[i] ==
-             p_cpufreq->scaling_cur_freq )
+             p_cpufreq->u.s.scaling_cur_freq )
             printf(" *%d", p_cpufreq->scaling_available_frequencies[i]);
         else
             printf(" %d", p_cpufreq->scaling_available_frequencies[i]);
     printf("\n");
 
     printf("scaling frequency    : max [%u] min [%u] cur [%u]\n",
-           p_cpufreq->scaling_max_freq,
-           p_cpufreq->scaling_min_freq,
-           p_cpufreq->scaling_cur_freq);
+           p_cpufreq->u.s.scaling_max_freq,
+           p_cpufreq->u.s.scaling_min_freq,
+           p_cpufreq->u.s.scaling_cur_freq);
 
     printf("turbo mode           : %s\n",
            p_cpufreq->turbo_enabled ? "enabled" : "disabled or n/a");
diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c
index 1bae635101..f5a9ac3f1a 100644
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -258,37 +258,38 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
         cpufreq_driver.get ? cpufreq_driver.get(op->cpuid) : policy->cur;
     op->u.get_para.cpuinfo_max_freq = policy->cpuinfo.max_freq;
     op->u.get_para.cpuinfo_min_freq = policy->cpuinfo.min_freq;
-    op->u.get_para.scaling_cur_freq = policy->cur;
-    op->u.get_para.scaling_max_freq = policy->max;
-    op->u.get_para.scaling_min_freq = policy->min;
+
+    op->u.get_para.u.s.scaling_cur_freq = policy->cur;
+    op->u.get_para.u.s.scaling_max_freq = policy->max;
+    op->u.get_para.u.s.scaling_min_freq = policy->min;
 
     if ( cpufreq_driver.name[0] )
-        strlcpy(op->u.get_para.scaling_driver, 
+        strlcpy(op->u.get_para.scaling_driver,
             cpufreq_driver.name, CPUFREQ_NAME_LEN);
     else
         strlcpy(op->u.get_para.scaling_driver, "Unknown", CPUFREQ_NAME_LEN);
 
     if ( policy->governor->name[0] )
-        strlcpy(op->u.get_para.scaling_governor, 
+        strlcpy(op->u.get_para.u.s.scaling_governor,
             policy->governor->name, CPUFREQ_NAME_LEN);
     else
-        strlcpy(op->u.get_para.scaling_governor, "Unknown", CPUFREQ_NAME_LEN);
+        strlcpy(op->u.get_para.u.s.scaling_governor, "Unknown", CPUFREQ_NAME_LEN);
 
     /* governor specific para */
-    if ( !strncasecmp(op->u.get_para.scaling_governor,
+    if ( !strncasecmp(op->u.get_para.u.s.scaling_governor,
                       "userspace", CPUFREQ_NAME_LEN) )
     {
-        op->u.get_para.u.userspace.scaling_setspeed = policy->cur;
+        op->u.get_para.u.s.u.userspace.scaling_setspeed = policy->cur;
     }
 
-    if ( !strncasecmp(op->u.get_para.scaling_governor,
+    if ( !strncasecmp(op->u.get_para.u.s.scaling_governor,
                       "ondemand", CPUFREQ_NAME_LEN) )
     {
         ret = get_cpufreq_ondemand_para(
-            &op->u.get_para.u.ondemand.sampling_rate_max,
-            &op->u.get_para.u.ondemand.sampling_rate_min,
-            &op->u.get_para.u.ondemand.sampling_rate,
-            &op->u.get_para.u.ondemand.up_threshold);
+            &op->u.get_para.u.s.u.ondemand.sampling_rate_max,
+            &op->u.get_para.u.s.u.ondemand.sampling_rate_min,
+            &op->u.get_para.u.s.u.ondemand.sampling_rate,
+            &op->u.get_para.u.s.u.ondemand.up_threshold);
     }
     op->u.get_para.turbo_enabled = cpufreq_get_turbo_status(op->cpuid);
 
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index fa7147de47..c11c0b1a6c 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -317,16 +317,20 @@ struct xen_get_cpufreq_para {
     uint32_t cpuinfo_cur_freq;
     uint32_t cpuinfo_max_freq;
     uint32_t cpuinfo_min_freq;
-    uint32_t scaling_cur_freq;
-
-    char scaling_governor[CPUFREQ_NAME_LEN];
-    uint32_t scaling_max_freq;
-    uint32_t scaling_min_freq;
-
-    /* for specific governor */
     union {
-        struct  xen_userspace userspace;
-        struct  xen_ondemand ondemand;
+        struct {
+            uint32_t scaling_cur_freq;
+
+            char scaling_governor[CPUFREQ_NAME_LEN];
+            uint32_t scaling_max_freq;
+            uint32_t scaling_min_freq;
+
+            /* for specific governor */
+            union {
+                struct  xen_userspace userspace;
+                struct  xen_ondemand ondemand;
+            } u;
+        } s;
     } u;
 
     int32_t turbo_enabled;
-- 
2.41.0
Re: [PATCH v7 04/15] xen/sysctl: Nest cpufreq scaling options
Posted by Anthony PERARD 1 year, 4 months ago
On Wed, Jul 26, 2023 at 01:09:34PM -0400, Jason Andryuk wrote:
> Add a union and struct so that most of the scaling variables of struct
> xen_get_cpufreq_para are within in a binary-compatible layout.  This
> allows cppc_para to live in the larger union and use uint32_ts - struct
> xen_cppc_para will be 10 uint32_t's.
> 
> The new scaling struct is 3 * uint32_t + 16 bytes CPUFREQ_NAME_LEN + 4 *
> uint32_t for xen_ondemand = 11 uint32_t.  That means the old size is
> retained, int32_t turbo_enabled doesn't move and it's binary compatible.
> 
> The out-of-context memcpy() in xc_get_cpufreq_para() now handles the
> copying of the fields removed there.
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
> NOTE: Jan would like a toolside review / ack because:
>     Nevertheless I continue to be uncertain about all of this: Parts of
>     the struct can apparently go out of sync with the sysctl struct, but
>     other parts have to remain in sync without there being an
>     appropriate build-time check (checking merely sizes clearly isn't
>     enough). Therefore I'd really like to have a toolstack side review /
>     ack here as well.

I wish we could just use "struct xen_get_cpufreq_para" instead of
having to make a copy to replace the XEN_GUEST_HANDLE_*() macro.

Next I guess we could try to have something like the compat layer in xen
is doing, with plenty of CHECK_FIELD_ and other CHECK_* macro, but that
would be a lot of work. (xen/include/xen/compat.h and how it's used in
xen/include/compat/xlat.h)

Unless you feel like adding more build check, I guess the patch is good
enough like that:
Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD
Re: [PATCH v7 04/15] xen/sysctl: Nest cpufreq scaling options
Posted by Jason Andryuk 1 year, 4 months ago
On Thu, Jul 27, 2023 at 6:27 AM Anthony PERARD
<anthony.perard@citrix.com> wrote:
>
> On Wed, Jul 26, 2023 at 01:09:34PM -0400, Jason Andryuk wrote:
> > Add a union and struct so that most of the scaling variables of struct
> > xen_get_cpufreq_para are within in a binary-compatible layout.  This
> > allows cppc_para to live in the larger union and use uint32_ts - struct
> > xen_cppc_para will be 10 uint32_t's.
> >
> > The new scaling struct is 3 * uint32_t + 16 bytes CPUFREQ_NAME_LEN + 4 *
> > uint32_t for xen_ondemand = 11 uint32_t.  That means the old size is
> > retained, int32_t turbo_enabled doesn't move and it's binary compatible.
> >
> > The out-of-context memcpy() in xc_get_cpufreq_para() now handles the
> > copying of the fields removed there.
> >
> > Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> > Reviewed-by: Jan Beulich <jbeulich@suse.com>
> > ---
> > NOTE: Jan would like a toolside review / ack because:
> >     Nevertheless I continue to be uncertain about all of this: Parts of
> >     the struct can apparently go out of sync with the sysctl struct, but
> >     other parts have to remain in sync without there being an
> >     appropriate build-time check (checking merely sizes clearly isn't
> >     enough). Therefore I'd really like to have a toolstack side review /
> >     ack here as well.
>
> I wish we could just use "struct xen_get_cpufreq_para" instead of
> having to make a copy to replace the XEN_GUEST_HANDLE_*() macro.
>
> Next I guess we could try to have something like the compat layer in xen
> is doing, with plenty of CHECK_FIELD_ and other CHECK_* macro, but that
> would be a lot of work. (xen/include/xen/compat.h and how it's used in
> xen/include/compat/xlat.h)

I can add a set of BUILD_BUG_ON()s checking the offsets of the two
structs' members.  I think that would work and we don't need the
complication of the compat code.  The build of the library just deals
with a single bit-width and needs to be consistent.  The hypervisor
needs to deal with receiving differing pointer sizes and layouts, but
the userspace library just uses whatever it was compiled for.  The
preprocessor expands XEN_GUEST_HANDLE_64(uint32) -> "typedef struct {
uint32_t *p; } __guest_handle_uint32", so it's just going to be a
single pointer in size, which will match between the two.

Does that sound right, or am I missing something?

> Unless you feel like adding more build check, I guess the patch is good
> enough like that:
> Acked-by: Anthony PERARD <anthony.perard@citrix.com>

If I am incorrect about the above... I'll just leave it as-is.

Thanks,
Jason
Re: [PATCH v7 04/15] xen/sysctl: Nest cpufreq scaling options
Posted by Anthony PERARD 1 year, 4 months ago
On Thu, Jul 27, 2023 at 11:05:33AM -0400, Jason Andryuk wrote:
> On Thu, Jul 27, 2023 at 6:27 AM Anthony PERARD
> <anthony.perard@citrix.com> wrote:
> >
> > On Wed, Jul 26, 2023 at 01:09:34PM -0400, Jason Andryuk wrote:
> > > Add a union and struct so that most of the scaling variables of struct
> > > xen_get_cpufreq_para are within in a binary-compatible layout.  This
> > > allows cppc_para to live in the larger union and use uint32_ts - struct
> > > xen_cppc_para will be 10 uint32_t's.
> > >
> > > The new scaling struct is 3 * uint32_t + 16 bytes CPUFREQ_NAME_LEN + 4 *
> > > uint32_t for xen_ondemand = 11 uint32_t.  That means the old size is
> > > retained, int32_t turbo_enabled doesn't move and it's binary compatible.
> > >
> > > The out-of-context memcpy() in xc_get_cpufreq_para() now handles the
> > > copying of the fields removed there.
> > >
> > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> > > Reviewed-by: Jan Beulich <jbeulich@suse.com>
> > > ---
> > > NOTE: Jan would like a toolside review / ack because:
> > >     Nevertheless I continue to be uncertain about all of this: Parts of
> > >     the struct can apparently go out of sync with the sysctl struct, but
> > >     other parts have to remain in sync without there being an
> > >     appropriate build-time check (checking merely sizes clearly isn't
> > >     enough). Therefore I'd really like to have a toolstack side review /
> > >     ack here as well.
> >
> > I wish we could just use "struct xen_get_cpufreq_para" instead of
> > having to make a copy to replace the XEN_GUEST_HANDLE_*() macro.
> >
> > Next I guess we could try to have something like the compat layer in xen
> > is doing, with plenty of CHECK_FIELD_ and other CHECK_* macro, but that
> > would be a lot of work. (xen/include/xen/compat.h and how it's used in
> > xen/include/compat/xlat.h)
> 
> I can add a set of BUILD_BUG_ON()s checking the offsets of the two
> structs' members.

Yes, that would be fine.

> I think that would work and we don't need the
> complication of the compat code.  The build of the library just deals
> with a single bit-width and needs to be consistent.  The hypervisor
> needs to deal with receiving differing pointer sizes and layouts, but
> the userspace library just uses whatever it was compiled for.  The
> preprocessor expands XEN_GUEST_HANDLE_64(uint32) -> "typedef struct {
> uint32_t *p; } __guest_handle_uint32", so it's just going to be a
> single pointer in size, which will match between the two.
> 
> Does that sound right, or am I missing something?

That sounds about right.

Thanks,

-- 
Anthony PERARD