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

[Luca Dariz]

Il 28/02/23 07:39, Sergey Bugaev ha scritto:
> On Mon, Feb 27, 2023 at 11:46 PM Luca Dariz <luca@orpolo.org> wrote:
> > +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?

I'll clean up the formatting, \n and backslashes

> > +                 :                                      \
> > +                 : "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?)

I admit I added "memory" just because I saw it in Linux, but looking 
deeper it turns out that wrmsr is a serializing instruction, which is 
even more than a memory barrier, so I think it's better to explicitly 
tell the compiler about it (see e.g. §8.3 of Intel's System Programming 
Guide, vol. 3 [0[).

> > +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!)

I'd say both reading and writing a MSR is a side effect, as its value 
could change potentially in ways not predictable by the compiler, and 
the MSR itself is not directly "seen" by the compiler. It's a kind of 
I/O in this sense.

> > 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?

I'll try to change the installed file depending on i386/x86_64/USER32.

> 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.

Right, I'll remove USER32.


Thanks!

Luca


[0] here's the combined version, with volumes 1-4 
https://cdrdv2.intel.com/v1/dl/getContent/671200