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

Dave Jiang
2025-01-13T17:52:15Z

On 1/12/25 8:15 PM, Li Ming wrote:
> 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().

Thanks. I'll add endpoint checking to the mce_handler and also cxl_port_get_spa_cache_alias().

>
>
> Besides, seems like missing cxl unregister mce notifier implmentation?

Seems so. I'll add a devm_add_action_or_reset() for unregister.

Thanks for reviewing!

>
>
> 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 *
>
>