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

[Alex Williamson <alex.williamson@redhat.com>]

On Mon, 16 May 2016 17:58:18 +0800
Peter Xu <peterx@redhat.com> 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

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