[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH WIP] xen/public: move incomplete type definitions to xen.h
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |