Re: [RFC PATCH 10/13] fwctl/cxl: Add support for fwctl RPC command to enable CXL feature commands

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

On Thu, 18 Jul 2024 14:32:28 -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.

I'll come back to this in reply to the cover letter, but this is
problematic because some of those features will almost certainly be
covered by standard kernel drivers and if Set Feature is enabled
those drivers will need to be hardened against the state mysteriously
changing under them.

This corner is the bit that worries me most about fwctl in general.
Anyhow, one for the cover letter / policy discussion not deep
in the patch series.


> 
> 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_DEBUG_READ_ONLY. The Set Feature call is gated by the effects
> of the feature reported by Get Supported Features call for the specific
> feature.

Break the moves of error codes etc out as a precursor no-op patch to
make review of the real guts of this easier.

Trivial comment inline.
> diff --git a/drivers/fwctl/cxl/cxl.c b/drivers/fwctl/cxl/cxl.c
> index 22f62034c021..01f0771148e1 100644
> --- a/drivers/fwctl/cxl/cxl.c
> +++ b/drivers/fwctl/cxl/cxl.c
> @@ -51,10 +51,133 @@ static void *cxlctl_info(struct fwctl_uctx *uctx, size_t *length)
>  	return info;
>  }

> +static bool cxlctl_validate_hw_cmds(struct cxl_mailbox *cxl_mbox,
> +				    const struct fwctl_cxl_command *send_cmd,
> +				    enum fwctl_rpc_scope scope)
> +{
> +	struct cxl_mem_command *cmd;
> +
> +	/*
> +	 * Only supporting feature commands.
> +	 */
> +	if (!cxl_mbox->num_features)
> +		return false;
> +
> +	cmd = cxl_get_mem_command(send_cmd->id);
> +	if (!cmd)
> +		return false;
> +
> +	if (test_bit(cmd->info.id, cxl_mbox->enabled_cmds))
> +		return false;
> +
> +	if (test_bit(cmd->info.id, cxl_mbox->exclusive_cmds))
> +		return false;
> +
> +	switch (cmd->opcode) {
> +	case CXL_MBOX_OP_GET_SUPPORTED_FEATURES:
> +	case CXL_MBOX_OP_GET_FEATURE:
> +		if (scope >= FWCTL_RPC_DEBUG_READ_ONLY)
> +			return true;
> +		break;

return false here.

> +	case CXL_MBOX_OP_SET_FEATURE:
> +		return cxlctl_validate_set_features(cxl_mbox, send_cmd, scope);
> +	default:
> +		return false;
> +	};
> +
> +	return false;

And drop this.

> +}