Re: [Qemu-devel] [PATCH v4 1/3] pci: Make use of the devfn property when registering new devices

[Knut Omang]

On Thu, 2015-09-17 at 15:41 +0300, Marcel Apfelbaum wrote:
> On 09/17/2015 03:12 PM, Knut Omang wrote:
> > On Thu, 2015-09-17 at 14:11 +0300, Marcel Apfelbaum wrote:
> > > On 09/12/2015 03:36 PM, Knut Omang wrote:
> > > > Without this, the devfn argument to pci_create_*()
> > > > does not affect the assigned devfn.
> > > > 
> > > > Needed to support (VF_STRIDE,VF_OFFSET) values other than (1,1)
> > > > for SR/IOV.
> > > > 
> > > > Signed-off-by: Knut Omang <address@hidden>
> > > > ---
> > > >    hw/pci/pci.c | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > index ccea628..a5cc015 100644
> > > > --- a/hw/pci/pci.c
> > > > +++ b/hw/pci/pci.c
> > > > @@ -1840,7 +1840,7 @@ static void pci_qdev_realize(DeviceState
> > > > *qdev, Error **errp)
> > > >        bus = PCI_BUS(qdev_get_parent_bus(qdev));
> > > >        pci_dev = do_pci_register_device(pci_dev, bus,
> > > > 
> > > >   object_get_typename(OBJECT(qdev)),
> > > > -                                     pci_dev->devfn, errp);
> > > > +
> > > >   object_property_get_int(OBJECT(qdev), "addr", NULL), errp);
> > > Hi,
> > > 
> > > I don't get this, using object_property_get_int on "addr" should
> > > return the value of pci_dev->devfn,
> > > can you please tell what exactly is not working?
> > 
> > The problem is that at that point pci_dev->devfn has not been set
> > yet -
> > have commented on this before somewhere..
> > 
> 
> But "addr" property has the right value? Is indeed strange because it
> should
> get the value from pci_dev->devfn.

In the current version, in pci_qdev_realize() 
the devfn argument to do_pci_register_device() is set to 
pci_dev->devfn. Then inside do_pci_register_device that devfn argument
is again used to set pci_dev->devfn.

When the SR/IOV code tries to add a second device (the first VF) at 
(devfn + offset + stride) to the bus, it fails because devfn is 0 and
that hits the PF in the devices[] array.

> Don't get me wrong, this patch is OK.
> I just want to understand if we have a hidden bug somewhere.

Yes, havent dived into the details for a while but it probably works
because the pci_dev->devfn argument is often -1 (don't care) and then
the code will just search for an available function number.

> Anyway,
> Reviewed-by: Marcel Apfelbaum <address@hidden>

Thanks,

Knut

> > Knut
> > 
> > > Thanks,
> > > Marcel
> > > 
> > > 
> > > 
> > > >        if (pci_dev == NULL)
> > > >            return;
> > > > 
> > > > 
> > > 
> 
>