Fwd: s390/nospec: add an option to use thunk-extern

Fwd: s390/nospec: add an option to use thunk-extern

[Joe Lawrence]

[ adding appropriate lists ]

-------- Forwarded Message --------
Subject: Re: s390/nospec: add an option to use thunk-extern
Date: Thu, 2 Jun 2022 08:02:20 -0400
From: Joe Lawrence <joe.lawrence@redhat.com>
To: Vasily Gorbik <gor@linux.ibm.com>

Hi Vasily,

I couldn't find the upstream patch post for 1d2ad084800e ("s390/nospec:
add an option to use thunk-extern"), so replying off-list here.  Feel
free to cc the appropriate list.

Regarding this change, as I understand it, when CONFIG_EXPOLINE_EXTERN=y
out-of-tree kernel modules will need to link against
arch/s390x/lib/expoline.o, right?

And if so, shouldn't the top level 'prepare_modules' target create
expoline.o for this purpose?

Regards,
-- 
Joe

Re: s390/nospec: add an option to use thunk-extern

[Vasily Gorbik]

Hi Joe,

sorry for late reply.

> I couldn't find the upstream patch post for 1d2ad084800e ("s390/nospec:
> add an option to use thunk-extern"), so replying off-list here.  Feel
> free to cc the appropriate list.
> 
> Regarding this change, as I understand it, when CONFIG_EXPOLINE_EXTERN=y
> out-of-tree kernel modules will need to link against
> arch/s390x/lib/expoline.o, right?
> 
> And if so, shouldn't the top level 'prepare_modules' target create
> expoline.o for this purpose?

Thanks for bringing this up. I definitely missed out-of-tree kernel modules
build case without a prebuilt kernel. On the other hand this post-linking
trick is a rip off from powerpc:

KBUILD_LDFLAGS_MODULE += arch/powerpc/lib/crtsavres.o

So, now I wonder why powerpc doesn't have crtsavres.o in 'prepare_modules'.

Anyhow, below is couple of patches to consider. The first one is
meant to be backportable, as the second one requires 4efd417f298b.

I had to move expoline.S to a separate directory to be able to call into
its Makefile for 'prepare_modules' and avoid warnings for other targets
defined in the same Makefile. Not sure if there are better kbuild tricks
I could use. Another option I thought about is to keep expoline.S where
it is and add a condition into that Makefile:
expoline_prepare: prepare0
	$(Q)$(MAKE) $(build)=arch/s390/lib expoline_prepare=1 arch/s390/lib/expoline.o

arch/s390/lib/Makefile:
# first target defined
obj-$(CONFIG_EXPOLINE_EXTERN) += expoline.o
ifndef expoline_prepare
# ...other targets...

Vasily Gorbik (2):
  s390/nospec: build expoline.o for modules_prepare target
  s390/nospec: remove unneeded header includes

 arch/s390/Makefile                      | 8 +++++++-
 arch/s390/include/asm/nospec-insn.h     | 2 --
 arch/s390/lib/Makefile                  | 3 ++-
 arch/s390/lib/expoline/Makefile         | 3 +++
 arch/s390/lib/{ => expoline}/expoline.S | 0
 5 files changed, 12 insertions(+), 4 deletions(-)
 create mode 100644 arch/s390/lib/expoline/Makefile
 rename arch/s390/lib/{ => expoline}/expoline.S (100%)

-- 
2.35.1

[PATCH 1/2] s390/nospec: build expoline.o for modules_prepare target

[Vasily Gorbik]

When CONFIG_EXPOLINE_EXTERN is used expoline thunks are generated
from arch/s390/lib/expoline.S and postlinked into every module.
This is also true for external modules. Add expoline.o build to
the modules_prepare target.

Fixes: 1d2ad084800e ("s390/nospec: add an option to use thunk-extern")
Reported-by: Joe Lawrence <joe.lawrence@redhat.com>
Tested-by: Sumanth Korikkar <sumanthk@linux.ibm.com>
Acked-by: Sumanth Korikkar <sumanthk@linux.ibm.com>
Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
---
 arch/s390/Makefile                      | 8 +++++++-
 arch/s390/lib/Makefile                  | 3 ++-
 arch/s390/lib/expoline/Makefile         | 3 +++
 arch/s390/lib/{ => expoline}/expoline.S | 0
 4 files changed, 12 insertions(+), 2 deletions(-)
 create mode 100644 arch/s390/lib/expoline/Makefile
 rename arch/s390/lib/{ => expoline}/expoline.S (100%)

