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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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

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