diff mbox series

[PATCH-for-6.0,1/3] hw/virtio: Add configure switch to disable legacy VIRTIO

Message ID 20201105124353.3680057-2-philmd@redhat.com
State New
Headers show
Series hw/virtio: Allow to disable legacy VIRTIO | expand

Commit Message

Philippe Mathieu-Daudé Nov. 5, 2020, 12:43 p.m. UTC
Per [1] (Terminology):

  Legacy interfaces are not required; ie. don’t implement them
  unless you have a need for backwards compatibility!

[2] (Version 1.0):

  The device configuration space uses the little-endian format
  for multi-byte fields.

and [3] (Legacy Interface):

  for legacy interfaces, device configuration space is generally
  the guest’s native endian, rather than PCI’s little-endian.
  The correct endian-ness is documented for each device.

Add the --disable-virtio-legacy configure flag to produce builds
with VIRTIO 1.0 only, and the --enable-virtio-legacy to include
legacy VIRTIO support (supporting legacy VIRTIO is the default).

[1] http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-60001
[2] http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-170003
[3] http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-200003

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 configure                         | 10 ++++++++++
 meson.build                       |  1 +
 include/hw/virtio/virtio-access.h | 19 +++++--------------
 hw/virtio/virtio-legacy.c         | 29 +++++++++++++++++++++++++++++
 hw/virtio/meson.build             |  1 +
 5 files changed, 46 insertions(+), 14 deletions(-)
 create mode 100644 hw/virtio/virtio-legacy.c

Comments

Cornelia Huck Nov. 5, 2020, 1:06 p.m. UTC | #1
On Thu,  5 Nov 2020 13:43:51 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Per [1] (Terminology):
> 
>   Legacy interfaces are not required; ie. don’t implement them
>   unless you have a need for backwards compatibility!
> 
> [2] (Version 1.0):
> 
>   The device configuration space uses the little-endian format
>   for multi-byte fields.
> 
> and [3] (Legacy Interface):
> 
>   for legacy interfaces, device configuration space is generally
>   the guest’s native endian, rather than PCI’s little-endian.
>   The correct endian-ness is documented for each device.
> 
> Add the --disable-virtio-legacy configure flag to produce builds
> with VIRTIO 1.0 only, and the --enable-virtio-legacy to include
> legacy VIRTIO support (supporting legacy VIRTIO is the default).

This is only dealing with endianess issues; there are other differences
on the control plane as well.

Currently, virtio-pci has the option to make devices non-transitional,
but virtio-ccw has not (only for device types). For virtio-mmio, you
need to select one of legacy or non-transitional, IIRC.

> 
> [1] http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-60001
> [2] http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-170003
> [3] http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-200003
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  configure                         | 10 ++++++++++
>  meson.build                       |  1 +
>  include/hw/virtio/virtio-access.h | 19 +++++--------------
>  hw/virtio/virtio-legacy.c         | 29 +++++++++++++++++++++++++++++
>  hw/virtio/meson.build             |  1 +
>  5 files changed, 46 insertions(+), 14 deletions(-)
>  create mode 100644 hw/virtio/virtio-legacy.c

(...)

> diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
> index 6818a23a2d3..b6c060f8cc6 100644
> --- a/include/hw/virtio/virtio-access.h
> +++ b/include/hw/virtio/virtio-access.h
> @@ -20,24 +20,15 @@
>  #include "hw/virtio/virtio.h"
>  #include "hw/virtio/virtio-bus.h"
>  
> -#if defined(TARGET_PPC64) || defined(TARGET_ARM)
> -#define LEGACY_VIRTIO_IS_BIENDIAN 1
> -#endif
> -
> +#ifdef CONFIG_VIRTIO_LEGACY
> +bool virtio_access_is_big_endian(VirtIODevice *vdev);
> +#else
>  static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
>  {
> -#if defined(LEGACY_VIRTIO_IS_BIENDIAN)
> -    return virtio_is_big_endian(vdev);
> -#elif defined(TARGET_WORDS_BIGENDIAN)
> -    if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> -        /* Devices conforming to VIRTIO 1.0 or later are always LE. */
> -        return false;
> -    }
> -    return true;
> -#else
> +    /* Devices conforming to VIRTIO 1.0 or later are always LE. */
>      return false;

