* [PATCH] hw/pci/pci: Fix slot check for plugged devices
@ 2020-09-23 9:26 Julia Suvorova
2020-09-23 15:02 ` Michael S. Tsirkin
0 siblings, 1 reply; 4+ messages in thread
From: Julia Suvorova @ 2020-09-23 9:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Igor Mammedov, Julia Suvorova, Michael S. Tsirkin
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(-)
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] hw/pci/pci: Fix slot check for plugged devices
2020-09-23 9:26 [PATCH] hw/pci/pci: Fix slot check for plugged devices Julia Suvorova
@ 2020-09-23 15:02 ` Michael S. Tsirkin
2020-09-23 15:57 ` Julia Suvorova
0 siblings, 1 reply; 4+ messages in thread
From: Michael S. Tsirkin @ 2020-09-23 15:02 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Igor Mammedov, qemu-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] hw/pci/pci: Fix slot check for plugged devices
2020-09-23 15:02 ` Michael S. Tsirkin
@ 2020-09-23 15:57 ` Julia Suvorova
2020-09-24 7:56 ` Michael S. Tsirkin
0 siblings, 1 reply; 4+ messages in thread
From: Julia Suvorova @ 2020-09-23 15:57 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Igor Mammedov, QEMU Developers
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] hw/pci/pci: Fix slot check for plugged devices
2020-09-23 15:57 ` Julia Suvorova
@ 2020-09-24 7:56 ` Michael S. Tsirkin
0 siblings, 0 replies; 4+ messages in thread
From: Michael S. Tsirkin @ 2020-09-24 7:56 UTC (permalink / raw)
To: Julia Suvorova; +Cc: Igor Mammedov, QEMU Developers
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
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-24 7:57 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-23 9:26 [PATCH] hw/pci/pci: Fix slot check for plugged devices Julia Suvorova
2020-09-23 15:02 ` Michael S. Tsirkin
2020-09-23 15:57 ` Julia Suvorova
2020-09-24 7:56 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).