Re: [PATCH v6 08/12] cxl/acpi: Extract component registers of restricted hosts from RCRB

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 01 Dec 2022 13:34:05 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> From: Robert Richter <rrichter@amd.com>
> 
> 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). The
> RCRB base address is cached in the cxl_dport per-host bridge so that the
> upstream port component registers can be retrieved later by an RCD
> (RCIEP) associated with the host bridge.
> 
> 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.
> 
> Co-developed-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Link: https://lore.kernel.org/r/Y4dsGZ24aJlxSfI1@rric.localdomain
> [djbw: introduce devm_cxl_add_rch_dport()]
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Trivial moans that may have something to do with it being near going home time
on a Friday.

Otherwise looks sensible though this was a fairly superficial look.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>



> ---
>  drivers/cxl/acpi.c            |   51 ++++++++++++++++++++++++++++-----
>  drivers/cxl/core/port.c       |   53 ++++++++++++++++++++++++++++++----
>  drivers/cxl/core/regs.c       |   64 +++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h             |   16 ++++++++++
>  tools/testing/cxl/Kbuild      |    1 +
>  tools/testing/cxl/test/cxl.c  |   10 ++++++
>  tools/testing/cxl/test/mock.c |   19 ++++++++++++
>  tools/testing/cxl/test/mock.h |    3 ++
>  8 files changed, 203 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 50d82376097c..db8173f3ee10 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c

>  struct cxl_chbs_context {
> -	struct device *dev;
> -	unsigned long long uid;
> -	resource_size_t chbcr;
> +	struct device		*dev;
> +	unsigned long long	uid;
> +	resource_size_t		rcrb;
> +	resource_size_t		chbcr;
> +	u32			cxl_version;
>  };

I'm not keen on this style change because it slightly obscures the meaningful
changes in this diff + I suspect it's not consistent with rest of the file.



> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index ec178e69b18f..28ed0ec8ee3e 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -307,3 +307,67 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
>  	return -ENODEV;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
> +
> +resource_size_t cxl_rcrb_to_component(struct device *dev,
> +				      resource_size_t rcrb,
> +				      enum cxl_rcrb which)
> +{
> +	resource_size_t component_reg_phys;
> +	u32 bar0, bar1;
> +	void *addr;
> +	u16 cmd;
> +	u32 id;
> +
> +	if (which == CXL_RCRB_UPSTREAM)
> +		rcrb += SZ_4K;
> +
> +	/*
> +	 * 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).
> +	 */
> +	if (!request_mem_region(rcrb, SZ_4K, "CXL RCRB"))
> +		return CXL_RESOURCE_NONE;
> +	addr = ioremap(rcrb, SZ_4K);
> +	if (!addr) {
> +		dev_err(dev, "Failed to map region %pr\n", addr);
> +		release_mem_region(rcrb, SZ_4K);
> +		return CXL_RESOURCE_NONE;
> +	}
> +
> +	id = readl(addr + PCI_VENDOR_ID);
> +	cmd = readw(addr + PCI_COMMAND);
> +	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> +	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> +	iounmap(addr);
> +	release_mem_region(rcrb, SZ_4K);
> +
> +	/*
> +	 * Sanity check, see CXL 3.0 Figure 9-8 CXL Device that Does Not
> +	 * Remap Upstream Port and Component Registers
> +	 */
> +	if (id == U32_MAX) {
> +		if (which == CXL_RCRB_DOWNSTREAM)
> +			dev_err(dev, "Failed to access Downstream Port RCRB\n");
> +		return CXL_RESOURCE_NONE;
> +	}
> +	if (!(cmd & PCI_COMMAND_MEMORY))
> +		return CXL_RESOURCE_NONE;
> +	if (bar0 & (PCI_BASE_ADDRESS_MEM_TYPE_1M | PCI_BASE_ADDRESS_SPACE_IO))

Trivial: A positive match on what we do want might be better...

I had to got look up MEM_TYPE_1M to find out what on earth it was (marked obsolete which
I guess isn't surprising.... )

Up to you though...

> +		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;
> +
> +	/* MEMBAR is block size (64k) aligned. */
> +	if (!IS_ALIGNED(component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE))
> +		return CXL_RESOURCE_NONE;
> +
> +	return component_reg_phys;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 281b1db5a271..1342e4e61537 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h



>  #define CXL_RESOURCE_NONE ((resource_size_t) -1)
>  #define CXL_TARGET_STRLEN 20
>  
> @@ -486,12 +494,16 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
>   * @dport: PCI bridge or firmware device representing the downstream link
>   * @port_id: unique hardware identifier for dport in decoder target list
>   * @component_reg_phys: downstream port component registers
> + * @rcrb: base address for the Root Complex Register Block
> + * @rch: Indicate whether this dport was enumerated in RCH or VH mode

Clarify this as
	Indicate this dport was enumerated in RCH rather than VH mode.

a boolean with an or in the comment is confusing!

>   * @port: reference to cxl_port that contains this downstream port
>   */
>  struct cxl_dport {
>  	struct device *dport;
>  	int port_id;
>  	resource_size_t component_reg_phys;
> +	resource_size_t rcrb;
> +	bool rch;
>  	struct cxl_port *port;
>  };