diff mbox series

[v6,09/14] cxl: Add support for fwctl RPC command to enable CXL feature commands

Message ID 20250218225721.2682235-10-dave.jiang@intel.com
State Superseded
Headers show
Series cxl: Add CXL feature commands support via fwctl | expand

Commit Message

Dave Jiang Feb. 18, 2025, 10:54 p.m. UTC
fwctl provides a fwctl_ops->fw_rpc() callback in order to issue ioctls
to a device. The cxl fwctl driver will start by supporting the CXL
Feature commands: Get Supported Features, Get Feature, and Set Feature.

The fw_rpc() callback provides 'enum fwctl_rpc_scope' parameter where
it indicates the security scope of the call. The Get Supported Features
and Get Feature calls can be executed with the scope of
FWCTL_RPC_CONFIGRATION. The Set Feature call is gated by the effects
of the Feature reported by Get Supported Features call for the specific
Feature.

Only "Get Supported Features" is supported in this patch. Additional
commands will be added in follow on patches. "Get Supported Features"
will filter the Features that are exclusive to the kernel. The flag
field of the Feature details will be cleared of the "Changeable"
field and the "set feat size" will be set to 0 to indicate that
the feature is not changeable.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v6:
- Embed hw op in 'fwctl_rpc_cxl'. (Saeed, Jason)
- Move set_features bits to set_features enabling patch. (Saeed)
- Fix copyright years. (Jason)
---
 drivers/cxl/core/features.c | 121 +++++++++++++++++++++++++++++++++++-
 include/uapi/cxl/features.h |   1 +
 include/uapi/fwctl/cxl.h    |  57 +++++++++++++++++
 3 files changed, 177 insertions(+), 2 deletions(-)
 create mode 100644 include/uapi/fwctl/cxl.h

Comments

Li Ming Feb. 19, 2025, 7:27 a.m. UTC | #1
On 2/19/2025 6:54 AM, Dave Jiang wrote:
> fwctl provides a fwctl_ops->fw_rpc() callback in order to issue ioctls
> to a device. The cxl fwctl driver will start by supporting the CXL
> Feature commands: Get Supported Features, Get Feature, and Set Feature.
>
> The fw_rpc() callback provides 'enum fwctl_rpc_scope' parameter where
> it indicates the security scope of the call. The Get Supported Features
> and Get Feature calls can be executed with the scope of
> FWCTL_RPC_CONFIGRATION. The Set Feature call is gated by the effects
> of the Feature reported by Get Supported Features call for the specific
> Feature.
>
> Only "Get Supported Features" is supported in this patch. Additional
> commands will be added in follow on patches. "Get Supported Features"
> will filter the Features that are exclusive to the kernel. The flag
> field of the Feature details will be cleared of the "Changeable"
> field and the "set feat size" will be set to 0 to indicate that
> the feature is not changeable.
>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>
Jonathan Cameron Feb. 19, 2025, 5:53 p.m. UTC | #2
On Tue, 18 Feb 2025 15:54:38 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> fwctl provides a fwctl_ops->fw_rpc() callback in order to issue ioctls
> to a device. The cxl fwctl driver will start by supporting the CXL
> Feature commands: Get Supported Features, Get Feature, and Set Feature.
> 
> The fw_rpc() callback provides 'enum fwctl_rpc_scope' parameter where
> it indicates the security scope of the call. The Get Supported Features
> and Get Feature calls can be executed with the scope of
> FWCTL_RPC_CONFIGRATION. The Set Feature call is gated by the effects
> of the Feature reported by Get Supported Features call for the specific
> Feature.
> 
> Only "Get Supported Features" is supported in this patch. Additional
> commands will be added in follow on patches. "Get Supported Features"
> will filter the Features that are exclusive to the kernel. The flag
> field of the Feature details will be cleared of the "Changeable"
> field and the "set feat size" will be set to 0 to indicate that
> the feature is not changeable.
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Hi Dave,

I'm probably missing something but I don't understand the need for
__DECLARE_FLEXIBLE_ARRAY() in the unions.  We always seem to use one of
the structs (some of which have trailing flexible arrays).


