| Message ID | 20231220-cxl-cper-v5-0-1bb8a4ca2c7a@intel.com |
|---|---|
| Headers | show |
| Series | efi/cxl-cper: Report CPER CXL component events through trace events | expand |
On Wed, Dec 20, 2023 at 04:17:27PM -0800, Ira Weiny wrote: > cxl/trace: Pass uuid explicitly to event traces Nit: s/uuid/UUID/ would match the patches below > cxl/events: Promote CXL event structures to a core header > cxl/events: Create common event UUID defines > cxl/events: Remove passing a UUID to known event traces > cxl/events: Separate UUID from event structures > cxl/events: Create a CXL event union > acpi/ghes: Process CXL Component Events > PCI: Define scoped based management functions "scope based" unless I'm misunderstanding something. Maybe "cleanup and guard functions"? "management" is pretty generic.
On Wed, 20 Dec 2023 16:17:27 -0800 Ira Weiny <ira.weiny@intel.com> wrote: > Series status/background > ======================== > > Smita has been a great help with this series. Thank you again! > > Smita's testing found that the GHES code ended up printing the events > twice. This version avoids the duplicate print by calling the callback > from the GHES code instead of the EFI code as suggested by Dan. I'm not sure this is working as intended. There is nothing gating the call in ghes_proc() of ghes_print_estatus() and now the EFI code handling that pretty printed things is missing we get the horrible kernel logging for an unknown block instead. So I think we need some minimal code in cper.c to match the guids then not log them (on basis we are arguing there is no need for new cper records). Otherwise we are in for some messy kernel logs Something like: {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 {1}[Hardware Error]: event severity: recoverable {1}[Hardware Error]: Error 0, type: recoverable {1}[Hardware Error]: section type: unknown, fbcd0a77-c260-417f-85a9-088b1621eba6 {1}[Hardware Error]: section length: 0x90 {1}[Hardware Error]: 00000000: 00000090 00000007 00000000 0d938086 ................ {1}[Hardware Error]: 00000010: 00100000 00000000 00040000 00000000 ................ {1}[Hardware Error]: 00000020: 00000000 00000000 00000000 00000000 ................ {1}[Hardware Error]: 00000030: 00000000 00000000 00000000 00000000 ................ {1}[Hardware Error]: 00000040: 00000000 00000000 00000000 00000000 ................ {1}[Hardware Error]: 00000050: 00000000 00000000 00000000 00000000 ................ {1}[Hardware Error]: 00000060: 00000000 00000000 00000000 00000000 ................ {1}[Hardware Error]: 00000070: 00000000 00000000 00000000 00000000 ................ {1}[Hardware Error]: 00000080: 00000000 00000000 00000000 00000000 ................ cxl_general_media: memdev=mem1 host=0000:10:00.0 serial=4 log=Informational : time=0 uuid=fbcd0a77-c260-417f-85a9-088b1621eba6 len=0 flags='' handle=0 related_handle=0 maint_op_class=0 : dpa=0 dpa_flags='' descriptor='' type='ECC Error' transaction_type='Unknown' channel=0 rank=0 device=0 comp_id=00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 validity_flags='' (I'm filling the record with 0s currently)
On 1/8/2024 8:58 AM, Jonathan Cameron wrote: > On Wed, 20 Dec 2023 16:17:27 -0800 > Ira Weiny <ira.weiny@intel.com> wrote: > >> Series status/background >> ======================== >> >> Smita has been a great help with this series. Thank you again! >> >> Smita's testing found that the GHES code ended up printing the events >> twice. This version avoids the duplicate print by calling the callback >> from the GHES code instead of the EFI code as suggested by Dan. > > I'm not sure this is working as intended. > > There is nothing gating the call in ghes_proc() of ghes_print_estatus() > and now the EFI code handling that pretty printed things is missing we get > the horrible kernel logging for an unknown block instead. > > So I think we need some minimal code in cper.c to match the guids then not > log them (on basis we are arguing there is no need for new cper records). > Otherwise we are in for some messy kernel logs > > Something like: > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > {1}[Hardware Error]: event severity: recoverable > {1}[Hardware Error]: Error 0, type: recoverable > {1}[Hardware Error]: section type: unknown, fbcd0a77-c260-417f-85a9-088b1621eba6 > {1}[Hardware Error]: section length: 0x90 > {1}[Hardware Error]: 00000000: 00000090 00000007 00000000 0d938086 ................ > {1}[Hardware Error]: 00000010: 00100000 00000000 00040000 00000000 ................ > {1}[Hardware Error]: 00000020: 00000000 00000000 00000000 00000000 ................ > {1}[Hardware Error]: 00000030: 00000000 00000000 00000000 00000000 ................ > {1}[Hardware Error]: 00000040: 00000000 00000000 00000000 00000000 ................ > {1}[Hardware Error]: 00000050: 00000000 00000000 00000000 00000000 ................ > {1}[Hardware Error]: 00000060: 00000000 00000000 00000000 00000000 ................ > {1}[Hardware Error]: 00000070: 00000000 00000000 00000000 00000000 ................ > {1}[Hardware Error]: 00000080: 00000000 00000000 00000000 00000000 ................ > cxl_general_media: memdev=mem1 host=0000:10:00.0 serial=4 log=Informational : time=0 uuid=fbcd0a77-c260-417f-85a9-088b1621eba6 len=0 flags='' handle=0 related_handle=0 maint_op_class=0 : dpa=0 dpa_flags='' descriptor='' type='ECC Error' transaction_type='Unknown' channel=0 rank=0 device=0 comp_id=00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 validity_flags='' > > (I'm filling the record with 0s currently) Yeah, when I tested this, I thought its okay for the hexdump to be there in dmesg from EFI as the handling is done in trace events from GHES. If, we need to handle from EFI, then it would be a good reason to move the GUIDs out from GHES and place it in a common location for EFI/cper to share similar to protocol errors. Thanks, Smita >
Smita Koralahalli wrote: > On 1/8/2024 8:58 AM, Jonathan Cameron wrote: > > On Wed, 20 Dec 2023 16:17:27 -0800 > > Ira Weiny <ira.weiny@intel.com> wrote: > > > >> Series status/background > >> ======================== > >> > >> Smita has been a great help with this series. Thank you again! > >> > >> Smita's testing found that the GHES code ended up printing the events > >> twice. This version avoids the duplicate print by calling the callback > >> from the GHES code instead of the EFI code as suggested by Dan. > > > > I'm not sure this is working as intended. > > > > There is nothing gating the call in ghes_proc() of ghes_print_estatus() > > and now the EFI code handling that pretty printed things is missing we get > > the horrible kernel logging for an unknown block instead. > > > > So I think we need some minimal code in cper.c to match the guids then not > > log them (on basis we are arguing there is no need for new cper records). > > Otherwise we are in for some messy kernel logs > > > > Something like: > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > > {1}[Hardware Error]: event severity: recoverable > > {1}[Hardware Error]: Error 0, type: recoverable > > {1}[Hardware Error]: section type: unknown, fbcd0a77-c260-417f-85a9-088b1621eba6 > > {1}[Hardware Error]: section length: 0x90 > > {1}[Hardware Error]: 00000000: 00000090 00000007 00000000 0d938086 ................ > > {1}[Hardware Error]: 00000010: 00100000 00000000 00040000 00000000 ................ > > {1}[Hardware Error]: 00000020: 00000000 00000000 00000000 00000000 ................ > > {1}[Hardware Error]: 00000030: 00000000 00000000 00000000 00000000 ................ > > {1}[Hardware Error]: 00000040: 00000000 00000000 00000000 00000000 ................ > > {1}[Hardware Error]: 00000050: 00000000 00000000 00000000 00000000 ................ > > {1}[Hardware Error]: 00000060: 00000000 00000000 00000000 00000000 ................ > > {1}[Hardware Error]: 00000070: 00000000 00000000 00000000 00000000 ................ > > {1}[Hardware Error]: 00000080: 00000000 00000000 00000000 00000000 ................ > > cxl_general_media: memdev=mem1 host=0000:10:00.0 serial=4 log=Informational : time=0 uuid=fbcd0a77-c260-417f-85a9-088b1621eba6 len=0 flags='' handle=0 related_handle=0 maint_op_class=0 : dpa=0 dpa_flags='' descriptor='' type='ECC Error' transaction_type='Unknown' channel=0 rank=0 device=0 comp_id=00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 validity_flags='' > > > > (I'm filling the record with 0s currently) > > Yeah, when I tested this, I thought its okay for the hexdump to be there > in dmesg from EFI as the handling is done in trace events from GHES. > > If, we need to handle from EFI, then it would be a good reason to move > the GUIDs out from GHES and place it in a common location for EFI/cper > to share similar to protocol errors. Ah, yes, my expectation was more aligned with Jonathan's observation to do the processing in GHES code *and* skip the processing in the CPER code, something like: diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index 56a5d2ef9e0a..e13e5fa4df4b 100644 --- a/drivers/acpi/apei/ghes.c +++ b/drivers/acpi/apei/ghes.c @@ -666,30 +666,6 @@ static cxl_cper_callback cper_callback; /* CXL Event record UUIDs are formatted as GUIDs and reported in section type */ -/* - * General Media Event Record - * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43 - */ -#define CPER_SEC_CXL_GEN_MEDIA_GUID \ - GUID_INIT(0xfbcd0a77, 0xc260, 0x417f, \ - 0x85, 0xa9, 0x08, 0x8b, 0x16, 0x21, 0xeb, 0xa6) - -/* - * DRAM Event Record - * CXL rev 3.0 section 8.2.9.2.1.2; Table 8-44 - */ -#define CPER_SEC_CXL_DRAM_GUID \ - GUID_INIT(0x601dcbb3, 0x9c06, 0x4eab, \ - 0xb8, 0xaf, 0x4e, 0x9b, 0xfb, 0x5c, 0x96, 0x24) - -/* - * Memory Module Event Record - * CXL rev 3.0 section 8.2.9.2.1.3; Table 8-45 - */ -#define CPER_SEC_CXL_MEM_MODULE_GUID \ - GUID_INIT(0xfe927475, 0xdd59, 0x4339, \ - 0xa5, 0x86, 0x79, 0xba, 0xb1, 0x13, 0xb7, 0x74) - static void cxl_cper_post_event(enum cxl_event_type event_type, struct cxl_cper_event_rec *rec) { diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c index 35c37f667781..0a4eed470750 100644 --- a/drivers/firmware/efi/cper.c +++ b/drivers/firmware/efi/cper.c @@ -24,6 +24,7 @@ #include <linux/bcd.h> #include <acpi/ghes.h> #include <ras/ras_event.h> +#include <linux/cxl-event.h> #include "cper_cxl.h" /* @@ -607,6 +608,15 @@ cper_estatus_print_section(const char *pfx, struct acpi_hest_generic_data *gdata cper_print_prot_err(newpfx, prot_err); else goto err_section_too_small; + } else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) { + printk("%ssection_type: CXL General Media Error\n", newpfx); + /* see: cxl_cper_event_call() */ + } else if (guid_equal(sec_type, &CPER_SEC_CXL_DRAM_GUID)) { + printk("%ssection_type: CXL DRAM Error\n", newpfx); + /* see: cxl_cper_event_call() */ + } else if (guid_equal(sec_type, &CPER_SEC_CXL_MEM_MODULE_GUID)) { + printk("%ssection_type: CXL Memory Module Error\n", newpfx); + /* see: cxl_cper_event_call() */ } else { const void *err = acpi_hest_get_payload(gdata); diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h index 17eadee819b6..6d9a7df88d4a 100644 --- a/include/linux/cxl-event.h +++ b/include/linux/cxl-event.h @@ -1,12 +1,31 @@ /* SPDX-License-Identifier: GPL-2.0 */ #ifndef _LINUX_CXL_EVENT_H #define _LINUX_CXL_EVENT_H +#include <linux/uuid.h> /* - * CXL event records; CXL rev 3.0 - * - * Copyright(c) 2023 Intel Corporation. + * General Media Event Record + * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43 + */ +#define CPER_SEC_CXL_GEN_MEDIA_GUID \ + GUID_INIT(0xfbcd0a77, 0xc260, 0x417f, \ + 0x85, 0xa9, 0x08, 0x8b, 0x16, 0x21, 0xeb, 0xa6) + +/* + * DRAM Event Record + * CXL rev 3.0 section 8.2.9.2.1.2; Table 8-44 + */ +#define CPER_SEC_CXL_DRAM_GUID \ + GUID_INIT(0x601dcbb3, 0x9c06, 0x4eab, \ + 0xb8, 0xaf, 0x4e, 0x9b, 0xfb, 0x5c, 0x96, 0x24) + +/* + * Memory Module Event Record + * CXL rev 3.0 section 8.2.9.2.1.3; Table 8-45 */ +#define CPER_SEC_CXL_MEM_MODULE_GUID \ + GUID_INIT(0xfe927475, 0xdd59, 0x4339, \ + 0xa5, 0x86, 0x79, 0xba, 0xb1, 0x13, 0xb7, 0x74) struct cxl_event_record_hdr { u8 length;
Dan Williams wrote: > Smita Koralahalli wrote: > > On 1/8/2024 8:58 AM, Jonathan Cameron wrote: > > > On Wed, 20 Dec 2023 16:17:27 -0800 > > > Ira Weiny <ira.weiny@intel.com> wrote: > > > > > >> Series status/background > > >> ======================== > > >> > > >> Smita has been a great help with this series. Thank you again! > > >> > > >> Smita's testing found that the GHES code ended up printing the events > > >> twice. This version avoids the duplicate print by calling the callback > > >> from the GHES code instead of the EFI code as suggested by Dan. > > > > > > I'm not sure this is working as intended. > > > > > > There is nothing gating the call in ghes_proc() of ghes_print_estatus() > > > and now the EFI code handling that pretty printed things is missing we get > > > the horrible kernel logging for an unknown block instead. > > > > > > So I think we need some minimal code in cper.c to match the guids then not > > > log them (on basis we are arguing there is no need for new cper records). > > > Otherwise we are in for some messy kernel logs > > > > > > Something like: > > > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > > > {1}[Hardware Error]: event severity: recoverable > > > {1}[Hardware Error]: Error 0, type: recoverable > > > {1}[Hardware Error]: section type: unknown, fbcd0a77-c260-417f-85a9-088b1621eba6 > > > {1}[Hardware Error]: section length: 0x90 > > > {1}[Hardware Error]: 00000000: 00000090 00000007 00000000 0d938086 ................ > > > {1}[Hardware Error]: 00000010: 00100000 00000000 00040000 00000000 ................ > > > {1}[Hardware Error]: 00000020: 00000000 00000000 00000000 00000000 ................ > > > {1}[Hardware Error]: 00000030: 00000000 00000000 00000000 00000000 ................ > > > {1}[Hardware Error]: 00000040: 00000000 00000000 00000000 00000000 ................ > > > {1}[Hardware Error]: 00000050: 00000000 00000000 00000000 00000000 ................ > > > {1}[Hardware Error]: 00000060: 00000000 00000000 00000000 00000000 ................ > > > {1}[Hardware Error]: 00000070: 00000000 00000000 00000000 00000000 ................ > > > {1}[Hardware Error]: 00000080: 00000000 00000000 00000000 00000000 ................ > > > cxl_general_media: memdev=mem1 host=0000:10:00.0 serial=4 log=Informational : time=0 uuid=fbcd0a77-c260-417f-85a9-088b1621eba6 len=0 flags='' handle=0 related_handle=0 maint_op_class=0 : dpa=0 dpa_flags='' descriptor='' type='ECC Error' transaction_type='Unknown' channel=0 rank=0 device=0 comp_id=00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 validity_flags='' > > > > > > (I'm filling the record with 0s currently) > > > > Yeah, when I tested this, I thought its okay for the hexdump to be there > > in dmesg from EFI as the handling is done in trace events from GHES. > > > > If, we need to handle from EFI, then it would be a good reason to move > > the GUIDs out from GHES and place it in a common location for EFI/cper > > to share similar to protocol errors. > > Ah, yes, my expectation was more aligned with Jonathan's observation to > do the processing in GHES code *and* skip the processing in the CPER > code, something like: > Agreed this was intended I did not realize the above. > > diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c > index 35c37f667781..0a4eed470750 100644 > --- a/drivers/firmware/efi/cper.c > +++ b/drivers/firmware/efi/cper.c > @@ -24,6 +24,7 @@ > #include <linux/bcd.h> > #include <acpi/ghes.h> > #include <ras/ras_event.h> > +#include <linux/cxl-event.h> > #include "cper_cxl.h" > > /* > @@ -607,6 +608,15 @@ cper_estatus_print_section(const char *pfx, struct acpi_hest_generic_data *gdata > cper_print_prot_err(newpfx, prot_err); > else > goto err_section_too_small; > + } else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) { > + printk("%ssection_type: CXL General Media Error\n", newpfx); Do we want the printk's here? I did not realize that a generic event would be printed. So intention was nothing would be done on this path. Ira
Ira Weiny wrote: > Dan Williams wrote: > > Smita Koralahalli wrote: > > > On 1/8/2024 8:58 AM, Jonathan Cameron wrote: > > > > On Wed, 20 Dec 2023 16:17:27 -0800 > > > > Ira Weiny <ira.weiny@intel.com> wrote: > > > > > > > >> Series status/background > > > >> ======================== > > > >> > > > >> Smita has been a great help with this series. Thank you again! > > > >> > > > >> Smita's testing found that the GHES code ended up printing the events > > > >> twice. This version avoids the duplicate print by calling the callback > > > >> from the GHES code instead of the EFI code as suggested by Dan. > > > > > > > > I'm not sure this is working as intended. > > > > > > > > There is nothing gating the call in ghes_proc() of ghes_print_estatus() > > > > and now the EFI code handling that pretty printed things is missing we get > > > > the horrible kernel logging for an unknown block instead. > > > > > > > > So I think we need some minimal code in cper.c to match the guids then not > > > > log them (on basis we are arguing there is no need for new cper records). > > > > Otherwise we are in for some messy kernel logs > > > > > > > > Something like: > > > > > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > > > > {1}[Hardware Error]: event severity: recoverable > > > > {1}[Hardware Error]: Error 0, type: recoverable > > > > {1}[Hardware Error]: section type: unknown, fbcd0a77-c260-417f-85a9-088b1621eba6 > > > > {1}[Hardware Error]: section length: 0x90 > > > > {1}[Hardware Error]: 00000000: 00000090 00000007 00000000 0d938086 ................ > > > > {1}[Hardware Error]: 00000010: 00100000 00000000 00040000 00000000 ................ > > > > {1}[Hardware Error]: 00000020: 00000000 00000000 00000000 00000000 ................ > > > > {1}[Hardware Error]: 00000030: 00000000 00000000 00000000 00000000 ................ > > > > {1}[Hardware Error]: 00000040: 00000000 00000000 00000000 00000000 ................ > > > > {1}[Hardware Error]: 00000050: 00000000 00000000 00000000 00000000 ................ > > > > {1}[Hardware Error]: 00000060: 00000000 00000000 00000000 00000000 ................ > > > > {1}[Hardware Error]: 00000070: 00000000 00000000 00000000 00000000 ................ > > > > {1}[Hardware Error]: 00000080: 00000000 00000000 00000000 00000000 ................ > > > > cxl_general_media: memdev=mem1 host=0000:10:00.0 serial=4 log=Informational : time=0 uuid=fbcd0a77-c260-417f-85a9-088b1621eba6 len=0 flags='' handle=0 related_handle=0 maint_op_class=0 : dpa=0 dpa_flags='' descriptor='' type='ECC Error' transaction_type='Unknown' channel=0 rank=0 device=0 comp_id=00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 validity_flags='' > > > > > > > > (I'm filling the record with 0s currently) > > > > > > Yeah, when I tested this, I thought its okay for the hexdump to be there > > > in dmesg from EFI as the handling is done in trace events from GHES. > > > > > > If, we need to handle from EFI, then it would be a good reason to move > > > the GUIDs out from GHES and place it in a common location for EFI/cper > > > to share similar to protocol errors. > > > > Ah, yes, my expectation was more aligned with Jonathan's observation to > > do the processing in GHES code *and* skip the processing in the CPER > > code, something like: > > > > Agreed this was intended I did not realize the above. > > > > > diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c > > index 35c37f667781..0a4eed470750 100644 > > --- a/drivers/firmware/efi/cper.c > > +++ b/drivers/firmware/efi/cper.c > > @@ -24,6 +24,7 @@ > > #include <linux/bcd.h> > > #include <acpi/ghes.h> > > #include <ras/ras_event.h> > > +#include <linux/cxl-event.h> > > #include "cper_cxl.h" > > > > /* > > @@ -607,6 +608,15 @@ cper_estatus_print_section(const char *pfx, struct acpi_hest_generic_data *gdata > > cper_print_prot_err(newpfx, prot_err); > > else > > goto err_section_too_small; > > + } else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) { > > + printk("%ssection_type: CXL General Media Error\n", newpfx); > > Do we want the printk's here? I did not realize that a generic event > would be printed. So intention was nothing would be done on this path. I think we do otherwise the kernel will say {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 {1}[Hardware Error]: event severity: recoverable {1}[Hardware Error]: Error 0, type: recoverable ... ...leaving the user hanging vs: {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 {1}[Hardware Error]: event severity: recoverable {1}[Hardware Error]: Error 0, type: recoverable {1}[Hardware Error]: section type: General Media Error ...as an indicator to go follow up with rasdaemon or whatever else is doing the detailed monitoring of CXL events.
On Mon, 8 Jan 2024 18:59:16 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > Ira Weiny wrote: > > Dan Williams wrote: > > > Smita Koralahalli wrote: > > > > On 1/8/2024 8:58 AM, Jonathan Cameron wrote: > > > > > On Wed, 20 Dec 2023 16:17:27 -0800 > > > > > Ira Weiny <ira.weiny@intel.com> wrote: > > > > > > > > > >> Series status/background > > > > >> ======================== > > > > >> > > > > >> Smita has been a great help with this series. Thank you again! > > > > >> > > > > >> Smita's testing found that the GHES code ended up printing the events > > > > >> twice. This version avoids the duplicate print by calling the callback > > > > >> from the GHES code instead of the EFI code as suggested by Dan. > > > > > > > > > > I'm not sure this is working as intended. > > > > > > > > > > There is nothing gating the call in ghes_proc() of ghes_print_estatus() > > > > > and now the EFI code handling that pretty printed things is missing we get > > > > > the horrible kernel logging for an unknown block instead. > > > > > > > > > > So I think we need some minimal code in cper.c to match the guids then not > > > > > log them (on basis we are arguing there is no need for new cper records). > > > > > Otherwise we are in for some messy kernel logs > > > > > > > > > > Something like: > > > > > > > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > > > > > {1}[Hardware Error]: event severity: recoverable > > > > > {1}[Hardware Error]: Error 0, type: recoverable > > > > > {1}[Hardware Error]: section type: unknown, fbcd0a77-c260-417f-85a9-088b1621eba6 > > > > > {1}[Hardware Error]: section length: 0x90 > > > > > {1}[Hardware Error]: 00000000: 00000090 00000007 00000000 0d938086 ................ > > > > > {1}[Hardware Error]: 00000010: 00100000 00000000 00040000 00000000 ................ > > > > > {1}[Hardware Error]: 00000020: 00000000 00000000 00000000 00000000 ................ > > > > > {1}[Hardware Error]: 00000030: 00000000 00000000 00000000 00000000 ................ > > > > > {1}[Hardware Error]: 00000040: 00000000 00000000 00000000 00000000 ................ > > > > > {1}[Hardware Error]: 00000050: 00000000 00000000 00000000 00000000 ................ > > > > > {1}[Hardware Error]: 00000060: 00000000 00000000 00000000 00000000 ................ > > > > > {1}[Hardware Error]: 00000070: 00000000 00000000 00000000 00000000 ................ > > > > > {1}[Hardware Error]: 00000080: 00000000 00000000 00000000 00000000 ................ > > > > > cxl_general_media: memdev=mem1 host=0000:10:00.0 serial=4 log=Informational : time=0 uuid=fbcd0a77-c260-417f-85a9-088b1621eba6 len=0 flags='' handle=0 related_handle=0 maint_op_class=0 : dpa=0 dpa_flags='' descriptor='' type='ECC Error' transaction_type='Unknown' channel=0 rank=0 device=0 comp_id=00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 validity_flags='' > > > > > > > > > > (I'm filling the record with 0s currently) > > > > > > > > Yeah, when I tested this, I thought its okay for the hexdump to be there > > > > in dmesg from EFI as the handling is done in trace events from GHES. > > > > > > > > If, we need to handle from EFI, then it would be a good reason to move > > > > the GUIDs out from GHES and place it in a common location for EFI/cper > > > > to share similar to protocol errors. > > > > > > Ah, yes, my expectation was more aligned with Jonathan's observation to > > > do the processing in GHES code *and* skip the processing in the CPER > > > code, something like: > > > > > > > Agreed this was intended I did not realize the above. > > > > > > > > diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c > > > index 35c37f667781..0a4eed470750 100644 > > > --- a/drivers/firmware/efi/cper.c > > > +++ b/drivers/firmware/efi/cper.c > > > @@ -24,6 +24,7 @@ > > > #include <linux/bcd.h> > > > #include <acpi/ghes.h> > > > #include <ras/ras_event.h> > > > +#include <linux/cxl-event.h> > > > #include "cper_cxl.h" > > > > > > /* > > > @@ -607,6 +608,15 @@ cper_estatus_print_section(const char *pfx, struct acpi_hest_generic_data *gdata > > > cper_print_prot_err(newpfx, prot_err); > > > else > > > goto err_section_too_small; > > > + } else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) { > > > + printk("%ssection_type: CXL General Media Error\n", newpfx); > > > > Do we want the printk's here? I did not realize that a generic event > > would be printed. So intention was nothing would be done on this path. > > I think we do otherwise the kernel will say > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > {1}[Hardware Error]: event severity: recoverable > {1}[Hardware Error]: Error 0, type: recoverable > ... > > ...leaving the user hanging vs: > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > {1}[Hardware Error]: event severity: recoverable > {1}[Hardware Error]: Error 0, type: recoverable > {1}[Hardware Error]: section type: General Media Error > > ...as an indicator to go follow up with rasdaemon or whatever else is > doing the detailed monitoring of CXL events. Agreed. Maybe push it out to a static const table though. As the argument was that we shouldn't be spitting out big logs in this modern world, let's make it easy for people to add more entries. struct skip_me { guid_t guid; const char *name; }; static const struct skip_me skip_me = { { &CPER_SEC_CXL_GEN_MEDIA, "CXL General Media Error" }, etc. }; for (i = 0; i < ARRAY_SIZE(skip_me); i++) { if (guid_equal(sec_type, skip_me[i].guid)) { printk("%asection_type: %s\n", newpfx, skip_me[i].name); break; } or something like that in the final else.
Jonathan Cameron wrote: > On Mon, 8 Jan 2024 18:59:16 -0800 > Dan Williams <dan.j.williams@intel.com> wrote: > > > Ira Weiny wrote: > > > Dan Williams wrote: > > > > Smita Koralahalli wrote: > > > > > On 1/8/2024 8:58 AM, Jonathan Cameron wrote: > > > > > > On Wed, 20 Dec 2023 16:17:27 -0800 > > > > > > Ira Weiny <ira.weiny@intel.com> wrote: > > > > > > > > > > > >> Series status/background > > > > > >> ======================== > > > > > >> > > > > > >> Smita has been a great help with this series. Thank you again! > > > > > >> > > > > > >> Smita's testing found that the GHES code ended up printing the events > > > > > >> twice. This version avoids the duplicate print by calling the callback > > > > > >> from the GHES code instead of the EFI code as suggested by Dan. > > > > > > > > > > > > I'm not sure this is working as intended. > > > > > > > > > > > > There is nothing gating the call in ghes_proc() of ghes_print_estatus() > > > > > > and now the EFI code handling that pretty printed things is missing we get > > > > > > the horrible kernel logging for an unknown block instead. > > > > > > > > > > > > So I think we need some minimal code in cper.c to match the guids then not > > > > > > log them (on basis we are arguing there is no need for new cper records). > > > > > > Otherwise we are in for some messy kernel logs > > > > > > > > > > > > Something like: > > > > > > > > > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > > > > > > {1}[Hardware Error]: event severity: recoverable > > > > > > {1}[Hardware Error]: Error 0, type: recoverable > > > > > > {1}[Hardware Error]: section type: unknown, fbcd0a77-c260-417f-85a9-088b1621eba6 > > > > > > {1}[Hardware Error]: section length: 0x90 > > > > > > {1}[Hardware Error]: 00000000: 00000090 00000007 00000000 0d938086 ................ > > > > > > {1}[Hardware Error]: 00000010: 00100000 00000000 00040000 00000000 ................ > > > > > > {1}[Hardware Error]: 00000020: 00000000 00000000 00000000 00000000 ................ > > > > > > {1}[Hardware Error]: 00000030: 00000000 00000000 00000000 00000000 ................ > > > > > > {1}[Hardware Error]: 00000040: 00000000 00000000 00000000 00000000 ................ > > > > > > {1}[Hardware Error]: 00000050: 00000000 00000000 00000000 00000000 ................ > > > > > > {1}[Hardware Error]: 00000060: 00000000 00000000 00000000 00000000 ................ > > > > > > {1}[Hardware Error]: 00000070: 00000000 00000000 00000000 00000000 ................ > > > > > > {1}[Hardware Error]: 00000080: 00000000 00000000 00000000 00000000 ................ > > > > > > cxl_general_media: memdev=mem1 host=0000:10:00.0 serial=4 log=Informational : time=0 uuid=fbcd0a77-c260-417f-85a9-088b1621eba6 len=0 flags='' handle=0 related_handle=0 maint_op_class=0 : dpa=0 dpa_flags='' descriptor='' type='ECC Error' transaction_type='Unknown' channel=0 rank=0 device=0 comp_id=00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 validity_flags='' > > > > > > > > > > > > (I'm filling the record with 0s currently) > > > > > > > > > > Yeah, when I tested this, I thought its okay for the hexdump to be there > > > > > in dmesg from EFI as the handling is done in trace events from GHES. > > > > > > > > > > If, we need to handle from EFI, then it would be a good reason to move > > > > > the GUIDs out from GHES and place it in a common location for EFI/cper > > > > > to share similar to protocol errors. > > > > > > > > Ah, yes, my expectation was more aligned with Jonathan's observation to > > > > do the processing in GHES code *and* skip the processing in the CPER > > > > code, something like: > > > > > > > > > > Agreed this was intended I did not realize the above. > > > > > > > > > > > diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c > > > > index 35c37f667781..0a4eed470750 100644 > > > > --- a/drivers/firmware/efi/cper.c > > > > +++ b/drivers/firmware/efi/cper.c > > > > @@ -24,6 +24,7 @@ > > > > #include <linux/bcd.h> > > > > #include <acpi/ghes.h> > > > > #include <ras/ras_event.h> > > > > +#include <linux/cxl-event.h> > > > > #include "cper_cxl.h" > > > > > > > > /* > > > > @@ -607,6 +608,15 @@ cper_estatus_print_section(const char *pfx, struct acpi_hest_generic_data *gdata > > > > cper_print_prot_err(newpfx, prot_err); > > > > else > > > > goto err_section_too_small; > > > > + } else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) { > > > > + printk("%ssection_type: CXL General Media Error\n", newpfx); > > > > > > Do we want the printk's here? I did not realize that a generic event > > > would be printed. So intention was nothing would be done on this path. > > > > I think we do otherwise the kernel will say > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > > {1}[Hardware Error]: event severity: recoverable > > {1}[Hardware Error]: Error 0, type: recoverable > > ... > > > > ...leaving the user hanging vs: > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > > {1}[Hardware Error]: event severity: recoverable > > {1}[Hardware Error]: Error 0, type: recoverable > > {1}[Hardware Error]: section type: General Media Error > > > > ...as an indicator to go follow up with rasdaemon or whatever else is > > doing the detailed monitoring of CXL events. > > Agreed. Maybe push it out to a static const table though. > As the argument was that we shouldn't be spitting out big logs in this > modern world, let's make it easy for people to add more entries. > > struct skip_me { > guid_t guid; > const char *name; > }; > static const struct skip_me skip_me = { > { &CPER_SEC_CXL_GEN_MEDIA, "CXL General Media Error" }, > etc. > }; > > for (i = 0; i < ARRAY_SIZE(skip_me); i++) { > if (guid_equal(sec_type, skip_me[i].guid)) { > printk("%asection_type: %s\n", newpfx, skip_me[i].name); > break; > } > > or something like that in the final else. I like it. Any concerns with that being an -rc fixup, and move ahead with the base enabling for v6.8? I don't see that follow-on as a reason to push the whole thing to v6.9.
Dan Williams wrote: > Jonathan Cameron wrote: > > On Mon, 8 Jan 2024 18:59:16 -0800 > > Dan Williams <dan.j.williams@intel.com> wrote: > > > > > Ira Weiny wrote: > > > > Dan Williams wrote: > > > > > Smita Koralahalli wrote: > > > > > > On 1/8/2024 8:58 AM, Jonathan Cameron wrote: > > > > > > > On Wed, 20 Dec 2023 16:17:27 -0800 > > > > > > > Ira Weiny <ira.weiny@intel.com> wrote: > > > > > > > > > > > > > >> Series status/background > > > > > > >> ======================== > > > > > > >> > > > > > > >> Smita has been a great help with this series. Thank you again! > > > > > > >> > > > > > > >> Smita's testing found that the GHES code ended up printing the events > > > > > > >> twice. This version avoids the duplicate print by calling the callback > > > > > > >> from the GHES code instead of the EFI code as suggested by Dan. > > > > > > > > > > > > > > I'm not sure this is working as intended. > > > > > > > > > > > > > > There is nothing gating the call in ghes_proc() of ghes_print_estatus() > > > > > > > and now the EFI code handling that pretty printed things is missing we get > > > > > > > the horrible kernel logging for an unknown block instead. > > > > > > > > > > > > > > So I think we need some minimal code in cper.c to match the guids then not > > > > > > > log them (on basis we are arguing there is no need for new cper records). > > > > > > > Otherwise we are in for some messy kernel logs > > > > > > > > > > > > > > Something like: > > > > > > > > > > > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > > > > > > > {1}[Hardware Error]: event severity: recoverable > > > > > > > {1}[Hardware Error]: Error 0, type: recoverable > > > > > > > {1}[Hardware Error]: section type: unknown, fbcd0a77-c260-417f-85a9-088b1621eba6 > > > > > > > {1}[Hardware Error]: section length: 0x90 > > > > > > > {1}[Hardware Error]: 00000000: 00000090 00000007 00000000 0d938086 ................ > > > > > > > {1}[Hardware Error]: 00000010: 00100000 00000000 00040000 00000000 ................ > > > > > > > {1}[Hardware Error]: 00000020: 00000000 00000000 00000000 00000000 ................ > > > > > > > {1}[Hardware Error]: 00000030: 00000000 00000000 00000000 00000000 ................ > > > > > > > {1}[Hardware Error]: 00000040: 00000000 00000000 00000000 00000000 ................ > > > > > > > {1}[Hardware Error]: 00000050: 00000000 00000000 00000000 00000000 ................ > > > > > > > {1}[Hardware Error]: 00000060: 00000000 00000000 00000000 00000000 ................ > > > > > > > {1}[Hardware Error]: 00000070: 00000000 00000000 00000000 00000000 ................ > > > > > > > {1}[Hardware Error]: 00000080: 00000000 00000000 00000000 00000000 ................ > > > > > > > cxl_general_media: memdev=mem1 host=0000:10:00.0 serial=4 log=Informational : time=0 uuid=fbcd0a77-c260-417f-85a9-088b1621eba6 len=0 flags='' handle=0 related_handle=0 maint_op_class=0 : dpa=0 dpa_flags='' descriptor='' type='ECC Error' transaction_type='Unknown' channel=0 rank=0 device=0 comp_id=00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 validity_flags='' > > > > > > > > > > > > > > (I'm filling the record with 0s currently) > > > > > > > > > > > > Yeah, when I tested this, I thought its okay for the hexdump to be there > > > > > > in dmesg from EFI as the handling is done in trace events from GHES. > > > > > > > > > > > > If, we need to handle from EFI, then it would be a good reason to move > > > > > > the GUIDs out from GHES and place it in a common location for EFI/cper > > > > > > to share similar to protocol errors. > > > > > > > > > > Ah, yes, my expectation was more aligned with Jonathan's observation to > > > > > do the processing in GHES code *and* skip the processing in the CPER > > > > > code, something like: > > > > > > > > > > > > > Agreed this was intended I did not realize the above. > > > > > > > > > > > > > > diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c > > > > > index 35c37f667781..0a4eed470750 100644 > > > > > --- a/drivers/firmware/efi/cper.c > > > > > +++ b/drivers/firmware/efi/cper.c > > > > > @@ -24,6 +24,7 @@ > > > > > #include <linux/bcd.h> > > > > > #include <acpi/ghes.h> > > > > > #include <ras/ras_event.h> > > > > > +#include <linux/cxl-event.h> > > > > > #include "cper_cxl.h" > > > > > > > > > > /* > > > > > @@ -607,6 +608,15 @@ cper_estatus_print_section(const char *pfx, struct acpi_hest_generic_data *gdata > > > > > cper_print_prot_err(newpfx, prot_err); > > > > > else > > > > > goto err_section_too_small; > > > > > + } else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) { > > > > > + printk("%ssection_type: CXL General Media Error\n", newpfx); > > > > > > > > Do we want the printk's here? I did not realize that a generic event > > > > would be printed. So intention was nothing would be done on this path. > > > > > > I think we do otherwise the kernel will say > > > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > > > {1}[Hardware Error]: event severity: recoverable > > > {1}[Hardware Error]: Error 0, type: recoverable > > > ... > > > > > > ...leaving the user hanging vs: > > > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > > > {1}[Hardware Error]: event severity: recoverable > > > {1}[Hardware Error]: Error 0, type: recoverable > > > {1}[Hardware Error]: section type: General Media Error > > > > > > ...as an indicator to go follow up with rasdaemon or whatever else is > > > doing the detailed monitoring of CXL events. > > > > Agreed. Maybe push it out to a static const table though. > > As the argument was that we shouldn't be spitting out big logs in this > > modern world, let's make it easy for people to add more entries. > > > > struct skip_me { > > guid_t guid; > > const char *name; > > }; > > static const struct skip_me skip_me = { > > { &CPER_SEC_CXL_GEN_MEDIA, "CXL General Media Error" }, > > etc. > > }; > > > > for (i = 0; i < ARRAY_SIZE(skip_me); i++) { > > if (guid_equal(sec_type, skip_me[i].guid)) { > > printk("%asection_type: %s\n", newpfx, skip_me[i].name); > > break; > > } > > > > or something like that in the final else. > > I like it. > > Any concerns with that being an -rc fixup, and move ahead with the base > enabling for v6.8? I don't see that follow-on as a reason to push the > whole thing to v6.9. I will put it in -next for soak time and make an inclusion decision in a few days after I hear back.
On Wed, 10 Jan 2024 at 00:30, Dan Williams <dan.j.williams@intel.com> wrote: > > Dan Williams wrote: > > Jonathan Cameron wrote: > > > On Mon, 8 Jan 2024 18:59:16 -0800 > > > Dan Williams <dan.j.williams@intel.com> wrote: > > > > > > > Ira Weiny wrote: > > > > > Dan Williams wrote: > > > > > > Smita Koralahalli wrote: > > > > > > > On 1/8/2024 8:58 AM, Jonathan Cameron wrote: > > > > > > > > On Wed, 20 Dec 2023 16:17:27 -0800 > > > > > > > > Ira Weiny <ira.weiny@intel.com> wrote: > > > > > > > > > > > > > > > >> Series status/background > > > > > > > >> ======================== > > > > > > > >> > > > > > > > >> Smita has been a great help with this series. Thank you again! > > > > > > > >> > > > > > > > >> Smita's testing found that the GHES code ended up printing the events > > > > > > > >> twice. This version avoids the duplicate print by calling the callback > > > > > > > >> from the GHES code instead of the EFI code as suggested by Dan. > > > > > > > > > > > > > > > > I'm not sure this is working as intended. > > > > > > > > > > > > > > > > There is nothing gating the call in ghes_proc() of ghes_print_estatus() > > > > > > > > and now the EFI code handling that pretty printed things is missing we get > > > > > > > > the horrible kernel logging for an unknown block instead. > > > > > > > > > > > > > > > > So I think we need some minimal code in cper.c to match the guids then not > > > > > > > > log them (on basis we are arguing there is no need for new cper records). > > > > > > > > Otherwise we are in for some messy kernel logs > > > > > > > > > > > > > > > > Something like: > > > > > > > > > > > > > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > > > > > > > > {1}[Hardware Error]: event severity: recoverable > > > > > > > > {1}[Hardware Error]: Error 0, type: recoverable > > > > > > > > {1}[Hardware Error]: section type: unknown, fbcd0a77-c260-417f-85a9-088b1621eba6 > > > > > > > > {1}[Hardware Error]: section length: 0x90 > > > > > > > > {1}[Hardware Error]: 00000000: 00000090 00000007 00000000 0d938086 ................ > > > > > > > > {1}[Hardware Error]: 00000010: 00100000 00000000 00040000 00000000 ................ > > > > > > > > {1}[Hardware Error]: 00000020: 00000000 00000000 00000000 00000000 ................ > > > > > > > > {1}[Hardware Error]: 00000030: 00000000 00000000 00000000 00000000 ................ > > > > > > > > {1}[Hardware Error]: 00000040: 00000000 00000000 00000000 00000000 ................ > > > > > > > > {1}[Hardware Error]: 00000050: 00000000 00000000 00000000 00000000 ................ > > > > > > > > {1}[Hardware Error]: 00000060: 00000000 00000000 00000000 00000000 ................ > > > > > > > > {1}[Hardware Error]: 00000070: 00000000 00000000 00000000 00000000 ................ > > > > > > > > {1}[Hardware Error]: 00000080: 00000000 00000000 00000000 00000000 ................ > > > > > > > > cxl_general_media: memdev=mem1 host=0000:10:00.0 serial=4 log=Informational : time=0 uuid=fbcd0a77-c260-417f-85a9-088b1621eba6 len=0 flags='' handle=0 related_handle=0 maint_op_class=0 : dpa=0 dpa_flags='' descriptor='' type='ECC Error' transaction_type='Unknown' channel=0 rank=0 device=0 comp_id=00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 validity_flags='' > > > > > > > > > > > > > > > > (I'm filling the record with 0s currently) > > > > > > > > > > > > > > Yeah, when I tested this, I thought its okay for the hexdump to be there > > > > > > > in dmesg from EFI as the handling is done in trace events from GHES. > > > > > > > > > > > > > > If, we need to handle from EFI, then it would be a good reason to move > > > > > > > the GUIDs out from GHES and place it in a common location for EFI/cper > > > > > > > to share similar to protocol errors. > > > > > > > > > > > > Ah, yes, my expectation was more aligned with Jonathan's observation to > > > > > > do the processing in GHES code *and* skip the processing in the CPER > > > > > > code, something like: > > > > > > > > > > > > > > > > Agreed this was intended I did not realize the above. > > > > > > > > > > > > > > > > > diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c > > > > > > index 35c37f667781..0a4eed470750 100644 > > > > > > --- a/drivers/firmware/efi/cper.c > > > > > > +++ b/drivers/firmware/efi/cper.c > > > > > > @@ -24,6 +24,7 @@ > > > > > > #include <linux/bcd.h> > > > > > > #include <acpi/ghes.h> > > > > > > #include <ras/ras_event.h> > > > > > > +#include <linux/cxl-event.h> > > > > > > #include "cper_cxl.h" > > > > > > > > > > > > /* > > > > > > @@ -607,6 +608,15 @@ cper_estatus_print_section(const char *pfx, struct acpi_hest_generic_data *gdata > > > > > > cper_print_prot_err(newpfx, prot_err); > > > > > > else > > > > > > goto err_section_too_small; > > > > > > + } else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) { > > > > > > + printk("%ssection_type: CXL General Media Error\n", newpfx); > > > > > > > > > > Do we want the printk's here? I did not realize that a generic event > > > > > would be printed. So intention was nothing would be done on this path. > > > > > > > > I think we do otherwise the kernel will say > > > > > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > > > > {1}[Hardware Error]: event severity: recoverable > > > > {1}[Hardware Error]: Error 0, type: recoverable > > > > ... > > > > > > > > ...leaving the user hanging vs: > > > > > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > > > > {1}[Hardware Error]: event severity: recoverable > > > > {1}[Hardware Error]: Error 0, type: recoverable > > > > {1}[Hardware Error]: section type: General Media Error > > > > > > > > ...as an indicator to go follow up with rasdaemon or whatever else is > > > > doing the detailed monitoring of CXL events. > > > > > > Agreed. Maybe push it out to a static const table though. > > > As the argument was that we shouldn't be spitting out big logs in this > > > modern world, let's make it easy for people to add more entries. > > > > > > struct skip_me { > > > guid_t guid; > > > const char *name; > > > }; > > > static const struct skip_me skip_me = { > > > { &CPER_SEC_CXL_GEN_MEDIA, "CXL General Media Error" }, > > > etc. > > > }; > > > > > > for (i = 0; i < ARRAY_SIZE(skip_me); i++) { > > > if (guid_equal(sec_type, skip_me[i].guid)) { > > > printk("%asection_type: %s\n", newpfx, skip_me[i].name); > > > break; > > > } > > > > > > or something like that in the final else. > > > > I like it. > > > > Any concerns with that being an -rc fixup, and move ahead with the base > > enabling for v6.8? I don't see that follow-on as a reason to push the > > whole thing to v6.9. > > I will put it in -next for soak time and make an inclusion decision in a > few days after I hear back. > For the series and however you want to handle the merge: Acked-by: Ard Biesheuvel <ardb@kernel.org>
On Wed, 10 Jan 2024 00:31:17 +0100 Ard Biesheuvel <ardb@kernel.org> wrote: > On Wed, 10 Jan 2024 at 00:30, Dan Williams <dan.j.williams@intel.com> wrote: > > > > Dan Williams wrote: > > > Jonathan Cameron wrote: > > > > On Mon, 8 Jan 2024 18:59:16 -0800 > > > > Dan Williams <dan.j.williams@intel.com> wrote: > > > > > > > > > Ira Weiny wrote: > > > > > > Dan Williams wrote: > > > > > > > Smita Koralahalli wrote: > > > > > > > > On 1/8/2024 8:58 AM, Jonathan Cameron wrote: > > > > > > > > > On Wed, 20 Dec 2023 16:17:27 -0800 > > > > > > > > > Ira Weiny <ira.weiny@intel.com> wrote: > > > > > > > > > > > > > > > > > >> Series status/background > > > > > > > > >> ======================== > > > > > > > > >> > > > > > > > > >> Smita has been a great help with this series. Thank you again! > > > > > > > > >> > > > > > > > > >> Smita's testing found that the GHES code ended up printing the events > > > > > > > > >> twice. This version avoids the duplicate print by calling the callback > > > > > > > > >> from the GHES code instead of the EFI code as suggested by Dan. > > > > > > > > > > > > > > > > > > I'm not sure this is working as intended. > > > > > > > > > > > > > > > > > > There is nothing gating the call in ghes_proc() of ghes_print_estatus() > > > > > > > > > and now the EFI code handling that pretty printed things is missing we get > > > > > > > > > the horrible kernel logging for an unknown block instead. > > > > > > > > > > > > > > > > > > So I think we need some minimal code in cper.c to match the guids then not > > > > > > > > > log them (on basis we are arguing there is no need for new cper records). > > > > > > > > > Otherwise we are in for some messy kernel logs > > > > > > > > > > > > > > > > > > Something like: > > > > > > > > > > > > > > > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > > > > > > > > > {1}[Hardware Error]: event severity: recoverable > > > > > > > > > {1}[Hardware Error]: Error 0, type: recoverable > > > > > > > > > {1}[Hardware Error]: section type: unknown, fbcd0a77-c260-417f-85a9-088b1621eba6 > > > > > > > > > {1}[Hardware Error]: section length: 0x90 > > > > > > > > > {1}[Hardware Error]: 00000000: 00000090 00000007 00000000 0d938086 ................ > > > > > > > > > {1}[Hardware Error]: 00000010: 00100000 00000000 00040000 00000000 ................ > > > > > > > > > {1}[Hardware Error]: 00000020: 00000000 00000000 00000000 00000000 ................ > > > > > > > > > {1}[Hardware Error]: 00000030: 00000000 00000000 00000000 00000000 ................ > > > > > > > > > {1}[Hardware Error]: 00000040: 00000000 00000000 00000000 00000000 ................ > > > > > > > > > {1}[Hardware Error]: 00000050: 00000000 00000000 00000000 00000000 ................ > > > > > > > > > {1}[Hardware Error]: 00000060: 00000000 00000000 00000000 00000000 ................ > > > > > > > > > {1}[Hardware Error]: 00000070: 00000000 00000000 00000000 00000000 ................ > > > > > > > > > {1}[Hardware Error]: 00000080: 00000000 00000000 00000000 00000000 ................ > > > > > > > > > cxl_general_media: memdev=mem1 host=0000:10:00.0 serial=4 log=Informational : time=0 uuid=fbcd0a77-c260-417f-85a9-088b1621eba6 len=0 flags='' handle=0 related_handle=0 maint_op_class=0 : dpa=0 dpa_flags='' descriptor='' type='ECC Error' transaction_type='Unknown' channel=0 rank=0 device=0 comp_id=00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 validity_flags='' > > > > > > > > > > > > > > > > > > (I'm filling the record with 0s currently) > > > > > > > > > > > > > > > > Yeah, when I tested this, I thought its okay for the hexdump to be there > > > > > > > > in dmesg from EFI as the handling is done in trace events from GHES. > > > > > > > > > > > > > > > > If, we need to handle from EFI, then it would be a good reason to move > > > > > > > > the GUIDs out from GHES and place it in a common location for EFI/cper > > > > > > > > to share similar to protocol errors. > > > > > > > > > > > > > > Ah, yes, my expectation was more aligned with Jonathan's observation to > > > > > > > do the processing in GHES code *and* skip the processing in the CPER > > > > > > > code, something like: > > > > > > > > > > > > > > > > > > > Agreed this was intended I did not realize the above. > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c > > > > > > > index 35c37f667781..0a4eed470750 100644 > > > > > > > --- a/drivers/firmware/efi/cper.c > > > > > > > +++ b/drivers/firmware/efi/cper.c > > > > > > > @@ -24,6 +24,7 @@ > > > > > > > #include <linux/bcd.h> > > > > > > > #include <acpi/ghes.h> > > > > > > > #include <ras/ras_event.h> > > > > > > > +#include <linux/cxl-event.h> > > > > > > > #include "cper_cxl.h" > > > > > > > > > > > > > > /* > > > > > > > @@ -607,6 +608,15 @@ cper_estatus_print_section(const char *pfx, struct acpi_hest_generic_data *gdata > > > > > > > cper_print_prot_err(newpfx, prot_err); > > > > > > > else > > > > > > > goto err_section_too_small; > > > > > > > + } else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) { > > > > > > > + printk("%ssection_type: CXL General Media Error\n", newpfx); > > > > > > > > > > > > Do we want the printk's here? I did not realize that a generic event > > > > > > would be printed. So intention was nothing would be done on this path. > > > > > > > > > > I think we do otherwise the kernel will say > > > > > > > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > > > > > {1}[Hardware Error]: event severity: recoverable > > > > > {1}[Hardware Error]: Error 0, type: recoverable > > > > > ... > > > > > > > > > > ...leaving the user hanging vs: > > > > > > > > > > {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 > > > > > {1}[Hardware Error]: event severity: recoverable > > > > > {1}[Hardware Error]: Error 0, type: recoverable > > > > > {1}[Hardware Error]: section type: General Media Error > > > > > > > > > > ...as an indicator to go follow up with rasdaemon or whatever else is > > > > > doing the detailed monitoring of CXL events. > > > > > > > > Agreed. Maybe push it out to a static const table though. > > > > As the argument was that we shouldn't be spitting out big logs in this > > > > modern world, let's make it easy for people to add more entries. > > > > > > > > struct skip_me { > > > > guid_t guid; > > > > const char *name; > > > > }; > > > > static const struct skip_me skip_me = { > > > > { &CPER_SEC_CXL_GEN_MEDIA, "CXL General Media Error" }, > > > > etc. > > > > }; > > > > > > > > for (i = 0; i < ARRAY_SIZE(skip_me); i++) { > > > > if (guid_equal(sec_type, skip_me[i].guid)) { > > > > printk("%asection_type: %s\n", newpfx, skip_me[i].name); > > > > break; > > > > } > > > > > > > > or something like that in the final else. > > > > > > I like it. > > > > > > Any concerns with that being an -rc fixup, and move ahead with the base > > > enabling for v6.8? I don't see that follow-on as a reason to push the > > > whole thing to v6.9. > > > > I will put it in -next for soak time and make an inclusion decision in a > > few days after I hear back. > > > > For the series and however you want to handle the merge: > > Acked-by: Ard Biesheuvel <ardb@kernel.org> Any path in works for me as well. J
Series status/background ======================== Smita has been a great help with this series. Thank you again! Smita's testing found that the GHES code ended up printing the events twice. This version avoids the duplicate print by calling the callback from the GHES code instead of the EFI code as suggested by Dan. Dependencies ============ NOTE this series still depends on Dan's addition of a device guard[1]. Therefore, the base commit is not a stable commit. I've pushed a branch with this commit included for testing if folks are interested.[2] [1] https://lore.kernel.org/all/170250854466.1522182.17555361077409628655.stgit@dwillia2-xfh.jf.intel.com/ [2] https://github.com/weiny2/linux-kernel/tree/cxl-cper-2023-12-20 Cover letter ============ CXL Component Events, as defined by EFI 2.10 Section N.2.14, wrap a mostly CXL event payload in an EFI Common Platform Error Record (CPER) record. If a device is configured for firmware first CXL event records are not sent directly to the host. The CXL sub-system uniquely has DPA to HPA translation information. It also already has event format tracing. Restructure the code to make sharing the data between CPER/event logs most efficient. Then send the CXL CPER records to the CXL sub-system for processing. With event logs the events interrupt the driver directly. In the EFI case events are wrapped with device information which allows the CXL subsystem to identify the PCI device. Previous version considered matching the memdev differently. However, the most robust was to find the PCI device via Bus, Device, Function and use the PCI device to find the driver data. CPER records are identified with GUID's while CXL event logs contain UUID's. The UUID is reported for all events no matter the source. While the UUID is redundant for the known events the UUID's are already used by rasdaemon. To keep compatibility UUIDs are still reported. In addition this series cleans up the UUID defines. Signed-off-by: Ira Weiny <ira.weiny@intel.com> --- Changes in v5: - Smita/djbw: trigger trace from ghes_do_proc() - Jonathan: split out pci scoped based functions to it's own patch - Jonathan: remove unneeded static uuid variables - Smita/djbw: trace an unknown event type as a generic with null UUID - Jonathan: code clean ups - Link to v4: https://lore.kernel.org/r/20231215-cxl-cper-v4-0-01b6dab44fcd@intel.com --- Ira Weiny (9): cxl/trace: Pass uuid explicitly to event traces cxl/events: Promote CXL event structures to a core header cxl/events: Create common event UUID defines cxl/events: Remove passing a UUID to known event traces cxl/events: Separate UUID from event structures cxl/events: Create a CXL event union acpi/ghes: Process CXL Component Events PCI: Define scoped based management functions cxl/pci: Register for and process CPER events drivers/acpi/apei/ghes.c | 88 +++++++++++++++++++++++ drivers/cxl/core/mbox.c | 87 +++++++++++------------ drivers/cxl/core/trace.h | 14 ++-- drivers/cxl/cxlmem.h | 110 +++++++---------------------- drivers/cxl/pci.c | 58 ++++++++++++++- include/linux/cxl-event.h | 162 ++++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 2 + tools/testing/cxl/test/mem.c | 163 ++++++++++++++++++++++++------------------- 8 files changed, 476 insertions(+), 208 deletions(-) --- base-commit: 6436863dfabce0d7ac416c8dc661fd513b967d39 change-id: 20230601-cxl-cper-26ffc839c6c6 Best regards,