Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 16 Dec 2022 13:54:01 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
CC:     Dan Williams <dan.j.williams@intel.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Alison Schofield <alison.schofield@intel.com>,
        "Vishal Verma" <vishal.l.verma@intel.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        "Dave Jiang" <dave.jiang@intel.com>,
        <linux-kernel@vger.kernel.org>, <linux-pci@vger.kernel.org>,
        <linux-acpi@vger.kernel.org>, <linux-cxl@vger.kernel.org>
Subject: Re: [PATCH V4 2/9] cxl/mem: Read, trace, and clear events on driver
 load
Message-ID: <Y5zo+UqOmGCE4ObC@iweiny-desk3>
References: <20221212070627.1372402-1-ira.weiny@intel.com>
 <20221212070627.1372402-3-ira.weiny@intel.com>
 <20221216153939.00007c41@Huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20221216153939.00007c41@Huawei.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?xyY3dioahmWaoDRKqnakW+Kg7TEWJmB7Od2frDsPaRMoLg+fzsDhCPSeRdYX?=
 =?us-ascii?Q?li47w5K8dxp9MGqtQRM8lrK4G5ivShUN+8es2nJY2EtHPUf1RLkYqkqq2fZX?=
 =?us-ascii?Q?vyAk9MeDgOyRhKywK0SOvSiB86foItKfpXwvSNjpRwKaK3/r5+1yhjfSAxX2?=
 =?us-ascii?Q?61YzU59DW4oFpAOz2uH22jTE+hRjvv8gMVOkz9Y2WtZBvgaDvRU7QU27ZUHY?=
 =?us-ascii?Q?lSW2uY2VW9VxVJqQROhLvVYnVBIGpPsGbyoyYCHNzAWESzd/AgRsXGKr4S4i?=
 =?us-ascii?Q?1Pj5B+DIh/6VNQTeD8ubPLGSZho9I9I+iS36l4pEIlsys1/kjpjz1D/hL11O?=
 =?us-ascii?Q?/rznyq4QybTeuLLUVFCPJ0/tVkEaISWedWbzIHxzwPIFThQTLWaJAaJ2RFhS?=
 =?us-ascii?Q?KPaJucExy5FfGkWldGlI9vjSV6HAK5PCl7ZcBKszB96ADdqMOaivRREj0V+d?=
 =?us-ascii?Q?zMNVS0mFyxIFVInpnKHlS/J7qbSkgdmOaBqwjkcdZvJJ9VBxoxBsOxoL1Z15?=
 =?us-ascii?Q?wQ2vxSSS/glfPMBdCh1dqol2t/F3QTg3CX0LtReRZxC/A4MmuAblkVTfmrq1?=
 =?us-ascii?Q?mmrlRqOIJ3FmcJBfyIIcYH7txxZo8zDoRtAU7h0fC/av8XSpGamky6TM6lfn?=
 =?us-ascii?Q?yCrNoc+ck+O8lyhZHlg0tCXNB84Eq27Cch7Ou3IBayyrfmiji869edEaJNZf?=
 =?us-ascii?Q?p3VhrWTOpS1JO9oQgy1qwrtc+ObnwCRyL94Dwfu8viNCQQLBz//fdI6Uak/F?=
 =?us-ascii?Q?hE9REoqkfg9IujyOA0TDJN/uSAtoqKJPjYvU5+HHv/0zdRWwUtIUZfEka2ji?=
 =?us-ascii?Q?gdrzGAfdHSLJZ+vdAKJLA+zS6M844oK7eh01i2b8Ha2mMzSqpXtSlUYVsaW+?=
 =?us-ascii?Q?pI0l8X04mNscDrnTvfQmDkc8mEk6lpU6TQqDLC5WfNAC5eRkMj7TYZyH38PL?=
 =?us-ascii?Q?VOnTIIflL6jwjC8bs23NH7fT3pwSRNGBVADzCUeTqjtIfRsQa3u04rTfmHqu?=
 =?us-ascii?Q?8iibPki5pSkRHKcW1miZE2rNmPxSmAjd7+EfcYgZ9npLU2fWjJ/0IQHOpTJI?=
 =?us-ascii?Q?iZVlhvFJFPX9Drt+2SoovPeJuQCC+3GzdQw3F6zd0/VOhBLvbiSs5d4lVJ5p?=
 =?us-ascii?Q?7bhpLNl//CwykTuBOtHI6xUTgeCVz5lhFhma8RAPGaUHVssZgDRrXqGtnmGW?=
 =?us-ascii?Q?i6cPblYGvuvuENSV7d8YRbxS5aT6iYrmpe97uSBuY9C17Y5OZjrZVb1OjshM?=
 =?us-ascii?Q?8qmXvGBPkB2FDVL97c+flc1jBp62g0yFdimtGlGB0Fb/8x+cc9re0z7m8ihm?=
 =?us-ascii?Q?6iyYel4Y4bBSCvUddv4gQo4I5qn5CKP37FV7pfakQlchtUtWlZKzwGJZ0gNz?=
 =?us-ascii?Q?U93v9xfSK5JYUFgJu7tGXFm25IV9cakbdv5dZCEbbLOfLKJLCN7UcNki+U5b?=
 =?us-ascii?Q?iU9Fr3aW+LPVFz39Xj3KZOB0b7FfU5QQjz1A2NBAB7uaRv+ernGx4i0Txk2K?=
 =?us-ascii?Q?35sbC6LtNDt2vesT4rCiMcTK6Y/HKz5TiXK9FhcqH8RyGWvnkZkLmony2KN0?=
 =?us-ascii?Q?MMQuqPUBPOmcVpSxr9M1GP0JxmlFdq2yo9jwib0tJO6LQSNYeKGpY7bipOGb?=
 =?us-ascii?Q?Mw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f63f6bff-db95-4a22-6275-08dadfb00d4e
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Dec 2022 21:54:05.8543
 (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: 2YT387RgsRHC+cI2qOGJKUaadvK4QJ8vju+WNuo19Qj1UtnbkCBoJLMlhVvhGQyHuoP/mVMO6QZgzr/HVSkrYw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7909
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Dec 16, 2022 at 03:39:39PM +0000, Jonathan Cameron wrote:
> On Sun, 11 Dec 2022 23:06:20 -0800
> ira.weiny@intel.com wrote:
> 
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > CXL devices have multiple event logs which can be queried for CXL event
> > records.  Devices are required to support the storage of at least one
> > event record in each event log type.
> > 
> > Devices track event log overflow by incrementing a counter and tracking
> > the time of the first and last overflow event seen.
> > 
> > Software queries events via the Get Event Record mailbox command; CXL
> > rev 3.0 section 8.2.9.2.2 and clears events via CXL rev 3.0 section
> > 8.2.9.2.3 Clear Event Records mailbox command.
> > 
> > If the result of negotiating CXL Error Reporting Control is OS control,
> > read and clear all event logs on driver load.
> > 
> > Ensure a clean slate of events by reading and clearing the events on
> > driver load.
> > 
> > The status register is not used because a device may continue to trigger
> > events and the only requirement is to empty the log at least once.  This
> > allows for the required transition from empty to non-empty for interrupt
> > generation.  Handling of interrupts is in a follow on patch.
> > 
> > The device can return up to 1MB worth of event records per query.
> > Allocate a shared large buffer to handle the max number of records based
> > on the mailbox payload size.
> > 
> > This patch traces a raw event record and leaves specific event record
> > type tracing to subsequent patches.  Macros are created to aid in
> > tracing the common CXL Event header fields.
> > 
> > Each record is cleared explicitly.  A clear all bit is specified but is
> > only valid when the log overflows.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> A few things noticed inline.  I've tightened the QEMU code to reject the
> case of the input payload claims to be bigger than the mailbox size
> and hacked the size down to 256 bytes so it triggers the problem
> highlighted below.

I'm not sure what you did here.

> 
> > 
> > ---
> > Changes from V3:
> > 	Dan
> > 		Split off _OSC pcie bits
> > 			Use existing style for host bridge flag in that
> > 			patch
> > 		Clean up event processing loop
> > 		Use dev_err_ratelimited()
> > 		Clean up version change log
> > 		Delete 'EVENT LOG OVERFLOW'
> > 		Remove cxl_clear_event_logs()
> > 		Add comment for native cxl control
> > 		Fail driver load on event buf allocation failure
> > 		Comment why events are not processed without _OSC flag
> > ---
> >  drivers/cxl/core/mbox.c  | 136 +++++++++++++++++++++++++++++++++++++++
> >  drivers/cxl/core/trace.h | 120 ++++++++++++++++++++++++++++++++++
> >  drivers/cxl/cxl.h        |  12 ++++
> >  drivers/cxl/cxlmem.h     |  84 ++++++++++++++++++++++++
> >  drivers/cxl/pci.c        |  40 ++++++++++++
> >  5 files changed, 392 insertions(+)
> > 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index b03fba212799..9fb327370e08 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> 
> > +static int cxl_clear_event_record(struct cxl_dev_state *cxlds,
> > +				  enum cxl_event_log_type log,
> > +				  struct cxl_get_event_payload *get_pl)
> > +{
> > +	struct cxl_mbox_clear_event_payload payload = {
> > +		.event_log = log,
> > +	};
> > +	u16 total = le16_to_cpu(get_pl->record_count);
> > +	u8 max_handles = CXL_CLEAR_EVENT_MAX_HANDLES;
> > +	size_t pl_size = sizeof(payload);
> > +	struct cxl_mbox_cmd mbox_cmd;
> > +	u16 cnt;
> > +	int rc;
> > +	int i;
> > +
> > +	/* Payload size may limit the max handles */
> > +	if (pl_size > cxlds->payload_size) {
> > +		max_handles = CXL_CLEAR_EVENT_LIMIT_HANDLES(cxlds->payload_size);
> > +		pl_size = cxlds->payload_size;

pl_size is only the max size possible if that size was smaller than the size of
the record [sizeof(payload) above].

> > +	}
> > +
> > +	mbox_cmd = (struct cxl_mbox_cmd) {
> > +		.opcode = CXL_MBOX_OP_CLEAR_EVENT_RECORD,
> > +		.payload_in = &payload,
> > +		.size_in = pl_size,
> 
> This payload size should be whatever we need to store the records,
> not the max size possible.  Particularly as that size is currently
> bigger than the mailbox might be.

But the above check and set ensures that does not happen.

> 
> It shouldn't fail (I think) simply because a later version of the spec might
> add more to this message and things should still work, but definitely not
> good practice to tell the hardware this is much longer than it actually is.

I don't follow.

The full payload is going to be sent even if we are just clearing 1 record
which is inefficient but it should never overflow the hardware because it is
limited by the check above.

So why would this be a problem?

> 
> 
> > +	};
> > +
> > +	/*
> > +	 * Clear Event Records uses u8 for the handle cnt while Get Event
> > +	 * Record can return up to 0xffff records.
> > +	 */
> > +	i = 0;
> > +	for (cnt = 0; cnt < total; cnt++) {
> > +		payload.handle[i++] = get_pl->records[cnt].hdr.handle;
> > +		dev_dbg(cxlds->dev, "Event log '%d': Clearing %u\n",
> > +			log, le16_to_cpu(payload.handle[i]));
> > +
> > +		if (i == max_handles) {
> > +			payload.nr_recs = i;
> > +			rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
> > +			if (rc)
> > +				return rc;
> > +			i = 0;
> > +		}
> > +	}
> > +
> > +	/* Clear what is left if any */
> > +	if (i) {
> > +		payload.nr_recs = i;
> > +		rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
> > +		if (rc)
> > +			return rc;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> 
> ...
> 
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index ab138004f644..dd9aa3dd738e 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> 
> ...
> 
> > +
> > +/*
> > + * Clear Event Records input payload
> > + * CXL rev 3.0 section 8.2.9.2.3; Table 8-51
> > + */
> > +#define CXL_CLEAR_EVENT_MAX_HANDLES (0xff)
> > +struct cxl_mbox_clear_event_payload {
> > +	u8 event_log;		/* enum cxl_event_log_type */
> > +	u8 clear_flags;
> > +	u8 nr_recs;
> > +	u8 reserved[3];
> > +	__le16 handle[CXL_CLEAR_EVENT_MAX_HANDLES];
> 
> Doesn't fit in the smallest possible payload buffer.
> It's 526 bytes long.  Payload buffer might be 256 bytes in total.
> (8.2.8.4.3 Mailbox capabilities)
> 
> Lazy approach, make this smaller and do more loops when clearing.
> If we want to optimize this later can expand it to this size.

I agree but the code already checks for and adjusts this on the fly based on
cxlds->payload_size?

 +	/* Payload size may limit the max handles */
 +	if (pl_size > cxlds->payload_size) {
 +		max_handles = CXL_CLEAR_EVENT_LIMIT_HANDLES(cxlds->payload_size);
 +		pl_size = cxlds->payload_size;
 +	}

Why is this not ok?  [Other than being potentially inefficient.]

Do you have a patch to qemu which causes this?

Ira

> > +} __packed;
> > +#define CXL_CLEAR_EVENT_LIMIT_HANDLES(payload_size)			\
> > +	(((payload_size) -						\
> > +		(sizeof(struct cxl_mbox_clear_event_payload) -		\
> > +		 (sizeof(__le16) * CXL_CLEAR_EVENT_MAX_HANDLES))) /	\
> > +		sizeof(__le16))
> > +
> 
> ...
>