Re: [PATCH v2 08/14] powerpc/pseries/ras: fwnmi sreset should not interlock

[Nicholas Piggin <npiggin@gmail.com>]

Christophe Leroy's on April 4, 2020 12:35 am:
> 
> 
> Le 03/04/2020 à 15:26, Nicholas Piggin a écrit :
>> PAPR does not specify that fwnmi sreset should be interlocked, and
>> PowerVM (and therefore now QEMU) do not require it.
>> 
>> These "ibm,nmi-interlock" calls are ignored by firmware, but there
>> is a possibility that the sreset could have interrupted a machine
>> check and release the machine check's interlock too early, corrupting
>> it if another machine check came in.
>> 
>> This is an extremely rare case, but it should be fixed for clarity
>> and reducing the code executed in the sreset path. Firmware also
>> does not provide error information for the sreset case to look at, so
>> remove that comment.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   arch/powerpc/platforms/pseries/ras.c | 48 ++++++++++++++++++++--------
>>   1 file changed, 34 insertions(+), 14 deletions(-)
>> 
>> diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
>> index a40598e6e525..833ae34b7fec 100644
>> --- a/arch/powerpc/platforms/pseries/ras.c
>> +++ b/arch/powerpc/platforms/pseries/ras.c
>> @@ -406,6 +406,20 @@ static inline struct rtas_error_log *fwnmi_get_errlog(void)
>>   	return (struct rtas_error_log *)local_paca->mce_data_buf;
>>   }
>>   
>> +static unsigned long *fwnmi_get_savep(struct pt_regs *regs)
>> +{
>> +	unsigned long savep_ra;
>> +
>> +	/* Mask top two bits */
>> +	savep_ra = regs->gpr[3] & ~(0x3UL << 62);
>> +	if (!VALID_FWNMI_BUFFER(savep_ra)) {
>> +		printk(KERN_ERR "FWNMI: corrupt r3 0x%016lx\n", regs->gpr[3]);
> 
> Can't you use pr_err() instead ?

I think so.

>> +		return NULL;
>> +	}
>> +
>> +	return __va(savep_ra);
>> +}
>> +
>>   /*
>>    * Get the error information for errors coming through the
>>    * FWNMI vectors.  The pt_regs' r3 will be updated to reflect
>> @@ -423,20 +437,15 @@ static inline struct rtas_error_log *fwnmi_get_errlog(void)
>>    */
>>   static struct rtas_error_log *fwnmi_get_errinfo(struct pt_regs *regs)
>>   {
>> -	unsigned long savep_ra;
>>   	unsigned long *savep;
>>   	struct rtas_error_log *h;
>>   
>> -	/* Mask top two bits */
>> -	savep_ra = regs->gpr[3] & ~(0x3UL << 62);
>> -
>> -	if (!VALID_FWNMI_BUFFER(savep_ra)) {
>> -		printk(KERN_ERR "FWNMI: corrupt r3 0x%016lx\n", regs->gpr[3]);
>> +	savep = fwnmi_get_savep(regs);
>> +	if (!savep)
>>   		return NULL;
>> -	}
>>   
>> -	savep = __va(savep_ra);
>> -	regs->gpr[3] = be64_to_cpu(savep[0]);	/* restore original r3 */
>> +	/* restore original r3 */
>> +	regs->gpr[3] = be64_to_cpu(savep[0]);
> 
> Is it needed to change the location of the comment ?

No, I originally had other changes I think. Will fix.

Thanks,
Nick