linux-kernel - RE: [PATCH v2 07/12] cxl: Remove dev_is_cxl_root_child() check in devm_cxl_enumerate

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]

Date:   Thu, 20 Oct 2022 22:38:03 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Robert Richter <rrichter@....com>,
        Alison Schofield <alison.schofield@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        "Ira Weiny" <ira.weiny@...el.com>,
        Ben Widawsky <bwidawsk@...nel.org>,
        "Dan Williams" <dan.j.williams@...el.com>
CC:     <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>,
        Robert Richter <rrichter@....com>
Subject: RE: [PATCH v2 07/12] cxl: Remove dev_is_cxl_root_child() check in
 devm_cxl_enumerate_ports()

Robert Richter wrote:
> The dev_is_cxl_root_child() check adds complexity to the control flow
> of the iterator loop in devm_cxl_enumerate_ports(). This check is
> unnecessary and can safely be removed: If the port of a dport_dev is
> connected to the cxl root port, the grandparent of dport_dev will be
> null. This is checked early in the iterator loop and the function is
> left in this case.
> 
> Drop this check to ease the control flow in devm_cxl_enumerate_
> ports().
> 
> This change is a prerequisite of factoring out parts of the loop.

Ok, so this seems to be where we diverge on how an RCH topology maps
into the CXL subsystem object hierarchy. The main observation going
through this with Dave before this set came out is that
devm_cxl_enumerate_ports() is a nop and should be skipped in the RCH
case. devm_cxl_enumerate_ports() is only for discovering intermediate
ports between a host bridge and an endpoint.

In a CXL VH topology a direct attached endpoint is downstream of a
host-bridge's root port. In the CXL RCH topology the
Root-Complex-Integrated-Endpoint is a *peer* of a host-bridge's root
port. So one level of the hierarchy is removed and
devm_cxl_enumerate_ports() can be skipped.

The proposal is to have cxl_mem_probe() do something like:

       if (!cxlds->is_rcd) {
               rc = devm_cxl_enumerate_ports(cxlmd);
               if (rc)
                       return rc;
       }

The existing:

       parent_port = cxl_mem_find_port(cxlmd, &dport);

...should do the right thing as long as cxl_acpi registers the host
bridge as a dport with this change:

-       dport = devm_cxl_add_dport(root_port, match, uid, ...
+       dport = devm_cxl_add_dport(root_port, pci_root->bus->bridge, uid, ... 

That way the dport device is not the ACPI device but the 'struct
pci_dev' for the host-bridge.

With that scheme in place and some cxl-cli fixups from Vishal we are
seeing:

# cxl list -BEMPTu
{
  "bus":"root0",
  "provider":"ACPI.CXL",
  "nr_dports":1,
  "dports":[
    {
      "dport":"pci0000:38",
      "id":"0x31"
    }
  ],
  "endpoints:root0":[
    {
      "endpoint":"endpoint1",
      "host":"mem0",
      "depth":1,
      "memdev":{
        "memdev":"mem0",
        "pmem_size":0,
        "ram_size":"16.00 GiB (17.18 GB)",
        "serial":"0",
        "numa_node":0,
        "host":"0000:38:00.0"
      }
    }
  ]
}

Does that make sense? I think this patchset gets a lot simpler if it
does not try to make devm_cxl_enumerate_ports() understand the RCH
topology.