Re: [PATCH V4 1/1] Intel Sky Lake-E host root ports check.

From: Logan Gunthorpe <logang@deltatee.com>
To: Jason Gunthorpe <jgg@nvidia.com>, Bjorn Helgaas <helgaas@kernel.org>
Cc: Shlomo Pongratz <shlomopongratz@gmail.com>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	andrew.maier@eideticom.com, bhelgaas@google.com,
	Shlomo Pongratz <shlomop@pliops.com>
Subject: Re: [PATCH V4 1/1] Intel Sky Lake-E host root ports check.
Date: Wed, 30 Mar 2022 13:36:25 -0600	[thread overview]
Message-ID: <671cb318-5f4e-4372-d0ef-45be83bdf0b9@deltatee.com> (raw)
In-Reply-To: <20220330191145.GE2120790@nvidia.com>

On 2022-03-30 13:11, Jason Gunthorpe wrote:
> On Wed, Mar 30, 2022 at 02:10:17PM -0500, Bjorn Helgaas wrote:
>> On Wed, Mar 30, 2022 at 12:37:20PM -0300, Jason Gunthorpe wrote:
>>> On Wed, Mar 30, 2022 at 05:08:23PM +0300, Shlomo Pongratz wrote:
>>>> @@ -350,7 +353,10 @@ static struct pci_dev *pci_host_bridge_dev(struct pci_host_bridge *host)
>>>>  
>>>>  	if (!root)
>>>>  		return NULL;
>>>> -	if (root->devfn != PCI_DEVFN(0, 0))
>>>> +
>>>> +	/* Is it a host bridge or a root port? */
>>>
>>> This is a better comment:
>>>
>>> /* host bridges must have a 0 devfn, but some of the entries in the
>>>   whilelist are root ports that can have any devfn */
>>
>> Is this something in the spec or is it just common practice?  The PCIe
>> spec says very little about "host bridges" and I don't remember
>> anything about them having to be devfn 0 or even that they have to be
>> materialized as PCI devices.
> 
> I think we are relying on common practice here, but I don't know why
> this check was added in the first place? Logan?

Yeah, this was more about common practice than spec. Seeing we only
support a subset of devices it was expected that those added to our
support list would meet this criteria.

The main purpose here is to just get identifying information about the
host bridge. I wouldn't say that the host bridge is dev-fn 0 in any
sense of the word. What a host bridge is vs a root port is also a bit
murky . I'm really not sure what the tree looks like on this SkyLake
system, but on my test system, the two host bridges are also root ports.

00:00.0 Host bridge: Intel Corporation Xeon E5/Core i7 DMI2 (rev 07)
	Capabilities: [90] Express (v2) Root Port (Slot-), MSI 00

The point was really just finding the VID/PID of one of the PCI devices
belonging to the bridge for identifying whether it was safe to use.
Originally this function was implemented as simply

    pci_get_slot(host->bus, PCI_DEVFN(0, 0));

That was replaced with the current function solely to avoid needing to
take pci_bus_sem in this path. The check for dev-fn 0 was kept to ensure
it matched the old method.

Checking simply for PCI_EXP_TYPE_ROOT_PORT instead of a zero devfn is
probably a good idea, assuming it works for all existing systems. I'd
expect it would be set for all the devices currently allowed.

Logan