Re: [PATCH v5 02/19] pc-bios/s390-ccw: Use the libc from SLOF and remove sclp prints

[Thomas Huth]

On 22/10/2024 22.12, Jared Rossi wrote:
> On 10/22/24 1:36 PM, Thomas Huth wrote:
> > On 20/10/2024 03.29, jrossi@linux.ibm.com wrote:
> > > From: Jared Rossi <jrossi@linux.ibm.com>
> > >
> > > We are already using the libc from SLOF for the s390-netboot.img, and
> > > this libc implementation is way more complete and accurate than the
> > > simple implementation that we currently use for the s390-ccw.img binary.
> > > Since we are now always assuming that the SLOF submodule is available
> > > when building the s390-ccw bios (see commit bf6903f6944f), we can drop
> > > the simple implementation and use the SLOF libc for the s390-ccw.img
> > > binary, too.
> > >
> > > Additionally replace sclp_print calls with puts/printf now that it is 
> > > avaliable.
> > >
> > > Co-authored by: Thomas Huth <thuth@redhat.com>
> > > Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> > > ---
> > >   pc-bios/s390-ccw/netboot.mak     |  3 --
> > >   pc-bios/s390-ccw/bootmap.h       |  4 +-
> > >   pc-bios/s390-ccw/libc.h          | 89 --------------------------------
> >
> > I just had to discover that the removal of the old libc breaks "make 
> > check-tcg" since it uses the sclp.c from the firmware for printing.
> >
> > I'll squash this patch to fix it:
> >
> > diff --git a/tests/tcg/s390x/console.c b/tests/tcg/s390x/console.c
> > --- a/tests/tcg/s390x/console.c
> > +++ b/tests/tcg/s390x/console.c
> > @@ -4,7 +4,10 @@
> >   *
> >   * SPDX-License-Identifier: GPL-2.0-or-later
> >   */
> > +
> >  #include "../../../pc-bios/s390-ccw/sclp.c"
> > +#include "../../../roms/SLOF/lib/libc/string/memset.c"
> > +#include "../../../roms/SLOF/lib/libc/string/memcpy.c"
> >
> >  void __sys_outc(char c)
> >  {
> > diff --git a/tests/tcg/s390x/Makefile.softmmu-target b/tests/tcg/s390x/ 
> > Makefile.softmmu-target
> > --- a/tests/tcg/s390x/Makefile.softmmu-target
> > +++ b/tests/tcg/s390x/Makefile.softmmu-target
> > @@ -3,7 +3,9 @@ VPATH+=$(S390X_SRC)
> >  # EXTFLAGS can be passed by the user, e.g. to override the --accel
> >  QEMU_OPTS+=-action panic=exit-failure -nographic -serial chardev:output 
> > $(EXTFLAGS) -kernel
> >  LINK_SCRIPT=$(S390X_SRC)/softmmu.ld
> > -CFLAGS+=-ggdb -O0
> > +CFLAGS+=-ggdb -O0 \
> > +    -I$(SRC_PATH)/roms/SLOF/lib/libc/include/ \
>
> I'm not sure if it is just a problem with my setup, but with this patch 
> applied I hit an error when running "make check-tcg --always-make"
>
>          In file included from /root/staging/qemu/tests/tcg/multiarch/ 
> system/memory.c:17:
>          /usr/include/inttypes.h:351:8: error: unknown type name ‘intmax_t’
>            351 | extern intmax_t imaxabs (intmax_t __n) __THROW 
> __attribute__ ((__const__));
>
> It seems to be cause by including the SLOF libc, because it works for me 
> without that.
>
> For what it's worth, it worked until used I "--always-make", so if you 
> aren't building all targets some change might not be picked up here.

I don't see it locally, but I've hit the same problem in the CI:

 https://gitlab.com/thuth/qemu/-/jobs/8156508094#L6090

It seems to work for me, if I simply drop that 
"-I$(SRC_PATH)/roms/SLOF/lib/libc/include/" line again and only use the 
other -I line. I initially struggled a little bit with my cross-compiler 
setup, that's why I thought I'd need the line, but I think it is not 
required with a proper setup.
Could you please check whether it works for you if you remove that line?

 Thanks,
   Thomas