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

[Mark Cave-Ayland]

On 27/05/16 02:36, David Gibson wrote:
> On Thu, May 26, 2016 at 07:54:46AM +0100, Mark Cave-Ayland wrote:
>> On 19/05/16 13:11, Talha Imran wrote:
>>
>> (Adding CC to David as maintainer)
>>
>>> 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 <address@hidden>
>>> ---
>>>  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);
>>>
>>
>>
>> ATB,
> 
> Sorry, I'm not sure what that means.

Oh that's just my standard signature - no need to worry about that :)


ATB,

Mark.