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

[Peter Xu]

On Mon, May 16, 2016 at 10:54:13AM +0300, Michael S. Tsirkin wrote:
[...]
> >  
> > +/* Parse bridges up to the root complex and get final Requester ID
> > + * for this device.  For PCIe-all topology, this works exactly as
> > + * pci_get_bdf() does. However, several tricks are required for
> > + * legacy PCI topology and PCIe-to-PCI bridges, to be better aligned
> > + * with spec. */
> > +uint16_t pci_requester_id(PCIDevice *dev)
> > +{
> > +    uint8_t bus_n;
> > +    uint16_t result = pci_get_bdf(dev);
> 
> add empty line here pls.

Will do. Do I need to leave a blank line after declaration of local
variables every time? Just to confirm this for next time.

> 
> > +    while ((bus_n = pci_bus_num(dev->bus))) {
> > +        /* We are under PCI/PCIe bridges */
> > +        dev = dev->bus->parent_dev;
> > +        if (pci_is_express(dev)) {
> > +            if (pcie_cap_get_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE) {
> > +                /* When we pass through PCIe-to-PCI/PCIX bridges, we
> > +                 * override the requester ID using secondary bus
> > +                 * number with zeroed devfn (pcie-to-pci bridge spec
> > +                 * chap 2.3). */
> > +                result = PCI_BUILD_BDF(bus_n, 0);
> > +            }
> > +        } else {
> 
> 
> > +            /* Legacy PCI bus, override requester ID with the
> > +             * bridge's BDF upstream. */
> 
> Don't document what is done, pls document why.
> Please add an explanation why this is the right thing to do.

I see the above partly the "why" for it.. Then, how about this one:

"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). If
devices are attached under specific bridge (no matter there are one
or more bridges), only the requester ID of the bridge that directly
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, 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).

> 
> > +        }
> > +    }
> > +    return result;
> > +}
> > +
> >  static const TypeInfo pci_device_type_info = {
> >      .name = TYPE_PCI_DEVICE,
> >      .parent = TYPE_DEVICE,
> 
> OK but this is used on data path by msi_send_message and
> doing scans there isn't nice.
> Can we store a pointer to device who's requester id is used instead?

Sure. I can try that in v3.

Thanks,

-- peterx