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

[Mark Cave-Ayland]

On 22/07/2022 08:28, Thomas Huth wrote:

> 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?

(goes and looks)

Thanks! I see, so it appears the older patch wasn't merged because it wasn't possible 
to test the ARI logic (which is missing from Roman's patch) and I suspect 2014 
pre-dates the slot_reserved_mask functionality which I think is the better solution 
for recent QEMU.


ATB,

Mark.