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

Joe Lawrence
2022-07-01T21:39:10Z

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