Re: [RFC PATCH 0/4] Region Creation

From: Dan Williams <dan.j.williams@intel.com>
To: Ben Widawsky <ben.widawsky@intel.com>
Cc: linux-cxl@vger.kernel.org,
	Alison Schofield <alison.schofield@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Vishal Verma <vishal.l.verma@intel.com>
Subject: Re: [RFC PATCH 0/4] Region Creation
Date: Fri, 11 Jun 2021 17:44:02 -0700	[thread overview]
Message-ID: <CAPcyv4j=cFikFD_jrPwMfGuMbFZ+1DPUyQjYq7SqTYYauMxLOA@mail.gmail.com> (raw)
In-Reply-To: <20210610185725.897541-1-ben.widawsky@intel.com>

On Thu, Jun 10, 2021 at 11:58 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> CXL interleave sets and non-interleave sets are described via regions. A region
> is specified in the CXL 2.0 specification and the purpose is to create a
> standardized way to preserve the region across reboots.
>
> Introduced here is the basic mechanism to create and configure and delete a CXL
> region. Configuring a region simply means giving it a size, offset within the
> CFMWS window, UUID, and a target list. Enabling/activating a region, which
> ultimately means programming the HDM decoders in the chain, is left for later
> work.
>
> The patches are only minimally tested so far in QEMU emulation and so x1
> interleave is all that's supported.
>
> Here is a sample topology (also in patch #4)

I'm just going to react to the attributes before looking at the
implementation to make sure we're level set.

>
>     decoder1.0
>     ├── create_region
>     ├── delete_region
>     ├── devtype
>     ├── locked
>     ├── region1.0:0
>     │   ├── offset

Is this the region's offset relative to the next available free space
in the parent decoder range? If this is output only I think it's ok,
but I think the address space allocation decision belongs to the
region driver at activation time. I.e. userspace does not have much of
a chance at specifying this relative all the other dynamic operations
that can be happening in the decoder.

>     │   ├── size
>     │   ├── subsystem -> ../../../../../../../bus/cxl
>     │   ├── target0
>     │   ├── uevent
>     │   ├── uuid
>     │   └── verify

I don't understand the role of a standalone @verify attribute, there
is verification that can happen per attribute write, and there is
final verification that can happen at region bind time. Either way
anything verify would check is duplicated somewhere else, and the
verification per attribute update is more precise. For example writes
to @size can check for free space in parent decoder and fail if
unavailable. Writes to targetX can fail if the memdev is not connected
to this decoder's port topology, or the memdev is out of decoder
resources. The final region bind will fail if mid-level switches are
lacking decoder resources, or would require changing a decoder
configuration that is pinned active.