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

[Samuel Thibault]

Sergey Bugaev, le mar. 28 févr. 2023 10:45:47 +0300, a ecrit:
> 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].

Ah, I missed that it was there. Not enough quoting :)

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

Urgl, indeed.

Now it also has another meaning: since it clobbers memory, gcc is
not allowed to keep values in registers, i.e. this is a memory
synchronization barrier concerning the compiler. If for instance we have
prepared some data in memory before issuing the wrmsr, we need it for
the data preparation to be actually written. At least Linux does have
this clobber in its wrmsr inlines.

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

Sure, but it shows that the kernel needs to somehow tell apart 32 vs 64,
and relying on USER32 won't work. Simplest is to just ship different
files. There is nothing to share between the two versions anyway.

Samuel