Re: [PATCH v3] hw/pci/pci_bridge: ensure PCIe slots have only one slot

[Thomas Huth <thuth@redhat.com>]

On 21/07/2022 18.05, Mark Cave-Ayland wrote:
> On 21/07/2022 16:56, Daniel P. Berrangé wrote:
> 
>> On Thu, Jul 21, 2022 at 04:51:51PM +0100, Mark Cave-Ayland wrote:
>>> On 21/07/2022 15:28, Roman Kagan wrote:
>>>
>>> (lots cut)
>>>
>>>> In the guest (Fedora 34):
>>>>
>>>> [root@test ~]# lspci -tv
>>>> -[0000:00]-+-00.0  Intel Corporation 82G33/G31/P35/P31 Express DRAM 
>>>> Controller
>>>>              +-01.0  Device 1234:1111
>>>>              +-02.0  Red Hat, Inc. QEMU XHCI Host Controller
>>>>              +-05.0-[01]----00.0  Red Hat, Inc. Virtio block device
>>>>              +-05.1-[02]----00.0  Red Hat, Inc. Virtio network device
>>>>              +-05.2-[03]--
>>>>              +-05.3-[04]--
>>>>              +-1f.0  Intel Corporation 82801IB (ICH9) LPC Interface 
>>>> Controller
>>>>              \-1f.3  Intel Corporation 82801I (ICH9 Family) SMBus 
>>>> Controller
>>>>
>>>> Changing addr of the second disk from 4 to 0 makes it appear in the
>>>> guest.
>>>>
>>>> What exactly do you find odd?
>>>
>>> Thanks for this, the part I wasn't sure about was whether the device ids in
>>> the command line matched the primary PCI bus or the secondary PCI bus.
>>>
>>> In that case I suspect that the enumeration of non-zero PCIe devices fails
>>> in Linux because of the logic here:
>>> https://github.com/torvalds/linux/blob/master/drivers/pci/probe.c#L2622.
>>
>> Just above that though is logic that handles 'pci=pcie_scan_all'
>> kernel parameter, to make it look for non-zero devices.
>>
>>> I don't have a copy of the PCIe specification, but assuming the comment is
>>> true then your patch looks correct to me. I think it would be worth adding a
>>> similar comment and reference to your patch to explain why the logic is
>>> required, which should also help the PCI maintainers during review.
>>
>> The docs above with the pci=pcie_scan_all suggest it is unusual but not
>> forbidden.
> 
> That's interesting as I read it completely the other way around, i.e. PCIe 
> downstream ports should only have device 0 and the PCI_SCAN_ALL_PCIE_DEVS 
> flag is there for broken/exotic hardware :)
> 
> Perhaps if someone has a copy of the PCIe specification they can check the 
> wording in section 7.3.1 to see exactly what the correct behaviour should be?

I've got an older version here... it talks about the "Alternative Routing-ID 
Interpretation" (ARI) there:

"With non-ARI Devices, PCI Express components are restricted to implementing 
a single Device Number on their primary interface (Upstream Port), but are 
permitted to implement up to eight
independent Functions within that Device Number. [...] Downstream Ports that 
do not have ARI Forwarding enabled must associate only Device 0 with the 
device [...].
With an ARI Device, its Device Number is implied to be 0 rather than 
specified by a field within an ID. The traditional 5-bit Device Number and 
3-bit Function Number fields in its associated
Routing IDs, Requester IDs, and Completer IDs are interpreted as a single 
8-bit Function Number."

There was also an older patch similar to yours here:

 
https://lore.kernel.org/all/33183CC9F5247A488A2544077AF1902086D6B3F5@SZXEMA503-MBS.china.huawei.com/T/

... but if I've got that right, it has never been merged?

  HTH,
   Thomas