Re: [PATCH v1 14/19] cxl: Add support for fwctl RPC command to enable CXL feature commands

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

On Wed, 22 Jan 2025 16:50:45 -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 and only
> report out features that are not kernel only.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
A few comments inline.

Jonathan

>

> diff --git a/drivers/cxl/features.c b/drivers/cxl/features.c
> index cc72e73ae8d6..e65fc8479d21 100644
> --- a/drivers/cxl/features.c
> +++ b/drivers/cxl/features.c


> +static struct cxl_mem_command *
> +cxlctl_get_valid_hw_command(struct cxl_features_state *cfs,
> +			    const struct fwctl_rpc_cxl *rpc_in,
> +			    enum fwctl_rpc_scope scope)
> +{
> +	struct cxl_mem_command *cmd;
> +
> +	if (!cfs->num_features)
> +		return ERR_PTR(-EOPNOTSUPP);
> +
> +	cmd = cxl_find_feature_command_by_id(rpc_in->command_id);
> +	if (!cmd)
> +		return ERR_PTR(-ENOENT);
> +
> +	if (!cxl_feature_enabled(cfs, cmd->opcode))
> +		return ERR_PTR(-EPERM);
> +
> +	switch (cmd->opcode) {
> +	case CXL_MBOX_OP_GET_SUPPORTED_FEATURES:
> +	case CXL_MBOX_OP_GET_FEATURE:
> +		if (scope >= FWCTL_RPC_CONFIGURATION)
> +			return cmd;
> +		break;
> +	case CXL_MBOX_OP_SET_FEATURE:
> +		if (cxlctl_validate_set_features(cfs, rpc_in, scope))
> +			return cmd;
> +		break;

return ERR_PTR(-EINVAL); here perhaps.

> +	default:
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return ERR_PTR(-EINVAL);
Maybe better to return in the error cases above to make it clear locally
to them that they are errors.

> +}

> diff --git a/include/uapi/cxl/features.h b/include/uapi/cxl/features.h
> index 18b74252058a..55fa5b2a5d17 100644
> --- a/include/uapi/cxl/features.h
> +++ b/include/uapi/cxl/features.h
> @@ -14,6 +14,19 @@
>  #include <linux/uuid.h>
>  #endif
>  
> +/* CXL spec r3.2 Table 8-87 command effects */
> +#define CXL_CMD_CONFIG_CHANGE_COLD_RESET	BIT(0)

These are duplicated with those in include/cxl/features.
Can we avoid that?


> +#define CXL_CMD_CONFIG_CHANGE_IMMEDIATE		BIT(1)
> +#define CXL_CMD_DATA_CHANGE_IMMEDIATE		BIT(2)
> +#define CXL_CMD_POLICY_CHANGE_IMMEDIATE		BIT(3)
> +#define CXL_CMD_LOG_CHANGE_IMMEDIATE		BIT(4)
> +#define CXL_CMD_SECURITY_STATE_CHANGE		BIT(5)
> +#define CXL_CMD_BACKGROUND			BIT(6)
> +#define CXL_CMD_BGCMD_ABORT_SUPPORTED		BIT(7)
> +#define CXL_CMD_EFFECTS_EXTEND			BIT(9)
> +#define CXL_CMD_CONFIG_CHANGE_CONV_RESET	BIT(10)
> +#define CXL_CMD_CONFIG_CHANGE_CXL_RESET		BIT(11)
> +
>  /* Get Supported Features (0x500h) CXL r3.1 8.2.9.6.1 */
>  struct cxl_mbox_get_sup_feats_in {
>  	__le32 count;