From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48236) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bPmqk-0005P4-Kz for qemu-devel@nongnu.org; Wed, 20 Jul 2016 04:27:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bPmqh-0002Tr-As for qemu-devel@nongnu.org; Wed, 20 Jul 2016 04:27:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37628) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bPmqh-0002Tk-2H for qemu-devel@nongnu.org; Wed, 20 Jul 2016 04:27:23 -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 14B82C00F5E8 for ; Wed, 20 Jul 2016 08:27:22 +0000 (UTC) References: <1468953778-15295-1-git-send-email-marcel@redhat.com> <20160720013838-mutt-send-email-mst@kernel.org> From: Marcel Apfelbaum Message-ID: <578F35E7.6020005@redhat.com> Date: Wed, 20 Jul 2016 11:27:19 +0300 MIME-Version: 1.0 In-Reply-To: <20160720013838-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org On 07/20/2016 01:56 AM, Michael S. Tsirkin wrote: > 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, > Hi Michael, > why? could you explain pls? isn't onoffauto compatible with bit > properties? > Yes, indeed. the value of 'Off' is replaced by 'Auto', but the semantics for the names "on/off" remain. > >> 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. > OK >> >> /* >> * 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. > OK > 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. > The line length would be more than 80, so I needed to chose a way to "lose". I'll add || at the end of the line, sure. > > >> + 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; > > ? > We have 2 *different* logic: -disable-modern: make it *false* if plugged into a pcie_port or the user requested it. In this case update QEMU_PCI_CAP_EXPRESS flag accordingly. -disble-legacy: make it *true* if plugged into a pcie_port or the user requested it. If the code seems complicated I'll try to make it more readable. Maybe I'll add a switch for each flag. > > 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. > I thought this method is called before any other init call sites. (like virtio gpu or serial) I'll double-check and update accordingly. > Setting disable modern might be problematic for same reason, not sure what > to do about that one. > > Did I miss anything? I'll check this code runs before the other specific devices init code. If it runs before, we have no problem here, otherwise I'll need to re-think this patch. Thank you for the review and I'll provide the answers in the next version. Marcel > >> + 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