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

[Christophe Leroy]

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 at 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 ?

> +		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 ?

>   
>   	h = (struct rtas_error_log *)&savep[1];
>   	/* Use the per cpu buffer from paca to store rtas error log */
> @@ -483,11 +492,22 @@ int pSeries_system_reset_exception(struct pt_regs *regs)
>   #endif
>   
>   	if (fwnmi_active) {
> -		struct rtas_error_log *errhdr = fwnmi_get_errinfo(regs);
> -		if (errhdr) {
> -			/* XXX Should look at FWNMI information */
> -		}
> -		fwnmi_release_errinfo();
> +		unsigned long *savep;
> +
> +		/*
> +		 * Firmware (PowerVM and KVM) saves r3 to a save area like
> +		 * machine check, which is not exactly what PAPR (2.9)
> +		 * suggests but there is no way to detect otherwise, so this
> +		 * is the interface now.
> +		 *
> +		 * System resets do not save any error log or require an
> +		 * "ibm,nmi-interlock" rtas call to release.
> +		 */
> +
> +		savep = fwnmi_get_savep(regs);
> +		/* restore original r3 */
> +		if (savep)
> +			regs->gpr[3] = be64_to_cpu(savep[0]);
>   	}
>   
>   	if (smp_handle_nmi_ipi(regs))
> 

Christophe