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

Dave Jiang
2024-11-13T15:41:17Z

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

>
>> +}