Date:   Mon, 13 Mar 2023 15:47:35 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     <alison.schofield@intel.com>,
        Dan Williams <dan.j.williams@intel.com>,
        "Ira Weiny" <ira.weiny@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        "Dave Jiang" <dave.jiang@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        "Steven Rostedt" <rostedt@goodmis.org>
CC:     Alison Schofield <alison.schofield@intel.com>,
        <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>
Subject: Re: [PATCH v8 2/6] cxl/trace: Add TRACE support for CXL media-error
 records
Message-ID: <640fa806f2321_22c07f2944f@iweiny-mobl.notmuch>
References: <cover.1678468593.git.alison.schofield@intel.com>
 <a77d5513c55177361749a27544409cb6a0c94dd5.1678468593.git.alison.schofield@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <a77d5513c55177361749a27544409cb6a0c94dd5.1678468593.git.alison.schofield@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?7rGQr3Jt64M435nIGj3KxwKX5XegBktkzlXdVOKnrOaeJj8MZZENoHmZoCee?=
 =?us-ascii?Q?8Wex+27YyI7tVcWtbJESpkLqxMm9jVxXXSrMnJ6MoG1RtDFOFT9h/9Eg+WQO?=
 =?us-ascii?Q?vUXg//3ne2bAWjYVqV5/TZOvAoEpCjU9tneZ+mElrmnOpaHLm0KuVxqbeRuV?=
 =?us-ascii?Q?v58UCOOLolvLZVcF5792TriykgSvZL1SL9myTjhnBh961cgXH9qLq1TUMUd+?=
 =?us-ascii?Q?5Q/N3WvxDsLukg4fpOGj9UVrWA1kYkm03gylVx5hQoiJK4qWW5p1FbUYFf0T?=
 =?us-ascii?Q?K57qgEUylg7wQNi4+DjqI/E0KQnhQfTsSZls96iTVVZl9mc9RBD62Fw7Vj5A?=
 =?us-ascii?Q?wsOeSrVJRpmd2vSVA81h+aGkknfZ8uDiyiAaaTUZqXntJ05pdeuIwHIH1UPm?=
 =?us-ascii?Q?Hsn7WnMp+pVJucRTfzDOArkQgUzARwPJOKLBjwYwgZLnEbEHs/SWpopxjFyT?=
 =?us-ascii?Q?af2KOhxb/WLBaN3jGqDQDerlR6sWRgnMtULaiLTmSv0r/o9lgI1SkPiigWtz?=
 =?us-ascii?Q?SYjRQTYvhobGaqt4aL+rGEQsZrczTqIZUbnZ9u9+wvsfhN9k61w4Jtrx6N3E?=
 =?us-ascii?Q?JbzrqKc9S9iKW0qedHTNxpJRLAieFBdXTafikVJsOxYb3ISszdQy0PCGoQGz?=
 =?us-ascii?Q?9X/5T0a/DlcMlHAUYLZIjBcc9oYvi+bNAvEY9+Uw3jwfC6FdtANoy+gX7BbN?=
 =?us-ascii?Q?fFI1JRDzPcH7Jc9fJshcvdUU5iokPR58+mMiPW0D3DaRSH8V6JpH2we2Jcvn?=
 =?us-ascii?Q?QQIe87Mj3WnL4yKMnlqyVm1HCGnINUU05wU6hgUKPOe21+uFEe22MHv4I74k?=
 =?us-ascii?Q?nF50gEf/9ysH4/sf3m2jiZ/r9QuC9AcDth7CAKI3B1zVheuVnvvm7BAc75ZQ?=
 =?us-ascii?Q?ADFYsNokfH8THckopWqPBOTSp5tmuVJl9o43Uc0FNySze+pAoqST9zTci2Kl?=
 =?us-ascii?Q?+9W/ONmiZUlnY7YNHJZoO/aEz1eRvDj3JCl0szyWwZxX1ICn0H/9J74FjzgR?=
 =?us-ascii?Q?Wz4gsAhZyFj80+BC6bFpgaltDU/HNHd7ttKMzfSMppzcWvbFLBqlVNEv+cHe?=
 =?us-ascii?Q?33B/XainzXzqRtfDPOPXvseY+g741AWjnQ3rVmRpkqVFaSMjfBKblgmpQUyR?=
 =?us-ascii?Q?R1XsYJTvjgoJ9JN49ExhiIxTPUSWGQQFDvjrNR8MNSnDGzAa2Ku1Yrbj4SMs?=
 =?us-ascii?Q?7SfWmRZjxlQyvWkD23mKa1+3EgpxasPNhKc2feAWQOdGdslUwwEKBKUbM7tR?=
 =?us-ascii?Q?RVZcj2iMsHqpm7T8EOBBM7GR2c2laUL7xjjOSan+vJtyY4aT769STU/OYZfP?=
 =?us-ascii?Q?7mwTOFLSoVVhwkypgPOa7Xk8w/ZwIFt4ooqQkhZ1SgSu18+AxVz+l7RoGgQM?=
 =?us-ascii?Q?7crzAyhxrF9+tb2To2ykiKQeyIETXRTCOsT4D1/b5GEg6FFsq+N1riX4iLC3?=
 =?us-ascii?Q?akqobuzornWHem/QUBtIrnwROuTbIk6N0uinEHWnRbTHKxBVj1zoVGvBJTlz?=
 =?us-ascii?Q?m7oDVJhQeK8ks8NKeVO1+KQqDrlwq9KcxeW4ExTHtRQDW7dVyqgclquduY8+?=
 =?us-ascii?Q?ITmpb3qtQC4BEN6nsJ3AxKSUUPo+8LS3VEMr3sXT?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8bfffc06-20a5-4753-7732-08db2414f249
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Mar 2023 22:47:38.7934
 (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: tmsibvbarmbcvIiY1HsAeYhtKHMR6qYjNc0WZQeEb4UkNXS1v4wtfiWEtJ1HE/Nh8DasfdnH6ZvNzN937utWoA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7074
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> CXL devices may support the retrieval of a device poison list.
> Add a new trace event that the CXL subsystem may use to log
> the media-error records returned in the poison list.
> 
> Log each media-error record as a trace event of type 'cxl_poison'.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/cxl/core/mbox.c  |  4 +-
>  drivers/cxl/core/trace.h | 84 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 87 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 0da0a30511f2..77fc811bdfed 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1147,7 +1147,9 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>  		if (rc)
>  			break;
>  
> -		/* TODO TRACE the media error records */
> +		for (int i = 0; i < le16_to_cpu(po->count); i++)
> +			trace_cxl_poison(cxlmd, cxlr, &po->record[i],
> +					 po->flags, po->overflow_t);
>  
>  		/* Protect against an uncleared _FLAG_MORE */
>  		nr_records = nr_records + le16_to_cpu(po->count);
> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index 9b8d3d997834..33a22d26e742 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h
> @@ -7,6 +7,7 @@
>  #define _CXL_EVENTS_H
>  
>  #include <linux/tracepoint.h>
> +#include <linux/pci.h>
>  #include <asm-generic/unaligned.h>
>  
>  #include <cxl.h>
> @@ -600,6 +601,89 @@ TRACE_EVENT(cxl_memory_module,
>  	)
>  );
>  
> +#define __show_poison_source(source)                          \
> +	__print_symbolic(source,                              \
> +		{ CXL_POISON_SOURCE_UNKNOWN,   "Unknown"  },  \
> +		{ CXL_POISON_SOURCE_EXTERNAL,  "External" },  \
> +		{ CXL_POISON_SOURCE_INTERNAL,  "Internal" },  \
> +		{ CXL_POISON_SOURCE_INJECTED,  "Injected" },  \
> +		{ CXL_POISON_SOURCE_VENDOR,    "Vendor"   })
> +
> +#define show_poison_source(source)			     \
> +	(((source > CXL_POISON_SOURCE_INJECTED) &&	     \
> +	 (source != CXL_POISON_SOURCE_VENDOR)) ? "Reserved"  \
> +	 : __show_poison_source(source))
> +
> +#define show_poison_flags(flags)                             \
> +	__print_flags(flags, "|",                            \
> +		{ CXL_POISON_FLAG_MORE,      "More"     },   \
> +		{ CXL_POISON_FLAG_OVERFLOW,  "Overflow"  },  \
> +		{ CXL_POISON_FLAG_SCANNING,  "Scanning"  })
> +
> +#define __cxl_poison_addr(record)					\
> +	(le64_to_cpu(record->address))
> +#define cxl_poison_record_dpa(record)					\
> +	(__cxl_poison_addr(record) & CXL_POISON_START_MASK)
> +#define cxl_poison_record_source(record)				\
> +	(__cxl_poison_addr(record)  & CXL_POISON_SOURCE_MASK)
> +#define cxl_poison_record_length(record)				\
> +	(le32_to_cpu(record->length) * CXL_POISON_LEN_MULT)
> +#define cxl_poison_overflow(flags, time)				\
> +	(flags & CXL_POISON_FLAG_OVERFLOW ? le64_to_cpu(time) : 0)
> +
> +TRACE_EVENT(cxl_poison,
> +
> +	TP_PROTO(struct cxl_memdev *cxlmd, struct cxl_region *region,
> +		 const struct cxl_poison_record *record,
> +		 u8 flags, __le64 overflow_t),

FWIW I made event overflow a separate trace event.

Will this make all of the poisons in a single GetPoison command marked
with overflow in the trace buffer?

Ira

> +
> +	TP_ARGS(cxlmd, region, record, flags, overflow_t),
> +
> +	TP_STRUCT__entry(
> +		__string(memdev, dev_name(&cxlmd->dev))
> +		__string(host, dev_name(cxlmd->dev.parent))
> +		__field(u64, serial)
> +		__string(region, region)
> +		__field(u64, overflow_t)
> +		__field(u64, dpa)
> +		__field(u32, length)
> +		__array(char, uuid, 16)
> +		__field(u8, source)
> +		__field(u8, flags)
> +	    ),
> +
> +	TP_fast_assign(
> +		__assign_str(memdev, dev_name(&cxlmd->dev));
> +		__assign_str(host, dev_name(cxlmd->dev.parent));
> +		__entry->serial = cxlmd->cxlds->serial;
> +		__entry->overflow_t = cxl_poison_overflow(flags, overflow_t);
> +		__entry->dpa = cxl_poison_record_dpa(record);
> +		__entry->length = cxl_poison_record_length(record);
> +		__entry->source = cxl_poison_record_source(record);
> +		__entry->flags = flags;
> +		if (region) {
> +			__assign_str(region, dev_name(&region->dev));
> +			memcpy(__entry->uuid, &region->params.uuid, 16);
> +		} else {
> +			__assign_str(region, "");
> +			memset(__entry->uuid, 0, 16);
> +		}
> +	    ),
> +
> +	TP_printk("memdev=%s host=%s serial=%lld region=%s region_uuid=%pU dpa=0x%llx length=0x%x source=%s flags=%s overflow_time=%llu",
> +		__get_str(memdev),
> +		__get_str(host),
> +		__entry->serial,
> +		__get_str(region),
> +		__entry->uuid,
> +		__entry->dpa,
> +		__entry->length,
> +		show_poison_source(__entry->source),
> +		show_poison_flags(__entry->flags),
> +		__entry->overflow_t
> +	)
> +);
> +
>  #endif /* _CXL_EVENTS_H */
>  
>  #define TRACE_INCLUDE_FILE trace
> -- 
> 2.37.3
>