[7/8] pci: Add pci_for_each_device_all()

With all the prepared infrastructures, we can easily add one helper now to loop
over all the existing PCI devices across the whole system.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/pci/pci.c         | 25 +++++++++++++++++++++++++
 include/hw/pci/pci.h |  2 ++
 2 files changed, 27 insertions(+)

On Thu, Oct 21, 2021 at 06:42:58PM +0800, Peter Xu wrote:
> With all the prepared infrastructures, we can easily add one helper now to loop
> over all the existing PCI devices across the whole system.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/pci/pci.c         | 25 +++++++++++++++++++++++++
>  include/hw/pci/pci.h |  2 ++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 1623bc9099..5c970f0727 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2124,6 +2124,31 @@ void pci_for_each_root_bus(pci_bus_fn fn, void *opaque)
>      object_child_foreach_recursive(object_get_root(), pci_find_root_bus, &args);
>  }
>  
> +typedef struct {
> +    pci_bus_dev_fn fn;
> +    void *opaque;
> +} pci_bus_dev_args;

code style violation. CamelCase for structs pls.

> +
> +static void pci_single_bus_hook(PCIBus *bus, void *opaque)
> +{
> +    pci_bus_dev_args *args = opaque;
> +
> +    pci_for_each_device_under_bus(bus, args->fn, args->opaque);
> +}
> +
> +static void pci_root_bus_hook(PCIBus *bus, void *opaque)
> +{
> +    assert(pci_bus_is_root(bus));
> +    pci_for_each_bus(bus, pci_single_bus_hook, opaque);
> +}
> +
> +void pci_for_each_device_all(pci_bus_dev_fn fn, void *opaque)
> +{
> +    pci_bus_dev_args args = { .fn = fn, .opaque = opaque };
> +
> +    pci_for_each_root_bus(pci_root_bus_hook, &args);
> +}
> +
>  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn)
>  {
>      bus = pci_find_bus_nr(bus, bus_num);
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 9e490d8969..1a862d1903 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -476,6 +476,8 @@ void pci_for_each_bus_depth_first(PCIBus *bus, pci_bus_ret_fn begin,
>                                    pci_bus_fn end, void *parent_state);
>  /* Call `fn' for each pci root bus on the system */
>  void pci_for_each_root_bus(pci_bus_fn fn, void *opaque);
> +/* Call 'fn' for each pci device on the system */
> +void pci_for_each_device_all(pci_bus_dev_fn fn, void *opaque);

Instead of hacking pci making initialization o(N^2),
can't we add a variant of object_resolve_path_type ?


>  PCIDevice *pci_get_function_0(PCIDevice *pci_dev);
>  
>  /* Use this wrapper when specific scan order is not required. */
> -- 
> 2.32.0

Hi, Michael,

On Thu, Oct 21, 2021 at 06:54:59AM -0400, Michael S. Tsirkin wrote:
> > +typedef struct {
> > +    pci_bus_dev_fn fn;
> > +    void *opaque;
> > +} pci_bus_dev_args;
> 
> code style violation. CamelCase for structs pls.

OK.

> > +/* Call 'fn' for each pci device on the system */
> > +void pci_for_each_device_all(pci_bus_dev_fn fn, void *opaque);
> 
> Instead of hacking pci making initialization o(N^2),

Why it's O(N^2)?  One vIOMMU walks O(N), and we only have one vIOMMU, or am I
wrong?

> can't we add a variant of object_resolve_path_type ?

Could you elaborate?  Here what we want to do is to make sure there're no
specific PCI devices registered, and potentially it can be more than one type
of device in the future.

Thanks,

On Fri, Oct 22, 2021 at 10:33:15AM +0800, Peter Xu wrote:
> Hi, Michael,
> 
> On Thu, Oct 21, 2021 at 06:54:59AM -0400, Michael S. Tsirkin wrote:
> > > +typedef struct {
> > > +    pci_bus_dev_fn fn;
> > > +    void *opaque;
> > > +} pci_bus_dev_args;
> > 
> > code style violation. CamelCase for structs pls.
> 
> OK.
> 
> > > +/* Call 'fn' for each pci device on the system */
> > > +void pci_for_each_device_all(pci_bus_dev_fn fn, void *opaque);
> > 
> > Instead of hacking pci making initialization o(N^2),
> 
> Why it's O(N^2)?  One vIOMMU walks O(N), and we only have one vIOMMU, or am I
> wrong?

What I meant is this is O(N) and if called M times will be O(N * M)
yes your patches only call once so O(N), still we can do better.

> > can't we add a variant of object_resolve_path_type ?
> 
> Could you elaborate?  Here what we want to do is to make sure there're no
> specific PCI devices registered, and potentially it can be more than one type
> of device in the future.
> 
> Thanks,

All you seem to care about is checking there's no VFIO
(why - should really be documented in a code comment much more clearly).
Looks like object_resolve_path_type does that with O(1) complexity.
If we need a variant that checks for multiple types we can add that.

> -- 
> Peter Xu

On Fri, Oct 22, 2021 at 04:43:43AM -0400, Michael S. Tsirkin wrote:
> On Fri, Oct 22, 2021 at 10:33:15AM +0800, Peter Xu wrote:
> > Hi, Michael,
> > 
> > On Thu, Oct 21, 2021 at 06:54:59AM -0400, Michael S. Tsirkin wrote:
> > > > +typedef struct {
> > > > +    pci_bus_dev_fn fn;
> > > > +    void *opaque;
> > > > +} pci_bus_dev_args;
> > > 
> > > code style violation. CamelCase for structs pls.
> > 
> > OK.
> > 
> > > > +/* Call 'fn' for each pci device on the system */
> > > > +void pci_for_each_device_all(pci_bus_dev_fn fn, void *opaque);
> > > 
> > > Instead of hacking pci making initialization o(N^2),
> > 
> > Why it's O(N^2)?  One vIOMMU walks O(N), and we only have one vIOMMU, or am I
> > wrong?
> 
> What I meant is this is O(N) and if called M times will be O(N * M)
> yes your patches only call once so O(N), still we can do better.

I see.

> 
> > > can't we add a variant of object_resolve_path_type ?
> > 
> > Could you elaborate?  Here what we want to do is to make sure there're no
> > specific PCI devices registered, and potentially it can be more than one type
> > of device in the future.
> > 
> > Thanks,
> 
> All you seem to care about is checking there's no VFIO
> (why - should really be documented in a code comment much more clearly).

Right, Alex asked the same question.  I'll make sure to mention that in the
commit message in the next version.

> Looks like object_resolve_path_type does that with O(1) complexity.
> If we need a variant that checks for multiple types we can add that.

It's still O(N), or am I wrong?  I mean for example there's the loop in
object_resolve_partial_path().

But yeah I can use that too if you prefer, it's just that when we want to
detect more types of pci classes it could be slower iiuc, because we'll need to
call object_resolve_path_type() once for each type.  For pci bus scan it's
always one round because we only have at most one x86 vIOMMU for each guest.

At the meantime, IMHO patch 1-6 are cleanups that should be good even without
patch 7/8. If we prefer object_resolve_path_type() I'd still think it would be
good to propose patch 1-6 separately (with some patch properly squashed as
suggested by reviewers)?

Thanks,

On Mon, Oct 25, 2021 at 08:57:36PM +0800, Peter Xu wrote:
> On Fri, Oct 22, 2021 at 04:43:43AM -0400, Michael S. Tsirkin wrote:
> > On Fri, Oct 22, 2021 at 10:33:15AM +0800, Peter Xu wrote:
> > > Hi, Michael,
> > > 
> > > On Thu, Oct 21, 2021 at 06:54:59AM -0400, Michael S. Tsirkin wrote:
> > > > > +typedef struct {
> > > > > +    pci_bus_dev_fn fn;
> > > > > +    void *opaque;
> > > > > +} pci_bus_dev_args;
> > > > 
> > > > code style violation. CamelCase for structs pls.
> > > 
> > > OK.
> > > 
> > > > > +/* Call 'fn' for each pci device on the system */
> > > > > +void pci_for_each_device_all(pci_bus_dev_fn fn, void *opaque);
> > > > 
> > > > Instead of hacking pci making initialization o(N^2),
> > > 
> > > Why it's O(N^2)?  One vIOMMU walks O(N), and we only have one vIOMMU, or am I
> > > wrong?
> > 
> > What I meant is this is O(N) and if called M times will be O(N * M)
> > yes your patches only call once so O(N), still we can do better.
> 
> I see.
> 
> > 
> > > > can't we add a variant of object_resolve_path_type ?
> > > 
> > > Could you elaborate?  Here what we want to do is to make sure there're no
> > > specific PCI devices registered, and potentially it can be more than one type
> > > of device in the future.
> > > 
> > > Thanks,
> > 
> > All you seem to care about is checking there's no VFIO
> > (why - should really be documented in a code comment much more clearly).
> 
> Right, Alex asked the same question.  I'll make sure to mention that in the
> commit message in the next version.
> 
> > Looks like object_resolve_path_type does that with O(1) complexity.
> > If we need a variant that checks for multiple types we can add that.
> 
> It's still O(N), or am I wrong?  I mean for example there's the loop in
> object_resolve_partial_path().

Only if there's a hash collision.

> But yeah I can use that too if you prefer, it's just that when we want to
> detect more types of pci classes it could be slower iiuc, because we'll need to
> call object_resolve_path_type() once for each type.  For pci bus scan it's
> always one round because we only have at most one x86 vIOMMU for each guest.
> 
> At the meantime, IMHO patch 1-6 are cleanups that should be good even without
> patch 7/8. If we prefer object_resolve_path_type() I'd still think it would be
> good to propose patch 1-6 separately (with some patch properly squashed as
> suggested by reviewers)?
> 
> Thanks,

OK let's handle that separately.

> -- 
> Peter Xu