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 E9EA81FF0AA for ; Tue, 20 Oct 2026 14:29:11 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 0DEF921682; Tue, 20 Oct 2026 14:29:01 +0200 (CEST) Message-ID: <06b73d99-0f28-459c-bca5-74c3e8b40387@proxmox.com> Date: Tue, 20 Oct 2026 14:28:53 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH qemu-server 1/5] drive: improve and extend TPM drive size helpers To: Thomas Ellmenreich , pve-devel@lists.proxmox.com References: <20261019095346.175659-1-t.ellmenreich@proxmox.com> <20261019095346.175659-2-t.ellmenreich@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20261019095346.175659-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: 1792499333910 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.350 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: GYP3Y7TV2Q4DFC5CDUDTBFFNQ6UFMOVJ X-Message-ID-Hash: GYP3Y7TV2Q4DFC5CDUDTBFFNQ6UFMOVJ 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: 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 > Signed-off-by: Thomas Ellmenreich > --- > 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) = @_; >