[pve-devel] [PATCH v3 qemu-server 1/3] Add USB3 capablities to Spice USB devices

[Aaron Lauterer]

On 9/10/19 10:46 AM, Thomas Lamprecht wrote:
> On 06.09.19 15:26, Aaron Lauterer wrote:
>> To not change current behaviour and thus breaking live migration USB3
>> for a Spice USB device requires Qemu v4.1.
>>
>> The old behavior was that even though technically it was possible to
>> the set `usb3=1` setting, it was ignored. The bus was hardcoded to
>> ehci. If another USB2 device was added or the machine type was set to
>> Q35 an ehci controller was present and the VM was able to boot.
>>
>> With this patch the behaviour is changing and the bus is set to xhci if
>> USB3 is set for the Spice USB device and the VM is running under Qemu
>> v4.1.
>>
>> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
>> ---
>>
>> I use the `qemu_machine_feature_enabled` function from QemuServer.pm to
>> check against Qemu v4.1
>>
>>
>>   PVE/QemuServer.pm     |  2 +-
>>   PVE/QemuServer/USB.pm | 10 +++++++---
>>   2 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index a424720..0489b27 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -3830,7 +3830,7 @@ sub config_to_command {
>>       }
>>   
>>       # usb devices
>> -    my @usbdevices = PVE::QemuServer::USB::get_usb_devices($conf, $usbdesc->{format}, $MAX_USB_DEVICES);
>> +    my @usbdevices = PVE::QemuServer::USB::get_usb_devices($conf, $usbdesc->{format}, $MAX_USB_DEVICES, $machine_type, $kvmver);
>>       push @$devices, @usbdevices if @usbdevices;
>>       # serial devices
>>       for (my $i = 0; $i < $MAX_SERIAL_PORTS; $i++)  {
>> diff --git a/PVE/QemuServer/USB.pm b/PVE/QemuServer/USB.pm
>> index a2097b9..af24636 100644
>> --- a/PVE/QemuServer/USB.pm
>> +++ b/PVE/QemuServer/USB.pm
>> @@ -3,6 +3,7 @@ package PVE::QemuServer::USB;
>>   use strict;
>>   use warnings;
>>   use PVE::QemuServer::PCI qw(print_pci_addr);
>> +use PVE::QemuServer;
> 
> this adds a cyclic module dependency:
> 
> PVE::QemuServer -> QemuServer::USB -> PVE::QemuServer
> 
> While it's intra-package, and thus not as harmful as cross package, I'd still
> like to avoid that if anyhow possible..

Yeah, that was one of the things where I wasn't sure if it would fly.
> 
>>   use PVE::JSONSchema;
>>   use base 'Exporter';
>>   
>> @@ -74,7 +75,7 @@ sub get_usb_controllers {
>>   }
>>   
>>   sub get_usb_devices {
>> -    my ($conf, $format, $max_usb_devices) = @_;
>> +    my ($conf, $format, $max_usb_devices, $machine, $kvmver) = @_;
> 
> instead off adding those two you could also just add a single feature flag?
> Maybe a hash? alà
> 
> my ($conf, $format, $max_usb_devices, $features) = @_;
> 
> where $features includes a "spice_usb3 => 1" entry if it should be used?
> as an idea..

I will move the qemu_machine_feature_check to the QemuServer.pm and try 
something like this.

> 
>>   
>>       my $devices = [];
>>   
>> @@ -87,9 +88,12 @@ sub get_usb_devices {
>>   	    my $hostdevice = parse_usb_device($d->{host});
>>   	    $hostdevice->{usb3} = $d->{usb3};
>>   	    if (defined($hostdevice->{spice}) && $hostdevice->{spice}) {
> 
> while you do not touch above, it's still redundant,
> if ($hostdevice->{spice})
> would be enough...

Should I clean this up with the next version? Maybe as a separate commit?

> 
>> -		# usb redir support for spice, currently no usb3
>> +		# usb redir support for spice
>> +		my $bus = 'ehci';
>> +		$bus = 'xhci' if $hostdevice->{usb3} && PVE::QemuServer::qemu_machine_feature_enabled($machine, $kvmver, 4, 1);
>> +
>>   		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);
>>   	    }
>>
> 
>