Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200128165906.GA67781@gmail.com> <CAHk-=wgm+2ac4nnprPST6CnehHXScth=A7-ayrNyhydNC+xG-g@mail.gmail.com>
 <CAHk-=wi=otQxzhLAofWEvULLMk2X3G3zcWfUWz7e1CFz+xYs2Q@mail.gmail.com>
 <20200129132618.GA30979@zn.tnic> <20200129170725.GA21265@agluck-desk2.amr.corp.intel.com>
In-Reply-To: <20200129170725.GA21265@agluck-desk2.amr.corp.intel.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed, 29 Jan 2020 09:40:58 -0800
Message-ID: <CAHk-=wgns2Tvph77XZWN=r_qAtUwxrTzDXNffi8nGKz1mLZNHw@mail.gmail.com>
Subject: Re: [GIT PULL] x86/asm changes for v5.6
To:     "Luck, Tony" <tony.luck@intel.com>
Cc:     Borislav Petkov <bp@suse.de>, Ingo Molnar <mingo@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Borislav Petkov <bp@alien8.de>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        Andrew Morton <akpm@linux-foundation.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jan 29, 2020 at 9:07 AM Luck, Tony <tony.luck@intel.com> wrote:
>
> This returns "3" ... not what we want. But swapping the ERMS/FSRM order
> gets the correct version.

That actually makes sense, and is what I suspected (after I wrote the
patch) what would happen. It would just be good to have it explicitly
documented at the macro.

> > And yes, your idea makes sense to use ALTERNATIVE_2 but as it is, it
> > triple-faults my guest. I'll debug it more later to find out why, when I
> > get a chance.
>
> Triple fault is a surprise.  As long as you have ERMS, it shouldn't
> hurt to take the FSRM code path.
>
> Does the code that performs the patch use memmove() to copy the alternate
> version into place? That could get ugly!

That would be bad indeed, but I don't think it should matter
particularly for this case - it would have been bad before too.

I suspect there is some other issue going on, like the NOP padding
logic being confused.

In particular, look here, this is the alt_instruction entries:

   altinstruction_entry 140b,143f,\feature1,142b-140b,144f-143f,142b-141b
   altinstruction_entry 140b,144f,\feature2,142b-140b,145f-144f,142b-141b

where the arguments are "orig alt feature orig_len alt_len pad_len" in
that order.

Notice how "pad_len" in both cases is the padding from the _original_
instruction (142b-141b).

So what happens when all the three alternatives are different sizes?
In particular, we have

 (a) first 'orig' with 'orig_pad'

 (b) then we do the first feature replacement, and we use that correct
original padding

 (c) then we do the _second_ feature replacemement - except now
'orig_len, pad_len' doesn't actually match what the code is, because
we've done that first replacement.

I still don't see why it would be a problem, really, because the two
replacement sequences don't actually care about the padding (they both
end with a 'ret', so you don't need to get the padding nops right),
but I wonder if this casues confusion.

I do note that all the existing uses of ALTERNATIVE_2 in asm code that
might have this issue (REP_GOOD vs ERMS) has an empty instruction in
the middle, and the final instruction is the same size as the
original. So _if_ there is some padding confusion in alternatives
handling, it might easily have been hidden by that.

So I'm just hand-waving. Maybe there was some simpler explanation
(like me just picking the wrong instructions when I did the rough
conversion and simply breaking things with some stupid bug).

               Linus