Re: virtio-vsock requires 'disable-legacy=on' in QEMU 5.1

[Auger Eric <eric.auger@redhat.com>]

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 :(
> 
>