Received-SPF: pass (google.com: domain of linux-kernel+bounces-175289-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 10 May 2024 08:26:16 +0300
From: Mika Westerberg <mika.westerberg@linux.intel.com>
To: Lukas Wunner <lukas@wunner.de>
Cc: Esther Shimanovich <eshimanovich@chromium.org>,
	Mario Limonciello <mario.limonciello@amd.com>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Bjorn Helgaas <bhelgaas@google.com>, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org, Rajat Jain <rajatja@google.com>
Subject: Re: [PATCH v4] PCI: Relabel JHL6540 on Lenovo X1 Carbon 7,8
Message-ID: <20240510052616.GC4162345@black.fi.intel.com>
References: <20240419044945.GR112498@black.fi.intel.com>
 <CA+Y6NJEpWpfPqHO6=Z1XFCXZDUq1+g6EFryB+Urq1=h0PhT+fg@mail.gmail.com>
 <7d68a112-0f48-46bf-9f6d-d99b88828761@amd.com>
 <20240423053312.GY112498@black.fi.intel.com>
 <7197b2ce-f815-48a1-a78e-9e139de796b7@amd.com>
 <20240424085608.GE112498@black.fi.intel.com>
 <CA+Y6NJFyi6e7ype6dTAjxsy5aC80NdVOt+Vg-a0O0y_JsfwSGg@mail.gmail.com>
 <Zi0VLrvUWH6P1_or@wunner.de>
 <CA+Y6NJE8hA+wt+auW1wJBWA6EGMc6CGpmdExr3475E_Yys-Zdw@mail.gmail.com>
 <ZjsKPSgV39SF0gdX@wunner.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <ZjsKPSgV39SF0gdX@wunner.de>

Hi,

On Wed, May 08, 2024 at 07:14:37AM +0200, Lukas Wunner wrote:
> On Wed, May 01, 2024 at 06:23:28PM -0400, Esther Shimanovich wrote:
> > On Sat, Apr 27, 2024 at 3:17AM Lukas Wunner <lukas@wunner.de> wrote:
> > That is correct, when the user-visible issue occurs, no driver is
> > bound to the NHI and XHCI. The discrete JHL chip is not permitted to
> > attach to the external-facing root port because of the security
> > policy, so the NHI and XHCI are not seen by the computer.
> 
> Could you rework your patch to only rectify the NHI's and XHCI's
> device properties and leave the bridges untouched?

As an alternative, I also spent some time to figure out whether there is
a way to detect the integrated Thunderbolt PCIe Root Ports and turns out
it is not "impossible" at least :) Basically it is a list of Ice Lake
and Tiger Lake Thunderbolt PCIe Root Ports. Everything after this is
using the "usb4-host-interface" device property.

I went a head and did a patch that, instead of relabeling, sets the
"untrusted" and "removable" flags based on this and some heuristics (to
figure out the discrete controller) directly on the source. I did some
testing over the hardware I have here and it sets the flags like this:

  - Everything below integrated Thunderbolt/USB4 PCIe root ports are
    marked as "untrusted" and "removable", this includes the Ice Lake
    and Tiger Lake ones. Whereas the NHI and xHCI here are untouched.

  - Everything below discrete Thunderbolt/USB4 host controller PCIe
    downstream ports that are behind a PCIe Root Port with
    "external_facing" set are marked as "untrusted" and "removable"
    whereas endpoints are still "trusted" and "fixed".

I'm sharing the code below. @Esther, you may use it as you like, parts
of it or just ignore the whole thing completely.

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 1325fbae2f28..38bc80c931d6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1612,6 +1612,127 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
 		dev->is_thunderbolt = 1;
 }
 
