diff mbox series

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

Message ID 20240628175535.272472-1-fabio.m.de.francesco@linux.intel.com
State Accepted
Commit bebfbbaffccfb69126c5a6a1d41cd868b6419370
Headers show
Series [v2] cxl/acpi: Warn on mixed CXL VH and RCH/RCD Hierarchy | expand

Commit Message

Fabio M. De Francesco June 28, 2024, 5:48 p.m. UTC 
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@intel.com>
Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
---

--- Changes for v2 ---

	- Rewrite the Subject line (Alison)
	- Address a bug found by Alison while testing (thanks!)
	- 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@linux.intel.com/

 drivers/cxl/acpi.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

Comments

Davidlohr Bueso July 1, 2024, 3:49 p.m. UTC | #1 
On Fri, 28 Jun 2024, 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@intel.com>
>Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Alison Schofield July 1, 2024, 10:17 p.m. UTC | #2 
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@intel.com>
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> ---
> 
> --- 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

> 	- 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@linux.intel.com/
> 
>  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;
>  }
>  
> -- 
> 2.45.2
>
Fabio M. De Francesco July 2, 2024, 7:14 p.m. UTC | #3 
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@intel.com>
> > Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> > ---
> > 
> > --- 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@linux.intel.com/
> > 
> >  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;
> >  }
> >  
>
Alison Schofield July 2, 2024, 10:59 p.m. UTC | #4 
On Tue, Jul 02, 2024 at 09:14:51PM +0200, Fabio M. De Francesco wrote:
> 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@intel.com>
> > > Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> > > ---
> > > 
> > > --- 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. 
Thanks!

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

>
snip

>
diff mbox series

Patch

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;
 }