[Qemu-devel] [PATCH] target-ppc/fpu_helper: Fix efscmp* instructions handling

[Qemu-devel] [PATCH] target-ppc/fpu_helper: Fix efscmp* instructions handling

[Talha Imran]

With specification at hand from the reference manual from Freescale
http://cache.nxp.com/files/32bit/doc/ref_manual/SPEPEM.pdf , I have found a fix
to efscmp* instructions handling in QEMU.

efscmp* instructions in QEMU set crD (Condition Register nibble) values as 
(0b0100 << 2) = 0b10000 (consider the HELPER_SINGLE_SPE_CMP macro which left 
shifts the value returned by efscmp* handler by 2 bits). A value of 0b10000 is
not correct according the to the reference manual.

The reference manual expects efscmp* instructions to return a value of 0bx1xx.
Please find attached a patch which disables left shifting in
HELPER_SINGLE_SPE_CMP macro. This macro is used by efscmp* and efstst*
instructions only. efstst* instruction handlers, in turn, call efscmp* handlers
too.

*Explanation:*
Traditionally, each crD (condition register nibble) consist of 4 bits, which is
set by comparisons as follows:
crD = W X Y Z
where
W = Less than
X = Greater than
Y = Equal to

However, efscmp* instructions being a special case return a binary result.
(efscmpeq will set the crD = 0bx1xx iff when op1 == op2 and 0bx0xx otherwise;
i.e. there is no notion of different crD values based on Less than, Greater
than and Equal to).

This effectively means that crD will store a "Greater than" comparison result
iff efscmp* instruction comparison is TRUE. Compiler exploits this feature by
checking for "Branch if Less than or Equal to" (ble instruction) OR "Branch if
Greater than" (bgt instruction) for Branch if FALSE OR Branch if TRUE
respectively after an efscmp* instruction. This can be seen in a assembly code
snippet below:

27          if (__real__ x != 3.0f || __imag__ x != 4.0f)
10000498:   lwz r10,8(r31)
1000049c:   lis r9,16448
100004a0:   efscmpeq cr7,r10,r9
100004a4:   ble- cr7,0x100004b8 <bar+60>  //jump to abort() call
100004a8:   lwz r10,12(r31)
100004ac:   lis r9,16512
100004b0:   efscmpeq cr7,r10,r9
100004b4:   bgt- cr7,0x100004bc <bar+64>  //skip abort() call
28            abort ();
100004b8:   bl 0x10000808 <abort>

Signed-off-by: Talha Imran <talha_imran@mentor.com>
---
 target-ppc/fpu_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
index b67ebca..6fd56a8 100644
--- a/target-ppc/fpu_helper.c
+++ b/target-ppc/fpu_helper.c
@@ -1442,7 +1442,7 @@ static inline uint32_t efststeq(CPUPPCState *env, uint32_t op1, uint32_t op2)
 #define HELPER_SINGLE_SPE_CMP(name)                                     \
     uint32_t helper_e##name(CPUPPCState *env, uint32_t op1, uint32_t op2) \
     {                                                                   \
-        return e##name(env, op1, op2) << 2;                             \
+        return e##name(env, op1, op2);                                  \
     }
 /* efststlt */
 HELPER_SINGLE_SPE_CMP(fststlt);
-- 
1.9.1

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target-ppc/fpu_helper: Fix efscmp* instructions handling

[David Gibson]

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

On Thu, May 19, 2016 at 05:11:35PM +0500, Talha Imran wrote:
> With specification at hand from the reference manual from Freescale
> http://cache.nxp.com/files/32bit/doc/ref_manual/SPEPEM.pdf , I have found a fix
> to efscmp* instructions handling in QEMU.
> 
> efscmp* instructions in QEMU set crD (Condition Register nibble) values as 
> (0b0100 << 2) = 0b10000 (consider the HELPER_SINGLE_SPE_CMP macro which left 
> shifts the value returned by efscmp* handler by 2 bits). A value of 0b10000 is
> not correct according the to the reference manual.
> 
> The reference manual expects efscmp* instructions to return a value of 0bx1xx.
> Please find attached a patch which disables left shifting in
> HELPER_SINGLE_SPE_CMP macro. This macro is used by efscmp* and efstst*
> instructions only. efstst* instruction handlers, in turn, call efscmp* handlers
> too.
> 
> *Explanation:*
> Traditionally, each crD (condition register nibble) consist of 4 bits, which is
> set by comparisons as follows:
> crD = W X Y Z
> where
> W = Less than
> X = Greater than
> Y = Equal to
> 
> However, efscmp* instructions being a special case return a binary result.
> (efscmpeq will set the crD = 0bx1xx iff when op1 == op2 and 0bx0xx otherwise;
> i.e. there is no notion of different crD values based on Less than, Greater
> than and Equal to).
> 
> This effectively means that crD will store a "Greater than" comparison result
> iff efscmp* instruction comparison is TRUE. Compiler exploits this feature by
> checking for "Branch if Less than or Equal to" (ble instruction) OR "Branch if
> Greater than" (bgt instruction) for Branch if FALSE OR Branch if TRUE
> respectively after an efscmp* instruction. This can be seen in a assembly code
> snippet below:
> 
> 27          if (__real__ x != 3.0f || __imag__ x != 4.0f)
> 10000498:   lwz r10,8(r31)
> 1000049c:   lis r9,16448
> 100004a0:   efscmpeq cr7,r10,r9
> 100004a4:   ble- cr7,0x100004b8 <bar+60>  //jump to abort() call
> 100004a8:   lwz r10,12(r31)
> 100004ac:   lis r9,16512
> 100004b0:   efscmpeq cr7,r10,r9
> 100004b4:   bgt- cr7,0x100004bc <bar+64>  //skip abort() call
> 28            abort ();
> 100004b8:   bl 0x10000808 <abort>
> 
> Signed-off-by: Talha Imran <talha_imran@mentor.com>

