From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 78A30A2043 for ; Fri, 16 Jun 2023 15:05:52 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DFFC3334D5 for ; Fri, 16 Jun 2023 15:05:51 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 16 Jun 2023 15:05:45 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 43FDE45B6C for ; Fri, 16 Jun 2023 15:05:45 +0200 (CEST) From: Dominik Csapak To: pve-devel@lists.proxmox.com Date: Fri, 16 Jun 2023 15:05:21 +0200 Message-Id: <20230616130542.199182-2-d.csapak@proxmox.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20230616130542.199182-1-d.csapak@proxmox.com> References: <20230616130542.199182-1-d.csapak@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.015 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: [pve-devel] [PATCH qemu-server v7 1/7] usb: refactor usb code and move some into USB module X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 16 Jun 2023 13:05:52 -0000 similar to how we handle the PCI module and format. This makes the 'verify_usb_device' method and format unnecessary since we simply check the format with a regex. while doing tihs, i noticed that we don't correctly check for the case-insensitive variant for 'spice' during hotplug, so fix that too With this we can also remove some parameters from the get_usb_devices and get_usb_controllers functions while were at it, refactor the permission checks for the usb config too and use the new 'my sub' style for the functions also make print_usbdevice_full parse the device itself, so we don't have to do it in multiple places (especially in places where we don't see that this is needed) No functional change intended Signed-off-by: Dominik Csapak --- new in v7 PVE/API2/Qemu.pm | 40 ++++++++++-------- PVE/QemuServer.pm | 72 ++++--------------------------- PVE/QemuServer/USB.pm | 98 +++++++++++++++++++++++++++++++------------ 3 files changed, 103 insertions(+), 107 deletions(-) diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm index c92734a6..c6a4cd2a 100644 --- a/PVE/API2/Qemu.pm +++ b/PVE/API2/Qemu.pm @@ -583,19 +583,29 @@ my $check_vm_create_serial_perm = sub { return 1; }; -my $check_vm_create_usb_perm = sub { +my sub check_usb_perm { + my ($rpcenv, $authuser, $vmid, $pool, $opt, $value) = @_; + + return 1 if $authuser eq 'root@pam'; + + my $device = PVE::JSONSchema::parse_property_string('pve-qm-usb', $value); + if ($device->{host} =~ m/^spice$/i) { + $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']); + } else { + die "only root can set '$opt' config for real devices\n"; + } + + return 1; +} + +my sub check_vm_create_usb_perm { my ($rpcenv, $authuser, $vmid, $pool, $param) = @_; return 1 if $authuser eq 'root@pam'; foreach my $opt (keys %{$param}) { next if $opt !~ m/^usb\d+$/; - - if ($param->{$opt} =~ m/spice/) { - $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']); - } else { - die "only root can set '$opt' config for real devices\n"; - } + check_usb_perm($rpcenv, $authuser, $vmid, $pool, $opt, $param->{$opt}); } return 1; @@ -878,7 +888,8 @@ __PACKAGE__->register_method({ &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, $pool, [ keys %$param]); &$check_vm_create_serial_perm($rpcenv, $authuser, $vmid, $pool, $param); - &$check_vm_create_usb_perm($rpcenv, $authuser, $vmid, $pool, $param); + check_vm_create_usb_perm($rpcenv, $authuser, $vmid, $pool, $param); + PVE::QemuServer::check_bridge_access($rpcenv, $authuser, $param); &$check_cpu_model_access($rpcenv, $authuser, $param); @@ -1719,11 +1730,7 @@ my $update_vm_api = sub { PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force); PVE::QemuConfig->write_config($vmid, $conf); } elsif ($opt =~ m/^usb\d+$/) { - if ($val =~ m/spice/) { - $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']); - } elsif ($authuser ne 'root@pam') { - die "only root can delete '$opt' config for real devices\n"; - } + check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $val); PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force); PVE::QemuConfig->write_config($vmid, $conf); } elsif ($opt eq 'tags') { @@ -1784,11 +1791,10 @@ my $update_vm_api = sub { } $conf->{pending}->{$opt} = $param->{$opt}; } elsif ($opt =~ m/^usb\d+/) { - if ((!defined($conf->{$opt}) || $conf->{$opt} =~ m/spice/) && $param->{$opt} =~ m/spice/) { - $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']); - } elsif ($authuser ne 'root@pam') { - die "only root can modify '$opt' config for real devices\n"; + if (my $olddevice = $conf->{$opt}) { + check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $conf->{$opt}); } + check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt}); $conf->{pending}->{$opt} = $param->{$opt}; } elsif ($opt eq 'tags') { assert_tag_permissions($vmid, $conf->{$opt}, $param->{$opt}, $rpcenv, $authuser); diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm index 6cbaf878..38200fea 100644 --- a/PVE/QemuServer.pm +++ b/PVE/QemuServer.pm @@ -56,7 +56,7 @@ use PVE::QemuServer::Machine; use PVE::QemuServer::Memory; use PVE::QemuServer::Monitor qw(mon_cmd); use PVE::QemuServer::PCI qw(print_pci_addr print_pcie_addr print_pcie_root_port parse_hostpci); -use PVE::QemuServer::USB qw(parse_usb_device); +use PVE::QemuServer::USB; my $have_sdn; eval { @@ -835,7 +835,6 @@ while (my ($k, $v) = each %$confdesc) { PVE::JSONSchema::register_standard_option("pve-qm-$k", $v); } -my $MAX_USB_DEVICES = 14; my $MAX_NETS = 32; my $MAX_SERIAL_PORTS = 4; my $MAX_PARALLEL_PORTS = 3; @@ -1081,44 +1080,6 @@ sub verify_volume_id_or_absolute_path { return $volid; } -my $usb_fmt = { - host => { - default_key => 1, - type => 'string', format => 'pve-qm-usb-device', - format_description => 'HOSTUSBDEVICE|spice', - description => < { - optional => 1, - type => 'boolean', - description => "Specifies whether if given host option is a USB3 device or port." - ." For modern guests (machine version >= 7.1 and ostype l26 and windows > 7), this flag" - ." is irrelevant (all devices are plugged into a xhci controller).", - default => 0, - }, -}; - -my $usbdesc = { - optional => 1, - type => 'string', format => $usb_fmt, - description => "Configure an USB device (n is 0 to 4, for machine version >= 7.1 and ostype" - ." l26 or windows > 7, n can be up to 14).", -}; -PVE::JSONSchema::register_standard_option("pve-qm-usb", $usbdesc); - my $serialdesc = { optional => 1, type => 'string', @@ -1167,8 +1128,8 @@ for my $key (keys %{$PVE::QemuServer::Drive::drivedesc_hash}) { $confdesc->{$key} = $PVE::QemuServer::Drive::drivedesc_hash->{$key}; } -for (my $i = 0; $i < $MAX_USB_DEVICES; $i++) { - $confdesc->{"usb$i"} = $usbdesc; +for (my $i = 0; $i < $PVE::QemuServer::USB::MAX_USB_DEVICES; $i++) { + $confdesc->{"usb$i"} = $PVE::QemuServer::USB::usbdesc; } my $boot_fmt = { @@ -2244,17 +2205,6 @@ sub qemu_created_version_fixups { return; } -PVE::JSONSchema::register_format('pve-qm-usb-device', \&verify_usb_device); -sub verify_usb_device { - my ($value, $noerr) = @_; - - return $value if parse_usb_device($value); - - return if $noerr; - - die "unable to parse usb device\n"; -} - # add JSON properties for create and set function sub json_config_properties { my ($prop, $with_disk_alloc) = @_; @@ -3740,7 +3690,7 @@ sub config_to_command { # add usb controllers my @usbcontrollers = PVE::QemuServer::USB::get_usb_controllers( - $conf, $bridges, $arch, $machine_type, $usbdesc->{format}, $MAX_USB_DEVICES, $machine_version); + $conf, $bridges, $arch, $machine_type, $machine_version); push @$devices, @usbcontrollers if @usbcontrollers; my $vga = parse_vga($conf->{vga}); @@ -3782,7 +3732,7 @@ sub config_to_command { $usb_dev_features->{spice_usb3} = 1 if min_version($machine_version, 4, 0); my @usbdevices = PVE::QemuServer::USB::get_usb_devices( - $conf, $usbdesc->{format}, $MAX_USB_DEVICES, $usb_dev_features, $bootorder, $machine_version); + $conf, $usb_dev_features, $bootorder, $machine_version); push @$devices, @usbdevices if @usbdevices; # serial devices @@ -4653,12 +4603,8 @@ sub qemu_usb_hotplug { qemu_deviceadd($vmid, PVE::QemuServer::USB::print_qemu_xhci_controller($pciaddr)); } - # print_usbdevice_full expects the parsed device - my $d = parse_usb_device($device->{host}); - $d->{usb3} = $device->{usb3}; - # add the new one - vm_deviceplug($storecfg, $conf, $vmid, $deviceid, $d, $arch, $machine_type); + vm_deviceplug($storecfg, $conf, $vmid, $deviceid, $device, $arch, $machine_type); } sub qemu_cpu_hotplug { @@ -5120,9 +5066,9 @@ sub vmconfig_hotplug_pending { } elsif ($opt =~ m/^usb(\d+)$/) { my $index = $1; die "skip\n" if !$usb_hotplug; - my $d = eval { parse_property_string($usbdesc->{format}, $value) }; + my $d = eval { parse_property_string('pve-qm-usb', $value) }; my $id = $opt; - if ($d->{host} eq 'spice') { + if ($d->{host} =~ m/^spice$/i) { $id = "usbredirdev$index"; } qemu_usb_hotplug($storecfg, $conf, $vmid, $id, $d, $arch, $machine_type); @@ -5199,7 +5145,7 @@ sub vmconfig_hotplug_pending { # unplug xhci controller if no usb device is left if ($usb_hotplug) { my $has_usb = 0; - for (my $i = 0; $i < $MAX_USB_DEVICES; $i++) { + for (my $i = 0; $i < $PVE::QemuServer::USB::MAX_USB_DEVICES; $i++) { next if !defined($conf->{"usb$i"}); $has_usb = 1; last; diff --git a/PVE/QemuServer/USB.pm b/PVE/QemuServer/USB.pm index 686461cc..fe541c1f 100644 --- a/PVE/QemuServer/USB.pm +++ b/PVE/QemuServer/USB.pm @@ -15,6 +15,52 @@ get_usb_devices ); my $OLD_MAX_USB = 5; +our $MAX_USB_DEVICES = 14; + + +my $USB_ID_RE = qr/(0x)?([0-9A-Fa-f]{4}):(0x)?([0-9A-Fa-f]{4})/; +my $USB_PATH_RE = qr/(\d+)\-(\d+(\.\d+)*)/; + +my $usb_fmt = { + host => { + default_key => 1, + type => 'string', + pattern => qr/(?:(?:$USB_ID_RE)|(?:$USB_PATH_RE)|[Ss][Pp][Ii][Cc][Ee])/, + format_description => 'HOSTUSBDEVICE|spice', + description => < { + optional => 1, + type => 'boolean', + description => "Specifies whether if given host option is a USB3 device or port." + ." For modern guests (machine version >= 7.1 and ostype l26 and windows > 7), this flag" + ." is irrelevant (all devices are plugged into a xhci controller).", + default => 0, + }, +}; + +PVE::JSONSchema::register_format('pve-qm-usb', $usb_fmt); + +our $usbdesc = { + optional => 1, + type => 'string', format => $usb_fmt, + description => "Configure an USB device (n is 0 to 4, for machine version >= 7.1 and ostype" + ." l26 or windows > 7, n can be up to 14).", +}; +PVE::JSONSchema::register_standard_option("pve-qm-usb", $usbdesc); sub parse_usb_device { my ($value) = @_; @@ -22,10 +68,10 @@ sub parse_usb_device { return if !$value; my $res = {}; - if ($value =~ m/^(0x)?([0-9A-Fa-f]{4}):(0x)?([0-9A-Fa-f]{4})$/) { + if ($value =~ m/^$USB_ID_RE$/) { $res->{vendorid} = $2; $res->{productid} = $4; - } elsif ($value =~ m/^(\d+)\-(\d+(\.\d+)*)$/) { + } elsif ($value =~ m/^$USB_PATH_RE$/) { $res->{hostbus} = $1; $res->{hostport} = $2; } elsif ($value =~ m/^spice$/i) { @@ -47,7 +93,7 @@ my sub assert_usb_index_is_useable { } sub get_usb_controllers { - my ($conf, $bridges, $arch, $machine, $format, $max_usb_devices, $machine_version) = @_; + my ($conf, $bridges, $arch, $machine, $machine_version) = @_; my $devices = []; my $pciaddr = ""; @@ -68,10 +114,10 @@ sub get_usb_controllers { my ($use_usb2, $use_usb3) = 0; my $any_usb = 0; - for (my $i = 0; $i < $max_usb_devices; $i++) { + for (my $i = 0; $i < $MAX_USB_DEVICES; $i++) { next if !$conf->{"usb$i"}; assert_usb_index_is_useable($i, $use_qemu_xhci); - my $d = eval { PVE::JSONSchema::parse_property_string($format,$conf->{"usb$i"}) } or next; + my $d = eval { PVE::JSONSchema::parse_property_string($usb_fmt, $conf->{"usb$i"}) } or next; $any_usb = 1; $use_usb3 = 1 if $d->{usb3}; $use_usb2 = 1 if !$d->{usb3}; @@ -93,7 +139,7 @@ sub get_usb_controllers { } sub get_usb_devices { - my ($conf, $format, $max_usb_devices, $features, $bootorder, $machine_version) = @_; + my ($conf, $features, $bootorder, $machine_version) = @_; my $devices = []; @@ -101,30 +147,26 @@ sub get_usb_devices { my $use_qemu_xhci = min_version($machine_version, 7, 1) && defined($ostype) && ($ostype eq 'l26' || windows_version($ostype) > 7); - for (my $i = 0; $i < $max_usb_devices; $i++) { + for (my $i = 0; $i < $MAX_USB_DEVICES; $i++) { my $devname = "usb$i"; next if !$conf->{$devname}; assert_usb_index_is_useable($i, $use_qemu_xhci); - my $d = eval { PVE::JSONSchema::parse_property_string($format,$conf->{$devname}) }; + my $d = eval { PVE::JSONSchema::parse_property_string($usb_fmt, $conf->{$devname}) }; next if !$d; my $port = $use_qemu_xhci ? $i + 1 : undef; - if (defined($d->{host})) { - my $hostdevice = parse_usb_device($d->{host}); - $hostdevice->{usb3} = $d->{usb3}; - if ($hostdevice->{spice}) { - # usb redir support for spice - my $bus = 'ehci'; - $bus = 'xhci' if ($hostdevice->{usb3} && $features->{spice_usb3}) || $use_qemu_xhci; - - push @$devices, '-chardev', "spicevmc,id=usbredirchardev$i,name=usbredir"; - push @$devices, '-device', print_spice_usbdevice($i, $bus, $port); - - warn "warning: spice usb port set as bootdevice, ignoring\n" if $bootorder->{$devname}; - } else { - push @$devices, '-device', print_usbdevice_full($conf, $devname, $hostdevice, $bootorder, $port); - } + if ($d->{host} && $d->{host} =~ m/^spice$/) { + # usb redir support for spice + my $bus = 'ehci'; + $bus = 'xhci' if ($d->{usb3} && $features->{spice_usb3}) || $use_qemu_xhci; + + push @$devices, '-chardev', "spicevmc,id=usbredirchardev$i,name=usbredir"; + push @$devices, '-device', print_spice_usbdevice($i, $bus, $port); + + warn "warning: spice usb port set as bootdevice, ignoring\n" if $bootorder->{$devname}; + } else { + push @$devices, '-device', print_usbdevice_full($conf, $devname, $d, $bootorder, $port); } } @@ -157,10 +199,12 @@ sub print_usbdevice_full { $usbdevice .= ",port=$port" if defined($port); } - if (defined($device->{vendorid}) && defined($device->{productid})) { - $usbdevice .= ",vendorid=0x$device->{vendorid},productid=0x$device->{productid}"; - } elsif (defined($device->{hostbus}) && defined($device->{hostport})) { - $usbdevice .= ",hostbus=$device->{hostbus},hostport=$device->{hostport}"; + my $parsed = parse_usb_device($device->{host}); + + if (defined($parsed->{vendorid}) && defined($parsed->{productid})) { + $usbdevice .= ",vendorid=0x$parsed->{vendorid},productid=0x$parsed->{productid}"; + } elsif (defined($parsed->{hostbus}) && defined($parsed->{hostport})) { + $usbdevice .= ",hostbus=$parsed->{hostbus},hostport=$parsed->{hostport}"; } else { die "no usb id or path given\n"; } -- 2.30.2