Re: [PATCH v2] cxl/acpi: Warn on mixed CXL VH and RCH/RCD Hierarchy

[Fabio M. De Francesco]

On Tuesday, July 2, 2024 12:17:25 AM GMT+2 Alison Schofield wrote:
> On Fri, Jun 28, 2024 at 07:48:07PM +0200, Fabio M. De Francesco wrote:
> > Each Host Bridge instance has a corresponding CXL Host Bridge Structure
> > (CHBS) ACPI table that identifies its capabilities. CHBS tables can be
> > two types (CXL 3.1 Table 9-21): The PCIe Root Complex Register Block
> > (RCRB) and CXL Host Bridge Component Registers (CHBCR).
> > 
> > If a Host Bridge is attached to a device that is operating in Restricted
> > CXL Device Mode (RCD), BIOS publishes an RCRB with the base address of
> > registers that describe its capabilities (CXL 3.1 sec. 9.11).
> > 
> > Instead, the new (CXL 2.0+) Component registers can only be accessed
> > by means of a base address published with a CHBCR (CXL 3.1 sec. 9.12).
> > 
> > If an eRCD (a device that forces the host-bridge into CXL 1.1 Restricted
> > CXL Host mode) is attached to a CXL 2.0+ Host-Bridge, the current CXL
> > specification does not define a mechanism for finding CXL-2.0-only
> > root-port component registers like HDM decoders and Extended Security
> > capability.
> > 
> > An algorithm to locate a CHBCR associated with an RCRB, would be too
> > invasive to land without some concrete motivation.
> > 
> > Therefore, just print a message to inform of unsupported config.
> > 
> > Count how many different CHBS "Version" types are detected by
> > cxl_get_chbs_iter(). Then make cxl_get_chbs() print a warning if that sum
> > is greater than 1.
> > 
> > Tested-by: Alison Schofield <alison.schofield@xxxxxxxxx>
> > Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@xxxxxxxxxxxxxxx>
> > ---
> > 
> > --- Changes for v2 ---
> > 
> > 	- Rewrite the Subject line (Alison)
> > 	- Address a bug found by Alison while testing (thanks!)
> 
> 'Address' a bug doesn't descibe what changed from v1 especially for
> folks who reviewed v1.
> 
> I'm not asking you to rev this patch, just explain the saved_version
> change.
> 
> Thanks
> -- Alison
>

Alison,

Thanks again for testing, and also for noticing that I didn't give any 
explanation of the new saved_version variable and the solution I introduced to 
fix the bug you found. Then...

During tests, Alison noticed that the CHBS iterator was testing chbs-
>cxl_version always against UINT_MAX.  

ctx->cxl_version must be assigned, along with ctx->base, before returning from 
the CHBS iterator, only if all 'if' statement evaluate false.

Therefore, don't test anymore against ctx->cxl_version. Instead save every new 
different chbs->cxl_version in ctx->saved_version and test against this new 
field at every iteration. 

Add an inline comment that briefly (and indirectly) refers to this logic. 

Fabio 
 
> > 	- Add reference to CXL 3.1 Spec. (Alison)
> > 	- Extend the commit messages by borrowing comments to v1 (Dan)
> > 	- Rename field "count" to "nr_versions" (Alison)
> > 	- Add brackets to oneline 'if' statement in precence of comments
> > 	  (Dan)
> > 
> > --- Link to v1 ---
> > 
> > https://lore.kernel.org/linux-cxl/20240619125949.167936-1-fabio.m.de.francesco@xxxxxxxxxxxxxxx/
> > 
> >  drivers/cxl/acpi.c | 34 +++++++++++++++++++++++++++-------
> >  1 file changed, 27 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index 571069863c62..f9035dbabb1c 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -482,6 +482,8 @@ struct cxl_chbs_context {
> >  	unsigned long long uid;
> >  	resource_size_t base;
> >  	u32 cxl_version;
> > +	int nr_versions;
> > +	u32 saved_version;
> >  };
> >  
> >  static int cxl_get_chbs_iter(union acpi_subtable_headers *header, void 
*arg,
> > @@ -490,22 +492,31 @@ static int cxl_get_chbs_iter(union 
acpi_subtable_headers *header, void *arg,
> >  	struct cxl_chbs_context *ctx = arg;
> >  	struct acpi_cedt_chbs *chbs;
> >  
> > -	if (ctx->base != CXL_RESOURCE_NONE)
> > -		return 0;
> > -
> >  	chbs = (struct acpi_cedt_chbs *) header;
> >  
> > -	if (ctx->uid != chbs->uid)
> > +	if (chbs->cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11 &&
> > +	    chbs->length != CXL_RCRB_SIZE)
> >  		return 0;
> >  
> > -	ctx->cxl_version = chbs->cxl_version;
> >  	if (!chbs->base)
> >  		return 0;
> >  
> > -	if (chbs->cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11 &&
> > -	    chbs->length != CXL_RCRB_SIZE)
> > +	if (ctx->saved_version != chbs->cxl_version) {
> > +		/*
> > +		 * cxl_version cannot be overwritten before the next 
two
> > +		 * checks, then use saved_version
> > +		 */
> > +		ctx->saved_version = chbs->cxl_version;
> > +		ctx->nr_versions++;
> > +	}
> > +
> > +	if (ctx->base != CXL_RESOURCE_NONE)
> > +		return 0;
> > +
> > +	if (ctx->uid != chbs->uid)
> >  		return 0;
> >  
> > +	ctx->cxl_version = chbs->cxl_version;
> >  	ctx->base = chbs->base;
> >  
> >  	return 0;
> > @@ -529,10 +540,19 @@ static int cxl_get_chbs(struct device *dev, struct 
acpi_device *hb,
> >  		.uid = uid,
> >  		.base = CXL_RESOURCE_NONE,
> >  		.cxl_version = UINT_MAX,
> > +		.saved_version = UINT_MAX,
> >  	};
> >  
> >  	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs_iter, 
ctx);
> >  
> > +	if (ctx->nr_versions > 1) {
> > +		/*
> > +		 * Disclaim eRCD support given some component register 
may
> > +		 * only be found via CHBCR
> > +		 */
> > +		dev_info(dev, "Unsupported platform config, mixed 
Virtual Host and Restricted CXL Host hierarchy.");
> > +	}
> > +
> >  	return 0;
> >  }
> >  
>