linux-kernel - Re: [RFC PATCH 4/4] cxl/events: Updates for CXL Memory Module Event Record

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241017134417.00005c6b@Huawei.com>
Date: Thu, 17 Oct 2024 13:44:17 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: <shiju.jose@...wei.com>
CC: <dave.jiang@...el.com>, <dan.j.williams@...el.com>,
	<alison.schofield@...el.com>, <vishal.l.verma@...el.com>,
	<ira.weiny@...el.com>, <dave@...olabs.net>, <linux-cxl@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linuxarm@...wei.com>,
	<tanxiaofei@...wei.com>, <prime.zeng@...ilicon.com>
Subject: Re: [RFC PATCH 4/4] cxl/events: Updates for CXL Memory Module Event
 Record

On Wed, 16 Oct 2024 17:33:49 +0100
<shiju.jose@...wei.com> wrote:

> From: Shiju Jose <shiju.jose@...wei.com>
> 
> CXL spec 3.1 section 8.2.9.2.1.3 Table 8-47, Memory Module Event Record
> has updated with following new fields and new info for Device Event Type
> and Device Health Information fields.
> 1. Validity Flags
> 2. Component Identifier
> 3. Device Event Sub-Type
> 
> Add updates for the above spec changes in the CXL events record and CXL
> Memory Module trace event implementations.
> 
> Signed-off-by: Shiju Jose <shiju.jose@...wei.com>

A few minor things inline, but with the event_sub_type naming feel
free to add
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>

>  
>  	CXL_EVT_TP_printk("event_type='%s' health_status='%s' media_status='%s' " \
>  		"as_life_used=%s as_dev_temp=%s as_cor_vol_err_cnt=%s " \
>  		"as_cor_per_err_cnt=%s life_used=%u device_temp=%d " \
> -		"dirty_shutdown_cnt=%u cor_vol_err_cnt=%u cor_per_err_cnt=%u",
> +		"dirty_shutdown_cnt=%u cor_vol_err_cnt=%u cor_per_err_cnt=%u " \
> +		"validity_flags='%s' comp_id=%s sub_type='%s'",
>  		show_dev_evt_type(__entry->event_type),
>  		show_health_status_flags(__entry->health_status),
>  		show_media_status(__entry->media_status),
> @@ -750,7 +782,11 @@ TRACE_EVENT(cxl_memory_module,
>  		show_one_bit_status(CXL_DHI_AS_COR_PER_ERR_CNT(__entry->add_status)),
>  		__entry->life_used, __entry->device_temp,
>  		__entry->dirty_shutdown_cnt, __entry->cor_vol_err_cnt,
> -		__entry->cor_per_err_cnt
> +		__entry->cor_per_err_cnt,
> +		show_mem_module_valid_flags(__entry->validity_flags),
> +		cxl_print_component_id(__entry->validity_flags, CXL_MMER_VALID_COMPONENT,
> +				       CXL_MMER_VALID_COMPONENT_ID_FORMAT, __entry->comp_id),
> +		show_dev_event_sub_type(__entry->sub_type)
If we are going to reorganize for the other patches, why not move this next to the event type
field?  There isn't a validity flag for this (0 means not specified)
so fine to move it earlier I think.
>  	)
>  );
>  
> diff --git a/include/cxl/event.h b/include/cxl/event.h
> index 7e98492c85df..18b7f96dea77 100644
> --- a/include/cxl/event.h
> +++ b/include/cxl/event.h
> @@ -102,7 +102,10 @@ struct cxl_event_mem_module {
>  	struct cxl_event_record_hdr hdr;
>  	u8 event_type;
>  	struct cxl_get_health_info info;
> -	u8 reserved[0x3d];
> +	u8 validity_flags[2];
> +	u8 component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
> +	u8 sub_type;
maybe event_sub_type to match spec naming?

> +	u8 reserved[0x2a];
>  } __packed;
>  
>  union cxl_event {