| Message ID | 7a880e33ff91d0c76986159e3559c56ee6894d21.1695324653.git.ehem+xen@m5p.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [WIP] xen/public: move incomplete type definitions to xen.h | expand |
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). > 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. > 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? > Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com> > --- > 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). > --- 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__". > @@ -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. Jan
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@m5p.com> > > --- > > 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).
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. Jan
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?
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
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h index c0f4551247..896440333c 100644 --- a/xen/include/public/arch-x86/xen.h +++ b/xen/include/public/arch-x86/xen.h @@ -143,8 +143,6 @@ struct trap_info { uint16_t cs; /* code selector */ unsigned long address; /* code offset */ }; -typedef struct trap_info trap_info_t; -DEFINE_XEN_GUEST_HANDLE(trap_info_t); typedef uint64_t tsc_timestamp_t; /* RDTSC timestamp */ diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h index 15777b5416..bb7f2dfcb0 100644 --- a/xen/include/public/platform.h +++ b/xen/include/public/platform.h @@ -659,8 +659,6 @@ struct xen_platform_op { uint8_t pad[128]; } u; }; -typedef struct xen_platform_op xen_platform_op_t; -DEFINE_XEN_GUEST_HANDLE(xen_platform_op_t); #endif /* __XEN_PUBLIC_PLATFORM_H__ */ diff --git a/xen/include/public/sched.h b/xen/include/public/sched.h index b4362c6a1d..2b65c0db8c 100644 --- a/xen/include/public/sched.h +++ b/xen/include/public/sched.h @@ -118,8 +118,6 @@ struct sched_shutdown { unsigned int reason; /* SHUTDOWN_* => enum sched_shutdown_reason */ }; -typedef struct sched_shutdown sched_shutdown_t; -DEFINE_XEN_GUEST_HANDLE(sched_shutdown_t); struct sched_poll { XEN_GUEST_HANDLE(evtchn_port_t) ports; diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h index b812a0a324..32a76afbd4 100644 --- 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 @@ -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); /* * ` enum neg_errnoval
Hypercall wrappers need the incomplete type definitions. Only when the actual structure needed. As such these incomplete definitions should be in xen.h next to their hypercalls, rather than spread all over. 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. Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com> --- 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. I'm wondering about __ASSEMBLY__. I suspect this could be handled better by having a macro for all these suspiciously similar type definitions. I suspect it would be handy for DEFINE_XEN_GUEST_HANDLE() to be null when __ASSEMBLY__ is defined. This seems to suggest all the __HYPERVISOR_* definitions need to move later in the file. --- xen/include/public/arch-x86/xen.h | 2 -- xen/include/public/platform.h | 2 -- xen/include/public/sched.h | 2 -- xen/include/public/xen.h | 24 ++++++++++++++++++++---- 4 files changed, 20 insertions(+), 10 deletions(-)