On Sun, Jul 07, 2024 at 09:10:56PM -0700, Dongli Zhang wrote:
> When the devfn is already assigned in the command line, the
> do_pci_register_device() may verify if the function 0 is already occupied.
>
> However, when devfn < 0, the verification is skipped because it is part of
> the last "else if".
>
> For instance, suppose there is already a device at addr=00.00 of a port.
>
> -device pcie-root-port,bus=pcie.0,chassis=115,id=port01,addr=0e.00 \
> -device virtio-net-pci,bus=port01,id=vnet01,addr=00.00 \
>
> When 'addr' is specified for the 2nd device, the hotplug is denied.
>
> (qemu) device_add virtio-net-pci,bus=port01,id=vnet02,addr=01.00
> Error: PCI: slot 0 function 0 already occupied by virtio-net-pci, new func
> virtio-net-pci cannot be exposed to guest.
>
> When 'addr' is automatically assigned, the hotplug is not denied. This is
> because the verification is skipped.
>
> (qemu) device_add virtio-net-pci,bus=port01,id=vnet02
> warning: PCI: slot 1 is not valid for virtio-net-pci, parent device only
> allows plugging into slot 0.
>
> Fix the issue by moving the verification into an independent 'if'
> statement.
>
> Fixes: 3f1e1478db2d ("enable multi-function hot-add")
> Reported-by: Aswin Unnikrishnan <aswin.u.unnikrish...@oracle.com>
> Signed-off-by: Dongli Zhang <dongli.zh...@oracle.com>
Well, the user did nothing wrong here.
Either we forbid a mix of manual and auto-assigned functions,
or we make it work correctly.
Also note such change in behaviour will need a compat knob
and only affect new machine types.
> ---
> hw/pci/pci.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 4c7be52951..82ebd243d0 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1186,14 +1186,15 @@ static PCIDevice *do_pci_register_device(PCIDevice
> *pci_dev,
> PCI_SLOT(devfn), PCI_FUNC(devfn), name,
> bus->devices[devfn]->name, bus->devices[devfn]->qdev.id);
> return NULL;
> - } /*
> - * Populating function 0 triggers a scan from the guest that
> - * exposes other non-zero functions. Hence we need to ensure that
> - * function 0 wasn't added yet.
> - */
> - else if (dev->hotplugged &&
> - !pci_is_vf(pci_dev) &&
> - pci_get_function_0(pci_dev)) {
> + }
> +
> + /*
> + * Populating function 0 triggers a scan from the guest that
> + * exposes other non-zero functions. Hence we need to ensure that
> + * function 0 wasn't added yet.
> + */
> + if (dev->hotplugged && !pci_is_vf(pci_dev) &&
> + pci_get_function_0(pci_dev)) {
> error_setg(errp, "PCI: slot %d function 0 already occupied by %s,"
> " new func %s cannot be exposed to guest.",
> PCI_SLOT(pci_get_function_0(pci_dev)->devfn),
> --
> 2.34.1
