linux-kernel - Re: [PATCH v1 15/29] cxl/region: Use an endpoint's SPA range to find a region

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: <Z6Ml5kirvUK38PQ-@rric.localdomain>
Date: Wed, 5 Feb 2025 09:48:38 +0100
From: Robert Richter <rrichter@....com>
To: Gregory Price <gourry@...rry.net>
Cc: Alison Schofield <alison.schofield@...el.com>,
	Vishal Verma <vishal.l.verma@...el.com>,
	Ira Weiny <ira.weiny@...el.com>,
	Dan Williams <dan.j.williams@...el.com>,
	Jonathan Cameron <jonathan.cameron@...wei.com>,
	Dave Jiang <dave.jiang@...el.com>,
	Davidlohr Bueso <dave@...olabs.net>, linux-cxl@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	"Fabio M. De Francesco" <fabio.m.de.francesco@...ux.intel.com>,
	Terry Bowman <terry.bowman@....com>
Subject: Re: [PATCH v1 15/29] cxl/region: Use an endpoint's SPA range to find
 a region

On 07.01.25 14:14:14, Gregory Price wrote:
> On Tue, Jan 07, 2025 at 03:10:01PM +0100, Robert Richter wrote:
> > To find the correct region and root port of an endpoint of a system
> > needing address translation, the endpoint's HPA range must be
> > translated to each of the parent port address ranges up to the root
> > decoder.
> > 
> > Calculate the SPA range using the newly introduced callback function
> > port->to_hpa() that translates the decoder's HPA range to its parent
> > port's HPA range of the next outer memory domain. Introduce the helper
> > function cxl_port_calc_hpa() for this to calculate address ranges
> > using the low-level port->to_hpa() callbacks. Determine the root port
> > SPA range by iterating all the ports up to the root. Store the
> > endpoint's SPA range and use it to find the endpoint's region.
> >
> 
> Some comments/questions inline.
> 
> > Signed-off-by: Robert Richter <rrichter@....com>
> > ---
> >  drivers/cxl/core/region.c | 85 ++++++++++++++++++++++++++++++++-------
> >  drivers/cxl/cxl.h         |  1 +
> >  2 files changed, 71 insertions(+), 15 deletions(-)
> ... snip ...
> > +	while (1) {
> > +		parent = parent_port_of(iter);
> > +
> 
> I'm always suspicious of unbounded while loops.  Should this simply be
> 
> 	while(iter) {
> 		...
> 	}
> 
> Instead?  Given the rest of the function, I don't think this matters,
> but it's at least a bit clearer.
> 
> > +		if (is_cxl_endpoint(iter))
> > +			cxld = &cxled->cxld;
> > +		else if (!parent || parent->to_hpa)
> > +			cxld = cxl_port_find_switch_decoder(iter, &hpa);
> > +
> > +		if (!cxld) {
> > +			dev_err(cxlmd->dev.parent,
> > +				"%s:%s no CXL window for range %#llx:%#llx\n",
> > +				dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> > +				hpa.start, hpa.end);
> > +			return -ENXIO;
> > +		}
> 
> I also think we should be able to move this check out of the loop on
> various break conditions (iter==NULL, cxld not found, etc).
> 
> > +
> > +		/* No parent means the root port was found. */
> > +		if (!parent)
> > +			break;
> > +
> > +		/* Translate HPA to the next upper memory domain. */
> > +		if (cxl_port_calc_hpa(parent, cxld, &hpa))
> > +			return -ENXIO;

We must check for !parent before and it is clearer then to directly
leave the loop.

That is, using

 while(iter) {} 

does not improve the loop because that would introduce duplicate
checks. It is not possible otherwise to break at the beginning or end
of the loop, as there is code to run, either
cxl_port_find_switch_decoder() or cxl_port_calc_hpa().

> > +
> > +		iter = parent;

> >  	}
> >
> > +	dev_dbg(cxld->dev.parent,
> > +		"%s:%s: range:%#llx-%#llx\n",
> > +		dev_name(&cxled->cxld.dev), dev_name(&cxld->dev),
> > +		hpa.start, hpa.end);
> > +
> >  	cxled->cxlrd = to_cxl_root_decoder(&cxld->dev);
> > +	cxled->spa_range = hpa;
> >  
> >  	return 0;
> >  }
> 
> Overall good, just think we might be able to improve the readability /
> safety of this loop a bit.
> 
> Stupid question: I presume a look in this iteration is not (generally?)
> possible, but if it were to happen this while loop as-is would go infinite.
> Is that something we should worry about it?

It is safe, this kind of iterator is used elsewhere too.

-Robert