| Message ID | CAGxU2F7pVNWtJG2BM2bk9qtJ_UHgDw4kjVqRmL-=yme7VX83Vg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-vsock requires 'disable-legacy=on' in QEMU 5.1 | expand |
On Thu, 13 Aug 2020 11:16:56 +0200 Stefano Garzarella <sgarzare@redhat.com> wrote: > Hi, > > Qinghua discovered that virtio-vsock-pci requires 'disable-legacy=on' in > QEMU 5.1: > $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5 > qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5: > device is modern-only, use disable-legacy=on > > Bisecting I found that this behaviour starts from this commit: > 9b3a35ec82 ("virtio: verify that legacy support is not accidentally on") Oh, I had heard that from others already, was still trying to figure out what to do. > > IIUC virtio-vsock is modern-only, so I tried this patch and it works: > > diff --git a/hw/virtio/vhost-user-vsock-pci.c b/hw/virtio/vhost-user-vsock-pci.c > index f4cf95873d..6e4cc874cd 100644 > --- a/hw/virtio/vhost-user-vsock-pci.c > +++ b/hw/virtio/vhost-user-vsock-pci.c > @@ -40,6 +40,7 @@ static void vhost_user_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > VHostUserVSockPCI *dev = VHOST_USER_VSOCK_PCI(vpci_dev); > DeviceState *vdev = DEVICE(&dev->vdev); > > + virtio_pci_force_virtio_1(vpci_dev); > qdev_realize(vdev, BUS(&vpci_dev->bus), errp); > } > > diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c > index a815278e69..f641b974e9 100644 > --- a/hw/virtio/vhost-vsock-pci.c > +++ b/hw/virtio/vhost-vsock-pci.c > @@ -44,6 +44,7 @@ static void vhost_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > VHostVSockPCI *dev = VHOST_VSOCK_PCI(vpci_dev); > DeviceState *vdev = DEVICE(&dev->vdev); > > + virtio_pci_force_virtio_1(vpci_dev); > qdev_realize(vdev, BUS(&vpci_dev->bus), errp); > } > > > Do you think this is the right approach or is there a better way to > solve this issue? We basically have three possible ways to deal with this: - Force it to modern (i.e., what you have been doing; would need the equivalent changes in ccw as well.) Pro: looks like the cleanest approach. Con: not sure if we would need backwards compatibility support, which looks hairy. - Add vsock to the list of devices with legacy support. Pro: Existing setups continue to work. Con: If vsock is really virtio-1-only, we still carry around possibly broken legacy support. - Do nothing, have users force legacy off. Bad idea, as ccw has no way to do that on the command line. The first option is probably best.
On Thu, Aug 13, 2020 at 11:28:20AM +0200, Cornelia Huck wrote: > On Thu, 13 Aug 2020 11:16:56 +0200 > Stefano Garzarella <sgarzare@redhat.com> wrote: > > > Hi, > > > > Qinghua discovered that virtio-vsock-pci requires 'disable-legacy=on' in > > QEMU 5.1: > > $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5 > > qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5: > > device is modern-only, use disable-legacy=on > > > > Bisecting I found that this behaviour starts from this commit: > > 9b3a35ec82 ("virtio: verify that legacy support is not accidentally on") > > Oh, I had heard that from others already, was still trying to figure > out what to do. > > > > > IIUC virtio-vsock is modern-only, so I tried this patch and it works: > > > > diff --git a/hw/virtio/vhost-user-vsock-pci.c b/hw/virtio/vhost-user-vsock-pci.c > > index f4cf95873d..6e4cc874cd 100644 > > --- a/hw/virtio/vhost-user-vsock-pci.c > > +++ b/hw/virtio/vhost-user-vsock-pci.c > > @@ -40,6 +40,7 @@ static void vhost_user_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > > VHostUserVSockPCI *dev = VHOST_USER_VSOCK_PCI(vpci_dev); > > DeviceState *vdev = DEVICE(&dev->vdev); > > > > + virtio_pci_force_virtio_1(vpci_dev); > > qdev_realize(vdev, BUS(&vpci_dev->bus), errp); > > } > > > > diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c > > index a815278e69..f641b974e9 100644 > > --- a/hw/virtio/vhost-vsock-pci.c > > +++ b/hw/virtio/vhost-vsock-pci.c > > @@ -44,6 +44,7 @@ static void vhost_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > > VHostVSockPCI *dev = VHOST_VSOCK_PCI(vpci_dev); > > DeviceState *vdev = DEVICE(&dev->vdev); > > > > + virtio_pci_force_virtio_1(vpci_dev); > > qdev_realize(vdev, BUS(&vpci_dev->bus), errp); > > } > > > > > > Do you think this is the right approach or is there a better way to > > solve this issue? > > We basically have three possible ways to deal with this: > > - Force it to modern (i.e., what you have been doing; would need the > equivalent changes in ccw as well.) Oo, thanks for pointing out ccw! I don't know ccw well, in this case should we set dev->max_rev to 1 or 2 to force to modern? > Pro: looks like the cleanest approach. > Con: not sure if we would need backwards compatibility support, > which looks hairy. Not sure too. > - Add vsock to the list of devices with legacy support. > Pro: Existing setups continue to work. > Con: If vsock is really virtio-1-only, we still carry around > possibly broken legacy support. I'm not sure it is virtio-1-only, but virtio-vsock was introduced in 2016, so I supposed it is modern-only. How can I verify that? Maybe forcing legacy mode and run some tests. > - Do nothing, have users force legacy off. Bad idea, as ccw has no way > to do that on the command line. > > The first option is probably best. > Yeah, I agree with you! Thanks, Stefano
On Thu, 13 Aug 2020 12:24:30 +0200 Stefano Garzarella <sgarzare@redhat.com> wrote: > On Thu, Aug 13, 2020 at 11:28:20AM +0200, Cornelia Huck wrote: > > On Thu, 13 Aug 2020 11:16:56 +0200 > > Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > Hi, > > > > > > Qinghua discovered that virtio-vsock-pci requires 'disable-legacy=on' in > > > QEMU 5.1: > > > $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5 > > > qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5: > > > device is modern-only, use disable-legacy=on > > > > > > Bisecting I found that this behaviour starts from this commit: > > > 9b3a35ec82 ("virtio: verify that legacy support is not accidentally on") > > > > Oh, I had heard that from others already, was still trying to figure > > out what to do. > > > > > > > > IIUC virtio-vsock is modern-only, so I tried this patch and it works: > > > > > > diff --git a/hw/virtio/vhost-user-vsock-pci.c b/hw/virtio/vhost-user-vsock-pci.c > > > index f4cf95873d..6e4cc874cd 100644 > > > --- a/hw/virtio/vhost-user-vsock-pci.c > > > +++ b/hw/virtio/vhost-user-vsock-pci.c > > > @@ -40,6 +40,7 @@ static void vhost_user_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > > > VHostUserVSockPCI *dev = VHOST_USER_VSOCK_PCI(vpci_dev); > > > DeviceState *vdev = DEVICE(&dev->vdev); > > > > > > + virtio_pci_force_virtio_1(vpci_dev); > > > qdev_realize(vdev, BUS(&vpci_dev->bus), errp); > > > } > > > > > > diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c > > > index a815278e69..f641b974e9 100644 > > > --- a/hw/virtio/vhost-vsock-pci.c > > > +++ b/hw/virtio/vhost-vsock-pci.c > > > @@ -44,6 +44,7 @@ static void vhost_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > > > VHostVSockPCI *dev = VHOST_VSOCK_PCI(vpci_dev); > > > DeviceState *vdev = DEVICE(&dev->vdev); > > > > > > + virtio_pci_force_virtio_1(vpci_dev); > > > qdev_realize(vdev, BUS(&vpci_dev->bus), errp); > > > } > > > > > > > > > Do you think this is the right approach or is there a better way to > > > solve this issue? > > > > We basically have three possible ways to deal with this: > > > > - Force it to modern (i.e., what you have been doing; would need the > > equivalent changes in ccw as well.) > > Oo, thanks for pointing out ccw! > I don't know ccw well, in this case should we set dev->max_rev to 1 or 2 > to force to modern? No, ->max_rev is the wrong side of the limit :) You want ccw_dev->force_revision_1 = true; in _instance_init() (see e.g. virtio-ccw-gpu.c). > > > Pro: looks like the cleanest approach. > > Con: not sure if we would need backwards compatibility support, > > which looks hairy. > > Not sure too. Yes, I'm not sure at all how to handle user-specified values for legacy/modern. > > > - Add vsock to the list of devices with legacy support. > > Pro: Existing setups continue to work. > > Con: If vsock is really virtio-1-only, we still carry around > > possibly broken legacy support. > > I'm not sure it is virtio-1-only, but virtio-vsock was introduced in > 2016, so I supposed it is modern-only. Yes, I would guess so as well. > > How can I verify that? Maybe forcing legacy mode and run some tests. Probably yes. The likeliest area with issues is probably endianness, so maybe with something big endian in the mix? > > > - Do nothing, have users force legacy off. Bad idea, as ccw has no way > > to do that on the command line. > > > > The first option is probably best. > > > > Yeah, I agree with you! Yes, it's really a pity we only noticed this after the release; this was supposed to stop new devices with legacy support creeping in, not to break existing command lines :(
On Thu, Aug 13, 2020 at 12:37:37PM +0200, Cornelia Huck wrote: > On Thu, 13 Aug 2020 12:24:30 +0200 > Stefano Garzarella <sgarzare@redhat.com> wrote: > > > On Thu, Aug 13, 2020 at 11:28:20AM +0200, Cornelia Huck wrote: > > > On Thu, 13 Aug 2020 11:16:56 +0200 > > > Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > > Hi, > > > > > > > > Qinghua discovered that virtio-vsock-pci requires 'disable-legacy=on' in > > > > QEMU 5.1: > > > > $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5 > > > > qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5: > > > > device is modern-only, use disable-legacy=on > > > > > > > > Bisecting I found that this behaviour starts from this commit: > > > > 9b3a35ec82 ("virtio: verify that legacy support is not accidentally on") > > > > > > Oh, I had heard that from others already, was still trying to figure > > > out what to do. > > > > > > > > > > > IIUC virtio-vsock is modern-only, so I tried this patch and it works: > > > > > > > > diff --git a/hw/virtio/vhost-user-vsock-pci.c b/hw/virtio/vhost-user-vsock-pci.c > > > > index f4cf95873d..6e4cc874cd 100644 > > > > --- a/hw/virtio/vhost-user-vsock-pci.c > > > > +++ b/hw/virtio/vhost-user-vsock-pci.c > > > > @@ -40,6 +40,7 @@ static void vhost_user_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > > > > VHostUserVSockPCI *dev = VHOST_USER_VSOCK_PCI(vpci_dev); > > > > DeviceState *vdev = DEVICE(&dev->vdev); > > > > > > > > + virtio_pci_force_virtio_1(vpci_dev); > > > > qdev_realize(vdev, BUS(&vpci_dev->bus), errp); > > > > } > > > > > > > > diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c > > > > index a815278e69..f641b974e9 100644 > > > > --- a/hw/virtio/vhost-vsock-pci.c > > > > +++ b/hw/virtio/vhost-vsock-pci.c > > > > @@ -44,6 +44,7 @@ static void vhost_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > > > > VHostVSockPCI *dev = VHOST_VSOCK_PCI(vpci_dev); > > > > DeviceState *vdev = DEVICE(&dev->vdev); > > > > > > > > + virtio_pci_force_virtio_1(vpci_dev); > > > > qdev_realize(vdev, BUS(&vpci_dev->bus), errp); > > > > } > > > > > > > > > > > > Do you think this is the right approach or is there a better way to > > > > solve this issue? > > > > > > We basically have three possible ways to deal with this: > > > > > > - Force it to modern (i.e., what you have been doing; would need the > > > equivalent changes in ccw as well.) > > > > Oo, thanks for pointing out ccw! > > I don't know ccw well, in this case should we set dev->max_rev to 1 or 2 > > to force to modern? > > No, ->max_rev is the wrong side of the limit :) You want Well :-) Thanks! > > ccw_dev->force_revision_1 = true; > > in _instance_init() (see e.g. virtio-ccw-gpu.c). > > > > > > Pro: looks like the cleanest approach. > > > Con: not sure if we would need backwards compatibility support, > > > which looks hairy. > > > > Not sure too. > > Yes, I'm not sure at all how to handle user-specified values for > legacy/modern. > > > > > > - Add vsock to the list of devices with legacy support. > > > Pro: Existing setups continue to work. > > > Con: If vsock is really virtio-1-only, we still carry around > > > possibly broken legacy support. > > > > I'm not sure it is virtio-1-only, but virtio-vsock was introduced in > > 2016, so I supposed it is modern-only. > > Yes, I would guess so as well. > > > > > How can I verify that? Maybe forcing legacy mode and run some tests. > > Probably yes. The likeliest area with issues is probably endianness, so > maybe with something big endian in the mix? > Yeah, I'll try this setup! > > > > > - Do nothing, have users force legacy off. Bad idea, as ccw has no way > > > to do that on the command line. > > > > > > The first option is probably best. > > > > > > > Yeah, I agree with you! > > Yes, it's really a pity we only noticed this after the release; this > was supposed to stop new devices with legacy support creeping in, not > to break existing command lines :( > Yes, I forgot to test vsock stuff before the release :-( Maybe we should add some tests... Thanks, Stefano
Patchew URL: https://patchew.org/QEMU/CAGxU2F7pVNWtJG2BM2bk9qtJ_UHgDw4kjVqRmL-=yme7VX83Vg@mail.gmail.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: CAGxU2F7pVNWtJG2BM2bk9qtJ_UHgDw4kjVqRmL-=yme7VX83Vg@mail.gmail.com Subject: virtio-vsock requires 'disable-legacy=on' in QEMU 5.1 === 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 === Switched to a new branch 'test' c6ee8c6 virtio-vsock requires 'disable-legacy=on' in QEMU 5.1 === OUTPUT BEGIN === ERROR: Missing Signed-off-by: line(s) total: 1 errors, 0 warnings, 14 lines checked Commit c6ee8c6d364d (virtio-vsock requires 'disable-legacy=on' in QEMU 5.1) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/CAGxU2F7pVNWtJG2BM2bk9qtJ_UHgDw4kjVqRmL-=yme7VX83Vg@mail.gmail.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On Thu, 13 Aug 2020 14:04:15 +0200 Stefano Garzarella <sgarzare@redhat.com> wrote: > On Thu, Aug 13, 2020 at 12:37:37PM +0200, Cornelia Huck wrote: > > On Thu, 13 Aug 2020 12:24:30 +0200 > > Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > On Thu, Aug 13, 2020 at 11:28:20AM +0200, Cornelia Huck wrote: > > > > We basically have three possible ways to deal with this: > > > > > > > > - Force it to modern (i.e., what you have been doing; would need the > > > > equivalent changes in ccw as well.) > > > > > > Oo, thanks for pointing out ccw! > > > I don't know ccw well, in this case should we set dev->max_rev to 1 or 2 > > > to force to modern? > > > > No, ->max_rev is the wrong side of the limit :) You want > > Well :-) Thanks! > > > > > ccw_dev->force_revision_1 = true; > > > > in _instance_init() (see e.g. virtio-ccw-gpu.c). > > > > > > > > > Pro: looks like the cleanest approach. > > > > Con: not sure if we would need backwards compatibility support, > > > > which looks hairy. > > > > > > Not sure too. > > > > Yes, I'm not sure at all how to handle user-specified values for > > legacy/modern. Thinking a bit more about it, I'm not sure whether we even *can* provide backwards compatibility: we have different autoconfigurations for PCI based upon where it is plugged, and ccw does not have a way to turn legacy on/off, except from within the code. > > > > > > > > > - Add vsock to the list of devices with legacy support. > > > > Pro: Existing setups continue to work. > > > > Con: If vsock is really virtio-1-only, we still carry around > > > > possibly broken legacy support. > > > > > > I'm not sure it is virtio-1-only, but virtio-vsock was introduced in > > > 2016, so I supposed it is modern-only. > > > > Yes, I would guess so as well. > > > > > > > > How can I verify that? Maybe forcing legacy mode and run some tests. > > > > Probably yes. The likeliest area with issues is probably endianness, so > > maybe with something big endian in the mix? > > > > Yeah, I'll try this setup! > > > > > > > > - Do nothing, have users force legacy off. Bad idea, as ccw has no way > > > > to do that on the command line. > > > > > > > > The first option is probably best. The first option is now "force modern, but with no backwards compatibility", which is not that great; but "allow legacy, even though it should not exist" is not particularly appealing, either... what a mess :( > > > > > > > > > > Yeah, I agree with you! > > > > Yes, it's really a pity we only noticed this after the release; this > > was supposed to stop new devices with legacy support creeping in, not > > to break existing command lines :( > > > > Yes, I forgot to test vsock stuff before the release :-( > Maybe we should add some tests... Speaking of tests: do you have a quick way to test vhost-vsock at hand? Maybe I should add it to my manual repertoire...
On Mon, Aug 17, 2020 at 12:27:46PM +0200, Cornelia Huck wrote: > On Thu, 13 Aug 2020 14:04:15 +0200 > Stefano Garzarella <sgarzare@redhat.com> wrote: > > > On Thu, Aug 13, 2020 at 12:37:37PM +0200, Cornelia Huck wrote: > > > On Thu, 13 Aug 2020 12:24:30 +0200 > > > Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > > On Thu, Aug 13, 2020 at 11:28:20AM +0200, Cornelia Huck wrote: > > > > > We basically have three possible ways to deal with this: > > > > > > > > > > - Force it to modern (i.e., what you have been doing; would need the > > > > > equivalent changes in ccw as well.) > > > > > > > > Oo, thanks for pointing out ccw! > > > > I don't know ccw well, in this case should we set dev->max_rev to 1 or 2 > > > > to force to modern? > > > > > > No, ->max_rev is the wrong side of the limit :) You want > > > > Well :-) Thanks! > > > > > > > > ccw_dev->force_revision_1 = true; > > > > > > in _instance_init() (see e.g. virtio-ccw-gpu.c). > > > > > > > > > > > > Pro: looks like the cleanest approach. > > > > > Con: not sure if we would need backwards compatibility support, > > > > > which looks hairy. > > > > > > > > Not sure too. > > > > > > Yes, I'm not sure at all how to handle user-specified values for > > > legacy/modern. > > Thinking a bit more about it, I'm not sure whether we even *can* > provide backwards compatibility: we have different autoconfigurations > for PCI based upon where it is plugged, and ccw does not have a way to > turn legacy on/off, except from within the code. Yes, I discovered today for example that the PCIe bus set auto-legacy mode to off. > > > > > > > > > > > > > - Add vsock to the list of devices with legacy support. > > > > > Pro: Existing setups continue to work. > > > > > Con: If vsock is really virtio-1-only, we still carry around > > > > > possibly broken legacy support. > > > > > > > > I'm not sure it is virtio-1-only, but virtio-vsock was introduced in > > > > 2016, so I supposed it is modern-only. > > > > > > Yes, I would guess so as well. > > > > > > > > > > > How can I verify that? Maybe forcing legacy mode and run some tests. > > > > > > Probably yes. The likeliest area with issues is probably endianness, so > > > maybe with something big endian in the mix? > > > > > > > Yeah, I'll try this setup! > > > > > > > > > > > - Do nothing, have users force legacy off. Bad idea, as ccw has no way > > > > > to do that on the command line. > > > > > > > > > > The first option is probably best. > > The first option is now "force modern, but with no backwards > compatibility", which is not that great; but "allow legacy, even though > it should not exist" is not particularly appealing, either... what a > mess :( Yeah, it's a mess :-( anyway I still prefer option 1, it seems a little bit more correct to me. > > > > > > > > > > > > > > Yeah, I agree with you! > > > > > > Yes, it's really a pity we only noticed this after the release; this > > > was supposed to stop new devices with legacy support creeping in, not > > > to break existing command lines :( > > > > > > > Yes, I forgot to test vsock stuff before the release :-( > > Maybe we should add some tests... > > Speaking of tests: do you have a quick way to test vhost-vsock at hand? > Maybe I should add it to my manual repertoire... > Sure, maybe the quickest way is to use ncat. Starting from version 7.80, it supports AF_VSOCK sockets: host$ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5 host$ ncat --vsock -l 1234 # vsock address is <cid, port>, cid=2 is used always to reach the host guest$ ncat --vsock 2 1234 Other tests that I usually run are: - iperf-vsock: https://github.com/stefano-garzarella/iperf-vsock - vsock test suite in the Linux kernel (tools/testing/vsock) Let me know if you want more details on these :-) Thanks, Stefano
On Mon, 17 Aug 2020 15:11:28 +0200 Stefano Garzarella <sgarzare@redhat.com> wrote: > On Mon, Aug 17, 2020 at 12:27:46PM +0200, Cornelia Huck wrote: > > Speaking of tests: do you have a quick way to test vhost-vsock at hand? > > Maybe I should add it to my manual repertoire... > > > > Sure, maybe the quickest way is to use ncat. Starting from version 7.80, > it supports AF_VSOCK sockets: > > host$ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5 > > host$ ncat --vsock -l 1234 > > # vsock address is <cid, port>, cid=2 is used always to reach the host > guest$ ncat --vsock 2 1234 > > Other tests that I usually run are: > - iperf-vsock: https://github.com/stefano-garzarella/iperf-vsock > - vsock test suite in the Linux kernel (tools/testing/vsock) > > Let me know if you want more details on these :-) Thanks, simply doing some smoke tests with ncat should be enough :)
On Mon, 17 Aug 2020 15:11:28 +0200 Stefano Garzarella <sgarzare@redhat.com> wrote: > On Mon, Aug 17, 2020 at 12:27:46PM +0200, Cornelia Huck wrote: > > On Thu, 13 Aug 2020 14:04:15 +0200 > > Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > On Thu, Aug 13, 2020 at 12:37:37PM +0200, Cornelia Huck wrote: > > > > On Thu, 13 Aug 2020 12:24:30 +0200 > > > > Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > > > > On Thu, Aug 13, 2020 at 11:28:20AM +0200, Cornelia Huck wrote: > > > > > > We basically have three possible ways to deal with this: > > > > > > > > > > > > - Force it to modern (i.e., what you have been doing; would need the > > > > > > equivalent changes in ccw as well.) > > > > > > > > > > Oo, thanks for pointing out ccw! > > > > > I don't know ccw well, in this case should we set dev->max_rev to 1 or 2 > > > > > to force to modern? > > > > > > > > No, ->max_rev is the wrong side of the limit :) You want > > > > > > Well :-) Thanks! > > > > > > > > > > > ccw_dev->force_revision_1 = true; > > > > > > > > in _instance_init() (see e.g. virtio-ccw-gpu.c). > > > > > > > > > > > > > > > Pro: looks like the cleanest approach. > > > > > > Con: not sure if we would need backwards compatibility support, > > > > > > which looks hairy. > > > > > > > > > > Not sure too. > > > > > > > > Yes, I'm not sure at all how to handle user-specified values for > > > > legacy/modern. > > > > Thinking a bit more about it, I'm not sure whether we even *can* > > provide backwards compatibility: we have different autoconfigurations > > for PCI based upon where it is plugged, and ccw does not have a way to > > turn legacy on/off, except from within the code. > > Yes, I discovered today for example that the PCIe bus set auto-legacy > mode to off. And vhost-vsock actually really seems to be modern-only, see below. > > > > > > > > > > > > > > > > > > - Add vsock to the list of devices with legacy support. > > > > > > Pro: Existing setups continue to work. > > > > > > Con: If vsock is really virtio-1-only, we still carry around > > > > > > possibly broken legacy support. > > > > > > > > > > I'm not sure it is virtio-1-only, but virtio-vsock was introduced in > > > > > 2016, so I supposed it is modern-only. > > > > > > > > Yes, I would guess so as well. > > > > > > > > > > > > > > How can I verify that? Maybe forcing legacy mode and run some tests. > > > > > > > > Probably yes. The likeliest area with issues is probably endianness, so > > > > maybe with something big endian in the mix? > > > > > > > > > > Yeah, I'll try this setup! Ok, I tried this now with an x86 host and an s390x guest. Reverted the checking commit, tried both with a -ccw and a -pci device and your ncat example. - When using virtio-1, both devices work fine. - When using the -pci device with disable-modern=yes, I get "reset by peer". - When using the -ccw device with max_revision=0, I get an instant timeout. Smells like endianness problems (aka weird things are happening). Also noticed that vhost-vsock-ccw does not have an immediate problem, even with the commit: The code only checks whether the device has been forced to legacy, not whether legacy is allowed (which cannot be controlled by the user anyway). Probably best to address after we've dealt with the vhost-vsock issue and made sure that there are no other problems. > > > > > > > > > > > > > > - Do nothing, have users force legacy off. Bad idea, as ccw has no way > > > > > > to do that on the command line. > > > > > > > > > > > > The first option is probably best. > > > > The first option is now "force modern, but with no backwards > > compatibility", which is not that great; but "allow legacy, even though > > it should not exist" is not particularly appealing, either... what a > > mess :( > > Yeah, it's a mess :-( anyway I still prefer option 1, it seems a little > bit more correct to me. It seems to me that the status before this was "works by accident, but only if we're not negotiating to legacy, or the guest/host are both little endian". IOW, no visible breakage for most people (or we'd probably have heard of it already). Now we have a setup that's correct, but forces users to adapt their QEMU command lines. Option 1 would eliminate the need to do that, but would cause possibly not-really-fixable migration issues (you can probably deal with that manually, detaching and re-attaching the device as a last resort.) So, force modern, probably also remove the -transitional device type, and put a prominent explanation into the change log?
On Tue, Aug 18, 2020 at 02:44:50PM +0200, Cornelia Huck wrote: > On Mon, 17 Aug 2020 15:11:28 +0200 > Stefano Garzarella <sgarzare@redhat.com> wrote: > > > On Mon, Aug 17, 2020 at 12:27:46PM +0200, Cornelia Huck wrote: > > > On Thu, 13 Aug 2020 14:04:15 +0200 > > > Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > > On Thu, Aug 13, 2020 at 12:37:37PM +0200, Cornelia Huck wrote: > > > > > On Thu, 13 Aug 2020 12:24:30 +0200 > > > > > Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > > > > > > > > On Thu, Aug 13, 2020 at 11:28:20AM +0200, Cornelia Huck wrote: > > > > > > > We basically have three possible ways to deal with this: > > > > > > > > > > > > > > - Force it to modern (i.e., what you have been doing; would need the > > > > > > > equivalent changes in ccw as well.) > > > > > > > > > > > > Oo, thanks for pointing out ccw! > > > > > > I don't know ccw well, in this case should we set dev->max_rev to 1 or 2 > > > > > > to force to modern? > > > > > > > > > > No, ->max_rev is the wrong side of the limit :) You want > > > > > > > > Well :-) Thanks! > > > > > > > > > > > > > > ccw_dev->force_revision_1 = true; > > > > > > > > > > in _instance_init() (see e.g. virtio-ccw-gpu.c). > > > > > > > > > > > > > > > > > > Pro: looks like the cleanest approach. > > > > > > > Con: not sure if we would need backwards compatibility support, > > > > > > > which looks hairy. > > > > > > > > > > > > Not sure too. > > > > > > > > > > Yes, I'm not sure at all how to handle user-specified values for > > > > > legacy/modern. > > > > > > Thinking a bit more about it, I'm not sure whether we even *can* > > > provide backwards compatibility: we have different autoconfigurations > > > for PCI based upon where it is plugged, and ccw does not have a way to > > > turn legacy on/off, except from within the code. > > > > Yes, I discovered today for example that the PCIe bus set auto-legacy > > mode to off. > > And vhost-vsock actually really seems to be modern-only, see below. > > > > > > > > > > > > > > > > > > > > > > > > - Add vsock to the list of devices with legacy support. > > > > > > > Pro: Existing setups continue to work. > > > > > > > Con: If vsock is really virtio-1-only, we still carry around > > > > > > > possibly broken legacy support. > > > > > > > > > > > > I'm not sure it is virtio-1-only, but virtio-vsock was introduced in > > > > > > 2016, so I supposed it is modern-only. > > > > > > > > > > Yes, I would guess so as well. > > > > > > > > > > > > > > > > > How can I verify that? Maybe forcing legacy mode and run some tests. > > > > > > > > > > Probably yes. The likeliest area with issues is probably endianness, so > > > > > maybe with something big endian in the mix? > > > > > > > > > > > > > Yeah, I'll try this setup! > > Ok, I tried this now with an x86 host and an s390x guest. Reverted the > checking commit, tried both with a -ccw and a -pci device and your ncat > example. > - When using virtio-1, both devices work fine. > - When using the -pci device with disable-modern=yes, I get "reset by > peer". > - When using the -ccw device with max_revision=0, I get an instant > timeout. Great, thanks for testing! > > Smells like endianness problems (aka weird things are happening). Yeah. > > Also noticed that vhost-vsock-ccw does not have an immediate problem, > even with the commit: The code only checks whether the device has been > forced to legacy, not whether legacy is allowed (which cannot be > controlled by the user anyway). Probably best to address after we've > dealt with the vhost-vsock issue and made sure that there are no other > problems. Make sense! > > > > > > > > > > > > > > > > > > - Do nothing, have users force legacy off. Bad idea, as ccw has no way > > > > > > > to do that on the command line. > > > > > > > > > > > > > > The first option is probably best. > > > > > > The first option is now "force modern, but with no backwards > > > compatibility", which is not that great; but "allow legacy, even though > > > it should not exist" is not particularly appealing, either... what a > > > mess :( > > > > Yeah, it's a mess :-( anyway I still prefer option 1, it seems a little > > bit more correct to me. > > It seems to me that the status before this was "works by accident, but > only if we're not negotiating to legacy, or the guest/host are both > little endian". IOW, no visible breakage for most people (or we'd > probably have heard of it already). Now we have a setup that's correct, > but forces users to adapt their QEMU command lines. Option 1 would > eliminate the need to do that, but would cause possibly > not-really-fixable migration issues (you can probably deal with that > manually, detaching and re-attaching the device as a last resort.) > > So, force modern, probably also remove the -transitional device type, > and put a prominent explanation into the change log? > I completely agree with your analysis and solution. So, for now we need to patch vhost-vsock-pci and vhost-user-vsock-pci, and queue the patches in stable. Do you prefer to send them? Otherwise I can do that. Thanks again for the help and the test with s390x guest! Stefano
On Tue, 18 Aug 2020 16:01:20 +0200 Stefano Garzarella <sgarzare@redhat.com> wrote: > On Tue, Aug 18, 2020 at 02:44:50PM +0200, Cornelia Huck wrote: > > It seems to me that the status before this was "works by accident, but > > only if we're not negotiating to legacy, or the guest/host are both > > little endian". IOW, no visible breakage for most people (or we'd > > probably have heard of it already). Now we have a setup that's correct, > > but forces users to adapt their QEMU command lines. Option 1 would > > eliminate the need to do that, but would cause possibly > > not-really-fixable migration issues (you can probably deal with that > > manually, detaching and re-attaching the device as a last resort.) > > > > So, force modern, probably also remove the -transitional device type, > > and put a prominent explanation into the change log? > > > > I completely agree with your analysis and solution. > > So, for now we need to patch vhost-vsock-pci and vhost-user-vsock-pci, > and queue the patches in stable. I think we should also change -ccw; even though users won't get an error when starting QEMU, they might still run into the legacy problems in theory. Not sure how fast we'll have a stable release, though. > > Do you prefer to send them? Otherwise I can do that. If you already have something on your disk, please go ahead :) > > Thanks again for the help and the test with s390x guest! np; especially as it was my patch which started this in the first place :/
On Tue, Aug 18, 2020 at 04:31:01PM +0200, Cornelia Huck wrote: > On Tue, 18 Aug 2020 16:01:20 +0200 > Stefano Garzarella <sgarzare@redhat.com> wrote: > > > On Tue, Aug 18, 2020 at 02:44:50PM +0200, Cornelia Huck wrote: > > > > It seems to me that the status before this was "works by accident, but > > > only if we're not negotiating to legacy, or the guest/host are both > > > little endian". IOW, no visible breakage for most people (or we'd > > > probably have heard of it already). Now we have a setup that's correct, > > > but forces users to adapt their QEMU command lines. Option 1 would > > > eliminate the need to do that, but would cause possibly > > > not-really-fixable migration issues (you can probably deal with that > > > manually, detaching and re-attaching the device as a last resort.) > > > > > > So, force modern, probably also remove the -transitional device type, > > > and put a prominent explanation into the change log? > > > > > > > I completely agree with your analysis and solution. > > > > So, for now we need to patch vhost-vsock-pci and vhost-user-vsock-pci, > > and queue the patches in stable. > > I think we should also change -ccw; even though users won't get an > error when starting QEMU, they might still run into the legacy problems > in theory. Okay. > > Not sure how fast we'll have a stable release, though. > > > > > Do you prefer to send them? Otherwise I can do that. > > If you already have something on your disk, please go ahead :) Yes, I have something for pci, and I'll follow your suggestion for ccw! If you have time, can you share with me some tips on how to install s390x guest on my laptop? > > > > > Thanks again for the help and the test with s390x guest! > > np; especially as it was my patch which started this in the first place > :/ > I'm not sure that was a bad thing, since we found out that virtio-vsock is modern only :-) Thanks, Stefano
On Tue, 18 Aug 2020 17:28:07 +0200 Stefano Garzarella <sgarzare@redhat.com> wrote: > On Tue, Aug 18, 2020 at 04:31:01PM +0200, Cornelia Huck wrote: > > > Do you prefer to send them? Otherwise I can do that. > > > > If you already have something on your disk, please go ahead :) > > Yes, I have something for pci, and I'll follow your suggestion for ccw! Cool, thx! > > If you have time, can you share with me some tips on how to install > s390x guest on my laptop? The easiest way is probably to use virt-manager. I have installed Fedora s390x guests via this, works well. Once you have a guest, you can grab the qemu command line from libvirt's log, trim it down, and start it directly. https://wiki.qemu.org/Documentation/Platforms/S390X might also be helpful.
Hi, On 8/13/20 12:37 PM, Cornelia Huck wrote: > On Thu, 13 Aug 2020 12:24:30 +0200 > Stefano Garzarella <sgarzare@redhat.com> wrote: > >> On Thu, Aug 13, 2020 at 11:28:20AM +0200, Cornelia Huck wrote: >>> On Thu, 13 Aug 2020 11:16:56 +0200 >>> Stefano Garzarella <sgarzare@redhat.com> wrote: >>> >>>> Hi, >>>> >>>> Qinghua discovered that virtio-vsock-pci requires 'disable-legacy=on' in >>>> QEMU 5.1: >>>> $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5 >>>> qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5: >>>> device is modern-only, use disable-legacy=on For info that's the same for virtio-iommu. + Jean-Philippe. Reading this thread to better understand what is the best thing to do now ;-) Thanks Eric >>>> >>>> Bisecting I found that this behaviour starts from this commit: >>>> 9b3a35ec82 ("virtio: verify that legacy support is not accidentally on") >>> >>> Oh, I had heard that from others already, was still trying to figure >>> out what to do. >>> >>>> >>>> IIUC virtio-vsock is modern-only, so I tried this patch and it works: >>>> >>>> diff --git a/hw/virtio/vhost-user-vsock-pci.c b/hw/virtio/vhost-user-vsock-pci.c >>>> index f4cf95873d..6e4cc874cd 100644 >>>> --- a/hw/virtio/vhost-user-vsock-pci.c >>>> +++ b/hw/virtio/vhost-user-vsock-pci.c >>>> @@ -40,6 +40,7 @@ static void vhost_user_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) >>>> VHostUserVSockPCI *dev = VHOST_USER_VSOCK_PCI(vpci_dev); >>>> DeviceState *vdev = DEVICE(&dev->vdev); >>>> >>>> + virtio_pci_force_virtio_1(vpci_dev); >>>> qdev_realize(vdev, BUS(&vpci_dev->bus), errp); >>>> } >>>> >>>> diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c >>>> index a815278e69..f641b974e9 100644 >>>> --- a/hw/virtio/vhost-vsock-pci.c >>>> +++ b/hw/virtio/vhost-vsock-pci.c >>>> @@ -44,6 +44,7 @@ static void vhost_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) >>>> VHostVSockPCI *dev = VHOST_VSOCK_PCI(vpci_dev); >>>> DeviceState *vdev = DEVICE(&dev->vdev); >>>> >>>> + virtio_pci_force_virtio_1(vpci_dev); >>>> qdev_realize(vdev, BUS(&vpci_dev->bus), errp); >>>> } >>>> >>>> >>>> Do you think this is the right approach or is there a better way to >>>> solve this issue? >>> >>> We basically have three possible ways to deal with this: >>> >>> - Force it to modern (i.e., what you have been doing; would need the >>> equivalent changes in ccw as well.) >> >> Oo, thanks for pointing out ccw! >> I don't know ccw well, in this case should we set dev->max_rev to 1 or 2 >> to force to modern? > > No, ->max_rev is the wrong side of the limit :) You want > > ccw_dev->force_revision_1 = true; > > in _instance_init() (see e.g. virtio-ccw-gpu.c). > >> >>> Pro: looks like the cleanest approach. >>> Con: not sure if we would need backwards compatibility support, >>> which looks hairy. >> >> Not sure too. > > Yes, I'm not sure at all how to handle user-specified values for > legacy/modern. > >> >>> - Add vsock to the list of devices with legacy support. >>> Pro: Existing setups continue to work. >>> Con: If vsock is really virtio-1-only, we still carry around >>> possibly broken legacy support. >> >> I'm not sure it is virtio-1-only, but virtio-vsock was introduced in >> 2016, so I supposed it is modern-only. > > Yes, I would guess so as well. > >> >> How can I verify that? Maybe forcing legacy mode and run some tests. > > Probably yes. The likeliest area with issues is probably endianness, so > maybe with something big endian in the mix? > >> >>> - Do nothing, have users force legacy off. Bad idea, as ccw has no way >>> to do that on the command line. >>> >>> The first option is probably best. >>> >> >> Yeah, I agree with you! > > Yes, it's really a pity we only noticed this after the release; this > was supposed to stop new devices with legacy support creeping in, not > to break existing command lines :( > >
diff --git a/hw/virtio/vhost-user-vsock-pci.c b/hw/virtio/vhost-user-vsock-pci.c index f4cf95873d..6e4cc874cd 100644 --- a/hw/virtio/vhost-user-vsock-pci.c +++ b/hw/virtio/vhost-user-vsock-pci.c @@ -40,6 +40,7 @@ static void vhost_user_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) VHostUserVSockPCI *dev = VHOST_USER_VSOCK_PCI(vpci_dev); DeviceState *vdev = DEVICE(&dev->vdev); + virtio_pci_force_virtio_1(vpci_dev); qdev_realize(vdev, BUS(&vpci_dev->bus), errp); } diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c index a815278e69..f641b974e9 100644 --- a/hw/virtio/vhost-vsock-pci.c +++ b/hw/virtio/vhost-vsock-pci.c @@ -44,6 +44,7 @@ static void vhost_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) VHostVSockPCI *dev = VHOST_VSOCK_PCI(vpci_dev); DeviceState *vdev = DEVICE(&dev->vdev); + virtio_pci_force_virtio_1(vpci_dev); qdev_realize(vdev, BUS(&vpci_dev->bus), errp); }
Hi, Qinghua discovered that virtio-vsock-pci requires 'disable-legacy=on' in QEMU 5.1: $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5 qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5: device is modern-only, use disable-legacy=on Bisecting I found that this behaviour starts from this commit: 9b3a35ec82 ("virtio: verify that legacy support is not accidentally on") IIUC virtio-vsock is modern-only, so I tried this patch and it works: Do you think this is the right approach or is there a better way to solve this issue? Thanks, Stefano