Message-ID: <Y5YS3TfboLM5BA3M@iweiny-mobl>
Date:   Sun, 11 Dec 2022 09:26:53 -0800
From:   Ira Weiny <ira.weiny@...el.com>
To:     Dan Williams <dan.j.williams@...el.com>
CC:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Alison Schofield <alison.schofield@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        Davidlohr Bueso <dave@...olabs.net>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Dave Jiang <dave.jiang@...el.com>,
        <linux-kernel@...r.kernel.org>, <linux-pci@...r.kernel.org>,
        <linux-acpi@...r.kernel.org>, <linux-cxl@...r.kernel.org>
Subject: Re: [PATCH V3 6/8] cxl/test: Add generic mock events

On Fri, Dec 09, 2022 at 02:48:29PM -0800, Dan Williams wrote:
> ira.weiny@ wrote:
> > From: Ira Weiny <ira.weiny@...el.com>
> > 
> > Facilitate testing basic Get/Clear Event functionality by creating
> > multiple logs and generic events with made up UUID's.
> > 
> > Data is completely made up with data patterns which should be easy to
> > spot in trace output.
> > 
> > A single sysfs entry resets the event data and triggers collecting the
> > events for testing.
> > 
> > Test traces are easy to obtain with a small script such as this:
> > 
> > 	#!/bin/bash -x
> > 
> > 	devices=`find /sys/devices/platform -name cxl_mem*`
> > 
> > 	# Turn on tracing
> > 	echo "" > /sys/kernel/tracing/trace
> > 	echo 1 > /sys/kernel/tracing/events/cxl/enable
> > 	echo 1 > /sys/kernel/tracing/tracing_on
> > 
> > 	# Generate fake interrupt
> > 	for device in $devices; do
> > 	        echo 1 > $device/event_trigger
> > 	done
> > 
> > 	# Turn off tracing and report events
> > 	echo 0 > /sys/kernel/tracing/tracing_on
> > 	cat /sys/kernel/tracing/trace
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@...el.com>
> > 
> > ---
> > Changes from v2:
> > 	Adjust for tracing being part of cxl core
> > 	Dan/Dave J.
> > 		Adjust to Dave J.s mock data structure
> > 	Dan
> > 		Remove mock specific functionality in main code
> > 
> > Changes from v1:
> > 	Fix up for new structures
> > 	Jonathan
> > 		Update based on specification discussion
> > 
> > Changes from RFC v2:
> > 	Adjust to simulate the event status register
> > 
> > Changes from RFC:
> > 	Separate out the event code
> > 	Adjust for struct changes.
> > 	Clean up devm_cxl_mock_event_logs()
> > 	Clean up naming and comments
> > 	Jonathan
> > 		Remove dynamic allocation of event logs
> > 		Clean up comment
> > 		Remove unneeded xarray
> > 		Ensure event_trigger sysfs is valid prior to the driver
> > 		going active.
> > 	Dan
> > 		Remove the fill/reset event sysfs as these operations
> > 		can be done together
> > ---
> >  tools/testing/cxl/test/Kbuild   |   4 +-
> >  tools/testing/cxl/test/events.c | 195 ++++++++++++++++++++++++++++++++
> >  tools/testing/cxl/test/events.h |  32 ++++++
> >  tools/testing/cxl/test/mem.c    |  33 ++++--
> >  tools/testing/cxl/test/mock.h   |  12 ++
> >  5 files changed, 264 insertions(+), 12 deletions(-)
> >  create mode 100644 tools/testing/cxl/test/events.c
> >  create mode 100644 tools/testing/cxl/test/events.h
> > 
> > diff --git a/tools/testing/cxl/test/Kbuild b/tools/testing/cxl/test/Kbuild
> > index 4e59e2c911f6..c48d912e3781 100644
> > --- a/tools/testing/cxl/test/Kbuild
> > +++ b/tools/testing/cxl/test/Kbuild
> > @@ -1,5 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0
> > -ccflags-y := -I$(srctree)/drivers/cxl/
> > +ccflags-y := -I$(srctree)/drivers/cxl/ -I$(srctree)/drivers/cxl/core
> >  
> >  obj-m += cxl_test.o
> >  obj-m += cxl_mock.o
> > @@ -7,4 +7,4 @@ obj-m += cxl_mock_mem.o
> >  
> >  cxl_test-y := cxl.o
> >  cxl_mock-y := mock.o
> > -cxl_mock_mem-y := mem.o
> > +cxl_mock_mem-y := mem.o events.o
> > diff --git a/tools/testing/cxl/test/events.c b/tools/testing/cxl/test/events.c
> > new file mode 100644
> > index 000000000000..1346c38dce1d
> > --- /dev/null
> > +++ b/tools/testing/cxl/test/events.c
> > @@ -0,0 +1,195 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +// Copyright(c) 2022 Intel Corporation. All rights reserved.
> > +
> > +#include "mock.h"
> > +#include "events.h"
> > +#include "trace.h"
> > +
> > +struct mock_event_log *find_event_log(struct device *dev, int log_type)
> > +{
> > +	struct cxl_mockmem_data *mdata = dev_get_drvdata(dev);
> > +
> > +	if (log_type >= CXL_EVENT_TYPE_MAX)
> > +		return NULL;
> > +	return &mdata->mes.mock_logs[log_type];
> > +}
> > +
> > +struct cxl_event_record_raw *get_cur_event(struct mock_event_log *log)
> > +{
> > +	return log->events[log->cur_idx];
> > +}
> > +
> > +void reset_event_log(struct mock_event_log *log)
> > +{
> > +	log->cur_idx = 0;
> > +	log->clear_idx = 0;
> > +}
> > +
> > +/* Handle can never be 0 use 1 based indexing for handle */
> > +u16 get_clear_handle(struct mock_event_log *log)
> > +{
> > +	return log->clear_idx + 1;
> > +}
> > +
> > +/* Handle can never be 0 use 1 based indexing for handle */
> > +__le16 get_cur_event_handle(struct mock_event_log *log)
> > +{
> > +	u16 cur_handle = log->cur_idx + 1;
> > +
> > +	return cpu_to_le16(cur_handle);
> > +}
> > +
> > +static bool log_empty(struct mock_event_log *log)
> > +{
> > +	return log->cur_idx == log->nr_events;
> > +}
> > +
> > +static void event_store_add_event(struct mock_event_store *mes,
> > +				  enum cxl_event_log_type log_type,
> > +				  struct cxl_event_record_raw *event)
> > +{
> > +	struct mock_event_log *log;
> > +
> > +	if (WARN_ON(log_type >= CXL_EVENT_TYPE_MAX))
> > +		return;
> > +
> > +	log = &mes->mock_logs[log_type];
> > +	if (WARN_ON(log->nr_events >= CXL_TEST_EVENT_CNT_MAX))
> > +		return;
> > +
> > +	log->events[log->nr_events] = event;
> > +	log->nr_events++;
> > +}
> > +
> > +int mock_get_event(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
> > +{
> > +	struct cxl_get_event_payload *pl;
> > +	struct mock_event_log *log;
> > +	u8 log_type;
> > +	int i;
> > +
> > +	if (cmd->size_in != sizeof(log_type))
> > +		return -EINVAL;
> > +
> > +	if (cmd->size_out < struct_size(pl, records, CXL_TEST_EVENT_CNT))
> > +		return -EINVAL;
> > +
> > +	log_type = *((u8 *)cmd->payload_in);
> > +	if (log_type >= CXL_EVENT_TYPE_MAX)
> > +		return -EINVAL;
> > +
> > +	memset(cmd->payload_out, 0, cmd->size_out);
> > +
> > +	log = find_event_log(cxlds->dev, log_type);
> > +	if (!log || log_empty(log))
> > +		return 0;
> > +
> > +	pl = cmd->payload_out;
> > +
> > +	for (i = 0; i < CXL_TEST_EVENT_CNT && !log_empty(log); i++) {
> > +		memcpy(&pl->records[i], get_cur_event(log), sizeof(pl->records[i]));
> > +		pl->records[i].hdr.handle = get_cur_event_handle(log);
> > +		log->cur_idx++;
> > +	}
> > +
> > +	pl->record_count = cpu_to_le16(i);
> > +	if (!log_empty(log))
> > +		pl->flags |= CXL_GET_EVENT_FLAG_MORE_RECORDS;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mock_get_event);
> 
> I believe I asked this before on the last review. Why is this exported?
> The caller is within the same module.

I don't recall the comment, sorry.  I don't recall why I felt the need to add
the export.  This is now obviously not needed.

> 
> I also notice now that the event support code is even smaller than the
> security code that is already in that file, just go ahead and move this
> infrastructure in there as well. Then this does not need to move the
> cxl_mockmem_data definition around or create other new headers.
> 
> Other than that minor detail the implementation looks good. I appreciate
> the effort to get cxl_test to light up this interface!
> 
> After consolidating in mem.c you can add:

After all the changes the code has shrunk quite a bit.

Moved to mem.c.

> 
> Reviewed-by: Dan Williams <dan.j.williams@...el.com>

Thanks!
Ira

> 
> > +
> > +int mock_clear_event(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
> > +{
> > +	struct cxl_mbox_clear_event_payload *pl = cmd->payload_in;
> > +	struct mock_event_log *log;
> > +	u8 log_type = pl->event_log;
> > +	u16 handle;
> > +	int nr;
> > +
> > +	if (log_type >= CXL_EVENT_TYPE_MAX)
> > +		return -EINVAL;
> > +
> > +	log = find_event_log(cxlds->dev, log_type);
> > +	if (!log)
> > +		return 0; /* No mock data in this log */
> > +
> > +	/*
> > +	 * This check is technically not invalid per the specification AFAICS.
> > +	 * (The host could 'guess' handles and clear them in order).
> > +	 * However, this is not good behavior for the host so test it.
> > +	 */
> > +	if (log->clear_idx + pl->nr_recs > log->cur_idx) {
> > +		dev_err(cxlds->dev,
> > +			"Attempting to clear more events than returned!\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Check handle order prior to clearing events */
> > +	for (nr = 0, handle = get_clear_handle(log);
> > +	     nr < pl->nr_recs;
> > +	     nr++, handle++) {
> > +		if (handle != le16_to_cpu(pl->handle[nr])) {
> > +			dev_err(cxlds->dev, "Clearing events out of order\n");
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	/* Clear events */
> > +	log->clear_idx += pl->nr_recs;
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mock_clear_event);
> > +
> > +void cxl_mock_event_trigger(struct device *dev)
> > +{
> > +	struct cxl_mockmem_data *mdata = dev_get_drvdata(dev);
> > +	struct mock_event_store *mes = &mdata->mes;
> > +	int i;
> > +
> > +	for (i = CXL_EVENT_TYPE_INFO; i < CXL_EVENT_TYPE_MAX; i++) {
> > +		struct mock_event_log *log;
> > +
> > +		log = find_event_log(dev, i);
> > +		if (log)
> > +			reset_event_log(log);
> > +	}
> > +
> > +	cxl_mem_get_event_records(mes->cxlds, mes->ev_status);
> > +}
> > +EXPORT_SYMBOL_GPL(cxl_mock_event_trigger);
> > +
> > +struct cxl_event_record_raw maint_needed = {
> > +	.hdr = {
> > +		.id = UUID_INIT(0xBA5EBA11, 0xABCD, 0xEFEB,
> > +				0xa5, 0x5a, 0xa5, 0x5a, 0xa5, 0xa5, 0x5a, 0xa5),
> > +		.length = sizeof(struct cxl_event_record_raw),
> > +		.flags[0] = CXL_EVENT_RECORD_FLAG_MAINT_NEEDED,
> > +		/* .handle = Set dynamically */
> > +		.related_handle = cpu_to_le16(0xa5b6),
> > +	},
> > +	.data = { 0xDE, 0xAD, 0xBE, 0xEF },
> > +};
> > +
> > +struct cxl_event_record_raw hardware_replace = {
> > +	.hdr = {
> > +		.id = UUID_INIT(0xABCDEFEB, 0xBA11, 0xBA5E,
> > +				0xa5, 0x5a, 0xa5, 0x5a, 0xa5, 0xa5, 0x5a, 0xa5),
> > +		.length = sizeof(struct cxl_event_record_raw),
> > +		.flags[0] = CXL_EVENT_RECORD_FLAG_HW_REPLACE,
> > +		/* .handle = Set dynamically */
> > +		.related_handle = cpu_to_le16(0xb6a5),
> > +	},
> > +	.data = { 0xDE, 0xAD, 0xBE, 0xEF },
> > +};
> > +
> > +void cxl_mock_add_event_logs(struct mock_event_store *mes)
> > +{
> > +	event_store_add_event(mes, CXL_EVENT_TYPE_INFO, &maint_needed);
> > +	mes->ev_status |= CXLDEV_EVENT_STATUS_INFO;
> > +
> > +	event_store_add_event(mes, CXL_EVENT_TYPE_FATAL, &hardware_replace);
> > +	mes->ev_status |= CXLDEV_EVENT_STATUS_FATAL;
> > +}
> > +EXPORT_SYMBOL_GPL(cxl_mock_add_event_logs);
> > diff --git a/tools/testing/cxl/test/events.h b/tools/testing/cxl/test/events.h
> > new file mode 100644
> > index 000000000000..626cd79f1871
> > --- /dev/null
> > +++ b/tools/testing/cxl/test/events.h
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef CXL_TEST_EVENTS_H
> > +#define CXL_TEST_EVENTS_H
> > +
> > +#include <cxlmem.h>
> > +
> > +#define CXL_TEST_EVENT_CNT_MAX 15
> > +
> > +/* Set a number of events to return at a time for simulation.  */
> > +#define CXL_TEST_EVENT_CNT 3
> > +
> > +struct mock_event_log {
> > +	u16 clear_idx;
> > +	u16 cur_idx;
> > +	u16 nr_events;
> > +	struct cxl_event_record_raw *events[CXL_TEST_EVENT_CNT_MAX];
> > +};
> > +
> > +struct mock_event_store {
> > +	struct cxl_dev_state *cxlds;
> > +	struct mock_event_log mock_logs[CXL_EVENT_TYPE_MAX];
> > +	u32 ev_status;
> > +};
> > +
> > +int mock_get_event(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
> > +int mock_clear_event(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
> > +void cxl_mock_add_event_logs(struct mock_event_store *mes);
> > +void cxl_mock_remove_event_logs(struct device *dev);
> > +void cxl_mock_event_trigger(struct device *dev);
> > +
> > +#endif /* CXL_TEST_EVENTS_H */
> > diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> > index 5e4ecd93f1d2..7674d6305d28 100644
> > --- a/tools/testing/cxl/test/mem.c
> > +++ b/tools/testing/cxl/test/mem.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/sizes.h>
> >  #include <linux/bits.h>
> >  #include <cxlmem.h>
> > +#include "mock.h"
> >  
> >  #define LSA_SIZE SZ_128K
> >  #define DEV_SIZE SZ_2G
> > @@ -67,16 +68,6 @@ static struct {
> >  
> >  #define PASS_TRY_LIMIT 3
> >  
> > -struct cxl_mockmem_data {
> > -	void *lsa;
> > -	u32 security_state;
> > -	u8 user_pass[NVDIMM_PASSPHRASE_LEN];
> > -	u8 master_pass[NVDIMM_PASSPHRASE_LEN];
> > -	int user_limit;
> > -	int master_limit;
> > -
> > -};
> > -
> >  static int mock_gsl(struct cxl_mbox_cmd *cmd)
> >  {
> >  	if (cmd->size_out < sizeof(mock_gsl_payload))
> > @@ -582,6 +573,12 @@ static int cxl_mock_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *
> >  	case CXL_MBOX_OP_GET_PARTITION_INFO:
> >  		rc = mock_partition_info(cxlds, cmd);
> >  		break;
> > +	case CXL_MBOX_OP_GET_EVENT_RECORD:
> > +		rc = mock_get_event(cxlds, cmd);
> > +		break;
> > +	case CXL_MBOX_OP_CLEAR_EVENT_RECORD:
> > +		rc = mock_clear_event(cxlds, cmd);
> > +		break;
> >  	case CXL_MBOX_OP_SET_LSA:
> >  		rc = mock_set_lsa(cxlds, cmd);
> >  		break;
> > @@ -628,6 +625,15 @@ static bool is_rcd(struct platform_device *pdev)
> >  	return !!id->driver_data;
> >  }
> >  
> > +static ssize_t event_trigger_store(struct device *dev,
> > +				   struct device_attribute *attr,
> > +				   const char *buf, size_t count)
> > +{
> > +	cxl_mock_event_trigger(dev);
> > +	return count;
> > +}
> > +static DEVICE_ATTR_WO(event_trigger);
> > +
> >  static int cxl_mock_mem_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -655,6 +661,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
> >  	cxlds->serial = pdev->id;
> >  	cxlds->mbox_send = cxl_mock_mbox_send;
> >  	cxlds->payload_size = SZ_4K;
> > +	cxlds->event.buf = (struct cxl_get_event_payload *) mdata->event_buf;
> >  	if (is_rcd(pdev)) {
> >  		cxlds->rcd = true;
> >  		cxlds->component_reg_phys = CXL_RESOURCE_NONE;
> > @@ -672,10 +679,15 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
> >  	if (rc)
> >  		return rc;
> >  
> > +	mdata->mes.cxlds = cxlds;
> > +	cxl_mock_add_event_logs(&mdata->mes);
> > +
> >  	cxlmd = devm_cxl_add_memdev(cxlds);
> >  	if (IS_ERR(cxlmd))
> >  		return PTR_ERR(cxlmd);
> >  
> > +	cxl_mem_get_event_records(cxlds, CXLDEV_EVENT_STATUS_ALL);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -714,6 +726,7 @@ static DEVICE_ATTR_RW(security_lock);
> >  
> >  static struct attribute *cxl_mock_mem_attrs[] = {
> >  	&dev_attr_security_lock.attr,
> > +	&dev_attr_event_trigger.attr,
> >  	NULL
> >  };
> >  ATTRIBUTE_GROUPS(cxl_mock_mem);
> > diff --git a/tools/testing/cxl/test/mock.h b/tools/testing/cxl/test/mock.h
> > index ef33f159375e..e7827ddedb06 100644
> > --- a/tools/testing/cxl/test/mock.h
> > +++ b/tools/testing/cxl/test/mock.h
> > @@ -3,6 +3,18 @@
> >  #include <linux/list.h>
> >  #include <linux/acpi.h>
> >  #include <cxl.h>
> > +#include "events.h"
> > +
> > +struct cxl_mockmem_data {
> > +	void *lsa;
> > +	u32 security_state;
> > +	u8 user_pass[NVDIMM_PASSPHRASE_LEN];
> > +	u8 master_pass[NVDIMM_PASSPHRASE_LEN];
> > +	int user_limit;
> > +	int master_limit;
> > +	struct mock_event_store mes;
> > +	u8 event_buf[SZ_4K];
> > +};
> 
> Related to the above this does not belong here. test/mock.h is dedicated
> to supporting the symbols that are rerouted between the CXL modules,
> cxl_mock, and cxl_test.

Powered by blists - more mailing lists