From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41792) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bPdwA-0006ZW-Qm for qemu-devel@nongnu.org; Tue, 19 Jul 2016 18:56:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bPdw5-0002Ro-Ss for qemu-devel@nongnu.org; Tue, 19 Jul 2016 18:56:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42514) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bPdw5-0002Rj-Ki for qemu-devel@nongnu.org; Tue, 19 Jul 2016 18:56:21 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D61EE7F740 for ; Tue, 19 Jul 2016 22:56:20 +0000 (UTC) Date: Wed, 20 Jul 2016 01:56:16 +0300 From: "Michael S. Tsirkin" Message-ID: <20160720013838-mutt-send-email-mst@kernel.org> References: <1468953778-15295-1-git-send-email-marcel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1468953778-15295-1-git-send-email-marcel@redhat.com> Subject: Re: [Qemu-devel] [PATCH V2] hw/virtio-pci: fix virtio behaviour on modern (PCIe) machines List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum Cc: qemu-devel@nongnu.org On Tue, Jul 19, 2016 at 09:42:58PM +0300, Marcel Apfelbaum wrote: > Modern machines are expected to be used by newer setups with > modern guests aiming the use of the latest features. > > Enable modern and disable legacy for virtio devices > plugged into PCIe ports (Root ports or Downstream ports). > Using the Virtio 1 mode will remove the limitation > of the number of devices that can be attached to a machine > by removing the need for the IO BAR. > > Convert 'disable-modern' and 'disable-legacy' properties to OnOffAuto > with default Auto. > > Signed-off-by: Marcel Apfelbaum > --- > > Hi, > > v1 -> v2: > - Stick to existing defaults for old machine types (Michael S. Tsirkin) > > If everyone agrees, I am thinking about getting it into 2.7 > to avoid the ~15 virtio devices limitation per machine. > > Notes: > - The non PCIe machines behaviour should remain the same. > - I hope is OK to make the disable-* properties OnOffAuto. Previous setups > using them can be affected, why? could you explain pls? isn't onoffauto compatible with bit properties? > but libvirt is not using them yet (as far as I know) > - My tests were limited to checking all possible disable-* configurations (and make check for all archs) > > Thanks, > Marcel > > hw/virtio/virtio-pci.c | 31 ++++++++++++++++++++++++------- > hw/virtio/virtio-pci.h | 2 ++ > include/hw/compat.h | 8 ++++++++ > 3 files changed, 34 insertions(+), 7 deletions(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 2b34b43..ec9e84f 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1716,6 +1716,8 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) > { > VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev); > VirtioPCIClass *k = VIRTIO_PCI_GET_CLASS(pci_dev); > + bool pcie_port = (pci_bus_is_express(pci_dev->bus) && > + !pci_bus_is_root(pci_dev->bus)); drop the extra outside () please. > > /* > * virtio pci bar layout used by default. > @@ -1766,8 +1768,23 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) > > address_space_init(&proxy->modern_as, &proxy->modern_cfg, "virtio-pci-cfg-as"); > > - if (pci_is_express(pci_dev) && pci_bus_is_express(pci_dev->bus) && > - !pci_bus_is_root(pci_dev->bus)) { > + if ((pcie_port && (proxy->disable_modern == ON_OFF_AUTO_AUTO)) > + || (proxy->disable_modern == ON_OFF_AUTO_OFF)) { drop the () around == - logic and math mix naturally in C. also, pls put || at end of line, not at the beginning of continuation. this way you can see there is continuation as you read the code naturally. > + proxy->flags &= ~VIRTIO_PCI_FLAG_DISABLE_MODERN; > + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > + } else { > + proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_MODERN; > + pci_dev->cap_present &= ~QEMU_PCI_CAP_EXPRESS; > + } > + > + if ((pcie_port && (proxy->disable_legacy == ON_OFF_AUTO_AUTO)) > + || (proxy->disable_legacy == ON_OFF_AUTO_ON)) { > + proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_LEGACY; > + } else { > + proxy->flags &= ~VIRTIO_PCI_FLAG_DISABLE_LEGACY; > + } > + So this is a bit messy. Can we do: if (pcie_port) default_disable_legacy = false; else default_disable_legacy = true; now if (proxy->disable_legacy == ON_OFF_AUTO_ON || (proxy->disable_legacy == ON_OFF_AUTO_AUTO && default_disable_legacy)) proxy->flags |= VIRTIO_PCI_FLAG_DISABLE_LEGACY; else proxy->flags &= ~VIRTIO_PCI_FLAG_DISABLE_LEGACY; ? Also, I wonder how does this interact with devices that play with these flags themselves, like virtio gpu? I guess we could just set VIRTIO_PCI_FLAG_DISABLE_LEGACY, avoid clearing it. Setting disable modern might be problematic for same reason, not sure what to do about that one. Did I miss anything? > + if (pcie_port && pci_is_express(pci_dev)) { > int pos; > > pos = pcie_endpoint_cap_init(pci_dev, 0); > @@ -1821,10 +1838,10 @@ static void virtio_pci_reset(DeviceState *qdev) > static Property virtio_pci_properties[] = { > DEFINE_PROP_BIT("virtio-pci-bus-master-bug-migration", VirtIOPCIProxy, flags, > VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT, false), > - DEFINE_PROP_BIT("disable-legacy", VirtIOPCIProxy, flags, > - VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false), > - DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags, > - VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, true), > + DEFINE_PROP_ON_OFF_AUTO("disable-legacy", VirtIOPCIProxy, disable_legacy, > + ON_OFF_AUTO_AUTO), > + DEFINE_PROP_ON_OFF_AUTO("disable-modern", VirtIOPCIProxy, disable_modern, > + ON_OFF_AUTO_AUTO), > DEFINE_PROP_BIT("migrate-extra", VirtIOPCIProxy, flags, > VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT, true), > DEFINE_PROP_BIT("modern-pio-notify", VirtIOPCIProxy, flags, > @@ -1841,7 +1858,7 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp) > PCIDevice *pci_dev = &proxy->pci_dev; > > if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) && > - !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)) { > + !(proxy->disable_modern == ON_OFF_AUTO_ON)) { > pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > } > > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h > index e4548c2..4f219d4 100644 > --- a/hw/virtio/virtio-pci.h > +++ b/hw/virtio/virtio-pci.h > @@ -144,6 +144,8 @@ struct VirtIOPCIProxy { > uint32_t modern_mem_bar; > int config_cap; > uint32_t flags; > + OnOffAuto disable_modern; > + OnOffAuto disable_legacy; > uint32_t class_code; > uint32_t nvectors; > uint32_t dfselect; > diff --git a/include/hw/compat.h b/include/hw/compat.h > index 9914e7a..1531399 100644 > --- a/include/hw/compat.h > +++ b/include/hw/compat.h > @@ -6,6 +6,14 @@ > .driver = "virtio-mmio",\ > .property = "format_transport_address",\ > .value = "off",\ > + },{\ > + .driver = "virtio-pci",\ > + .property = "disable-modern",\ > + .value = "on",\ > + },{\ > + .driver = "virtio-pci",\ > + .property = "disable-legacy",\ > + .value = "off",\ > }, > > #define HW_COMPAT_2_5 \ > -- > 2.4.3