linux-cxl.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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: [PATCH 12/23] cxl/region: Add region creation ABI
Date: Thu, 26 Aug 2021 14:44:01 -0700	[thread overview]
Message-ID: <CAPcyv4h2bvbYU-NSADUzXbwX_qjqCYeKDbrvAKmUZtaCYNF17w@mail.gmail.com> (raw)
In-Reply-To: <20210826210138.qf5ck3sdg2ka33p6@intel.com>

On Thu, Aug 26, 2021 at 2:01 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-08-13 19:19:28, Dan Williams wrote:
> > On Fri, Jul 23, 2021 at 2:06 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >
> > > Regions are created as a child of the decoder that encompasses an
> > > address space with constraints. Regions only exist for persistent
> > > capacities.
> >
> > Maybe I am misinterpreting, but regions need to exist for volatile
> > capacities too, otherwise what's the interface for enumerating and
> > reconfiguring volatile interleaves? The only difference is one is
> > recorded in labels.
>
> You mean for hotplugged volatile regions? I thought for static volatile regions,
> BIOS will always set it up and we'll get just a chunk of address space via EFI
> memory map. What would we be reconfiguring?

While the BIOS must set up CXL memory that it advertises as
Conventional Memory to the OS, there is no requirement that the BIOS
sets up *all* volatile memory present at boot. I expect that the BIOS
might do things like setup direct host-bridge attached CXL, or
whatever the OEM expects to be present. I also expect there will be
BIOS implementations that punt on configuring after-market add-on
devices, or ones that are deeper in a switch topology. Regardless
there will be CXL regions that the BIOS sets up that consume CFMWS
space and the driver needs to account for those and provide address
translation. The mechanism for kernel services and resource management
for CXL to operate is to establish CXL regions for them.



> > Perhaps before creating regions we should write the code to enumerate
> > volatile regions that might be present since boot. We'll need that
> > anyway to determine how much CFMWS space is available for dynamic
> > creation. Once happy with the read side then proceed to the write
> > side. Thoughts?
>
> I'm fine either way. I started writing a tool to generate fake labels for this,
> but then we switched course. I think since you sent this email, we've kind of
> decided to do both at the same time.

Yeah, both at the same time works for me.

[..]
> > > +/**
> > > + * struct cxl_region - CXL region
> > > + * @dev: This region's device.
> > > + * @id: This regions id. Id is globally unique across all regions.
> > > + * @res: Address space consumed by this region.
> > > + * @requested_size: Size of the region determined from LSA or userspace.
> > > + * @uuid: The UUID for this region.
> > > + * @list: Node in decoders region list.
> > > + * @eniw: Number of interleave ways this region is configured for.
> > > + * @targets: The memory devices comprising the region.
> > > + */
> > > +struct cxl_region {
> > > +       struct device dev;
> > > +       int id;
> > > +       struct resource *res;
> > > +       u64 requested_size;
> >
> > Why requested_size, and not size? Perhaps this instead should be a
> > pointer to a 'struct cxl_region_label' where the LSA data is cached.
>
> I'd been thinking that regions could be rounded up in size by the driver. It
> could point to a region label too. I hadn't been touching any label creation yet
> because I thought you're bringing that up from the other side. If there's some
> pointer I should use for this, I'm game. Which one?

'struct cxl_region_label' is currently in drivers/nvdimm/label.h it
would need to be moved to common location in include/. I am roughly
expecting that drivers/cxl/ calls into drivers/nvdimm/ to manipulate
the label area.

>
> >
> > I think we'll want struct cxl_volatile_region and struct
> > cxl_persistent_region wrapping a common struct cxl_region.
> >
>
> Reserving the right to change my mind, I think one region struct should be fine,
> it's just one needs to be serialized to the LSA, and the other is ephemeral.

It can be like nvdimm nd_region's where it's one data-structure object
with varying 'struct device_type' types.

  reply	other threads:[~2021-08-26 21:44 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-23 21:06 [PATCH RFCish 00/23] cxl_region and cxl_memdev drivers Ben Widawsky
2021-07-23 21:06 ` [PATCH 01/23] cxl: Move cxl_core to new directory Ben Widawsky
2021-07-23 21:06 ` [PATCH 02/23] cxl/core: Improve CXL core kernel docs Ben Widawsky
2021-07-23 21:06 ` [PATCH 03/23] cxl/core: Extract register and pmem functionality Ben Widawsky
2021-07-23 21:06 ` [PATCH 04/23] cxl/mem: Move character device region creation Ben Widawsky
2021-07-23 21:06 ` [PATCH 05/23] cxl: Pass fops and shutdown to memdev creation Ben Widawsky
2021-07-23 21:06 ` [PATCH 06/23] cxl/core: Move memdev management to core Ben Widawsky
2021-07-23 21:06 ` [PATCH 07/23] cxl/pci: Ignore unknown register block types Ben Widawsky
2021-07-23 21:06 ` [PATCH 08/23] cxl/pci: Simplify register setup Ben Widawsky
2021-07-23 21:06 ` [PATCH 09/23] cxl/pci: Retain map information in cxl_mem_probe Ben Widawsky
2021-07-23 21:06 ` [PATCH 10/23] cxl/decoder: Support parentless decoders Ben Widawsky
2021-07-30 21:03   ` Dan Williams
2021-07-23 21:06 ` [PATCH 11/23] cxl: Enable an endpoint decoder type Ben Widawsky
2021-07-23 21:06 ` [PATCH 12/23] cxl/region: Add region creation ABI Ben Widawsky
2021-08-14  2:19   ` Dan Williams
2021-08-26 21:01     ` Ben Widawsky
2021-08-26 21:44       ` Dan Williams [this message]
2021-07-23 21:06 ` [PATCH 13/23] cxl/region: Introduce concept of region configuration Ben Widawsky
2021-07-23 21:06 ` [PATCH 14/23] cxl: Convert driver id to an enum Ben Widawsky
2021-07-23 21:06 ` [PATCH 15/23] cxl/region: Introduce a cxl_region driver Ben Widawsky
2021-07-23 21:06 ` [PATCH 16/23] cxl/core: Convert decoder range to resource Ben Widawsky
2021-07-23 21:06 ` [PATCH 17/23] cxl/region: Handle region's address space allocation Ben Widawsky
2021-07-23 21:06 ` [PATCH 18/23] cxl/region: Only allow CXL capable targets Ben Widawsky
2021-07-23 21:06 ` [PATCH 19/23] cxl/mem: Introduce CXL mem driver Ben Widawsky
2021-07-23 21:06 ` [PATCH 20/23] cxl/memdev: Determine CXL.mem capability Ben Widawsky
2021-07-23 21:06 ` [PATCH 21/23] cxl/mem: Check that the device is CXL.mem capable Ben Widawsky
2021-07-23 21:06 ` [PATCH 22/23] cxl/mem: Add device as a port Ben Widawsky
2021-07-23 21:06 ` [PATCH 23/23] cxl/core: Map component registers for ports Ben Widawsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPcyv4h2bvbYU-NSADUzXbwX_qjqCYeKDbrvAKmUZtaCYNF17w@mail.gmail.com \
    --to=dan.j.williams@intel.com \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=alison.schofield@intel.com \
    --cc=ben.widawsky@intel.com \
    --cc=ira.weiny@intel.com \
    --cc=linux-cxl@vger.kernel.org \
    --cc=vishal.l.verma@intel.com \
    --subject='Re: [PATCH 12/23] cxl/region: Add region creation ABI' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).