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 0F34F1FF0DF for ; Thu, 05 Nov 2026 16:24:19 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 08E3C21669; Thu, 05 Nov 2026 16:24:07 +0100 (CET) Message-ID: Date: Thu, 5 Nov 2026 16:23:58 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH qemu-server v3 10/19] bios: add a custom firmware sub-property To: Christian Ludwig , pve-devel@lists.proxmox.com References: Content-Language: en-US From: Fiona Ebner In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793892238517 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.293 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: B4RLUNW24FDNYKXJW4BRWQRT334WLW7Q X-Message-ID-Hash: B4RLUNW24FDNYKXJW4BRWQRT334WLW7Q 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 27.10.26 um 11:14 AM schrieb Christian Ludwig: > Add the 'firmware' sub-property to the bios option, pointing to a volume > of the efi-firmware content type: > > bios: ovmf,firmware=local:efi-firmware/custom.fd > > Custom firmware images are only supported with OVMF for now, so require > the 'ovmf' BIOS type when generating the command line. > > The image replaces the system OVMF code image for the pflash, blockdev > and '-bios' variants. Include the volume when activating a VM's volumes. > Check the volume's storage and content type before that, so that errors > are reported in the context of the custom firmware, e.g. when a template > or a restored VM references an image that is not available on the node. > > Firmware images are content and not owned by the VM, like ISO images, so > refuse migration unless they are on a shared storage. They are never > copied, so refuse remote migration too. > > Signed-off-by: Christian Ludwig > --- > src/PVE/QemuConfig.pm | 17 ++++++++++ > src/PVE/QemuMigrate.pm | 6 ++++ > src/PVE/QemuServer.pm | 20 ++++++++--- > src/PVE/QemuServer/BIOS.pm | 28 ++++++++++++++++ > src/PVE/QemuServer/OVMF.pm | 68 ++++++++++++++++++++++++++++++++------ > 5 files changed, 125 insertions(+), 14 deletions(-) > > diff --git a/src/PVE/QemuConfig.pm b/src/PVE/QemuConfig.pm > index d83a474c..c2b2cab7 100644 > --- a/src/PVE/QemuConfig.pm > +++ b/src/PVE/QemuConfig.pm > @@ -113,6 +113,17 @@ sub parse_volume { > die $err; > } > $volume = { 'file' => $volume_string }; > + } elsif ($key eq 'bios') { > + my $bios = eval { PVE::QemuServer::BIOS::parse_bios($volume_string) }; > + if (my $err = $@) { > + return if $noerr; > + die $err; > + } > + # only a custom firmware image is a volume, plain BIOS settings are not > + return if !defined($bios->{firmware}); > + > + $bios->{file} = delete($bios->{firmware}); > + $volume = $bios; It is a bit hacky, but I suppose it can be fine. An alternative would be to handle it in foreach_volid() instead, or in all transitive callers where actually required, but that is probably not nicer, so let's just keep it like this. > } else { > $volume = PVE::QemuServer::Drive::parse_drive($key, $volume_string); > } > @@ -125,6 +136,12 @@ sub parse_volume { > sub print_volume { > my ($class, $key, $volume) = @_; > > + if ($key eq 'bios') { > + my $bios = { $volume->%* }; > + $bios->{firmware} = delete($bios->{file}); > + return PVE::QemuServer::BIOS::print_bios($bios); > + } Even if going with the above for parse_volume(), IMHO there should rather be an error when encountering a 'bios' key here, as this is not how the 'bios' property is supposed to be printed. > + > return PVE::QemuServer::Drive::print_drive($volume); > } > > diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm > index 8da6f15d..78416896 100644 > --- a/src/PVE/QemuMigrate.pm > +++ b/src/PVE/QemuMigrate.pm > @@ -431,6 +431,10 @@ sub scan_local_volumes { > # check if storage is available on both nodes > my $scfg = PVE::Storage::storage_check_enabled($storecfg, $sid); > > + # the custom firmware image is not owned by the VM, so it is never copied over > + die "custom firmware image is not available on the remote cluster\n" > + if $attr->{is_firmware} && $self->{opts}->{remote}; Nit: it might be if the user placed it there manually. So the error message should rather state something like "remote migration with custom BIOS firmware image is not possible". > + > my $targetsid = $sid; > # NOTE: local ignores shared mappings, remote maps them > if (!$scfg->{shared} || $self->{opts}->{remote}) { > @@ -440,6 +444,8 @@ sub scan_local_volumes { > $self->target_storage_check_available($storecfg, $targetsid, $volid); > return if $scfg->{shared} && !$self->{opts}->{remote}; > > + die "local custom firmware image - use a shared storage\n" if $attr->{is_firmware}; Should mention that it's the one from the BIOS and/or the volume ID for context. > + > $local_volumes->{$volid}->{ref} = 'pending' if $attr->{referenced_in_pending}; > $local_volumes->{$volid}->{ref} = 'snapshot' if $attr->{referenced_in_snapshot}; > $local_volumes->{$volid}->{ref} = 'unused' if $attr->{is_unused}; > diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm > index c288ba22..87baa1f0 100644 > --- a/src/PVE/QemuServer.pm > +++ b/src/PVE/QemuServer.pm > @@ -3259,6 +3259,9 @@ sub config_to_command { > push @$cmd, '-smbios', "type=1" . $smbios_string; > } > > + PVE::QemuServer::BIOS::check_bios(PVE::QemuServer::BIOS::parse_bios($conf->{bios})); > + PVE::QemuServer::OVMF::check_custom_firmware($storecfg, $conf); > + > if (get_bios_type($conf) eq 'ovmf') { > die "OVMF (UEFI) BIOS is not supported on 32-bit CPU types\n" > if !$forcecpu && get_cpu_bitness($conf->{cpu}, $arch) == 32; > @@ -4519,11 +4522,14 @@ sub foreach_volid { The get_replicatable_volumes() caller of foreach_volid() should skip volumes with the 'is_firmware' attribute. In vm_stop_cleanup(), there is a get_vm_volumes() call, but that actually should skip efi-firmware volumes, as those should not be deactivated there. You could add a flag to get_vm_volumes() for skipping volumes with the 'is_firmware' attribute. > $volhash->{$volid}->{is_tpmstate} //= 0; > $volhash->{$volid}->{is_tpmstate} = 1 if $key eq 'tpmstate0'; > > + $volhash->{$volid}->{is_firmware} //= 0; > + $volhash->{$volid}->{is_firmware} = 1 if $key eq 'bios'; > + > $volhash->{$volid}->{drivename} = $key if is_valid_drivename($key); > }; > > my $include_opts = { > - extra_keys => ['vmstate'], > + extra_keys => ['vmstate', 'bios'], > include_unused => 1, > }; > > @@ -6074,7 +6080,8 @@ sub get_vm_volumes { > return $vollist; > } > > -# Get volumes defined in the current VM configuration, including the VM state file. > +# Get volumes defined in the current VM configuration, including the VM state file and the custom > +# firmware image. > sub get_current_vm_volumes { > my ($storecfg, $conf) = @_; > > @@ -6082,12 +6089,17 @@ sub get_current_vm_volumes { > > PVE::QemuConfig->foreach_volume_full( > $conf, > - { extra_keys => ['vmstate'] }, > + { extra_keys => ['vmstate', 'bios'] }, > sub { > my ($ds, $drive) = @_; > > if (PVE::Storage::parse_volume_id($drive->{file}, 1)) { > - check_volume_storage_type($storecfg, $drive->{file}); > + if ($ds eq 'bios') { > + # report errors in the context of the custom firmware > + PVE::QemuServer::OVMF::check_custom_firmware($storecfg, $conf); > + } else { > + check_volume_storage_type($storecfg, $drive->{file}); > + } > push $volumes->@*, $drive->{file}; > } > }, > diff --git a/src/PVE/QemuServer/BIOS.pm b/src/PVE/QemuServer/BIOS.pm > index 4e1a3936..eecf877f 100644 > --- a/src/PVE/QemuServer/BIOS.pm > +++ b/src/PVE/QemuServer/BIOS.pm > @@ -11,6 +11,8 @@ our @EXPORT_OK = qw( > parse_bios > print_bios > get_bios_type > + get_bios_firmware > + check_bios > ); > > my $bios_fmt = { > @@ -20,6 +22,15 @@ my $bios_fmt = { > default_key => 1, > description => "Select BIOS implementation.", > }, > + firmware => { > + type => 'string', > + format => 'pve-volume-id', > + format_description => 'volume', > + description => "Custom firmware image to use instead of the one provided by the system." > + . " Must reference a volume with content type 'efi-firmware', for example" > + . " 'local:efi-firmware/custom.fd'. Only supported with the 'ovmf' BIOS type.", > + optional => 1, > + }, > }; > PVE::JSONSchema::register_format('pve-qm-bios', $bios_fmt); > > @@ -45,4 +56,21 @@ sub get_bios_type { > return parse_bios($conf->{bios})->{type} // 'seabios'; > } > > +# Returns the volume ID of the custom firmware image, if one is configured. > +sub get_bios_firmware { > + my ($conf) = @_; > + > + return parse_bios($conf->{bios})->{firmware}; > +} > + > +# Dies if the combination of settings in the parsed bios property-string is not supported. > +sub check_bios { > + my ($bios) = @_; Style nit: both the caller here and in the next patch use PVE::QemuServer::BIOS::check_bios(PVE::QemuServer::BIOS::parse_bios(...)) so you could also require the property string as a parameter and do the parsing within here instead. > + > + die "a custom firmware image requires the 'ovmf' BIOS type\n" > + if defined($bios->{firmware}) && $bios->{type} ne 'ovmf'; > + > + return; > +} > + > 1; ---snip 8<---