Received-SPF: pass (google.com: domain of linux-kernel+bounces-20271-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 8 Jan 2024 18:08:13 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>, "Jonathan
 Cameron" <Jonathan.Cameron@huawei.com>, Ira Weiny <ira.weiny@intel.com>
CC: Dan Williams <dan.j.williams@intel.com>, Shiju Jose
	<shiju.jose@huawei.com>, Yazen Ghannam <yazen.ghannam@amd.com>, "Davidlohr
 Bueso" <dave@stgolabs.net>, Dave Jiang <dave.jiang@intel.com>, "Alison
 Schofield" <alison.schofield@intel.com>, Vishal Verma
	<vishal.l.verma@intel.com>, Ard Biesheuvel <ardb@kernel.org>,
	<linux-efi@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-cxl@vger.kernel.org>, "Rafael J. Wysocki" <rafael@kernel.org>, "Bjorn
 Helgaas" <bhelgaas@google.com>
Subject: Re: [PATCH v5 0/9] efi/cxl-cper: Report CPER CXL component events
 through trace events
Message-ID: <659caa8da651c_127da22947b@dwillia2-xfh.jf.intel.com.notmuch>
References: <20231220-cxl-cper-v5-0-1bb8a4ca2c7a@intel.com>
 <20240108165855.00002f5a@Huawei.com>
 <eefc5888-4610-8e39-61ed-2d84e9ebf255@amd.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <eefc5888-4610-8e39-61ed-2d84e9ebf255@amd.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?5arirFYx0epNs7yIF6NHEiSiCuOUNV0MI0LKkZmiKbT624DLp7VPvcM54gwp?=
 =?us-ascii?Q?tSCf5n0aqKRUKnPlk3iMcXimFbwAGYUe+3obCgfjeLKhjyy0ZKLmXGGaut4m?=
 =?us-ascii?Q?SRQ8tsTLb4B+4VU16u+OMDbUtj6XHMobsUb7O6ugqi1e87NE382Hn2IqEn/C?=
 =?us-ascii?Q?ZYhs/H+dS1Xl8+O+OJMwxKWhZnxDWWXaijbPyGHrZxYYI0/hCo9I5JKxx3Va?=
 =?us-ascii?Q?HPvYZxFf322fO0tIWUuNuvyts6HZLnQqDTYTxtL9iwixOMtA7Et1jfH34FOr?=
 =?us-ascii?Q?rBuzCkUzfBuTA54taQSzT69rYkgNBm20/JM8bKK3GrY8Zve3z1FZVvThQlAl?=
 =?us-ascii?Q?5fh0BUKqiK7C8p2TBHOJjJXwSq4i2lTTqK6yKaF9xeX3EpGeaKvbanxEuPzE?=
 =?us-ascii?Q?7Im5/QrOEDe7AgixqhSDShUPBXLavchJLZ57a0R1Vy2nkpmJlLu3SPwYBg39?=
 =?us-ascii?Q?JXg0WqHDinpxc/3wFkJ+ZIEC5VkDCHzpOQ+S0Azcivof5D+/BZqkWZ9JxdCv?=
 =?us-ascii?Q?x5fXMTFNT2+ITEp+M39m4p7XW1tcfEf/lfRBaM7scCMWKhojWHi/RzNtQLG4?=
 =?us-ascii?Q?pIz8j3epnNEJ//g3ArYYiOpAKXCQHvM06CglltWcxXU1mQu40pZsUlVn0H8A?=
 =?us-ascii?Q?DUoyklsvmkL+0cNToDsbbqNDtIW2n6EXJOtBnXAo+Za4SBpCZMhLgxVen0FX?=
 =?us-ascii?Q?JeEVj9plarUX6S5Gg9Y04hzNoWEVLmJ4Rk5eYIjfYNfvE5vvJm2DBLWf2A4+?=
 =?us-ascii?Q?QdY5GLSe4E+UT5UNGjLa/eJtEmg648O3yBXdmuNYdJvPEYTcctmShXY9rS+x?=
 =?us-ascii?Q?4lVGoemjv/a4oGPv0MUGx+IY2pZpbT9evGRciFUuO9q/K6XnI06Lal0CSBUj?=
 =?us-ascii?Q?yyzwvLHyNiVvA+20tHsDarRzqMCtYaqb9ZyJ8BGfcBLVFvDPpy85YVHf64Jn?=
 =?us-ascii?Q?El7qV8jhev1D7a/1ACRzMRmHqbMPmQOGzWxJV736vBQxlSglbIVLurRR/EN8?=
 =?us-ascii?Q?bS/QMbLh9Wxprl5jKRiwc8PkgFZIgwAnh4FbajenJVNY3GWF4ina241A7Lml?=
 =?us-ascii?Q?W0vin/IWqH3rwCUs7mFIa2UKtrMdi+uePAIdGugZhl9VaJ70KBX9HI/IJin7?=
 =?us-ascii?Q?gblgGSuru+gbHMZqmSTJHUGDVbCAJ3Ts5M0EbicavO0gXoKumL+q6MJMAVR7?=
 =?us-ascii?Q?WSHWorWf1w35aQngzEkGuyzCEnwxQx4AyCK7vSOoUr9sCFURpip+dyRL88mV?=
 =?us-ascii?Q?cWitKeWd3zgfBWeGamGm4P3ZxAy3RajGm7KFsnpmShineLWGp4LZXt+3QWWj?=
 =?us-ascii?Q?RQReUDz55tP5IwToddzukL29Zqi1xf7KbOpLPsR7leFu96iGwc6MlWgv7w2r?=
 =?us-ascii?Q?inv2U+stDCl6V0vpguy9tvsjyZf5UxPsoUFwueJaUwoDLUsALvh5QT/nA88u?=
 =?us-ascii?Q?52qrkMlKuKLrTAiz1amLSuS+VJfOllIhnATZFoucfmKy0sKDaio6Ngc86i+D?=
 =?us-ascii?Q?pRfLRMb2RdVU4YjI8kskSWFfkZCMHrWXOnTLTgjWK84j9IRxnBjtmdlNGIJ2?=
 =?us-ascii?Q?Xgqx1mfWdmQD3tkRaXLH4ZdkziQ2OxgW5FOuP2PffcijL63mJmbcZNqAXKQQ?=
 =?us-ascii?Q?0w=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3d3762a6-23db-453b-b2ec-08dc10b7d805
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Jan 2024 02:08:17.1158
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 8TOFrmqj+tgqcEM4BoyqA2dBkeMaOeZf2yoh7d9hi6yzJFPrBnYMS6nyNL7f0FdcdJVf7XDJDkyRxi4lKGsVX9Q85W+a+M6w+s+q0ddMvwc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB6289
X-OriginatorOrg: intel.com

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;