linux-kernel - Re: [PATCH RFC v2 03/18] cxl/mem: Read Dynamic capacity configuration from the device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <64fba59f73319_1e8e78294e2@iweiny-mobl.notmuch>
Date:   Fri, 8 Sep 2023 15:52:15 -0700
From:   Ira Weiny <ira.weiny@...el.com>
To:     Fan Ni <fan.ni@....us>, <ira.weiny@...el.com>
CC:     Dan Williams <dan.j.williams@...el.com>,
        Navneet Singh <navneet.singh@...el.com>,
        Fan Ni <fan.ni@...sung.com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Davidlohr Bueso <dave@...olabs.net>,
        "Dave Jiang" <dave.jiang@...el.com>,
        Alison Schofield <alison.schofield@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC v2 03/18] cxl/mem: Read Dynamic capacity
 configuration from the device

Fan Ni wrote:
> On Mon, Aug 28, 2023 at 10:20:54PM -0700, ira.weiny@...el.com wrote:
> > From: Navneet Singh <navneet.singh@...el.com>
> >

[snip]

> >
> > +static int cxl_dc_save_region_info(struct cxl_memdev_state *mds, int index,
> > +				   struct cxl_dc_region_config *region_config)
> > +{
> > +	struct cxl_dc_region_info *dcr = &mds->dc_region[index];
> > +	struct device *dev = mds->cxlds.dev;
> > +
> > +	dcr->base = le64_to_cpu(region_config->region_base);
> > +	dcr->decode_len = le64_to_cpu(region_config->region_decode_length);
> > +	dcr->decode_len *= CXL_CAPACITY_MULTIPLIER;
> > +	dcr->len = le64_to_cpu(region_config->region_length);
> > +	dcr->blk_size = le64_to_cpu(region_config->region_block_size);
> > +	dcr->dsmad_handle = le32_to_cpu(region_config->region_dsmad_handle);
> > +	dcr->flags = region_config->flags;
> > +	snprintf(dcr->name, CXL_DC_REGION_STRLEN, "dc%d", index);
> > +
> > +	/* Check regions are in increasing DPA order */
> > +	if (index > 0) {
> > +		struct cxl_dc_region_info *prev_dcr = &mds->dc_region[index - 1];
> > +
> > +		if ((prev_dcr->base + prev_dcr->decode_len) > dcr->base) {
> > +			dev_err(dev,
> > +				"DPA ordering violation for DC region %d and %d\n",
> > +				index - 1, index);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	/* Check the region is 256 MB aligned */
> > +	if (!IS_ALIGNED(dcr->base, SZ_256M)) {
> > +		dev_err(dev, "DC region %d not aligned to 256MB: %#llx\n",
> > +			index, dcr->base);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Check Region base and length are aligned to block size */
> > +	if (!IS_ALIGNED(dcr->base, dcr->blk_size) ||
> > +	    !IS_ALIGNED(dcr->len, dcr->blk_size)) {
> > +		dev_err(dev, "DC region %d not aligned to %#llx\n", index,
> > +			dcr->blk_size);
> > +		return -EINVAL;
> > +	}
> 
> Based on on cxl 3.0 spec: Table 8-126, we may need some extra checks
> here:
> 1. region len <= decode_len
> 2. region block size should be power of 2 and a multiple of 40H.

Thanks for pointing these additional checks out!  I've added these.

> 
> Also, if region len or block size is 0, it mentions that DC will not be
> available, we may also need to handle that.

I've just added checks for 0 in region length, length and block size.

I don't think we need to handle this in any special way.  Any of these
checks will fail the device probe.  From my interpretation of the spec
reading these values as 0 would indicate an invalid device configuration.

That said I think the spec is a bit vague here.  On the one hand the
number of DC regions should reflect the number of valid regions.

Table 8-125 'Number of Available Regions':
	"This is the number of valid region configurations returned in
	this payload."

But it also says:
	"Each region may be unconfigured or configured with a different
	block size and capacity."

I don't believe that a 0 in the Region Decode Length, Region Length, or
Region Block Size is going to happen with the code structured the way it
is.  I believe these values are used if the host specifically requests the
configuration of a region not indicated by 'Number of Available Regions'
through the Starting Region Index in Table 8-163.  This code does not do
that.

Would you agree with this?

Thanks again,
Ira