+static bool pcie_switch_directly_under(struct pci_dev *bridge,
+				       struct pci_dev *parent,
+				       struct pci_dev *pdev)
+{
+	u64 serial, upstream_serial;
+
+	/*
+	 * Check the type of the device to make sure it is part of a PCIe
+	 * switch and if it is try to match the serial numbers too with
+	 * the assumption that they all share the same serial number.
+	 */
+	switch (pci_pcie_type(pdev)) {
+	case PCI_EXP_TYPE_UPSTREAM:
+		if (parent == bridge)
+			return pci_get_dsn(pdev) != 0;
+		break;
+
+	case PCI_EXP_TYPE_DOWNSTREAM:
+		if (pci_pcie_type(parent) == PCI_EXP_TYPE_UPSTREAM) {
+			upstream_serial = pci_get_dsn(parent);
+			if (!upstream_serial)
+				return false;
+			serial = pci_get_dsn(pdev);
+			if (!serial)
+				return false;
+			if (serial != upstream_serial)
+				return false;
+			parent = pci_upstream_bridge(parent);
+			if (parent == bridge)
+				return true;
+		}
+		break;
+
+	case PCI_EXP_TYPE_ENDPOINT:
+		if (pci_pcie_type(parent) == PCI_EXP_TYPE_DOWNSTREAM) {
+			serial = pci_get_dsn(parent);
+			if (!serial)
+				return false;
+			parent = pci_upstream_bridge(parent);
+			if (parent && pci_pcie_type(parent) == PCI_EXP_TYPE_UPSTREAM) {
+				upstream_serial = pci_get_dsn(parent);
+				if (!upstream_serial)
+					return false;
+				if (serial != upstream_serial)
+					return false;
+				parent = pci_upstream_bridge(parent);
+				if (parent == bridge)
+					return true;
+			}
+		}
+		break;
+	}
+
+	return false;
+}
+
+static bool pcie_has_usb4_host_interface(struct pci_dev *pdev)
+{
+	struct fwnode_handle *fwnode;
+
+	/*
+	 * For USB4 the tunneled PCIe root or downstream ports are marked with
+	 * the "usb4-host-interface" property so we look for that first. This
+	 * should cover the most cases.
+	 */
+	fwnode = fwnode_find_reference(dev_fwnode(&pdev->dev),
+				       "usb4-host-interface", 0);
+	if (!IS_ERR(fwnode)) {
+		fwnode_handle_put(fwnode);
+		return true;
+	}
+
+	/*
+	 * Any integrated Thunderbolt 3/4 PCIe root ports from Intel
+	 * before Alder Lake do not have the above device property so we
+	 * use their PCI IDs instead. All these are tunneled. This list
+	 * is not expected to grow.
+	 */
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
+		switch (pdev->device) {
+		/* Ice Lake Thunderbolt 3 PCIe Root Ports */
+		case 0x8a1d:
+		case 0x8a1f:
+		case 0x8a21:
+		case 0x8a23:
+		/* Tiger Lake-LP Thunderbolt 4 PCIe Root Ports */
+		case 0x9a23:
+		case 0x9a25:
+		case 0x9a27:
+		case 0x9a29:
+		/* Tiger Lake-H Thunderbolt 4 PCIe Root Ports */
+		case 0x9a2b:
+		case 0x9a2d:
+		case 0x9a2f:
+		case 0x9a31:
+			return true;
+		}
+	}
+
+	return false;
+}
+
+/* root->external_facing is true, parent != NULL */
+static bool pcie_is_tunneled(struct pci_dev *root, struct pci_dev *parent,
+			     struct pci_dev *pdev)
+{
+	/* Anything directly behind a "usb4-host-interface" is tunneled */
+	if (pcie_has_usb4_host_interface(parent))
+		return true;
+
+	/*
+	 * Check if this is a discrete Thunderbolt/USB4 controller that is
+	 * directly behind a PCIe Root Port marked as "ExternalFacingPort".
+	 * These are not behind a PCIe tunnel.
+	 */
+	if (pcie_switch_directly_under(root, parent, pdev))
+		return false;
+
+	return true;
+}
+
 static void set_pcie_untrusted(struct pci_dev *dev)
 {
 	struct pci_dev *parent;
@@ -1621,8 +1742,32 @@ static void set_pcie_untrusted(struct pci_dev *dev)
 	 * untrusted as well.
 	 */
 	parent = pci_upstream_bridge(dev);
-	if (parent && (parent->untrusted || parent->external_facing))
-		dev->untrusted = true;
+	if (parent) {
+		struct pci_dev *root;
+
+		/* If parent is untrusted so are we */
+		if (parent->untrusted) {
+			pci_dbg(dev, "marking as untrusted\n");
+			dev->untrusted = true;
+			return;
+		}
+
+		root = pcie_find_root_port(dev);
+		if (root && root->external_facing) {
+			/*
+			 * Only PCIe root ports can be marked as
+			 * "ExternalFacingPort", However, in case of a
+			 * discrete Thunderbolt/USB4 controller only its
+			 * downstream facing ports are actually
+			 * something that are exposed to the wild so we
+			 * only mark devices behind those as untrusted.
+			 */
+			if (pcie_is_tunneled(root, parent, dev)) {
+				pci_dbg(dev, "marking as untrusted\n");
+				dev->untrusted = true;
+			}
+		}
+	}
 }
 
 static void pci_set_removable(struct pci_dev *dev)
@@ -1639,10 +1784,15 @@ static void pci_set_removable(struct pci_dev *dev)
 	 * the port is marked with external_facing, such devices are less
 	 * accessible to user / may not be removed by end user, and thus not
 	 * exposed as "removable" to userspace.
+	 *
+	 * These are the same devices marked as untrusted by the above
+	 * function. The ports and endpoints part of the discrete
+	 * Thunderbolt/USB4 controller are not marked as removable.
 	 */
-	if (parent &&
-	    (parent->external_facing || dev_is_removable(&parent->dev)))
+	if (dev->untrusted || (parent && dev_is_removable(&parent->dev))) {
+		pci_dbg(dev, "marking as removable\n");
 		dev_set_removable(&dev->dev, DEVICE_REMOVABLE);
+	}
 }
 
 /**