[pve-devel] [PATCH qemu-server 1/2] Add USB3 support to Spice USB redirection

[Thomas Lamprecht]

On 9/2/19 2:29 PM, Aaron Lauterer wrote:
> 
> 
> On 9/2/19 1:57 PM, Thomas Lamprecht wrote:
>> On 8/30/19 9:40 AM, Aaron Lauterer wrote:
>>> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
>>> ---
>>>   PVE/QemuServer/USB.pm | 7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/PVE/QemuServer/USB.pm b/PVE/QemuServer/USB.pm
>>> index a2097b9..05c78cf 100644
>>> --- a/PVE/QemuServer/USB.pm
>>> +++ b/PVE/QemuServer/USB.pm
>>> @@ -87,9 +87,12 @@ sub get_usb_devices {
>>>           my $hostdevice = parse_usb_device($d->{host});
>>>           $hostdevice->{usb3} = $d->{usb3};
>>>           if (defined($hostdevice->{spice}) && $hostdevice->{spice}) {
>>> -        # usb redir support for spice, currently no usb3
>>> +        # usb redir support for spice
>>> +        my $bus = 'ehci';
>>> +        $bus = 'xhci' if $hostdevice->{usb3};
>>> +
>>>           push @$devices, '-chardev', "spicevmc,id=usbredirchardev$i,name=usbredir";
>>> -        push @$devices, '-device', "usb-redir,chardev=usbredirchardev$i,id=usbredirdev$i,bus=ehci.0";
>>> +        push @$devices, '-device', "usb-redir,chardev=usbredirchardev$i,id=usbredirdev$i,bus=$bus.0";
>>>           } else {
>>>           push @$devices, '-device', print_usbdevice_full($conf, "usb$i", $hostdevice);
>>>           }
>>>
>>
>> what the commit message does not mentions:
>>
>> One could have set an usb3 spice variant already, e.g., with:
>> # qm set 1000 --usb2 spice,usb3=1
>>
>> and started it without issues, as we just ignored it.
>> Now, with your patch it will suddenly change the backing bus to
>> xhci, which (I assume) kills live migrations or restorations of
>> snapshot with RAM.
>>
>> So this would need to be guarded somehow, e.g., with a qemu
>> machine version check.
>>
> 
> As described in the cover letter this is only true if other USB2 devices are added. Because only then will there be a "ehci.0" bus.

Did not saw that, and nobody would when it had been applied as is if
they looked at the Commit in the future, keep such information in the
commit itself (too), if you know that already.

> 
> Otherwise the start of the VM will fail with this error: "Bus 'ehci.0' not found".

I had that I my VM were I tested it, so it worked instantly and seems
not to unrealistically. Further, a "it's only happens" is not a full
excuse, just because the chance for such a setup is rather low we do
never actively want to break forward live migration, that not good.

> 
> I did not think about the snapshot restore situation. I will integrate a qemu machine version check to handle it.