diff --git a/arch/s390/Makefile b/arch/s390/Makefile
index 495c68a4df1e..fc72a35a1f07 100644
--- a/arch/s390/Makefile
+++ b/arch/s390/Makefile
@@ -82,7 +82,7 @@ endif
 
 ifdef CONFIG_EXPOLINE
   ifdef CONFIG_EXPOLINE_EXTERN
-    KBUILD_LDFLAGS_MODULE += arch/s390/lib/expoline.o
+    KBUILD_LDFLAGS_MODULE += arch/s390/lib/expoline/expoline.o
     CC_FLAGS_EXPOLINE := -mindirect-branch=thunk-extern
     CC_FLAGS_EXPOLINE += -mfunction-return=thunk-extern
   else
@@ -163,6 +163,12 @@ vdso_prepare: prepare0
 	$(Q)$(MAKE) $(build)=arch/s390/kernel/vdso64 include/generated/vdso64-offsets.h
 	$(if $(CONFIG_COMPAT),$(Q)$(MAKE) \
 		$(build)=arch/s390/kernel/vdso32 include/generated/vdso32-offsets.h)
+
+ifdef CONFIG_EXPOLINE_EXTERN
+modules_prepare: expoline_prepare
+expoline_prepare: prepare0
+	$(Q)$(MAKE) $(build)=arch/s390/lib/expoline arch/s390/lib/expoline/expoline.o
+endif
 endif
 
 # Don't use tabs in echo arguments
diff --git a/arch/s390/lib/Makefile b/arch/s390/lib/Makefile
index 5d415b3db6d1..580d2e3265cb 100644
--- a/arch/s390/lib/Makefile
+++ b/arch/s390/lib/Makefile
@@ -7,7 +7,6 @@ lib-y += delay.o string.o uaccess.o find.o spinlock.o
 obj-y += mem.o xor.o
 lib-$(CONFIG_KPROBES) += probes.o
 lib-$(CONFIG_UPROBES) += probes.o
-obj-$(CONFIG_EXPOLINE_EXTERN) += expoline.o
 obj-$(CONFIG_S390_KPROBES_SANITY_TEST) += test_kprobes_s390.o
 test_kprobes_s390-objs += test_kprobes_asm.o test_kprobes.o
 
@@ -22,3 +21,5 @@ obj-$(CONFIG_S390_MODULES_SANITY_TEST) += test_modules.o
 obj-$(CONFIG_S390_MODULES_SANITY_TEST_HELPERS) += test_modules_helpers.o
 
 lib-$(CONFIG_FUNCTION_ERROR_INJECTION) += error-inject.o
+
+obj-$(CONFIG_EXPOLINE_EXTERN) += expoline/
diff --git a/arch/s390/lib/expoline/Makefile b/arch/s390/lib/expoline/Makefile
new file mode 100644
index 000000000000..854631d9cb03
--- /dev/null
+++ b/arch/s390/lib/expoline/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-y += expoline.o
diff --git a/arch/s390/lib/expoline.S b/arch/s390/lib/expoline/expoline.S
similarity index 100%
rename from arch/s390/lib/expoline.S
rename to arch/s390/lib/expoline/expoline.S
-- 
2.35.1

[PATCH 2/2] s390/nospec: remove unneeded header includes

[Vasily Gorbik]

