Re: [PATCH v2 4/4] cxl: Add mce notifier to emit aliased address for extended linear cache

[Li Ming <ming.li@zohomail.com>]

On 1/10/2025 11:17 PM, Dave Jiang wrote:
> Below is a setup with extended linear cache configuration with an example
> layout of memory region shown below presented as a single memory region
> consists of 256G memory where there's 128G of DRAM and 128G of CXL memory.
> The kernel sees a region of total 256G of system memory.
>
>               128G DRAM                          128G CXL memory
> |-----------------------------------|-------------------------------------|
>
> Data resides in either DRAM or far memory (FM) with no replication. Hot
> data is swapped into DRAM by the hardware behind the scenes. When error is
> detected in one location, it is possible that error also resides in the
> aliased location. Therefore when a memory location that is flagged by MCE
> is part of the special region, the aliased memory location needs to be
> offlined as well.
>
> Add an mce notify callback to identify if the MCE address location is part
> of an extended linear cache region and handle accordingly.
>
> Added symbol export to set_mce_nospec() in x86 code in order to call
> set_mce_nospec() from the CXL MCE notify callback.
>
> Link: https://lore.kernel.org/linux-cxl/668333b17e4b2_5639294fd@dwillia2-xfh.jf.intel.com.notmuch/
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v2:
> - Update new EXPORT_SYMOBOL_() input. (Jonathan)
> - Remove unnecessary pointer cast. (Jonathan)
> - Introduce CONFIG_CXL_MCE to shield from missing CONFIG_MEMORY_FAILURE kbuilds.
> - Emit alias address0 (Jonathan)
> ---
>  arch/x86/mm/pat/set_memory.c |  1 +
>  drivers/cxl/Kconfig          |  4 +++
>  drivers/cxl/core/Makefile    |  1 +
>  drivers/cxl/core/mbox.c      |  3 ++
>  drivers/cxl/core/mce.c       | 53 ++++++++++++++++++++++++++++++++++++
>  drivers/cxl/core/mce.h       | 14 ++++++++++
>  drivers/cxl/core/region.c    | 25 +++++++++++++++++
>  drivers/cxl/cxl.h            |  6 ++++
>  drivers/cxl/cxlmem.h         |  2 ++
>  9 files changed, 109 insertions(+)
>  create mode 100644 drivers/cxl/core/mce.c
>  create mode 100644 drivers/cxl/core/mce.h
>
> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index 95bc50a8541c..a0df698f46a2 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -2083,6 +2083,7 @@ int set_mce_nospec(unsigned long pfn)
>  		pr_warn("Could not invalidate pfn=0x%lx from 1:1 map\n", pfn);
>  	return rc;
>  }
> +EXPORT_SYMBOL_GPL(set_mce_nospec);
>  
>  /* Restore full speculative operation to the pfn. */
>  int clear_mce_nospec(unsigned long pfn)
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 876469e23f7a..d1c91dacae56 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -146,4 +146,8 @@ config CXL_REGION_INVALIDATION_TEST
>  	  If unsure, or if this kernel is meant for production environments,
>  	  say N.
>  
> +config CXL_MCE
> +	def_bool y
> +	depends on X86_MCE && MEMORY_FAILURE
> +
>  endif
> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> index 1a0c9c6ca818..61c9332b3582 100644
> --- a/drivers/cxl/core/Makefile
> +++ b/drivers/cxl/core/Makefile
> @@ -17,3 +17,4 @@ cxl_core-y += cdat.o
>  cxl_core-y += acpi.o
>  cxl_core-$(CONFIG_TRACING) += trace.o
>  cxl_core-$(CONFIG_CXL_REGION) += region.o
> +cxl_core-$(CONFIG_CXL_MCE) += mce.o
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index d7999260f004..4e12f1c17909 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -11,6 +11,7 @@
>  
>  #include "core.h"
>  #include "trace.h"
> +#include "mce.h"
>  
>  static bool cxl_raw_allow_all;
>  
> @@ -1478,6 +1479,8 @@ struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev)
>  	mds->ram_perf.qos_class = CXL_QOS_CLASS_INVALID;
>  	mds->pmem_perf.qos_class = CXL_QOS_CLASS_INVALID;
>  
> +	cxl_register_mce_notifier(&mds->mce_notifier);
> +

Hi Dave,


cxl_memdev_state_create() is invoked in cxl_pci_probe(), at that moment, 'cxlmd->endpoint' is NULL, 'cxlmd->endpoint' will be updated until the endpoint added to hierarchy via devm_cxl_add_endpoint().

If a MCE happened after cxl_memdev_state_create() and before 'cxlmd->endpoint' updated, acessing NULL endpoint pointer could happen in the function cxl_port_get_spa_cache_alias().

So I think maybe you can delay to register mce notifier after devm_cxl_add_endpoint(), or check if cxlmd->endpoint is NULL in cxl_port_get_spa_cache_alias().


Besides, seems like missing cxl unregister mce notifier implmentation?


Ming

