From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59253) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zm0rT-00058B-VM for qemu-devel@nongnu.org; Tue, 13 Oct 2015 10:47:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zm0rO-0007X6-So for qemu-devel@nongnu.org; Tue, 13 Oct 2015 10:47:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47260) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zm0rO-0007WF-Ld for qemu-devel@nongnu.org; Tue, 13 Oct 2015 10:47:26 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 2412D8C1DF for ; Tue, 13 Oct 2015 14:47:26 +0000 (UTC) Date: Tue, 13 Oct 2015 17:47:22 +0300 From: "Michael S. Tsirkin" Message-ID: <20151013174558-mutt-send-email-mst@redhat.com> References: <1444663074-32617-1-git-send-email-marcel@redhat.com> <20151012183823-mutt-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum Cc: Marcel Apfelbaum , "qemu-devel@nongnu.org" On Tue, Oct 13, 2015 at 05:31:27PM +0300, Marcel Apfelbaum wrote: >=20 >=20 > On Monday, October 12, 2015, Michael S. Tsirkin wrote: >=20 > On Mon, Oct 12, 2015 at 06:17:54PM +0300, Marcel Apfelbaum wrote: > > The virtio devices are converted to PCI-Express > > if they are plugged into a PCI-Express bus and > > the 'modern' protocol is enabled. > > > > Signed-off-by: Marcel Apfelbaum > > --- > > > > This is an RFC because all it does it adds the PCIe capability an= d > nothing more. >=20 > Express capability is easy. > But if you go over express space you will see that a bunch of > other capabilities are required, such as PM capability etc. > These might need more work. >=20 >=20 > Sure, I'll look on the PCIe spec for the minimum requirements. > =A0 >=20 >=20 > > I post it early so I can get feedbacks on what is the best way to > continue it. > > > > Any comments would be appreciated, > > Thanks, > > Marcel > > > >=A0 hw/virtio/virtio-pci.c | 11 +++++++++++ > >=A0 1 file changed, 11 insertions(+) > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index 6703806..f7c93d9 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -1591,6 +1591,17 @@ static void virtio_pci_realize(PCIDevice *= pci_dev, > Error **errp) > > > >=A0 =A0 =A0 address_space_init(&proxy->modern_as, &proxy->modern_c= fg, > "virtio-pci-cfg-as"); > > > > +=A0 =A0 if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) > > +=A0 =A0 =A0 =A0 && pci_bus_is_express(pci_dev->bus)) { >=20 > One point: we probably want to avoid doing this for integrated > devices on root bus. Does pci_bus_is_express return true there? >=20 >=20 > Hmm, I'll check, but I think it does. Is this a must?=A0 It's probably a smart thing to do because you can't e.g. hot-unplug them. So supporting PCI E capability for integrated devices is probably more work than just making the PCI devices (which is not spec compliant but very popular so guests support this). >=20 >=20 > > +=A0 =A0 =A0 =A0 int pos =3D pci_add_capability(pci_dev, PCI_CAP_= ID_EXP, 0, > > +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0PCI_EXP_VER2_SIZEOF); > > + > > +=A0 =A0 =A0 =A0 if (pos > 0) { >=20 > We probably want to assert on pos < 0 instead. > That implies a code bug. >=20 >=20 >=20 > I was wondering what to do here, thanks for the idea! >=20 > Thanks, > Marcel >=20 > =A0 >=20 >=20 > > +=A0 =A0 =A0 =A0 =A0 =A0 pci_dev->exp.exp_cap =3D pos; > > +=A0 =A0 =A0 =A0 =A0 =A0 pci_dev->cap_present |=3D QEMU_PCI_CAP_E= XPRESS; > > +=A0 =A0 =A0 =A0 } > > +=A0 =A0 } > > + > >=A0 =A0 =A0 virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), pr= oxy); > >=A0 =A0 =A0 if (k->realize) { > >=A0 =A0 =A0 =A0 =A0 k->realize(proxy, errp); > > -- > > 2.1.0 >=20 >=20