Re: Backport 44623b2818f4a442726639572f44fd9b6d0ef68c to kernel 5.4

Re: Backport 44623b2818f4a442726639572f44fd9b6d0ef68c to kernel 5.4

[Greg KH]

On Mon, Oct 26, 2020 at 06:17:00PM -0700, Jian Cai wrote:
> Hello,
> 
> I am working on assembling kernel 5.4 with LLVM's integrated assembler on
> ChromeOS, and the following patch is required to make it work. Would you
> please consider backporting it to 5.4?
> 
> 
> commit 44623b2818f4a442726639572f44fd9b6d0ef68c
> Author: Arnd Bergmann <arnd@arndb.de>
> Date:   Wed May 27 16:17:40 2020 +0200
> 
>     crypto: x86/crc32c - fix building with clang ias
> 
> Link:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=44623b2818f4a442726639572f44fd9b6d0ef68c
> 

It does not apply cleanly, can you please provide a properly backported
and tested version?

thanks,

greg k-h

Re: Backport 44623b2818f4a442726639572f44fd9b6d0ef68c to kernel 5.4

[Nick Desaulniers]

On Thu, Oct 29, 2020 at 4:01 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Oct 26, 2020 at 06:17:00PM -0700, Jian Cai wrote:
> > Hello,
> >
> > I am working on assembling kernel 5.4 with LLVM's integrated assembler on
> > ChromeOS, and the following patch is required to make it work. Would you
> > please consider backporting it to 5.4?
> >
> >
> > commit 44623b2818f4a442726639572f44fd9b6d0ef68c
> > Author: Arnd Bergmann <arnd@arndb.de>
> > Date:   Wed May 27 16:17:40 2020 +0200
> >
> >     crypto: x86/crc32c - fix building with clang ias
> >
> > Link:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=44623b2818f4a442726639572f44fd9b6d0ef68c
> >
>
> It does not apply cleanly, can you please provide a properly backported
> and tested version?

Hi Jian,
Thanks for proactively identifying and requesting a backport of
44623b2818.  We'll need it for Android as well soon.

One thing I do when requesting backports from stable is I checkout the
branch of the stable tree and see if the patch cherry picks cleanly.

stable is its own tree; you could add it as a remote or check out a
copy of it.  If you go to kernel.org, click any stable branch, go to
summary tab in top left, scroll down for the git url to clone.  Then
you can `git checkout -b linux-5.4.y origin/linux-5.4.y` to setup the
branch.  Fetch/pull so it's up to date, then `git cherry-pick -x
<sha>`. If it applies without conflict, you can simply send an email
as you've done.

If it does not, then you should fix up the conflict, and test it.
Then you add your signed off by tag (`git commit --amend -s`) and
sometime people leave a note like `[<initials>: had to drop a hunk
because a packport of <upstream sha> doesn't exist in this branch]`.
If you `git log` in a stable tree's branch, you should be able to see
examples.

Another thing I like to do is include comments (in the request email,
not the commit message of the patch) on my risk assessment and what
the first version of the mainline tree the patch landed in.  In a
mainline tree (not stable tree), I run this function I've added to my
shell's rc file:
first_tag () {
        tag=$1
        git describe --match 'v*' --contains "$tag" | sed 's/~.*//'
}

$ first_tag <sha>
That information can help the stable maintainers better assess the
risks in accepting the backport.  Though I'm not always great about
sticking to my own advice in this regard; the last request I made I
forgot to include info about the upstream version.  But
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
doesn't mention that's actually necessary.

Anyways I'm sure you already knew most of the above; I don't mean to
patronize.  My apologies in that regard.  Having it typed out helps me
forward a URL to future travelers.
-- 
Thanks,
~Nick Desaulniers

Re: Backport 44623b2818f4a442726639572f44fd9b6d0ef68c to kernel 5.4

[Sasha Levin]

On Thu, Oct 29, 2020 at 11:05:01AM -0700, Nick Desaulniers wrote:
>On Thu, Oct 29, 2020 at 4:01 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>> On Mon, Oct 26, 2020 at 06:17:00PM -0700, Jian Cai wrote:
>> > Hello,
>> >
>> > I am working on assembling kernel 5.4 with LLVM's integrated assembler on
>> > ChromeOS, and the following patch is required to make it work. Would you
>> > please consider backporting it to 5.4?
>> >
>> >
>> > commit 44623b2818f4a442726639572f44fd9b6d0ef68c
>> > Author: Arnd Bergmann <arnd@arndb.de>
>> > Date:   Wed May 27 16:17:40 2020 +0200
>> >
>> >     crypto: x86/crc32c - fix building with clang ias
>> >
>> > Link:
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=44623b2818f4a442726639572f44fd9b6d0ef68c
>> >
>>
>> It does not apply cleanly, can you please provide a properly backported
>> and tested version?
>
>Hi Jian,
>Thanks for proactively identifying and requesting a backport of
>44623b2818.  We'll need it for Android as well soon.
>
>One thing I do when requesting backports from stable is I checkout the
>branch of the stable tree and see if the patch cherry picks cleanly.

btw, an easy way to get an idea of possible dependencies is to look at
the dependency repo :) For this commit on 5.4:

