Re: [PATCH v3 07/10] fwctl/mlx5: Support for communicating with mlx5 fw

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

On Wed, 21 Aug 2024 15:10:59 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> From: Saeed Mahameed <saeedm@nvidia.com>
> 
> mlx5's fw has long provided a User Context concept. This has a long
> history in RDMA as part of the devx extended verbs programming
> interface. A User Context is a security envelope that contains objects and
> controls access. It contains the Protection Domain object from the
> InfiniBand Architecture and both togther provide the OS with the necessary
> tools to bind a security context like a process to the device.
> 
> The security context is restricted to not be able to touch the kernel or
> other processes. In the RDMA verbs case it is also restricted to not touch
> global device resources.
> 
> The fwctl_mlx5 takes this approach and builds a User Context per fwctl
> file descriptor and uses a FW security capability on the User Context to
> enable access to global device resources. This makes the context useful
> for provisioning and debugging the global device state.
> 
> mlx5 already has a robust infrastructure for delivering RPC messages to
> fw. Trivially connect fwctl's RPC mechanism to mlx5_cmd_do(). Enforce the
> User Context ID in every RPC header so the FW knows the security context
> of the issuing ID.
> 
> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Trivial stuff only. Feel free to ignore if you really like the code
the way it is.  I don't know the MLX5 parts, but based on just what
is visible here and in this series.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> diff --git a/drivers/fwctl/mlx5/main.c b/drivers/fwctl/mlx5/main.c
> new file mode 100644
> index 00000000000000..8839770fbe7ba5
> --- /dev/null
> +++ b/drivers/fwctl/mlx5/main.c


> +
> +static void *mlx5ctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
> +			    void *rpc_in, size_t in_len, size_t *out_len)
> +{
> +	struct mlx5ctl_dev *mcdev =
> +		container_of(uctx->fwctl, struct mlx5ctl_dev, fwctl);
> +	struct mlx5ctl_uctx *mfd =
> +		container_of(uctx, struct mlx5ctl_uctx, uctx);
> +	void *rpc_alloc __free(kvfree) = NULL;

Whilst you can't completely pair this with destructor, I'd still
move this as locally as possible.

> +	void *rpc_out;
> +	int ret;
> +
> +	if (in_len < MLX5_ST_SZ_BYTES(mbox_in_hdr) ||
> +	    *out_len < MLX5_ST_SZ_BYTES(mbox_out_hdr))
> +		return ERR_PTR(-EMSGSIZE);
> +
> +	/* FIXME: Requires device support for more scopes */
> +	if (scope != FWCTL_RPC_CONFIGURATION &&
> +	    scope != FWCTL_RPC_DEBUG_READ_ONLY)
> +		return ERR_PTR(-EOPNOTSUPP);
> +
> +	mlx5ctl_dbg(mcdev, "[UID %d] cmdif: opcode 0x%x inlen %zu outlen %zu\n",
> +		    mfd->uctx_uid, MLX5_GET(mbox_in_hdr, rpc_in, opcode),
> +		    in_len, *out_len);
> +
> +	if (!mlx5ctl_validate_rpc(rpc_in, scope))
> +		return ERR_PTR(-EBADMSG);
> +
> +	/*
> +	 * mlx5_cmd_do() copies the input message to its own buffer before
> +	 * executing it, so we can reuse the allocation for the output.
> +	 */
> +	if (*out_len <= in_len) {
> +		rpc_out = rpc_in;
> +	} else {
> +		rpc_out = rpc_alloc = kvzalloc(*out_len, GFP_KERNEL);
> +		if (!rpc_alloc)
> +			return ERR_PTR(-ENOMEM);
> +	}
> +
> +	/* Enforce the user context for the command */
> +	MLX5_SET(mbox_in_hdr, rpc_in, uid, mfd->uctx_uid);
> +	ret = mlx5_cmd_do(mcdev->mdev, rpc_in, in_len, rpc_out, *out_len);
> +
> +	mlx5ctl_dbg(mcdev,
> +		    "[UID %d] cmdif: opcode 0x%x status 0x%x retval %pe\n",
> +		    mfd->uctx_uid, MLX5_GET(mbox_in_hdr, rpc_in, opcode),
> +		    MLX5_GET(mbox_out_hdr, rpc_out, status), ERR_PTR(ret));
> +
> +	/*
> +	 * -EREMOTEIO means execution succeeded and the out is valid,
> +	 * but an error code was returned inside out. Everything else
> +	 * means the RPC did not make it to the device.
> +	 */
> +	if (ret && ret != -EREMOTEIO)
> +		return ERR_PTR(ret);
> +	if (rpc_out == rpc_in)
> +		return rpc_in;
> +	return_ptr(rpc_alloc);
> +}
> +

> +static void mlx5ctl_remove(struct auxiliary_device *adev)
> +{
> +	struct mlx5ctl_dev *mcdev __free(mlx5ctl) = auxiliary_get_drvdata(adev);

I'm not keen on the non constructor being paired with destructor
but it's your code so you get keep the confusion if you really
like it.

I'd just have an explicit put.

> +
> +	fwctl_unregister(&mcdev->fwctl);
> +}