On Thu, Aug 13, 2020 at 12:37:37PM +0200, Cornelia Huck wrote: > On Thu, 13 Aug 2020 12:24:30 +0200 > Stefano Garzarella <sgarz...@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 <sgarz...@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