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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxx>
> ---
> 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