| Message ID | 1442061415-17430-2-git-send-email-knut.omang@oracle.com |
|---|---|
| State | New |
| Headers | show |
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 <knut.omang@oracle.com> > --- > 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? Thanks, Marcel > if (pci_dev == NULL) > return; > >
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 <knut.omang@oracle.com> > > --- > > 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.. Knut > Thanks, > Marcel > > > > > if (pci_dev == NULL) > > return; > > > > >
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 <knut.omang@oracle.com> >>> --- >>> 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. Don't get me wrong, this patch is OK. I just want to understand if we have a hidden bug somewhere. Anyway, Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> > Knut > >> Thanks, >> Marcel >> >> >> >>> if (pci_dev == NULL) >>> return; >>> >>> >>
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 <knut.omang@oracle.com> > > > > --- > > > > 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 <marcel@redhat.com> Thanks, Knut > > Knut > > > > > Thanks, > > > Marcel > > > > > > > > > > > > > if (pci_dev == NULL) > > > > return; > > > > > > > > > > > > >
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); if (pci_dev == NULL) return;
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(-)