Re: [PATCH v12 1/6] cxl/mbox: Add GET_POISON_LIST mailbox command

[Dan Williams <dan.j.williams@intel.com>]

Alison Schofield wrote:
> On Tue, Apr 11, 2023 at 06:47:56PM -0700, Dan Williams wrote:
> > alison.schofield@ wrote:
> > > From: Alison Schofield <alison.schofield@intel.com>
> > > 
> > > CXL devices maintain a list of locations that are poisoned or result
> > > in poison if the addresses are accessed by the host.
> > > 
> > > Per the spec, (CXL 3.0 8.2.9.8.4.1), the device returns this Poison
> > > list as a set of Media Error Records that include the source of the
> > > error, the starting device physical address, and length. The length is
> > > the number of adjacent DPAs in the record and is in units of 64 bytes.
> > > 
> > > Retrieve the poison list.
> > > 
> > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > > ---
> > >  drivers/cxl/core/mbox.c | 71 +++++++++++++++++++++++++++++++++++++++++
> > >  drivers/cxl/cxlmem.h    | 67 ++++++++++++++++++++++++++++++++++++++
> > >  drivers/cxl/pci.c       |  4 +++
> > >  3 files changed, 142 insertions(+)
> > > 
> > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > > index f2addb457172..69a5d69dd53b 100644
> > > --- a/drivers/cxl/core/mbox.c
> > > +++ b/drivers/cxl/core/mbox.c
> > > @@ -5,6 +5,8 @@
> > >  #include <linux/debugfs.h>
> > >  #include <linux/ktime.h>
> > >  #include <linux/mutex.h>
> > > +#include <asm/unaligned.h>
> > > +#include <cxlpci.h>
> > >  #include <cxlmem.h>
> > >  #include <cxl.h>
> > >  
> > > @@ -994,6 +996,7 @@ int cxl_dev_state_identify(struct cxl_dev_state *cxlds)
> > >  	/* See CXL 2.0 Table 175 Identify Memory Device Output Payload */
> > >  	struct cxl_mbox_identify id;
> > >  	struct cxl_mbox_cmd mbox_cmd;
> > > +	u32 val;
> > >  	int rc;
> > >  
> > >  	mbox_cmd = (struct cxl_mbox_cmd) {
> > > @@ -1017,6 +1020,11 @@ int cxl_dev_state_identify(struct cxl_dev_state *cxlds)
> > >  	cxlds->lsa_size = le32_to_cpu(id.lsa_size);
> > >  	memcpy(cxlds->firmware_version, id.fw_revision, sizeof(id.fw_revision));
> > >  
> > > +	if (test_bit(CXL_MEM_COMMAND_ID_GET_POISON, cxlds->enabled_cmds)) {
> > > +		val = get_unaligned_le24(id.poison_list_max_mer);
> > > +		cxlds->poison.max_errors = min_t(u32, val, CXL_POISON_LIST_MAX);
> > > +	}
> > > +
> > 
> > With this new interface I do not expect we want to support user tooling
> > that wants to retrieve the list via ioctl. So I think this wants a
> > lead-in patch that deprecates the poison command support so that the
> > linux-cxl community only has one mechanism to maintain going forward.
> > 
> > Something like the below as a lead-in, and then you would add code to
> > cxl_walk_cel() to set a flag for the "get poison" machinery.
> 
> In the inject & clear series, I made the commands kernel exclusive (and
> also blocked their usage in raw mode).
> 
> https://lore.kernel.org/linux-cxl/1576040e-e8db-bc78-2fa3-622c8f7da8ec@intel.com/T/#m5b86f3e88ee7ad5b92843babdb9fd41b7f03cf36
> 
> Is that not enough or not the right protection?

I think I'm having deja vu and maybe we talked about this already and I
forgot the outcome?

The concern is having a consistent approach across the commands that are
valid from userspace all the time, those that are valid sometimes
(temporarily marked exclusive), and those that are unsupported from
userspace (unsupported via raw command being a special case of that).

For recent new mailbox functionality like CXL_MBOX_OP_GET_EVENT_RECORD
and the DCD commands, that are only expected to be executed from the
kernel, those are kept out of the CXL_CMDS list, not marked exclusive.

The commands marked exclusive like the label commands return EBUSY, but
only while libnvdimm owns the label area. If libnvdimm is disconnected
(echo pmemX > /sys/bus/cxl/drivers/cxl_pmem/unbind), then those commands
stop being exclusive.

So, I think I want to keep "exclusive" as a set of commands that are
suitable to build into a user tool just that the tool needs to know the
rules about when those commands might be busy.

For poison the user tool is expected to use sysfs + trace-events, so
including it in the list of available commands returned by
CXL_MEM_QUERY_COMMANDS only to never be able to execute it seems wrong.

Marking it deprecated as a "whoops" feels more honest and brings it in
line with the other commands that are permanently kernel exclusive.