Re: [PATCH 1/3] efi/cper, cxl: Decode CXL Component Events General Media Event Record

[Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>]

Hi Ira,

On 10/12/2023 5:25 PM, Ira Weiny wrote:
> Smita Koralahalli wrote:
>> Add support for decoding CXL Component Events General Media Event Record
>> as defined in CXL rev 3.0 section 8.2.9.2.1.1.
>>
>> All the event records as defined in Event Record Identifier field of the
>> Common Event Record Format in CXL rev 3.0 section 8.2.9.2.1 follow the
>> CPER format for representing the hardware errors if reported by a
>> platform.
>>
>> According to the CPER format, each event record including the General
>> Media is logged as a CXL Component Event as defined in UEFI 2.10
>> Section N.2.14 and is identified by a UUID as defined by Event Record
>> Identifier field in Common Event Record Format of CXL rev 3.0 section
>> 8.2.9.2.1. CXL Component Event Log field in Component Events Section
>> corresponds to the component/event specified by the section type UUID.
>>
>> Add support for decoding CXL Component Events as defined in UEFI 2.10
>> Section N.2.14 and decoding Common Event Record as defined in CXL rev 3.0
>> section 8.2.9.2.1.
>>
>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> 
> [snip]
> 
>> +
>> +/*
>> + * Compute Express Link Common Event Record
>> + * CXL rev 3.0 section 8.2.9.2.1; Table 8-42
>> + */
>> +struct common_event_record {
>> +	u8 identifier[16];
> 
> I interpreted the CPER structure as not having this identifier here.
> 
>  From Section N.2.14:
> 
> "For the CXL Component Event Log: Refer to the Common Event Record field
> (Offset 16) of the Events Record Format for each CXL component."
> 
> This implies that the data coming from the CPER record starts at length.

Thanks for pointing this out. According to Spec, you are right. Our 
records did show up the identifier. Hence, I added that field. We should 
probably fix it from our end.

Meanwhile, I'm taking a look at your patches.

Thanks,
Smita
> 
> I'd be happy if my interpretation is wrong.  But if I am wrong then there
> is no reason to duplicate these structures which are already defined in
> cxlmem.h
> 
> I was plumbing up the cxl code to make a copy from length on and splice in
> the uuid from the guid passed from the Section Type.
> 
> Ira
> 
>> +	u8 length;
>> +	u8 flags[3];
>> +	u16 handle;
>> +	u16 related_handle;
>> +	u64 timestamp;
>> +	u8 maint_op_class;
>> +	u8 reserved[15];
>> +};
>> +
>> +/*
>> + * CXL General Media Event Record - GMER
>> + * CXL rev 3.0 section 8.2.9.2.1.1; Table 8-43
>> + */
>> +struct cper_sec_gen_media {
>> +	struct common_event_record record;
>> +	u64 dpa;
>> +	u8 descriptor;
>> +	u8 type;
>> +	u8 transaction_type;
>> +	u16 validity_flags;
>> +	u8 channel;
>> +	u8 rank;
>> +	u8 device[3];
>> +	u8 comp_id[16];
>> +	u8 reserved[46];
>> +};
>> +
> 
> [snip]