Does this patch supersede the earlier patch you posted for efcmp
instructions on e500v1?  Or is it in addition to that patch?

> ---
>  target-ppc/fpu_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
> index b67ebca..6fd56a8 100644
> --- a/target-ppc/fpu_helper.c
> +++ b/target-ppc/fpu_helper.c
> @@ -1442,7 +1442,7 @@ static inline uint32_t efststeq(CPUPPCState *env, uint32_t op1, uint32_t op2)
>  #define HELPER_SINGLE_SPE_CMP(name)                                     \
>      uint32_t helper_e##name(CPUPPCState *env, uint32_t op1, uint32_t op2) \
>      {                                                                   \
> -        return e##name(env, op1, op2) << 2;                             \
> +        return e##name(env, op1, op2);                                  \
>      }
>  /* efststlt */
>  HELPER_SINGLE_SPE_CMP(fststlt);

-- 
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: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target-ppc/fpu_helper: Fix efscmp* instructions handling

[Imran, Talha]

On 05/27/2016 06:37 AM, David Gibson wrote:
> On Thu, May 19, 2016 at 05:11:35PM +0500, Talha Imran wrote:
>> With specification at hand from the reference manual from Freescale
>> http://cache.nxp.com/files/32bit/doc/ref_manual/SPEPEM.pdf , I have found a fix
>> to efscmp* instructions handling in QEMU.
>>
...
>> Signed-off-by: Talha Imran <talha_imran@mentor.com>
>
> Does this patch supersede the earlier patch you posted for efcmp
> instructions on e500v1?  Or is it in addition to that patch?
>
[talha]: Yes, it supersedes the earlier patches. This is a better solution.

>> ---
>>  target-ppc/fpu_helper.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
>> index b67ebca..6fd56a8 100644
>> --- a/target-ppc/fpu_helper.c
>> +++ b/target-ppc/fpu_helper.c
>> @@ -1442,7 +1442,7 @@ static inline uint32_t efststeq(CPUPPCState *env, uint32_t op1, uint32_t op2)
>>  #define HELPER_SINGLE_SPE_CMP(name)                                     \
>>      uint32_t helper_e##name(CPUPPCState *env, uint32_t op1, uint32_t op2) \
>>      {                                                                   \
>> -        return e##name(env, op1, op2) << 2;                             \
>> +        return e##name(env, op1, op2);                                  \
>>      }
>>  /* efststlt */
>>  HELPER_SINGLE_SPE_CMP(fststlt);
>

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target-ppc/fpu_helper: Fix efscmp* instructions handling

[David Gibson]

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

On Fri, May 27, 2016 at 07:43:46AM +0000, Imran, Talha wrote:
> 
> On 05/27/2016 06:37 AM, David Gibson wrote:
> > On Thu, May 19, 2016 at 05:11:35PM +0500, Talha Imran wrote:
> >> With specification at hand from the reference manual from Freescale
> >> http://cache.nxp.com/files/32bit/doc/ref_manual/SPEPEM.pdf , I have found a fix
> >> to efscmp* instructions handling in QEMU.
> >>
> ...
> >> Signed-off-by: Talha Imran <talha_imran@mentor.com>
> >
> > Does this patch supersede the earlier patch you posted for efcmp
> > instructions on e500v1?  Or is it in addition to that patch?
> >
> [talha]: Yes, it supersedes the earlier patches. This is a better
> solution.

Ok, I managed to read up enough on the SPE instructions to convince
myself this was correct, so I've merged it to ppc-for-2.7.

> 
> >> ---
> >>  target-ppc/fpu_helper.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
> >> index b67ebca..6fd56a8 100644
> >> --- a/target-ppc/fpu_helper.c
> >> +++ b/target-ppc/fpu_helper.c
> >> @@ -1442,7 +1442,7 @@ static inline uint32_t efststeq(CPUPPCState *env, uint32_t op1, uint32_t op2)
> >>  #define HELPER_SINGLE_SPE_CMP(name)                                     \
> >>      uint32_t helper_e##name(CPUPPCState *env, uint32_t op1, uint32_t op2) \
> >>      {                                                                   \
> >> -        return e##name(env, op1, op2) << 2;                             \
> >> +        return e##name(env, op1, op2);                                  \
> >>      }
> >>  /* efststlt */
> >>  HELPER_SINGLE_SPE_CMP(fststlt);
> >
> 
> 

-- 
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: 819 bytes --]