Re: [Qemu-devel] [PATCH v2] pci: fix pci_requester_id()

[Michael S. Tsirkin]

On Mon, May 16, 2016 at 09:44:28AM -0600, Alex Williamson wrote:
> On Mon, 16 May 2016 17:58:18 +0800
> Peter Xu <address@hidden> wrote:
> 
> > On Mon, May 16, 2016 at 12:21:54PM +0300, Michael S. Tsirkin wrote:
> > [...]
> > > > "Legacy PCI bus, override requester ID with the bridge's BDF
> > > > upstream.  The root complex of legacy PCI system can only get
> > > > requester ID from directly attached devices (including bridges).  
> > > 
> > > When do legacy pci systems use requester id at all?
> > > PCI spec does not mention this concept.  
> > 
> > I see some descriptions about this in vt-d spec, e.g., chap
> > 3.9.2. Maybe somewhere else too, but I cannot remember. In the spec,
> > it is told something like:
> > 
> > "...the source-id in the DMA requests is the requester-id of the
> > bridge device..."
> > 
> > Similar thing on interrupt remapping desc in 5.1.1.
> > 
> > Actually I am curious about how generic PCI system delivers
> > requester ID (if there is)... For PCIe, we have encoded TLP header,
> > and requester ID is filled in the specific field of the header.
> > However for legacy PCI system, all the data is transmitted via the
> > parallel interface (no matter 32/64 bits) and I found no place that
> > the requester ID can be included. I was assuming there is some way
> > for the root complex to know it (when request comes, the root
> > complex should be able to know where the request come from, or say,
> > its connected BDF). Never digger into the details, or am I wrong?
> 
> There's no such thing as a requester ID on conventional PCI.  We should
> probably be making use of pci_bus_is_express() to determine whether we
> have a valid requester ID and error if we hit pci_bus_is_root() and we
> still don't have an express bus.  And as MST says, testing for
> bus number zero is not a valid test for the root bus.  Thanks,
> 
> Alex

Well MSI code sticks the requester ID in the MSI message
unconditionally. It's typically later ignored by the the
PC machine type though.

> > >   
> > > > If
> > > > devices are attached under specific bridge (no matter  
> > > 
> > > should be "no matter if"
> > >   
> > > > there are one
> > > > or more bridges), only the requester ID of the bridge that directly  
> > > 
> > > should be "that is directly"  
> > 
> > Will fix above two.
> > 
> > >   
> > > > attached to the root complex can be recognized."
> > > >   
> > > > >   
> > > > > > +            result = pci_get_bdf(dev);  
> > > > > 
> > > > > Won't dev be NULL for a root bus?  
> > > > 
> > > > Should not. The above while() is checking whether dev's parent bus
> > > > number (N) is zero,  
> > > 
> > > OK but from pci perspective it's not a given that it's zero.
> > > I think it isn't for pci expander.
> > > BTW did you try this with expander bridges?  
> > 
> > Nop... I used the same test in as in v1 (Radim's one, with IR
> > patchset applied, since until now IR seems the only one that uses
> > this field), since I found it hard to cover all the combinations
> > (include different PCI/PCIX/PCIe buses, PCI/PCIe devices, and all
> > kinds of topologies, etc.).  Do you think I should do thorough tests
> > for this change? If so, do you have suggestion on which test cases I
> > should (at least) cover?
> > 
> > >   
> > > > and reach here only if it's non-zero. Here, dev
> > > > is already re-used to store the PCIDevice struct for the bus device,
> > > > whose secondary bus number is N (as checked in the while
> > > > condition). So it should never be the root pci bus (which has
> > > > so-called secondary bus number 0).  
> > > 
> > > Pls don't make this assumption. If you want to know
> > > whether it's a root, call pci_bus_is_root.  
> > 
> > Ah, yes, I should use that.
> > 
> > Thanks,
> > 
> > -- peterx