[PULL 81/94] target/sparc: Move FDMULQ to decodetree

Richard Henderson posted 94 patches 1 year, 1 month ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Laurent Vivier <laurent@vivier.eu>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Artyom Tarasenko <atar4qemu@gmail.com>, WANG Xuerui <git@xen0n.name>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Aurelien Jarno <aurelien@aurel32.net>, Huacai Chen <chenhuacai@kernel.org>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <Alistair.Francis@wdc.com>, Stefan Weil <sw@weilnetz.de>
There is a newer version of this series
[PULL 81/94] target/sparc: Move FDMULQ to decodetree
Posted by Richard Henderson 1 year, 1 month ago
Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/sparc/insns.decode |  1 +
 target/sparc/translate.c  | 41 +++++++++++++++++++++------------------
 2 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/target/sparc/insns.decode b/target/sparc/insns.decode
index 6817d52ca2..a19d191603 100644
--- a/target/sparc/insns.decode
+++ b/target/sparc/insns.decode
@@ -261,6 +261,7 @@ FDIVs       10 ..... 110100 ..... 0 0100 1101 .....        @r_r_r
 FDIVd       10 ..... 110100 ..... 0 0100 1110 .....        @r_r_r
 FDIVq       10 ..... 110100 ..... 0 0100 1111 .....        @r_r_r
 FsMULd      10 ..... 110100 ..... 0 0110 1001 .....        @r_r_r
+FdMULq      10 ..... 110100 ..... 0 0110 1110 .....        @r_r_r
 FdTOx       10 ..... 110100 00000 0 1000 0010 .....        @r_r2
 FxTOd       10 ..... 110100 00000 0 1000 1000 .....        @r_r2
 FiTOs       10 ..... 110100 00000 0 1100 0100 .....        @r_r2
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 6626042776..29643768e1 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -1669,21 +1669,6 @@ static void gen_ne_fop_QQ(DisasContext *dc, int rd, int rs,
 }
 #endif
 
-static void gen_fop_QDD(DisasContext *dc, int rd, int rs1, int rs2,
-                        void (*gen)(TCGv_ptr, TCGv_i64, TCGv_i64))
-{
-    TCGv_i64 src1, src2;
-
-    src1 = gen_load_fpr_D(dc, rs1);
-    src2 = gen_load_fpr_D(dc, rs2);
-
-    gen(tcg_env, src1, src2);
-    gen_helper_check_ieee_exceptions(cpu_fsr, tcg_env);
-
-    gen_op_store_QT0_fpr(QFPREG(rd));
-    gen_update_fprs_dirty(dc, QFPREG(rd));
-}
-
 #ifdef TARGET_SPARC64
 static void gen_fop_DF(DisasContext *dc, int rd, int rs,
                        void (*gen)(TCGv_i64, TCGv_ptr, TCGv_i32))
@@ -4982,6 +4967,27 @@ TRANS(FSUBq, ALL, do_env_qqq, a, gen_helper_fsubq)
 TRANS(FMULq, ALL, do_env_qqq, a, gen_helper_fmulq)
 TRANS(FDIVq, ALL, do_env_qqq, a, gen_helper_fdivq)
 
