From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35409) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmJHw-0002EY-67 for qemu-devel@nongnu.org; Wed, 14 Oct 2015 06:28:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZmJHr-0006KZ-1b for qemu-devel@nongnu.org; Wed, 14 Oct 2015 06:28:04 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:20420) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmJHq-0006KH-Rd for qemu-devel@nongnu.org; Wed, 14 Oct 2015 06:27:58 -0400 Message-ID: <1444818459.7681.42.camel@oracle.com> From: Knut Omang Date: Wed, 14 Oct 2015 12:27:39 +0200 In-Reply-To: <55FAB511.1030901@redhat.com> References: <1442061415-17430-1-git-send-email-knut.omang@oracle.com> <1442061415-17430-2-git-send-email-knut.omang@oracle.com> <55FA9FFA.2020901@gmail.com> <1442491936.2488.135.camel@oracle.com> <55FAB511.1030901@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 1/3] pci: Make use of the devfn property when registering new devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum , qemu-devel@nongnu.org Cc: Eduardo Habkost , "Michael S. Tsirkin" , "Richard W.M. Jones" , Alex Williamson , "Gonglei (Arei)" , Jan Kiszka , Paolo Bonzini , Dotan Barak , Richard Henderson 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 > > > > --- > > > > 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 Thanks, Knut > > Knut > > > > > Thanks, > > > Marcel > > > > > > > > > > > > > if (pci_dev == NULL) > > > > return; > > > > > > > > > > > > >