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

[Elliott Mitchell <ehem+xen@xxxxxxx>]

On Mon, Sep 25, 2023 at 08:27:31AM +0200, Jan Beulich wrote:
> On 22.09.2023 17:42, Elliott Mitchell wrote:
> > On Fri, Sep 22, 2023 at 10:21:21AM +0200, Jan Beulich wrote:
> >> On 21.09.2023 18:18, Elliott Mitchell wrote:
> >>>  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.
> 
> Downstream consumers of the public headers are free to adjust them to their
> needs. The upstream form wants to remain sufficiently generic, which to me
> includes not exposing types which aren't relevant for a particular arch.

Problem with not exposing the type is you instead get inconsistency,
which can be worse than pollution of the namespace.  There is the case
I'm bring up here which makes sharing headers difficult.

What if some project was developed to run on Xen/ARM.  Such a project
might create a "trap_info" structure for something unrelated to the
Xen/x86 one, they might similarly create a "trap_info_t" type definition.
If such a hypothetical project later tried to port to Xen/x86, the
inconsistency would be painful to deal with.

So might consistency be a rather more important virtue?


-- 
(\___(\___(\______          --=> 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