[PATCH qemu-server 0/3] drive: account for differing TPM sizes

[PATCH qemu-server 0/3] drive: account for differing TPM sizes

[Thomas Ellmenreich]

Our current systems to handle TPM drives always assume that the disks have a
fixed 4MiB size that cannot be changed even if that may not be the case. In
situations where the granularity of the backing storage exceeds the 4MiB, an
allocation of a TPM drive will be at least as big as the granularity of said
backing storage.

To solve this mismatch in a very simple way, the default size is retained for
allocation. Only after the allocation do we check the actual size of the
created TPM disk and update the configuration accordingly. A minimum size of
4MiB is always applied to situations where that may not be the case.

In the case of movement and cloning of disks, we just refer to the
actual disk size instead of the TPM size constant. Doing so also makes it now
possible to move a larger (than 4MiB) TPM disk to a different storage.

Future work
-----------

The last two patches in the RFC performed a rescan of a VM's disks after a
migration. By doing so, any inaccuracies in the size of TPM disks would be
fixed, but also so much more. Since this solution was a bit too "brute", we
decided to drop it in favour of a future, cleaner, implementation.

After migration, TPM disks retain their previous size, even if the granularity
might have forced the size to increase. A future patch series should address
this by going through the disks of the migrated VM and adjusting the sizes
accordingly. The previous implementation is inadequate because it also reveals
orphaned volumes and does so without using the migration tunnel.

Changelog
---------

* RFC -> v1
    - Reworded commit messages and subroutine descriptions.

    - Subroutines now fail if the passed arguments are undefined.

    - Move TPM size utilities to QemuServer::Drive.

    - Utilities print a warning if getting the real size fails.

    - Removed the last 2 patches that rescanned all drives after a migration
      as that warrants more discussion.


qemu-server:

Thomas Ellmenreich (3):
  drive: update TPM size as max of current and minimum size
  api: create disks: update TPM size on creation
  clone disk: read actual size when cloning TPM drive

 src/PVE/API2/Qemu.pm        | 19 +++++++++++-----
 src/PVE/QemuMigrate.pm      |  9 ++++----
 src/PVE/QemuServer.pm       | 11 ++--------
 src/PVE/QemuServer/Drive.pm | 44 +++++++++++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+), 18 deletions(-)


Summary over all repositories:
  4 files changed, 65 insertions(+), 18 deletions(-)

-- 
Generated by murpp 0.12.0

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

[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
sizes and performes the max comparison, returing the ideal size.

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.
     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.
+sub update_tpmstate_size {
+    my ($conf, $storecfg) = @_;
+
+    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);
+
+    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.
+sub get_tpmstate_size {
+    my ($volid, $storecfg) = @_;
+
+    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);
+
+    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;
-- 
2.47.3

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

[Fiona Ebner]

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;

[PATCH qemu-server 2/3] api: create disks: update TPM size on creation

[Thomas Ellmenreich]

When creating a TPM drive, the default size is 4MiB. This default is
constant and cannot be changed, unless the backing storage has a bigger
granularity, in which case it is forcibly changed to the bigger size.

To directly show the correct size of a TPM drive, query its size after
creation, and set that as the drive size.

Suggested-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/API2/Qemu.pm | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index f4dbd2a1..cb5c17f0 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -34,7 +34,11 @@ use PVE::QemuServer::Blockdev;
 use PVE::QemuServer::BlockJob;
 use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CPUConfig;
-use PVE::QemuServer::Drive qw(checked_volume_format checked_parse_volname);
+use PVE::QemuServer::Drive qw(
+    checked_volume_format
+    checked_parse_volname
+    get_tpmstate_size
+);
 use PVE::QemuServer::Helpers;
 use PVE::QemuServer::ImportDisk;
 use PVE::QemuServer::Monitor qw(mon_cmd vm_qmp_peer);
@@ -669,13 +673,18 @@ my sub create_disks : prototype($$$$$$$$$$$) {
                         $storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm, $cvm_type,
                     );
                 } elsif ($ds eq 'tpmstate0') {
-                    # A fixed size is used for TPM state volumes
-                    $size = PVE::Tools::convert_size(
+                    # A fixed size is requested for TPM state volumes, but the
+                    # storage might increase it up to its allocation granularity
+                    my $target_size = PVE::Tools::convert_size(
                         PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE,
                         'b' => 'kb',
                     );
-                    $volid =
-                        PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
+                    $volid = PVE::Storage::vdisk_alloc(
+                        $storecfg, $storeid, $vmid, $fmt, undef, $target_size,
+                    );
+                    $size = PVE::Tools::convert_size(
+                        get_tpmstate_size($volid, $storecfg), 'b' => 'kb',
+                    );
                 } else {
                     $volid =
                         PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
-- 
2.47.3

[PATCH qemu-server 3/3] clone disk: read actual size when cloning TPM drive

[Thomas Ellmenreich]

When cloning a TPM drive, don't assume the default size of 4MiB, instead
read and then use the actual size. The default size is constant, but can
be changed, if the granularity of the backing storage is bigger than
4MiB.

This allows TPM volumes on storages with a larger granularity than 4MiB
be moved to different storages without producing an error.

Suggested-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/QemuServer.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 954acd05..0c130041 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -78,6 +78,7 @@ use PVE::QemuServer::Drive qw(
     parse_drive
     print_drive
     storage_allows_io_uring_default
+    get_tpmstate_size
 );
 use PVE::QemuServer::DriveDevice qw(print_drivedevice_full scsihw_infos);
 use PVE::QemuServer::Machine;
@@ -7888,7 +7889,7 @@ sub clone_disk {
         } elsif ($dst_drivename eq 'efidisk0') {
             $size = $efisize or die "internal error - need to specify EFI disk size\n";
         } elsif ($dst_drivename eq 'tpmstate0') {
-            $size = PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE;
+            $size = get_tpmstate_size($drive->{file}, $storecfg);
         } else {
             clone_disk_check_io_uring(
                 $vmid, $drive, $storecfg, $src_storeid, $storeid, $use_drive_mirror,
-- 
2.47.3