Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CAKwvOd=a+0VNP-d=mHnbM4ujdtPrujru4dxHtfDo+EEM+b6deg@mail.gmail.com>
 <20220927172839.3708280-1-ndesaulniers@google.com> <202209271143.7B7D15D@keescook>
In-Reply-To: <202209271143.7B7D15D@keescook>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Tue, 27 Sep 2022 13:01:35 -0700
Message-ID: <CAKwvOdk1QM4iZqEVj5MzTW3jMr1FiD_0C065Kub=EHKnH+xNaw@mail.gmail.com>
Subject: Re: [PATCH v2] x86, mem: move memmove to out of line assembler
To:     Kees Cook <keescook@chromium.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H . Peter Anvin" <hpa@zytor.com>,
        Peter Zijlstra <peterz@infradead.org>,
        linux-kernel@vger.kernel.org,
        Linus Torvalds <torvalds@linux-foundation.org>,
        llvm@lists.linux.dev, Andy Lutomirski <luto@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Sep 27, 2022 at 12:24 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Sep 27, 2022 at 10:28:39AM -0700, Nick Desaulniers wrote:
> > In addition to having complex constraints that can't always be resolved,
> > the clobber list seems to be missing %bx and %dx, and possibly %cl. By
> > using numbered operands rather than symbolic operands, the constraints
> > are quite obnoxious to refactor.
> > [...]
> > -             /*
> > -              * Move data from 2 bytes to 3 bytes.
> > -              */
> > -             ".p2align 4\n\t"
> > -             "9:\n\t"
> > -             "cmp $2, %0\n\t"
> > -             "jb 10f\n\t"
> > -             "movw 0*2(%1), %%dx\n\t"
> > -             "movw -1*2(%1, %0), %%bx\n\t"
> > -             "movw %%dx, 0*2(%2)\n\t"
> > -             "movw %%bx, -1*2(%2, %0)\n\t"
> > -             "jmp 11f\n\t"
> > [...]
> > +.set tmp0, %edi
> > [...]
> > +     /*
> > +      * Move data from 2 bytes to 3 bytes.
> > +      */
> > +.p2align 4
> > +.L4_byteswap:
> > +     cmpl    $2, n
> > +     jb      .Lbyteswap
> > +     movw    0*2(src), %di
> > +     movw    -1*2(src, n), %bx
> > +     movw    %dx, 0*2(dest)
> > +     movw    %bx, -1*2(dest, n)
> > +     jmp     .Ldone
>
> Found it (need to use %di instead of %dx). With this changed, the kunit
> test passes again:
>
> diff --git a/arch/x86/lib/memmove_32.S b/arch/x86/lib/memmove_32.S
> index 73314a391a72..9e33c9a1c595 100644
> --- a/arch/x86/lib/memmove_32.S
> +++ b/arch/x86/lib/memmove_32.S
> @@ -179,7 +179,7 @@ SYM_FUNC_START(memmove)
>         jb      .Lbyteswap
>         movw    0*2(src), %di
>         movw    -1*2(src, n), %bx
> -       movw    %dx, 0*2(dest)
> +       movw    %di, 0*2(dest)
>         movw    %bx, -1*2(dest, n)
>         jmp     .Ldone

That was stupid of me, I updated the scratch register operand in the
instruction 2 before the one at issue and forgot to update the operand
for the register in question, breaking the swap.

I'll use .set directives to give these more descriptive names to avoid
such a mistake.

.set tmp0w %di

then use tmp0w everywhere.  I will give names to the remaining
register operands, too.

Off thread, can you show me how to run the kunit tests, please?

Will send a v3 shortly.

>
> -Kees
>
> --
> Kees Cook


-- 
Thanks,
~Nick Desaulniers