+static bool trans_FdMULq(DisasContext *dc, arg_r_r_r *a)
+{
+    TCGv_i64 src1, src2;
+
+    if (gen_trap_ifnofpu(dc)) {
+        return true;
+    }
+    if (gen_trap_float128(dc)) {
+        return true;
+    }
+
+    gen_op_clear_ieee_excp_and_FTT();
+    src1 = gen_load_fpr_D(dc, a->rs1);
+    src2 = gen_load_fpr_D(dc, a->rs2);
+    gen_helper_fdmulq(tcg_env, src1, src2);
+    gen_helper_check_ieee_exceptions(cpu_fsr, tcg_env);
+    gen_op_store_QT0_fpr(QFPREG(a->rd));
+    gen_update_fprs_dirty(dc, QFPREG(a->rd));
+    return advance_pc(dc);
+}
+
 #define CHECK_IU_FEATURE(dc, FEATURE)                      \
     if (!((dc)->def->features & CPU_FEATURE_ ## FEATURE))  \
         goto illegal_insn;
@@ -5048,11 +5054,8 @@ static void disas_sparc_legacy(DisasContext *dc, unsigned int insn)
                 case 0x4b: /* fmulq */
                 case 0x4f: /* fdivq */
                 case 0x69: /* fsmuld */
-                    g_assert_not_reached(); /* in decodetree */
                 case 0x6e: /* fdmulq */
-                    CHECK_FPU_FEATURE(dc, FLOAT128);
-                    gen_fop_QDD(dc, rd, rs1, rs2, gen_helper_fdmulq);
-                    break;
+                    g_assert_not_reached(); /* in decodetree */
                 case 0xc6: /* fdtos */
                     gen_fop_FD(dc, rd, rs2, gen_helper_fdtos);
                     break;
-- 
2.34.1
Re: [PULL 81/94] target/sparc: Move FDMULQ to decodetree
Posted by Mark Cave-Ayland 1 year ago
On 26/10/2023 01:15, Richard Henderson wrote:

> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/sparc/insns.decode |  1 +
>   target/sparc/translate.c  | 41 +++++++++++++++++++++------------------
>   2 files changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/target/sparc/insns.decode b/target/sparc/insns.decode
> index 6817d52ca2..a19d191603 100644
> --- a/target/sparc/insns.decode
> +++ b/target/sparc/insns.decode
> @@ -261,6 +261,7 @@ FDIVs       10 ..... 110100 ..... 0 0100 1101 .....        @r_r_r
>   FDIVd       10 ..... 110100 ..... 0 0100 1110 .....        @r_r_r
>   FDIVq       10 ..... 110100 ..... 0 0100 1111 .....        @r_r_r
>   FsMULd      10 ..... 110100 ..... 0 0110 1001 .....        @r_r_r
> +FdMULq      10 ..... 110100 ..... 0 0110 1110 .....        @r_r_r
>   FdTOx       10 ..... 110100 00000 0 1000 0010 .....        @r_r2
>   FxTOd       10 ..... 110100 00000 0 1000 1000 .....        @r_r2
>   FiTOs       10 ..... 110100 00000 0 1100 0100 .....        @r_r2
> diff --git a/target/sparc/translate.c b/target/sparc/translate.c
> index 6626042776..29643768e1 100644
> --- a/target/sparc/translate.c
> +++ b/target/sparc/translate.c
> @@ -1669,21 +1669,6 @@ static void gen_ne_fop_QQ(DisasContext *dc, int rd, int rs,
>   }
>   #endif
>   
> -static void gen_fop_QDD(DisasContext *dc, int rd, int rs1, int rs2,
> -                        void (*gen)(TCGv_ptr, TCGv_i64, TCGv_i64))
> -{
> -    TCGv_i64 src1, src2;
> -
> -    src1 = gen_load_fpr_D(dc, rs1);
> -    src2 = gen_load_fpr_D(dc, rs2);
> -
> -    gen(tcg_env, src1, src2);
> -    gen_helper_check_ieee_exceptions(cpu_fsr, tcg_env);
> -
> -    gen_op_store_QT0_fpr(QFPREG(rd));
> -    gen_update_fprs_dirty(dc, QFPREG(rd));
> -}
> -
>   #ifdef TARGET_SPARC64
>   static void gen_fop_DF(DisasContext *dc, int rd, int rs,
>                          void (*gen)(TCGv_i64, TCGv_ptr, TCGv_i32))
> @@ -4982,6 +4967,27 @@ TRANS(FSUBq, ALL, do_env_qqq, a, gen_helper_fsubq)
>   TRANS(FMULq, ALL, do_env_qqq, a, gen_helper_fmulq)
>   TRANS(FDIVq, ALL, do_env_qqq, a, gen_helper_fdivq)
>   
> +static bool trans_FdMULq(DisasContext *dc, arg_r_r_r *a)
> +{
> +    TCGv_i64 src1, src2;
> +
> +    if (gen_trap_ifnofpu(dc)) {
> +        return true;
> +    }
> +    if (gen_trap_float128(dc)) {
> +        return true;
> +    }
> +
> +    gen_op_clear_ieee_excp_and_FTT();
> +    src1 = gen_load_fpr_D(dc, a->rs1);
> +    src2 = gen_load_fpr_D(dc, a->rs2);
> +    gen_helper_fdmulq(tcg_env, src1, src2);
> +    gen_helper_check_ieee_exceptions(cpu_fsr, tcg_env);
> +    gen_op_store_QT0_fpr(QFPREG(a->rd));
> +    gen_update_fprs_dirty(dc, QFPREG(a->rd));
> +    return advance_pc(dc);
> +}
> +
>   #define CHECK_IU_FEATURE(dc, FEATURE)                      \
>       if (!((dc)->def->features & CPU_FEATURE_ ## FEATURE))  \
>           goto illegal_insn;
> @@ -5048,11 +5054,8 @@ static void disas_sparc_legacy(DisasContext *dc, unsigned int insn)
>                   case 0x4b: /* fmulq */
>                   case 0x4f: /* fdivq */
>                   case 0x69: /* fsmuld */
> -                    g_assert_not_reached(); /* in decodetree */
>                   case 0x6e: /* fdmulq */
> -                    CHECK_FPU_FEATURE(dc, FLOAT128);
> -                    gen_fop_QDD(dc, rd, rs1, rs2, gen_helper_fdmulq);
> -                    break;
> +                    g_assert_not_reached(); /* in decodetree */
>                   case 0xc6: /* fdtos */
>                       gen_fop_FD(dc, rd, rs2, gen_helper_fdtos);
>                       break;

Hi Richard,

I was working through my SPARC boot tests for your latest target/sparc series when I 
spotted a segfault on my FreeBSD SPARC64 image. A git bisect indicated that this was 
the patch that originally introduced the error, something I must have missed when 
testing the original decodetree conversion series.

The reproducer is:

./qemu-system-sparc64 -m 256 -cdrom FreeBSD-10.3-RELEASE-sparc64-bootonly.iso \
     -boot d -nographic

and the error is a segfault in devd:

...
...
Trying to mount root from cd9660:/dev/iso9660/10_3_RELEASE_SPARC64_BO [ro]...
Entropy harvesting: interrupts ethernet point_to_point swi.
Starting file system checks:
Mounting local file systems:.
Writing entropy file:.
/etc/rc: WARNING: $hostname is not set -- see rc.conf(5).
Starting Network: lo0 hme0.
lo0: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> metric 0 mtu 16384
         options=600003<RXCSUM,TXCSUM,RXCSUM_IPV6,TXCSUM_IPV6>
         inet6 ::1 prefixlen 128
         inet6 fe80::1%lo0 prefixlen 64 scopeid 0x2
         inet 127.0.0.1 netmask 0xff000000
         nd6 options=21<PERFORMNUD,AUTO_LINKLOCAL>
hme0: flags=8802<BROADCAST,SIMPLEX,MULTICAST> metric 0 mtu 1500
         options=8000b<RXCSUM,TXCSUM,VLAN_MTU,LINKSTATE>
         ether 52:54:00:12:34:56
         nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>
         media: Ethernet autoselect
Starting devd.
pid 246 (ps), uid 0: exited on signal 11
Segmentation fault
^^^^^^^^^^^^^^^^^^

Starting Network: hme0.
hme0: flags=8802<BROADCAST,SIMPLEX,MULTICAST> metric 0 mtu 1500
         options=8000b<RXCSUM,TXCSUM,VLAN_MTU,LINKSTATE>
         ether 52:54:00:12:34:56
         nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>
         media: Ethernet autoselect
...
...


ATB,

Mark.
Re: [PULL 81/94] target/sparc: Move FDMULQ to decodetree
Posted by Richard Henderson 1 year ago
On 11/6/23 14:02, Mark Cave-Ayland wrote:
> I was working through my SPARC boot tests for your latest target/sparc series when I 
> spotted a segfault on my FreeBSD SPARC64 image. A git bisect indicated that this was the 
> patch that originally introduced the error, something I must have missed when testing the 
> original decodetree conversion series.
> 
> The reproducer is:
> 
> ./qemu-system-sparc64 -m 256 -cdrom FreeBSD-10.3-RELEASE-sparc64-bootonly.iso \
>      -boot d -nographic
> 
> and the error is a segfault in devd:
> 
> ...
> ...
> Trying to mount root from cd9660:/dev/iso9660/10_3_RELEASE_SPARC64_BO [ro]...
> Entropy harvesting: interrupts ethernet point_to_point swi.
> Starting file system checks:
> Mounting local file systems:.
> Writing entropy file:.
> /etc/rc: WARNING: $hostname is not set -- see rc.conf(5).
> Starting Network: lo0 hme0.
> lo0: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> metric 0 mtu 16384
>          options=600003<RXCSUM,TXCSUM,RXCSUM_IPV6,TXCSUM_IPV6>
>          inet6 ::1 prefixlen 128
>          inet6 fe80::1%lo0 prefixlen 64 scopeid 0x2
>          inet 127.0.0.1 netmask 0xff000000
>          nd6 options=21<PERFORMNUD,AUTO_LINKLOCAL>
> hme0: flags=8802<BROADCAST,SIMPLEX,MULTICAST> metric 0 mtu 1500
>          options=8000b<RXCSUM,TXCSUM,VLAN_MTU,LINKSTATE>
>          ether 52:54:00:12:34:56
>          nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>
>          media: Ethernet autoselect
> Starting devd.
> pid 246 (ps), uid 0: exited on signal 11
> Segmentation fault
> ^^^^^^^^^^^^^^^^^^


I certainly can't imagine that FdMULq is really at fault, because it's not implemented on 
real hardware (and thus I really doubt FreeBSD attempted to use it), and 
CPU_FEATURE_FLOAT128 is not enabled by your command-line.

The only thing that I can imagine is that this is some sort of timing related issue and 
bisect behaved randomly.

All that said, I can't replicate this with master.
Can you, now?


r~

Re: [PULL 81/94] target/sparc: Move FDMULQ to decodetree
Posted by Mark Cave-Ayland 1 year ago
On 07/11/2023 04:49, Richard Henderson wrote:

> On 11/6/23 14:02, Mark Cave-Ayland wrote:
>> I was working through my SPARC boot tests for your latest target/sparc series when 
>> I spotted a segfault on my FreeBSD SPARC64 image. A git bisect indicated that this 
>> was the patch that originally introduced the error, something I must have missed 
>> when testing the original decodetree conversion series.
>>
>> The reproducer is:
>>
>> ./qemu-system-sparc64 -m 256 -cdrom FreeBSD-10.3-RELEASE-sparc64-bootonly.iso \
>>      -boot d -nographic
>>
>> and the error is a segfault in devd:
>>
>> ...
>> ...
>> Trying to mount root from cd9660:/dev/iso9660/10_3_RELEASE_SPARC64_BO [ro]...
>> Entropy harvesting: interrupts ethernet point_to_point swi.
>> Starting file system checks:
>> Mounting local file systems:.
>> Writing entropy file:.
>> /etc/rc: WARNING: $hostname is not set -- see rc.conf(5).
>> Starting Network: lo0 hme0.
>> lo0: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> metric 0 mtu 16384
>>          options=600003<RXCSUM,TXCSUM,RXCSUM_IPV6,TXCSUM_IPV6>
>>          inet6 ::1 prefixlen 128
>>          inet6 fe80::1%lo0 prefixlen 64 scopeid 0x2
>>          inet 127.0.0.1 netmask 0xff000000
>>          nd6 options=21<PERFORMNUD,AUTO_LINKLOCAL>
>> hme0: flags=8802<BROADCAST,SIMPLEX,MULTICAST> metric 0 mtu 1500
>>          options=8000b<RXCSUM,TXCSUM,VLAN_MTU,LINKSTATE>
>>          ether 52:54:00:12:34:56
>>          nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>
>>          media: Ethernet autoselect
>> Starting devd.
>> pid 246 (ps), uid 0: exited on signal 11
>> Segmentation fault
>> ^^^^^^^^^^^^^^^^^^
> 
> 
> I certainly can't imagine that FdMULq is really at fault, because it's not 
> implemented on real hardware (and thus I really doubt FreeBSD attempted to use it), 
> and CPU_FEATURE_FLOAT128 is not enabled by your command-line.
> 
> The only thing that I can imagine is that this is some sort of timing related issue 
> and bisect behaved randomly.

Hmmm you're right, it seems that there is a semi-random aspect to the issue which is 
why the bisection didn't give a good result.

In order to mitigate this, I repeated the bisection again but this time booting the 
FreeBSD image 5 times in a row, only marking the commit as good if all 5 boot tests 
passed [1] without displaying a segfault message [2]. That bisection led me to this 
commit:


86b82fe021f46ed4501b16132f7e3fccd0a1ad5d is the first bad commit
commit 86b82fe021f46ed4501b16132f7e3fccd0a1ad5d
Author: Richard Henderson <richard.henderson@linaro.org>
Date:   Wed Oct 4 17:51:37 2023 -0700

      target/sparc: Move JMPL, RETT, RETURN to decodetree

      Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
      Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
      Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

   target/sparc/insns.decode |   7 +++
   target/sparc/translate.c  | 126 +++++++++++++++++++++++++++++-----------------
   2 files changed, 88 insertions(+), 45 deletions(-)


I tried repeating the boot 5 times in a row test on both this commit and the commit 
before it, and that appeared to confirm the bisection result with the failures only 
appearing with commit 86b82fe021.

 > All that said, I can't replicate this with master.
 > Can you, now?

Yes, the issue is still present in master for me. My host is Debian bookworm (stable, 
x86_64) configured as './configure' '--target-list=sparc64-softmmu' '--enable-slirp' 
'--enable-debug' and the command line is:

./qemu-system-sparc64 -m 256 -cdrom FreeBSD-10.3-RELEASE-sparc64-bootonly.iso \
      -boot d -nographic


ATB,

Mark.

[1] Sometimes the image hangs just after "Trying to mount root .." but that appears 
to be a timing issue and not directly related to this series. In this cases where 
this happened, I simply quit QEMU and rebooted the image again.

[2] Normally the segfault message comes from devd, but during the bisection I did 
occasionally see it coming from other processes.