https://git.kernel.org/pub/scm/linux/kernel/git/sashal/deps.git/plain/v5.4/44623b2818f4a442726639572f44fd9b6d0ef68c

-- 
Thanks,
Sasha

Re: Backport 44623b2818f4a442726639572f44fd9b6d0ef68c to kernel 5.4

[Nick Desaulniers]

On Thu, Oct 29, 2020 at 4:36 PM Sasha Levin <sashal@kernel.org> wrote:
>
> On Thu, Oct 29, 2020 at 11:05:01AM -0700, Nick Desaulniers wrote:
> >Hi Jian,
> >Thanks for proactively identifying and requesting a backport of
> >44623b2818.  We'll need it for Android as well soon.
> >
> >One thing I do when requesting backports from stable is I checkout the
> >branch of the stable tree and see if the patch cherry picks cleanly.
>
> btw, an easy way to get an idea of possible dependencies is to look at
> the dependency repo :) For this commit on 5.4:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/sashal/deps.git/plain/v5.4/44623b2818f4a442726639572f44fd9b6d0ef68c

Why you guys never tell me this before? :P Very cool, how is the
dependency chain built? Is it built for every commit?
-- 
Thanks,
~Nick Desaulniers

Re: Backport 44623b2818f4a442726639572f44fd9b6d0ef68c to kernel 5.4

[Sasha Levin]

On Thu, Oct 29, 2020 at 04:45:52PM -0700, Nick Desaulniers wrote:
>On Thu, Oct 29, 2020 at 4:36 PM Sasha Levin <sashal@kernel.org> wrote:
>>
>> On Thu, Oct 29, 2020 at 11:05:01AM -0700, Nick Desaulniers wrote:
>> >Hi Jian,
>> >Thanks for proactively identifying and requesting a backport of
>> >44623b2818.  We'll need it for Android as well soon.
>> >
>> >One thing I do when requesting backports from stable is I checkout the
>> >branch of the stable tree and see if the patch cherry picks cleanly.
>>
>> btw, an easy way to get an idea of possible dependencies is to look at
>> the dependency repo :) For this commit on 5.4:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/sashal/deps.git/plain/v5.4/44623b2818f4a442726639572f44fd9b6d0ef68c
>
>Why you guys never tell me this before? :P Very cool, how is the
>dependency chain built? Is it built for every commit?

git bisect run for each commit on each branch we have. I have a little
stable-deps tool that looks something like this to make it easy:

ver=$(make SUBLEVEL= kernelversion)
cmt=$(git rev-parse $1)

