| Message ID | 20211021104259.57754-5-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | pci/iommu: Fail early if vfio-pci detected before vIOMMU | expand |
On 21.10.21 12:42, Peter Xu wrote: > The pci_bus_fn is similar to pci_bus_dev_fn that only takes a PCIBus* and an > opaque. The pci_bus_ret_fn is similar to pci_bus_fn but it allows to return a > void* pointer. > > Use them where proper in pci.[ch], and to be used elsewhere. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- Reviewed-by: David Hildenbrand <david@redhat.com> But should we just have squashed that into patch #1?
On 10/21/21 12:42 PM, Peter Xu wrote: > The pci_bus_fn is similar to pci_bus_dev_fn that only takes a PCIBus* and an > opaque. The pci_bus_ret_fn is similar to pci_bus_fn but it allows to return a > void* pointer. > > Use them where proper in pci.[ch], and to be used elsewhere. > > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Eric Auger <eric.auger@redhat.com> Eric > --- > hw/pci/pci.c | 6 ++---- > include/hw/pci/pci.h | 12 +++++------- > 2 files changed, 7 insertions(+), 11 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 6b834cace5..4a84e478ce 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -2072,10 +2072,8 @@ static PCIBus *pci_find_bus_nr(PCIBus *bus, int bus_num) > return NULL; > } > > -void pci_for_each_bus_depth_first(PCIBus *bus, > - void *(*begin)(PCIBus *bus, void *parent_state), > - void (*end)(PCIBus *bus, void *state), > - void *parent_state) > +void pci_for_each_bus_depth_first(PCIBus *bus, pci_bus_ret_fn begin, > + pci_bus_fn end, void *parent_state) > { > PCIBus *sec; > void *state; > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index 437eabe609..a7e81f04d3 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -402,6 +402,8 @@ OBJECT_DECLARE_TYPE(PCIBus, PCIBusClass, PCI_BUS) > #define TYPE_PCIE_BUS "PCIE" > > typedef void (*pci_bus_dev_fn)(PCIBus *b, PCIDevice *d, void *opaque); > +typedef void (*pci_bus_fn)(PCIBus *b, void *opaque); > +typedef void* (*pci_bus_ret_fn)(PCIBus *b, void *opaque); > > bool pci_bus_is_express(PCIBus *bus); > > @@ -470,17 +472,13 @@ void pci_for_each_device_under_bus(PCIBus *bus, > void pci_for_each_device_under_bus_reverse(PCIBus *bus, > pci_bus_dev_fn fn, > void *opaque); > -void pci_for_each_bus_depth_first(PCIBus *bus, > - void *(*begin)(PCIBus *bus, void *parent_state), > - void (*end)(PCIBus *bus, void *state), > - void *parent_state); > +void pci_for_each_bus_depth_first(PCIBus *bus, pci_bus_ret_fn begin, > + pci_bus_fn end, void *parent_state); > PCIDevice *pci_get_function_0(PCIDevice *pci_dev); > > /* Use this wrapper when specific scan order is not required. */ > static inline > -void pci_for_each_bus(PCIBus *bus, > - void (*fn)(PCIBus *bus, void *opaque), > - void *opaque) > +void pci_for_each_bus(PCIBus *bus, pci_bus_fn fn, void *opaque) > { > pci_for_each_bus_depth_first(bus, NULL, fn, opaque); > }
On 10/21/21 12:42, Peter Xu wrote: > The pci_bus_fn is similar to pci_bus_dev_fn that only takes a PCIBus* and an > opaque. The pci_bus_ret_fn is similar to pci_bus_fn but it allows to return a > void* pointer. > > Use them where proper in pci.[ch], and to be used elsewhere. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > hw/pci/pci.c | 6 ++---- > include/hw/pci/pci.h | 12 +++++------- > 2 files changed, 7 insertions(+), 11 deletions(-) > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -402,6 +402,8 @@ OBJECT_DECLARE_TYPE(PCIBus, PCIBusClass, PCI_BUS) > #define TYPE_PCIE_BUS "PCIE" > > typedef void (*pci_bus_dev_fn)(PCIBus *b, PCIDevice *d, void *opaque); > +typedef void (*pci_bus_fn)(PCIBus *b, void *opaque); > +typedef void* (*pci_bus_ret_fn)(PCIBus *b, void *opaque); $ git grep -F '* (*'|wc -l 12 $ git grep -F ' *(*'|wc -l 88 Nitpicking but I'd rather follow the project typedef style, otherwise: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On 10/21/21 13:44, Philippe Mathieu-Daudé wrote: > On 10/21/21 12:42, Peter Xu wrote: >> The pci_bus_fn is similar to pci_bus_dev_fn that only takes a PCIBus* and an >> opaque. The pci_bus_ret_fn is similar to pci_bus_fn but it allows to return a >> void* pointer. >> >> Use them where proper in pci.[ch], and to be used elsewhere. >> >> Signed-off-by: Peter Xu <peterx@redhat.com> >> --- >> hw/pci/pci.c | 6 ++---- >> include/hw/pci/pci.h | 12 +++++------- >> 2 files changed, 7 insertions(+), 11 deletions(-) > >> --- a/include/hw/pci/pci.h >> +++ b/include/hw/pci/pci.h >> @@ -402,6 +402,8 @@ OBJECT_DECLARE_TYPE(PCIBus, PCIBusClass, PCI_BUS) >> #define TYPE_PCIE_BUS "PCIE" >> >> typedef void (*pci_bus_dev_fn)(PCIBus *b, PCIDevice *d, void *opaque); >> +typedef void (*pci_bus_fn)(PCIBus *b, void *opaque); >> +typedef void* (*pci_bus_ret_fn)(PCIBus *b, void *opaque); Now looking at patch #8, I wonder if it wouldn't be cleaner to have a single: /* * pci_bus_fn: * @bus: PCI bus * @opaque: Pointer to opaque pointer, can be used as input *and* output * Return #true on success, #false on failure */ typedef bool (*pci_bus_fn)(PCIBus *bus, void **opaque);
On Thu, Oct 21, 2021 at 02:54:44PM +0200, Philippe Mathieu-Daudé wrote: > On 10/21/21 13:44, Philippe Mathieu-Daudé wrote: > > On 10/21/21 12:42, Peter Xu wrote: > >> The pci_bus_fn is similar to pci_bus_dev_fn that only takes a PCIBus* and an > >> opaque. The pci_bus_ret_fn is similar to pci_bus_fn but it allows to return a > >> void* pointer. > >> > >> Use them where proper in pci.[ch], and to be used elsewhere. > >> > >> Signed-off-by: Peter Xu <peterx@redhat.com> > >> --- > >> hw/pci/pci.c | 6 ++---- > >> include/hw/pci/pci.h | 12 +++++------- > >> 2 files changed, 7 insertions(+), 11 deletions(-) > > > >> --- a/include/hw/pci/pci.h > >> +++ b/include/hw/pci/pci.h > >> @@ -402,6 +402,8 @@ OBJECT_DECLARE_TYPE(PCIBus, PCIBusClass, PCI_BUS) > >> #define TYPE_PCIE_BUS "PCIE" > >> > >> typedef void (*pci_bus_dev_fn)(PCIBus *b, PCIDevice *d, void *opaque); > >> +typedef void (*pci_bus_fn)(PCIBus *b, void *opaque); > >> +typedef void* (*pci_bus_ret_fn)(PCIBus *b, void *opaque); > > Now looking at patch #8, I wonder if it wouldn't be cleaner to have > a single: > > /* > * pci_bus_fn: > * @bus: PCI bus > * @opaque: Pointer to opaque pointer, > can be used as input *and* output > * Return #true on success, #false on failure > */ > typedef bool (*pci_bus_fn)(PCIBus *bus, void **opaque); Having the errp could be better imho for patch 8 so we can allow the per-device hook to set the exact error message. That could be useful when we extend the list of devices to scan (besides vfio-pci) so we can set different error messages rather than return "false" for all. I'll stole the rb and I'll adjust the "void* ()". Thanks for looking, Phil.
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 6b834cace5..4a84e478ce 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -2072,10 +2072,8 @@ static PCIBus *pci_find_bus_nr(PCIBus *bus, int bus_num) return NULL; } -void pci_for_each_bus_depth_first(PCIBus *bus, - void *(*begin)(PCIBus *bus, void *parent_state), - void (*end)(PCIBus *bus, void *state), - void *parent_state) +void pci_for_each_bus_depth_first(PCIBus *bus, pci_bus_ret_fn begin, + pci_bus_fn end, void *parent_state) { PCIBus *sec; void *state; diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index 437eabe609..a7e81f04d3 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -402,6 +402,8 @@ OBJECT_DECLARE_TYPE(PCIBus, PCIBusClass, PCI_BUS) #define TYPE_PCIE_BUS "PCIE" typedef void (*pci_bus_dev_fn)(PCIBus *b, PCIDevice *d, void *opaque); +typedef void (*pci_bus_fn)(PCIBus *b, void *opaque); +typedef void* (*pci_bus_ret_fn)(PCIBus *b, void *opaque); bool pci_bus_is_express(PCIBus *bus); @@ -470,17 +472,13 @@ void pci_for_each_device_under_bus(PCIBus *bus, void pci_for_each_device_under_bus_reverse(PCIBus *bus, pci_bus_dev_fn fn, void *opaque); -void pci_for_each_bus_depth_first(PCIBus *bus, - void *(*begin)(PCIBus *bus, void *parent_state), - void (*end)(PCIBus *bus, void *state), - void *parent_state); +void pci_for_each_bus_depth_first(PCIBus *bus, pci_bus_ret_fn begin, + pci_bus_fn end, void *parent_state); PCIDevice *pci_get_function_0(PCIDevice *pci_dev); /* Use this wrapper when specific scan order is not required. */ static inline -void pci_for_each_bus(PCIBus *bus, - void (*fn)(PCIBus *bus, void *opaque), - void *opaque) +void pci_for_each_bus(PCIBus *bus, pci_bus_fn fn, void *opaque) { pci_for_each_bus_depth_first(bus, NULL, fn, opaque); }
The pci_bus_fn is similar to pci_bus_dev_fn that only takes a PCIBus* and an opaque. The pci_bus_ret_fn is similar to pci_bus_fn but it allows to return a void* pointer. Use them where proper in pci.[ch], and to be used elsewhere. Signed-off-by: Peter Xu <peterx@redhat.com> --- hw/pci/pci.c | 6 ++---- include/hw/pci/pci.h | 12 +++++------- 2 files changed, 7 insertions(+), 11 deletions(-)