Re: [PATCH v3] target/ppc: Fix fallback to MFSS for MFFS* instructions on pre 3.0 ISAs

[Daniel Henrique Barboza <danielhb413@gmail.com>]

Queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks,


Daniel

On 5/10/23 08:19, Richard Purdie wrote:
> The following commits changed the code such that the fallback to MFSS for MFFSCRN,
> MFFSCRNI, MFFSCE and MFFSL on pre 3.0 ISAs was removed and became an illegal instruction:
> 
>    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.
> This means applications that were segfaulting under qemu when encountering these
> instructions which is used in glibc libm functions for example.
> 
> The fallback for MFFSCDRN and MFFSCDRNI added in a later patch was also missing.
> 
> This patch restores the fallback to MFSS for these instructions on pre 3.0s ISAs
> as the hardware decoder would, fixing the segfaulting libm code. It doesn't have
> the fallback for 3.0 onwards to match hardware behaviour.
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>   target/ppc/insn32.decode           | 20 +++++++++++++-------
>   target/ppc/translate/fp-impl.c.inc | 22 ++++++++++++++++------
>   2 files changed, 29 insertions(+), 13 deletions(-)
> 
> v3 - drop fallback to MFFS for 3.0 ISA to match hardware
> v2 - switch to use decodetree pattern groups per feedback
> 
> diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
> index f8f589e9fd..4fcf3af8d0 100644
> --- a/target/ppc/insn32.decode
> +++ b/target/ppc/insn32.decode
> @@ -390,13 +390,19 @@ SETNBCR         011111 ..... ..... ----- 0111100000 -   @X_bi
>   
>   ### Move To/From FPSCR
>   
> -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
> +{
> +  # 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
> +  ]
> +}
>   
>   ### Decimal Floating-Point Arithmetic Instructions
>   
> diff --git a/target/ppc/translate/fp-impl.c.inc b/target/ppc/translate/fp-impl.c.inc
> index 57d8437851..874774eade 100644
> --- a/target/ppc/translate/fp-impl.c.inc
> +++ b/target/ppc/translate/fp-impl.c.inc
> @@ -568,6 +568,22 @@ static void store_fpscr_masked(TCGv_i64 fpscr, uint64_t clear_mask,
>       gen_helper_store_fpscr(cpu_env, fpscr_masked, st_mask);
>   }
>   
> +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;
> +}
> +
>   static bool trans_MFFS(DisasContext *ctx, arg_X_t_rc *a)
>   {
>       REQUIRE_FPU(ctx);
> @@ -584,7 +600,6 @@ static bool trans_MFFSCE(DisasContext *ctx, arg_X_t *a)
>   {
>       TCGv_i64 fpscr;
>   
> -    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
>       REQUIRE_FPU(ctx);
>   
>       gen_reset_fpstatus();
> @@ -597,7 +612,6 @@ static bool trans_MFFSCRN(DisasContext *ctx, arg_X_tb *a)
>   {
>       TCGv_i64 t1, fpscr;
>   
> -    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
>       REQUIRE_FPU(ctx);
>   
>       t1 = tcg_temp_new_i64();
> @@ -614,7 +628,6 @@ static bool trans_MFFSCDRN(DisasContext *ctx, arg_X_tb *a)
>   {
>       TCGv_i64 t1, fpscr;
>   
> -    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
>       REQUIRE_FPU(ctx);
>   
>       t1 = tcg_temp_new_i64();
> @@ -631,7 +644,6 @@ static bool trans_MFFSCRNI(DisasContext *ctx, arg_X_imm2 *a)
>   {
>       TCGv_i64 t1, fpscr;
>   
> -    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
>       REQUIRE_FPU(ctx);
>   
>       t1 = tcg_temp_new_i64();
> @@ -647,7 +659,6 @@ static bool trans_MFFSCDRNI(DisasContext *ctx, arg_X_imm3 *a)
>   {
>       TCGv_i64 t1, fpscr;
>   
> -    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
>       REQUIRE_FPU(ctx);
>   
>       t1 = tcg_temp_new_i64();
> @@ -661,7 +672,6 @@ 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);
>       REQUIRE_FPU(ctx);
>   
>       gen_reset_fpstatus();