Re: [PATCH v4 3/3] efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Fri, 21 Jun 2024 10:30:50 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu:

> On Thu, 20 Jun 2024 20:01:46 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > Up to UEFI spec, the type byte of CPER struct for ARM processor was
> > defined simply as:
> > 
> > Type at byte offset 4:
> > 
> > 	- Cache error
> > 	- TLB Error
> > 	- Bus Error
> > 	- Micro-architectural Error
> > 	All other values are reserved
> > 
> > Yet, there was no information about how this would be encoded.
> > 
> > Spec 2.9A errata corrected it by defining:
> > 
> > 	- Bit 1 - Cache Error
> > 	- Bit 2 - TLB Error
> > 	- Bit 3 - Bus Error
> > 	- Bit 4 - Micro-architectural Error
> > 	All other values are reserved
> > 
> > That actually aligns with the values already defined on older
> > versions at N.2.4.1. Generic Processor Error Section.
> > 
> > Spec 2.10 also preserve the same encoding as 2.9A
> > 
> > See: https://uefi.org/specs/UEFI/2.10/Apx_N_Common_Platform_Error_Record.html#arm-processor-error-information
> > 
> > Adjust CPER and GHES handling code for both generic and ARM
> > processors to properly handle UEFI 2.9A and 2.10 encoding.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> 
> I think you can avoid complexity of your masking solution.
> Cost is we don't have that function print that there were reserved bits
> set, but that could be easily handled at the caller including notifying
> on bits above the defined range which might be helpful.
> 
> > diff --git a/drivers/firmware/efi/cper-arm.c b/drivers/firmware/efi/cper-arm.c
> > index d9bbcea0adf4..4c101a09fd80 100644
> > --- a/drivers/firmware/efi/cper-arm.c
> > +++ b/drivers/firmware/efi/cper-arm.c  
> ...
> 
> >  	if (error_info & CPER_ARM_ERR_VALID_PROC_CONTEXT_CORRUPT) {
> > @@ -241,6 +232,7 @@ void cper_print_proc_arm(const char *pfx,
> >  	struct cper_arm_err_info *err_info;
> >  	struct cper_arm_ctx_info *ctx_info;
> >  	char newpfx[64], infopfx[65];
> > +	char error_type[120];
> >  
> >  	printk("%sMIDR: 0x%016llx\n", pfx, proc->midr);
> >  
> > @@ -289,9 +281,11 @@ void cper_print_proc_arm(const char *pfx,
> >  				       newpfx);
> >  		}
> >  
> > -		printk("%serror_type: %d, %s\n", newpfx, err_info->type,
> > -			err_info->type < ARRAY_SIZE(cper_proc_error_type_strs) ?
> > -			cper_proc_error_type_strs[err_info->type] : "unknown");
> > +		cper_bits_to_str(error_type, sizeof(error_type), err_info->type,
> > +				 cper_proc_error_type_strs,
> > +				 ARRAY_SIZE(cper_proc_error_type_strs),
> > +				 CPER_ARM_ERR_TYPE_MASK);  
> 
> Maybe drop this mask complexity and just use
> FIELD_GET() to extract the relevant field with no shift from 0.

IMO not using the function will make the code here more complex, as the
same code needs to be duplicated on two places: here and at ghes, where
the error bits are printed using pr_warn_ratelimited():

                cper_bits_to_str(error_type, sizeof(error_type), err_info->type,
                                 cper_proc_error_type_strs,
                                 ARRAY_SIZE(cper_proc_error_type_strs),
                                 CPER_ARM_ERR_TYPE_MASK);
 
                pr_warn_ratelimited(FW_WARN GHES_PFX
                                    "Unhandled processor error type: %s\n",


Also, other parts of CPER uses cper_bits_print() for the same reason:
to have the common print code handled inside a function instead of
repeating the same print pattern everywhere.

> > +		printk("%serror_type: %s\n", newpfx, error_type);
> >  		if (err_info->validation_bits & CPER_ARM_INFO_VALID_ERR_INFO) {
> >  			printk("%serror_info: 0x%016llx\n", newpfx,
> >  			       err_info->error_info);  
> 
> 
Regards,
Mauro