1. Patchew
  2. QEMU
  3. View series

[PATCH] target/ppc: Fix fallback to MFSS for MFFSCRN, MFFSCRNI, MFFSCE and MFFSL

Richard Purdie posted 1 patch 12 months ago
Failed in applying to current master (apply log)
target/ppc/translate/fp-impl.c.inc | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
[PATCH] target/ppc: Fix fallback to MFSS for MFFSCRN, MFFSCRNI, MFFSCE and MFFSL
Posted by Richard Purdie 12 months ago 
The following commits changed the code such that these instructions became invalid
on pre 3.0 ISAs:

  bf8adfd88b547680aa857c46098f3a1e94373160 - target/ppc: Move mffscrn[i] to decodetree
  394c2e2fda70da722f20fb60412d6c0ca4bfaa03 - target/ppc: Move mffsce to decodetree
  3e5bce70efe6bd1f684efbb21fd2a316cbf0657e - target/ppc: Move mffsl to decodetree

The hardware will handle them as a MFFS instruction as the code did previously.
Restore that behaviour. This means applications that were segfaulting under qemu
when encountering these instructions now operate correctly. The instruction
is used in glibc libm functions for example.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 target/ppc/translate/fp-impl.c.inc | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/target/ppc/translate/fp-impl.c.inc b/target/ppc/translate/fp-impl.c.inc
index 57d8437851..cb86381c3f 100644
--- a/target/ppc/translate/fp-impl.c.inc
+++ b/target/ppc/translate/fp-impl.c.inc
@@ -584,7 +584,10 @@ static bool trans_MFFSCE(DisasContext *ctx, arg_X_t *a)
 {
     TCGv_i64 fpscr;
 
-    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
+    if (unlikely(!(ctx->insns_flags2 & PPC2_ISA300))) {
+        return trans_MFFS(ctx, a);
+    }
+
     REQUIRE_FPU(ctx);
 
     gen_reset_fpstatus();
@@ -597,7 +600,10 @@ static bool trans_MFFSCRN(DisasContext *ctx, arg_X_tb *a)
 {
     TCGv_i64 t1, fpscr;
 
-    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
+    if (unlikely(!(ctx->insns_flags2 & PPC2_ISA300))) {
+        return trans_MFFS(ctx, a);
+    }
+
     REQUIRE_FPU(ctx);
 
     t1 = tcg_temp_new_i64();
@@ -631,7 +637,10 @@ static bool trans_MFFSCRNI(DisasContext *ctx, arg_X_imm2 *a)
 {
     TCGv_i64 t1, fpscr;
 
-    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
+    if (unlikely(!(ctx->insns_flags2 & PPC2_ISA300))) {
+        return trans_MFFS(ctx, a);
+    }
+
     REQUIRE_FPU(ctx);
 
     t1 = tcg_temp_new_i64();
@@ -661,7 +670,10 @@ static bool trans_MFFSCDRNI(DisasContext *ctx, arg_X_imm3 *a)
 
 static bool trans_MFFSL(DisasContext *ctx, arg_X_t *a)
 {
-    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
+    if (unlikely(!(ctx->insns_flags2 & PPC2_ISA300))) {
+        return trans_MFFS(ctx, a);
+    }
+
     REQUIRE_FPU(ctx);
 
     gen_reset_fpstatus();
Re: [PATCH] target/ppc: Fix fallback to MFSS for MFFSCRN, MFFSCRNI, MFFSCE and MFFSL
Posted by Matheus K. Ferst 12 months ago 
On 04/05/2023 08:01, Richard Purdie wrote:
> The following commits changed the code such that these instructions became invalid
> on pre 3.0 ISAs:
> 
>    bf8adfd88b547680aa857c46098f3a1e94373160 - target/ppc: Move mffscrn[i] to decodetree
>    394c2e2fda70da722f20fb60412d6c0ca4bfaa03 - target/ppc: Move mffsce to decodetree
>    3e5bce70efe6bd1f684efbb21fd2a316cbf0657e - target/ppc: Move mffsl to decodetree
> 
> The hardware will handle them as a MFFS instruction as the code did previously.
> Restore that behaviour. This means applications that were segfaulting under qemu
> when encountering these instructions now operate correctly. The instruction
> is used in glibc libm functions for example.
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>   target/ppc/translate/fp-impl.c.inc | 20 ++++++++++++++++----
>   1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/translate/fp-impl.c.inc b/target/ppc/translate/fp-impl.c.inc
> index 57d8437851..cb86381c3f 100644
> --- a/target/ppc/translate/fp-impl.c.inc
> +++ b/target/ppc/translate/fp-impl.c.inc
> @@ -584,7 +584,10 @@ static bool trans_MFFSCE(DisasContext *ctx, arg_X_t *a)
>   {
>       TCGv_i64 fpscr;
> 
> -    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
> +    if (unlikely(!(ctx->insns_flags2 & PPC2_ISA300))) {
> +        return trans_MFFS(ctx, a);
> +    }
> +

Hi Richard, nice catch!

I believe this may be better addressed by decodetree pattern groups, e.g.:

On insns32.decode:
{
   # Before Power ISA v3.0, MFFS bits 11~15 were reserved and should be 
ignored
   MFFS_ISA207   111111 ..... ----- ----- 1001000111 .   @X_t_rc
   [
     MFFS        111111 ..... 00000 ----- 1001000111 .   @X_t_rc
     MFFSCE      111111 ..... 00001 ----- 1001000111 -   @X_t
     MFFSCRN     111111 ..... 10110 ..... 1001000111 -   @X_tb
     MFFSCDRN    111111 ..... 10100 ..... 1001000111 -   @X_tb
     MFFSCRNI    111111 ..... 10111 ---.. 1001000111 -   @X_imm2
     MFFSCDRNI   111111 ..... 10101 --... 1001000111 -   @X_imm3
     MFFSL       111111 ..... 11000 ----- 1001000111 -   @X_t
   ]
}

And on fp-impl.c.inc:
static bool trans_MFFS_ISA207(DisasContext *ctx, arg_X_t_rc *a)
{
     if (!(ctx->insns_flags2 & PPC2_ISA300)) {
         /*
          * Before Power ISA v3.0, MFFS bits 11~15 were reserved, any 
instruction
          * with OPCD=63 and XO=583 should be decoded as MFFS.
          */
         return trans_MFFS(ctx, a);
     }
     /*
      * For Power ISA v3.0+, return false and let the pattern group
      * select the correct instruction.
      */
     return false;
}

That way, I believe it'll be easier to add more MFFS variants in the 
future without thinking too much about the behavior in previous versions 
of Power ISA.

Thanks,
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
Re: [PATCH] target/ppc: Fix fallback to MFSS for MFFSCRN, MFFSCRNI, MFFSCE and MFFSL
Posted by Richard Henderson 11 months, 4 weeks ago 
On 5/4/23 18:17, Matheus K. Ferst wrote:
> On 04/05/2023 08:01, Richard Purdie wrote:
>> The following commits changed the code such that these instructions became invalid
>> on pre 3.0 ISAs:
>>
>>    bf8adfd88b547680aa857c46098f3a1e94373160 - target/ppc: Move mffscrn[i] to decodetree
>>    394c2e2fda70da722f20fb60412d6c0ca4bfaa03 - target/ppc: Move mffsce to decodetree
>>    3e5bce70efe6bd1f684efbb21fd2a316cbf0657e - target/ppc: Move mffsl to decodetree
>>
>> The hardware will handle them as a MFFS instruction as the code did previously.
>> Restore that behaviour. This means applications that were segfaulting under qemu
>> when encountering these instructions now operate correctly. The instruction
>> is used in glibc libm functions for example.
>>
>> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
>> ---
>>   target/ppc/translate/fp-impl.c.inc | 20 ++++++++++++++++----
>>   1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/ppc/translate/fp-impl.c.inc b/target/ppc/translate/fp-impl.c.inc
>> index 57d8437851..cb86381c3f 100644
>> --- a/target/ppc/translate/fp-impl.c.inc
>> +++ b/target/ppc/translate/fp-impl.c.inc
>> @@ -584,7 +584,10 @@ static bool trans_MFFSCE(DisasContext *ctx, arg_X_t *a)
>>   {
>>       TCGv_i64 fpscr;
>>
>> -    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
>> +    if (unlikely(!(ctx->insns_flags2 & PPC2_ISA300))) {
>> +        return trans_MFFS(ctx, a);
>> +    }
>> +
> 
> Hi Richard, nice catch!
> 
> I believe this may be better addressed by decodetree pattern groups, e.g.:
> 
> On insns32.decode:
> {
>    # Before Power ISA v3.0, MFFS bits 11~15 were reserved and should be ignored
>    MFFS_ISA207   111111 ..... ----- ----- 1001000111 .   @X_t_rc
>    [
>      MFFS        111111 ..... 00000 ----- 1001000111 .   @X_t_rc
>      MFFSCE      111111 ..... 00001 ----- 1001000111 -   @X_t
>      MFFSCRN     111111 ..... 10110 ..... 1001000111 -   @X_tb
>      MFFSCDRN    111111 ..... 10100 ..... 1001000111 -   @X_tb
>      MFFSCRNI    111111 ..... 10111 ---.. 1001000111 -   @X_imm2
>      MFFSCDRNI   111111 ..... 10101 --... 1001000111 -   @X_imm3
>      MFFSL       111111 ..... 11000 ----- 1001000111 -   @X_t
>    ]
> }
> 
> And on fp-impl.c.inc:
> static bool trans_MFFS_ISA207(DisasContext *ctx, arg_X_t_rc *a)
> {
>      if (!(ctx->insns_flags2 & PPC2_ISA300)) {
>          /*
>           * Before Power ISA v3.0, MFFS bits 11~15 were reserved, any instruction
>           * with OPCD=63 and XO=583 should be decoded as MFFS.
>           */
>          return trans_MFFS(ctx, a);
>      }
>      /*
>       * For Power ISA v3.0+, return false and let the pattern group
>       * select the correct instruction.
>       */
>      return false;
> }

Not quite.  Should be

{
   [
     MFFSCE  111111 ..... 00001 ----- 1001000111 -  @X_t
     ...
     MFFSL   111111 ..... 11000 ----- 1001000111 -  @X_t
   ]
   MFFS      111111 ..... ----- ----- 1001000111 .  @X_t_rc
}

where all of the 3.0 insns do

     if (!(ctx->insns_flags2 & PPC2_ISA300)) {
         return false;
     }

I do not believe that v3.0 rejects bits [11:15] = 00010, for example, which would have 
been accepted and ignored with v2.07.  It should simply treat it as the full MFFS insn.


r~
Re: [PATCH] target/ppc: Fix fallback to MFSS for MFFSCRN, MFFSCRNI, MFFSCE and MFFSL
Posted by Matheus K. Ferst 11 months, 3 weeks ago 
On 05/05/2023 12:23, Richard Henderson wrote:
> On 5/4/23 18:17, Matheus K. Ferst wrote:
>> On 04/05/2023 08:01, Richard Purdie wrote:
>>> The following commits changed the code such that these instructions 
>>> became invalid
>>> on pre 3.0 ISAs:
>>>
>>>    bf8adfd88b547680aa857c46098f3a1e94373160 - target/ppc: Move 
>>> mffscrn[i] to decodetree
>>>    394c2e2fda70da722f20fb60412d6c0ca4bfaa03 - target/ppc: Move mffsce 
>>> to decodetree
>>>    3e5bce70efe6bd1f684efbb21fd2a316cbf0657e - target/ppc: Move mffsl 
>>> to decodetree
>>>
>>> The hardware will handle them as a MFFS instruction as the code did 
>>> previously.
>>> Restore that behaviour. This means applications that were segfaulting 
>>> under qemu
>>> when encountering these instructions now operate correctly. The 
>>> instruction
>>> is used in glibc libm functions for example.
>>>
>>> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
>>> ---
>>>   target/ppc/translate/fp-impl.c.inc | 20 ++++++++++++++++----
>>>   1 file changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/target/ppc/translate/fp-impl.c.inc 
>>> b/target/ppc/translate/fp-impl.c.inc
>>> index 57d8437851..cb86381c3f 100644
>>> --- a/target/ppc/translate/fp-impl.c.inc
>>> +++ b/target/ppc/translate/fp-impl.c.inc
>>> @@ -584,7 +584,10 @@ static bool trans_MFFSCE(DisasContext *ctx, 
>>> arg_X_t *a)
>>>   {
>>>       TCGv_i64 fpscr;
>>>
>>> -    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
>>> +    if (unlikely(!(ctx->insns_flags2 & PPC2_ISA300))) {
>>> +        return trans_MFFS(ctx, a);
>>> +    }
>>> +
>>
>> Hi Richard, nice catch!
>>
>> I believe this may be better addressed by decodetree pattern groups, 
>> e.g.:
>>
>> On insns32.decode:
>> {
>>    # Before Power ISA v3.0, MFFS bits 11~15 were reserved and should 
>> be ignored
>>    MFFS_ISA207   111111 ..... ----- ----- 1001000111 .   @X_t_rc
>>    [
>>      MFFS        111111 ..... 00000 ----- 1001000111 .   @X_t_rc
>>      MFFSCE      111111 ..... 00001 ----- 1001000111 -   @X_t
>>      MFFSCRN     111111 ..... 10110 ..... 1001000111 -   @X_tb
>>      MFFSCDRN    111111 ..... 10100 ..... 1001000111 -   @X_tb
>>      MFFSCRNI    111111 ..... 10111 ---.. 1001000111 -   @X_imm2
>>      MFFSCDRNI   111111 ..... 10101 --... 1001000111 -   @X_imm3
>>      MFFSL       111111 ..... 11000 ----- 1001000111 -   @X_t
>>    ]
>> }
>>
>> And on fp-impl.c.inc:
>> static bool trans_MFFS_ISA207(DisasContext *ctx, arg_X_t_rc *a)
>> {
>>      if (!(ctx->insns_flags2 & PPC2_ISA300)) {
>>          /*
>>           * Before Power ISA v3.0, MFFS bits 11~15 were reserved, any 
>> instruction
>>           * with OPCD=63 and XO=583 should be decoded as MFFS.
>>           */
>>          return trans_MFFS(ctx, a);
>>      }
>>      /*
>>       * For Power ISA v3.0+, return false and let the pattern group
>>       * select the correct instruction.
>>       */
>>      return false;
>> }
> 
> Not quite.  Should be
> 
> {
>    [
>      MFFSCE  111111 ..... 00001 ----- 1001000111 -  @X_t
>      ...
>      MFFSL   111111 ..... 11000 ----- 1001000111 -  @X_t
>    ]
>    MFFS      111111 ..... ----- ----- 1001000111 .  @X_t_rc
> }
> 
> where all of the 3.0 insns do
> 
>      if (!(ctx->insns_flags2 & PPC2_ISA300)) {
>          return false;
>      }
> 
> I do not believe that v3.0 rejects bits [11:15] = 00010, for example, 
> which would have
> been accepted and ignored with v2.07.  It should simply treat it as the 
> full MFFS insn.

Hi Richard, sorry for the delayed response. Testing on a POWER9, it does 
reject opcodes with undefined values in [11:15]:

$ cat > mffs.c << EOF
#include <signal.h>

#include <unistd.h>


int main(void) {
         signal(SIGILL, _exit);
         asm(MFFS_OPCD);
         return 0;
}
EOF
$ for i in {0..15}; do
     opc="$(( 0xfc00048e | ($i << 14) ))"
     gcc -DMFFS_OPCD="\".long $opc\"" mffs.c -o mffs
     printf "%s " $(echo "obase=2; $opc" | bc)
     if ./mffs ; then
         echo "valid"
     else
         echo "invalid"
     fi
done
11111100000000000000010010001110 valid  # MFFS
11111100000000010000010010001110 valid  # MFFSCE
11111100000000100000010010001110 invalid
11111100000000110000010010001110 invalid
11111100000001000000010010001110 invalid
11111100000001010000010010001110 invalid
11111100000001100000010010001110 invalid
11111100000001110000010010001110 invalid
11111100000010000000010010001110 invalid
11111100000010010000010010001110 invalid
11111100000010100000010010001110 invalid
11111100000010110000010010001110 invalid
11111100000011000000010010001110 invalid
11111100000011010000010010001110 invalid
11111100000011100000010010001110 invalid
11111100000011110000010010001110 invalid
11111100000100000000010010001110 invalid
11111100000100010000010010001110 invalid
11111100000100100000010010001110 invalid
11111100000100110000010010001110 invalid
11111100000101000000010010001110 valid  # MFFSCDRN
11111100000101010000010010001110 valid  # MFFSCDRNI
11111100000101100000010010001110 valid  # MFFSCRN
11111100000101110000010010001110 valid  # MFFSCRNI
11111100000110000000010010001110 valid  # MFFSL
11111100000110010000010010001110 invalid
11111100000110100000010010001110 invalid
11111100000110110000010010001110 invalid
11111100000111000000010010001110 invalid
11111100000111010000010010001110 invalid
11111100000111100000010010001110 invalid
11111100000111110000010010001110 invalid

Thanks,
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.