On Wed, Jan 22, 2025 at 3:16 PM Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
>
> On 1/22/25 11:39, Tom Lendacky wrote:
> >> I think you need to do something like.
> >>
> >> alternative("wbinvd", ".byte 0xf3; wbinvd", X86_FEATURE_WBNOINVD);
> > I think "rep; wbinvd" would work as well.
>
> I don't want to bike shed this too much.
>
> But, please, no.
>
> The fact that wbnoinvd can be expressed as "rep; wbinvd" is a
> implementation detail. Exposing how it's encoded in the ISA is only
> going to add confusion. This:
>
> static __always_inline void wbnoinvd(void)
> {
> asm volatile(".byte 0xf3,0x0f,0x09\n\t": : :"memory");
> }
>
> is perfectly fine and a billion times less confusing than:
>
> asm volatile(".byte 0xf3; wbinvd\n\t": : :"memory");
> or
> asm volatile("rep; wbinvd\n\t": : :"memory");
>
> It only gets worse if it's mixed in an alternative() with the _actual_
> wbinvd.
Works for me. I will explicitly use 0xf3,0x0f,0x09 in v5, which I will
send shortly.
> BTW, I don't think you should be compelled to use alternative() as
> opposed to a good old:
>
> if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
> ...
Agreed, though I'm leaving as alternative() for now (both because it
results in fewer checks and because that's what is used in the rest of
the file); please holler if you prefer otherwise. If so, my slight
preference in that case would be to update the whole file
stylistically in a separate commit.
