| Message ID | 20200625100430.22407-1-mhartmay@linux.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Enable virtio-fs on s390x | expand |
Patchew URL: https://patchew.org/QEMU/20200625100430.22407-1-mhartmay@linux.ibm.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [RFC 0/4] Enable virtio-fs on s390x
Type: series
Message-id: 20200625100430.22407-1-mhartmay@linux.ibm.com
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
- [tag update] patchew/20200622153318.751107-1-berrange@redhat.com -> patchew/20200622153318.751107-1-berrange@redhat.com
* [new tag] patchew/20200625100430.22407-1-mhartmay@linux.ibm.com -> patchew/20200625100430.22407-1-mhartmay@linux.ibm.com
* [new tag] patchew/20200625100811.12690-1-peter.maydell@linaro.org -> patchew/20200625100811.12690-1-peter.maydell@linaro.org
Switched to a new branch 'test'
b2cd719 HACK: Hard-code the libvhost-user.o-cflags for s390x
f0ba6d5 libvhost-user: handle endianness as mandated by the spec
5f2feba libvhost-user: print invalid address on vu_panic
19994e0 virtio: add vhost-user-fs-ccw device
=== OUTPUT BEGIN ===
1/4 Checking commit 19994e01a67c (virtio: add vhost-user-fs-ccw device)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#25:
new file mode 100644
ERROR: space required after that ',' (ctx:VxV)
#86: FILE: hw/s390x/vhost-user-fs-ccw.c:57:
+ device_class_set_props(dc,vhost_user_fs_ccw_properties);
^
total: 1 errors, 1 warnings, 81 lines checked
Patch 1/4 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/4 Checking commit 5f2feba1e3c5 (libvhost-user: print invalid address on vu_panic)
3/4 Checking commit f0ba6d5325c1 (libvhost-user: handle endianness as mandated by the spec)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#27:
new file mode 100644
ERROR: braces {} are necessary for all arms of this statement
#100: FILE: contrib/libvhost-user/libvhost-access.h:69:
+ if (vu_access_is_big_endian(vdev))
[...]
ERROR: braces {} are necessary for all arms of this statement
#107: FILE: contrib/libvhost-user/libvhost-access.h:76:
+ if (vu_access_is_big_endian(vdev))
[...]
ERROR: braces {} are necessary for all arms of this statement
#114: FILE: contrib/libvhost-user/libvhost-access.h:83:
+ if (vu_access_is_big_endian(vdev))
[...]
WARNING: line over 80 characters
#375: FILE: contrib/libvhost-user/libvhost-user.c:2515:
+ vu_ldq_p(dev, &desc[i].addr), vu_ldl_p(dev, &desc[i].len));
WARNING: line over 80 characters
#384: FILE: contrib/libvhost-user/libvhost-user.c:2523:
+ vu_ldq_p(dev, &desc[i].addr), vu_ldl_p(dev, &desc[i].len));
total: 3 errors, 3 warnings, 392 lines checked
Patch 3/4 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/4 Checking commit b2cd71997828 (HACK: Hard-code the libvhost-user.o-cflags for s390x)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200625100430.22407-1-mhartmay@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Patchew URL: https://patchew.org/QEMU/20200625100430.22407-1-mhartmay@linux.ibm.com/
Hi,
This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
CC qga/qapi-generated/qga-qapi-commands.o
CC qga/qapi-generated/qga-qapi-init-commands.o
In file included from /tmp/qemu-test/src/contrib/libvhost-user/libvhost-user.c:44:0:
/tmp/qemu-test/src/include/qemu/osdep.h:32:27: fatal error: config-target.h: No such file or directory
#include "config-target.h"
^
compilation terminated.
---
BUILD pc-bios/optionrom/linuxboot_dma.raw
SIGN pc-bios/optionrom/pvh.bin
SIGN pc-bios/optionrom/linuxboot_dma.bin
make: *** [contrib/libvhost-user/libvhost-user.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 669, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=7638277b753647899fcb0c31ef086305', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-uuens814/src/docker-src.2020-06-25-06.13.41.17712:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=7638277b753647899fcb0c31ef086305
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-uuens814/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 2m49.544s
user 0m8.892s
The full log is available at
http://patchew.org/logs/20200625100430.22407-1-mhartmay@linux.ibm.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On Thu, 25 Jun 2020 12:04:26 +0200 Marc Hartmayer <mhartmay@linux.ibm.com> wrote: > This RFC is about enabling virtio-fs on s390x. For that we need > + some shim code (first patch), and we need > + libvhost-user to deal with virtio endiannes as mandated by the spec. > > The second part is trickier, because unlike QEMU we are not certain > about the guest's native endianness, which is needed to handle the > legacy-interface appropriately. In fact, this is the reason why just > RFC. > > One of the open questions is whether to build separate versions, one > for guest little endian and one for guest big endian, or do we want > something like a command line option? (Digression on the libvirt > modeling) > > A third option would be to refuse legacy altogether. The third option looks the most tempting to me. It is a new device, so I don't think there's much of a case to be made for pre-virtio-1 support?
On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote: > This RFC is about enabling virtio-fs on s390x. For that we need > + some shim code (first patch), and we need > + libvhost-user to deal with virtio endiannes as mandated by the spec. > > The second part is trickier, because unlike QEMU we are not certain > about the guest's native endianness, which is needed to handle the > legacy-interface appropriately. In fact, this is the reason why just > RFC. > > One of the open questions is whether to build separate versions, one > for guest little endian and one for guest big endian, or do we want > something like a command line option? (Digression on the libvirt > modeling) When you talk about big vs little endian, are you referring to TCG scenarios with mixed host/guest arch, or arches which can support either endianess, or both ? i guess it doesn't matter actually, as I think the latter forces a specific answer. Considering that some architectures allow the guest OS to flip between big & little endian as they boot, libvirt cannot know what endianess the guest is using when it launches virtiofsd. It thus cannot pick between two different endianness builds of virtiofsd automatically. This would force the user to tell libvirt what arch the guest is using at the time they define the guest. This is an undesirable restriction for use cases where the admin of the guest OS has no direct control over the host config. IOW, I think the only practical answer is to have a single binary that automagically does the right thing at runtime according to guest endianess that currently is in use. Regards, Daniel
On Thu, 25 Jun 2020 11:19:35 +0100 Daniel P. Berrangé <berrange@redhat.com> wrote: > On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote: > > This RFC is about enabling virtio-fs on s390x. For that we need > > + some shim code (first patch), and we need > > + libvhost-user to deal with virtio endiannes as mandated by the spec. > > > > The second part is trickier, because unlike QEMU we are not certain > > about the guest's native endianness, which is needed to handle the > > legacy-interface appropriately. In fact, this is the reason why just > > RFC. > > > > One of the open questions is whether to build separate versions, one > > for guest little endian and one for guest big endian, or do we want > > something like a command line option? (Digression on the libvirt > > modeling) > > When you talk about big vs little endian, are you referring to TCG > scenarios with mixed host/guest arch, or arches which can support > either endianess, or both ? i guess it doesn't matter actually, as > I think the latter forces a specific answer. > > Considering that some architectures allow the guest OS to flip between > big & little endian as they boot, libvirt cannot know what endianess > the guest is using when it launches virtiofsd. It thus cannot pick > between two different endianness builds of virtiofsd automatically. > This would force the user to tell libvirt what arch the guest is using > at the time they define the guest. This is an undesirable restriction > for use cases where the admin of the guest OS has no direct control > over the host config. Right, but that is in practice only a problem for legacy devices, isn't it? The standard says that non-legacy devices use little-endian everywhere; it's the legacy 'device endian' that is causing us headaches. Which leads to the question: Do we really need to support legacy virtio-fs devices, or can we just force virtio-1, as many (most?) newer virtio devices do? > > IOW, I think the only practical answer is to have a single binary that > automagically does the right thing at runtime according to guest > endianess that currently is in use. > > Regards, > Daniel
On Thu, Jun 25, 2020 at 12:31:36PM +0200, Cornelia Huck wrote: > On Thu, 25 Jun 2020 11:19:35 +0100 > Daniel P. Berrangé <berrange@redhat.com> wrote: > > > On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote: > > > This RFC is about enabling virtio-fs on s390x. For that we need > > > + some shim code (first patch), and we need > > > + libvhost-user to deal with virtio endiannes as mandated by the spec. > > > > > > The second part is trickier, because unlike QEMU we are not certain > > > about the guest's native endianness, which is needed to handle the > > > legacy-interface appropriately. In fact, this is the reason why just > > > RFC. > > > > > > One of the open questions is whether to build separate versions, one > > > for guest little endian and one for guest big endian, or do we want > > > something like a command line option? (Digression on the libvirt > > > modeling) > > > > When you talk about big vs little endian, are you referring to TCG > > scenarios with mixed host/guest arch, or arches which can support > > either endianess, or both ? i guess it doesn't matter actually, as > > I think the latter forces a specific answer. > > > > Considering that some architectures allow the guest OS to flip between > > big & little endian as they boot, libvirt cannot know what endianess > > the guest is using when it launches virtiofsd. It thus cannot pick > > between two different endianness builds of virtiofsd automatically. > > This would force the user to tell libvirt what arch the guest is using > > at the time they define the guest. This is an undesirable restriction > > for use cases where the admin of the guest OS has no direct control > > over the host config. > > Right, but that is in practice only a problem for legacy devices, isn't > it? The standard says that non-legacy devices use little-endian > everywhere; it's the legacy 'device endian' that is causing us > headaches. > > Which leads to the question: Do we really need to support legacy > virtio-fs devices, or can we just force virtio-1, as many (most?) newer > virtio devices do? I'd hope virtio-fs is already forced to modern only, as there's no legacy PCI ID assigned to it in the spec. Regards, Daniel
On Thu, 25 Jun 2020 11:39:24 +0100 Daniel P. Berrangé <berrange@redhat.com> wrote: > On Thu, Jun 25, 2020 at 12:31:36PM +0200, Cornelia Huck wrote: > > On Thu, 25 Jun 2020 11:19:35 +0100 > > Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote: > > > > This RFC is about enabling virtio-fs on s390x. For that we need > > > > + some shim code (first patch), and we need > > > > + libvhost-user to deal with virtio endiannes as mandated by the spec. > > > > > > > > The second part is trickier, because unlike QEMU we are not certain > > > > about the guest's native endianness, which is needed to handle the > > > > legacy-interface appropriately. In fact, this is the reason why just > > > > RFC. > > > > > > > > One of the open questions is whether to build separate versions, one > > > > for guest little endian and one for guest big endian, or do we want > > > > something like a command line option? (Digression on the libvirt > > > > modeling) > > > > > > When you talk about big vs little endian, are you referring to TCG > > > scenarios with mixed host/guest arch, or arches which can support > > > either endianess, or both ? i guess it doesn't matter actually, as > > > I think the latter forces a specific answer. > > > > > > Considering that some architectures allow the guest OS to flip between > > > big & little endian as they boot, libvirt cannot know what endianess > > > the guest is using when it launches virtiofsd. It thus cannot pick > > > between two different endianness builds of virtiofsd automatically. > > > This would force the user to tell libvirt what arch the guest is using > > > at the time they define the guest. This is an undesirable restriction > > > for use cases where the admin of the guest OS has no direct control > > > over the host config. > > > > Right, but that is in practice only a problem for legacy devices, isn't > > it? The standard says that non-legacy devices use little-endian > > everywhere; it's the legacy 'device endian' that is causing us > > headaches. > > > > Which leads to the question: Do we really need to support legacy > > virtio-fs devices, or can we just force virtio-1, as many (most?) newer > > virtio devices do? > > I'd hope virtio-fs is already forced to modern only, as there's no legacy > PCI ID assigned to it in the spec. I did not find a call to virtio_pci_force_virtio_1(), so apparently not?
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote: > > This RFC is about enabling virtio-fs on s390x. For that we need > > + some shim code (first patch), and we need > > + libvhost-user to deal with virtio endiannes as mandated by the spec. > > > > The second part is trickier, because unlike QEMU we are not certain > > about the guest's native endianness, which is needed to handle the > > legacy-interface appropriately. In fact, this is the reason why just > > RFC. > > > > One of the open questions is whether to build separate versions, one > > for guest little endian and one for guest big endian, or do we want > > something like a command line option? (Digression on the libvirt > > modeling) > > When you talk about big vs little endian, are you referring to TCG > scenarios with mixed host/guest arch, or arches which can support > either endianess, or both ? i guess it doesn't matter actually, as > I think the latter forces a specific answer. > > Considering that some architectures allow the guest OS to flip between > big & little endian as they boot, libvirt cannot know what endianess > the guest is using when it launches virtiofsd. It thus cannot pick > between two different endianness builds of virtiofsd automatically. > This would force the user to tell libvirt what arch the guest is using > at the time they define the guest. This is an undesirable restriction > for use cases where the admin of the guest OS has no direct control > over the host config. > > IOW, I think the only practical answer is to have a single binary that > automagically does the right thing at runtime according to guest > endianess that currently is in use. Modifying the fuse code in virtiofsd to handle cross-endian would be a big job; IMHO unless you really have a cross-endian need I'd just make sure it deals ok with big<->big and little<->little. Dave > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, 25 Jun 2020 12:17:55 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Thu, 25 Jun 2020 12:04:26 +0200 > Marc Hartmayer <mhartmay@linux.ibm.com> wrote: > > > This RFC is about enabling virtio-fs on s390x. For that we need > > + some shim code (first patch), and we need > > + libvhost-user to deal with virtio endiannes as mandated by the spec. > > > > The second part is trickier, because unlike QEMU we are not certain > > about the guest's native endianness, which is needed to handle the > > legacy-interface appropriately. In fact, this is the reason why just > > RFC. > > > > One of the open questions is whether to build separate versions, one > > for guest little endian and one for guest big endian, or do we want > > something like a command line option? (Digression on the libvirt > > modeling) > > > > A third option would be to refuse legacy altogether. > > The third option looks the most tempting to me. It is a new device, so > I don't think there's much of a case to be made for pre-virtio-1 > support? > > Yes, virtio-fs is a new device, but libvhost-user is not specific to virtio-fs. Regards, Halil
On Thu, 25 Jun 2020 11:19:35 +0100 Daniel P. Berrangé <berrange@redhat.com> wrote: > On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote: > > This RFC is about enabling virtio-fs on s390x. For that we need > > + some shim code (first patch), and we need > > + libvhost-user to deal with virtio endiannes as mandated by the spec. > > > > The second part is trickier, because unlike QEMU we are not certain > > about the guest's native endianness, which is needed to handle the > > legacy-interface appropriately. In fact, this is the reason why just > > RFC. > > > > One of the open questions is whether to build separate versions, one > > for guest little endian and one for guest big endian, or do we want > > something like a command line option? (Digression on the libvirt > > modeling) > > When you talk about big vs little endian, are you referring to TCG > scenarios with mixed host/guest arch, or arches which can support > either endianess, or both ? i guess it doesn't matter actually, as > I think the latter forces a specific answer. > Our primary concern is big endian cpu and little endian virtio because virto 1.0 or better. But since we talk about a lib here, everything is possible. > Considering that some architectures allow the guest OS to flip between > big & little endian as they boot, libvirt cannot know what endianess > the guest is using when it launches virtiofsd. It thus cannot pick > between two different endianness builds of virtiofsd automatically. > This would force the user to tell libvirt what arch the guest is using > at the time they define the guest. This is an undesirable restriction > for use cases where the admin of the guest OS has no direct control > over the host config. > > IOW, I think the only practical answer is to have a single binary that > automagically does the right thing at runtime according to guest > endianess that currently is in use. > The problem is that AFAIK there is no way to figure out what is the right thing at the moment. I guess QEMU should now, but not virtiofsd or whatever userspace virtio device. But I will double check if there is something in the protocol addressing this. Regards, Halil
On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote: > This RFC is about enabling virtio-fs on s390x. For that we need > + some shim code (first patch), and we need > + libvhost-user to deal with virtio endiannes as mandated by the spec. > > The second part is trickier, because unlike QEMU we are not certain > about the guest's native endianness, which is needed to handle the > legacy-interface appropriately. In fact, this is the reason why just > RFC. > > One of the open questions is whether to build separate versions, one > for guest little endian and one for guest big endian, or do we want > something like a command line option? (Digression on the libvirt > modeling) > > A third option would be to refuse legacy altogether. I suggest the following: 1. Combinations that worked with libvhost-user in the past must not break. 2. New combinations should only support VIRTIO 1.0 and later. This means continue to allow Legacy mode devices where they already run today but don't add new code for the cases that didn't work. Stefan
On Mon, Jun 29, 2020 at 01:53:05PM +0100, Stefan Hajnoczi wrote: > On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote: > > This RFC is about enabling virtio-fs on s390x. For that we need > > + some shim code (first patch), and we need > > + libvhost-user to deal with virtio endiannes as mandated by the spec. > > > > The second part is trickier, because unlike QEMU we are not certain > > about the guest's native endianness, which is needed to handle the > > legacy-interface appropriately. In fact, this is the reason why just > > RFC. > > > > One of the open questions is whether to build separate versions, one > > for guest little endian and one for guest big endian, or do we want > > something like a command line option? (Digression on the libvirt > > modeling) > > > > A third option would be to refuse legacy altogether. > > I suggest the following: > > 1. Combinations that worked with libvhost-user in the past must not break. > > 2. New combinations should only support VIRTIO 1.0 and later. > > This means continue to allow Legacy mode devices where they already run > today but don't add new code for the cases that didn't work. What I'm missing here is what PCI product ID was being used when the current impl is in legacy/transitional mode ? Normally legacy and transitional mode devices need an explicit PCI ID reserved, where as modern-only devices have a PCI ID derived from their VirtIO ID + a fixed offset. Was this mistakenly using a VirtIO ID + fixed offset for the legacy mode too ? Regards, Daniel
On Mon, Jun 29, 2020 at 02:07:16PM +0100, Daniel P. Berrangé wrote: > On Mon, Jun 29, 2020 at 01:53:05PM +0100, Stefan Hajnoczi wrote: > > On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote: > > > This RFC is about enabling virtio-fs on s390x. For that we need > > > + some shim code (first patch), and we need > > > + libvhost-user to deal with virtio endiannes as mandated by the spec. > > > > > > The second part is trickier, because unlike QEMU we are not certain > > > about the guest's native endianness, which is needed to handle the > > > legacy-interface appropriately. In fact, this is the reason why just > > > RFC. > > > > > > One of the open questions is whether to build separate versions, one > > > for guest little endian and one for guest big endian, or do we want > > > something like a command line option? (Digression on the libvirt > > > modeling) > > > > > > A third option would be to refuse legacy altogether. > > > > I suggest the following: > > > > 1. Combinations that worked with libvhost-user in the past must not break. > > > > 2. New combinations should only support VIRTIO 1.0 and later. > > > > This means continue to allow Legacy mode devices where they already run > > today but don't add new code for the cases that didn't work. > > What I'm missing here is what PCI product ID was being used when the > current impl is in legacy/transitional mode ? > > Normally legacy and transitional mode devices need an explicit PCI ID > reserved, where as modern-only devices have a PCI ID derived from their > VirtIO ID + a fixed offset. > > Was this mistakenly using a VirtIO ID + fixed offset for the legacy > mode too ? vhost-user-fs-pci does not support Legacy or Transitional mode. See hw/virtio/vhost-user-fs-pci.c: static const VirtioPCIDeviceTypeInfo vhost_user_fs_pci_info = { .base_name = TYPE_VHOST_USER_FS_PCI, .non_transitional_name = "vhost-user-fs-pci", ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ .instance_size = sizeof(VHostUserFSPCI), .instance_init = vhost_user_fs_pci_instance_init, .class_init = vhost_user_fs_pci_class_init, }; Stefan
On Tue, 30 Jun 2020 10:04:51 +0100 Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Mon, Jun 29, 2020 at 02:07:16PM +0100, Daniel P. Berrangé wrote: > > On Mon, Jun 29, 2020 at 01:53:05PM +0100, Stefan Hajnoczi wrote: > > > On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote: > > > > This RFC is about enabling virtio-fs on s390x. For that we need > > > > + some shim code (first patch), and we need > > > > + libvhost-user to deal with virtio endiannes as mandated by the spec. > > > > > > > > The second part is trickier, because unlike QEMU we are not certain > > > > about the guest's native endianness, which is needed to handle the > > > > legacy-interface appropriately. In fact, this is the reason why just > > > > RFC. > > > > > > > > One of the open questions is whether to build separate versions, one > > > > for guest little endian and one for guest big endian, or do we want > > > > something like a command line option? (Digression on the libvirt > > > > modeling) > > > > > > > > A third option would be to refuse legacy altogether. > > > > > > I suggest the following: > > > > > > 1. Combinations that worked with libvhost-user in the past must not break. > > > > > > 2. New combinations should only support VIRTIO 1.0 and later. > > > > > > This means continue to allow Legacy mode devices where they already run > > > today but don't add new code for the cases that didn't work. > > > > What I'm missing here is what PCI product ID was being used when the > > current impl is in legacy/transitional mode ? > > > > Normally legacy and transitional mode devices need an explicit PCI ID > > reserved, where as modern-only devices have a PCI ID derived from their > > VirtIO ID + a fixed offset. > > > > Was this mistakenly using a VirtIO ID + fixed offset for the legacy > > mode too ? > > vhost-user-fs-pci does not support Legacy or Transitional mode. See > hw/virtio/vhost-user-fs-pci.c: > > static const VirtioPCIDeviceTypeInfo vhost_user_fs_pci_info = { > .base_name = TYPE_VHOST_USER_FS_PCI, > .non_transitional_name = "vhost-user-fs-pci", > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > .instance_size = sizeof(VHostUserFSPCI), > .instance_init = vhost_user_fs_pci_instance_init, > .class_init = vhost_user_fs_pci_class_init, > }; This makes it very unlikely that someone accidentally configures non-modern, but does not prevent it AFAICS. See <20200630113527.7b27f34f.cohuck@redhat.com>, which I just sent. (I may be off, because that is all very confusing...)
On Tue, Jun 30, 2020 at 11:39:32AM +0200, Cornelia Huck wrote: > On Tue, 30 Jun 2020 10:04:51 +0100 > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > On Mon, Jun 29, 2020 at 02:07:16PM +0100, Daniel P. Berrangé wrote: > > > On Mon, Jun 29, 2020 at 01:53:05PM +0100, Stefan Hajnoczi wrote: > > > > On Thu, Jun 25, 2020 at 12:04:26PM +0200, Marc Hartmayer wrote: > > > > > This RFC is about enabling virtio-fs on s390x. For that we need > > > > > + some shim code (first patch), and we need > > > > > + libvhost-user to deal with virtio endiannes as mandated by the spec. > > > > > > > > > > The second part is trickier, because unlike QEMU we are not certain > > > > > about the guest's native endianness, which is needed to handle the > > > > > legacy-interface appropriately. In fact, this is the reason why just > > > > > RFC. > > > > > > > > > > One of the open questions is whether to build separate versions, one > > > > > for guest little endian and one for guest big endian, or do we want > > > > > something like a command line option? (Digression on the libvirt > > > > > modeling) > > > > > > > > > > A third option would be to refuse legacy altogether. > > > > > > > > I suggest the following: > > > > > > > > 1. Combinations that worked with libvhost-user in the past must not break. > > > > > > > > 2. New combinations should only support VIRTIO 1.0 and later. > > > > > > > > This means continue to allow Legacy mode devices where they already run > > > > today but don't add new code for the cases that didn't work. > > > > > > What I'm missing here is what PCI product ID was being used when the > > > current impl is in legacy/transitional mode ? > > > > > > Normally legacy and transitional mode devices need an explicit PCI ID > > > reserved, where as modern-only devices have a PCI ID derived from their > > > VirtIO ID + a fixed offset. > > > > > > Was this mistakenly using a VirtIO ID + fixed offset for the legacy > > > mode too ? > > > > vhost-user-fs-pci does not support Legacy or Transitional mode. See > > hw/virtio/vhost-user-fs-pci.c: > > > > static const VirtioPCIDeviceTypeInfo vhost_user_fs_pci_info = { > > .base_name = TYPE_VHOST_USER_FS_PCI, > > .non_transitional_name = "vhost-user-fs-pci", > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > .instance_size = sizeof(VHostUserFSPCI), > > .instance_init = vhost_user_fs_pci_instance_init, > > .class_init = vhost_user_fs_pci_class_init, > > }; > > This makes it very unlikely that someone accidentally configures > non-modern, but does not prevent it AFAICS. See > <20200630113527.7b27f34f.cohuck@redhat.com>, which I just sent. > > (I may be off, because that is all very confusing...) Right. We'll block legacy for modern only devices going forward. Going back to the patchset in question, virtio-fs is modern only, legacy will not be supported.