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 996321FF0B3 for ; Wed, 28 Oct 2026 11:32:32 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id AB86D213A6; Wed, 28 Oct 2026 11:32:29 +0100 (CET) Message-ID: Date: Wed, 28 Oct 2026 11:32:24 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH qemu-server 1/3] drive: update TPM size as max of current and minimum size To: Thomas Ellmenreich , pve-devel@lists.proxmox.com References: <20261027101723.144576-1-t.ellmenreich@proxmox.com> <20261027101723.144576-2-t.ellmenreich@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20261027101723.144576-2-t.ellmenreich@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793183544386 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.337 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: V4VFEFIGG75Z7JMLFDCMIRKH7CJXCA73 X-Message-ID-Hash: V4VFEFIGG75Z7JMLFDCMIRKH7CJXCA73 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: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 > Signed-off-by: Thomas Ellmenreich > --- > 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;