Re: [Qemu-ppc] [PATCH v2] fpu_helper.c: fix helper_fpscr_clrbit() function

[David Gibson]

On Mon, Jun 18, 2018 at 11:50:24AM -0400, John Arbuckle wrote:
> Fix the helper_fpscr_clrbit() function so it correctly
> sets the FEX and VX bits.
> 
> Determining the value for the Floating Point Status and Control
> Register's (FPSCR) FEX bit is suppose to be done like this:
> 
> FEX = (VX & VE) | (OX & OE) | (UX & UE) | (ZX & ZE) | (XX & XE))
> 
> It is described as "the logical OR of all the floating-point exception bits
> masked by their respective enable bits". It was not implemented correctly. 
> The value of FEX would stay on even when all other bits were set to off.
> 
> The VX bit is described as "the logical OR of all of the invalid operation 
> exceptions". This bit was also not implemented correctly. It too would stay
> on when all the other bits were set to off.
> 
> My main source of information is an IBM document called: 
> 
> PowerPC Microprocessor Family:
> The Programming Environments for 32-Bit Microprocessors
> 
> Page 62 is where the FPSCR information is located.
> 
> This is an older copy than the one I use but it is still very useful:
> https://www.pdfdrive.net/powerpc-microprocessor-family-the-programming-environments-for-32-e3087633.html
> 
> I use a G3 and G5 iMac to compare bit values with QEMU. This patch fixed all 
> the problems I was having with these bits.
> 
> Signed-off-by: John Arbuckle <address@hidden>
> ---
> v2 changes:
> - Removed the FPSCR_VX case because it is not a bit that can be set directly.
> - Replaced previous code with predefined macros fpscr_ix and
> fpscr_eex.

Thanks for the updated version, this is much easier to review.

This is definitely better than what we have, and I've applied it to
ppc-for-3.0.  The existing code is pretty eye-watering, and longer
term I do wonder if it would be better to just compute the VX and FEX
bits when we actually read the fpscr, rather than incrementally.

I think there's also one case you've missed..

> 
>  target/ppc/fpu_helper.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index d31a933cbb..7714bfe0f9 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -325,6 +325,34 @@ void helper_fpscr_clrbit(CPUPPCState *env, uint32_t bit)
>          case FPSCR_RN:
>              fpscr_set_rounding_mode(env);
>              break;
> +        case FPSCR_VXSNAN:
> +        case FPSCR_VXISI:
> +        case FPSCR_VXIDI:
> +        case FPSCR_VXZDZ:
> +        case FPSCR_VXIMZ:
> +        case FPSCR_VXVC:
> +        case FPSCR_VXSOFT:
> +        case FPSCR_VXSQRT:
> +        case FPSCR_VXCVI:
> +            if (!fpscr_ix) {
> +                /* Set VX bit to zero */
> +                env->fpscr &= ~(1 << FPSCR_VX);
> +            }

This can clear the VX bit, which could affect the expected value of
the FEX bit, but you won't actually recompute it in that case.

> +            break;
> +        case FPSCR_OX:
> +        case FPSCR_UX:
> +        case FPSCR_ZX:
> +        case FPSCR_XX:
> +        case FPSCR_VE:
> +        case FPSCR_OE:
> +        case FPSCR_UE:
> +        case FPSCR_ZE:
> +        case FPSCR_XE:
> +            if (!fpscr_eex) {
> +                /* Set the FEX bit */
> +                env->fpscr &= ~(1 << FPSCR_FEX);
> +            }
> +            break;
>          default:
>              break;
>          }

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature