Re: [PATCH qemu-server 1/5] drive: improve and extend TPM drive size helpers

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

Nit: "improve and extend" could be replaced with something more concrete

Am 19.10.26 um 11:53 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, the TPM silently
> ignores it. Account for this properly.

Here "the TPM silently ignores it" and "Account for this properly" are
also rather vague. You can mention more concretely that swtpm can deal
with block-based volumes larger than 4 MiB and that this needs to be
accounted for migration.

> Adjust the 'update_tpmstate_size' subroutine so that it checks for the
> actual TPM drive size, but falls back on the constant to ensure a
> minimum size of 4MiB.

I'd move this to after the next paragraph and mention that the new
method is used in update_tpmstate_size, and that this addresses migration.

> 
> Introduce a new 'get_tpmstate_size' subroutine that only determines
> the current best size and returns it as bytes.

What does "current best size" mean? Again, I suggest mentioning what it
is concretely, i.e. "the maximum of the current size and the static size
of 4 MiB".

> 
> Suggested-by: Fiona Ebner <f.ebner@proxmox.com>
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/PVE/QemuMigrate.pm |  7 ++++---
>  src/PVE/QemuServer.pm  | 25 +++++++++++++++++++++++--

Pre-existing, but please move/add the helpers to the Drive module
instead. Your commit title is very suggestive of this too ;)

>  2 files changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
> index 6605f5d8..3845fb41 100644
> --- a/src/PVE/QemuMigrate.pm
> +++ b/src/PVE/QemuMigrate.pm
> @@ -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

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".

> +    # transfer, update the TPM disk size.
>      if (defined($conf->{tpmstate0})) {
> -        PVE::QemuServer::update_tpmstate_size($conf);
> +        PVE::QemuServer::update_tpmstate_size($conf, $self->{storecfg});
>      }
>  }
>  
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 135c993f..89192e9f 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -7972,14 +7972,35 @@ sub update_efidisk_size {
>      return;
>  }
>  
> +# Takes a VM config and the storage configuration. Parses the configuration,
> +# checks the drive size and updates it to a more up to date value and finally

I suggest replacing "more up to date value" with "the maximum of the
current size and the static size of 4 MiB".

> +# writes the new size back into the provided config.
>  sub update_tpmstate_size {
> -    my ($conf) = @_;
> +    my ($conf, $storecfg) = @_;
> +    return if !defined($conf) || !defined($storecfg) || !defined($conf->{tpmstate0});

I'd consider $conf and $storecfg required parameters, i.e. the function
should rather die if they are missing and not quietly return.

>  
>      my $disk = PVE::QemuServer::parse_drive('tpmstate0', $conf->{tpmstate0});

When moving to the Drive module, you can replace this with the proper,
i.e. non-imported, parse_drive() function.

> -    $disk->{size} = PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE;
> +
> +    $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.

Should mention that it uses the static TPMSTATE_DISK_SIZE as a minimum.

> +sub get_tpmstate_size {
> +    my ($volid, $storecfg) = @_;
> +
> +    return if !defined($volid) || !defined($storecfg);

Similarly here, those are required parameters.

> +
> +    my $size = eval { PVE::Storage::volume_size_info($storecfg, $volid, 10) };

Please print the error message as a warning if there is one.

> +
> +    if (!$size || $size < PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE) {
> +        $size = PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE;
> +    }
> +
> +    return $size;
> +}
> +
>  sub vm_iothreads_list {
>      my ($vmid) = @_;
>