[PATCH v2] hw/pci/pci: Fix slot check for plugged devices

[PATCH v2] hw/pci/pci: Fix slot check for plugged devices

[Julia Suvorova]

If devfn is assigned automatically, 'else' clauses will never be
executed. And if it does not matter for the reserved and available
devfn, because we have already checked it, the check for function0
needs to be done again.

Steps to reproduce:
1. Run QEMU with:
        -M q35 \
        -device pcie-root-port,id=rp,.. \
        -device pci-device,bus=rp
2. Add a new device to the same port:
        device_add pci-device,bus=rp

The last command will add the device to slot 1 instead of
failing with "PCI: slot 0 function 0 already occupied..."

Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
v2:
    * add example to the commit message [Michael]

 hw/pci/pci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index de0fae10ab..ae132b0b52 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1034,8 +1034,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
                    PCI_SLOT(devfn), PCI_FUNC(devfn), name,
                    bus->devices[devfn]->name);
         return NULL;
-    } else if (dev->hotplugged &&
-               pci_get_function_0(pci_dev)) {
+    };
+
+    if (dev->hotplugged && pci_get_function_0(pci_dev)) {
         error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
                    " new func %s cannot be exposed to guest.",
                    PCI_SLOT(pci_get_function_0(pci_dev)->devfn),
-- 
2.25.4

Re: [PATCH v2] hw/pci/pci: Fix slot check for plugged devices

[Stefano Garzarella]

On Tue, Oct 06, 2020 at 02:59:08PM +0200, Julia Suvorova wrote:
> If devfn is assigned automatically, 'else' clauses will never be
> executed. And if it does not matter for the reserved and available
> devfn, because we have already checked it, the check for function0
> needs to be done again.
> 
> Steps to reproduce:
> 1. Run QEMU with:
>         -M q35 \
>         -device pcie-root-port,id=rp,.. \
>         -device pci-device,bus=rp
> 2. Add a new device to the same port:
>         device_add pci-device,bus=rp
> 
> The last command will add the device to slot 1 instead of
> failing with "PCI: slot 0 function 0 already occupied..."
> 
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
> v2:
>     * add example to the commit message [Michael]
> 
>  hw/pci/pci.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index de0fae10ab..ae132b0b52 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1034,8 +1034,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>                     PCI_SLOT(devfn), PCI_FUNC(devfn), name,
>                     bus->devices[devfn]->name);
>          return NULL;
> -    } else if (dev->hotplugged &&
> -               pci_get_function_0(pci_dev)) {
> +    };
        ^
This semicolon seems unnecessary, can we take it out?

Thanks,
Stefano

> +
> +    if (dev->hotplugged && pci_get_function_0(pci_dev)) {
>          error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
>                     " new func %s cannot be exposed to guest.",
>                     PCI_SLOT(pci_get_function_0(pci_dev)->devfn),
> -- 
> 2.25.4
> 
> 

Re: [PATCH v2] hw/pci/pci: Fix slot check for plugged devices

[Julia Suvorova]

On Wed, Oct 7, 2020 at 9:39 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Tue, Oct 06, 2020 at 02:59:08PM +0200, Julia Suvorova wrote:
> > If devfn is assigned automatically, 'else' clauses will never be
> > executed. And if it does not matter for the reserved and available
> > devfn, because we have already checked it, the check for function0
> > needs to be done again.
> >
> > Steps to reproduce:
> > 1. Run QEMU with:
> >         -M q35 \
> >         -device pcie-root-port,id=rp,.. \
> >         -device pci-device,bus=rp
> > 2. Add a new device to the same port:
> >         device_add pci-device,bus=rp
> >
> > The last command will add the device to slot 1 instead of
> > failing with "PCI: slot 0 function 0 already occupied..."
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> > v2:
> >     * add example to the commit message [Michael]
> >
> >  hw/pci/pci.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index de0fae10ab..ae132b0b52 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1034,8 +1034,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> >                     PCI_SLOT(devfn), PCI_FUNC(devfn), name,
> >                     bus->devices[devfn]->name);
> >          return NULL;
> > -    } else if (dev->hotplugged &&
> > -               pci_get_function_0(pci_dev)) {
> > +    };
>         ^
> This semicolon seems unnecessary, can we take it out?

Oops, thanks for pointing out.