| 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: <aba901a8-4d7d-4f2e-9656-7d6b6a1bc4d2@amd.com>
Date: Mon, 24 Jun 2024 10:51:21 -0500
From: Terry Bowman <Terry.Bowman@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.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
Hi Jonathan,
I added responses inline below.
On 6/20/24 07:46, Jonathan Cameron wrote:
> 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.
>
Ok
>> - 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.
>
Ok
>> + 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.
>
>
Good point.
>> +}
>> +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?
>
>
Only the RAS is mapped. But, as you point out this can be changed to
be cxl_component_regs and it will be more precise in how it's used.
>> 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.
Any suggestions on what to use for the rename?
Regards,
Terry
>
>> +{
>> + 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