From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 9C1901FF146 for ; Tue, 07 Jul 2026 11:11:51 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D60ED2143C; Tue, 07 Jul 2026 11:11:50 +0200 (CEST) Message-ID: <8a406c16-c8d7-4b0f-85a0-02fd75ebcb62@proxmox.com> Date: Tue, 7 Jul 2026 11:11:15 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH qemu-server v3] fix #7711: pci: try to detect large memory region preconditions To: Dominik Csapak , pve-devel@lists.proxmox.com References: <20260706131036.3583490-1-d.csapak@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20260706131036.3583490-1-d.csapak@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: 1783415468899 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.036 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) 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: PWYCJ2XHZPXW57U6ERNDEJMUEO66CC4T X-Message-ID-Hash: PWYCJ2XHZPXW57U6ERNDEJMUEO66CC4T 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 06.07.26 um 3:10 PM schrieb Dominik Csapak: > @@ -5719,6 +5729,21 @@ sub vm_start_nolock { > } > } > push @$cmd, '-uuid', $uuid if defined($uuid); > + > + if ($need_min_phys_bits > 0) { > + my $size = render_bytes(2**$need_min_phys_bits); > + my $warn_text = > + "A PCI device with a large memory region (e.g. VRAM) was detected, but VM" > + . " is not configured for a big enough address space for OVMF (needs $size). Fails to build, there is a quote missing at the end. > + . " Consider enabling CPU type 'host' or setting 'phys-bits'" > + . " (at least $need_min_phys_bits)."; > + > + if ($need_min_phys_bits > 40) { > + $warn_text .= " You also need to set the 'pdpe1gb' flag."; > + } > + > + log_warn($warn_text); > + } > }; > if (my $err = $@) { > eval { PVE::Storage::deactivate_volumes($storecfg, $vollist); }; > diff --git a/src/PVE/QemuServer/PCI.pm b/src/PVE/QemuServer/PCI.pm > index 0b67943c..7c6a0c6b 100644 > --- a/src/PVE/QemuServer/PCI.pm > +++ b/src/PVE/QemuServer/PCI.pm > @@ -5,11 +5,13 @@ use strict; > > use IO::File; > > +use PVE::File; > use PVE::JSONSchema; > use PVE::Mapping::PCI; > use PVE::SysFSTools; > use PVE::Tools; > > +use PVE::QemuServer::CPUConfig; Sorry, that I only notice now, but I don't think we should pull in the CPUConfig here and have logic for CPU settings in the latter half of min_phys_bits_needed(). Let's pass along the CPU phys bits from outside instead. Maybe have an effective_phys_bits_heuristic() helper in CPUConfig to calculate it depending on bios/pdpe1gb flag. And we can skip the call to min_phys_bits_needed() up-front if we know that the check does not apply for the current CPU model/phys-bits setting. > use PVE::QemuServer::Helpers; > use PVE::QemuServer::Machine; > use PVE::QemuServer::PCI::Mdev; > @@ -871,4 +873,138 @@ sub reserve_pci_usage { > die $@ if $@; > } > > +=pod > + > +=head3 get_biggest_memory_region > + > + my $size = get_biggest_memory_region($pci_id); > + > +Returns the size of biggest memory region for a PCI device in bytes Nit: missing dot after sentence > +This can be used to check if the config is correct for having an MMIO size that is large enough. > + > +Parameters: > + > +=over > + > +=item C<$pci_id>: The PCI Id to get the biggest memory region from. > + > +=back > + > +=cut > + > +sub get_biggest_memory_region { > + my ($pci_id) = @_; > + > + $pci_id = PVE::SysFSTools::normalize_pci_id($pci_id); > + > + # read resource regions from sysfs > + my $resource_file = "/sys/bus/pci/devices/$pci_id/resource"; > + my $regions = PVE::File::file_get_contents($resource_file); > + > + # for each line parse start/end/flags. > + my $biggest_size = 0; > + for my $line (split('\n', $regions)) { > + if ($line =~ m/^0x([a-f0-9]{16})\s0x([a-f0-9]{16})\s0x([a-f0-9]{16})$/i) { > + # avoid warning when parsing long hex values with hex() > + no warnings 'portable'; # Support for 64-bit ints required > + > + my $start = hex($1); > + my $end = hex($2); > + my $flags = hex($3); > + > + # find largest memory region with 'IORESOURCE_MEM' flag (see include/linux/ioport.h in kernel source) Nit: comment too long > + if (($flags & 0x200) != 0) { > + # $end is inclusive, so + 1 for the overall size > + my $size = $end - $start + 1; > + if ($size > $biggest_size) { > + $biggest_size = $size; > + } > + } > + } Should we add an else branch warning about unexpected lines just to be sure? > + } > + > + return $biggest_size; > +} > + > +# return the max bit set by the given number s/by the given number/for the given number/ I'd mention that it's the position starting from 1. Otherwise, it could as well be the offset. Or maybe say "number of bits required to represent the number in binary"? The potential confusion with offset makes me wonder if we even need this helper. Or if having it inline and a good code comment might be better? But no big deal, both variants are fine. > +my sub get_max_set_bit { > + my ($num) = @_; > + return length(sprintf("%b", $num)); > +} > + > +=pod > + > +=head3 min_phys_bits_needed > + > + my $bits = min_phys_bits_needed($conf, $pci_id); > + > +Returns the minimum phys-bits value that needs to be configured so that the MMIO size is enough. > +For PCI devices with memory regions > 16G, the vm either has to: > +* boot with seabios > +* use 'host' type cpu > +* use high enough 'phys-bits' value (or 'host') and (possibly) 'pdpe1gb' Style nits: Note that this does not show up as a bullet point list when viewed with perldoc. > + > +return 0 if vm config does have enough MMIO space configured already s/return/Returns/ Missing dot at the end of the sentence. > + > +Parameters: > + > +=over > + > +=item C<$conf>: The config of the given VM. > + > +=item C<$pci_id>: The PCI ID to check. > + > +=back > + > +=cut > + > +sub min_phys_bits_needed { > + my ($conf, $pci_id) = @_; > + > + return 0 if ($conf->{bios} // 'seabios') eq 'seabios'; > + > + my $size = get_biggest_memory_region($pci_id); > + > + return 0 if $size <= 16 * 1024 * 1024 * 1024; > + > + # calculate the needed phys bits. The MMIO size needs to be larger than $size, > + # so the bits needed must be bigger than log2($size) + 3. So simply get the max set bit (via snprintf). Style nit: comment line too long > + # edk2 limits the mmio space to an eigth of the overall space. (PhysMemAddressWidth - 3) s/eigth/eighth/ > diff --git a/src/test/run_pci_memory_detection_tests.pl b/src/test/run_pci_memory_detection_tests.pl > new file mode 100755 > index 00000000..f9c246da > --- /dev/null > +++ b/src/test/run_pci_memory_detection_tests.pl > @@ -0,0 +1,162 @@ > +foreach my $test (@{$tests}) { Style nit: use for instead of foreach > + my $name = $test->{name}; > + my $expected = $test->{expected}; > + my $conf = $test->{conf}; > + for my $size (sort keys $region_pci_map->%*) { > + my $pciid = $region_pci_map->{$size}->{address}; > + my $actual = PVE::QemuServer::PCI::min_phys_bits_needed($conf, $pciid); > + > + is($actual, $expected->{$size}, "$name - $size"); > + } > + > +} > + > +done_testing();