| Message ID | 20200923092636.118676-1-jusual@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/pci/pci: Fix slot check for plugged devices | expand |
On Wed, Sep 23, 2020 at 11:26:36AM +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. > > Signed-off-by: Julia Suvorova <jusual@redhat.com> This is just cosmetics right? I wouldn't describe this as a "fix" then - "simplify" would be clearer. > --- > 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
On Wed, Sep 23, 2020 at 5:03 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Sep 23, 2020 at 11:26:36AM +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. > > > > Signed-off-by: Julia Suvorova <jusual@redhat.com> > > This is just cosmetics right? I wouldn't describe this as > a "fix" then - "simplify" would be clearer. No, this is a bug fix. For example, if you had a root port with a device on it already, and you do 'device_add new_device,bus=the_same_root_port', then it will miss the last 'if' and will be added to slot 1. > > --- > > 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 >
On Wed, Sep 23, 2020 at 05:57:19PM +0200, Julia Suvorova wrote: > On Wed, Sep 23, 2020 at 5:03 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Wed, Sep 23, 2020 at 11:26:36AM +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. > > > > > > Signed-off-by: Julia Suvorova <jusual@redhat.com> > > > > This is just cosmetics right? I wouldn't describe this as > > a "fix" then - "simplify" would be clearer. > > No, this is a bug fix. For example, if you had a root port with a > device on it already, and you do > 'device_add new_device,bus=the_same_root_port', then it will miss the > last 'if' and will be added to slot 1. OK and I think that is the only example - if devfn is not supplied and is auto-assigned. Can you add this to commit log pls? > > > --- > > > 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 > >
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),
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. Signed-off-by: Julia Suvorova <jusual@redhat.com> --- hw/pci/pci.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)