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

[Thomas Lamprecht]

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..

>  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..

>  
>      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...

> -		# 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);
>  	    }
>