Re: [PATCH qemu-server 1/3] drive: update TPM size as max of current and minimum size

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

Am 27.10.26 um 11:17 AM schrieb Thomas Ellmenreich:
> The size of a created TPM drive is always based on a constant of 4 MiB, but
> if the backing storage has a larger granularity than this, it is
> possible to create larger TPM drives. Currently there is no way to deal
> with TPM drives, larger then the constant (4MiB), this update to the
> helpers introduces this.
> 
> Adjust the 'update_tpmstate_size' subroutine so that sets the configured
> TPM state size as the max of the actual disk size and a constant
> minimum of 4MiB.
> 
> Introduce a new 'get_tpmstate_size' subroutine that gets the actual

Nit: s/gets/queries/

> sizes and performes the max comparison, returing the ideal size.

s/sizes/size/
s/performes/performs/
s/returing/returning/

> 
> Suggested-by: Fiona Ebner <f.ebner@proxmox.com>
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/PVE/QemuMigrate.pm      |  9 ++++----
>  src/PVE/QemuServer.pm       |  8 -------
>  src/PVE/QemuServer/Drive.pm | 44 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 49 insertions(+), 12 deletions(-)
> 
> diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
> index 96035a75..2c9960db 100644
> --- a/src/PVE/QemuMigrate.pm
> +++ b/src/PVE/QemuMigrate.pm
> @@ -30,7 +30,7 @@ use PVE::QemuMigrate::Helpers;
>  use PVE::QemuServer::Agent;
>  use PVE::QemuServer::BlockJob;
>  use PVE::QemuServer::CPUConfig;
> -use PVE::QemuServer::Drive qw(checked_volume_format);
> +use PVE::QemuServer::Drive qw(checked_volume_format update_tpmstate_size);
>  use PVE::QemuServer::Helpers qw(min_version);
>  use PVE::QemuServer::Machine;
>  use PVE::QemuServer::Monitor qw(mon_cmd vm_qmp_peer);
> @@ -685,10 +685,11 @@ sub config_update_local_disksizes {
>          PVE::QemuServer::update_efidisk_size($conf);
>      }
>  
> -    # TPM state might have an irregular filesize, to avoid problems on transfer
> -    # we always assume the static size of 4M to allocate on the target
> +    # TPM state might have an irregular file size, or have increased
> +    # significantly due to differences in granularity. To avoid problems on
> +    # transfer, update the TPM disk size.

Nit: I would replace "or have increased significantly due to differences
in granularity" with "or be larger than the static size of 4 MiB if the
storage has a larger granularity".

>      if (defined($conf->{tpmstate0})) {
> -        PVE::QemuServer::update_tpmstate_size($conf);
> +        update_tpmstate_size($conf, $self->{storecfg});
>      }
>  }
>  
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 1efeeb66..954acd05 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -7984,14 +7984,6 @@ sub update_efidisk_size {
>      return;
>  }
>  
> -sub update_tpmstate_size {
> -    my ($conf) = @_;
> -
> -    my $disk = PVE::QemuServer::parse_drive('tpmstate0', $conf->{tpmstate0});
> -    $disk->{size} = PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE;
> -    $conf->{tpmstate0} = print_drive($disk);
> -}
> -
>  sub vm_iothreads_list {
>      my ($vmid) = @_;
>  
> diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm
> index 0b573717..8cda01ef 100644
> --- a/src/PVE/QemuServer/Drive.pm
> +++ b/src/PVE/QemuServer/Drive.pm
> @@ -27,6 +27,8 @@ our @EXPORT_OK = qw(
>      parse_drive
>      print_drive
>      storage_allows_io_uring_default
> +    update_tpmstate_size
> +    get_tpmstate_size
>  );
>  
>  my $DROPPED_PROPERTIES = ['cyls', 'heads', 'secs', 'trans'];
> @@ -1240,4 +1242,46 @@ sub drive_qmp_peer {
>      return drive_uses_qsd_fuse($storecfg, $drive) ? qsd_qmp_peer($vmid) : vm_qmp_peer($vmid);
>  }
>  
> +# Takes a VM config and the storage configuration. Parses the configuration,
> +# checks the drive size and updates it to the maximum between the actual size
> +# and the static TPMSTATE_DISK_SIZE which is 4MiB.

Nit: Mentioning that it parses the configuration is quite explicit. I'd
also suggest replacing the latter part with "the result of
get_tpmstate_size()" then it won't get out of date.

> +sub update_tpmstate_size {
> +    my ($conf, $storecfg) = @_;

I think we should order $storecfg first for consistency with other
functions (the only exception in this module is get_scsi_device_type()).

> +
> +    die "a configuration has to be supplied in which to update the tpm state size"
> +        if !defined($conf);
> +
> +    die "the storage config has to be supplied to determine tpm state size"
> +        if !defined($storecfg);

I'm a bit on the edge if we really need these checks. It's not too
difficult to check that the callers pass along known-good values by
looking at the code. When we convert to function signatures, we'll get a
weaker version of this checks again (parameter count rather than
definedness).

If we do keep them, they should be prefixed with "internal error:" and
terminated with a newline.

> +
> +    return if !defined($conf->{tpmstate0});
> +
> +    my $disk = parse_drive('tpmstate0', $conf->{tpmstate0});
> +
> +    $disk->{size} = get_tpmstate_size($disk->{file}, $storecfg);
> +    $conf->{tpmstate0} = print_drive($disk);
> +}
> +
> +# Takes in a volume id and the storage configuration. Will then try to find
> +# the actual storage size of the TPM drive. The final value has a minimum
> +# determined by the TPMSTATE_DISK_SIZE static, which is 4MiB.

Nit: I'd replace "The final value ..." with "Returns ..." and the
sentence from above, i.e. "Returns the maximum between the actual size
and the static TPMSTATE_DISK_SIZE which is 4MiB." which is a bit easier
to read IMHO.

> +sub get_tpmstate_size {
> +    my ($volid, $storecfg) = @_;

Same here regarding parameter ordering.

> +
> +    die "a volume id has to be supplied to determine tpm state size"
> +        if !defined($volid);
> +
> +    die "the storage config has to be supplied to determine tpm state size"
> +        if !defined($storecfg);

Same here regarding the checks.

> +
> +    my $size = eval { PVE::Storage::volume_size_info($storecfg, $volid, 10) };
> +    warn "failed to get size of the tpm drive $volid: $@" if $@;
> +
> +    if (!$size || $size < TPMSTATE_DISK_SIZE) {
> +        $size = TPMSTATE_DISK_SIZE;
> +    }
> +
> +    return $size;
> +}
> +
>  1;