| 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
| ||
|
Date: Thu, 20 Jun 2024 13:46:26 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Terry Bowman <terry.bowman@....com>
CC: <dan.j.williams@...el.com>, <ira.weiny@...el.com>, <dave@...olabs.net>,
<dave.jiang@...el.com>, <alison.schofield@...el.com>, <ming4.li@...el.com>,
<vishal.l.verma@...el.com>, <jim.harris@...sung.com>,
<ilpo.jarvinen@...ux.intel.com>, <ardb@...nel.org>,
<sathyanarayanan.kuppuswamy@...ux.intel.com>, <linux-cxl@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <Yazen.Ghannam@....com>,
<Robert.Richter@....com>
Subject: Re: [RFC PATCH 4/9] cxl/pci: Map CXL PCIe ports' RAS registers
On Mon, 17 Jun 2024 15:04:06 -0500
Terry Bowman <terry.bowman@....com> wrote:
> RAS registers are not currently mapped for CXL root ports, CXL downstream
> switch ports, and CXL upstream switch ports. Update the driver to map the
> ports' RAS registers in preparation for RAS logging and handling to be
> added in the future.
>
> Add a 'struct cxl_regs' variable to 'struct cxl_port'. This will be used
> to store a pointer to the upstream port's mapped RAS registers.
>
> Invoke the RAS mapping logic from the CXL memory device probe routine
> after the endpoint is added. This ensures the ports have completed
> training and the RAS registers are present in CXL.cachemem.
>
> Refactor the cxl_dport_map_regs() function to support mapping the CXL
> PCIe ports. Also, check for previously mapped registers in the topology
> including CXL switch. Endpoints under a CXL switch share a CXL root port
> and will be iterated for each endpoint. Only map once.
>
> Signed-off-by: Terry Bowman <terry.bowman@....com>
Hi Terry,
A few minor comments inline.
Thanks,
Jonathan
> ---
> drivers/cxl/core/pci.c | 30 +++++++++++++++++++++++++-----
> drivers/cxl/cxl.h | 5 +++++
> drivers/cxl/mem.c | 32 ++++++++++++++++++++++++++++++--
> 3 files changed, 60 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 0df09bd79408..e6c91b3dfccf 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -787,16 +787,26 @@ static void cxl_dport_map_rch_aer(struct cxl_dport *dport)
> dport->regs.dport_aer = dport_aer;
> }
>
> -static void cxl_dport_map_regs(struct cxl_dport *dport)
> +static void cxl_port_map_regs(struct device *dev,
> + struct cxl_register_map *map,
> + struct cxl_regs *regs)
> {
> - struct cxl_register_map *map = &dport->reg_map;
> - struct device *dev = dport->dport_dev;
> -
> if (!map->component_map.ras.valid)
> dev_dbg(dev, "RAS registers not found\n");
Maybe return here as nothing useful is going to occur after this any more.
> - else if (cxl_map_component_regs(map, &dport->regs.component,
> + else if (regs->ras)
> + dev_dbg(dev, "RAS registers already initialized\n");
likewise, return if this condition happened.
> + else if (cxl_map_component_regs(map, ®s->component,
> BIT(CXL_CM_CAP_CAP_ID_RAS)))
> dev_dbg(dev, "Failed to map RAS capability.\n");
> +}
> +
> +static void cxl_dport_map_regs(struct cxl_dport *dport)
> +{
> + struct cxl_register_map *map = &dport->reg_map;
> + struct cxl_regs *regs = &dport->regs;
> + struct device *dev = dport->dport_dev;
> +
> + cxl_port_map_regs(dev, map, regs);
>
> if (dport->rch)
> cxl_dport_map_rch_aer(dport);
> @@ -831,6 +841,16 @@ static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
> }
> }
>
> +void cxl_setup_parent_uport(struct device *host, struct cxl_port *port)
> +{
> + struct cxl_register_map *map = &port->reg_map;
> + struct cxl_regs *regs = &port->regs;
> + struct device *uport_dev = port->uport_dev;
> +
> + cxl_port_map_regs(uport_dev, map, regs);
Maybe it will be used later, but based on this patch alone.
cxl_port_map_regs(port->uport_dev, &port->reg_map,
&port->regs);
is more compact and I don't think looses anything on readability front.
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_setup_parent_uport, CXL);
> +
> void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport)
> {
> struct device *dport_dev = dport->dport_dev;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 036d17db68e0..7cee678fdb75 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -587,6 +587,7 @@ struct cxl_dax_region {
> * @parent_dport: dport that points to this port in the parent
> * @decoder_ida: allocator for decoder ids
> * @reg_map: component and ras register mapping parameters
> + * @regs: mapped component registers
> * @nr_dports: number of entries in @dports
> * @hdm_end: track last allocated HDM decoder instance for allocation ordering
> * @commit_end: cursor to track highest committed decoder for commit ordering
> @@ -607,6 +608,7 @@ struct cxl_port {
> struct cxl_dport *parent_dport;
> struct ida decoder_ida;
> struct cxl_register_map reg_map;
> + struct cxl_regs regs;
Does mapping the whole cxl_regs in make sense?
At least currently we can't use the pmu regs in there from here
for instance - the mess with interrupts means that has to bind
via the port driver (for now anyway).
Maybe struct cxl_component_regs is more appropriate here?
> int nr_dports;
> int hdm_end;
> int commit_end;
> @@ -757,9 +759,12 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>
> #ifdef CONFIG_PCIEAER_CXL
> void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport);
> +void cxl_setup_parent_uport(struct device *host, struct cxl_port *port);
> #else
> static inline void cxl_setup_parent_dport(struct device *host,
> struct cxl_dport *dport) { }
> +static inline void cxl_setup_parent_uport(struct device *host,
> + struct cxl_port *port) { }
> #endif
>
> struct cxl_decoder *to_cxl_decoder(struct device *dev);
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 0c79d9ce877c..51a4641fc9a6 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -45,10 +45,39 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
> return 0;
> }
>
> +static bool cxl_dev_is_pci_type(struct device *dev, u32 pcie_type)
Naming perhaps needs work to make it clear this is checking if
it's a CXL device of that type.
Also, seems like general functionality that belongs in core/pci.c or
similar.
> +{
> + struct pci_dev *pdev;
> +
> + if (!dev_is_pci(dev))
> + return false;
> +
> + pdev = to_pci_dev(dev);
> + if (pci_pcie_type(pdev) != pcie_type)
> + return false;
> +
> + return pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL,
> + CXL_DVSEC_REG_LOCATOR);
> +}
> +
> +static void cxl_setup_ep_parent_ports(struct cxl_ep *ep, struct device *host)
> +{
> + struct cxl_dport *dport = ep->dport;
> +
> + if (cxl_dev_is_pci_type(dport->dport_dev, PCI_EXP_TYPE_DOWNSTREAM) ||
> + cxl_dev_is_pci_type(dport->dport_dev, PCI_EXP_TYPE_ROOT_PORT))
> + cxl_setup_parent_dport(host, ep->dport);
> +
> + if (cxl_dev_is_pci_type(dport->port->uport_dev, PCI_EXP_TYPE_UPSTREAM))
> + cxl_setup_parent_uport(host, ep->dport->port);
> +}
> +
> static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
> struct cxl_dport *parent_dport)
> {
> struct cxl_port *parent_port = parent_dport->port;
> + struct cxl_dev_state *cxlds = cxlmd->cxlds;
> + struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> struct cxl_port *endpoint, *iter, *down;
> int rc;
>
> @@ -62,6 +91,7 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>
> ep = cxl_ep_load(iter, cxlmd);
> ep->next = down;
> + cxl_setup_ep_parent_ports(ep, &pdev->dev);
> }
>
> /* Note: endpoint port component registers are derived from @cxlds */
> @@ -157,8 +187,6 @@ static int cxl_mem_probe(struct device *dev)
> else
> endpoint_parent = &parent_port->dev;
>
> - cxl_setup_parent_dport(dev, dport);
> -
> device_lock(endpoint_parent);
> if (!endpoint_parent->driver) {
> dev_err(dev, "CXL port topology %s not enabled\n",
Powered by blists - more mailing lists