Crypto/nx842: Ignore invalid XER[S0] return error

Crypto/nx842: Ignore invalid XER[S0] return error

[Haren Myneni]

NX842 coprocessor sets 3rd bit in CR register with XER[S0] which is
nothing to do with NX request. On powerpc, XER[S0] will be set if
overflow in FPU and stays until another floating point operation is
executed. Since this bit can be set with other valuable return status,
ignore this XER[S0] value.

One of other bits (INITIATED, BUSY or REJECTED) will be returned for
any given NX request.

Signed-off-by: Haren Myneni <haren@us.ibm.com>

diff --git a/arch/powerpc/include/asm/icswx.h b/arch/powerpc/include/asm/icswx.h
index 9f8402b..27e588f 100644
--- a/arch/powerpc/include/asm/icswx.h
+++ b/arch/powerpc/include/asm/icswx.h
@@ -164,6 +164,7 @@ struct coprocessor_request_block {
 #define ICSWX_INITIATED		(0x8)
 #define ICSWX_BUSY		(0x4)
 #define ICSWX_REJECTED		(0x2)
+#define ICSWX_XERS0		(0x1)	/* undefined or set from XERSO. */
 
 static inline int icswx(__be32 ccw, struct coprocessor_request_block *crb)
 {
diff --git a/drivers/crypto/nx/nx-842-powernv.c b/drivers/crypto/nx/nx-842-powernv.c
index 9ef51fa..6bc33ae 100644
--- a/drivers/crypto/nx/nx-842-powernv.c
+++ b/drivers/crypto/nx/nx-842-powernv.c
@@ -442,6 +442,16 @@ static int nx842_powernv_function(const unsigned char *in, unsigned int inlen,
 			     (unsigned int)ccw,
 			     (unsigned int)be32_to_cpu(crb->ccw));
 
+	/*
+	 * NX842 coprocessor sets 3rd bit in CR register with XER[S0].
+	 * Setting XER[S0] happens if overflow in FPU and stays until
+	 * other floating operation is executed. XER[S0] value is nothing
+	 * to NX and no use to user. Since this bit can be set with other
+	 * return values, ignore this error.
+	 */
+	if (ret & ICSWX_XERS0)
+		ret &= ~ICSWX_XERS0;
+
 	switch (ret) {
 	case ICSWX_INITIATED:
 		ret = wait_for_csb(wmem, csb);
@@ -454,10 +464,6 @@ static int nx842_powernv_function(const unsigned char *in, unsigned int inlen,
 		pr_err_ratelimited("ICSWX rejected\n");
 		ret = -EPROTO;
 		break;
-	default:
-		pr_err_ratelimited("Invalid ICSWX return code %x\n", ret);
-		ret = -EPROTO;
-		break;
 	}
 
 	if (!ret)

Re: Crypto/nx842: Ignore invalid XER[S0] return error

[Segher Boessenkool]

On Fri, Dec 11, 2015 at 07:30:29PM -0800, Haren Myneni wrote:
> NX842 coprocessor sets 3rd bit in CR register with XER[S0] which is
> nothing to do with NX request. On powerpc, XER[S0] will be set if
> overflow in FPU and stays until another floating point operation is
> executed. Since this bit can be set with other valuable return status,
> ignore this XER[S0] value.

XER[SO] is the *integer* summary overflow bit.  It is set by OE=1
instructions ("addo" and the like), and can only be cleared explicitly
(using "mtxer").

The floating point overflow bit is FPSCR[OX].

> +	/*
> +	 * NX842 coprocessor sets 3rd bit in CR register with XER[S0].
> +	 * Setting XER[S0] happens if overflow in FPU and stays until
> +	 * other floating operation is executed. XER[S0] value is nothing
> +	 * to NX and no use to user. Since this bit can be set with other
> +	 * return values, ignore this error.
> +	 */
> +	if (ret & ICSWX_XERS0)
> +		ret &= ~ICSWX_XERS0;

You can just always clear it, there is no need to check if it is set first.


Segher

Re: Crypto/nx842: Ignore invalid XER[S0] return error

[Haren Myneni]

On 12/12/2015 12:43 AM, Segher Boessenkool wrote:
> On Fri, Dec 11, 2015 at 07:30:29PM -0800, Haren Myneni wrote:
>> NX842 coprocessor sets 3rd bit in CR register with XER[S0] which is
>> nothing to do with NX request. On powerpc, XER[S0] will be set if
>> overflow in FPU and stays until another floating point operation is
>> executed. Since this bit can be set with other valuable return status,
>> ignore this XER[S0] value.
> 
> XER[SO] is the *integer* summary overflow bit.  It is set by OE=1
> instructions ("addo" and the like), and can only be cleared explicitly
> (using "mtxer").

Thanks for the correct description. I was told XER[S0] is floating overflow from FPU.

> 
> The floating point overflow bit is FPSCR[OX].
> 
>> +	/*
>> +	 * NX842 coprocessor sets 3rd bit in CR register with XER[S0].
>> +	 * Setting XER[S0] happens if overflow in FPU and stays until
>> +	 * other floating operation is executed. XER[S0] value is nothing
>> +	 * to NX and no use to user. Since this bit can be set with other
>> +	 * return values, ignore this error.
>> +	 */
>> +	if (ret & ICSWX_XERS0)
>> +		ret &= ~ICSWX_XERS0;
> 
> You can just always clear it, there is no need to check if it is set first.

Do you mean reset this before calling NX? I believe NX coprocessor should not set CR bit as XER[S0] nothing to do with NX request and it is no use. NX is copying this CR bit with XER. But reset XER[S0] has to be done before NX request. We can not do this in icswx since this instruction can be used by other coprocessors in future. But I am not comfortable clearing as we are not touching this XER in the driver or result of NX operation. So I am proposing this patch to fix this not proper NX behaviour - ignores CR bit. 

If you are OK, I can repost the patch with proper description.

Thanks
Haren 


  
> 
> 
> Segher
> 

Re: Crypto/nx842: Ignore invalid XER[S0] return error

[Segher Boessenkool]

On Sat, Dec 12, 2015 at 03:01:26PM -0800, Haren Myneni wrote:
> On 12/12/2015 12:43 AM, Segher Boessenkool wrote:
> > On Fri, Dec 11, 2015 at 07:30:29PM -0800, Haren Myneni wrote:
> >> NX842 coprocessor sets 3rd bit in CR register with XER[S0] which is
> >> nothing to do with NX request. On powerpc, XER[S0] will be set if
> >> overflow in FPU and stays until another floating point operation is
> >> executed. Since this bit can be set with other valuable return status,
> >> ignore this XER[S0] value.
> > 
> > XER[SO] is the *integer* summary overflow bit.  It is set by OE=1
> > instructions ("addo" and the like), and can only be cleared explicitly
> > (using "mtxer").
> 
> Thanks for the correct description. I was told XER[S0] is floating overflow from FPU.

You can use the Power ISA document to make sure for yourself.

> >> +	if (ret & ICSWX_XERS0)
> >> +		ret &= ~ICSWX_XERS0;
> > 
> > You can just always clear it, there is no need to check if it is set first.
> 
> Do you mean reset this before calling NX?

I mean write this as simply

+	ret &= ~ICSWX_XERS0;

(without any "if").

> I believe NX coprocessor should not set CR bit as XER[S0] nothing to do with NX request and it is no use.

Many instructions set the CR bit to XER[SO] -- store conditional, slbfee.,
and all "normal" dot insns and of course cmp[l][i].  Or, shorter: "everything"
does this.

> NX is copying this CR bit with XER. But reset XER[S0] has to be done before NX request.

Only if you care what the final value of bit 3 in the CR will be.  Even
in the unusual case where you want to look at all CR field bits at once
it is cheap to just mask out the bit (as you do).

> We can not do this in icswx since this instruction can be used by other coprocessors in future. But I am not comfortable clearing as we are not touching this XER in the driver or result of NX operation. So I am proposing this patch to fix this not proper NX behaviour - ignores CR bit. 

I really wouldn't call it "not proper", that makes it sound like there
is an implementation bug or design mistake.  Instead, you could say that
your switch statement looks at the values of bits 0, 1, and 2, so you
just mask those -- you do not care about the value of bit 3, so you mask
it out.

Something like

+	/* Mask out the bits we do not care about. */
+	ret &= ~ICSWX_XERS0;


Segher

Re: Crypto/nx842: Ignore invalid XER[S0] return error

[Haren Myneni]

On 12/12/2015 04:05 PM, Segher Boessenkool wrote:
> On Sat, Dec 12, 2015 at 03:01:26PM -0800, Haren Myneni wrote:
>> On 12/12/2015 12:43 AM, Segher Boessenkool wrote:
>>> On Fri, Dec 11, 2015 at 07:30:29PM -0800, Haren Myneni wrote:
>>>> NX842 coprocessor sets 3rd bit in CR register with XER[S0] which is
>>>> nothing to do with NX request. On powerpc, XER[S0] will be set if
>>>> overflow in FPU and stays until another floating point operation is
>>>> executed. Since this bit can be set with other valuable return status,
>>>> ignore this XER[S0] value.
>>>
>>> XER[SO] is the *integer* summary overflow bit.  It is set by OE=1
>>> instructions ("addo" and the like), and can only be cleared explicitly
>>> (using "mtxer").
>>
>> Thanks for the correct description. I was told XER[S0] is floating overflow from FPU.
> 
> You can use the Power ISA document to make sure for yourself.
> 
>>>> +	if (ret & ICSWX_XERS0)
>>>> +		ret &= ~ICSWX_XERS0;
>>>
>>> You can just always clear it, there is no need to check if it is set first.
>>
>> Do you mean reset this before calling NX?
> 
> I mean write this as simply
> 
> +	ret &= ~ICSWX_XERS0;
> 
> (without any "if").
> 
>> I believe NX coprocessor should not set CR bit as XER[S0] nothing to do with NX request and it is no use.
> 
> Many instructions set the CR bit to XER[SO] -- store conditional, slbfee.,
> and all "normal" dot insns and of course cmp[l][i].  Or, shorter: "everything"
> does this.
> 
>> NX is copying this CR bit with XER. But reset XER[S0] has to be done before NX request.
> 
> Only if you care what the final value of bit 3 in the CR will be.  Even
> in the unusual case where you want to look at all CR field bits at once
> it is cheap to just mask out the bit (as you do).
> 
>> We can not do this in icswx since this instruction can be used by other coprocessors in future. But I am not comfortable clearing as we are not touching this XER in the driver or result of NX operation. So I am proposing this patch to fix this not proper NX behaviour - ignores CR bit. 
> 
> I really wouldn't call it "not proper", that makes it sound like there
> is an implementation bug or design mistake.  Instead, you could say that
> your switch statement looks at the values of bits 0, 1, and 2, so you
> just mask those -- you do not care about the value of bit 3, so you mask
> it out.
> 
> Something like
> 
> +	/* Mask out the bits we do not care about. */
> +	ret &= ~ICSWX_XERS0;

icswx RFC says CR[3] bit can be undefined or set to XER[S0]. So was thinking NX should have ignored since no use to user. I may be wrong.

Thanks for your help. I will repost this patch with your suggestions.

Thanks
Haren
> 
> 
> Segher
>