> ---
> v6:
> - Embed hw op in 'fwctl_rpc_cxl'. (Saeed, Jason)
> - Move set_features bits to set_features enabling patch. (Saeed)
> - Fix copyright years. (Jason)
> ---
>  drivers/cxl/core/features.c | 121 +++++++++++++++++++++++++++++++++++-
>  include/uapi/cxl/features.h |   1 +
>  include/uapi/fwctl/cxl.h    |  57 +++++++++++++++++
>  3 files changed, 177 insertions(+), 2 deletions(-)
>  create mode 100644 include/uapi/fwctl/cxl.h
> 
> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> index 846de3294a5e..106ea9e25c82 100644
> --- a/drivers/cxl/core/features.c
> +++ b/drivers/cxl/core/features.c
> @@ -4,6 +4,7 @@
>  #include <linux/device.h>
>  #include <cxl/mailbox.h>
>  #include <cxl/features.h>
> +#include <uapi/fwctl/cxl.h>
>  #include "cxl.h"
>  #include "core.h"
>  #include "cxlmem.h"
> @@ -349,11 +350,127 @@ static void cxlctl_close_uctx(struct fwctl_uctx *uctx)
>  {
>  }
>  
> +static void *cxlctl_get_supported_features(struct cxl_features_state *cxlfs,
> +					   const struct fwctl_rpc_cxl *rpc_in,
> +					   size_t *out_len)
> +{
> +	const struct cxl_mbox_get_sup_feats_in *feat_in;
> +	struct cxl_mbox_get_sup_feats_out *feat_out;
> +	struct cxl_feat_entry *pos;
> +	size_t out_size;
> +	int requested;
> +	u32 count;
> +	u16 start;
> +	int i;
> +
> +	if (rpc_in->op_size != sizeof(*feat_in))
> +		return ERR_PTR(-EINVAL);
> +
> +	feat_in = &rpc_in->get_sup_feats_in;
> +	count = le32_to_cpu(feat_in->count);
> +	start = le16_to_cpu(feat_in->start_idx);
> +	requested = count / sizeof(*pos);
> +
> +	/*
> +	 * Make sure that the total requested number of entries is not greater
> +	 * than the total number of supported features allowed for userspace.
> +	 */
> +	if (start >= cxlfs->entries->num_features)
> +		return ERR_PTR(-EINVAL);
> +
> +	requested = min_t(int, requested, cxlfs->entries->num_features - start);
> +
> +	out_size = sizeof(struct fwctl_rpc_cxl_out) +
> +		struct_size(feat_out, ents, requested);
> +
> +	struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) =
> +		kvzalloc(out_size, GFP_KERNEL);
> +	if (!rpc_out)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rpc_out->size = struct_size(feat_out, ents, requested);
> +	feat_out = (struct cxl_mbox_get_sup_feats_out *)rpc_out->payload;

Why not, 

	feat_out = rpc_out->get_sup_feats_out;
?


> +	if (requested == 0) {
> +		feat_out->num_entries = cpu_to_le16(requested);
> +		feat_out->supported_feats =
> +			cpu_to_le16(cxlfs->entries->num_features);
> +		rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS;
> +		*out_len = out_size;
> +		return no_free_ptr(rpc_out);
> +	}
> +
> +	for (i = start, pos = &feat_out->ents[0];
> +	     i < cxlfs->entries->num_features; i++, pos++) {
> +		if (i - start == requested)
> +			break;
> +
> +		memcpy(pos, &cxlfs->entries->ent[i], sizeof(*pos));
> +		/*
> +		 * If the feature is exclusive, set the set_feat_size to 0 to
> +		 * indicate that the feature is not changeable.
> +		 */
> +		if (is_cxl_feature_exclusive(pos)) {
> +			u32 flags;
> +
> +			pos->set_feat_size = 0;
> +			flags = le32_to_cpu(pos->flags);
> +			flags &= ~CXL_FEATURE_F_CHANGEABLE;
> +			pos->flags = cpu_to_le32(flags);
> +		}
> +	}
> +
> +	feat_out->num_entries = cpu_to_le16(requested);
> +	feat_out->supported_feats = cpu_to_le16(cxlfs->entries->num_features);
> +	rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS;
> +	*out_len = out_size;
> +
> +	return no_free_ptr(rpc_out);
> +}