for i in $(curl -s https://git.kernel.org/pub/scm/linux/kernel/git/sashal/deps.git/plain/v$ver/$cmt | awk {'print $1'}); do
         stable commit-in-tree $i
         if [ $? -eq 1 ]; then
                 continue
         fi
         git ol $i
done

-- 
Thanks,
Sasha

Re: Backport 44623b2818f4a442726639572f44fd9b6d0ef68c to kernel 5.4

[Nathan Chancellor]

Hi Jian,

On Thu, Oct 29, 2020 at 06:13:07PM -0700, 'Jian Cai' via Clang Built Linux wrote:
> Thanks @Nick Desaulniers <ndesaulniers@google.com>  and @Sasha Levin
> <sashal@kernel.org> for the tips. For this particular change, it seems we
> do not need to backport all the dependencies (if they have not been merged
> into 5.4 stable). @Greg KH <gregkh@linuxfoundation.org>, please find the
> custom backport as below. It has passed all the tests on ChromeOS (
> http://crrev.com/c/2504570).
> 
> Thanks,
> Jian

The below patch won't apply because it appears to be copy pasted into
this message:

Applying: Backport 44623b2818f4a442726639572f44fd9b6d0ef68c to kernel 5.4
error: git diff header lacks filename information when removing 1 leading pathname component (line 6)
Patch failed at 0001 Backport 44623b2818f4a442726639572f44fd9b6d0ef68c to kernel 5.4
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

I would recommend resending the patch with git send-email or attaching
the patch file created by 'git format-patch -1' to a future email for
proper application.

> From 60891062750a39d8bba9710d500e381a26c11f75 Mon Sep 17 00:00:00 2001
> From: Jian Cai <jiancai@google.com>
> Date: Thu, 29 Oct 2020 17:49:39 -0700

Authorship and date should be fixed to retain the information of the
original commit.

It is trivial to just redo the cherry-pick to fix that information in
this instance but this is the command I usually run for more non-trivial
backports that I have already done:

$ git commit -s --amend -C <sha> --date "$(git show -s --format=%ai <sha>)"

This should allow you to retain the commit message of the original
message along with the author's date.

> Subject: [PATCH] crypto: x86/crc32c - fix building with clang ias
> 
> commit 44623b2818f4a442726639572f44fd9b6d0ef68c upstream
> 
> The clang integrated assembler complains about movzxw:
> 
> arch/x86/crypto/crc32c-pcl-intel-asm_64.S:173:2: error: invalid instruction
> mnemonic 'movzxw'
> 
> It seems that movzwq is the mnemonic that it expects instead,
> and this is what objdump prints when disassembling the file.
> 
> NOTE: this is a custom backport as the surrounding code has been
> changed upstream.

A note of this nature is traditionally placed after the signoffs of the
original patch like my example below:

> Fixes: 6a8ce1ef3940 ("crypto: crc32c - Optimize CRC32C calculation with
> PCLMULQDQ instruction")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
[jc: Backport to 5.4]
> Signed-off-by: Jian Cai <caij2003@gmail.com>

I usually like to notate why the patch did not apply cleanly so that it
is easier for others to audit, such as:

[jc: Fixed conflicts due to lack of 34fdce6981b969]

That is merely a suggestion, not required by any means.

Otherwise, the backport seems obvious fine to me.

Cheers,
Nathan

> ---
>  arch/x86/crypto/crc32c-pcl-intel-asm_64.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/crypto/crc32c-pcl-intel-asm_64.S
> b/arch/x86/crypto/crc32c-pcl-intel-asm_64.S
> index d9b734d0c8cc..3c6e01520a97 100644
> --- a/arch/x86/crypto/crc32c-pcl-intel-asm_64.S
> +++ b/arch/x86/crypto/crc32c-pcl-intel-asm_64.S
> @@ -170,7 +170,7 @@ continue_block:
> 
>   ## branch into array
>   lea jump_table(%rip), bufp
> - movzxw  (bufp, %rax, 2), len
> + movzwq  (bufp, %rax, 2), len
>   lea crc_array(%rip), bufp
>   lea     (bufp, len, 1), bufp
>   JMP_NOSPEC bufp
> -- 
> 2.29.1.341.ge80a0c044ae-goog

Re: [PATCH] crypto: x86/crc32c - fix building with clang ias

[Nick Desaulniers]

+ stable

On Fri, Oct 30, 2020 at 12:04 PM Jian Cai <caij2003@gmail.com> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> commit 44623b2818f4 ("crypto: x86/crc32c - fix building with clang ias")
> upstream
>
> The clang integrated assembler complains about movzxw:
>
> arch/x86/crypto/crc32c-pcl-intel-asm_64.S:173:2: error: invalid instruction mnemonic 'movzxw'
>
> It seems that movzwq is the mnemonic that it expects instead,
> and this is what objdump prints when disassembling the file.
>
> Fixes: 6a8ce1ef3940 ("crypto: crc32c - Optimize CRC32C calculation with PCLMULQDQ instruction")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> [jc: Fixed conflicts due to lack of 34fdce6981b969]
> Signed-off-by: Jian Cai <jiancai@google.com>
> ---
>
> Thanks Nathan! This patch addresses Nathan's comments regarding format
> and note.
>
>
>  arch/x86/crypto/crc32c-pcl-intel-asm_64.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/crypto/crc32c-pcl-intel-asm_64.S b/arch/x86/crypto/crc32c-pcl-intel-asm_64.S
> index d9b734d0c8cc..3c6e01520a97 100644
> --- a/arch/x86/crypto/crc32c-pcl-intel-asm_64.S
> +++ b/arch/x86/crypto/crc32c-pcl-intel-asm_64.S
> @@ -170,7 +170,7 @@ continue_block:
>
>         ## branch into array
>         lea     jump_table(%rip), bufp
> -       movzxw  (bufp, %rax, 2), len
> +       movzwq  (bufp, %rax, 2), len
>         lea     crc_array(%rip), bufp
>         lea     (bufp, len, 1), bufp
>         JMP_NOSPEC bufp
> --
> 2.29.1.341.ge80a0c044ae-goog
>


-- 
Thanks,
~Nick Desaulniers

Re: Backport 44623b2818f4a442726639572f44fd9b6d0ef68c to kernel 5.4

[Nick Desaulniers]

On Fri, Oct 30, 2020 at 12:24 PM Jian Cai <jiancai@google.com> wrote:
>
> Hi Nathan,
>
> Thanks for all the tips! I have fixed the issues mentioned in your comments and used git send-email to resend the patch as recommended. FYI I used the Message ID of this thread but it created a new thread anyway.

No, I'll bet you're using gmail which has issues showing threads when
the subject is changed or is not `Re: <old subject>`. If you look at
lore, it's correct:
https://lore.kernel.org/stable/20201030014930.GB2519055@ubuntu-m3-large-x86/T/#t
Just that you forgot to cc stable. :^P  Don't worry about it; I forget
to do that still myself.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] crypto: x86/crc32c - fix building with clang ias

[Greg KH]

On Fri, Oct 30, 2020 at 12:06:28PM -0700, Nick Desaulniers wrote:
> + stable
> 
> On Fri, Oct 30, 2020 at 12:04 PM Jian Cai <caij2003@gmail.com> wrote:
> >
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > commit 44623b2818f4 ("crypto: x86/crc32c - fix building with clang ias")
> > upstream
> >
> > The clang integrated assembler complains about movzxw:
> >
> > arch/x86/crypto/crc32c-pcl-intel-asm_64.S:173:2: error: invalid instruction mnemonic 'movzxw'
> >
> > It seems that movzwq is the mnemonic that it expects instead,
> > and this is what objdump prints when disassembling the file.
> >
> > Fixes: 6a8ce1ef3940 ("crypto: crc32c - Optimize CRC32C calculation with PCLMULQDQ instruction")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> > [jc: Fixed conflicts due to lack of 34fdce6981b969]

Nit, please spell out commit ids as the documentation asks you to.  I've
edited it and done that now...

thanks,

greg k-h