Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Thu, 7 Dec 2023 18:04:22 -0800
From:   Dan Williams <dan.j.williams@intel.com>
To:     Ira Weiny <ira.weiny@intel.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Jonathan Cameron <jonathan.cameron@huawei.com>,
        Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>,
        Shiju Jose <shiju.jose@huawei.com>
CC:     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>,
        Ira Weiny <ira.weiny@intel.com>
Subject: Re: [PATCH 6/6] cxl/memdev: Register for and process CPER events
Message-ID: <657279a68c270_b991294e@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <20230601-cxl-cper-v1-0-d19f1ac18ab6@intel.com>
 <20230601-cxl-cper-v1-6-d19f1ac18ab6@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230601-cxl-cper-v1-6-d19f1ac18ab6@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?KCpxVnhqkiEtvDile6QqKkfONeVlpwijAvnGtfbGicDR1VaSsFCCLYZJCjCd?=
 =?us-ascii?Q?7monWtP6y+52acMAc3gumk/x2/wQiF+nJscKPVS+8SP6CWIUDA/UmN09aXgp?=
 =?us-ascii?Q?EC8XggewKrL/dgDa6+Uc95rM6Q/Reyn4a+cZWG77kl7QOUjSA/YLYlMlEz88?=
 =?us-ascii?Q?jkTdfC7SsdsSRw5t3/dEoku3c6XHZ9IfZACFSSVv6Ld05Il/5yuW0HQfeO4+?=
 =?us-ascii?Q?E39NiPevJAjC3vXiD2lgTfmcbsVCTPK9dM2Bsu8FP15mZbFHGaCiZwCrv3hB?=
 =?us-ascii?Q?NCwElAcD3U4b60xk++FSOGyxGSKrmBRKjabyDACZE7ZXVG8wzhBIOHz4qM/3?=
 =?us-ascii?Q?aO4HYEhUXh3t8v5JtcSI2kKq2WXsil+Q9Ih9GUit1X1X+F5UhBFaNocNSIpG?=
 =?us-ascii?Q?cPQCxCdmiWZiISnSNRNgPeIjBMSlm/srYPdP3SnZJWgEZGgVB7JpyLFQqfzE?=
 =?us-ascii?Q?GoNgMPItKne7kswvUSsiQIB+ZRWfO6eHwtdQ70F118WHM3jtvueTLyE2Mv5L?=
 =?us-ascii?Q?4pYW1wIqVFeRbpMvz/EFxq+PPltWggdUWb0hHQSMLTtJkhTfXXeFB/CFW5J3?=
 =?us-ascii?Q?bOJARZK0OxqXuylKq8c6fcl1ZkKrX8ZltWt2ULk/Yjg66XOdUz74JrPVQjXg?=
 =?us-ascii?Q?7OTh1n9ClHlxEA1OB0xzBAI08YtZ4lfPCu74jIE6Jpr9hUfi4lAqs8vbjzex?=
 =?us-ascii?Q?df85dIY9FaCL+9hsyXKpM51VCpUCbLBJcpGj9dPxkpYWxfOof7XxCnH6XA0+?=
 =?us-ascii?Q?xuEnTjoGbCtk0I3HUnnZwhIFFwg97PptQbYLEgFK8Drm0hMu7MAb4gifr3WB?=
 =?us-ascii?Q?thh33L8JojBPsS5tr9gX9S+z0HFKC2w+Duy1Qax2tMIlUz6u+PMaP561RSWz?=
 =?us-ascii?Q?kLSLS0q0i9FYi9ijJfKgPwhSiDZFVK/jcU/5rw2JD9QEKrdYSBc7gRlFOsti?=
 =?us-ascii?Q?bbzJO3xsSF1R4QxWQB4lT2CM6bzJoTlmsv3F+jeAhPZnr2xqL8vSfjwCSuxl?=
 =?us-ascii?Q?tpvrIyUeKNcAuIHYo+JppHtc33ZTzOjv3r6fbhcXCoLL3j67e+pajeaaVCP/?=
 =?us-ascii?Q?HSHaItLFj6N06iWQ4METUx4OPfhpTUgi0Ay9Ez0LXrMPKUzvvQfn3rZprkcx?=
 =?us-ascii?Q?cFNI1Y8/y1vY9MaT2ehJQVP6lKj6QIJPgkL+3HnDwmLFt+ZNo3GsdUJSX6Gf?=
 =?us-ascii?Q?FQwWwjAe1jA4huXOyd2n0eh2c1wQayrrZIKFlQQirIKKLaiLtX2G7rUvQKI0?=
 =?us-ascii?Q?7voukPPCMpMLrwhnWgr9F0JaOMkRsD6DqhJaeRXhIYDSF5mCaE6Na9WAnnlj?=
 =?us-ascii?Q?wcpEUGtcmrBbhYrWb5tm3JAdqZ7IJ9P0+gIvGdg4o8rf6XISvf2vvWaYVJuD?=
 =?us-ascii?Q?1sLKupOT2T/ZspIMRGSF1aK6EyXMTYA3C9AWQMbJH08hAfNSX6mbziSANpii?=
 =?us-ascii?Q?9+ItDM28067r0Q+DmqFYsUf8SvCSlxHmW5DxtiH2y6jW9ZeykbkDDUeCTqfR?=
 =?us-ascii?Q?K81yjsjpk2gnzk2D2lQhXCERqg3zIJOfTI/W3trJKDN6DJPO8T5Tgia+1QGF?=
 =?us-ascii?Q?UMMiZBASGBC0Y3E5eEFw30jPEB9zj5wXsy7oTC2/sE0nuawukSWAWxqn6s0Z?=
 =?us-ascii?Q?mg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 635f40d9-c36f-4c8c-1eaf-08dbf79200cf
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2023 02:04:25.5827
 (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: Cruw8udhok99kHWwg9NC2np02kan5UmYnuDOdzdvcazFkcnjzoOUX55swvDugkzS8V2zlbfLWGkpX/fhKFI/WnDqxbQK/HpSOxHiVsXOEow=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB6976
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Thu, 07 Dec 2023 18:04:53 -0800 (PST)

Ira Weiny wrote:
> If the firmware has configured CXL event support to be firmware first
> the OS can process those events through CPER records.  The CXL layer has
> unique DPA to HPA knowledge and standard event trace parsing in place.
> Matching memory devices to the CPER records can be done via Bus, Device,
> Function which is part of the CPER record header.
> 
> Detect firmware first, register a notifier callback for each memdev, and
> trace events when they match the proper device.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/cxl/core/mbox.c | 31 +++++++++++++++++++++-----
>  drivers/cxl/cxlmem.h    |  6 +++++
>  drivers/cxl/pci.c       | 58 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 89 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 5ccc3843b736..8a0d4f67540d 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -860,9 +860,30 @@ static const uuid_t mem_mod_event_uuid =
>  	UUID_INIT(0xfe927475, 0xdd59, 0x4339,
>  		  0xa5, 0x86, 0x79, 0xba, 0xb1, 0x13, 0xb7, 0x74);
>  
> -static void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> -				   enum cxl_event_log_type type,
> -				   struct cxl_event_record_raw *record)
> +void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> +			    enum cxl_event_log_type type,
> +			    enum cxl_event_type event_type,
> +			    union cxl_event *event)
> +{
> +	switch (event_type) {
> +	case CXL_CPER_EVENT_GEN_MEDIA:
> +		trace_cxl_general_media(cxlmd, type, &gen_media_event_uuid,
> +					&event->gen_media);
> +		break;
> +	case CXL_CPER_EVENT_DRAM:
> +		trace_cxl_dram(cxlmd, type, &dram_event_uuid, &event->dram);
> +		break;
> +	case CXL_CPER_EVENT_MEM_MODULE:
> +		trace_cxl_memory_module(cxlmd, type, &mem_mod_event_uuid,
> +					&event->mem_module);
> +		break;
> +	}
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL);
> +
> +static void __cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> +				     enum cxl_event_log_type type,
> +				     struct cxl_event_record_raw *record)
>  {
>  	union cxl_event *evt = &record->event;
>  	uuid_t *id = &record->id;
> @@ -985,8 +1006,8 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  			break;
>  
>  		for (i = 0; i < nr_rec; i++)
> -			cxl_event_trace_record(cxlmd, type,
> -					       &payload->records[i]);
> +			__cxl_event_trace_record(cxlmd, type,
> +						 &payload->records[i]);
>  
>  		if (payload->flags & CXL_GET_EVENT_FLAG_OVERFLOW)
>  			trace_cxl_overflow(cxlmd, type, payload);
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index f0e7ebb84f02..9cb0e3448780 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -481,6 +481,8 @@ struct cxl_memdev_state {
>  	struct cxl_security_state security;
>  	struct cxl_fw_state fw;
>  
> +	struct notifier_block cxl_cper_nb;
> +
>  	struct rcuwait mbox_wait;
>  	int (*mbox_send)(struct cxl_memdev_state *mds,
>  			 struct cxl_mbox_cmd *cmd);
> @@ -778,6 +780,10 @@ void set_exclusive_cxl_commands(struct cxl_memdev_state *mds,
>  void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
>  				  unsigned long *cmds);
>  void cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status);
> +void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> +			    enum cxl_event_log_type type,
> +			    enum cxl_event_type event_type,
> +			    union cxl_event *event);
>  int cxl_set_timestamp(struct cxl_memdev_state *mds);
>  int cxl_poison_state_init(struct cxl_memdev_state *mds);
>  int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 0155fb66b580..ec65c11baf17 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
> +#include <asm-generic/unaligned.h>
>  #include <linux/io-64-nonatomic-lo-hi.h>
>  #include <linux/moduleparam.h>
>  #include <linux/module.h>
> @@ -741,6 +742,59 @@ static bool cxl_event_int_is_fw(u8 setting)
>  	return mode == CXL_INT_FW;
>  }
>  
> +#define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0)
> +static int cxl_cper_event_call(struct notifier_block *nb, unsigned long action,
> +			       void *data)
> +{
> +	struct cxl_cper_notifier_data *nd = data;
> +	struct cper_cxl_event_devid *device_id = &nd->rec->hdr.device_id;
> +	enum cxl_event_log_type log_type;
> +	struct cxl_memdev_state *mds;
> +	struct cxl_dev_state *cxlds;
> +	struct pci_dev *pdev;
> +	unsigned int devfn;
> +	u32 hdr_flags;
> +
> +	mds = container_of(nb, struct cxl_memdev_state, cxl_cper_nb);
> +
> +	devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
> +	pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
> +					   device_id->bus_num, devfn);
> +	cxlds = pci_get_drvdata(pdev);
> +	if (cxlds != &mds->cxlds) {

Checks of drvdata are only valid under the device lock, or with the
assumption that this callback will never be called while pci_get_drvdata
would return NULL.

With that, the check of cxlds looks like another artifact of using a
blocking notifier chain for this callback. With an explicit single
callback it simply becomes safe to assume that it is being called back
before unregister_cxl_cper() has run. I.e. it is impossible to even
write this check in that case.

> +		pci_dev_put(pdev);
> +		return NOTIFY_DONE;
> +	}
> +
> +	/* Fabricate a log type */
> +	hdr_flags = get_unaligned_le24(nd->rec->event.generic.hdr.flags);
> +	log_type = FIELD_GET(CXL_EVENT_HDR_FLAGS_REC_SEVERITY, hdr_flags);
> +
> +	cxl_event_trace_record(mds->cxlds.cxlmd, log_type, nd->event_type,
> +			       &nd->rec->event);
> +	pci_dev_put(pdev);
> +	return NOTIFY_OK;
> +}
> +
> +static void cxl_unregister_cper_events(void *_mds)
> +{
> +	struct cxl_memdev_state *mds = _mds;
> +
> +	unregister_cxl_cper_notifier(&mds->cxl_cper_nb);
> +}
> +
> +static void register_cper_events(struct cxl_memdev_state *mds)
> +{
> +	mds->cxl_cper_nb.notifier_call = cxl_cper_event_call;
> +
> +	if (register_cxl_cper_notifier(&mds->cxl_cper_nb)) {
> +		dev_err(mds->cxlds.dev, "CPER registration failed\n");
> +		return;
> +	}
> +
> +	devm_add_action_or_reset(mds->cxlds.dev, cxl_unregister_cper_events, mds);

Longer term I am not sure cxl_pci should be doing this registration
directly to the CPER code vs some indirection in the core that the
generic type-3 and the type-2 cases can register for processing. That
can definitely wait until a Type-2 CXL.mem device driver arrives and
wants to get notified of CXL CPER events.

> +}
> +
>  static int cxl_event_config(struct pci_host_bridge *host_bridge,
>  			    struct cxl_memdev_state *mds)
>  {
> @@ -751,8 +805,10 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge,
>  	 * When BIOS maintains CXL error reporting control, it will process
>  	 * event records.  Only one agent can do so.
>  	 */
> -	if (!host_bridge->native_cxl_error)
> +	if (!host_bridge->native_cxl_error) {
> +		register_cper_events(mds);
>  		return 0;
> +	}
>  
>  	rc = cxl_mem_alloc_event_buf(mds);
>  	if (rc)
> 
> -- 
> 2.42.0
>