Re: [PATCH qemu-server v2 3/4] pci: don't try to hotplug bridges

[Fiona Ebner <f.ebner@proxmox.com>]

Am 07.09.26 um 2:43 PM schrieb Dominik Csapak:
> While bridges can be hotplugged (on PCI on i440fx), no device can be
> hotplugged in that afterwards. For that to work the SHPC option would
> have to be enabled and the guest must support that. Since this is not
> guaranteed to work and bridges don't show up in our config, this could
> lead to bridges added that are not represented in the config.
> 
> To be on the safe side, simply don't allow hotplugging bridges at all.
> Luckily, the only bridge we ever tried hotplugging (since machine
> version 2.3) was pci.4 which only houses scsihw2/3/4 at the moment.
> 
> These are only used for scsiX where X > 13 and only if the scsihw is an
> LSI controller, so not very likely to occur.
> 
> This fixes an issue where trying to hotplug a scsi disk with index >=14
> on a i440fx machine with an LSI scsi controller would leave the bridge
> around after failing to add the scsi controller, and the machine would
> subsequently crash on live migration.
> 
> Fixes: 2513b862 (fix #2566: increase scsi limit to 31)
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/QemuServer.pm | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 149f17be..ab13bc41 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -3992,13 +3992,6 @@ sub vm_deviceplug {
>              warn $@ if $@;
>              die $err;
>          }
> -    } elsif (!$q35 && $deviceid =~ m/^(pci\.)(\d+)$/) {
> -        my $bridgeid = $2;
> -        my $pciaddr = print_pci_addr($deviceid, undef, $arch);
> -        my $devicefull = "pci-bridge,id=pci.$bridgeid,chassis_nr=$bridgeid$pciaddr";
> -
> -        qemu_deviceadd($vmid, $devicefull);
> -        qemu_deviceaddverify($vmid, $deviceid);
>      } else {
>          die "can't hotplug device '$deviceid'\n";
>      }

The single caller that passes pci.N to vm_deviceplug() is
qemu_add_pci_bridge(). That in turn has a single caller at the beginning
of vm_deviceplug(). But after removing the actual hotplug, the code
reads confusingly, since it still suggests that a bridge will be added.
I think we should either remove qemu_add_pci_bridge() altogether or turn
it into an assert_pci_bridge_present(), replacing its vm_deviceplug()
call with a die. The latter is nicer for getting clearer errors.