> diff --git a/include/uapi/fwctl/cxl.h b/include/uapi/fwctl/cxl.h
> new file mode 100644
> index 000000000000..39996ec56816
> --- /dev/null
> +++ b/include/uapi/fwctl/cxl.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (c) 2024-2025 Intel Corporation
> + *
> + * These are definitions for the mailbox command interface of CXL subsystem.
> + */
> +#ifndef _UAPI_FWCTL_CXL_H_
> +#define _UAPI_FWCTL_CXL_H_
> +
> +#include <linux/types.h>
> +#include <linux/stddef.h>
> +#include <cxl/features.h>
> +
> +struct cxl_mbox_get_sup_feats_in;
> +struct cxl_mbox_get_sup_feats_out;

These are defined now in uapi/cxl/features.h anyway so why is forwards
def needed?

> +
> +/**
> + * struct fwctl_rpc_cxl - ioctl(FWCTL_RPC) input for CXL
> + * @opcode: CXL mailbox command opcode
> + * @flags: Flags for the command (input).
> + * @op_size: Size of input payload.
> + * @reserved1: Reserved. Must be 0s.
> + * @get_sup_feats_in: Get Supported Features input
> + * @op: hardware operation input byte array
> + */
> +struct fwctl_rpc_cxl {
> +	__struct_group(fwctl_rpc_cxl_hdr, hdr, /* no attrs */,
> +		__u32 opcode;
> +		__u32 flags;
> +		__u32 op_size;
> +		__u32 reserved1;
> +	);
> +	union {
> +		struct cxl_mbox_get_sup_feats_in get_sup_feats_in;
> +		__DECLARE_FLEX_ARRAY(__u8, op);

Similar to below. op isn't used in this patch that I can see.
Is it just here so it's obvious what op_size refers to?

> +	};
> +};
> +
> +/**
> + * struct fwctl_rpc_cxl_out - ioctl(FWCTL_RPC) output for CXL
> + * @size: Size of the output payload
> + * @retval: Return value from device
> + * @get_sup_feats_out: Get Supported Features output
> + * @payload: Return data from device
> + */
> +struct fwctl_rpc_cxl_out {
> +	__struct_group(fwctl_rpc_cxl_out_hdr, hdr, /* no attrs */,
> +		__u32 size;
> +		__u32 retval;
> +	);
> +	union {
> +		struct cxl_mbox_get_sup_feats_out get_sup_feats_out;
> +		__DECLARE_FLEX_ARRAY(__u8, payload);

I this patch I don't see a particular use for payload other
than where I've noted I think you can use get_sup_feats_out
directly.

> +	};
> +};
> +
> +#endif
Jason Gunthorpe Feb. 19, 2025, 5:56 p.m. UTC | #3
On Wed, Feb 19, 2025 at 05:53:12PM +0000, Jonathan Cameron wrote:

> I'm probably missing something but I don't understand the need for
> __DECLARE_FLEXIBLE_ARRAY() in the unions.  We always seem to use one of
> the structs (some of which have trailing flexible arrays).

Can you even use those defines in a uapi header???

Jason
Dave Jiang Feb. 19, 2025, 6 p.m. UTC | #4
On 2/19/25 10:56 AM, Jason Gunthorpe wrote:
> On Wed, Feb 19, 2025 at 05:53:12PM +0000, Jonathan Cameron wrote:
> 
>> I'm probably missing something but I don't understand the need for
>> __DECLARE_FLEXIBLE_ARRAY() in the unions.  We always seem to use one of
>> the structs (some of which have trailing flexible arrays).
> 
> Can you even use those defines in a uapi header???

Yes. It's defined in uapi/linux/stddef.h. I compiled and tested against the unit test in userspace. No issues.

> 
> Jason
>
Dave Jiang Feb. 19, 2025, 6:29 p.m. UTC | #5
On 2/19/25 10:53 AM, Jonathan Cameron wrote:
> On Tue, 18 Feb 2025 15:54:38 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> fwctl provides a fwctl_ops->fw_rpc() callback in order to issue ioctls
>> to a device. The cxl fwctl driver will start by supporting the CXL
>> Feature commands: Get Supported Features, Get Feature, and Set Feature.
>>
>> The fw_rpc() callback provides 'enum fwctl_rpc_scope' parameter where
>> it indicates the security scope of the call. The Get Supported Features
>> and Get Feature calls can be executed with the scope of
>> FWCTL_RPC_CONFIGRATION. The Set Feature call is gated by the effects
>> of the Feature reported by Get Supported Features call for the specific
>> Feature.
>>
>> Only "Get Supported Features" is supported in this patch. Additional
>> commands will be added in follow on patches. "Get Supported Features"
>> will filter the Features that are exclusive to the kernel. The flag
>> field of the Feature details will be cleared of the "Changeable"
>> field and the "set feat size" will be set to 0 to indicate that
>> the feature is not changeable.
>>
>> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Hi Dave,
> 
> I'm probably missing something but I don't understand the need for
> __DECLARE_FLEXIBLE_ARRAY() in the unions.  We always seem to use one of
> the structs (some of which have trailing flexible arrays).

