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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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@xxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> 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@xxxxxxxxxx>

Thanks,

-- 
Anthony PERARD