From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50812) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQAF7-0007S5-Fd for qemu-devel@nongnu.org; Thu, 21 Jul 2016 05:26:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bQAF5-0005T0-C1 for qemu-devel@nongnu.org; Thu, 21 Jul 2016 05:26:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53095) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQAF5-0005Sh-3R for qemu-devel@nongnu.org; Thu, 21 Jul 2016 05:26:07 -0400 References: <1469028501-23780-1-git-send-email-marcel@redhat.com> <20160721105421.4dfe9669.cornelia.huck@de.ibm.com> From: Marcel Apfelbaum Message-ID: <5790952B.1090506@redhat.com> Date: Thu, 21 Jul 2016 12:26:03 +0300 MIME-Version: 1.0 In-Reply-To: <20160721105421.4dfe9669.cornelia.huck@de.ibm.com> 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: Cornelia Huck Cc: qemu-devel@nongnu.org, mst@redhat.com, kraxel@redhat.com On 07/21/2016 11:54 AM, Cornelia Huck wrote: > On Wed, 20 Jul 2016 18:28:21 +0300 > Marcel Apfelbaum wrote: > >> Enable transitional virtio devices by default. >> Enable virtio-1.0 for devices plugged into >> PCIe ports (Root ports or Downstream ports). > Hi Cornelia, Thank you for the review. > Add "by default", as this can still be overridden? > Yes, using -device virtio*,disable-modern=x,disable-legacy=y are respected as before. >> >> Using the virtio-1 mode will remove the limitation > > s/Using the virtio-1 mode/Disabling the legacy mode/ > > ? > Well, the way I see it virtio-1 'pure' is not using the IO BAR. This is why virtio-1 == disable-modern=off && disable-legacy=on IMHO. If you or Michael see this differently I have nothing against re-wording it. >> 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 > > (...) > >> +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) > > One thing I still find a bit confusing is that you refer to 'modern' > above, but force to 'virtio_1' here... but that's a minor thing. > I went for 'virtio-1' because of the existing comments (force virtio-1) and also because 'modern' does not imply "no legacy" - those are independent flags. BTW, instead of the 'disable*' properties (which I find hard to follow) I would go for one property : "mode" with "legacy"/"transitional"/"virtio-1"/"auto" values. But is too late for that (at least for 2.7). >> +{ >> + 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",\ > > After looking at the code, I think this will work - did you test this > with a compat machine, though? > Yes, I tested it with pc/q35 2.5 and 2.6 machines. The previous behavior remains the same. >> }, >> >> #define HW_COMPAT_2_5 \ > > But generally, looks good and I think this is also an improvement in > readability. > Thanks! Marcel