This will make migration from a QEMU that has devices for which 1.0 has
not been negotiated fail.

> -#endif
>  }
> +#endif
>  
>  static inline uint16_t virtio_lduw_phys(VirtIODevice *vdev, hwaddr pa)
>  {
Paolo Bonzini Nov. 5, 2020, 1:08 p.m. UTC | #2
On 05/11/20 13:43, Philippe Mathieu-Daudé wrote:
> +virtio_legacy="enabled"

>   virtiofsd="auto"

>   virtfs=""

>   libudev="auto"

> @@ -1001,6 +1002,10 @@ for opt do

>     ;;

>     --enable-libudev) libudev="enabled"

>     ;;

> +  --disable-virtio-legacy) virtio_legacy="disabled"

> +  ;;

> +  --enable-virtio-legacy) virtio_legacy="enabled"

> +  ;;

>     --disable-virtiofsd) virtiofsd="disabled"

>     ;;

>     --enable-virtiofsd) virtiofsd="enabled"

> @@ -1764,6 +1769,7 @@ disabled with --disable-FEATURE, default is enabled if available:

>     vnc-png         PNG compression for VNC server

>     cocoa           Cocoa UI (Mac OS X only)

>     virtfs          VirtFS

> +  virtio-legacy   enable support for legacy virtio (before VIRTIO 1.0)

>     virtiofsd       build virtiofs daemon (virtiofsd)

>     libudev         Use libudev to enumerate host devices

>     mpath           Multipath persistent reservation passthrough

> @@ -6816,6 +6822,10 @@ if test "$safe_stack" = "yes"; then

>     echo "CONFIG_SAFESTACK=y" >> $config_host_mak

>   fi

>   

> +if test "$virtio_legacy" = "enabled"; then

> +  echo "CONFIG_VIRTIO_LEGACY=y" >> $config_host_mak

> +fi


Please use meson_options.txt instead (you can make it a boolean option 
with true/false as the choices for the shell variable).

Paolo

>   # If we're using a separate build tree, set it up now.

>   # DIRS are directories which we simply mkdir in the build tree;

>   # LINKS are things to symlink back into the source tree

> diff --git a/meson.build b/meson.build

> index 39ac5cf6d8a..51406c28c6e 100644

> --- a/meson.build

> +++ b/meson.build

> @@ -2061,6 +2061,7 @@

>   summary_info += {'Block whitelist (rw)': config_host['CONFIG_BDRV_RW_WHITELIST']}

>   summary_info += {'Block whitelist (ro)': config_host['CONFIG_BDRV_RO_WHITELIST']}

>   summary_info += {'VirtFS support':    config_host.has_key('CONFIG_VIRTFS')}

