Re: [PATCH v3 05/16] cxl/mbox: Add GET_FEATURE mailbox command

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 4 Feb 2025 16:54:43 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> Dave Jiang wrote:
> > From: Shiju Jose <shiju.jose@huawei.com>
> > 
> > Add support for GET_FEATURE mailbox command.
> > 
> > CXL spec r3.2 section 8.2.9.6 describes optional device specific features.
> > The settings of a feature can be retrieved using Get Feature command.
> > CXL spec r3.2 section 8.2.9.6.2 describes Get Feature command.
> > 
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Shiju Jose <shiju.jose@huawei.com>
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > ---
> >  drivers/cxl/core/Makefile   |  1 +
> >  drivers/cxl/core/features.c | 59 +++++++++++++++++++++++++++++++++++++
> >  include/cxl/features.h      | 29 ++++++++++++++++++
> >  tools/testing/cxl/Kbuild    |  1 +
> >  4 files changed, 90 insertions(+)
> >  create mode 100644 drivers/cxl/core/features.c
> > 
> > diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> > index 9259bcc6773c..73b6348afd67 100644
> > --- a/drivers/cxl/core/Makefile
> > +++ b/drivers/cxl/core/Makefile
> > @@ -14,5 +14,6 @@ cxl_core-y += pci.o
> >  cxl_core-y += hdm.o
> >  cxl_core-y += pmu.o
> >  cxl_core-y += cdat.o
> > +cxl_core-y += features.o  
> 
> Would have expected:
> 
> cxl_core-$(CONFIG_CXL_FEATURES) += features.o
> 
> I think it is ok to throw all of Feature and FWCTL support behind a
> single config option. That is, until it becomes clear that someone has a
> "yes, kernel internal CXL Features, no FWCTL CXL Features" use case.
> 
> >  cxl_core-$(CONFIG_TRACING) += trace.o
> >  cxl_core-$(CONFIG_CXL_REGION) += region.o
> > diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> > new file mode 100644
> > index 000000000000..b01dc5ebb24d
> > --- /dev/null
> > +++ b/drivers/cxl/core/features.c
> > @@ -0,0 +1,59 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
> > +#include <linux/device.h>
> > +#include <cxl/mailbox.h>
> > +#include <cxl/features.h>
> > +#include "cxl.h"
> > +#include "core.h"
> > +#include "cxlmem.h"
> > +#include "features.h"
> > +
> > +size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
> > +		       enum cxl_get_feat_selection selection,
> > +		       void *feat_out, size_t feat_out_size, u16 offset,
> > +		       u16 *return_code)
> > +{
> > +	size_t data_to_rd_size, size_out;
> > +	struct cxl_mbox_get_feat_in pi;
> > +	struct cxl_mbox_cmd mbox_cmd;
> > +	size_t data_rcvd_size = 0;
> > +	int rc;
> > +
> > +	if (return_code)
> > +		*return_code = CXL_MBOX_CMD_RC_INPUT;
> > +
> > +	if (!feat_out || !feat_out_size)
> > +		return 0;
> > +
> > +	size_out = min(feat_out_size, cxl_mbox->payload_size);
> > +	uuid_copy(&pi.uuid, feat_uuid);
> > +	pi.selection = selection;
> > +	do {
> > +		data_to_rd_size = min(feat_out_size - data_rcvd_size,
> > +				      cxl_mbox->payload_size);
> > +		pi.offset = cpu_to_le16(offset + data_rcvd_size);
> > +		pi.count = cpu_to_le16(data_to_rd_size);
> > +
> > +		mbox_cmd = (struct cxl_mbox_cmd) {
> > +			.opcode = CXL_MBOX_OP_GET_FEATURE,
> > +			.size_in = sizeof(pi),
> > +			.payload_in = &pi,
> > +			.size_out = size_out,
> > +			.payload_out = feat_out + data_rcvd_size,
> > +			.min_out = data_to_rd_size,
> > +		};
> > +		rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
> > +		if (rc < 0 || !mbox_cmd.size_out) {
> > +			if (return_code)
> > +				*return_code = mbox_cmd.return_code;
> > +			return 0;
> > +		}
> > +		data_rcvd_size += mbox_cmd.size_out;
> > +	} while (data_rcvd_size < feat_out_size);
> > +
> > +	if (return_code)
> > +		*return_code = CXL_MBOX_CMD_RC_SUCCESS;
> > +
> > +	return data_rcvd_size;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_get_feature, "CXL");  
> 
> So I see this is exported to a new cxl_fwtcl.ko module, but I think all
> of this can be core built-in functionality similar to memdev ioctl and
> firmware upload support. As long as distributions can opt-out of FWCTL
> and Features at build time then this is no worse than
> CONFIG_CXL_MEM_RAW_COMMANDS from a proprietary use case / security model
> stance. With that software only needs to worry about finding a
> cxl_memdev object and not manually loading a cxl_fwctl module.

It'll get exported shortly anyway as it's used by the EDAC series
and that should be separate modules.  

I'm not disagreeing that it might not be needed for fwctl though
and obviously we can add the exports in that series (subject to
yet more versions as this changes...)


>