Re: [PATCH v2 3/5] cxl: Introduce 'struct cxl_dpa_partition' and 'struct cxl_range_info'

[Ira Weiny <ira.weiny@intel.com>]

Dan Williams wrote:
> Ira Weiny wrote:
> > Dan Williams wrote:
> > 
> > [snip]
> > 
> > > +/* if this fails the caller must destroy @cxlds, there is no recovery */
> > > +int cxl_dpa_setup(struct cxl_dev_state *cxlds, const struct cxl_dpa_info *info)
> > > +{
> > > +	struct device *dev = cxlds->dev;
> > > +
> > > +	guard(rwsem_write)(&cxl_dpa_rwsem);
> > > +
> > > +	if (cxlds->nr_partitions)
> > > +		return -EBUSY;
> > > +
> > > +	if (!info->size || !info->nr_partitions) {
> > > +		cxlds->dpa_res = DEFINE_RES_MEM(0, 0);
> > > +		cxlds->nr_partitions = 0;
> > > +		return 0;
> > > +	}
> > > +
> > > +	cxlds->dpa_res = DEFINE_RES_MEM(0, info->size);
> > > +
> > > +	for (int i = 0; i < info->nr_partitions; i++) {
> > > +		const struct cxl_dpa_part_info *part = &info->part[i];
> > > +		const char *desc;
> > > +		int rc;
> > > +
> > > +		if (part->mode == CXL_PARTMODE_RAM)
> > > +			desc = "ram";
> > > +		else if (part->mode == CXL_PARTMODE_PMEM)
> > > +			desc = "pmem";
> > > +		else
> > > +			desc = "";
> > 
> > This can be a follow on patch but why not allow devices to name their
> > partitions?
> 
> The proposal in patch5 is that the partition resource name is the
> operation mode. See the changes to mode_show().
> 
> So the name is there for the kernel/user ABI to tie the decoder's
> assigned partition to an operation mode.
> 
> Now, what may need to happen is that the partitions and their modes get
> exported in case userspace needs to know that allocating a decoder to
> "dynamic ram" before it allocates a decoder to "ram" implies that future
> attempts to allocate "ram" will fail. That may not need to include the
> actual partition indices, just an ordering of operation modes.

I'm not buying the 'dynamic ram' partition type just yet.  What evidence
do we have that a device will not have 2 dynamic ram partitions?

Further, in all our discussions on the use cases of DCD no one has
mentioned a need for the partition attributes to be communicated to the
user.

I'm starting to think this is better left to the user with the export of
the attributes of each partition to the user.

But that has little to do with this patch now.

> 
> So far we have been saved from needing such a thing as "ram+pmem"
> devices are only an emulation test case, not something where end users
> can get themselves into trouble by doing out-of-order allocations.
> 
> Usually RAM is pre-allocated by BIOS which also limits the possibility
> of the 'skip' code ever being used.
> 
> > > +		cxlds->part[i].perf.qos_class = CXL_QOS_CLASS_INVALID;
> > > +		cxlds->part[i].mode = part->mode;
> > > +		rc = add_dpa_res(dev, &cxlds->dpa_res, &cxlds->part[i].res,
> > > +				 part->range.start, range_len(&part->range),
> > > +				 desc);
> > > +		if (rc)
> > > +			return rc;
> > > +		cxlds->nr_partitions++;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cxl_dpa_setup);
> > > +
> > >  int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
> > >  				resource_size_t base, resource_size_t len,
> > >  				resource_size_t skipped)
> > 
> > [snip]
> > 
> > >  
> > > -static inline struct resource *to_ram_res(struct cxl_dev_state *cxlds)
> > > +
> > > +/* Static RAM is only expected at partition 0. */
> > 
> > Is this because the spec requires RAM first and the partition array must
> > remain in DPA order?
> 
> Yes.
> 
> > This could be in a follow on patch, but unless I'm missing something the
> > partition information must be specified in increasing DPA order.  Perhaps
> > that is accounted for in this series later.
> 
> The partition information is specified in increasing DPA order in these
> patches, so I am missing the concern?

No not missing anything.

Ira