On Mon, 24 Jun 2024 19:47:31 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> From: Saeed Mahameed <saeedm@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
A few minor comments + a reference counting question.
> diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig
> index 37147a695add9a..e5ee2d46d43126 100644
> --- a/drivers/fwctl/Kconfig
> +++ b/drivers/fwctl/Kconfig
> @@ -7,3 +7,17 @@ menuconfig FWCTL
> support a wide range of lockdown compatible device behaviors including
> manipulating device FLASH, debugging, and other activities that don't
> fit neatly into an existing subsystem.
> +
> +if FWCTL
Why not use depends on FWCTL?
> +config FWCTL_MLX5
> + tristate "mlx5 ConnectX control fwctl driver"
> + depends on MLX5_CORE
> + help
> + MLX5CTL provides interface for the user process to access the debug and
> + configuration registers of the ConnectX hardware family
> + (NICs, PCI switches and SmartNIC SoCs).
> + This will allow configuration and debug tools to work out of the box on
> + mainstream kernel.
> +
> + If you don't know what to do here, say N.
> +endif
> diff --git a/drivers/fwctl/mlx5/main.c b/drivers/fwctl/mlx5/main.c
> new file mode 100644
> index 00000000000000..5e64371d7e5508
> --- /dev/null
> +++ b/drivers/fwctl/mlx5/main.c
> +static void mlx5ctl_remove(struct auxiliary_device *adev)
> +{
> + struct mlx5ctl_dev *mcdev __free(mlx5ctl) = auxiliary_get_drvdata(adev);
So this is calling fwctl_put(&mcdev->fwctl) on scope exit.
Why do you need to drop a reference beyond the one fwctl_unregister() is dropping
in cdev_device_del()? Where am I missing a reference get?
> +
> + fwctl_unregister(&mcdev->fwctl);
> +}
> +
> +static const struct auxiliary_device_id mlx5ctl_id_table[] = {
> + {.name = MLX5_ADEV_NAME ".fwctl",},
> + {},
No point in comma after terminating entries
> +};
> +MODULE_DEVICE_TABLE(auxiliary, mlx5ctl_id_table);
> +
> +static struct auxiliary_driver mlx5ctl_driver = {
> + .name = "mlx5_fwctl",
> + .probe = mlx5ctl_probe,
> + .remove = mlx5ctl_remove,
> + .id_table = mlx5ctl_id_table,
> +};
> +
> +module_auxiliary_driver(mlx5ctl_driver);
> +
> +MODULE_IMPORT_NS(FWCTL);
> +MODULE_DESCRIPTION("mlx5 ConnectX fwctl driver");
> +MODULE_AUTHOR("Saeed Mahameed <saeedm@xxxxxxxxxx>");
> +MODULE_LICENSE("Dual BSD/GPL");
> +#endif
