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 <[email protected]> > --- > 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
