| 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
| ||
|
Message-ID: <20230601113811.00006cd5@Huawei.com>
Date: Thu, 1 Jun 2023 11:38:11 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Terry Bowman <terry.bowman@....com>
CC: <alison.schofield@...el.com>, <vishal.l.verma@...el.com>,
<ira.weiny@...el.com>, <bwidawsk@...nel.org>,
<dan.j.williams@...el.com>, <dave.jiang@...el.com>,
<linux-cxl@...r.kernel.org>, <rrichter@....com>,
<linux-kernel@...r.kernel.org>, <bhelgaas@...gle.com>
Subject: Re: [PATCH v4 02/23] cxl/rch: Prepare for caching the MMIO mapped
PCIe AER capability
On Tue, 23 May 2023 18:21:53 -0500
Terry Bowman <terry.bowman@....com> wrote:
> From: Dan Williams <dan.j.williams@...el.com>
>
> Prepare cxl_probe_rcrb() for retrieving more than just the component
> register block. The RCH AER handling code wants to get back to the AER
> capability that happens to be MMIO mapped rather then configuration
> cycles.
>
> Move RCRB specific downstream port data, like the RCRB base and the
> AER capability offset, into its own data structure ('struct
> cxl_rcrb_info') for cxl_probe_rcrb() to fill. Extend 'struct
> cxl_dport' to include a 'struct cxl_rcrb_info' attribute.
>
There are several other refactors going on in here. I'd rather
see it broken down into a few separate patches. See inline.
> This centralizes all RCRB scanning in one routine.
>
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> Co-developed-by: Robert Richter <rrichter@....com>
> Signed-off-by: Robert Richter <rrichter@....com>
> Signed-off-by: Terry Bowman <terry.bowman@....com>
> ---
> drivers/cxl/core/port.c | 7 ++++---
> drivers/cxl/core/regs.c | 10 ++++++----
> drivers/cxl/cxl.h | 19 ++++++++++++-------
> drivers/cxl/mem.c | 16 +++++++++-------
> tools/testing/cxl/Kbuild | 2 +-
> tools/testing/cxl/test/cxl.c | 10 ++++++----
> tools/testing/cxl/test/mock.c | 12 ++++++------
> tools/testing/cxl/test/mock.h | 7 ++++---
> 8 files changed, 48 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 1a3f8729a616..618865ca6a9f 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -939,8 +939,9 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> return ERR_PTR(-ENOMEM);
>
> if (rcrb != CXL_RESOURCE_NONE) {
> - component_reg_phys = cxl_rcrb_to_component(dport_dev,
> - rcrb, CXL_RCRB_DOWNSTREAM);
> + component_reg_phys =
> + cxl_probe_rcrb(dport_dev, rcrb, &dport->rcrb,
> + CXL_RCRB_DOWNSTREAM);
> if (component_reg_phys == CXL_RESOURCE_NONE) {
> dev_warn(dport_dev, "Invalid Component Registers in RCRB");
> return ERR_PTR(-ENXIO);
> @@ -957,7 +958,7 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> dport->port_id = port_id;
> dport->component_reg_phys = component_reg_phys;
> dport->port = port;
> - dport->rcrb = rcrb;
> + dport->rcrb.base = rcrb;
>
> cond_cxl_root_lock(port);
> rc = add_dport(port, dport);
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 1476a0299c9b..08da4c917f99 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -332,9 +332,8 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
> }
> 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 cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
> + struct cxl_rcrb_info *ri, enum cxl_rcrb which)
> {
> resource_size_t component_reg_phys;
> void __iomem *addr;
> @@ -344,6 +343,8 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>
> if (which == CXL_RCRB_UPSTREAM)
> rcrb += SZ_4K;
> + else if (ri)
> + ri->base = rcrb;
I'm struggling a bit to follow flow, but I 'think' you set this to the same
address here and at the end of __devm_cxl_add_dport()
>
> /*
> * RCRB's BAR[0..1] point to component block containing CXL
> @@ -364,6 +365,7 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
> cmd = readw(addr + PCI_COMMAND);
> bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> +
Trivial but I love to moan about these :)
Stray change that shouldn't be in this patch...
> iounmap(addr);
> release_mem_region(rcrb, SZ_4K);
>
> @@ -395,4 +397,4 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>
> return component_reg_phys;
> }
> -EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
> +EXPORT_SYMBOL_NS_GPL(cxl_probe_rcrb, CXL);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index a5cd661face2..29e0bd2b8f2a 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -267,9 +267,9 @@ enum cxl_rcrb {
> CXL_RCRB_DOWNSTREAM,
> CXL_RCRB_UPSTREAM,
> };
> -resource_size_t cxl_rcrb_to_component(struct device *dev,
> - resource_size_t rcrb,
> - enum cxl_rcrb which);
> +struct cxl_rcrb_info;
> +resource_size_t cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
> + struct cxl_rcrb_info *ri, enum cxl_rcrb which);
>
> #define CXL_RESOURCE_NONE ((resource_size_t) -1)
> #define CXL_TARGET_STRLEN 20
> @@ -587,22 +587,27 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
> return xa_load(&port->dports, (unsigned long)dport_dev);
> }
>
> +struct cxl_rcrb_info {
> + resource_size_t base;
> + u16 aer_cap;
> +};
> +
> /**
> * struct cxl_dport - CXL downstream port
> * @dport: PCI bridge or firmware device representing the downstream link
> + * @port: reference to cxl_port that contains this downstream port
> * @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
> - * @port: reference to cxl_port that contains this downstream port
> + * @rcrb: Data about the Root Complex Register Block layout
> */
> struct cxl_dport {
> struct device *dport;
> + struct cxl_port *port;
Why the reorder? It's adding noise we don't need in this patch...
> int port_id;
> resource_size_t component_reg_phys;
> - resource_size_t rcrb;
> bool rch;
> - struct cxl_port *port;
> + struct cxl_rcrb_info rcrb;
> };
>
> /**
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 519edd0eb196..7ecdaa7f9315 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -51,7 +51,6 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
> struct cxl_port *parent_port = parent_dport->port;
> struct cxl_dev_state *cxlds = cxlmd->cxlds;
> struct cxl_port *endpoint, *iter, *down;
> - resource_size_t component_reg_phys;
> int rc;
>
> /*
> @@ -71,12 +70,15 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
> * host-bridge RCRB if they are not already mapped via the
> * typical register locator mechanism.
> */
> - if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE)
> - component_reg_phys = cxl_rcrb_to_component(
> - &cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
> - else
> - component_reg_phys = cxlds->component_reg_phys;
> - endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys,
> + if (parent_dport->rch &&
> + cxlds->component_reg_phys == CXL_RESOURCE_NONE) {
> + cxlds->component_reg_phys =
> + cxl_probe_rcrb(&cxlmd->dev, parent_dport->rcrb.base,
> + NULL, CXL_RCRB_UPSTREAM);
This use of the component_reg_phys pointer in cxlds isn't closely related
to the other changes. This patch would (I think) be more readable
if that change was done in a precursor patch.
> + }
> +
> + endpoint = devm_cxl_add_port(host, &cxlmd->dev,
> + cxlds->component_reg_phys,
> parent_dport);
> if (IS_ERR(endpoint))
> return PTR_ERR(endpoint);
Powered by blists - more mailing lists