On Tue, Feb 28, 2023 at 10:18 AM Samuel Thibault <samuel.thiba...@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
