Re: [PATCH qemu-server v3 10/19] bios: add a custom firmware sub-property

[Fiona Ebner <f.ebner@proxmox.com>]

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 <christian_ludwig@genua.de>
> ---
>  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<---