Re: [RFC PATCH 4/4] cxl/region: Introduce concept of region configuration

From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Ben Widawsky <ben.widawsky@intel.com>
Cc: <linux-cxl@vger.kernel.org>,
	Alison Schofield <alison.schofield@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	"Ira Weiny" <ira.weiny@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>
Subject: Re: [RFC PATCH 4/4] cxl/region: Introduce concept of region configuration
Date: Mon, 14 Jun 2021 17:20:55 +0100	[thread overview]
Message-ID: <20210614172055.00007ae3@Huawei.com> (raw)
In-Reply-To: <20210614161851.iipfwo5dvxkht7yw@intel.com>

On Mon, 14 Jun 2021 09:18:51 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> On 21-06-11 14:52:06, Jonathan Cameron wrote:
> > On Thu, 10 Jun 2021 11:57:25 -0700
> > Ben Widawsky <ben.widawsky@intel.com> wrote:
> >   
> > > The region creation APIs leave a region unconfigured. Configuring the
> > > region will work in the same way as similar subsystems such as devdax.
> > > Sysfs attrs will be provided to allow userspace to configure the region.
> > > Finally once all configuration is complete, userspace may "commit" the
> > > config. What the kernel decides to do after a config is committed is out
> > > of scope at this point.
> > > 
> > > Introduced here are the most basic attributes needed to configure a
> > > region.
> > > 
> > > A x1 interleave example is provided below:
> > > 
> > > decoder1.0
> > > ├── create_region
> > > ├── delete_region
> > > ├── devtype
> > > ├── locked
> > > ├── region1.0:0
> > > │   ├── offset
> > > │   ├── size
> > > │   ├── subsystem -> ../../../../../../../bus/cxl
> > > │   ├── target0
> > > │   ├── uevent
> > > │   ├── uuid
> > > │   └── verify
> > > ├── size
> > > ├── start
> > > ├── subsystem -> ../../../../../../bus/cxl
> > > ├── target_list
> > > ├── target_type
> > > └── uevent
> > > 
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-cxl |  27 +++
> > >  drivers/cxl/mem.h                       |   2 +
> > >  drivers/cxl/region.c                    | 227 +++++++++++++++++++++++-
> > >  3 files changed, 255 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> > > index 699c8514fd7b..d7174a84f70d 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-cxl
> > > +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> > > @@ -159,3 +159,30 @@ Description:
> > >  		integer is returned describing the first error found in the
> > >  		configuration. A verified region can still fail binding due to
> > >  		lack of resources.
> > > +
> > > +What:		/sys/bus/cxl/devices/decoderX.Y/regionX.Y:Z/{offset,size}  
> > 
> > I missed this before, but why do we need X.Y in the region naming given it's
> > always inside decoderX.Y.  Seems like RegionZ would work.
> >   
> 
> Yes. There are two options, either we do X.Y:Z, or Z where Z is globally unique
> across all decoders. The reason for this is the devices are symlinked into sysfs
> and so you can't have the same Z for multiple regions in different decoders.
> 
> My preference is to have regionX.y:[0-n] for each decoder, rather than the globally
> unique. I'll entertain an argument the other way though.

I'm happy with either approach.

Thanks,

Jonathan