Re: [PATCH 1/1] pci: don't skip function 0 occupancy verification for devfn auto assign

[Michael S. Tsirkin]

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.unnikrishnan@oracle.com>
> Signed-off-by: Dongli Zhang <dongli.zhang@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