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 09620D0FD for ; Thu, 17 Aug 2023 14:29:03 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E01403051F for ; Thu, 17 Aug 2023 14:29:02 +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 ; Thu, 17 Aug 2023 14:29:02 +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 CC4EA42012 for ; Thu, 17 Aug 2023 14:29:01 +0200 (CEST) Message-ID: Date: Thu, 17 Aug 2023 14:29:00 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.1 Content-Language: en-US To: Proxmox VE development discussion , Markus Frank References: <20230118135800.131382-1-m.frank@proxmox.com> <20230118135800.131382-3-m.frank@proxmox.com> From: Fiona Ebner In-Reply-To: <20230118135800.131382-3-m.frank@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.524 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 NICE_REPLY_A -3.165 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [qemuconfig.pm, machine.pm, qemu.org, qemuserver.pm] Subject: Re: [pve-devel] [PATCH qemu-server v5 2/5] feature #3784: Parameter for guest vIOMMU & machine as property-string 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: Thu, 17 Aug 2023 12:29:03 -0000 Am 18.01.23 um 14:57 schrieb Markus Frank: > vIOMMU enables the option to passthrough pci devices to L2 VMs > in L1 VMs via Nested Virtualisation. > > QEMU-Parameters: > https://www.qemu.org/docs/master/system/qemu-manpage.html > https://wiki.qemu.org/Features/VT-d > > -machine ...,kernel-irqchip=split: > > "split" because of intremap see below. > > -device intel-iommu: > > * caching-mode=on: > > "It is required for -device vfio-pci to work with the VT-d device, because host > assigned devices requires to setup the DMA mapping on the host before guest DMA > starts." > > * intremap=on: > > "This enables interrupt remapping feature. It's required to enable complete > x2apic. Currently it only supports kvm kernel-irqchip modes off or split, while > full kernel-irqchip is not yet supported." > > Signed-off-by: Markus Frank > --- This one needs a rebase (...) > + my $q35 = $machine_conf->{type} && ($machine_conf->{type} =~ m/q35/) ? 1 : 0; > + my $kvm = $conf->{kvm}; > + $kvm //= 1 if PVE::QemuServer::is_native($arch); > + if ($machine_conf->{viommu} && (!$kvm || !$q35)) { > + die "to use vIOMMU please enable kvm and set the machine type to q35\n"; > + } > > PVE::QemuConfig->write_config($vmid, $conf); > > @@ -1770,7 +1778,16 @@ my $update_vm_api = sub { > } elsif ($opt eq 'tags') { > assert_tag_permissions($vmid, $conf->{$opt}, $param->{$opt}, $rpcenv, $authuser); > $conf->{pending}->{$opt} = PVE::GuestHelpers::get_unique_tags($param->{$opt}); > - } else { > + } elsif ($opt eq 'machine') { > + my $machine_conf = PVE::QemuServer::parse_machine($param->{$opt}); > + my $q35 = $machine_conf->{type} && ($machine_conf->{type} =~ m/q35/) ? 1 : 0; > + my $kvm = $conf->{kvm}; > + $kvm //= 1 if PVE::QemuServer::is_native($arch); > + if ($machine_conf->{viommu} && (!$kvm || !$q35)) { > + die "to use vIOMMU please enable kvm and set the machine type to q35\n"; > + } Maybe worth adding a helper function taking in the config and the machine option. It's the very same check as above. > + $conf->{pending}->{$opt} = $param->{$opt}; > + }else { Style nit: missing space before else > $conf->{pending}->{$opt} = $param->{$opt}; > > if ($opt eq 'boot') { > diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm > index 051382c..7c998ef 100644 > --- a/PVE/QemuConfig.pm > +++ b/PVE/QemuConfig.pm > @@ -433,7 +433,8 @@ sub __snapshot_rollback_hook { > } else { > # Note: old code did not store 'machine', so we try to be smart > # and guess the snapshot was generated with kvm 1.4 (pc-i440fx-1.4). > - $data->{forcemachine} = $conf->{machine} || 'pc-i440fx-1.4'; > + my $machine_conf = PVE::QemuServer::parse_machine($conf->{machine}); > + $data->{forcemachine} = $machine_conf->{type} || 'pc-i440fx-1.4'; > > # we remove the 'machine' configuration if not explicitly specified > # in the original config. > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm > index 987908d..55c11d5 100644 > --- a/PVE/QemuServer.pm > +++ b/PVE/QemuServer.pm > @@ -124,6 +124,19 @@ PVE::JSONSchema::register_standard_option('pve-qemu-machine', { > optional => 1, > }); > > +my $machine_fmt = { > + type => get_standard_option('pve-qemu-machine', { Any reason against changing the standard option itself to be the whole property string rather than keep the option just being the type? I noticed that 'runningmachine' still uses only "get_standard_option('pve-qemu-machine'", but that is wrong after this patch. Changing the standard option itself would avoid that. > + default_key => 1, > + format_description => "pve-qemu-machine-type", That format description is not very telling at all. Usually, this is used to clarify what exact format the string is, e.g. base64. I don't think it's needed here and there's already a description of the property itself. > + }), > + viommu => { > + type => 'boolean', > + description => "enable guest vIOMMU (needs kvm to be enabled and q35 to be set as machine)", Nit: "as machine type" sounds slightly better now that machine is more than just the type > + default => 0, > + optional => 1, > + }, > +}; > + > # FIXME: remove in favor of just using the INotify one, it's cached there exactly the same way > my $nodename_cache; > sub nodename { > @@ -626,7 +639,12 @@ EODESCR > pattern => $PVE::QemuServer::CPUConfig::qemu_cmdline_cpu_re, > format_description => 'QEMU -cpu parameter' > }, > - machine => get_standard_option('pve-qemu-machine'), > + machine => { > + description => "Specifies the Qemu machine type.", Nit: QEMU should be capitalized > + type => 'string', > + optional => 1, > + format => $machine_fmt, > + }, > arch => { > description => "Virtual processor architecture. Defaults to the host.", > optional => 1, > @@ -2134,6 +2152,21 @@ sub parse_watchdog { > return $res; > } > > +sub parse_machine { > + my ($value) = @_; > + > + return if !$value; > + > + my $res = eval { parse_property_string($machine_fmt, $value) }; > + die $@ if $@; No need for eval if you just die with the exact same error ;) > + return $res; > +} > + > +sub print_machine { > + my ($machine_conf) = @_; > + return PVE::JSONSchema::print_property_string($machine_conf, $machine_fmt); > +} > + > sub parse_guest_agent { > my ($conf) = @_; > (...) > @@ -4137,6 +4174,15 @@ sub config_to_command { > } > push @$machineFlags, "type=${machine_type_min}"; > > + if ($machine_conf->{viommu} && (!$kvm || !$q35)) { > + die "to use vIOMMU please enable kvm and set the machine type to q35\n"; > + } > + > + if ($machine_conf->{viommu}) { Style nit: the check for kvm+q35 and the die from above could be moved into this block Or instead re-use the check helper I suggested above > + unshift @$devices, '-device', "intel-iommu,intremap=on,caching-mode=on"; > + push @$machineFlags, 'kernel-irqchip=split'; > + } > + > push @$cmd, @$devices; > push @$cmd, '-rtc', join(',', @$rtcFlags) if scalar(@$rtcFlags); > push @$cmd, '-machine', join(',', @$machineFlags) if scalar(@$machineFlags); > diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm > index d9429ed..bfbde59 100644 > --- a/PVE/QemuServer/Machine.pm > +++ b/PVE/QemuServer/Machine.pm > @@ -15,7 +15,8 @@ our $PVE_MACHINE_VERSION = { > sub machine_type_is_q35 { > my ($conf) = @_; > > - return $conf->{machine} && ($conf->{machine} =~ m/q35/) ? 1 : 0; > + my $machine_conf = PVE::QemuServer::parse_machine($conf->{machine}); > + return $machine_conf->{type} && ($machine_conf->{type} =~ m/q35/) ? 1 : 0; > } The parse and print functions and option definition should really live inside here, i.e. Machine.pm. The Machine module should not call back into PVE::QemuServer if we can somehow avoid it. > > sub current_from_query_machines { > @@ -120,7 +121,8 @@ sub qemu_machine_pxe { > > my $machine = get_current_qemu_machine($vmid); > > - if ($conf->{machine} && $conf->{machine} =~ m/\.pxe$/) { > + my $machine_conf = PVE::QemuServer::parse_machine($conf->{machine}); > + if ($machine_conf->{type} && $machine_conf->{type} =~ m/\.pxe$/) { > $machine .= '.pxe'; > } >