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

[Michael S. Tsirkin]

On Mon, May 16, 2016 at 05:00:05PM +0800, Peter Xu wrote:
> 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.

Yes, that's preferable.

> > 
> > > +    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).

When do legacy pci systems use requester id at all?
PCI spec does not mention this concept.

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

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

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

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