Re: [PATCH 3/7] tests/tcg: Add tests for prefixed load/store instructions

[Gustavo Romero]

Hi Alex,

Thanks a lot for the review.

On 10/5/20 1:26 PM, Alex Bennée wrote:
> Gustavo Romero <gromero@linux.ibm.com> writes:
>
> > From: Michael Roth <mdroth@linux.vnet.ibm.com>
> >
> > This commit adds various tests to exercise the implementation of prefixed
> > load/store instructions on POWER10.
> >
> > It adds a softmmu Makefile so tests can be built for the ppc64-softmmu
> > target correctly with:
> >
> > $ make build-tcg-tests-ppc64-softmmu
> >
> > Moreover it fixes missing BUILD_DIR variable in Makefile.include when
> > calling Makefile.qemu, otherwise config-$(TARGET).mak is not found (include
> > fails silently) in Makefile.qemu and build finishes with no errors but no
> > test is build. It also fixes quiet-command in Makefile.qemu, because it's
> > not defined when cross-build-guest-tests target calls it, so no command is
> > executed in fact, hence no test is build after make finishes.
>
> Hmm the context of the make should already be in BUILD_DIR anyway. This
> seems a little bogus and also unrelated to the main patch so please
> split so it can be reviewed separately.

The fix for the issue below (for quiet-command) actually fixed that issue too.

So after:

commit 2c243053060f7fd47b71dcb6e3b80346b89c796b
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Mon Sep 21 10:34:47 2020 -0400

    tests/tcg: reinstate or replace desired parts of rules.mak
    
    Commit 660f79309303d696531ffb394719dfab3e0c42c0 was a bit overzealous
    with respect to tests/tcg, which needed quiet-command and $(BUILD_DIR).
    Reinstate quiet-command, and replace $(BUILD_DIR) with just the
    current directory.
    
    Reported-by: Alex Bennée <alex.bennee@linaro.org>
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

the inclusion of config-$(TARGET).mak doesn't depend on BUILD_DIR anymore.

Previously it failed because although the make context is already in the
build dir, since BUILD_DIR is not defined the include considers the include
path an absolute path due to the slash starting it, so not a relative one.

Either way, I confirm both issues are resolved by the patch above.


> > Currently check-tcg target is not working, so '$ make check-tcg' fails
> > because such a target will try to run the generated test binary as if
> > it were a raw VM image, which is not the case. It's not a bootable image
> > either, so the binary can't be loaded by SLOF, so after the build it's
> > necessary to copy the test binary manually and execute it in a VM or a
> > real POWER10 machine (e.g. for comparison) with support for prefixed
> > instructions.
> >
> > Signed-off-by: Michael Roth <mroth@lamentation.net>
> > [ gromero - fix to avoid alignment interrupt, don't cross 64-byte boundary
> >            - fix displacement for pl{bz,hz,ha,wz,wa,d} to skip branch insn.
> >            - tweaks in debug output
> >            - add Makefile for ppc64-softmmu target
> >            - build fixes in Makefile.{qemu,include}
> >            - commit log ]
> > Signed-off-by: Gustavo Romero <gromero@linux.ibm.com>
> > ---
> <snip>
> >   
> >   .PHONY: build-tcg
> > diff --git a/tests/tcg/Makefile.qemu b/tests/tcg/Makefile.qemu
> > index 0332bad10f..b531da19dc 100644
> > --- a/tests/tcg/Makefile.qemu
> > +++ b/tests/tcg/Makefile.qemu
> > @@ -26,6 +26,8 @@ include $(SRC_PATH)/tests/docker/Makefile.include
> >   
> >   ifdef CROSS_CC_GUEST
> >   
> > +quiet-command = $(if $(V),$1,$(if $(2),@printf "  %-7s %s\n" $2 $3 && $1, @$1))
> > +
>
> This is a separate fix which I think is already merged anyway.

Yep :)


> > +#define dprintf(...) \
> > +    do { \
> > +        if (debug == true) { \
> > +            fprintf(stderr, "%s: ", __func__); \
> > +            fprintf(stderr, __VA_ARGS__); \
> > +        } \
> > +    } while (0);
>
> This is not a softmmu test. Surely this is a linux-user test?

Yes, Michael Roth originally wrote it for the linux-user target, but since
I've tested it only on softmmu I adapted it for the softmmu, hence I will
revert it back to a linux-user test, so no Linux image is necessary to
run it plus the test can be done automatically by '$ make check-tcg'.

One thing I don't understand yet is, if I would provide it also for the
softmmu target, what is actually necessary? I see softmmu relies on an
image, for instance, but I don't know how it should be provided for the
make check-tcg, nor I see how it would run automatically. Do you have any
suggestion in that sense?

--

On reverting to the original linux-user test I realized that no complain
is issued by configure if a non-existent cross compiler is specified, for
example: --cross-cc-ppc=wrong-gcc-binary. In that case the only result is
that no CROSS_CC_GUEST is defined in the .mak file per target and when
make check-tcg is executed it just skips the tests for the target. Thus
I'm wondering if it's ok to contribute the following simple fix that will
make configure refuse to go further if the cross compiler can't be found:
(if you agree I'll submit it later, not in the series)

diff --git a/configure b/configure
index f839c2a557..fb52c814db 100755
--- a/configure
+++ b/configure
@@ -6967,7 +6967,7 @@ done
   export $i
 done
 export target_list source_path use_containers
-$source_path/tests/tcg/configure.sh)
+$source_path/tests/tcg/configure.sh) || exit 1
 
 # temporary config to build submodules
 for rom in seabios; do
diff --git a/tests/tcg/configure.sh b/tests/tcg/configure.sh
index be51bdb5a4..91f1eaf1e6 100755
--- a/tests/tcg/configure.sh
+++ b/tests/tcg/configure.sh
@@ -212,8 +212,10 @@ for target in $target_list; do
 
     eval "target_compiler=\${cross_cc_$i}"
     if ! has $target_compiler; then
-      continue
+      echo "Specified cross-compiler '$target_compiler' not found!"
+      exit 1
     fi
+
     write_c_skeleton
     if ! do_compiler "$target_compiler" $target_compiler_cflags -o $TMPE $TMPC 
-static ; then
       # For host systems we might get away with building without -static
 
Finally, we've discussed a bit on IRC about the mmap test failing on a
POWER8 host with 64k pagesize. I've checked it also for the ppc-linux-user
target that uses the following line to force the mmap test to run with
4k pagesize setup:

https://github.com/qemu/qemu/blame/master/tests/tcg/ppc/Makefile.target#L11

and it fails too. Removing the constraint to run it with 4k (so it uses the
same pagesize as the host, i.e. 64k) makes the test pass. Do you remember
why the comments in that file say 64k is broken and 4k works? It seems the
opposite, so I'm wondering if the ppc and ppc64 targets need to get fixed
(by removing the -4096 in all cases).


Cheers,
Gustavo