Re: [PATCH v5 06/12] cxl/memdev: Make inject and clear poison cmds kernel exclusive

From: Dave Jiang <dave.jiang@intel.com>
To: alison.schofield@intel.com,
	Dan Williams <dan.j.williams@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	Ben Widawsky <bwidawsk@kernel.org>
Cc: linux-cxl@vger.kernel.org,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>
Subject: Re: [PATCH v5 06/12] cxl/memdev: Make inject and clear poison cmds kernel exclusive
Date: Fri, 31 Mar 2023 12:10:16 -0700	[thread overview]
Message-ID: <1576040e-e8db-bc78-2fa3-622c8f7da8ec@intel.com> (raw)
In-Reply-To: <d88366e1b749c0100f2480f0036117667b62bc95.1679892337.git.alison.schofield@intel.com>


On 3/26/23 10:03 PM, 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 debugfs interface 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/debugfs-cxl
> [2] include/uapi/linux/cxl_mem.h
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/cxl/core/memdev.c    |  6 ++++++
>   include/uapi/linux/cxl_mem.h | 20 +++++++++++++++-----
>   2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 71ebe3795616..617d8378ca9a 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -11,6 +11,8 @@
>   
>   static DECLARE_RWSEM(cxl_memdev_rwsem);
>   
> +static __read_mostly DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX);
> +
>   /*
>    * An entire PCI topology full of devices should be enough for any
>    * config
> @@ -628,6 +630,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..6294278f9dcb 100644
> --- a/include/uapi/linux/cxl_mem.h
> +++ b/include/uapi/linux/cxl_mem.h
> @@ -74,17 +74,27 @@ 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
> + *         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,
> + *         and are supported through a debugfs interface.
> + *         See: Documentation/ABI/testing/debugfs-cxl
>    *
>    * @size_in: Expected input size, or ~0 if variable length.
>    * @size_out: Expected output size, or ~0 if variable length.