From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52698) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZvTTx-0004Y6-Rc for qemu-devel@nongnu.org; Sun, 08 Nov 2015 12:10:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZvTTt-0000rC-Oe for qemu-devel@nongnu.org; Sun, 08 Nov 2015 12:10:21 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35560) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZvTTt-0000r8-HC for qemu-devel@nongnu.org; Sun, 08 Nov 2015 12:10:17 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id D2A64225 for ; Sun, 8 Nov 2015 17:10:16 +0000 (UTC) Date: Sun, 8 Nov 2015 19:10:14 +0200 From: "Michael S. Tsirkin" Message-ID: <20151108190623-mutt-send-email-mst@redhat.com> References: <1446119788-19722-1-git-send-email-marcel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1446119788-19722-1-git-send-email-marcel@redhat.com> Subject: Re: [Qemu-devel] [PATCH V3] hw/virtio: Add PCIe capability to virtio devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum Cc: qemu-devel@nongnu.org, ehabkost@redhat.com, kraxel@redhat.com On Thu, Oct 29, 2015 at 01:56:28PM +0200, 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. > > Devices plugged directly into the Root Complex as > Integrated Endpoints remain PCI. > > Signed-off-by: Marcel Apfelbaum Looks ok. Two comments: > --- > v2 -> v3: > - Addressed Michael S. Tsirkin's comments: > - enable pcie only for 2.5+ machines. > > v1 -> v2: > - Addressed Michael S. Tsirkin's comments: > - Added the minimum required capabilities for PCIe devices > - Integrated Endpoints remain PCI > > - Use pcie_endpoint_cap_init instead of manually creating the pcie capability. > > - Regarding Gerd Hoffman's comments: > - Creating virtio-pcie devices: > For the moment I prefer to not duplicate the virtio definitions, > at least until we don't have a consensus (Personally I don't like it) > - Removing the IO bar: > This would be my next patch on the "virtio to express" series, I plan > to remove it only for "modern" devices. > > Thanks, > Marcel > > hw/virtio/virtio-pci.c | 22 ++++++++++++++++++++++ > hw/virtio/virtio-pci.h | 2 ++ > include/hw/compat.h | 46 +++++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 69 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index f55dd2b..a288d8b 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1592,6 +1592,26 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) > > address_space_init(&proxy->modern_as, &proxy->modern_cfg, "virtio-pci-cfg-as"); > > + if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) > + && !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) > + && pci_bus_is_express(pci_dev->bus) > + && !pci_bus_is_root(pci_dev->bus)) { > + int pos; > + > + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > + pos = pcie_endpoint_cap_init(pci_dev, 0); > + assert(pos > 0); > + > + pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF); > + assert(pos > 0); > + > + /* > + * Indicates that this function complies with revision 1.2 of the > + * PCI Power Management Interface Specification. > + */ > + pci_set_word(pci_dev->config + pos + PCI_PM_PMC, 0x3); > + } > + > virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy); > if (k->realize) { > k->realize(proxy, errp); > @@ -1622,6 +1642,8 @@ static Property virtio_pci_properties[] = { > VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false), > DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags, > VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, true), > + DEFINE_PROP_BIT("disable-pcie", VirtIOPCIProxy, flags, > + VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT, false), It's preferable to call this one x-disable-pcie - this is the convention to mark it as "not for user". > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h > index 801c23a..1a487fc 100644 > --- a/hw/virtio/virtio-pci.h > +++ b/hw/virtio/virtio-pci.h > @@ -72,8 +72,10 @@ typedef struct VirtioBusClass VirtioPCIBusClass; > /* virtio version flags */ > #define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2 > #define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3 > +#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 4 > #define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT) > #define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT) > +#define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT) > > typedef struct { > MSIMessage msg; > diff --git a/include/hw/compat.h b/include/hw/compat.h > index 095de5d..0a08531 100644 > --- a/include/hw/compat.h > +++ b/include/hw/compat.h > @@ -2,7 +2,51 @@ > #define HW_COMPAT_H > > #define HW_COMPAT_2_4 \ > - /* empty */ > + {\ > + .driver = "virtio-blk-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-scsi-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-net-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-input-host-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-keyboard-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-mouse-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-serial-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-tablet-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-gpu-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-balloon-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-rng-pci",\ > + .property = "disable-pcie",\ > + .value = "on",\ > + }, Can't these properties be defined for type virtio-pci? If no - why not? > > #define HW_COMPAT_2_3 \ > {\ > -- > 2.1.0