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

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

[John Arbuckle]

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 <programmingkidx@gmail.com>
---
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. 

 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);
+            }
+            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;
         }
-- 
2.14.3 (Apple Git-98)

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3668 bytes --]

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 <programmingkidx@gmail.com>
> ---
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]