There are some usages. I probably should move the addition of that to the said patch. For example, feature data output is a byte stream. 

> 
> 
>> ---
>> v6:
>> - Embed hw op in 'fwctl_rpc_cxl'. (Saeed, Jason)
>> - Move set_features bits to set_features enabling patch. (Saeed)
>> - Fix copyright years. (Jason)
>> ---
>>  drivers/cxl/core/features.c | 121 +++++++++++++++++++++++++++++++++++-
>>  include/uapi/cxl/features.h |   1 +
>>  include/uapi/fwctl/cxl.h    |  57 +++++++++++++++++
>>  3 files changed, 177 insertions(+), 2 deletions(-)
>>  create mode 100644 include/uapi/fwctl/cxl.h
>>
>> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
>> index 846de3294a5e..106ea9e25c82 100644
>> --- a/drivers/cxl/core/features.c
>> +++ b/drivers/cxl/core/features.c
>> @@ -4,6 +4,7 @@
>>  #include <linux/device.h>
>>  #include <cxl/mailbox.h>
>>  #include <cxl/features.h>
>> +#include <uapi/fwctl/cxl.h>
>>  #include "cxl.h"
>>  #include "core.h"
>>  #include "cxlmem.h"
>> @@ -349,11 +350,127 @@ static void cxlctl_close_uctx(struct fwctl_uctx *uctx)
>>  {
>>  }
>>  
>> +static void *cxlctl_get_supported_features(struct cxl_features_state *cxlfs,
>> +					   const struct fwctl_rpc_cxl *rpc_in,
>> +					   size_t *out_len)
>> +{
>> +	const struct cxl_mbox_get_sup_feats_in *feat_in;
>> +	struct cxl_mbox_get_sup_feats_out *feat_out;
>> +	struct cxl_feat_entry *pos;
>> +	size_t out_size;
>> +	int requested;
>> +	u32 count;
>> +	u16 start;
>> +	int i;
>> +
>> +	if (rpc_in->op_size != sizeof(*feat_in))
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	feat_in = &rpc_in->get_sup_feats_in;
>> +	count = le32_to_cpu(feat_in->count);
>> +	start = le16_to_cpu(feat_in->start_idx);
>> +	requested = count / sizeof(*pos);
>> +
>> +	/*
>> +	 * Make sure that the total requested number of entries is not greater
>> +	 * than the total number of supported features allowed for userspace.
>> +	 */
>> +	if (start >= cxlfs->entries->num_features)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	requested = min_t(int, requested, cxlfs->entries->num_features - start);
>> +
>> +	out_size = sizeof(struct fwctl_rpc_cxl_out) +
>> +		struct_size(feat_out, ents, requested);
>> +
>> +	struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) =
>> +		kvzalloc(out_size, GFP_KERNEL);
>> +	if (!rpc_out)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	rpc_out->size = struct_size(feat_out, ents, requested);
>> +	feat_out = (struct cxl_mbox_get_sup_feats_out *)rpc_out->payload;
> 
> Why not, 
> 
> 	feat_out = rpc_out->get_sup_feats_out;
> ?

I missed that one during refactoring.

