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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.09.2023 02:47, Elliott Mitchell wrote:
> 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.

Well, that's owing to the fact that trap_info itself doesn't respect name
space rules. It should have been xen_trap_info.

> So might consistency be a rather more important virtue?

I don't think so.

Jan