>  	return mds;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_memdev_state_create, "CXL");
> diff --git a/drivers/cxl/core/mce.c b/drivers/cxl/core/mce.c
> new file mode 100644
> index 000000000000..bd1c4f46a67f
> --- /dev/null
> +++ b/drivers/cxl/core/mce.c
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2024 Intel Corporation. All rights reserved. */
> +#include <linux/mm.h>
> +#include <linux/notifier.h>
> +#include <linux/set_memory.h>
> +#include <asm/mce.h>
> +#include <cxlmem.h>
> +#include "mce.h"
> +
> +static int cxl_handle_mce(struct notifier_block *nb, unsigned long val,
> +			  void *data)
> +{
> +	struct cxl_memdev_state *mds = container_of(nb, struct cxl_memdev_state,
> +						    mce_notifier);
> +	struct cxl_memdev *cxlmd = mds->cxlds.cxlmd;
> +	struct cxl_port *endpoint = cxlmd->endpoint;
> +	struct mce *mce = data;
> +	u64 spa, spa_alias;
> +	unsigned long pfn;
> +
> +	if (!mce || !mce_usable_address(mce))
> +		return NOTIFY_DONE;
> +
> +	spa = mce->addr & MCI_ADDR_PHYSADDR;
> +
> +	pfn = spa >> PAGE_SHIFT;
> +	if (!pfn_valid(pfn))
> +		return NOTIFY_DONE;
> +
> +	spa_alias = cxl_port_get_spa_cache_alias(endpoint, spa);
> +	if (!spa_alias)
> +		return NOTIFY_DONE;
> +
> +	pfn = spa_alias >> PAGE_SHIFT;
> +
> +	/*
> +	 * Take down the aliased memory page. The original memory page flagged
> +	 * by the MCE will be taken cared of by the standard MCE handler.
> +	 */
> +	dev_emerg(mds->cxlds.dev, "Offlining aliased SPA address0: %#llx\n",
> +		  spa_alias);
> +	if (!memory_failure(pfn, 0))
> +		set_mce_nospec(pfn);
> +
> +	return NOTIFY_OK;
> +}
> +
> +void cxl_register_mce_notifier(struct notifier_block *mce_notifier)
> +{
> +	mce_notifier->notifier_call = cxl_handle_mce;
> +	mce_notifier->priority = MCE_PRIO_UC;
> +	mce_register_decode_chain(mce_notifier);
> +}
> diff --git a/drivers/cxl/core/mce.h b/drivers/cxl/core/mce.h
> new file mode 100644
> index 000000000000..b3c63bddf14e
> --- /dev/null
> +++ b/drivers/cxl/core/mce.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright(c) 2024 Intel Corporation. All rights reserved. */
> +#ifndef _CXL_CORE_MCE_H_
> +#define _CXL_CORE_MCE_H_
> +
> +#include <linux/notifier.h>
> +
> +#ifdef CONFIG_CXL_MCE
> +void cxl_register_mce_notifier(struct notifier_block *mce_notifer);
> +#else
> +static inline void cxl_register_mce_notifier(struct notifier_block *mce_notifier) {}
> +#endif
> +
> +#endif
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 5d23bd26d9ba..f866ee2fefed 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3449,6 +3449,31 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_add_to_region, "CXL");
>  
> +u64 cxl_port_get_spa_cache_alias(struct cxl_port *endpoint, u64 spa)
> +{
> +	struct cxl_region_ref *iter;
> +	unsigned long index;
> +
> +	guard(rwsem_write)(&cxl_region_rwsem);
> +
> +	xa_for_each(&endpoint->regions, index, iter) {
> +		struct cxl_region_params *p = &iter->region->params;
> +
> +		if (p->res->start <= spa && spa <= p->res->end) {
> +			if (!p->cache_size)
> +				return 0;
> +
> +			if (spa > p->res->start + p->cache_size)
> +				return spa - p->cache_size;
> +
> +			return spa + p->cache_size;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_port_get_spa_cache_alias, "CXL");
> +
>  static int is_system_ram(struct resource *res, void *arg)
>  {
>  	struct cxl_region *cxlr = arg;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 6a1fb784f74a..cff98e803722 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -876,6 +876,7 @@ struct cxl_pmem_region *to_cxl_pmem_region(struct device *dev);
>  int cxl_add_to_region(struct cxl_port *root,
>  		      struct cxl_endpoint_decoder *cxled);
>  struct cxl_dax_region *to_cxl_dax_region(struct device *dev);
> +u64 cxl_port_get_spa_cache_alias(struct cxl_port *endpoint, u64 spa);
>  #else
>  static inline bool is_cxl_pmem_region(struct device *dev)
>  {
> @@ -894,6 +895,11 @@ static inline struct cxl_dax_region *to_cxl_dax_region(struct device *dev)
>  {
>  	return NULL;
>  }
> +static inline u64 cxl_port_get_spa_cache_alias(struct cxl_port *endpoint,
> +					       u64 spa)
> +{
> +	return 0;
> +}
>  #endif
>  
>  void cxl_endpoint_parse_cdat(struct cxl_port *port);
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 2a25d1957ddb..55752cbf408c 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -477,6 +477,7 @@ static inline struct cxl_dev_state *mbox_to_cxlds(struct cxl_mailbox *cxl_mbox)
>   * @poison: poison driver state info
>   * @security: security driver state info
>   * @fw: firmware upload / activation state
> + * @mce_notifier: MCE notifier
>   *
>   * See CXL 3.0 8.2.9.8.2 Capacity Configuration and Label Storage for
>   * details on capacity parameters.
> @@ -503,6 +504,7 @@ struct cxl_memdev_state {
>  	struct cxl_poison_state poison;
>  	struct cxl_security_state security;
>  	struct cxl_fw_state fw;
> +	struct notifier_block mce_notifier;
>  };
>  
>  static inline struct cxl_memdev_state *