> +summary_info += {'Legacy VIRTIO support': config_host.has_key('CONFIG_VIRTIO_LEGACY')}
Philippe Mathieu-Daudé Nov. 5, 2020, 1:53 p.m. UTC | #3
On 11/5/20 2:06 PM, Cornelia Huck wrote:
> On Thu,  5 Nov 2020 13:43:51 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> Per [1] (Terminology):
>>
>>   Legacy interfaces are not required; ie. don’t implement them
>>   unless you have a need for backwards compatibility!
>>
>> [2] (Version 1.0):
>>
>>   The device configuration space uses the little-endian format
>>   for multi-byte fields.
>>
>> and [3] (Legacy Interface):
>>
>>   for legacy interfaces, device configuration space is generally
>>   the guest’s native endian, rather than PCI’s little-endian.
>>   The correct endian-ness is documented for each device.
>>
>> Add the --disable-virtio-legacy configure flag to produce builds
>> with VIRTIO 1.0 only, and the --enable-virtio-legacy to include
>> legacy VIRTIO support (supporting legacy VIRTIO is the default).
> 
> This is only dealing with endianess issues; there are other differences
> on the control plane as well.
> 
> Currently, virtio-pci has the option to make devices non-transitional,
> but virtio-ccw has not (only for device types). For virtio-mmio, you
> need to select one of legacy or non-transitional, IIRC.
> 
>>
>> [1] http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-60001
>> [2] http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-170003
>> [3] http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-200003
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  configure                         | 10 ++++++++++
>>  meson.build                       |  1 +
>>  include/hw/virtio/virtio-access.h | 19 +++++--------------
>>  hw/virtio/virtio-legacy.c         | 29 +++++++++++++++++++++++++++++
>>  hw/virtio/meson.build             |  1 +
>>  5 files changed, 46 insertions(+), 14 deletions(-)
>>  create mode 100644 hw/virtio/virtio-legacy.c
> 
> (...)
> 
>> diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
>> index 6818a23a2d3..b6c060f8cc6 100644
>> --- a/include/hw/virtio/virtio-access.h
>> +++ b/include/hw/virtio/virtio-access.h
>> @@ -20,24 +20,15 @@
>>  #include "hw/virtio/virtio.h"
>>  #include "hw/virtio/virtio-bus.h"
>>  
>> -#if defined(TARGET_PPC64) || defined(TARGET_ARM)
>> -#define LEGACY_VIRTIO_IS_BIENDIAN 1
>> -#endif
>> -
>> +#ifdef CONFIG_VIRTIO_LEGACY
>> +bool virtio_access_is_big_endian(VirtIODevice *vdev);
>> +#else
>>  static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
>>  {
>> -#if defined(LEGACY_VIRTIO_IS_BIENDIAN)
>> -    return virtio_is_big_endian(vdev);
>> -#elif defined(TARGET_WORDS_BIGENDIAN)
>> -    if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>> -        /* Devices conforming to VIRTIO 1.0 or later are always LE. */
>> -        return false;
>> -    }
>> -    return true;
>> -#else
>> +    /* Devices conforming to VIRTIO 1.0 or later are always LE. */
>>      return false;
> 
> This will make migration from a QEMU that has devices for which 1.0 has
> not been negotiated fail.

Oh good point... Not as easy as I thought then :/
Now I'm seeing plenty of possible problems.

Commits 9b3a35ec823 & d55f518248f help a bit:
("virtio: verify that legacy support is not accidentally on")
("virtio: skip legacy support check on machine types less than 5.1")

Thanks for warning :)

> 
>> -#endif
>>  }
>> +#endif
>>  
>>  static inline uint16_t virtio_lduw_phys(VirtIODevice *vdev, hwaddr pa)
>>  {
>
diff mbox series

Patch

diff --git a/configure b/configure
index 2c3c69f1188..f46216b3788 100755
--- a/configure
+++ b/configure
@@ -302,6 +302,7 @@  fdt="auto"
 netmap="no"
 sdl="auto"
 sdl_image="auto"
+virtio_legacy="enabled"
 virtiofsd="auto"
 virtfs=""
 libudev="auto"
@@ -1001,6 +1002,10 @@  for opt do
   ;;
   --enable-libudev) libudev="enabled"
   ;;
+  --disable-virtio-legacy) virtio_legacy="disabled"
+  ;;
+  --enable-virtio-legacy) virtio_legacy="enabled"
+  ;;
   --disable-virtiofsd) virtiofsd="disabled"
   ;;
   --enable-virtiofsd) virtiofsd="enabled"
@@ -1764,6 +1769,7 @@  disabled with --disable-FEATURE, default is enabled if available:
   vnc-png         PNG compression for VNC server
   cocoa           Cocoa UI (Mac OS X only)
   virtfs          VirtFS
+  virtio-legacy   enable support for legacy virtio (before VIRTIO 1.0)
   virtiofsd       build virtiofs daemon (virtiofsd)
   libudev         Use libudev to enumerate host devices
   mpath           Multipath persistent reservation passthrough