> 
> 
>> +	if (requested == 0) {
>> +		feat_out->num_entries = cpu_to_le16(requested);
>> +		feat_out->supported_feats =
>> +			cpu_to_le16(cxlfs->entries->num_features);
>> +		rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS;
>> +		*out_len = out_size;
>> +		return no_free_ptr(rpc_out);
>> +	}
>> +
>> +	for (i = start, pos = &feat_out->ents[0];
>> +	     i < cxlfs->entries->num_features; i++, pos++) {
>> +		if (i - start == requested)
>> +			break;
>> +
>> +		memcpy(pos, &cxlfs->entries->ent[i], sizeof(*pos));
>> +		/*
>> +		 * If the feature is exclusive, set the set_feat_size to 0 to
>> +		 * indicate that the feature is not changeable.
>> +		 */
>> +		if (is_cxl_feature_exclusive(pos)) {
>> +			u32 flags;
>> +
>> +			pos->set_feat_size = 0;
>> +			flags = le32_to_cpu(pos->flags);
>> +			flags &= ~CXL_FEATURE_F_CHANGEABLE;
>> +			pos->flags = cpu_to_le32(flags);
>> +		}
>> +	}
>> +
>> +	feat_out->num_entries = cpu_to_le16(requested);
>> +	feat_out->supported_feats = cpu_to_le16(cxlfs->entries->num_features);
>> +	rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS;
>> +	*out_len = out_size;
>> +
>> +	return no_free_ptr(rpc_out);
>> +}
> 
>> diff --git a/include/uapi/fwctl/cxl.h b/include/uapi/fwctl/cxl.h
>> new file mode 100644
>> index 000000000000..39996ec56816
>> --- /dev/null
>> +++ b/include/uapi/fwctl/cxl.h
>> @@ -0,0 +1,57 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * Copyright (c) 2024-2025 Intel Corporation
>> + *
>> + * These are definitions for the mailbox command interface of CXL subsystem.
>> + */
>> +#ifndef _UAPI_FWCTL_CXL_H_
>> +#define _UAPI_FWCTL_CXL_H_
>> +
>> +#include <linux/types.h>
>> +#include <linux/stddef.h>
>> +#include <cxl/features.h>
>> +
>> +struct cxl_mbox_get_sup_feats_in;
>> +struct cxl_mbox_get_sup_feats_out;
> 
> These are defined now in uapi/cxl/features.h anyway so why is forwards
> def needed?

I'll drop those.

> 
>> +
>> +/**
>> + * struct fwctl_rpc_cxl - ioctl(FWCTL_RPC) input for CXL
>> + * @opcode: CXL mailbox command opcode
>> + * @flags: Flags for the command (input).
>> + * @op_size: Size of input payload.
>> + * @reserved1: Reserved. Must be 0s.
>> + * @get_sup_feats_in: Get Supported Features input
>> + * @op: hardware operation input byte array
>> + */
>> +struct fwctl_rpc_cxl {
>> +	__struct_group(fwctl_rpc_cxl_hdr, hdr, /* no attrs */,
>> +		__u32 opcode;
>> +		__u32 flags;
>> +		__u32 op_size;
>> +		__u32 reserved1;
>> +	);
>> +	union {
>> +		struct cxl_mbox_get_sup_feats_in get_sup_feats_in;
>> +		__DECLARE_FLEX_ARRAY(__u8, op);
> 
> Similar to below. op isn't used in this patch that I can see.
> Is it just here so it's obvious what op_size refers to?

Yes. and also I cast it to the feature uuid field as the common header. But I can move this addition to the get_feature enabling.

> 
>> +	};
>> +};
>> +
>> +/**
>> + * struct fwctl_rpc_cxl_out - ioctl(FWCTL_RPC) output for CXL
>> + * @size: Size of the output payload
>> + * @retval: Return value from device
>> + * @get_sup_feats_out: Get Supported Features output
>> + * @payload: Return data from device
>> + */
>> +struct fwctl_rpc_cxl_out {
>> +	__struct_group(fwctl_rpc_cxl_out_hdr, hdr, /* no attrs */,
>> +		__u32 size;
>> +		__u32 retval;
>> +	);
>> +	union {
>> +		struct cxl_mbox_get_sup_feats_out get_sup_feats_out;
>> +		__DECLARE_FLEX_ARRAY(__u8, payload);
> 
> I this patch I don't see a particular use for payload other
> than where I've noted I think you can use get_sup_feats_out
> directly.

The get_feature has a byte stream for feature data as output. It's also convenient for the user app to access the feature data.  

> 
>> +	};
>> +};
>> +
>> +#endif
>
diff mbox series

Patch

diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
index 846de3294a5e..106ea9e25c82 100644
--- a/drivers/cxl/core/features.c
+++ b/drivers/cxl/core/features.c
@@ -4,6 +4,7 @@ 
 #include <linux/device.h>
 #include <cxl/mailbox.h>
 #include <cxl/features.h>
+#include <uapi/fwctl/cxl.h>
 #include "cxl.h"
 #include "core.h"
 #include "cxlmem.h"
@@ -349,11 +350,127 @@  static void cxlctl_close_uctx(struct fwctl_uctx *uctx)
 {
 }
 
+static void *cxlctl_get_supported_features(struct cxl_features_state *cxlfs,
+					   const struct fwctl_rpc_cxl *rpc_in,
+					   size_t *out_len)
+{
+	const struct cxl_mbox_get_sup_feats_in *feat_in;
+	struct cxl_mbox_get_sup_feats_out *feat_out;
+	struct cxl_feat_entry *pos;
+	size_t out_size;
+	int requested;
+	u32 count;
+	u16 start;
+	int i;
+
+	if (rpc_in->op_size != sizeof(*feat_in))
+		return ERR_PTR(-EINVAL);
+
+	feat_in = &rpc_in->get_sup_feats_in;
+	count = le32_to_cpu(feat_in->count);
+	start = le16_to_cpu(feat_in->start_idx);
+	requested = count / sizeof(*pos);
+
+	/*
+	 * Make sure that the total requested number of entries is not greater
+	 * than the total number of supported features allowed for userspace.
+	 */
+	if (start >= cxlfs->entries->num_features)
+		return ERR_PTR(-EINVAL);
+
+	requested = min_t(int, requested, cxlfs->entries->num_features - start);
+
+	out_size = sizeof(struct fwctl_rpc_cxl_out) +
+		struct_size(feat_out, ents, requested);
+
+	struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) =
+		kvzalloc(out_size, GFP_KERNEL);
+	if (!rpc_out)
+		return ERR_PTR(-ENOMEM);
+
+	rpc_out->size = struct_size(feat_out, ents, requested);
+	feat_out = (struct cxl_mbox_get_sup_feats_out *)rpc_out->payload;
+	if (requested == 0) {
+		feat_out->num_entries = cpu_to_le16(requested);
+		feat_out->supported_feats =
+			cpu_to_le16(cxlfs->entries->num_features);
+		rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS;
+		*out_len = out_size;
+		return no_free_ptr(rpc_out);
+	}
+
+	for (i = start, pos = &feat_out->ents[0];
+	     i < cxlfs->entries->num_features; i++, pos++) {
+		if (i - start == requested)
+			break;
+
+		memcpy(pos, &cxlfs->entries->ent[i], sizeof(*pos));
+		/*
+		 * If the feature is exclusive, set the set_feat_size to 0 to
+		 * indicate that the feature is not changeable.
+		 */
+		if (is_cxl_feature_exclusive(pos)) {
+			u32 flags;
+
+			pos->set_feat_size = 0;
+			flags = le32_to_cpu(pos->flags);
+			flags &= ~CXL_FEATURE_F_CHANGEABLE;
+			pos->flags = cpu_to_le32(flags);
+		}
+	}
+
+	feat_out->num_entries = cpu_to_le16(requested);
+	feat_out->supported_feats = cpu_to_le16(cxlfs->entries->num_features);
+	rpc_out->retval = CXL_MBOX_CMD_RC_SUCCESS;
+	*out_len = out_size;
+
+	return no_free_ptr(rpc_out);
+}
+
+static bool cxlctl_validate_hw_command(struct cxl_features_state *cxlfs,
+				       const struct fwctl_rpc_cxl *rpc_in,
+				       enum fwctl_rpc_scope scope,
+				       u16 opcode)
+{
+	struct cxl_mailbox *cxl_mbox = &cxlfs->cxlds->cxl_mbox;
+
+	switch (opcode) {
+	case CXL_MBOX_OP_GET_SUPPORTED_FEATURES:
+		if (cxl_mbox->feat_cap < CXL_FEATURES_RO)
+			return false;
+		if (scope >= FWCTL_RPC_CONFIGURATION)
+			return true;
+		return false;
+	default:
+		return false;
+	}
+}
+
+static void *cxlctl_handle_commands(struct cxl_features_state *cxlfs,
+				    const struct fwctl_rpc_cxl *rpc_in,
+				    size_t *out_len, u16 opcode)
+{
+	switch (opcode) {
+	case CXL_MBOX_OP_GET_SUPPORTED_FEATURES:
+		return cxlctl_get_supported_features(cxlfs, rpc_in, out_len);
+	default:
+		return ERR_PTR(-EOPNOTSUPP);
+	}
+}
+
 static void *cxlctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
 			   void *in, size_t in_len, size_t *out_len)
 {
-	/* Place holder */
-	return ERR_PTR(-EOPNOTSUPP);
+	struct fwctl_device *fwctl_dev = uctx->fwctl;
+	struct cxl_memdev *cxlmd = fwctl_to_memdev(fwctl_dev);
+	struct cxl_features_state *cxlfs = to_cxlfs(cxlmd->cxlds);
+	const struct fwctl_rpc_cxl *rpc_in = in;
+	u16 opcode = rpc_in->opcode;
+
+	if (!cxlctl_validate_hw_command(cxlfs, rpc_in, scope, opcode))
+		return ERR_PTR(-EINVAL);
+
+	return cxlctl_handle_commands(cxlfs, rpc_in, out_len, opcode);
 }
 
 static const struct fwctl_ops cxlctl_ops = {
diff --git a/include/uapi/cxl/features.h b/include/uapi/cxl/features.h
index 17f9da63f982..eae0ce008c3a 100644
--- a/include/uapi/cxl/features.h
+++ b/include/uapi/cxl/features.h
@@ -33,6 +33,7 @@  struct cxl_mbox_get_sup_feats_in {
 #define CXL_CMD_EFFECTS_VALID			BIT(9)
 #define CXL_CMD_CONFIG_CHANGE_CONV_RESET	BIT(10)
 #define CXL_CMD_CONFIG_CHANGE_CXL_RESET		BIT(11)
+#define CXL_CMD_EFFECTS_RESERVED		GENMASK(15, 12)
 
 /*
  * CXL spec r3.2 Table 8-109
diff --git a/include/uapi/fwctl/cxl.h b/include/uapi/fwctl/cxl.h
new file mode 100644
index 000000000000..39996ec56816
--- /dev/null
+++ b/include/uapi/fwctl/cxl.h
@@ -0,0 +1,57 @@ 
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (c) 2024-2025 Intel Corporation
+ *
+ * These are definitions for the mailbox command interface of CXL subsystem.
+ */
+#ifndef _UAPI_FWCTL_CXL_H_
+#define _UAPI_FWCTL_CXL_H_
+
+#include <linux/types.h>
+#include <linux/stddef.h>
+#include <cxl/features.h>
+
+struct cxl_mbox_get_sup_feats_in;
+struct cxl_mbox_get_sup_feats_out;
+
+/**
+ * struct fwctl_rpc_cxl - ioctl(FWCTL_RPC) input for CXL
+ * @opcode: CXL mailbox command opcode
+ * @flags: Flags for the command (input).
+ * @op_size: Size of input payload.
+ * @reserved1: Reserved. Must be 0s.
+ * @get_sup_feats_in: Get Supported Features input
+ * @op: hardware operation input byte array
+ */
+struct fwctl_rpc_cxl {
+	__struct_group(fwctl_rpc_cxl_hdr, hdr, /* no attrs */,
+		__u32 opcode;
+		__u32 flags;
+		__u32 op_size;
+		__u32 reserved1;
+	);
+	union {
+		struct cxl_mbox_get_sup_feats_in get_sup_feats_in;
+		__DECLARE_FLEX_ARRAY(__u8, op);
+	};
+};
+
+/**
+ * struct fwctl_rpc_cxl_out - ioctl(FWCTL_RPC) output for CXL
+ * @size: Size of the output payload
+ * @retval: Return value from device
+ * @get_sup_feats_out: Get Supported Features output
+ * @payload: Return data from device
+ */
+struct fwctl_rpc_cxl_out {
+	__struct_group(fwctl_rpc_cxl_out_hdr, hdr, /* no attrs */,
+		__u32 size;
+		__u32 retval;
+	);
+	union {
+		struct cxl_mbox_get_sup_feats_out get_sup_feats_out;
+		__DECLARE_FLEX_ARRAY(__u8, payload);
+	};
+};
+
+#endif