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

[Philippe Mathieu-Daudé]

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)
>>  {
>