Re: [NDCTL PATCH v3 3/3] ndctl: cxl: add QoS class check for CXL region creation

From: Alison Schofield <alison.schofield@intel.com>
To: Dave Jiang <dave.jiang@intel.com>
Cc: linux-cxl@vger.kernel.org, nvdimm@lists.linux.dev,
	vishal.l.verma@intel.com
Subject: Re: [NDCTL PATCH v3 3/3] ndctl: cxl: add QoS class check for CXL region creation
Date: Fri, 26 Jan 2024 10:14:23 -0800	[thread overview]
Message-ID: <ZbP2f31s09WRzWb1@aschofie-mobl2> (raw)
In-Reply-To: <170612969410.2745924.538640158518770317.stgit@djiang5-mobl3>

On Wed, Jan 24, 2024 at 01:54:54PM -0700, Dave Jiang wrote:
> The CFMWS provides a QTG ID. The kernel driver creates a root decoder that
> represents the CFMWS. A qos_class attribute is exported via sysfs for the root
> decoder.
> 
> One or more QoS class tokens are retrieved via QTG ID _DSM from the ACPI0017
> device for a CXL memory device. The input for the _DSM is the read and write
> latency and bandwidth for the path between the device and the CPU. The
> numbers are constructed by the kernel driver for the _DSM input. When a
> device is probed, QoS class tokens  are retrieved. This is useful for a
> hot-plugged CXL memory device that does not have regions created.
> 
> Add a check for config check during region creation. Emit a warning if the

Maybe "Add a QoS check during region creation."

> QoS class token from the root decoder is different than the mem device QoS
> class token. User parameter options are provided to fail instead of just
> warning.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v3:
> - Rebase to pending branch
> ---
>  Documentation/cxl/cxl-create-region.txt |    9 ++++
>  cxl/region.c                            |   67 +++++++++++++++++++++++++++++++
>  2 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/cxl/cxl-create-region.txt b/Documentation/cxl/cxl-create-region.txt
> index f11a412bddfe..9ab2e0fee152 100644
> --- a/Documentation/cxl/cxl-create-region.txt
> +++ b/Documentation/cxl/cxl-create-region.txt
> @@ -105,6 +105,15 @@ include::bus-option.txt[]
>  	supplied, the first cross-host bridge (if available), decoder that
>  	supports the largest interleave will be chosen.
>  
> +-e::
> +--strict::
> +	Enforce strict execution where any potential error will force failure.
> +	For example, if QTG ID mismatches will cause failure.

The definition of this 'Enforce ...' sounds very broad like it's
going to be used for more that this QTG ID check. Is it?  Maybe I
missed some earlier reviews and that is intentional.

I was expecting it to say something like: Enforce strict QTG ID matching.
Fail to create region on any mismatch.

> +
> +-q::
> +--no-enforce-qtg::
> +	Parameter to bypass QTG ID mismatch failure. Will only emit warning.
> +
>  include::human-option.txt[]
>  
>  include::debug-option.txt[]

snip

> 