Re: [PATCH gnumach] Update the 64bit RPC ABI to be simpler

[Luca]

Hi,

Il 17/05/23 05:03, Flavio Cruz ha scritto:
> * Make full use of the 8 bytes available in mach_msg_type_t by moving
>    into the unused 4 bytes. This allows us to use 32bits for
>    mach_msg_type_number_t whether we use the longform or not.
> * Make mach_msg_type_long_t exactly the same as mach_msg_type_t. I'm not
>    changing any of the code but keeping the same interface using macros.
>    Updating MiG is strongly encouraged since it will generate better code
>    to handle this new format.
>
> After this change, any compatibility with compiled binaries for Hurd x86_64
> will break since the message format is different. However, the new
> schema simplifies the overall ABI, without having "holes" and also
> avoids the need to have a 16 byte mach_msg_type_long_t.
>
> Tested with simple programs on pure 64 bit and 64/32 bit combinations >
> ---
> Not too thrilled about the use of the macros for msgtl_name/size/number
> but happy to hear about other alternatives. Potentially we could
> introduce accessors and update all the calls to use them.
>
> Note that the follow up MiG patch is necessary to make this work.
>
>   include/mach/message.h |  37 +++++++++++++--
>   ipc/ipc_kmsg.c         |   4 ++
>   x86_64/copy_user.c     | 100 ++++++++++++++++++++++++++++++++---------
>   3 files changed, 115 insertions(+), 26 deletions(-)
>
> diff --git a/include/mach/message.h b/include/mach/message.h
> index 0eab9d41..d1783715 100644
> --- a/include/mach/message.h
> +++ b/include/mach/message.h
> @@ -222,6 +222,30 @@ typedef unsigned int mach_msg_type_size_t;
>   typedef natural_t  mach_msg_type_number_t;
>   
>   typedef struct  {
> +#ifdef __x86_64__
> +    /*
> +     * For 64 bits, this struct is 8 bytes long so we
> +     * can pack the same amount of information as mach_msg_type_long_t.
> +     * Note that for 64 bit userland, msgt_size only needs to be 8 bits long
> +     * but for kernel compatibility with 32 bit userland we allow it to be
> +     * 16 bits long.
> +     *
> +     * Effectively, we don't need mach_msg_type_long_t but we are keeping it
> +     * for a while to make the code similar between 32 and 64 bits.
> +     *
> +     * We also keep the msgt_longform bit around simply because it makes it
> +     * very easy to convert messages from a 32 bit userland into a 64 bit
> +     * kernel. Otherwise, we would have to replicate some of the MiG logic
> +     * internally in the kernel.
> +     */
> +    unsigned int       msgt_inline : 1,
> +                       msgt_longform : 1,
> +                       msgt_deallocate : 1,
> +                       msgt_name : 8,
> +                       msgt_size : 16,
> +                       msgt_unused : 5;
> +    mach_msg_type_number_t   msgt_number;
> +#else
>       unsigned int      msgt_name : 8,
>                         msgt_size : 8,
>                         msgt_number : 12,
> @@ -229,18 +253,23 @@ typedef struct  {
>                         msgt_longform : 1,
>                         msgt_deallocate : 1,
>                         msgt_unused : 1;
> -#ifdef __x86_64__
> -    /* TODO: We want to eventually use this in favor of mach_msg_type_long_t
> -     * as it makes the mach_msg protocol require only mach_msg_type_t. */
> -    mach_msg_type_number_t   unused_msgtl_number;
>   #endif
>   } __attribute__ ((aligned (__alignof__ (uintptr_t)))) mach_msg_type_t;

Maybe the user/kernel variants of mach_msg_type_t and 
mach_msg_type_long_t should also be moved here from copy_user.c, just as 
the header struct (or all the user variants to a kernel-specific header)

> +/* On x86_64 this is equivalent to mach_msg_type_t.  */
>   typedef       struct  {
>       mach_msg_type_t   msgtl_header;
> +#ifdef __x86_64__
> +/* Since we don't have these separate fields, we remap them
> + * into the fields from mach_msg_type_t. */
> +#define msgtl_name msgtl_header.msgt_name
> +#define msgtl_size msgtl_header.msgt_size
> +#define msgtl_number msgtl_header.msgt_number
> +#else
>       unsigned short    msgtl_name;
>       unsigned short    msgtl_size;
>       natural_t         msgtl_number;
> +#endif
>   } __attribute__ ((aligned (__alignof__ (uintptr_t)))) mach_msg_type_long_t;

one alternative to the #defines could be something similar to

typedef union {
    mach_msg_type_t     msgtl_header;
    struct {
        unsigned int    msgt_unused1 : 3,
            msgt_name : 8,
            msgt_size : 16,
            msgt_unused2 : 5;
        mach_msg_type_number_t   msgt_number;
    };
}  __attribute__ ((aligned (__alignof__ (uintptr_t)))) mach_msg_type_long_t;

This would replicate the definition of the bitfields, but IMHO it would 
be more readable than another level of macro. I guess we can ensure both 
definitions are consistent with some static asserts.


Luca