Commit 4efd417f298b ("s390: raise minimum supported machine generation
to z10") removed the usage of alternatives and lowcore in expolines
macros. Remove unneeded header includes as well.

With that, expoline.S doesn't require asm-offsets.h and
expoline_prepare target dependency could be removed.

Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
---
 arch/s390/Makefile                  | 2 +-
 arch/s390/include/asm/nospec-insn.h | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/s390/Makefile b/arch/s390/Makefile
index fc72a35a1f07..4cb5d17e7ead 100644
--- a/arch/s390/Makefile
+++ b/arch/s390/Makefile
@@ -166,7 +166,7 @@ vdso_prepare: prepare0
 
 ifdef CONFIG_EXPOLINE_EXTERN
 modules_prepare: expoline_prepare
-expoline_prepare: prepare0
+expoline_prepare:
 	$(Q)$(MAKE) $(build)=arch/s390/lib/expoline arch/s390/lib/expoline/expoline.o
 endif
 endif
diff --git a/arch/s390/include/asm/nospec-insn.h b/arch/s390/include/asm/nospec-insn.h
index d910d71b5bb5..7e9e99523e95 100644
--- a/arch/s390/include/asm/nospec-insn.h
+++ b/arch/s390/include/asm/nospec-insn.h
@@ -2,8 +2,6 @@
 #ifndef _ASM_S390_NOSPEC_ASM_H
 #define _ASM_S390_NOSPEC_ASM_H
 
-#include <asm/alternative-asm.h>
-#include <asm/asm-offsets.h>
 #include <asm/dwarf.h>
 
 #ifdef __ASSEMBLY__
-- 
2.35.1

Re: s390/nospec: add an option to use thunk-extern

[Joe Lawrence]

On 6/27/22 8:50 AM, Vasily Gorbik wrote:
> Hi Joe,
> 
> sorry for late reply.
> 
>> I couldn't find the upstream patch post for 1d2ad084800e ("s390/nospec:
>> add an option to use thunk-extern"), so replying off-list here.  Feel
>> free to cc the appropriate list.
>>
>> Regarding this change, as I understand it, when CONFIG_EXPOLINE_EXTERN=y
>> out-of-tree kernel modules will need to link against
>> arch/s390x/lib/expoline.o, right?
>>
>> And if so, shouldn't the top level 'prepare_modules' target create
>> expoline.o for this purpose?
> 
> Thanks for bringing this up. I definitely missed out-of-tree kernel modules
> build case without a prebuilt kernel. On the other hand this post-linking
> trick is a rip off from powerpc:
> 
> KBUILD_LDFLAGS_MODULE += arch/powerpc/lib/crtsavres.o
> 
> So, now I wonder why powerpc doesn't have crtsavres.o in 'prepare_modules'.
> 
> Anyhow, below is couple of patches to consider. The first one is
> meant to be backportable, as the second one requires 4efd417f298b.
> 
> I had to move expoline.S to a separate directory to be able to call into
> its Makefile for 'prepare_modules' and avoid warnings for other targets
> defined in the same Makefile. Not sure if there are better kbuild tricks
> I could use. Another option I thought about is to keep expoline.S where
> it is and add a condition into that Makefile:
> expoline_prepare: prepare0
> 	$(Q)$(MAKE) $(build)=arch/s390/lib expoline_prepare=1 arch/s390/lib/expoline.o
> 
> arch/s390/lib/Makefile:
> # first target defined
> obj-$(CONFIG_EXPOLINE_EXTERN) += expoline.o
> ifndef expoline_prepare
> # ...other targets...
> 
> Vasily Gorbik (2):
>   s390/nospec: build expoline.o for modules_prepare target
>   s390/nospec: remove unneeded header includes
> 
>  arch/s390/Makefile                      | 8 +++++++-
>  arch/s390/include/asm/nospec-insn.h     | 2 --
>  arch/s390/lib/Makefile                  | 3 ++-
>  arch/s390/lib/expoline/Makefile         | 3 +++
>  arch/s390/lib/{ => expoline}/expoline.S | 0
>  5 files changed, 12 insertions(+), 4 deletions(-)
>  create mode 100644 arch/s390/lib/expoline/Makefile
>  rename arch/s390/lib/{ => expoline}/expoline.S (100%)
> 

Thanks, Vasily.  We'll test these with OOT and the original gitlab
pipeline where we spotted potential issue with packaging and report back.

-- 
Joe

Re: s390/nospec: add an option to use thunk-extern

[Joe Lawrence]

On 7/1/22 5:18 PM, C. Erastus Toe wrote:
> On Wed, Jun 29, 2022 at 11:16 AM Joe Lawrence <joe.lawrence@redhat.com
> <mailto:joe.lawrence@redhat.com>> wrote:
> 
>     On 6/27/22 8:50 AM, Vasily Gorbik wrote:
>     > Hi Joe,
>     >
>     > sorry for late reply.
>     >
>     >> I couldn't find the upstream patch post for 1d2ad084800e
>     ("s390/nospec:
>     >> add an option to use thunk-extern"), so replying off-list here.  Feel
>     >> free to cc the appropriate list.
>     >>
>     >> Regarding this change, as I understand it, when
>     CONFIG_EXPOLINE_EXTERN=y
>     >> out-of-tree kernel modules will need to link against
>     >> arch/s390x/lib/expoline.o, right?
>     >>
>     >> And if so, shouldn't the top level 'prepare_modules' target create
>     >> expoline.o for this purpose?
>     >
>     > Thanks for bringing this up. I definitely missed out-of-tree
>     kernel modules
>     > build case without a prebuilt kernel. On the other hand this
>     post-linking
>     > trick is a rip off from powerpc:
>     >
>     > KBUILD_LDFLAGS_MODULE += arch/powerpc/lib/crtsavres.o
>     >
>     > So, now I wonder why powerpc doesn't have crtsavres.o in
>     'prepare_modules'.
>     >
>     > Anyhow, below is couple of patches to consider. The first one is
>     > meant to be backportable, as the second one requires 4efd417f298b.
>     >
>     > I had to move expoline.S to a separate directory to be able to
>     call into
>     > its Makefile for 'prepare_modules' and avoid warnings for other
>     targets
>     > defined in the same Makefile. Not sure if there are better kbuild
>     tricks
>     > I could use. Another option I thought about is to keep expoline.S
>     where
>     > it is and add a condition into that Makefile:
>     > expoline_prepare: prepare0
>     >       $(Q)$(MAKE) $(build)=arch/s390/lib expoline_prepare=1
>     arch/s390/lib/expoline.o
>     >
>     > arch/s390/lib/Makefile:
>     > # first target defined
>     > obj-$(CONFIG_EXPOLINE_EXTERN) += expoline.o
>     > ifndef expoline_prepare
>     > # ...other targets...
>     >
>     > Vasily Gorbik (2):
>     >   s390/nospec: build expoline.o for modules_prepare target
>     >   s390/nospec: remove unneeded header includes
>     >
>     >  arch/s390/Makefile                      | 8 +++++++-
>     >  arch/s390/include/asm/nospec-insn.h     | 2 --
>     >  arch/s390/lib/Makefile                  | 3 ++-
>     >  arch/s390/lib/expoline/Makefile         | 3 +++
>     >  arch/s390/lib/{ => expoline}/expoline.S | 0
>     >  5 files changed, 12 insertions(+), 4 deletions(-)
>     >  create mode 100644 arch/s390/lib/expoline/Makefile
>     >  rename arch/s390/lib/{ => expoline}/expoline.S (100%)
>     >
> 
>     Thanks, Vasily.  We'll test these with OOT and the original gitlab
>     pipeline where we spotted potential issue with packaging and report
>     back.
> 
> Hi, 
> 
> Successfully tested the first patch in a rhel-9 backport. (had to skip
> the second as it has dependencies on other patches like [1] that
> deprecated symbols like __LC_BR_R1. Without those, the build resulted in
> a flood of: depmod: WARNING: <module>.ko needs unknown symbol __LC_BR_R1.)
> 
> For ("s390/nospec: build expoline.o for modules_prepare target"), 
> Tested-by: C. Erastus Toe <ctoe@redhat.com <mailto:ctoe@redhat.com>> 
> 
> [1] 4efd417f298b ("s390: raise minimum supported machine generation to z10")
> 

And then for the entire series (tested on top of v5.19-rc4),
Tested-by: Joe Lawrence <joe.lawrence@redhat.com>

-- 
Joe

Re: s390/nospec: add an option to use thunk-extern

[Sumanth Korikkar]

Hi Joe,

c4e789572557 ("s390/nospec: build expoline.o for modules_prepare 
target") is now in linux.git.

Note: arch/s390/lib/expoline.o is moved to 
arch/s390/lib/expoline/expoline.o. This means kernel-devel package in 
fedora should also include this updated file path.

Thanks

On 7/1/22 23:39, Joe Lawrence wrote:
> On 7/1/22 5:18 PM, C. Erastus Toe wrote:
>> On Wed, Jun 29, 2022 at 11:16 AM Joe Lawrence <joe.lawrence@redhat.com
>> <mailto:joe.lawrence@redhat.com>> wrote:
>>
>>      On 6/27/22 8:50 AM, Vasily Gorbik wrote:
>>      > Hi Joe,
>>      >
>>      > sorry for late reply.
>>      >
>>      >> I couldn't find the upstream patch post for 1d2ad084800e
>>      ("s390/nospec:
>>      >> add an option to use thunk-extern"), so replying off-list here.  Feel
>>      >> free to cc the appropriate list.
>>      >>
>>      >> Regarding this change, as I understand it, when
>>      CONFIG_EXPOLINE_EXTERN=y
>>      >> out-of-tree kernel modules will need to link against
>>      >> arch/s390x/lib/expoline.o, right?
>>      >>
>>      >> And if so, shouldn't the top level 'prepare_modules' target create
>>      >> expoline.o for this purpose?
>>      >
>>      > Thanks for bringing this up. I definitely missed out-of-tree
>>      kernel modules
>>      > build case without a prebuilt kernel. On the other hand this
>>      post-linking
>>      > trick is a rip off from powerpc:
>>      >
>>      > KBUILD_LDFLAGS_MODULE += arch/powerpc/lib/crtsavres.o
>>      >
>>      > So, now I wonder why powerpc doesn't have crtsavres.o in
>>      'prepare_modules'.
>>      >
>>      > Anyhow, below is couple of patches to consider. The first one is
>>      > meant to be backportable, as the second one requires 4efd417f298b.
>>      >
>>      > I had to move expoline.S to a separate directory to be able to
>>      call into
>>      > its Makefile for 'prepare_modules' and avoid warnings for other
>>      targets
>>      > defined in the same Makefile. Not sure if there are better kbuild
>>      tricks
>>      > I could use. Another option I thought about is to keep expoline.S
>>      where
>>      > it is and add a condition into that Makefile:
>>      > expoline_prepare: prepare0
>>      >       $(Q)$(MAKE) $(build)=arch/s390/lib expoline_prepare=1
>>      arch/s390/lib/expoline.o
>>      >
>>      > arch/s390/lib/Makefile:
>>      > # first target defined
>>      > obj-$(CONFIG_EXPOLINE_EXTERN) += expoline.o
>>      > ifndef expoline_prepare
>>      > # ...other targets...
>>      >
>>      > Vasily Gorbik (2):
>>      >   s390/nospec: build expoline.o for modules_prepare target
>>      >   s390/nospec: remove unneeded header includes
>>      >
>>      >  arch/s390/Makefile                      | 8 +++++++-
>>      >  arch/s390/include/asm/nospec-insn.h     | 2 --
>>      >  arch/s390/lib/Makefile                  | 3 ++-
>>      >  arch/s390/lib/expoline/Makefile         | 3 +++
>>      >  arch/s390/lib/{ => expoline}/expoline.S | 0
>>      >  5 files changed, 12 insertions(+), 4 deletions(-)
>>      >  create mode 100644 arch/s390/lib/expoline/Makefile
>>      >  rename arch/s390/lib/{ => expoline}/expoline.S (100%)
>>      >
>>
>>      Thanks, Vasily.  We'll test these with OOT and the original gitlab
>>      pipeline where we spotted potential issue with packaging and report
>>      back.
>>
>> Hi,
>>
>> Successfully tested the first patch in a rhel-9 backport. (had to skip
>> the second as it has dependencies on other patches like [1] that
>> deprecated symbols like __LC_BR_R1. Without those, the build resulted in
>> a flood of: depmod: WARNING: <module>.ko needs unknown symbol __LC_BR_R1.)
>>
>> For ("s390/nospec: build expoline.o for modules_prepare target"),
>> Tested-by: C. Erastus Toe <ctoe@redhat.com <mailto:ctoe@redhat.com>>
>>
>> [1] 4efd417f298b ("s390: raise minimum supported machine generation to z10")
>>
> 
> And then for the entire series (tested on top of v5.19-rc4),
> Tested-by: Joe Lawrence <joe.lawrence@redhat.com>
> 

-- 
Sumanth

Re: s390/nospec: add an option to use thunk-extern

[Joe Lawrence]

On 7/17/22 09:11, Sumanth Korikkar wrote:
> Hi Joe,
> 
> c4e789572557 ("s390/nospec: build expoline.o for modules_prepare
> target") is now in linux.git.
> 
> Note: arch/s390/lib/expoline.o is moved to
> arch/s390/lib/expoline/expoline.o. This means kernel-devel package in
> fedora should also include this updated file path.
> 

To follow up on the last part:

* Mon Aug 15 2022 Fedora Kernel Team <kernel-team@fedoraproject.org>
[6.0.0-0.rc1.12]
...
- kernel.spec.template: update (s390x) expoline.o path (Joe Lawrence)


-- 
Joe

Re: [PATCH 2/2] s390/nospec: remove unneeded header includes

[Jiri Slaby]

On 27. 06. 22, 14:50, Vasily Gorbik wrote:
> Commit 4efd417f298b ("s390: raise minimum supported machine generation
> to z10") removed the usage of alternatives and lowcore in expolines
> macros. Remove unneeded header includes as well.
> 
> With that, expoline.S doesn't require asm-offsets.h and
> expoline_prepare target dependency could be removed.
> 
> Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
> ---
>   arch/s390/Makefile                  | 2 +-
>   arch/s390/include/asm/nospec-insn.h | 2 --
>   2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/s390/Makefile b/arch/s390/Makefile
> index fc72a35a1f07..4cb5d17e7ead 100644
> --- a/arch/s390/Makefile
> +++ b/arch/s390/Makefile
> @@ -166,7 +166,7 @@ vdso_prepare: prepare0
>   
>   ifdef CONFIG_EXPOLINE_EXTERN
>   modules_prepare: expoline_prepare
> -expoline_prepare: prepare0
> +expoline_prepare:

Hi,

this likely broke s390 build as expolines still depend on 
scripts/basic/fixdep. And build of expolines can now race with fixdep build:
      make[1]: *** Deleting file 'arch/s390/lib/expoline/expoline.o'
      /bin/sh: line 1: scripts/basic/fixdep: Permission denied
      make[1]: *** [../scripts/Makefile.build:385: 
arch/s390/lib/expoline/expoline.o] Error 126
      make: *** [../arch/s390/Makefile:166: expoline_prepare] Error 2

I returned there:
   expoline_prepare: prepare0
and it looks good so far. Maybe even:
   expoline_prepare: scripts
would be enough.

Opinions?

thanks,
-- 
js
suse labs

Re: [PATCH 2/2] s390/nospec: remove unneeded header includes

[Masahiro Yamada]

On Thu, Mar 16, 2023 at 8:14 PM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 27. 06. 22, 14:50, Vasily Gorbik wrote:
> > Commit 4efd417f298b ("s390: raise minimum supported machine generation
> > to z10") removed the usage of alternatives and lowcore in expolines
> > macros. Remove unneeded header includes as well.
> >
> > With that, expoline.S doesn't require asm-offsets.h and
> > expoline_prepare target dependency could be removed.
> >
> > Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
> > ---
> >   arch/s390/Makefile                  | 2 +-
> >   arch/s390/include/asm/nospec-insn.h | 2 --
> >   2 files changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/arch/s390/Makefile b/arch/s390/Makefile
> > index fc72a35a1f07..4cb5d17e7ead 100644
> > --- a/arch/s390/Makefile
> > +++ b/arch/s390/Makefile
> > @@ -166,7 +166,7 @@ vdso_prepare: prepare0
> >
> >   ifdef CONFIG_EXPOLINE_EXTERN
> >   modules_prepare: expoline_prepare
> > -expoline_prepare: prepare0
> > +expoline_prepare:
>
> Hi,
>
> this likely broke s390 build as expolines still depend on
> scripts/basic/fixdep. And build of expolines can now race with fixdep build:
>       make[1]: *** Deleting file 'arch/s390/lib/expoline/expoline.o'
>       /bin/sh: line 1: scripts/basic/fixdep: Permission denied
>       make[1]: *** [../scripts/Makefile.build:385:
> arch/s390/lib/expoline/expoline.o] Error 126
>       make: *** [../arch/s390/Makefile:166: expoline_prepare] Error 2


Indeed. This is a race.


> I returned there:
>    expoline_prepare: prepare0
> and it looks good so far. Maybe even:
>    expoline_prepare: scripts
> would be enough.
>
> Opinions?
>

Technically, 'scripts' might be enough, but
it is difficult to predict whether
arch/s390/lib/expoline/expoline.S
includes generated headers or not.

The chain of header inclusion changes all the time.

-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] s390/nospec: remove unneeded header includes

[Vasily Gorbik]

On Thu, Mar 16, 2023 at 12:14:27PM +0100, Jiri Slaby wrote:
> On 27. 06. 22, 14:50, Vasily Gorbik wrote:
> > With that, expoline.S doesn't require asm-offsets.h and
> > expoline_prepare target dependency could be removed.
> > 
> > +++ b/arch/s390/Makefile
> > @@ -166,7 +166,7 @@ vdso_prepare: prepare0
> >   ifdef CONFIG_EXPOLINE_EXTERN
> >   modules_prepare: expoline_prepare
> > -expoline_prepare: prepare0
> > +expoline_prepare:
> 
> this likely broke s390 build as expolines still depend on
> scripts/basic/fixdep. And build of expolines can now race with fixdep build:
>      make[1]: *** Deleting file 'arch/s390/lib/expoline/expoline.o'
>      /bin/sh: line 1: scripts/basic/fixdep: Permission denied
>      make[1]: *** [../scripts/Makefile.build:385:
> arch/s390/lib/expoline/expoline.o] Error 126
>      make: *** [../arch/s390/Makefile:166: expoline_prepare] Error 2
> 
> I returned there:
>   expoline_prepare: prepare0
> and it looks good so far. Maybe even:
>   expoline_prepare: scripts
> would be enough.

Hi Jiri, thanks for looking into this!

Probably even scripts_basic would be enough to add explicit dependency
to fixdep. But I just couldn't reproduce missing scripts/basic/fixdep
neither with modules_prepare nor expoline_prepare targets.

With which specific build command were you able to get those error
messages? I wonder where
        make[1]: *** Deleting file 'arch/s390/lib/expoline/expoline.o'
is coming from. Could it be smth like?

make ARCH=s390 CROSS_COMPILE=s390x-12.2.0- -j64 arch/s390/lib/expoline/expoline.o

Playing around with this build target I found it is broken:

  AS      arch/s390/lib/expoline/expoline.o
  AS      arch/s390/lib/expoline/expoline.o
fixdep: error opening file: arch/s390/lib/expoline/.expoline.o.d: No such file or directory
make[3]: *** [scripts/Makefile.build:374: arch/s390/lib/expoline/expoline.o] Error 2
make[3]: *** Deleting file 'arch/s390/lib/expoline/expoline.o'
make[2]: *** [scripts/Makefile.build:494: arch/s390/lib/expoline] Error 2
make[1]: *** [scripts/Makefile.build:494: arch/s390/lib] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:2028: .] Error 2

Notice dup AS call, which is probably causing this:
make[3]: *** Deleting file 'arch/s390/lib/expoline/expoline.o'

But that would be a different issue from the one you are trying to fix.

Re: [PATCH 2/2] s390/nospec: remove unneeded header includes

[Jiri Slaby]

On 17. 03. 23, 12:17, Vasily Gorbik wrote:
> On Thu, Mar 16, 2023 at 12:14:27PM +0100, Jiri Slaby wrote:
>> On 27. 06. 22, 14:50, Vasily Gorbik wrote:
>>> With that, expoline.S doesn't require asm-offsets.h and
>>> expoline_prepare target dependency could be removed.
>>>
>>> +++ b/arch/s390/Makefile
>>> @@ -166,7 +166,7 @@ vdso_prepare: prepare0
>>>    ifdef CONFIG_EXPOLINE_EXTERN
>>>    modules_prepare: expoline_prepare
>>> -expoline_prepare: prepare0
>>> +expoline_prepare:
>>
>> this likely broke s390 build as expolines still depend on
>> scripts/basic/fixdep. And build of expolines can now race with fixdep build:
>>       make[1]: *** Deleting file 'arch/s390/lib/expoline/expoline.o'
>>       /bin/sh: line 1: scripts/basic/fixdep: Permission denied
>>       make[1]: *** [../scripts/Makefile.build:385:
>> arch/s390/lib/expoline/expoline.o] Error 126
>>       make: *** [../arch/s390/Makefile:166: expoline_prepare] Error 2
>>
>> I returned there:
>>    expoline_prepare: prepare0
>> and it looks good so far. Maybe even:
>>    expoline_prepare: scripts
>> would be enough.
> 
> Hi Jiri, thanks for looking into this!
> 
> Probably even scripts_basic would be enough to add explicit dependency
> to fixdep. But I just couldn't reproduce missing scripts/basic/fixdep
> neither with modules_prepare nor expoline_prepare targets.

Hi,

yes, I could not reproduce locally too. It likely needs a "slow" and 
sort of specific machine to happen. This happened randomly only on SUSE 
build systems. And only on the internal ones. There are no failures on 
public ones:
https://build.opensuse.org/packages/kernel-default/job_history/Kernel:stable/S390/s390x

The kernel is built as:
make prepare # builds scripts/basic and other stuff we use
make clean # remove all but scripts and config
make all # scripts/basic/fixdep is rebuilt

fixdep is rebuilt due to clean-ed .fixdep.o.cmd -- that one is 
regenerated and fixdep built anew.

The whole process (make log) is dumped at:
https://build.opensuse.org/package/live_build_log/Kernel:stable/kernel-default/S390/s390x

> With which specific build command were you able to get those error
> messages? I wonder where
>          make[1]: *** Deleting file 'arch/s390/lib/expoline/expoline.o'
> is coming from. Could it be smth like?

It's from make after the build (fixdep invocation) failure. So that 
stale files do not exist.

Note that fixdep likely exist, but it is a stub -- linking phase still runs.

> make ARCH=s390 CROSS_COMPILE=s390x-12.2.0- -j64 arch/s390/lib/expoline/expoline.o
> 
> Playing around with this build target I found it is broken:
> 
>    AS      arch/s390/lib/expoline/expoline.o
>    AS      arch/s390/lib/expoline/expoline.o
> fixdep: error opening file: arch/s390/lib/expoline/.expoline.o.d: No such file or directory
> make[3]: *** [scripts/Makefile.build:374: arch/s390/lib/expoline/expoline.o] Error 2
> make[3]: *** Deleting file 'arch/s390/lib/expoline/expoline.o'
> make[2]: *** [scripts/Makefile.build:494: arch/s390/lib/expoline] Error 2
> make[1]: *** [scripts/Makefile.build:494: arch/s390/lib] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:2028: .] Error 2
> 
> Notice dup AS call, which is probably causing this:
> make[3]: *** Deleting file 'arch/s390/lib/expoline/expoline.o'
> 
> But that would be a different issue from the one you are trying to fix.

Likely.

thanks,
-- 
js

Re: [PATCH 2/2] s390/nospec: remove unneeded header includes

[Masahiro Yamada]

On Fri, Mar 17, 2023 at 8:17 PM Vasily Gorbik <gor@linux.ibm.com> wrote:
>
> On Thu, Mar 16, 2023 at 12:14:27PM +0100, Jiri Slaby wrote:
> > On 27. 06. 22, 14:50, Vasily Gorbik wrote:
> > > With that, expoline.S doesn't require asm-offsets.h and
> > > expoline_prepare target dependency could be removed.
> > >
> > > +++ b/arch/s390/Makefile
> > > @@ -166,7 +166,7 @@ vdso_prepare: prepare0
> > >   ifdef CONFIG_EXPOLINE_EXTERN
> > >   modules_prepare: expoline_prepare
> > > -expoline_prepare: prepare0
> > > +expoline_prepare:
> >
> > this likely broke s390 build as expolines still depend on
> > scripts/basic/fixdep. And build of expolines can now race with fixdep build:
> >      make[1]: *** Deleting file 'arch/s390/lib/expoline/expoline.o'
> >      /bin/sh: line 1: scripts/basic/fixdep: Permission denied
> >      make[1]: *** [../scripts/Makefile.build:385:
> > arch/s390/lib/expoline/expoline.o] Error 126
> >      make: *** [../arch/s390/Makefile:166: expoline_prepare] Error 2
> >
> > I returned there:
> >   expoline_prepare: prepare0
> > and it looks good so far. Maybe even:
> >   expoline_prepare: scripts
> > would be enough.
>
> Hi Jiri, thanks for looking into this!
>
> Probably even scripts_basic would be enough to add explicit dependency
> to fixdep. But I just couldn't reproduce missing scripts/basic/fixdep
> neither with modules_prepare nor expoline_prepare targets.
>
> With which specific build command were you able to get those error
> messages? I wonder where
>         make[1]: *** Deleting file 'arch/s390/lib/expoline/expoline.o'
> is coming from. Could it be smth like?
>
> make ARCH=s390 CROSS_COMPILE=s390x-12.2.0- -j64 arch/s390/lib/expoline/expoline.o
>
> Playing around with this build target I found it is broken:
>
>   AS      arch/s390/lib/expoline/expoline.o
>   AS      arch/s390/lib/expoline/expoline.o
> fixdep: error opening file: arch/s390/lib/expoline/.expoline.o.d: No such file or directory
> make[3]: *** [scripts/Makefile.build:374: arch/s390/lib/expoline/expoline.o] Error 2
> make[3]: *** Deleting file 'arch/s390/lib/expoline/expoline.o'
> make[2]: *** [scripts/Makefile.build:494: arch/s390/lib/expoline] Error 2
> make[1]: *** [scripts/Makefile.build:494: arch/s390/lib] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:2028: .] Error 2
>
> Notice dup AS call, which is probably causing this:
> make[3]: *** Deleting file 'arch/s390/lib/expoline/expoline.o'
>
> But that would be a different issue from the one you are trying to fix.


Kbuild is able to build a single object that is built
in a normal descending.


Since you built arch/s390/lib/expoline/expoline.o
from the special target 'expoline_prepare',
two different threads simultaneously built
arch/s390/lib/expoline/expoline.o
(one from the normal descending, the other
from 'expoline_prepare')

That's why.




--
Best Regards
Masahiro Yamada