From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49848) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQV81-0001ef-2F for qemu-devel@nongnu.org; Fri, 22 Jul 2016 03:44:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bQV7w-0003UR-D5 for qemu-devel@nongnu.org; Fri, 22 Jul 2016 03:44:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57291) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQV7w-0003Tw-4S for qemu-devel@nongnu.org; Fri, 22 Jul 2016 03:44:08 -0400 References: <1469028501-23780-1-git-send-email-marcel@redhat.com> <20160721231418-mutt-send-email-mst@kernel.org> From: Marcel Apfelbaum Message-ID: <5791CEC2.8030409@redhat.com> Date: Fri, 22 Jul 2016 10:44:02 +0300 MIME-Version: 1.0 In-Reply-To: <20160721231418-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V3] hw/virtio-pci: fix virtio behaviour List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org, kraxel@redhat.com, cornelia.huck@de.ibm.com On 07/21/2016 11:18 PM, Michael S. Tsirkin wrote: > On Wed, Jul 20, 2016 at 06:28:21PM +0300, Marcel Apfelbaum wrote: >> Enable transitional virtio devices by default. >> Enable virtio-1.0 for devices plugged into > > disable legacy is better, I agree. > >> 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. >> >> Signed-off-by: Marcel Apfelbaum > > I think you also want to add some comment with a description explaining > *why* you are disabling legacy for these specific devices. Hi Michael, I thought the above paragraph in the commit message explains it: " 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." What do you think I should add? Thanks, Marcel > > >> --- >> >> Hi, >> >> v2 -> v3: >> - Various code tweaks to simplify if statements (Michael) >> - Enable virtio modern by default (Gerd and Cornelia) >> - Replace virtio flags with actual fields (Gerd) >> - Wrappers for more readable code >> >> 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. >> >> My tests were limited to checking all possible disable-* configurations (and make check for all archs) >> >> Thanks, >> Marcel >> >> hw/display/virtio-gpu-pci.c | 4 +--- >> hw/display/virtio-vga.c | 4 +--- >> hw/virtio/virtio-pci.c | 34 ++++++++++++++++++---------------- >> hw/virtio/virtio-pci.h | 21 +++++++++++++++++---- >> include/hw/compat.h | 8 ++++++++ >> 5 files changed, 45 insertions(+), 26 deletions(-) >> >> diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c >> index a71b230..34a724c 100644 >> --- a/hw/display/virtio-gpu-pci.c >> +++ b/hw/display/virtio-gpu-pci.c >> @@ -30,9 +30,7 @@ static void virtio_gpu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) >> int i; >> >> qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); >> - /* force virtio-1.0 */ >> - vpci_dev->flags &= ~VIRTIO_PCI_FLAG_DISABLE_MODERN; >> - vpci_dev->flags |= VIRTIO_PCI_FLAG_DISABLE_LEGACY; >> + virtio_pci_force_virtio_1(vpci_dev); >> object_property_set_bool(OBJECT(vdev), true, "realized", errp); >> >> for (i = 0; i < g->conf.max_outputs; i++) { >> diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c >> index 315b7fc..5b510a1 100644 >> --- a/hw/display/virtio-vga.c >> +++ b/hw/display/virtio-vga.c >> @@ -134,9 +134,7 @@ static void virtio_vga_realize(VirtIOPCIProxy *vpci_dev, Error **errp) >> >> /* init virtio bits */ >> qdev_set_parent_bus(DEVICE(g), BUS(&vpci_dev->bus)); >> - /* force virtio-1.0 */ >> - vpci_dev->flags &= ~VIRTIO_PCI_FLAG_DISABLE_MODERN; >> - vpci_dev->flags |= VIRTIO_PCI_FLAG_DISABLE_LEGACY; >> + virtio_pci_force_virtio_1(vpci_dev); >> object_property_set_bool(OBJECT(g), true, "realized", &err); >> if (err) { >> error_propagate(errp, err); >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c >> index 2b34b43..11cd634 100644 >> --- a/hw/virtio/virtio-pci.c >> +++ b/hw/virtio/virtio-pci.c >> @@ -161,7 +161,7 @@ static bool virtio_pci_modern_state_needed(void *opaque) >> { >> VirtIOPCIProxy *proxy = opaque; >> >> - return !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN); >> + return virtio_pci_modern(proxy); >> } >> >> static const VMStateDescription vmstate_virtio_pci_modern_state = { >> @@ -300,8 +300,8 @@ static int virtio_pci_ioeventfd_assign(DeviceState *d, EventNotifier *notifier, >> VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d); >> VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); >> VirtQueue *vq = virtio_get_queue(vdev, n); >> - bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY); >> - bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN); >> + bool legacy = virtio_pci_legacy(proxy); >> + bool modern = virtio_pci_modern(proxy); >> bool fast_mmio = kvm_ioeventfd_any_length_enabled(); >> bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY; >> MemoryRegion *modern_mr = &proxy->notify.mr; >> @@ -1576,8 +1576,8 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) >> { >> VirtIOPCIProxy *proxy = VIRTIO_PCI(d); >> VirtioBusState *bus = &proxy->bus; >> - bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY); >> - bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN); >> + bool legacy = virtio_pci_legacy(proxy); >> + bool modern = virtio_pci_modern(proxy); >> bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY; >> uint8_t *config; >> uint32_t size; >> @@ -1696,7 +1696,7 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) >> static void virtio_pci_device_unplugged(DeviceState *d) >> { >> VirtIOPCIProxy *proxy = VIRTIO_PCI(d); >> - bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN); >> + bool modern = virtio_pci_modern(proxy); >> bool modern_pio = proxy->flags & VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY; >> >> virtio_pci_stop_ioeventfd(proxy); >> @@ -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); >> >> /* >> * virtio pci bar layout used by default. >> @@ -1766,8 +1768,11 @@ 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 (proxy->disable_legacy == ON_OFF_AUTO_AUTO) { >> + proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF; >> + } >> + >> + if (pcie_port && pci_is_express(pci_dev)) { >> int pos; >> >> pos = pcie_endpoint_cap_init(pci_dev, 0); >> @@ -1821,10 +1826,9 @@ 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_BOOL("disable-modern", VirtIOPCIProxy, disable_modern, false), >> DEFINE_PROP_BIT("migrate-extra", VirtIOPCIProxy, flags, >> VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT, true), >> DEFINE_PROP_BIT("modern-pio-notify", VirtIOPCIProxy, flags, >> @@ -1841,7 +1845,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)) { >> + virtio_pci_modern(proxy)) { >> pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; >> } >> >> @@ -2303,9 +2307,7 @@ static void virtio_input_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) >> DeviceState *vdev = DEVICE(&vinput->vdev); >> >> qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); >> - /* force virtio-1.0 */ >> - vpci_dev->flags &= ~VIRTIO_PCI_FLAG_DISABLE_MODERN; >> - vpci_dev->flags |= VIRTIO_PCI_FLAG_DISABLE_LEGACY; >> + virtio_pci_force_virtio_1(vpci_dev); >> object_property_set_bool(OBJECT(vdev), true, "realized", errp); >> } >> >> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h >> index e4548c2..25fbf8a 100644 >> --- a/hw/virtio/virtio-pci.h >> +++ b/hw/virtio/virtio-pci.h >> @@ -61,8 +61,6 @@ typedef struct VirtioBusClass VirtioPCIBusClass; >> enum { >> VIRTIO_PCI_FLAG_BUS_MASTER_BUG_MIGRATION_BIT, >> VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, >> - VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, >> - VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, >> VIRTIO_PCI_FLAG_MIGRATE_EXTRA_BIT, >> VIRTIO_PCI_FLAG_MODERN_PIO_NOTIFY_BIT, >> VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT, >> @@ -77,8 +75,6 @@ enum { >> #define VIRTIO_PCI_FLAG_USE_IOEVENTFD (1 << VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT) >> >> /* virtio version flags */ >> -#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) >> >> /* migrate extra state */ >> @@ -144,6 +140,8 @@ struct VirtIOPCIProxy { >> uint32_t modern_mem_bar; >> int config_cap; >> uint32_t flags; >> + bool disable_modern; >> + OnOffAuto disable_legacy; >> uint32_t class_code; >> uint32_t nvectors; >> uint32_t dfselect; >> @@ -158,6 +156,21 @@ struct VirtIOPCIProxy { >> VirtioBusState bus; >> }; >> >> +static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy) >> +{ >> + return !proxy->disable_modern; >> +} >> + >> +static inline bool virtio_pci_legacy(VirtIOPCIProxy *proxy) >> +{ >> + return proxy->disable_legacy == ON_OFF_AUTO_OFF; >> +} >> + >> +static inline void virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy) >> +{ >> + proxy->disable_modern = false; >> + proxy->disable_legacy = ON_OFF_AUTO_ON; >> +} >> >> /* >> * virtio-scsi-pci: This extends VirtioPCIProxy. >> 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