From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 150781FF09C for ; Mon, 05 Oct 2026 16:36:56 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 2E54C21677; Mon, 05 Oct 2026 16:36:53 +0200 (CEST) Message-ID: <44be5b02-5510-4458-ab5a-dd6a81f0c939@proxmox.com> Date: Mon, 5 Oct 2026 16:36:48 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 qemu-server 08/16] api: allow setting the efi-firmware option To: Christian Ludwig , pve-devel@lists.proxmox.com References: <112600c5d24f1a06ab9049b1b234035c989d0135.1790337726.git@genua.de> Content-Language: en-US From: Fiona Ebner In-Reply-To: <112600c5d24f1a06ab9049b1b234035c989d0135.1790337726.git@genua.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1791211008816 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.478 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 2EVXNVTGJJACFFCNP5Y5UEXNW2ZQV3U3 X-Message-ID-Hash: 2EVXNVTGJJACFFCNP5Y5UEXNW2ZQV3U3 X-MailFrom: f.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Am 28.09.26 um 7:49 AM schrieb Christian Ludwig: > Add the option to the POST/PUT {vmid}/config endpoints. It needs > VM.Config.Options permission, like the 'bios' option it modifies, and is > rejected without bios=ovmf. > > Deleting it does not trigger volume cleanup, firmware images are shared. > > Signed-off-by: Christian Ludwig > --- > src/PVE/API2/Qemu.pm | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm > index 71247eec..8b12bb53 100644 > --- a/src/PVE/API2/Qemu.pm > +++ b/src/PVE/API2/Qemu.pm > @@ -277,6 +277,10 @@ my $check_storage_access = sub { > "/storage/$settings->{vmstatestorage}", > ['Datastore.AllocateSpace'], > ) if defined($settings->{vmstatestorage}); > + > + PVE::Storage::check_volume_access( > + $rpcenv, $authuser, $storecfg, $vmid, $settings->{'efi-firmware'}, 'efi-firmware', > + ) if defined($settings->{'efi-firmware'}); > }; > > my $check_storage_access_clone = sub { > @@ -825,6 +829,7 @@ my $generaloptions = { I wonder if it's not more fitting in the $hwtypeoptions? But not sure. My suggestion to have it be a sub-property of 'bios' would conflict with that again. > 'autostart' => 1, > 'bios' => 1, > 'description' => 1, > + 'efi-firmware' => 1, > 'keyboard' => 1, > 'localtime' => 1, > 'migrate_downtime' => 1, > @@ -1366,6 +1371,9 @@ __PACKAGE__->register_method({ > > $check_drive_param->($param, $storecfg); > > + raise_param_exc({ 'efi-firmware' => "requires bios=ovmf" }) > + if $param->{'efi-firmware'} && ($param->{bios} // '') ne 'ovmf'; > + > PVE::QemuServer::Network::add_random_macs($param); > } > > @@ -2527,6 +2535,13 @@ my $update_vm_api = sub { > print "automatic pinning of machine version failed - $@" if $@; > } > $conf->{pending}->{$opt} = $param->{$opt}; > + } elsif ($opt eq 'efi-firmware') { > + PVE::Storage::check_volume_access( > + $rpcenv, $authuser, $storecfg, $vmid, $param->{$opt}, 'efi-firmware', > + ); > + my $bios = $param->{bios} // $conf->{pending}->{bios} // $conf->{bios} // ''; > + raise_param_exc({ $opt => "requires bios=ovmf" }) if $bios ne 'ovmf'; > + $conf->{pending}->{$opt} = $param->{$opt}; This covers the case where 'efi-firmware' changes, but it could be that the 'bios' setting is changed away from 'ovmf' later on, so you'd also need to check in a branch for 'bios', there also for deletion. Might be good to have a helper and call that from all relevant places. Or if having 'bios' be a property string, it can be checked together. > } elsif ($opt eq 'cipassword') { > if (!PVE::QemuServer::Helpers::windows_version($conf->{ostype})) { > # Same logic as in cloud-init (but with the regex fixed...) Best Regards, Fiona