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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Thu, Jul 27, 2023 at 6:27 AM Anthony PERARD
<anthony.perard@xxxxxxxxxx> 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@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)

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@xxxxxxxxxx>

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

Thanks,
Jason