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 B65421FF0A5 for ; Fri, 04 Sep 2026 10:30:30 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id A8BD2215EA; Fri, 04 Sep 2026 10:30:17 +0200 (CEST) Message-ID: <80d7911d-77d5-40aa-af27-a0bac993858c@proxmox.com> Date: Fri, 4 Sep 2026 10:30:05 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH qemu-server 1/3] qemuserver: add pvpanic device To: Kaiyang Wu , pve-devel@lists.proxmox.com References: <20260827103529.393388-1-wukaiyang@loongfans.cn> <20260827103529.393388-2-wukaiyang@loongfans.cn> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20260827103529.393388-2-wukaiyang@loongfans.cn> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1788510601527 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.553 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: 3YIWXEMGWOHT3GCWEDSIME6N5RFV2QAZ X-Message-ID-Hash: 3YIWXEMGWOHT3GCWEDSIME6N5RFV2QAZ X-MailFrom: d.csapak@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 CC: Kaiyang Wu X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: comment inline On 8/27/26 12:35 PM, Kaiyang Wu wrote: > Add pvpanic device to monitor virtual machine system panics. Add pvpanic > field in virtual machine configuration files, allowing to choose the ISA > or the PCI variant of the pvpanic device. > > Signed-off-by: Kaiyang Wu > --- > src/PVE/API2/Qemu.pm | 1 + > src/PVE/QemuServer.pm | 19 +++++++++++++++++++ > src/PVE/QemuServer/PCI.pm | 1 + > 3 files changed, 21 insertions(+) > > diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm > index 71247eec..933f6221 100644 > --- a/src/PVE/API2/Qemu.pm > +++ b/src/PVE/API2/Qemu.pm > @@ -818,6 +818,7 @@ my $hwtypeoptions = { > 'watchdog' => 1, > 'audio0' => 1, > 'rng0' => 1, > + 'pvpanic0' => 1, > }; > > my $generaloptions = { > diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm > index 63d8c135..f476af94 100644 > --- a/src/PVE/QemuServer.pm > +++ b/src/PVE/QemuServer.pm > @@ -739,6 +739,13 @@ EODESCR > optional => 1, > default => 1, > }, > + pvpanic0 => { since it won't make sense (ever?) to add multiple panic devices, i think having simply 'pvpanic' is enough, no need to add an index > + optional => 1, > + type => 'string', > + description => "Configure a pvpanic device to monitor guest panics", > + enum => [qw(pvpanic pvpanic-pci)], is it really useful to have two variants? couldn't we always use the PCI variant or is there some specific usecase for the ISA one? It could even be simply an option somewhere else (e.g. on the machine maybe?) if we just have single option, this might simplify the code below... > + default => undef, > + }, > }; > > my $cicustom_fmt = { > @@ -3415,6 +3422,18 @@ sub config_to_command { > push @$cmd, '-nographic'; > } > > + my $pvpanic = $conf->{pvpanic0}; > + if (defined($pvpanic)) { > + if ($pvpanic eq 'pvpanic') { > + push @$devices, '-device', $pvpanic; > + } elsif ($pvpanic eq 'pvpanic-pci') { > + my $pvpanicpciaddr = print_pci_addr("pvpanic0", $bridges, $arch); > + push @$devices, '-device', "$pvpanic,id=pvpanic$pvpanicpciaddr"; > + } else { > + die "unknown pvpanic device type $pvpanic"; > + } > + } ... here. this would all collapse into a single --- if (defined()) { push ... } --- part > + > # For now, handles only specific parts, but the final goal is to cover everything. > my $cfg2cmd_opts = { forcemachine => $forcemachine }; > my $cfg2cmd = PVE::QemuServer::Cfg2Cmd->new($conf, $defaults, $version_guard, $cfg2cmd_opts); > diff --git a/src/PVE/QemuServer/PCI.pm b/src/PVE/QemuServer/PCI.pm > index 0b67943c..56770091 100644 > --- a/src/PVE/QemuServer/PCI.pm > +++ b/src/PVE/QemuServer/PCI.pm > @@ -220,6 +220,7 @@ sub get_pci_addr_map { > 'pci.4' => { bus => 1, addr => 28 }, > 'rng0' => { bus => 1, addr => 29 }, > 'pci.2-igd' => { bus => 1, addr => 30 }, # replaces pci.2 in case a legacy IGD device is passed through > + 'pvpanic0' => { bus => 1, addr => 31 }, > 'virtio6' => { bus => 2, addr => 1 }, > 'virtio7' => { bus => 2, addr => 2 }, > 'virtio8' => { bus => 2, addr => 3 }, side note (not really directed at you though)" urgh i really hate that we have to do it this way (finding free slots by checking each line if there is a gap...) I'm currently rewriting part of the pci addr handling so this should become a bit better in the future...