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

[Luca Dariz]

Il 01/03/23 21:18, Samuel Thibault ha scritto:
> Luca Dariz, le mer. 01 mars 2023 18:40:37 +0100, a ecrit:
> > +    asm volatile("wrmsr"
> > +                 :
> > +                 : "c" (regaddr), "a" (low), "d" (high)
> > +                 : "memory"  /* wrmsr is a serializing instruction */
>
> The comment could be misleading.
>
> The fact that it's a serialization instruction does not *require* to
> express it to the compiler.
>
> But the fact that wrmsr needs to be a serialization instruction (because
> it may depend on other writes etc.) means that one *also* wants to make
> the asm snippet serialized by the compiler thanks to the memory clobber.
>
> So I'd rather see:
>
>                   : "memory"  /* wrmsr usage needs serialization */

The comment comes directly from the intel doc about WRMSR:

The WRMSR instruction is a serializing instruction (see “Serializing 
Instructions” in Chapter 8 of the Intel® 64 and IA-32 Architectures 
Software Developer’s Manual, Volume 3A)

and in chapter 8, sec. 3:

The Intel 64 and IA-32 architectures define several serializing 
instructions. These instructions force the processor to complete all 
modifications to flags, registers, and memory by previous instructions 
and to drain all buffered writes to memory before the next instruction 
is fetched and executed.

so in my understanding the serialization is a side effect of the wrmsr 
instruction rather than a requirement, and we want to make sure the 
compiler is aware of this and the optimizers do not assume otherwise.

I'll try to make the comment more accurate.

> > +       /* avoid leaking information in callee-clobbered registers */
> > +       xorq    $0,%rdi
> > +       xorq    $0,%rsi
> > +       xorq    $0,%rdx
> > +       xorq    $0,%r10
> > +       xorq    $0,%r9
> > +       xorq    $0,%r8
>
> No, that's a no-op :)

argh, silly mistake... Thanks for checking this!


Luca