Re: [PATCH v4 1/2] cxl/core: introduce device reporting poison hanlding

[Shiyang Ruan <ruansy.fnst@fujitsu.com>]

在 2024/8/9 2:28, Fan Ni 写道:
> On Thu, Aug 08, 2024 at 11:13:27PM +0800, Shiyang Ruan wrote:
>> CXL device can find&report memory problems, even before MCE is detected
>> by CPU.  AFAIK, the current kernel only traces POISON error event
>> from FW-First/OS-First path, but it doesn't handle them, neither
>> notify processes who are using the POISON page like MCE does.
>>
>> Thus, user have to read logs from trace and find out which device
>> reported the error and which applications are affected.  That is not
>> an easy work and cannot be handled in time.  Thus, it is needed to add
>> the feature to make the work done automatically and quickly.  Once CXL
>> device reports the POISON error (via FW-First/OS-First), kernel
>> handles it immediately, similar to the flow when a MCE is triggered.
>>
>> The current call trace of error reporting&handling looks like this:
>> ```
>> 1.  MCE (interrupt #18, while CPU consuming POISON)
>>       -> do_machine_check()
>>         -> mce_log()
>>           -> notify chain (x86_mce_decoder_chain)
>>             -> memory_failure()
>>
>> 2.a FW-First (optional, CXL device proactively find&report)
>>       -> CXL device -> Firmware
>>         -> OS: ACPI->APEI->GHES->CPER -> CXL driver -> trace
>>                                                    \-> memory_failure()
>>                                                        ^----- ADD
>> 2.b OS-First (optional, CXL device proactively find&report)
>>       -> CXL device -> MSI
>>         -> OS: CXL driver -> trace
>>                          \-> memory_failure()
>>                              ^------------------------------- ADD
>> ```
>> This patch adds calling memory_failure() while CXL device reporting
>> error is received, marked as "ADD" in figure above.
>>
>> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
>> ---
>>   drivers/cxl/core/mbox.c   | 75 ++++++++++++++++++++++++++++++++-------
>>   drivers/cxl/cxlmem.h      |  8 ++---
>>   drivers/cxl/pci.c         |  4 +--
>>   include/linux/cxl-event.h | 16 ++++++++-
>>   4 files changed, 83 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
>> index e5cdeafdf76e..0cb6ef2e6600 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -849,10 +849,55 @@ int cxl_enumerate_cmds(struct cxl_memdev_state *mds)
>>   }
>>   EXPORT_SYMBOL_NS_GPL(cxl_enumerate_cmds, CXL);
>>   
>> -void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
>> -			    enum cxl_event_log_type type,
>> -			    enum cxl_event_type event_type,
>> -			    const uuid_t *uuid, union cxl_event *evt)
>> +static void cxl_report_poison(struct cxl_memdev *cxlmd, u64 hpa)
>> +{
>> +	unsigned long pfn = PHYS_PFN(hpa);
>> +
>> +	memory_failure_queue(pfn, 0);
>> +}
>> +
>> +static void cxl_event_handle_general_media(struct cxl_memdev *cxlmd,
>> +					   enum cxl_event_log_type type,
>> +					   u64 hpa,
>> +					   struct cxl_event_gen_media *rec)
>> +{
>> +	if (type == CXL_EVENT_TYPE_FAIL) {
>> +		switch (rec->media_hdr.transaction_type) {
>> +		case CXL_EVENT_TRANSACTION_READ:
>> +		case CXL_EVENT_TRANSACTION_WRITE:
>> +		case CXL_EVENT_TRANSACTION_SCAN_MEDIA:
>> +		case CXL_EVENT_TRANSACTION_INJECT_POISON:
>> +			cxl_report_poison(cxlmd, hpa);
>> +			break;
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +}
>> +
>> +static void cxl_event_handle_dram(struct cxl_memdev *cxlmd,
>> +				  enum cxl_event_log_type type,
>> +				  u64 hpa,
>> +				  struct cxl_event_dram *rec)
>> +{
>> +	if (type == CXL_EVENT_TYPE_FAIL) {
>> +		switch (rec->media_hdr.transaction_type) {
>> +		case CXL_EVENT_TRANSACTION_READ:
>> +		case CXL_EVENT_TRANSACTION_WRITE:
>> +		case CXL_EVENT_TRANSACTION_SCAN_MEDIA:
>> +		case CXL_EVENT_TRANSACTION_INJECT_POISON:
>> +			cxl_report_poison(cxlmd, hpa);
>> +			break;
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +}
>> +
>> +void cxl_event_handle_record(struct cxl_memdev *cxlmd,
>> +			     enum cxl_event_log_type type,
>> +			     enum cxl_event_type event_type,
>> +			     const uuid_t *uuid, union cxl_event *evt)
>>   {
>>   	if (event_type == CXL_CPER_EVENT_MEM_MODULE) {
>>   		trace_cxl_memory_module(cxlmd, type, &evt->mem_module);
>> @@ -880,18 +925,22 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
>>   		if (cxlr)
>>   			hpa = cxl_dpa_to_hpa(cxlr, cxlmd, dpa);
>>   
>> -		if (event_type == CXL_CPER_EVENT_GEN_MEDIA)
>> +		if (event_type == CXL_CPER_EVENT_GEN_MEDIA) {
>>   			trace_cxl_general_media(cxlmd, type, cxlr, hpa,
>>   						&evt->gen_media);
>> -		else if (event_type == CXL_CPER_EVENT_DRAM)
>> +			cxl_event_handle_general_media(cxlmd, type, hpa,
>> +						&evt->gen_media);
>> +		} else if (event_type == CXL_CPER_EVENT_DRAM) {
>>   			trace_cxl_dram(cxlmd, type, cxlr, hpa, &evt->dram);
>> +			cxl_event_handle_dram(cxlmd, type, hpa, &evt->dram);
> 
> Does it make sense to call the trace function in
> cxl_event_handle_dram/general_media and replace the trace function with
> the handle_* here?

Sorry for late reply.  I'm not really good at naming functions.  Since 
the trace functions already have the framework to deal with each kind of 
uuids and event types, I don't think we should make another one for the 
same logics.  Thus, I reused it and renamed the functions.  Maybe 
"handle" isn't a good word to describe "tracing records and doing 
memory_failure if necessary".  Could you help me to name it better?

> 
>> +		}
>>   	}
>>   }
>> -EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL);
>> +EXPORT_SYMBOL_NS_GPL(cxl_event_handle_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)
>> +static void __cxl_event_handle_record(struct cxl_memdev *cxlmd,
>> +				      enum cxl_event_log_type type,
>> +				      struct cxl_event_record_raw *record)
>>   {
>>   	enum cxl_event_type ev_type = CXL_CPER_EVENT_GENERIC;
>>   	const uuid_t *uuid = &record->id;
>> @@ -903,7 +952,7 @@ static void __cxl_event_trace_record(const struct cxl_memdev *cxlmd,
>>   	else if (uuid_equal(uuid, &CXL_EVENT_MEM_MODULE_UUID))
>>   		ev_type = CXL_CPER_EVENT_MEM_MODULE;
>>   
>> -	cxl_event_trace_record(cxlmd, type, ev_type, uuid, &record->event);
>> +	cxl_event_handle_record(cxlmd, type, ev_type, uuid, &record->event);
>>   }
>>   
>>   static int cxl_clear_event_record(struct cxl_memdev_state *mds,
>> @@ -1012,8 +1061,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_handle_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 afb53d058d62..5c4810dcbdeb 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -826,10 +826,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,
>> -			    const uuid_t *uuid, union cxl_event *evt);
>> +void cxl_event_handle_record(struct cxl_memdev *cxlmd,
>> +			     enum cxl_event_log_type type,
>> +			     enum cxl_event_type event_type,
>> +			     const uuid_t *uuid, union cxl_event *evt);
>>   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 4be35dc22202..6e65ca89f666 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -1029,8 +1029,8 @@ static void cxl_handle_cper_event(enum cxl_event_type ev_type,
>>   	hdr_flags = get_unaligned_le24(rec->event.generic.hdr.flags);
>>   	log_type = FIELD_GET(CXL_EVENT_HDR_FLAGS_REC_SEVERITY, hdr_flags);
>>   
>> -	cxl_event_trace_record(cxlds->cxlmd, log_type, ev_type,
>> -			       &uuid_null, &rec->event);
>> +	cxl_event_handle_record(cxlds->cxlmd, log_type, ev_type,
>> +				&uuid_null, &rec->event);
>>   }
>>   
>>   static void cxl_cper_work_fn(struct work_struct *work)
>> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
>> index 0bea1afbd747..be4342a2b597 100644
>> --- a/include/linux/cxl-event.h
>> +++ b/include/linux/cxl-event.h
>> @@ -7,6 +7,20 @@
>>   #include <linux/uuid.h>
>>   #include <linux/workqueue_types.h>
>>   
>> +/*
>> + * Event transaction type
>> + * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
> 
> Here and below, update the specification reference to reflect cxl 3.1.

Ok. Will update it.


--
Thanks,
Ruan.

> 
> Fan
>> + */
>> +enum cxl_event_transaction_type {
>> +	CXL_EVENT_TRANSACTION_UNKNOWN = 0X00,
>> +	CXL_EVENT_TRANSACTION_READ,
>> +	CXL_EVENT_TRANSACTION_WRITE,
>> +	CXL_EVENT_TRANSACTION_SCAN_MEDIA,
>> +	CXL_EVENT_TRANSACTION_INJECT_POISON,
>> +	CXL_EVENT_TRANSACTION_MEDIA_SCRUB,
>> +	CXL_EVENT_TRANSACTION_MEDIA_MANAGEMENT,
>> +};
>> +
>>   /*
>>    * Common Event Record Format
>>    * CXL rev 3.0 section 8.2.9.2.1; Table 8-42
>> @@ -26,7 +40,7 @@ struct cxl_event_media_hdr {
>>   	__le64 phys_addr;
>>   	u8 descriptor;
>>   	u8 type;
>> -	u8 transaction_type;
>> +	u8 transaction_type;	/* enum cxl_event_transaction_type */
>>   	/*
>>   	 * The meaning of Validity Flags from bit 2 is
>>   	 * different across DRAM and General Media records
>> -- 
>> 2.34.1
>>