Re: [PATCH 1/2] efi/cper, cxl: Decode CXL Protocol Error Section

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

> >> +	if (prot_err->valid_bits & PROT_ERR_VALID_SERIAL_NUMBER) {
> >> +		switch (prot_err->agent_type) {
> >> +		case RCD:
> >> +			pr_info("%s lower_dw: 0x%08x, upper_dw: 0x%08x\n", pfx,
> >> +				prot_err->dev_serial_num.lower_dw,
> >> +				prot_err->dev_serial_num.upper_dw);
> >> +			break;
> >> +		default:
> >> +			break;
> >> +		}
> >> +	}  
> > Nice to pretty print the cap structure and appropriate DVSEC and Error logs as well
> > if valid, but that could be a follow up patch.  
> 
> Hmm, I have added the Error log decoding support in the next patch.
> 
> But, I did not include cap structure and DVSEC though as I initially 
> thought it might not be
> that important to parse the error. Do you recommend adding them?

Given it would be straight forward to do, probably better to supply slightly too
much info than miss one particular chunk out.
> >  
> >> +struct cper_sec_prot_err {
> >> +	u64			valid_bits;
> >> +	u8			agent_type;
> >> +	u8			reserved[7];
> >> +	union {
> >> +		u64		agent_addr;  
> > Perhaps useful to add a few comments to say when the different union
> > elements are relevant.  Perhaps also name the field as per the spec
> > which would give the overall union the agent_address.
> > I admit that is a little confusing with the union element having
> > the same name for when it's treated as an address.
> > Perhaps call the union element rcrb_base_addr?  
> 
> Okay, probably something like this?
> 
>      union {
>                      u64                rcrb_base_addr;
>                      struct {
>                                      u8    function;
>                                      u8    device;
>                                      u8    bus;
>                                      u16  segment;
>                                      u8    reserved_1[3];
>                      };
>      } agent_addr;
> 
> And change printing from prot_err->segment to prot_err->agent_addr.segment
> and so on.. Please ignore my indentation/alignments here..

Looks good to me.


> 
> >  
> >> +		struct {
> >> +			u8	function;
> >> +			u8	device;
> >> +			u8	bus;
> >> +			u16	segment;
> >> +			u8	reserved_1[3];
> >> +		};
> >> +	};
> >> +	struct {
> >> +		u16		vendor_id;	
> >> +		u16		device_id;
> >> +		u16		sub_vendor_id;
> >> +		u16		sub_device_id;  
> > This is always fun.  Far as I can tell not all PCI elements
> > have subsystem IDs - so who knows what goes in these..
> > Also, there is wonderfully no such thing as a PCI subsystem device ID.
> > It's just called subsystem ID in the PCI spec.  
> 
> Thanks for correcting this. Will fix.

UEFI spec is inconsistent with PCIe :( 


> 
> >  
> >> +		u8		class_code[2];  
> > Why treat class code as two u8s?  If doing so, shall
> > we name them?  base_class_code, sub_class_code?  
> 
> Just followed the PCI CPER decoding here.. Will name them if that makes
> more sense..
> 

ok. Fine as is.


> 
> >
> > 	} device_id;
> >  
> >> +	struct {
> >> +		u32		lower_dw;
> >> +		u32		upper_dw;
> >> +	}			dev_serial_num;
> >> +	u8			capability[60];
> >> +	u16			dvsec_len;
> >> +	u16			err_len;
> >> +	u8			reserved_2[4];  
> > You could add a [] array on the end to make it clear there are more elements.
> > That's not a perfect solution though given there are two different variable length
> > fields on the end.  They aren't that variable (as defined by the structures
> > in the CXL spec) however the complexity comes from the fact that they may not
> > be valid / lengths will be 0 (I assume lengths will be 0
> > if not valid anyway, the spec doesn't seem to say either way...)  
> 
> Hmm, I probably got the idea to do this way by referring to "efi/cper-x86.c"
> (N.2.4.2 IA32/X64 Processor Error Section in UEFI spec) and probably at few
> places under the APEI tables.

Ah. I didn't check for precedence.  Down to maintainer preference then.

> 
> Also to note, defining the new variable array member the parsing needs 
> to be changed
> accordingly in the next patch.  Add dvsec length to this new variable array
> member to get to the Error Log field. Am I right?

Yes.

Jonathan