Date:   Thu, 20 Oct 2022 22:17:07 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Robert Richter <rrichter@....com>,
        Alison Schofield <alison.schofield@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        "Ira Weiny" <ira.weiny@...el.com>,
        Ben Widawsky <bwidawsk@...nel.org>,
        "Dan Williams" <dan.j.williams@...el.com>
CC:     <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <lenb@...nel.org>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        "Davidlohr Bueso" <dave@...olabs.net>,
        Robert Richter <rrichter@....com>,
        Terry Bowman <terry.bowman@....com>
Subject: RE: [PATCH v2 06/12] cxl/acpi: Extract component registers of
 restricted hosts from RCRB

Robert Richter wrote:
> A downstream port must be connected to a component register block.
> For restricted hosts the base address is determined from the RCRB. The
> RCRB is provided by the host's CEDT CHBS entry. Rework CEDT parser to
> get the RCRB and add code to extract the component register block from
> it.
> 
> RCRB's BAR[0..1] point to the component block containing CXL subsystem
> component registers. MEMBAR extraction follows the PCI base spec here,
> esp. 64 bit extraction and memory range alignment (6.0, 7.5.1.2.1).
> 
> Note: Right now the component register block is used for HDM decoder
> capability only which is optional for RCDs. If unsupported by the RCD,
> the HDM init will fail. It is future work to bypass it in this case.
> 
> Signed-off-by: Terry Bowman <terry.bowman@....com>
> Signed-off-by: Robert Richter <rrichter@....com>
> ---
>  drivers/cxl/acpi.c | 79 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 69 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index fb9f72813067..a92d5d7b7a92 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -9,6 +9,8 @@
>  #include "cxlpci.h"
>  #include "cxl.h"
>  
> +#define CXL_RCRB_SIZE	SZ_8K
> +
>  static unsigned long cfmws_to_decoder_flags(int restrictions)
>  {
>  	unsigned long flags = CXL_DECODER_F_ENABLE;
> @@ -229,27 +231,82 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  struct cxl_chbs_context {
>  	struct device *dev;
>  	unsigned long long uid;
> -	resource_size_t chbcr;
> +	struct acpi_cedt_chbs chbs;
>  };
>  
> -static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
> -			 const unsigned long end)
> +static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
> +			const unsigned long end)
>  {
>  	struct cxl_chbs_context *ctx = arg;
>  	struct acpi_cedt_chbs *chbs;
>  
> -	if (ctx->chbcr)
> +	if (ctx->chbs.base)
>  		return 0;
>  
>  	chbs = (struct acpi_cedt_chbs *) header;
>  
>  	if (ctx->uid != chbs->uid)
>  		return 0;
> -	ctx->chbcr = chbs->base;
> +	ctx->chbs = *chbs;
>  
>  	return 0;
>  }
>  
> +static resource_size_t cxl_get_chbcr(struct cxl_chbs_context *ctx)
> +{

The core logic of this looks good, but this wants to be shared with the
upstream port component register discovery.

Full disclosure I am reconciling these patches with an attempt that Dave
Jiang made at this topic. Since your series hit the list first I will
let it take the lead, but then fill it in with comments and learnings
from Dave's effort.

So in this case Dave moved this into the drivers/cxl/core/regs.c with a
function signature like:

enum cxl_rcrb {
       CXL_RCRB_DOWNSTREAM,
       CXL_RCRB_UPSTREAM,
};

resource_size_t cxl_rcrb_to_component(struct device *dev,
                                      resource_size_t rcrb_base, int len,
                                      enum cxl_rcrb which);

...where @which alternates when called by cxl_acpi for the downstream
case, or cxl_mem for the upstream case.


> +	struct acpi_cedt_chbs *chbs = &ctx->chbs;
> +	resource_size_t component_reg_phys, rcrb;
> +	u32 bar0, bar1;
> +	void *addr;
> +
> +	if (!chbs->base)
> +		return CXL_RESOURCE_NONE;
> +
> +	if (chbs->cxl_version != ACPI_CEDT_CHBS_VERSION_CXL11)
> +		return chbs->base;
> +
> +	/* Extract RCRB */
> +
> +	if (chbs->length != CXL_RCRB_SIZE)
> +		return CXL_RESOURCE_NONE;
> +
> +	rcrb = chbs->base;
> +
> +	dev_dbg(ctx->dev, "RCRB found for UID %lld: 0x%08llx\n",
> +		ctx->uid, (u64)rcrb);
> +
> +	/*
> +	 * RCRB's BAR[0..1] point to component block containing CXL
> +	 * subsystem component registers. MEMBAR extraction follows
> +	 * the PCI Base spec here, esp. 64 bit extraction and memory
> +	 * ranges alignment (6.0, 7.5.1.2.1).
> +	 */
> +	addr = ioremap(rcrb, PCI_BASE_ADDRESS_0 + SZ_8);

No failure check? This also only needs to map 4K at a time.

> +	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> +	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> +	iounmap(addr);
> +
> +	/* sanity check */
> +	if (bar0 & (PCI_BASE_ADDRESS_MEM_TYPE_1M | PCI_BASE_ADDRESS_SPACE_IO))
> +		return CXL_RESOURCE_NONE;
> +
> +	component_reg_phys = bar0 & PCI_BASE_ADDRESS_MEM_MASK;
> +	if (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_64)
> +		component_reg_phys |= ((u64)bar1) << 32;
> +
> +	if (!component_reg_phys)
> +		return CXL_RESOURCE_NONE;
> +
> +	/*
> +	 * Must be 8k aligned (size of combined CXL 1.1 Downstream and
> +	 * Upstream Port RCRBs).
> +	 */
> +	if (component_reg_phys & (CXL_RCRB_SIZE - 1))
> +		return CXL_RESOURCE_NONE;

This is open-coding the IS_ALIGNED() macro. More importantly, why is it
using RCRB size for the component register block alignment? The
component lock is 64K, and at least for CXL 2.0 devices it is 64K
aligned (8.1.9.1 Register Block Offset Low), so I am not sure what this
check is for?

---

Given that there are actual CXL RCH platforms in the wild I want this
topic branch to be the first thing queued for v6.2. To help us
coordinate I pushed:

https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/log/?h=rch

...with the patches from this set accepted so far. You can use that as
the baseline for the next spin.

Powered by blists - more mailing lists