Re: [PATCH WIP] xen/public: move incomplete type definitions to xen.h

[Elliott Mitchell <ehem+xen@xxxxxxx>]

On Fri, Sep 22, 2023 at 10:21:21AM +0200, Jan Beulich wrote:
> On 21.09.2023 18:18, Elliott Mitchell wrote:
> > Hypercall wrappers need the incomplete type definitions.  Only when the
> > actual structure needed.
> 
> While in the first sentence "only" looks to be missing, I can't really
> make sense of the second (without implying what I think you mean).

I'm not an editor, thinkos in commit messages happen.  Likely should have
removed that sentence.

> >  As such these incomplete definitions should be
> > in xen.h next to their hypercalls, rather than spread all over.
> 
> Perhaps s/incomplete definitions/forward declarations/.
> 
> There's a downside to the movement, though: You now introduce items
> into the namespace which may be entirely unused. The two contradicting
> goals need weighing as to their usefulness.

For the case which this is part of, they're not 100% unused.

> > trap_info_t is particularly notable since even though the hypercall is
> > x86-only, the wrapper is likely to be visible to generic source code.
> 
> Why would it be?

Related to converting ARM to using inline assembly-language wrappers
instead of the current declarations+small assembly wrapper function.

The first step is you split the Linux header
arch/x86/include/asm/xen/hypercall.h.  The upper portion (the x86
inline assembly language) remains in arch/x86/include, all the
HYPERVISOR_*() wrappers go into include/xen/$somewhere.  Several months
ago I sent a candidate header to implement _hypercall#() for ARM.

Problem is:
static inline int
HYPERVISOR_set_trap_table(struct trap_info *table)
{
        return _hypercall1(int, set_trap_table, table);
}
Without without `struct trap_info;` somewhere, this fails.

Now, this isn't used on ARM, but this is tricky to guess.  Someone
setting this up won't know whether any given function is absent due to
being legacy and unlikely to ever be on non-x86.  Versus simply not /yet/
being available on non-x86 (vPCI).

Perhaps xen/include/public/xen.h should only conditionally #define some
of the __HYPERVISOR_* constants.  Likely there should be a way to force
all the hypercall numbers to be available (for linting).  Yet as the
current Linux header hints, perhaps there should be a way to disable the
PV constants even on x86.


> > Signed-off-by: Elliott Mitchell <ehem+xen@xxxxxxx>
> > ---
> > trap_info_t and HYPERVISOR_set_trap_table() is something I ran into.
> > With the incomplete definition, the wrapper is accaptable to an ARM
> > compiler.  Without the incomplete definition, it fails.
> > 
> > Note, this has been shown to build in my environment.  I'm unsure
> > whether the incomplete structure plus type definition is acceptable to
> > all supportted compilers.
> 
> It's permitted by the standard, so ought to be acceptable to all C89
> compilers (which is what we use as baseline for the public headers).

FreeBSD recently changed $something which now makes this work.  Since I
had (less than 2 years ago) been noticing this.  This could be deemed
unnecessary at this point, I'm simply noting it.

> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -75,13 +75,25 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
> >   */
> >  
> >  #define __HYPERVISOR_set_trap_table        0
> > +#ifndef __ASSEMBLY__
> > +typedef struct trap_info trap_info_t;
> > +DEFINE_XEN_GUEST_HANDLE(trap_info_t);
> > +#endif
> >  #define __HYPERVISOR_mmu_update            1
> > +#ifndef __ASSEMBLY__
> > +typedef struct mmu_update mmu_update_t;
> > +DEFINE_XEN_GUEST_HANDLE(mmu_update_t);
> > +#endif
> >  #define __HYPERVISOR_set_gdt               2
> >  #define __HYPERVISOR_stack_switch          3
> >  #define __HYPERVISOR_set_callbacks         4
> >  #define __HYPERVISOR_fpu_taskswitch        5
> >  #define __HYPERVISOR_sched_op_compat       6 /* compat since 0x00030101 */
> >  #define __HYPERVISOR_platform_op           7
> > +#ifndef __ASSEMBLY__
> > +typedef struct xen_platform_op xen_platform_op_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_platform_op_t);
> > +#endif
> >  #define __HYPERVISOR_set_debugreg          8
> >  #define __HYPERVISOR_get_debugreg          9
> >  #define __HYPERVISOR_update_descriptor    10
> > @@ -100,9 +112,17 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
> >  #define __HYPERVISOR_vcpu_op              24
> >  #define __HYPERVISOR_set_segment_base     25 /* x86/64 only */
> >  #define __HYPERVISOR_mmuext_op            26
> > +#ifndef __ASSEMBLY__
> > +typedef struct mmuext_op mmuext_op_t;
> > +DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
> > +#endif
> >  #define __HYPERVISOR_xsm_op               27
> >  #define __HYPERVISOR_nmi_op               28
> >  #define __HYPERVISOR_sched_op             29
> > +#ifndef __ASSEMBLY__
> > +typedef struct sched_shutdown sched_shutdown_t;
> > +DEFINE_XEN_GUEST_HANDLE(sched_shutdown_t);
> > +#endif
> >  #define __HYPERVISOR_callback_op          30
> >  #define __HYPERVISOR_xenoprof_op          31
> >  #define __HYPERVISOR_event_channel_op     32
> 
> Interspersing the #define-s with typedef-s and
> DEFINE_XEN_GUEST_HANDLE()s clutters this section imo. If movement to
> a central place was wanted, then perhaps below all of the #define-s,
> then allowing to get away with just a single "#ifndef __ASSEMBLY__".

I like associating the hypercalls and their special structure type.
Perhaps roughly:
#ifdef __ASSEMBLY__
#define DEFINE_XEN_GUEST_HANDLE(arg)
#define XEN_TYPEDEF(type)
#else
#define XEN_TYPEDEF(type) typedef struct type type#_t;
#endif

(this hasn't been tested)

> > @@ -449,8 +469,6 @@ struct mmuext_op {
> >          xen_pfn_t src_mfn;
> >      } arg2;
> >  };
> > -typedef struct mmuext_op mmuext_op_t;
> > -DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
> >  #endif
> >  
> >  /*
> > @@ -615,8 +633,6 @@ struct mmu_update {
> >      uint64_t ptr;       /* Machine address of PTE. */
> >      uint64_t val;       /* New contents of PTE.    */
> >  };
> > -typedef struct mmu_update mmu_update_t;
> > -DEFINE_XEN_GUEST_HANDLE(mmu_update_t);
> 
> Imo a prereq to moving these up is to move the struct-s themselves into
> the x86 header. From all we can tell no present or future port is going
> to use these PV-only interfaces.

With this patch, an experimental build of FreeBSD/arm64 succeeded.  I'm
unsure which flavor of C standard is presently enabled with FreeBSD
kernel builds though (I believe it was bumped 6-12 months ago).


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@xxxxxxx  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445