@@ -6816,6 +6822,10 @@  if test "$safe_stack" = "yes"; then
   echo "CONFIG_SAFESTACK=y" >> $config_host_mak
 fi
 
+if test "$virtio_legacy" = "enabled"; then
+  echo "CONFIG_VIRTIO_LEGACY=y" >> $config_host_mak
+fi
+
 # If we're using a separate build tree, set it up now.
 # DIRS are directories which we simply mkdir in the build tree;
 # LINKS are things to symlink back into the source tree
diff --git a/meson.build b/meson.build
index 39ac5cf6d8a..51406c28c6e 100644
--- a/meson.build
+++ b/meson.build
@@ -2061,6 +2061,7 @@ 
 summary_info += {'Block whitelist (rw)': config_host['CONFIG_BDRV_RW_WHITELIST']}
 summary_info += {'Block whitelist (ro)': config_host['CONFIG_BDRV_RO_WHITELIST']}
 summary_info += {'VirtFS support':    config_host.has_key('CONFIG_VIRTFS')}
+summary_info += {'Legacy VIRTIO support': config_host.has_key('CONFIG_VIRTIO_LEGACY')}
 summary_info += {'build virtiofs daemon': have_virtiofsd}
 summary_info += {'Multipath support': mpathpersist.found()}
 summary_info += {'VNC support':       vnc.found()}
diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index 6818a23a2d3..b6c060f8cc6 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -20,24 +20,15 @@ 
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-bus.h"
 
-#if defined(TARGET_PPC64) || defined(TARGET_ARM)
-#define LEGACY_VIRTIO_IS_BIENDIAN 1
-#endif
-
+#ifdef CONFIG_VIRTIO_LEGACY
+bool virtio_access_is_big_endian(VirtIODevice *vdev);
+#else
 static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
 {
-#if defined(LEGACY_VIRTIO_IS_BIENDIAN)
-    return virtio_is_big_endian(vdev);
-#elif defined(TARGET_WORDS_BIGENDIAN)
-    if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
-        /* Devices conforming to VIRTIO 1.0 or later are always LE. */
-        return false;
-    }
-    return true;
-#else
+    /* Devices conforming to VIRTIO 1.0 or later are always LE. */
     return false;
-#endif
 }
+#endif
 
 static inline uint16_t virtio_lduw_phys(VirtIODevice *vdev, hwaddr pa)
 {
diff --git a/hw/virtio/virtio-legacy.c b/hw/virtio/virtio-legacy.c
new file mode 100644
index 00000000000..bf28c824a25
--- /dev/null
+++ b/hw/virtio/virtio-legacy.c
@@ -0,0 +1,29 @@ 
+/*
+ * Legacy virtio endian helpers.
+ *
+ * Copyright Red Hat, Inc. 2020
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include "qemu/osdep.h"
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/virtio-access.h"
+
+#if defined(TARGET_PPC64) || defined(TARGET_ARM)
+# define LEGACY_VIRTIO_IS_BIENDIAN 1
+#endif
+
+bool virtio_access_is_big_endian(VirtIODevice *vdev)
+{
+#if defined(LEGACY_VIRTIO_IS_BIENDIAN)
+    return virtio_is_big_endian(vdev);
+#elif defined(TARGET_WORDS_BIGENDIAN)
+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+        /* Devices conforming to VIRTIO 1.0 or later are always LE. */
+        return false;
+    }
+    return true;
+#else
+    return false;
+#endif
+}
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index fbff9bc9d4d..95415913a9a 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -11,6 +11,7 @@ 
 
 virtio_ss = ss.source_set()
 virtio_ss.add(files('virtio.c'))
+virtio_ss.add(when: 'CONFIG_VIRTIO_LEGACY', if_true: files('virtio-legacy.c'))
 virtio_ss.add(when: 'CONFIG_VHOST', if_true: files('vhost.c', 'vhost-backend.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_VDPA', if_true: files('vhost-vdpa.c'))