Re: [PATCH 5/5] x86_64: add 64-bit syscall entry point

[Sergey Bugaev]

On Mon, Feb 27, 2023 at 11:46 PM Luca Dariz <luca@orpolo.org> wrote:
>
> While theoretically we could still use the same call gate as for
> 32-bit userspace, it doesn't seem very common, and gcc seems to not
> encode properly the instruction. Instead we use syscall/sysret as
> other kernels (e.g. XNU,Linux). This version still has some
> limitations, but should be enough to start working on the 64-bit user
> space.

Hi -- cool! Some comments/nitpicks inline...

> +static inline void wrmsr(uint32_t regaddr, uint64_t value)
> +{
> +    uint32_t low=(uint32_t)value, high=((uint32_t)(value >> 32));

I think it'd be more idiomatic in both GNU and Mach styles to put more
spaces here, like this:

uint32_t low = (uint32_t) value, high = (uint32_t) (value >> 32);

> +    asm volatile("wrmsr\n"                              \

I don't think you even need the \n for a single instruction.

Does this really need the backslashes? They are needed in macros, but why here?

> +                 :                                      \
> +                 : "c" (regaddr), "a" (low), "d" (high) \
> +                 : "memory"                             \
> +        );
> +}

Why "memory" here? Can wrmsr clobber unrelated memory? (I don't know,
maybe it can -- if so, perhaps add a comment?)

> +static inline uint64_t rdmsr(uint32_t regaddr)
> +{
> +    uint32_t low, high;
> +    asm volatile("rdmsr\n"                              \
> +                 : "=a" (low), "=d" (high)              \
> +                 : "c" (regaddr)                        \
> +        );
> +    return ((uint64_t)high << 32) | low;

Ditto about spacing -- and does this need volatile? As in, does
reading from a MSR have side effects that we're interested in, or does
it only output the read value? (Again, I don't know!)

> diff --git a/i386/include/mach/i386/syscall_sw.h 
> b/i386/include/mach/i386/syscall_sw.h
> index 86f6ff2f..20ef7c13 100644
> --- a/i386/include/mach/i386/syscall_sw.h
> +++ b/i386/include/mach/i386/syscall_sw.h
> @@ -29,16 +29,16 @@
>
>  #include <mach/machine/asm.h>
>
> -#if BSD_TRAP
> -#define kernel_trap(trap_name,trap_number,number_args) \
> -ENTRY(trap_name) \
> -       movl    $ trap_number,%eax; \
> -       SVC; \
> -       jb LCL(cerror); \
> -       ret; \
> +#if defined(__x86_64__) && ! defined(USER32)
> +#define kernel_trap(trap_name,trap_number,number_args)  \
> +ENTRY(trap_name)                                       \
> +       movq    $ trap_number,%rax;                     \
> +       movq    %rcx,%r10;                              \
> +       syscall;                                        \
> +       ret;                                            \
>  END(trap_name)

OK, so the x86_64 syscall definition stays in i386/syscall_sw.h, and
not in a separate x86_64/syscall_sw.h file? That's what I thought. In
this case, we do want that mach-machine patch in glibc. Samuel, does
this make sense to you?

Predicating on USER32 is not really going to work here. This header is
AFAICS not used by Mach itself, it's the UAPI header, one that is
installed into the system's include dir for the userspace to include &
use. And surely userspace is not going to define USER32 either way.

But then glibc does not actually use mach/syscall_sw.h, it has its own
syscall impl, so I need to understand the ABI. trap_number is in rax,
args on the stack, return value in rax, is that right? What's rcx/r10?
Does a syscall preserve other registers? What about rflags? Is this
the same as Linux does or?..

Thanks!

Sergey