| Message ID | 20230705115925.5339-7-anisinha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
> On 05-Jul-2023, at 5:29 PM, Ani Sinha <anisinha@redhat.com> wrote: > > This change is cosmetic. A comment is added explaining why we need to check for > the availability of function 0 when we hotplug a device. Please ignore this patch. Its a duplicate of one already sent with an updated patch summary. > > CC: mst@redhat.com > Signed-off-by: Ani Sinha <anisinha@redhat.com> > --- > hw/pci/pci.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 62b393dfb7..7aee3a7f12 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -1181,9 +1181,14 @@ 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; > - } 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. > + */ > + else 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.39.1 >
On Wed, Jul 05, 2023 at 05:33:31PM +0530, Ani Sinha wrote: > > > > On 05-Jul-2023, at 5:29 PM, Ani Sinha <anisinha@redhat.com> wrote: > > > > This change is cosmetic. A comment is added explaining why we need to check for > > the availability of function 0 when we hotplug a device. > > Please ignore this patch. Its a duplicate of one already sent with an updated patch summary. I'm not sure which one it is, sorry. Dropped this for now, repost later pls. > > > > CC: mst@redhat.com > > Signed-off-by: Ani Sinha <anisinha@redhat.com> > > --- > > hw/pci/pci.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index 62b393dfb7..7aee3a7f12 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -1181,9 +1181,14 @@ 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; > > - } 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. > > + */ > > + else 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.39.1 > >
> On 11-Jul-2023, at 1:13 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Jul 05, 2023 at 05:33:31PM +0530, Ani Sinha wrote: >> >> >>> On 05-Jul-2023, at 5:29 PM, Ani Sinha <anisinha@redhat.com> wrote: >>> >>> This change is cosmetic. A comment is added explaining why we need to check for >>> the availability of function 0 when we hotplug a device. >> >> Please ignore this patch. Its a duplicate of one already sent with an updated patch summary. > > I'm not sure which one it is, sorry. Dropped this for now, repost later Sure. Since this is only a comment addition, should I also CC qemu-trivial? > pls. > >>> >>> CC: mst@redhat.com >>> Signed-off-by: Ani Sinha <anisinha@redhat.com> >>> --- >>> hw/pci/pci.c | 11 ++++++++--- >>> 1 file changed, 8 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >>> index 62b393dfb7..7aee3a7f12 100644 >>> --- a/hw/pci/pci.c >>> +++ b/hw/pci/pci.c >>> @@ -1181,9 +1181,14 @@ 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; >>> - } 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. >>> + */ >>> + else 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.39.1 >>> >
11.07.2023 06:46, Ani Sinha wrote:
> Sure. Since this is only a comment addition, should I also CC qemu-trivial?
A comment change does not mean the change is trivial. It's a trivial in a sense
the code changes are trivial (actually not-existent), but the meaning of the comment
is not trivial at all. I for one know nothing about this.
So no, this particular change isn't for -trivial, please :)
/mjt
> On 11-Jul-2023, at 9:21 AM, Michael Tokarev <mjt@tls.msk.ru> wrote: > > 11.07.2023 06:46, Ani Sinha wrote: > >> Sure. Since this is only a comment addition, should I also CC qemu-trivial? > > A comment change does not mean the change is trivial. It's a trivial in a sense > the code changes are trivial (actually not-existent), but the meaning of the comment > is not trivial at all. I for one know nothing about this. This comment was already disucussed in qemu-devel between me, mst and Igor. Perhaps you missed it. https://patchwork.kernel.org/project/qemu-devel/patch/20230621024824.3779-1-anisinha@redhat.com/
11.07.2023 07:03, Ani Sinha wrote: > > >> On 11-Jul-2023, at 9:21 AM, Michael Tokarev <mjt@tls.msk.ru> wrote: >> >> 11.07.2023 06:46, Ani Sinha wrote: >> >>> Sure. Since this is only a comment addition, should I also CC qemu-trivial? >> >> A comment change does not mean the change is trivial. It's a trivial in a sense >> the code changes are trivial (actually not-existent), but the meaning of the comment >> is not trivial at all. I for one know nothing about this. > > This comment was already disucussed in qemu-devel between me, mst and Igor. Perhaps you missed it. It doesn't matter really. The thing is that it needs explanation, hence it is not "trivial", that's what I tried to say. It is trivial for sure for someone who knows this particular subsystem well enough, - I'm not one of them ;) /mjt
> On 11-Jul-2023, at 9:43 AM, Michael Tokarev <mjt@tls.msk.ru> wrote: > > 11.07.2023 07:03, Ani Sinha wrote: >>> On 11-Jul-2023, at 9:21 AM, Michael Tokarev <mjt@tls.msk.ru> wrote: >>> >>> 11.07.2023 06:46, Ani Sinha wrote: >>> >>>> Sure. Since this is only a comment addition, should I also CC qemu-trivial? >>> >>> A comment change does not mean the change is trivial. It's a trivial in a sense >>> the code changes are trivial (actually not-existent), but the meaning of the comment >>> is not trivial at all. I for one know nothing about this. >> This comment was already discussed in qemu-devel between me, mst and Igor. Perhaps you missed it. > > It doesn't matter really. The thing is that it needs explanation, hence it is not "trivial", > that's what I tried to say. It is trivial for sure for someone who knows this particular > subsystem well enough, - I'm not one of them ;) That’s ok. I was commenting on your statement " I for one know nothing about this.” I wanted to make sure that people know that patches (at least those I post) are going though the qemu-devel list in a transparent way. It's up to those who are interested or have expertise in a certain area to monitor the list and review/comment on patches. If they want to be explicitly CC’d they can add themselves to MAINTAINERS. The tooling would ensure that they are added in the CC when certain files are updated or modified in the patches.
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 62b393dfb7..7aee3a7f12 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -1181,9 +1181,14 @@ 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; - } 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. + */ + else 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),
This change is cosmetic. A comment is added explaining why we need to check for the availability of function 0 when we hotplug a device. CC: mst@redhat.com Signed-off-by: Ani Sinha <anisinha@redhat.com> --- hw/pci/pci.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)