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

[Dave Jiang <dave.jiang@intel.com>]

On 7/29/24 4:29 AM, Jonathan Cameron wrote:
> 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.

Do we need to have an exclusive_features mask just like exclusive_cmds to block kernel managed features like RAS? That may be the best way to deal with things happening behind kernel's back.

> 
> 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.

Need to add a documentation patch and maybe we can put policies and rules in there?

> 
> 
>>
>> 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.

ok

> 
> 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.

ok

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

ok

> 
>> +}