| Message ID | 1409655606-19150-2-git-send-email-knut.omang@oracle.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 02, 2014 at 01:00:03PM +0200, 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 <knut.omang@oracle.com> Sorry, I don't understand the explanation exactly. pci_dev->devfn is not set correctly? why? > --- > 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 daeaeac..6b21dee 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -1757,7 +1757,7 @@ static int pci_qdev_init(DeviceState *qdev) > 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); > + object_property_get_int(OBJECT(qdev), "addr", NULL)); > if (pci_dev == NULL) > return -1; > > -- > 1.9.0
On Tue, 2014-09-02 at 16:03 +0300, Michael S. Tsirkin wrote: > On Tue, Sep 02, 2014 at 01:00:03PM +0200, 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 <knut.omang@oracle.com> > > Sorry, I don't understand the explanation exactly. > pci_dev->devfn is not set correctly? why? This probably has been broken by some of the qom adaptation if I understand it right: The devfn parameter in pci_create_multifunction() is only used to set the "addr" property, which in turn is not used anywhere as far as I can see. So the pci_dev->devfn has it's default value. What I observe is that when I enable VFs with the igb example device, now with stride = 2 and offset 0x80 (as my real lab hardware implements) and enable some VFs, this yields devfn's of 0x80, 0x82, 0x84... sent as parameters to pci_create..(), I still end up with this (eg. the "natural" next function will be selected instead of the provided devfn): 01:00.0 Ethernet controller: Intel Corporation 82576 Gigabit Network Connection (rev 01) 01:00.1 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) 01:00.2 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) 01:00.3 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) 01:00.4 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) With this patch applied, I get the expected: 01:00.0 Ethernet controller: Intel Corporation 82576 Gigabit Network Connection (rev 01) 01:10.0 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) 01:10.2 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) 01:10.4 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) 01:10.6 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > > --- > > 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 daeaeac..6b21dee 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -1757,7 +1757,7 @@ static int pci_qdev_init(DeviceState *qdev) > > 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); > > + object_property_get_int(OBJECT(qdev), "addr", NULL)); > > if (pci_dev == NULL) > > return -1; > > > > -- > > 1.9.0
On Tue, Sep 02, 2014 at 03:44:28PM +0200, Knut Omang wrote: > On Tue, 2014-09-02 at 16:03 +0300, Michael S. Tsirkin wrote: > > On Tue, Sep 02, 2014 at 01:00:03PM +0200, 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 <knut.omang@oracle.com> > > > > Sorry, I don't understand the explanation exactly. > > pci_dev->devfn is not set correctly? why? > > This probably has been broken by some of the qom adaptation if I > understand it right: The devfn parameter in pci_create_multifunction() > is only used to set the "addr" property, which in turn is not used > anywhere as far as I can see. So the pci_dev->devfn has it's default > value. There are a bunch of places that depend on devfn being correct though so I think this needs to be fixed. > > What I observe is that when I enable VFs with the igb example device, > now with stride = 2 and offset 0x80 (as my real lab hardware implements) > and enable some VFs, this yields devfn's of 0x80, 0x82, 0x84... sent as > parameters to pci_create..(), I still end up with this (eg. > the "natural" next function will be selected instead of the provided > devfn): > > 01:00.0 Ethernet controller: Intel Corporation 82576 Gigabit Network Connection (rev 01) > 01:00.1 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > 01:00.2 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > 01:00.3 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > 01:00.4 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > > With this patch applied, I get the expected: > > 01:00.0 Ethernet controller: Intel Corporation 82576 Gigabit Network Connection (rev 01) > 01:10.0 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > 01:10.2 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > 01:10.4 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > 01:10.6 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > > > --- > > > 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 daeaeac..6b21dee 100644 > > > --- a/hw/pci/pci.c > > > +++ b/hw/pci/pci.c > > > @@ -1757,7 +1757,7 @@ static int pci_qdev_init(DeviceState *qdev) > > > 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); > > > + object_property_get_int(OBJECT(qdev), "addr", NULL)); > > > if (pci_dev == NULL) > > > return -1; > > > > > > -- > > > 1.9.0 >
Michael, I just realized this patch set might have got "stuck" because I did not respond to this? Or did I miss something else? On Tue, 2014-09-02 at 16:55 +0300, Michael S. Tsirkin wrote: > On Tue, Sep 02, 2014 at 03:44:28PM +0200, Knut Omang wrote: > > On Tue, 2014-09-02 at 16:03 +0300, Michael S. Tsirkin wrote: > > > On Tue, Sep 02, 2014 at 01:00:03PM +0200, 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 <knut.omang@oracle.com> > > > > > > Sorry, I don't understand the explanation exactly. > > > pci_dev->devfn is not set correctly? why? > > > > This probably has been broken by some of the qom adaptation if I > > understand it right: The devfn parameter in pci_create_multifunction() > > is only used to set the "addr" property, which in turn is not used > > anywhere as far as I can see. So the pci_dev->devfn has it's default > > value. > > There are a bunch of places that depend on devfn being correct > though so I think this needs to be fixed. The problem with the current code is that pci_dev->devfn is used as an argument to the function that is the one that sets pci_dev->devfn, anything that uses devfn after it has been registered (the normal case) should be fine as far as I can see? Thanks, Knut > > > > What I observe is that when I enable VFs with the igb example device, > > now with stride = 2 and offset 0x80 (as my real lab hardware implements) > > and enable some VFs, this yields devfn's of 0x80, 0x82, 0x84... sent as > > parameters to pci_create..(), I still end up with this (eg. > > the "natural" next function will be selected instead of the provided > > devfn): > > > > 01:00.0 Ethernet controller: Intel Corporation 82576 Gigabit Network Connection (rev 01) > > 01:00.1 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > > 01:00.2 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > > 01:00.3 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > > 01:00.4 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > > > > With this patch applied, I get the expected: > > > > 01:00.0 Ethernet controller: Intel Corporation 82576 Gigabit Network Connection (rev 01) > > 01:10.0 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > > 01:10.2 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > > 01:10.4 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > > 01:10.6 Ethernet controller: Intel Corporation 82576 Virtual Function (rev 01) > > > > > --- > > > > 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 daeaeac..6b21dee 100644 > > > > --- a/hw/pci/pci.c > > > > +++ b/hw/pci/pci.c > > > > @@ -1757,7 +1757,7 @@ static int pci_qdev_init(DeviceState *qdev) > > > > 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); > > > > + object_property_get_int(OBJECT(qdev), "addr", NULL)); > > > > if (pci_dev == NULL) > > > > return -1; > > > > > > > > -- > > > > 1.9.0 > >
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index daeaeac..6b21dee 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -1757,7 +1757,7 @@ static int pci_qdev_init(DeviceState *qdev) 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); + object_property_get_int(OBJECT(qdev), "addr", NULL)); if (pci_dev == NULL) return -1;
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 <knut.omang@oracle.com> --- hw/pci/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)