Re: [PATCH qemu-server 1/3] qemuserver: add pvpanic device

[Dominik Csapak <d.csapak@proxmox.com>]

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 <wukaiyang@loongfans.cn>
> ---
>   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...