linux-kernel - Re: [PATCH v3 1/9] cxl/acpi: Register CXL host ports by bridge device

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y3Nr5t2XpK8Ek0as@rric.localdomain>
Date:   Tue, 15 Nov 2022 11:37:26 +0100
From:   Robert Richter <rrichter@....com>
To:     Dan Williams <dan.j.williams@...el.com>
CC:     Alison Schofield <alison.schofield@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        Ira Weiny <ira.weiny@...el.com>,
        Ben Widawsky <bwidawsk@...nel.org>,
        <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>,
        Dave Jiang <dave.jiang@...el.com>
Subject: Re: [PATCH v3 1/9] cxl/acpi: Register CXL host ports by bridge device

On 14.11.22 12:22:49, Dan Williams wrote:
> Robert Richter wrote:
> > A port of a CXL host bridge links to the bridge's acpi device
> > (&adev->dev) with its corresponding uport/dport device (uport_dev and
> > dport_dev respectively). The device is not a direct parent device in
> > the PCI topology as pdev->dev.parent points to a PCI bridge's (struct
> > pci_host_bridge) device. The following CXL memory device hierarchy
> > would be valid for an endpoint once an RCD EP would be enabled (note
> > this will be done in a later patch):
> > 
> > VH mode:
> > 
> >  cxlmd->dev.parent->parent
> >         ^^^\^^^^^^\ ^^^^^^\
> >             \      \       pci_dev (Type 1, Downstream Port)
> >              \      pci_dev (Type 0, PCI Express Endpoint)
> >               cxl mem device
> > 
> > RCD mode:
> > 
> >  cxlmd->dev.parent->parent
> >         ^^^\^^^^^^\ ^^^^^^\
> >             \      \       pci_host_bridge
> >              \      pci_dev (Type 0, RCiEP)
> >               cxl mem device
> > 
> > In VH mode a downstream port is created by port enumeration and thus
> > always exists.
> > 
> > Now, in RCD mode the host bridge also already exists but it references
> > to an ACPI device. A port lookup by the PCI device's parent device
> > will fail as a direct link to the registered port is missing. The ACPI
> > device of the bridge must be determined first.
> > 
> > To prevent this, change port registration of a CXL host to use the
> > bridge device instead. Do this also for the VH case as port topology
> > will better reflect the PCI topology then.
> > 
> > If a mock device is registered by a test driver, the bridge pointer
> > can be NULL. Keep using the matching ACPI device (&adev->dev) as a
> > fallback in this case.
> > 
> > Signed-off-by: Robert Richter <rrichter@....com>
> > ---
> >  drivers/cxl/acpi.c | 48 ++++++++++++++++++++++++++++++----------------
> >  1 file changed, 31 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index fb9f72813067..06150c953f58 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -185,6 +185,17 @@ __mock struct acpi_device *to_cxl_host_bridge(struct device *host,
> >  	return NULL;
> >  }
> >  
> > +static inline struct acpi_pci_root *to_cxl_pci_root(struct device *host,
> > +						    struct device *match)
> > +{
> > +	struct acpi_device *adev = to_cxl_host_bridge(host, match);
> > +
> > +	if (!adev)
> > +		return NULL;
> > +
> > +	return acpi_pci_find_root(adev->handle);
> > +}
> > +
> >  /*
> >   * A host bridge is a dport to a CFMWS decode and it is a uport to the
> >   * dport (PCIe Root Ports) in the host bridge.
> > @@ -193,35 +204,35 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> >  {
> >  	struct cxl_port *root_port = arg;
> >  	struct device *host = root_port->dev.parent;
> > -	struct acpi_device *bridge = to_cxl_host_bridge(host, match);
> > -	struct acpi_pci_root *pci_root;
> > +	struct acpi_pci_root *pci_root = to_cxl_pci_root(host, match);
> >  	struct cxl_dport *dport;
> >  	struct cxl_port *port;
> > +	struct device *bridge;
> >  	int rc;
> >  
> > -	if (!bridge)
> > +	if (!pci_root)
> >  		return 0;
> >  
> > -	dport = cxl_find_dport_by_dev(root_port, match);
> > +	/*
> > +	 * If it is a mock dev, the bridge can be NULL, use matching
> > +	 * device (&adev->dev) as a fallback then then.
> > +	 */
> > +	bridge = pci_root->bus->bridge ?: match;
> 
> While I appreciate that you ran this against the unit tests, production
> code should not know or care about the presence of mock devices. So,
> this was showing a gap in the mock implementation, now addressed here:
> 
> http://lore.kernel.org/r/166845667383.1449826.14492184009399164787.stgit@dwillia2-xfh.jf.intel.com

Yes, with that update the above check can be dropped and the code can
be implemented now without the helper. The patch below looks good to
me. Going to run a test with it.

> 
> With that, this approach can be simplified to the following:
> 
> -- >8 --
> >From 3cb7a46e100d016132727ad32904b629061e40d5 Mon Sep 17 00:00:00 2001
> From: Robert Richter <rrichter@....com>
> Date: Mon, 14 Nov 2022 12:20:27 -0800
> Subject: [PATCH v4] cxl/acpi: Register CXL host ports by bridge device

Thanks,

-Robert