Re: [PATCH v3 7/8] cxl/memdev: Make inject and clear poison cmds kernel exclusive

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

On Wed,  1 Mar 2023 13:36:32 -0800
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> Inject and clear poison commands are intended to be used in debug
> mode only, and if improperly used, can lead to data corruption. The
> kernel provides a sysfs interface that provides the protection needed
> to issue these commands.[1]
> 
> The CXL driver defines Enabled commands in its ABI.[2] Enabled means
> that the device and the driver both support the command. If a device
> supports inject and/or clear, those commands are flagged Enabled.
> 
> The ABI also defines another command flag: Exclusive. Exclusive
> commands are reserved for kernel use. The exclusive flags can be
> temporal, but for inject and clear, the status is permanent.
> 
> Document the exclusivity of Inject and Clear in the ABI kernel doc.
> (Clean up a typo in kdoc too: 'CXL_MEM_COMMAND_FLAG_ENABLED')
> 
> Create an exclusive commands bitmap in the memdev driver, add the
> inject and clear poison commands, and set it in the cxl_dev_state.
> 
> [1] Documentation/ABI/testing/sysfs-bus-cxl
> [2] include/uapi/linux/cxl_mem.h
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  drivers/cxl/core/memdev.c    |  6 ++++++
>  include/uapi/linux/cxl_mem.h | 21 ++++++++++++++++-----
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index ed3e4517dc3a..f746a0c222b6 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -10,6 +10,8 @@
>  
>  static DECLARE_RWSEM(cxl_memdev_rwsem);
>  
> +static __read_mostly DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX);

Seems like a macro to wrap up the DECLARE_BITMAP and the length
so that we don't have to be careful these are the correct size might be useful.



> +
>  /*
>   * An entire PCI topology full of devices should be enough for any
>   * config
> @@ -571,6 +573,10 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
>  	cxlmd->cxlds = cxlds;
>  	cxlds->cxlmd = cxlmd;
>  
> +	set_bit(CXL_MEM_COMMAND_ID_INJECT_POISON, exclusive_cmds);
> +	set_bit(CXL_MEM_COMMAND_ID_CLEAR_POISON, exclusive_cmds);
> +	set_exclusive_cxl_commands(cxlds, exclusive_cmds);
> +
>  	cdev = &cxlmd->cdev;
>  	rc = cdev_device_add(cdev, dev);
>  	if (rc)
> diff --git a/include/uapi/linux/cxl_mem.h b/include/uapi/linux/cxl_mem.h
> index 86bbacf2a315..6f9ae244f7fd 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -74,17 +74,28 @@ static const struct {
>   * @id: ID number for the command.
>   * @flags: Flags that specify command behavior.
>   *
> - *         CXL_MEM_COMMAND_FLAG_USER_ENABLED
> + *         CXL_MEM_COMMAND_FLAG_ENABLED
>   *
>   *         The given command id is supported by the driver and is supported by
>   *         a related opcode on the device.
>   *
>   *         CXL_MEM_COMMAND_FLAG_EXCLUSIVE
>   *
> - *         Requests with the given command id will terminate with EBUSY as the
> - *         kernel actively owns management of the given resource. For example,
> - *         the label-storage-area can not be written while the kernel is
> - *         actively managing that space.
> + *	   The given command id is for kernel exclusive use and is not
I'm guessing tabs and space differences to the above?

Other than those little things LGTM

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

> + *	   available to userspace. Requests will terminate with EBUSY.
> + *
> + *	   The exclusive flag may be temporal, and only set while the
> + *	   kernel actively owns management of the given resource. For
> + *	   example, the label-storage-area can not be written while the
> + *	   kernel is actively managing that space.
> + *
> + *	   The exclusive flag can be permanent, as in commands that can
> + *	   never be issued through the ioctl interface.
> + *
> + *	   INJECT_POISON and CLEAR_POISON are permanently kernel exclusive.
> + *	   They are supported through a sysfs interface that validates the
> + *	   safety of each command based on the state of the memdev.
> + *	   See: Documentation/ABI/testing/sysfs-bus-cxl
>   *
>   * @size_in: Expected input size, or ~0 if variable length.
>   * @size_out: Expected output size, or ~0 if variable length.