Re: [RFC PATCH 0/4] Region Creation

From: Ben Widawsky <ben.widawsky@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.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 0/4] Region Creation
Date: Fri, 11 Jun 2021 09:12:11 -0700	[thread overview]
Message-ID: <20210611161211.nte5a2an76d7ahrs@intel.com> (raw)
In-Reply-To: <20210611145331.0000366e@Huawei.com>

On 21-06-11 14:53:31, Jonathan Cameron wrote:
> On Fri, 11 Jun 2021 14:11:36 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Thu, 10 Jun 2021 11:57:21 -0700
> > 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.  
> > 
> > A specific section reference would be helpful.
> > 
> > > 
> > > 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.  
> > 
> > I'm guessing this is why it's an RFC rather than a final submission?
> > 
> > If you can call out the RFC reasons in a cover letter it is helpful
> > as saves people wondering what specifically you want comments on.
> 
> Hi Ben,
> 
> Having read through them all, I think this needs more thought than
> I feel up to on a Friday afternoon.  Will get back to you on v2
> perhaps.
> 
> Jonathan
> 

Thanks for looking. Totally fair too :-)

I'm mainly looking for feedback on the region creation and configuration from
ABI perspective. Nitty gritty code review can happen with the v1 submission.

> > 
> > > 
> > > Here is a sample topology (also in patch #4)
> > > 
> > >     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
> > > 
> > > Ben Widawsky (4):
> > >   cxl/region: Add region creation ABI
> > >   cxl/region: Create attribute structure / verify
> > >   cxl: Move cxl_memdev conversion helper to mem.h
> > >   cxl/region: Introduce concept of region configuration
> > > 
> > >  Documentation/ABI/testing/sysfs-bus-cxl       |  59 +++
> > >  .../driver-api/cxl/memory-devices.rst         |   8 +
> > >  drivers/cxl/Makefile                          |   2 +-
> > >  drivers/cxl/core.c                            |  71 ++++
> > >  drivers/cxl/cxl.h                             |  11 +
> > >  drivers/cxl/mem.h                             |  26 ++
> > >  drivers/cxl/pci.c                             |   5 -
> > >  drivers/cxl/region.c                          | 400 ++++++++++++++++++
> > >  8 files changed, 576 insertions(+), 6 deletions(-)
> > >  create mode 100644 drivers/cxl/region.c
> > >   
> > 
> 