Re: [PATCH 12/23] cxl/region: Add region creation ABI

[Dan Williams <dan.j.williams@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.