| Message ID | 20230726170945.34961-5-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Intel Hardware P-States (HWP) support | expand |
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,
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
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,
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;