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

[Peter Xu]

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?

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