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

[Sergey Bugaev]

On Tue, Feb 28, 2023 at 10:18 AM Samuel Thibault
<samuel.thibault@gnu.org> wrote:
>
> Sergey Bugaev, le mar. 28 févr. 2023 09:39:40 +0300, a ecrit:
> > > +                 :                                      \
> > > +                 : "c" (regaddr), "a" (low), "d" (high) \
> > > +                 : "memory"                             \
> > > +        );
> > > +}
> >
> > Why "memory" here? Can wrmsr clobber unrelated memory?
>
> No, but if you don't put a memory clobber here, the compiler will
> optimize the asm away since it's not said to have side effect. Put
> another way, the MSR register is some kind of memory.

No? -- that's what the "volatile" is for [0]. "memory" simply tells
GCC that this piece of asm, *if executed*, may clobber arbitrary
memory; it does not prevent GCC from optimizing the whole thing away.
Here's [1] a simple demo on godbolt that shows both of these to be
true.

[0]: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile
[1]: https://godbolt.org/z/M453v9Gco

Besides, "asm statements that have no output operands and asm goto
statements, are implicitly volatile", so even volatile is not
required, but better be explicit.

> > > +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,
>
> IIRC it does?

Okay.

>
> > > 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
> >
> > 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?
>
> Better separate them indeed.
>
> > Predicating on USER32 is not really going to work here.
>
> Partly because of that :)

But not that USER32 makes sense here either, userspace is either
64-bit or not, there is no middle state. I.e. just drop that  && !
defined(USER32), it's pointless since USER32 is never going to be
defined.

Either a separate file or not is fine by me, but let's settle on something.

Sergey