Hi Luca,
Spent some time doing more testing on this patch since initially I had only
tested it on
basic programs. After "copyinmsg: allow for the last message element to
have msgt_number = 0."
<https://git.savannah.gnu.org/cgit/hurd/gnumach.git/commit/?id=2e6b257f39ab90938ac9d425629cdf0897a47e48>
it can boot a basic system up to the bash shell. I haven't seen any other
issues so far.
On Mon, Jun 5, 2023 at 1:28 PM Luca <l...@orpolo.org> wrote:
> 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)
>
Yes, that makes sense, but maybe we can do it in a separate patch?
> > +/* 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.
>
Thanks, I feel this is better than using macros. Sent version 2.
>
> Luca
>
>
