Re: [PATCH v7 04/15] xen/sysctl: Nest cpufreq scaling options
On Thu, Jul 27, 2023 at 11:05:33AM -0400, Jason Andryuk wrote:
> On Thu, Jul 27, 2023 at 6:27 AM Anthony PERARD
> 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
> > > Reviewed-by: Jan Beulich
> > > ---
> > > 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
Re: [PATCH v7 04/15] xen/sysctl: Nest cpufreq scaling options
On Thu, Jul 27, 2023 at 6:27 AM Anthony PERARD
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
> > Reviewed-by: Jan Beulich
> > ---
> > 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
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
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 > Reviewed-by: Jan Beulich > --- > 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 Thanks, -- Anthony PERARD
[PATCH v7 04/15] xen/sysctl: Nest cpufreq scaling options
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
Reviewed-by: Jan Beulich
---
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);
+
