pci/arm: add PCI bus support
Subsystems (2)
cover pci/arm: add PCI bus support
This series extends the kvm-unit-tests/arm framework to support PCI.
Alexander Gordeev (11):
arm/pci: Device tree PCI probing
arm/pci: PCI bus scanning
arm/pci: Read devices BARs
arm/pci: Allocate and assign memory/io space resources arm/pci: Add pci_find_dev() and pci_bar_addr() functions arm/pci: PCI testdev existence test
arm/pci: PCI device operation test
arm/pci: PCI device read/write test
arm/pci: PCI host bridge info printing
arm/pci: PCI devices basic info printing arm/pci: PCI testdev test flavour printing
arm/pci-test.c | 32 +++ config/config-arm-common.mak | 6 +- lib/alloc.c | 3 - lib/libcflat.h | 3 + lib/pci-host-generic.c | 502 +++++++++++++++++++++++++++++++++++++++++++ lib/pci-host-generic.h | 37 ++++ lib/pci-testdev.c | 198 +++++++++++++++++ lib/pci.h | 33 +++ 8 files changed, 810 insertions(+), 4 deletions(-) create mode 100644 arm/pci-test.c create mode 100644 lib/pci-host-generic.c create mode 100644 lib/pci-host-generic.h create mode 100644 lib/pci-testdev.c create mode 100644 lib/pci.h
Signature · 1 line
1.8.3.1
PATCH 1/11 arm/pci: Device tree PCI probing
--- arm/pci-test.c | 25 +++++++ config/config-arm-common.mak | 5 +- lib/alloc.c | 3 - lib/libcflat.h | 3 + lib/pci-host-generic.c | 155 +++++++++++++++++++++++++++++++++++++++++++ lib/pci-host-generic.h | 26 ++++++++ lib/pci.h | 13 ++++ 7 files changed, 226 insertions(+), 4 deletions(-) create mode 100644 arm/pci-test.c create mode 100644 lib/pci-host-generic.c create mode 100644 lib/pci-host-generic.h create mode 100644 lib/pci.h
arm/pci-test.c new file +25 -0
@@ -0,0 +1,25 @@ +/* +* PCI bus operation test +* +* Copyright (C) 2015, Red Hat Inc, Andrew Jones <[email protected]> +* Copyright (C) 2015, Red Hat Inc, Alexander Gordeev <[email protected]> +* +* This work is licensed under the terms of the GNU LGPL, version 2. +*/ +#include <pci.h> + +int main (void ) +{ +struct pci *pci; + + pci =pci_dt_probe (); + +report ("PCI device tree probing" , pci); +if (!pci) + goto done; + +pci_shutdown (pci); + +done: +return report_summary (); +}
config/config-arm-common.mak +4 -1
@@ -11,7 +11,8 @@ endif tests-common = \ $(TEST_DIR)/selftest.flat \ - $(TEST_DIR)/spinlock-test.flat + $(TEST_DIR)/spinlock-test.flat \ + $(TEST_DIR)/pci-test.flat all: test_cases
@@ -29,6 +30,7 @@ include config/asm-offsets.mak cflatobjs += lib/alloc.o cflatobjs += lib/devicetree.o +cflatobjs += lib/pci-host-generic.o cflatobjs += lib/virtio.o cflatobjs += lib/virtio-mmio.o cflatobjs += lib/chr-testdev.o
@@ -70,3 +72,4 @@ test_cases: $(generated_files) $(tests-common) $(tests) $(TEST_DIR)/selftest.elf: $(cstart.o) $(TEST_DIR)/selftest.o $(TEST_DIR)/spinlock-test.elf: $(cstart.o) $(TEST_DIR)/spinlock-test.o +$(TEST_DIR)/pci-test.elf: $(cstart.o) $(TEST_DIR)/pci-test.o
lib/alloc.c +0 -3
@@ -7,9 +7,6 @@#include "asm/spinlock.h" #include "asm/io.h" -#define MIN (a, b) ((a) < (b) ? (a) : (b)) -#define MAX (a, b) ((a) > (b) ? (a) : (b)) -#define PHYS_ALLOC_NR_REGIONS 256struct phys_alloc_region {
lib/libcflat.h +3 -0
@@ -76,4 +76,7 @@ do { \abort (); \ } while (0) +#define MIN (a, b) ((a) < (b) ? (a) : (b)) +#define MAX (a, b) ((a) > (b) ? (a) : (b)) +#endif
lib/pci-host-generic.c new file +155 -0
155 lines not on this page · raw message
lib/pci-host-generic.h new file +26 -0
@@ -0,0 +1,26 @@ +#ifndef PCI_HOST_GENERIC_H +#define PCI_HOST_GENERIC_H + +struct pci_addr_range { +phys_addr_t start; +phys_addr_t size; +}; + +struct pci_addr_space { +struct pci_addr_range cpu_range; +struct pci_addr_range pci_range; +phys_addr_t alloc; +u32 of_flags; +}; + +struct pci_host_bridge { +struct pci_addr_range cpu_range; +int bus; +int bus_max; +int nr_addr_spaces; +struct pci_addr_space addr_space[]; +}; + +#define PCI_ECAM_BUS_SIZE (1 << 20) + +#endif
lib/pci.h new file +13 -0
@@ -0,0 +1,13 @@ +#ifndef PCI_H +#define PCI_H + +#include "alloc.h" + +struct pci { +void *sysdata; +}; + +extern struct pci *pci_dt_probe (void ); +extern void pci_shutdown (struct pci *pci); + +#endif
Signature · 1 line
1.8.3.1
Re: [kvm-unit-tests PATCH 01/11] arm/pci: Device tree PCI probing
Quoted 22 lines · Alexander Gordeev
Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- arm/pci-test.c | 25 +++++++ config/config-arm-common.mak | 5 +- lib/alloc.c | 3 - lib/libcflat.h | 3 + lib/pci-host-generic.c | 155 +++++++++++++++++++++++++++++++++++++++++++ lib/pci-host-generic.h | 26 ++++++++ lib/pci.h | 13 ++++ 7 files changed, 226 insertions(+), 4 deletions(-) create mode 100644 arm/pci-test.c create mode 100644 lib/pci-host-generic.c create mode 100644 lib/pci-host-generic.h create mode 100644 lib/pci.harm/pci-test.c new file +0
-0
arm/pci-test.c line 4
Quoted 4 lines
arm/pci-test.c new file +4
-0@@ -0,0 +1,25 @@ +/* +* PCI bus operation test +* +* Copyright (C) 2015, Red Hat Inc, Andrew Jones <[email protected]>
This has changed enough from the starter code I sent you that I don't need my name here.
arm/pci-test.c line 8
Quoted 4 lines
arm/pci-test.c new file +4
-0+* Copyright (C) 2015, Red Hat Inc, Alexander Gordeev <[email protected]> +* +* This work is licensed under the terms of the GNU LGPL, version 2. +*/
should explicitly include <libcflat.h> too
arm/pci-test.c line 15
Quoted 7 lines
arm/pci-test.c new file +7
-0+#include <pci.h> + +int main (void ) +{ +struct pci *pci; + + pci =pci_dt_probe ();
It would be better to only expose a 'pci_probe' to the unit tests. Internally pci_probe then determines it should use pci_dt_probe (unconditionally atm...)
Quoted 8 lines
arm/pci-test.c new file +6
-0+ +report ("PCI device tree probing" , pci); +if (!pci) + goto done; + +pci_shutdown (pci);From this patch it looks like we could call this function
pci_free,
and call it safely with a NULL pci parameter, but it appears some later patch will change that.
Quoted 71 lines
arm/pci-test.c new file +4
-0+ +done: +return report_summary (); +}config/config-arm-common.mak +4
-1@@ -11,7 +11,8 @@ endif tests-common = \ $(TEST_DIR)/selftest.flat \ - $(TEST_DIR)/spinlock-test.flat + $(TEST_DIR)/spinlock-test.flat \ + $(TEST_DIR)/pci-test.flat all: test_cases@@ -29,6 +30,7 @@ include config/asm-offsets.mak cflatobjs += lib/alloc.o cflatobjs += lib/devicetree.o +cflatobjs += lib/pci-host-generic.o cflatobjs += lib/virtio.o cflatobjs += lib/virtio-mmio.o cflatobjs += lib/chr-testdev.o@@ -70,3 +72,4 @@ test_cases: $(generated_files) $(tests-common) $(tests) $(TEST_DIR)/selftest.elf: $(cstart.o) $(TEST_DIR)/selftest.o $(TEST_DIR)/spinlock-test.elf: $(cstart.o) $(TEST_DIR)/spinlock-test.o +$(TEST_DIR)/pci-test.elf: $(cstart.o) $(TEST_DIR)/pci-test.olib/alloc.c +0
-3@@ -7,9 +7,6 @@#include "asm/spinlock.h" #include "asm/io.h" -#define MIN (a, b) ((a) < (b) ? (a) : (b)) -#define MAX (a, b) ((a) > (b) ? (a) : (b)) -#define PHYS_ALLOC_NR_REGIONS 256struct phys_alloc_region {lib/libcflat.h +3
-0@@ -76,4 +76,7 @@ do { \abort (); \ } while (0) +#define MIN (a, b) ((a) < (b) ? (a) : (b)) +#define MAX (a, b) ((a) > (b) ? (a) : (b)) +#endif lib/pci-host-generic.c new file +8
-0@@ -0,0 +1,155 @@ +/* +* Adapated from +* drivers/pci/host/pci-host-generic.c +* +* Copyright (C) 2015, Red Hat Inc, Andrew Jones <[email protected]> +* Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]> +* +* This work is licensed under the terms of the GNU LGPL, version 2.
lib/pci-host-generic.c line 18
Quoted 10 lines
lib/pci-host-generic.c new file +10
-0+*/ +#include "libcflat.h" +#include "devicetree.h" +#include "asm/io.h" +#include "pci.h" +#include "pci-host-generic.h" +#include <linux/pci_regs.h> + +static void pci_host_addr_space_init (struct pci_addr_space as[],int nr_as, +u32 cells[],int nr_range_cells)
^ range_cells might be better
Quoted 8 lines
lib/pci-host-generic.c new file +8
-0+{ +int i; + +/* +* The PCI binding claims the numerical representation of a PCI +* address consists of three cells, encoded as follows: +* +* phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
lib/pci-host-generic.c line 36
Quoted 10 lines
lib/pci-host-generic.c new file +10
-0+* phys.mid cell: hhhhhhhh hhhhhhhh hhhhhhhh hhhhhhhh +* phys.lo cell: llllllll llllllll llllllll llllllll +* +* PCI device bus address and flags are encoded into phys.high +* PCI 64 bit address is encoded into phys.mid and phys.low +*/ + +for (i =0 ; i < nr_as; i++, as++) { + as->of_flags =fdt32_to_cpu (cells[0 ]); + as->pci_range.start =fdt64_to_cpu (*((fdt64_t *)&cells[1 ]));
This is kind of yucky. I'd rather see something like
(fdt32_to_cpu(cells[1]) << 32) | fdt32_to_cpu(cells[2])
same comment for the next few assignments below
Quoted 13 lines
lib/pci-host-generic.c new file +13
-0+ +if (nr_range_cells ==6 ) { + as->cpu_range.start =fdt32_to_cpu (cells[3 ]); + as->cpu_range.size = +fdt64_to_cpu (*((fdt64_t *)&cells[4 ])); + }else { + as->cpu_range.start = +fdt64_to_cpu (*((fdt64_t *)&cells[3 ]));; + as->cpu_range.size = +fdt64_to_cpu (*((fdt64_t *)&cells[5 ])); + } + + as->pci_range.size = as->cpu_range.size;
lib/pci-host-generic.c line 59
Quoted 10 lines
lib/pci-host-generic.c new file +10
-0+ + cells += nr_range_cells; + } +} + +/* +* Probe DT for a generic PCI host controller +* See kernel Documentation/devicetree/bindings/pci/host-generic-pci.txt +*/ +static struct pci_host_bridge *pci_host_bridge_probe (void )
This function probes DT, but there's no _dt_ in the name.
Quoted 12 lines
lib/pci-host-generic.c new file +12
-0+{ +struct pci_host_bridge *host; +const void *fdt =dt_fdt (); +const struct fdt_property *prop; +struct dt_pbus_reg base; +struct dt_device dt_dev; +struct dt_bus dt_bus; +u32 bus, bus_max; +u32 nac, nsc, nac_root, nsc_root; +u32 nr_range_cells, nr_addr_spaces; +int ret, node, len; +
lib/pci-host-generic.c line 81
Quoted 10 lines
lib/pci-host-generic.c new file +10
-0+if (!dt_available ()) +return NULL ; + +dt_bus_init_defaults (&dt_bus); +dt_device_init (&dt_dev, &dt_bus,NULL ); + + node =fdt_path_offset (fdt,"/" ); +assert (node >=0 ); + +assert (dt_get_nr_cells (node, &nac_root, &nsc_root) ==0 );
Sorry the starter code I sent you had statements inside asserts(). We avoid those now, see commit 18ab6cadf
lib/pci-host-generic.c line 85
Quoted 4 lines
lib/pci-host-generic.c new file +4
-0+assert (nac_root ==1 || nac_root ==2 ); + + node =fdt_subnode_offset (fdt, node,"pcie" ); +if (node == -FDT_ERR_NOTFOUND )
It would be good to print that the expected node is missing so the user knows what's up.
lib/pci-host-generic.c line 90
Quoted 5 lines
lib/pci-host-generic.c new file +5
-0+return NULL ; +assert (node >=0 ); + + prop =fdt_get_property (fdt, node,"device_type" , &len); +if (!prop ||strcmp ((char *)prop->data,"pci" ))
You should check that len is big enough to do the strcmp without an overflow. Also, again an error message to the user that the DT is missing an expected property would be good.
lib/pci-host-generic.c line 95
Quoted 5 lines
lib/pci-host-generic.c new file +5
-0+return NULL ; + + ret =fdt_node_check_compatible (fdt, node,"pci-host-ecam-generic" ); +assert (ret >=0 ); +if (ret !=0 )
need error message
lib/pci-host-generic.c line 103
Quoted 8 lines
lib/pci-host-generic.c new file +8
-0+return NULL ; + +dt_device_bind_node (&dt_dev, node); +assert (dt_pbus_get_base (&dt_dev, &base) ==0 ); + + prop =fdt_get_property (fdt, node,"bus-range" , &len); +if (prop ==NULL ) { +assert (len !=0 );
Let's change this to assert(len == -FDT_ERR_NOTFOUND)
lib/pci-host-generic.c line 105
Quoted 2 lines
lib/pci-host-generic.c new file +2
-0+ bus =0x00 ; + bus_max =0xff ;
Should we just use 'base.size / PCI_ECAM_BUS_SIZE - 1' here for bus_max, and then change the MIN() below to an assert, checking that bus_max matches what's expected for the size on the other path?
Quoted 58 lines
lib/pci-host-generic.c new file +50
-0+ }else { +u32 *data = (u32 *)prop->data; + bus =fdt32_to_cpu (data[0 ]); + bus_max =fdt32_to_cpu (data[1 ]); + } + bus_max =MIN (bus_max, (base.size /PCI_ECAM_BUS_SIZE ) -1 ); + +assert (dt_get_nr_cells (node, &nac, &nsc) ==0 ); +assert (nac ==3 && nsc ==2 ); + +assert (prop =fdt_get_property (fdt, node,"ranges" , &len)); + + nr_range_cells = nac + nsc + nac_root; + nr_addr_spaces = (len /4 ) / nr_range_cells; + +assert (host =malloc (sizeof (*host) + +sizeof (host->addr_space[0 ]) * nr_addr_spaces)); + + host->cpu_range.start = base.addr; + host->cpu_range.size = base.size; + host->bus = bus; + host->bus_max = bus_max; + host->nr_addr_spaces = nr_addr_spaces; + +pci_host_addr_space_init (&host->addr_space[0 ], nr_addr_spaces, + (u32 *)prop->data, nr_range_cells); + +return host; +} + +struct pci *pci_dt_probe (void ) +{ +struct pci_host_bridge *host; +struct pci *pci; + + host =pci_host_bridge_probe (); +if (!host) +return NULL ; + +assert (pci =calloc (1 ,sizeof (*pci))); + pci->sysdata = host; + +return pci; +} + +void pci_shutdown (struct pci *pci) +{ +free (pci->sysdata); +free (pci); +}lib/pci-host-generic.h new file +2
-0@@ -0,0 +1,26 @@ +#ifndef PCI_HOST_GENERIC_H +#define PCI_HOST_GENERIC_H
lib/pci-host-generic.h line 12
Quoted 10 lines
lib/pci-host-generic.h new file +10
-0+ +struct pci_addr_range { +phys_addr_t start; +phys_addr_t size; +}; + +struct pci_addr_space { +struct pci_addr_range cpu_range; +struct pci_addr_range pci_range; +phys_addr_t alloc;
alloc isn't used in this patch, but guess it will be later
lib/pci-host-generic.h line 24
Quoted 12 lines
lib/pci-host-generic.h new file +12
-0+u32 of_flags; +}; + +struct pci_host_bridge { +struct pci_addr_range cpu_range; +int bus; +int bus_max; +int nr_addr_spaces; +struct pci_addr_space addr_space[]; +}; + +#define PCI_ECAM_BUS_SIZE (1 << 20)
Why not put this struct declaration (and supporting structs) in pci.h? Linux has struct pci_host_bridge in include/linux/pci.h
lib/pci.h line 2
Quoted 10 lines
lib/pci-host-generic.h new file +2
-0+ +#endif lib/pci.h new file +2
-0@@ -0,0 +1,13 @@ +#ifndef PCI_H +#define PCI_H
This file needs its copyright/gpl header. I also like to document the lib here in the header file, e.g. see lib/devicetree.h and lib/alloc.h
lib/pci.h line 7
Quoted 5 lines
lib/pci.h new file +5
-0+ +#include "alloc.h" + +struct pci { +void *sysdata;
The choice of sysdata for the name appears to come from the Linux kernel. Why not also call 'struct pci' 'struct pci_dev' for full consistency?
Quoted 9 lines
lib/pci.h new file +6
-0+}; + +extern struct pci *pci_dt_probe (void ); +extern void pci_shutdown (struct pci *pci); + +#endif Signature · 1 line
1.8.3.1
Re: [kvm-unit-tests PATCH 01/11] arm/pci: Device tree PCI probing
Quoted 4 lines · Andrew Jones
lib/pci-host-generic.c line 81
Quoted 1 line
lib/pci-host-generic.c new file +1
-0+assert (dt_get_nr_cells (node, &nac_root, &nsc_root) ==0 );Sorry the starter code I sent you had statements inside
asserts(). We avoid those now, see commit18ab6cadf
Hmm.. the commit does not appear addressing the described in changelog condition '...if somebody introduces a switch to "#define assert(...) /*nothing*/"...' as to-be-void asserts are not followed by return value checks (even when seemingly possible). If it worth following this pattern?
Thanks!
Re: [kvm-unit-tests PATCH 01/11] arm/pci: Device tree PCI probing
Quoted 6 lines · Alexander Gordeev
Quoted 4 lines · Andrew Jones
lib/pci-host-generic.c line 81
Quoted 1 line
lib/pci-host-generic.c new file +1
-0+assert (dt_get_nr_cells (node, &nac_root, &nsc_root) ==0 );Sorry the starter code I sent you had statements inside
asserts(). We avoid those now, see commit18ab6cadf
Quoted 6 lines
Hmm.. the commit does not appear addressing the described in changelog condition '...if somebody introduces a switch to "#define assert(...) /*nothing*/"...' as to-be-void asserts are not followed by return value checks (even when seemingly possible). If it worth following this pattern?
The asserts on return values that we've left are only there out of paranoia. It's highly unlikely the return codes will not be their expected values, but if they were, then it'd be good to halt immediately, rather than continue on making the issue more difficult to debug.
Now, it's also highly unlikely we'll ever run kvm-unit-tests with a no-op assert() function, but I'm glad Thomas suggested we avoid embedding actual functionality in them, as it's now the more correct way to use assert().
Thanks,
drew
Re: [kvm-unit-tests PATCH 01/11] arm/pci: Device tree PCI probing
In reply to Andrew JonesQuoted 21 lines · Andrew Jones
Quoted 8 lines
lib/pci-host-generic.h new file +2
-0@@ -0,0 +1,26 @@ +#ifndef PCI_HOST_GENERIC_H +#define PCI_HOST_GENERIC_H lib/pci-host-generic.h line 12
Quoted 10 lines
lib/pci-host-generic.h new file +10
-0+ +struct pci_addr_range { +phys_addr_t start; +phys_addr_t size; +}; + +struct pci_addr_space { +struct pci_addr_range cpu_range; +struct pci_addr_range pci_range; +phys_addr_t alloc;alloc isn't used in this patch, but guess it will be later
Quoted 15 lines
lib/pci-host-generic.h line 24
Quoted 12 lines
lib/pci-host-generic.h new file +12
-0+u32 of_flags; +}; + +struct pci_host_bridge { +struct pci_addr_range cpu_range; +int bus; +int bus_max; +int nr_addr_spaces; +struct pci_addr_space addr_space[]; +}; + +#define PCI_ECAM_BUS_SIZE (1 << 20)Why not put this struct declaration (and supporting structs) in
pci.h? Linux has structpci_host_bridgeininclude/linux/pci.h
I think we do not need any of these structures to get public.
My initial attempt was also an assessment of an idea to generalize PCI code across architectures. But my current judgement - an outcome is not worth it. With x86 PCI access so much different and only bus 0 function 0 "hierarchy" we only need the simplest required routines.
Hence, I would leave these in pci-host-generic.h (just not to blow pci-host-generic.c too big).
As result, just these routines would end up in pci.h:
bool pci_probe(void); void pci_free(void); /* pci_shutdown() ? */ int pci_bus_scan(void); int pci_testdev(void);
Thoughts?
Quoted 34 lines
lib/pci.h line 2
Quoted 10 lines
lib/pci-host-generic.h new file +2
-0+ +#endif lib/pci.h new file +2
-0@@ -0,0 +1,13 @@ +#ifndef PCI_H +#define PCI_H This file needs its copyright/gpl header. I also like to document the lib here in the header file, e.g. see
lib/devicetree.handlib/alloc.hlib/pci.h line 7
Quoted 5 lines
lib/pci.h new file +5
-0+ +#include "alloc.h" + +struct pci { +void *sysdata;The choice of sysdata for the name appears to come from the Linux kernel. Why not also call 'struct pci' 'struct
pci_dev' for full consistency?Quoted 9 lines
lib/pci.h new file +6
-0+}; + +extern struct pci *pci_dt_probe (void ); +extern void pci_shutdown (struct pci *pci); + +#endif Signature · 1 line
1.8.3.1
Re: [kvm-unit-tests PATCH 01/11] arm/pci: Device tree PCI probing
Quoted 49 lines · Alexander Gordeev
Quoted 21 lines · Andrew Jones
Quoted 8 lines
lib/pci-host-generic.h new file +2
-0@@ -0,0 +1,26 @@ +#ifndef PCI_HOST_GENERIC_H +#define PCI_HOST_GENERIC_H lib/pci-host-generic.h line 12
Quoted 10 lines
lib/pci-host-generic.h new file +10
-0+ +struct pci_addr_range { +phys_addr_t start; +phys_addr_t size; +}; + +struct pci_addr_space { +struct pci_addr_range cpu_range; +struct pci_addr_range pci_range; +phys_addr_t alloc;alloc isn't used in this patch, but guess it will be later
Quoted 15 lines
lib/pci-host-generic.h line 24
Quoted 12 lines
lib/pci-host-generic.h new file +12
-0+u32 of_flags; +}; + +struct pci_host_bridge { +struct pci_addr_range cpu_range; +int bus; +int bus_max; +int nr_addr_spaces; +struct pci_addr_space addr_space[]; +}; + +#define PCI_ECAM_BUS_SIZE (1 << 20)Why not put this struct declaration (and supporting structs) in
pci.h? Linux has structpci_host_bridgeininclude/linux/pci.hI think we do not need any of these structures to get public.
My initial attempt was also an assessment of an idea to generalize PCI code across architectures. But my current judgement - an outcome is not worth it. With x86 PCI access so much different and only bus 0 function 0 "hierarchy" we only need the simplest required routines.
Hence, I would leave these in
pci-host-generic.h(just not to blowpci-host-generic.ctoo big).
Quoted 8 lines
As result, just these routines would end up in
pci.h:bool pci_probe(void); void pci_free(void); /* pci_shutdown() ? */ int pci_bus_scan(void); int pci_testdev(void);Thoughts?
Whichever you prefer. We either "bloat" pci.h or we "bloat" the lib directory with another file :-) Actually, I still prefer pci.h, because if arm unit tests will always need both of them, then they'll always need two #include lines... OTOH, if you don't think unit tests will need these structures, then they could just be defined right in pci-host-generic.c (or pci.c if you consolidate the source files).
Thanks,
drew
Quoted 35 lines
Quoted 35 lines
lib/pci.h line 2
Quoted 11 lines
lib/pci-host-generic.h new file +2
-0+ +#endif lib/pci.h new file +2
-0@@ -0,0 +1,13 @@ +#ifndef PCI_H +#define PCI_H This file needs its copyright/gpl header. I also like to document the lib here in the header file, e.g. see
lib/devicetree.handlib/alloc.hlib/pci.h line 7
Quoted 5 lines
lib/pci.h new file +5
-0+ +#include "alloc.h" + +struct pci { +void *sysdata;The choice of sysdata for the name appears to come from the Linux kernel. Why not also call 'struct pci' 'struct
pci_dev' for full consistency?Quoted 9 lines
lib/pci.h new file +6
-0+}; + +extern struct pci *pci_dt_probe (void ); +extern void pci_shutdown (struct pci *pci); + +#endif Signature · 1 line
1.8.3.1
PATCH 2/11 arm/pci: PCI bus scanning
In reply to Alexander GordeevScan bus 0 and function 0 only for now
--- arm/pci-test.c | 4 +++ lib/pci-host-generic.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci-host-generic.h | 1 + lib/pci.h | 1 + 4 files changed, 73 insertions(+)
arm/pci-test.c +4 -0
@@ -11,6 +11,7 @@int main (void ) {struct pci *pci; +int ret; pci =pci_dt_probe ();
@@ -18,6 +19,9 @@ int main(void)if (!pci) goto done; + ret =pci_bus_scan (pci); +report ("PCI bus scanning detected %d devices" , true, ret); +pci_shutdown (pci); done:
lib/pci-host-generic.c +67 -0
85 lines not on this page · raw message
lib/pci-host-generic.h +1 -0
@@ -22,5 +22,6 @@ struct pci_host_bridge { };#define PCI_ECAM_BUS_SIZE (1 << 20) +#define PCI_ECAM_CONFIG_SIZE (1 << 12)#endif
lib/pci.h +1 -0
@@ -8,6 +8,7 @@ struct pci { };extern struct pci *pci_dt_probe (void ); +extern int pci_bus_scan (struct pci *pci);extern void pci_shutdown (struct pci *pci);#endif
Signature · 1 line
1.8.3.1
Re: [kvm-unit-tests PATCH 02/11] arm/pci: PCI bus scanning
Quoted 24 lines · Alexander Gordeev
Scan bus 0 and function 0 only for now
Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- arm/pci-test.c | 4 +++ lib/pci-host-generic.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci-host-generic.h | 1 + lib/pci.h | 1 + 4 files changed, 73 insertions(+)arm/pci-test.c +1
-0@@ -11,6 +11,7 @@int main (void ) {struct pci *pci; +int ret; pci =pci_dt_probe ();
arm/pci-test.c int main(void) · line 23
Quoted 5 lines
arm/pci-test.c +2
-0@@ -18,6 +19,9 @@ int main(void)if (!pci) goto done; + ret =pci_bus_scan (pci); +report ("PCI bus scanning detected %d devices" , true, ret);
How many devices are expected in this test? You can pass the expected number in on the command line, and then make this a "real" test report with
report(..., ret == atol(argv[0]), ret)
Quoted 9 lines
arm/pci-test.c +1
-0+pci_shutdown (pci); done:lib/pci-host-generic.c +0
-0
lib/pci-host-generic.c line 20
Quoted 7 lines
lib/pci-host-generic.c +4
-0@@ -14,6 +14,49 @@#include "pci-host-generic.h" #include <linux/pci_regs.h> +#define for_each_pci_dev (pci, dev, conf) \ + for (dev = find_next_dev(pci, -1, &conf); \ + dev >= 0; \ + dev = find_next_dev(pci, dev, &conf))
Why do we need to pass both a dev index and a conf pointer? I see below you can get the later from the former easily enough.
Quoted 5 lines
lib/pci-host-generic.c +5
-0+ +static u8 pci_config_readb (const void *conf,int off) +{ +return readb (conf + off); +}
lib/pci-host-generic.c line 35
Quoted 10 lines
lib/pci-host-generic.c +10
-0+ +static u32 pci_config_readl (const void *conf,int off) +{ +return readl (conf + off); +} + +static void pci_config_writel (u32 val,void *conf,int off) +{ +writel (val, conf + off); +}
These read/write wrappers don't look all that useful to me. They would be more useful if they took a 'struct pci[_dev] like Linux's pci_read_config_byte/word/dword do. In fact, stealing those from Linux makes sense.
lib/pci-host-generic.c line 47
Quoted 12 lines
lib/pci-host-generic.c +12
-0+ +static void *dev_conf (struct pci *pci,int dev) +{ +struct pci_host_bridge *host = pci->sysdata; +return (void *)host->cpu_range.start + (dev *8 ) *PCI_ECAM_CONFIG_SIZE ; +} + +/* We scan bus 0 only for now */ +static int find_next_dev (struct pci *pci,int dev,void **pconf) +{ +struct pci_host_bridge *host = pci->sysdata; +int limit =MIN (32u , host->cpu_range.size /PCI_ECAM_CONFIG_SIZE );
32 is the pci max devices per bus limit, right? Assuming a sane DT, is it possible to have host->cpu_range.size/PCI_ECAM_CONFIG_SIZE > 32?
Quoted 21 lines
lib/pci-host-generic.c +14
-0+ +for (dev++; dev < limit; dev++) { +void *conf =dev_conf (pci, dev); +if (pci_config_readl (conf,PCI_VENDOR_ID ) != ((u32 )~0 )) { + *pconf = conf; +return dev; + } + } + +return -1 ; +} +static void pci_host_addr_space_init (struct pci_addr_space as[],int nr_as,u32 cells[],int nr_range_cells) {@@ -133,6 +176,24 @@ static struct pci_host_bridge *pci_host_bridge_probe(void)return host; } +int pci_bus_scan (struct pci *pci) +{
lib/pci-host-generic.c static struct pci_host_bridge *pci_host_bridge_probe(void) · line 190
Quoted 10 lines
lib/pci-host-generic.c +10
-0+void *conf; +int dev; +int nr_dev =0 ; + +for_each_pci_dev (pci, dev, conf) { +/* We are only interested in normal PCI devices */ +if (pci_config_readb (conf,PCI_HEADER_TYPE ) != +PCI_HEADER_TYPE_NORMAL ) +continue ; +pci_config_writel (PCI_COMMAND_SERR , conf,PCI_COMMAND );
Shouldn't the driver (unit test) be the one that writes SERR? Also, doesn't this clear everything in the Command register except SERR? If a scan gets run again after driver/unittest has started using a device then this write may mess things up.
Quoted 10 lines
lib/pci-host-generic.c +6
-0+ nr_dev++; + } + +return nr_dev; +} +struct pci *pci_dt_probe (void ) {struct pci_host_bridge *host;
lib/pci-host-generic.c struct pci *pci_dt_probe(void) · line 218
Quoted 8 lines
lib/pci-host-generic.c +5
-0@@ -150,6 +211,12 @@ struct pci *pci_dt_probe(void)void pci_shutdown (struct pci *pci) { +void *conf; +int dev; + +for_each_pci_dev (pci, dev, conf) +pci_config_writel (PCI_COMMAND_INTX_DISABLE , conf,PCI_COMMAND );
I think I know why this is here, but I don't think that kvm-unit-tests needs to bother with it.
lib/pci-host-generic.h struct pci_host_bridge { · line 25
Quoted 13 lines
lib/pci-host-generic.c +1
-0+free (pci->sysdata);free (pci); }lib/pci-host-generic.h +1
-0@@ -22,5 +22,6 @@ struct pci_host_bridge { };#define PCI_ECAM_BUS_SIZE (1 << 20) +#define PCI_ECAM_CONFIG_SIZE (1 << 12)
Are these also defined in the Linux kernel? I'm too lazy to pull up the PCI spec right now, so I'd rather confirm the sizes by looking them up in Linux. If they are defined there somewhere, then please use the same names here. Otherwise, I guess references to some documentation or something would be nice for pci dummies like me.
Quoted 17 lines
lib/pci-host-generic.h +0
-0#endif lib/pci.h +1
-0@@ -8,6 +8,7 @@ struct pci { };extern struct pci *pci_dt_probe (void ); +extern int pci_bus_scan (struct pci *pci);extern void pci_shutdown (struct pci *pci);#endif Signature · 1 line
1.8.3.1
Re: [kvm-unit-tests PATCH 02/11] arm/pci: PCI bus scanning
Quoted 28 lines · Andrew Jones
Quoted 27 lines · Alexander Gordeev
Scan bus 0 and function 0 only for now
Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- arm/pci-test.c | 4 +++ lib/pci-host-generic.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci-host-generic.h | 1 + lib/pci.h | 1 + 4 files changed, 73 insertions(+)arm/pci-test.c +1
-0@@ -11,6 +11,7 @@int main (void ) {struct pci *pci; +int ret; pci =pci_dt_probe ();@@ -18,6 +19,9 @@ int main(void)if (!pci) goto done;
Quoted 8 lines
arm/pci-test.c int main(void) · line 23
Quoted 2 lines
arm/pci-test.c +2
-0+ ret =pci_bus_scan (pci); +report ("PCI bus scanning detected %d devices" , true, ret);How many devices are expected in this test? You can pass the expected number in on the command line, and then make this a "real" test report with
report(..., ret == atol(argv[0]), ret)
What if a machine's PCI hierarchy changed? Such test would break then.
Also, this report is not entirely dummy. It ensures PCI configuration space access operates, devices do respond and generally the bus is fine.
Quoted 12 lines
Quoted 12 lines
arm/pci-test.c +1
-0+pci_shutdown (pci); done:lib/pci-host-generic.c +0
-0@@ -14,6 +14,49 @@#include "pci-host-generic.h" #include <linux/pci_regs.h>
Quoted 8 lines
lib/pci-host-generic.c line 20
Quoted 4 lines
lib/pci-host-generic.c +4
-0+#define for_each_pci_dev (pci, dev, conf) \ + for (dev = find_next_dev(pci, -1, &conf); \ + dev >= 0; \ + dev = find_next_dev(pci, dev, &conf))Why do we need to pass both a dev index and a conf pointer? I see below you can get the later from the former easily enough.
No particular reason apart from the fact each for_each_pci_dev() would be followed by dev_conf() call - so I paired it to save few lines. I have no preference here, though.
Quoted 12 lines
Quoted 12 lines
lib/pci-host-generic.c +12
-0+ +static u8 pci_config_readb (const void *conf,int off) +{ +return readb (conf + off); +} + +static u32 pci_config_readl (const void *conf,int off) +{ +return readl (conf + off); +} + +static void pci_config_writel (u32 val,void *conf,int off)
Quoted 8 lines
lib/pci-host-generic.c line 35
Quoted 3 lines
lib/pci-host-generic.c +3
-0+{ +writel (val, conf + off); +}These read/write wrappers don't look all that useful to me. They would be more useful if they took a 'struct pci[_dev] like Linux's pci_read_config_byte/word/dword do. In fact, stealing those from Linux makes sense.
I put those ones only to emphasize PCI reads/writes as opposed to direct read*/write* accesses. Matching the Linux names is probably confusing.
Bringing Linux-like pci_read_config_byte/word/dword seems as an overkill to me as it entails pulling the whole data hierarchy pci_host_bridge->pci_bus->pci_dev.
By contrast, if we do not pull the whole hierarchy then we could introduce our own minimalistic data structure(s) and get off with it easily.
Quoted 7 lines
Quoted 7 lines
lib/pci-host-generic.c +7
-0+ +static void *dev_conf (struct pci *pci,int dev) +{ +struct pci_host_bridge *host = pci->sysdata; +return (void *)host->cpu_range.start + (dev *8 ) *PCI_ECAM_CONFIG_SIZE ; +} +
Quoted 8 lines
lib/pci-host-generic.c line 47
Quoted 5 lines
lib/pci-host-generic.c +5
-0+/* We scan bus 0 only for now */ +static int find_next_dev (struct pci *pci,int dev,void **pconf) +{ +struct pci_host_bridge *host = pci->sysdata; +int limit =MIN (32u , host->cpu_range.size /PCI_ECAM_CONFIG_SIZE );32 is the pci max devices per bus limit, right? Assuming a sane DT, is it possible to have host->cpu_range.size/PCI_ECAM_CONFIG_SIZE > 32?
The point here is not host->cpu_range.size/PCI_ECAM_CONFIG_SIZE > 32 but rather host->cpu_range.size/PCI_ECAM_CONFIG_SIZE < 32.
But you are right - a bus with less than 32 device slots is likely insane.
Quoted 19 lines
Quoted 19 lines
lib/pci-host-generic.c +12
-0+ +for (dev++; dev < limit; dev++) { +void *conf =dev_conf (pci, dev); +if (pci_config_readl (conf,PCI_VENDOR_ID ) != ((u32 )~0 )) { + *pconf = conf; +return dev; + } + } + +return -1 ; +} +static void pci_host_addr_space_init (struct pci_addr_space as[],int nr_as,u32 cells[],int nr_range_cells) {@@ -133,6 +176,24 @@ static struct pci_host_bridge *pci_host_bridge_probe(void)return host; }
Quoted 15 lines
lib/pci-host-generic.c static struct pci_host_bridge *pci_host_bridge_probe(void) · line 190
Quoted 12 lines
lib/pci-host-generic.c +12
-0+int pci_bus_scan (struct pci *pci) +{ +void *conf; +int dev; +int nr_dev =0 ; + +for_each_pci_dev (pci, dev, conf) { +/* We are only interested in normal PCI devices */ +if (pci_config_readb (conf,PCI_HEADER_TYPE ) != +PCI_HEADER_TYPE_NORMAL ) +continue ; +pci_config_writel (PCI_COMMAND_SERR , conf,PCI_COMMAND );Shouldn't the driver (unit test) be the one that writes SERR? Also, doesn't this clear everything in the Command register except SERR?
I do not thing a driver is in charge. It is a minimal devices initialization that is expected from firmware/BIOS AFAICT. A driver should inherit the properly initialized bus with allocated io/memory buffers to use.
Quoted 2 lines
If a scan gets run again after driver/unittest has started using a device then this write may mess things up.
PCI bus scan is normally done once to initialize hierarchy. I am struggling to imagine a usecase when it is called repeatedly, especially in such a small framework.
Quoted 11 lines
Quoted 11 lines
lib/pci-host-generic.c +6
-0+ nr_dev++; + } + +return nr_dev; +} +struct pci *pci_dt_probe (void ) {struct pci_host_bridge *host;@@ -150,6 +211,12 @@ struct pci *pci_dt_probe(void)
Quoted 10 lines
lib/pci-host-generic.c struct pci *pci_dt_probe(void) · line 218
Quoted 7 lines
lib/pci-host-generic.c +5
-0void pci_shutdown (struct pci *pci) { +void *conf; +int dev; + +for_each_pci_dev (pci, dev, conf) +pci_config_writel (PCI_COMMAND_INTX_DISABLE , conf,PCI_COMMAND );I think I know why this is here, but I don't think that kvm-unit-tests needs to bother with it.
Well, it is not about interrupts, rather about disabling devices access to access io/memory buffers, which is writing 0 to the command regiester. PCI_COMMAND_INTX_DISABLE is just a good one to write as well.
BTW, this is the reason I named this routine pci_shutdown(), not pci_free().
Quoted 37 lines
lib/pci-host-generic.h struct pci_host_bridge { · line 25
Quoted 13 lines
lib/pci-host-generic.c +1
-0+free (pci->sysdata);free (pci); }lib/pci-host-generic.h +1
-0@@ -22,5 +22,6 @@ struct pci_host_bridge { };#define PCI_ECAM_BUS_SIZE (1 << 20) +#define PCI_ECAM_CONFIG_SIZE (1 << 12)Are these also defined in the Linux kernel? I'm too lazy to pull up the PCI spec right now, so I'd rather confirm the sizes by looking them up in Linux. If they are defined there somewhere, then please use the same names here. Otherwise, I guess references to some documentation or something would be nice for pci dummies like me.
Quoted 17 lines
lib/pci-host-generic.h +0
-0#endif lib/pci.h +1
-0@@ -8,6 +8,7 @@ struct pci { };extern struct pci *pci_dt_probe (void ); +extern int pci_bus_scan (struct pci *pci);extern void pci_shutdown (struct pci *pci);#endif Signature · 1 line
1.8.3.1
Re: [kvm-unit-tests PATCH 02/11] arm/pci: PCI bus scanning
Quoted 36 lines · Alexander Gordeev
Quoted 35 lines · Andrew Jones
Quoted 24 lines · Alexander Gordeev
Scan bus 0 and function 0 only for now
Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- arm/pci-test.c | 4 +++ lib/pci-host-generic.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci-host-generic.h | 1 + lib/pci.h | 1 + 4 files changed, 73 insertions(+)arm/pci-test.c +1
-0@@ -11,6 +11,7 @@int main (void ) {struct pci *pci; +int ret; pci =pci_dt_probe ();arm/pci-test.c int main(void) · line 23
Quoted 5 lines
arm/pci-test.c +2
-0@@ -18,6 +19,9 @@ int main(void)if (!pci) goto done; + ret =pci_bus_scan (pci); +report ("PCI bus scanning detected %d devices" , true, ret);How many devices are expected in this test? You can pass the expected number in on the command line, and then make this a "real" test report with
Quoted 3 lines
Quoted 1 line
report(..., ret == atol(argv[0]), ret)What if a machine's PCI hierarchy changed? Such test would break then.
How would the number *of* detected devices change if we never pass more or less devices on the qemu command line? I realize the PCI addresses may change.
Quoted 25 lines
Also, this report is not entirely dummy. It ensures PCI configuration space access operates, devices do respond and generally the bus is fine.
Quoted 12 lines
Quoted 12 lines
arm/pci-test.c +1
-0+pci_shutdown (pci); done:lib/pci-host-generic.c +0
-0@@ -14,6 +14,49 @@#include "pci-host-generic.h" #include <linux/pci_regs.h> Quoted 8 lines
lib/pci-host-generic.c line 20
Quoted 4 lines
lib/pci-host-generic.c +4
-0+#define for_each_pci_dev (pci, dev, conf) \ + for (dev = find_next_dev(pci, -1, &conf); \ + dev >= 0; \ + dev = find_next_dev(pci, dev, &conf))Why do we need to pass both a dev index and a conf pointer? I see below you can get the later from the former easily enough.
Quoted 3 lines
No particular reason apart from the fact each
for_each_pci_dev()would be followed bydev_conf()call - so I paired it to save few lines. I have no preference here, though.
Then please drop one or the other.
Quoted 26 lines
Quoted 13 lines
Quoted 13 lines
lib/pci-host-generic.c +12
-0+ +static u8 pci_config_readb (const void *conf,int off) +{ +return readb (conf + off); +} + +static u32 pci_config_readl (const void *conf,int off) +{ +return readl (conf + off); +} + +static void pci_config_writel (u32 val,void *conf,int off)Quoted 8 lines
lib/pci-host-generic.c line 35
Quoted 3 lines
lib/pci-host-generic.c +3
-0+{ +writel (val, conf + off); +}These read/write wrappers don't look all that useful to me. They would be more useful if they took a 'struct pci[_dev] like Linux's pci_read_config_byte/word/dword do. In fact, stealing those from Linux makes sense.
I put those ones only to emphasize PCI reads/writes as opposed to direct read*/write* accesses. Matching the Linux names is probably confusing.
Quoted 7 lines
Bringing Linux-like pci_read_config_byte/word/dword seems as an overkill to me as it entails pulling the whole data hierarchy
pci_host_bridge->pci_bus->pci_dev.By contrast, if we do not pull the whole hierarchy then we could introduce our own minimalistic data structure(s) and get off with it easily.
Yes, that's what I was thinking. We already do have our own minimalistic data structure 'struct pci[_dev]'. However, now that I've made x86 pci code common you'll either just need to create a lib/arm/asm/pci.h with a pci_config_read like in lib/x86/asm/pci.h, or perhaps expand on that if you need to read different sizes, by adding the _byte/word/... variants.
Quoted 58 lines
Quoted 16 lines
lib/pci-host-generic.c line 47
Quoted 13 lines
lib/pci-host-generic.c +12
-0+ +static void *dev_conf (struct pci *pci,int dev) +{ +struct pci_host_bridge *host = pci->sysdata; +return (void *)host->cpu_range.start + (dev *8 ) *PCI_ECAM_CONFIG_SIZE ; +} + +/* We scan bus 0 only for now */ +static int find_next_dev (struct pci *pci,int dev,void **pconf) +{ +struct pci_host_bridge *host = pci->sysdata; +int limit =MIN (32u , host->cpu_range.size /PCI_ECAM_CONFIG_SIZE );32 is the pci max devices per bus limit, right? Assuming a sane DT, is it possible to have host->cpu_range.size/PCI_ECAM_CONFIG_SIZE > 32?
The point here is not host->cpu_range.size/PCI_ECAM_CONFIG_SIZE > 32 but rather host->cpu_range.size/PCI_ECAM_CONFIG_SIZE < 32.
But you are right - a bus with less than 32 device slots is likely insane.
Quoted 19 lines
Quoted 19 lines
lib/pci-host-generic.c +12
-0+ +for (dev++; dev < limit; dev++) { +void *conf =dev_conf (pci, dev); +if (pci_config_readl (conf,PCI_VENDOR_ID ) != ((u32 )~0 )) { + *pconf = conf; +return dev; + } + } + +return -1 ; +} +static void pci_host_addr_space_init (struct pci_addr_space as[],int nr_as,u32 cells[],int nr_range_cells) {@@ -133,6 +176,24 @@ static struct pci_host_bridge *pci_host_bridge_probe(void)return host; }Quoted 15 lines
lib/pci-host-generic.c static struct pci_host_bridge *pci_host_bridge_probe(void) · line 190
Quoted 12 lines
lib/pci-host-generic.c +12
-0+int pci_bus_scan (struct pci *pci) +{ +void *conf; +int dev; +int nr_dev =0 ; + +for_each_pci_dev (pci, dev, conf) { +/* We are only interested in normal PCI devices */ +if (pci_config_readb (conf,PCI_HEADER_TYPE ) != +PCI_HEADER_TYPE_NORMAL ) +continue ; +pci_config_writel (PCI_COMMAND_SERR , conf,PCI_COMMAND );Shouldn't the driver (unit test) be the one that writes SERR? Also, doesn't this clear everything in the Command register except SERR?
Quoted 4 lines
I do not thing a driver is in charge. It is a minimal devices initialization that is expected from
firmware/BIOSAFAICT. A driver should inherit the properly initialized bus with allocated io/memory buffers to use.
OK, does QEMU not provide that already?
Quoted 7 lines
Quoted 3 lines
If a scan gets run again after driver/unittest has started using a device then this write may mess things up.
PCI bus scan is normally done once to initialize hierarchy. I am struggling to imagine a usecase when it is called repeatedly, especially in such a small framework.
A unit test can do whatever it likes. I'd like the initial setup to be useful, but minimal, and easily modifiable.
Quoted 23 lines
Quoted 12 lines
Quoted 12 lines
lib/pci-host-generic.c +6
-0+ nr_dev++; + } + +return nr_dev; +} +struct pci *pci_dt_probe (void ) {struct pci_host_bridge *host;@@ -150,6 +211,12 @@ struct pci *pci_dt_probe(void)Quoted 10 lines
lib/pci-host-generic.c struct pci *pci_dt_probe(void) · line 218
Quoted 7 lines
lib/pci-host-generic.c +5
-0void pci_shutdown (struct pci *pci) { +void *conf; +int dev; + +for_each_pci_dev (pci, dev, conf) +pci_config_writel (PCI_COMMAND_INTX_DISABLE , conf,PCI_COMMAND );I think I know why this is here, but I don't think that kvm-unit-tests needs to bother with it.
Quoted 7 lines
Well, it is not about interrupts, rather about disabling devices access to access io/memory buffers, which is writing 0 to the command regiester.
PCI_COMMAND_INTX_DISABLEis just a good one to write as well.BTW, this is the reason I named this routine
pci_shutdown(), notpci_free().
I'm currently thinking any sort of shutdown is overkill for kvm-unit-tests, but I'll accept anything you can make a good case for :-)
Quoted 38 lines
Quoted 38 lines
Quoted 10 lines
lib/pci-host-generic.c +1
-0+free (pci->sysdata);free (pci); }lib/pci-host-generic.h +0
-0lib/pci-host-generic.h struct pci_host_bridge { · line 25
Quoted 4 lines
lib/pci-host-generic.h +1
-0@@ -22,5 +22,6 @@ struct pci_host_bridge { };#define PCI_ECAM_BUS_SIZE (1 << 20) +#define PCI_ECAM_CONFIG_SIZE (1 << 12)Are these also defined in the Linux kernel? I'm too lazy to pull up the PCI spec right now, so I'd rather confirm the sizes by looking them up in Linux. If they are defined there somewhere, then please use the same names here. Otherwise, I guess references to some documentation or something would be nice for pci dummies like me.
Quoted 17 lines
lib/pci-host-generic.h +0
-0#endif lib/pci.h +1
-0@@ -8,6 +8,7 @@ struct pci { };extern struct pci *pci_dt_probe (void ); +extern int pci_bus_scan (struct pci *pci);extern void pci_shutdown (struct pci *pci);#endif Signature · 1 line
1.8.3.1
PATCH 3/11 arm/pci: Read devices BARs
In reply to Alexander Gordeev--- lib/pci-host-generic.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci-host-generic.h | 10 ++++++ 2 files changed, 100 insertions(+)
lib/pci-host-generic.c +90 -0
@@ -176,9 +176,91 @@ static struct pci_host_bridge *pci_host_bridge_probe(void)return host; } +static void pci_get_bar_addr_size (void *conf,int bar,u32 *addr,u32 *size) +{ +int off =PCI_BASE_ADDRESS_0 + (bar *4 ); + + *addr =pci_config_readl (conf, off); +pci_config_writel (~0 , conf, off); + *size =pci_config_readl (conf, off); +pci_config_writel (*addr, conf, off); +} + +static bool pci_get_bar (void *conf,int bar,pci_res_type_t *type, +u64 *addr,u64 *size,bool *is64) +{ +u32 addr_low, size_low; +u64 mask; + +/* +* To determine the amount of address space needed by a PCI device, +* one must save the original value of the BAR, write a value of +* all 1's to the register, then read it back. The amount of memory +* can then be then determined by masking the information bits, +* performing a bitwise NOT and incrementing the value by 1. +* Use pci_get_bar_addr_size() helper to facilitate that algorithm. +*/ +pci_get_bar_addr_size (conf, bar, &addr_low, &size_low); + +if (addr_low &PCI_BASE_ADDRESS_SPACE_IO ) { + mask =PCI_BASE_ADDRESS_IO_MASK ; + *type =PCI_RES_TYPE_IO ; + *is64 = false; + }else { + mask =PCI_BASE_ADDRESS_MEM_MASK ; +if ((addr_low &PCI_BASE_ADDRESS_MEM_TYPE_MASK ) == +PCI_BASE_ADDRESS_MEM_TYPE_64 ) { +if (addr_low &PCI_BASE_ADDRESS_MEM_PREFETCH ) + *type =PCI_RES_TYPE_PREFMEM64 ; +else + *type =PCI_RES_TYPE_MEM64 ; + *is64 = true; + }else { +if (addr_low &PCI_BASE_ADDRESS_MEM_PREFETCH ) + *type =PCI_RES_TYPE_PREFMEM32 ; +else + *type =PCI_RES_TYPE_MEM32 ; + *is64 = false; + } + } + +if (*is64) { +u32 addr_high, size_high; +u64 size64; + +assert (bar <5 ); +pci_get_bar_addr_size (conf, bar +1 , &addr_high, &size_high); + + size64 = (~((((u64 )size_high <<32 ) | size_low) & mask)) +1 ; +if (!size64) +return false; + +if (size) + *size = size64; +if (addr) + *addr = (((u64 )addr_high <<32 ) | addr_low) & mask; + }else { +u32 size32; + + size32 = (~(size_low & (u32 )mask)) +1 ; +if (!size32) +return false; + +if (size) + *size = size32; +if (addr) + *addr = addr_low & mask; + } + +return true; +} +int pci_bus_scan (struct pci *pci) {void *conf; +pci_res_type_t type; +bool is64; +int bar;int dev;int nr_dev =0 ;
@@ -187,6 +269,14 @@ int pci_bus_scan(struct pci *pci)if (pci_config_readb (conf,PCI_HEADER_TYPE ) !=PCI_HEADER_TYPE_NORMAL )continue ; + +for (bar =0 ; bar <PCI_HEADER_TYPE_NORMAL_NUM_BARS ; bar++) { +if (!pci_get_bar (conf, bar, &type,NULL ,NULL , &is64)) +break ; +if (is64) + bar++; + } +pci_config_writel (PCI_COMMAND_SERR , conf,PCI_COMMAND ); nr_dev++; }
lib/pci-host-generic.h +10 -0
@@ -21,7 +21,17 @@ struct pci_host_bridge { struct pci_addr_space addr_space[]; }; +typedef enum pci_res_type { +PCI_RES_TYPE_CONF =0 , +PCI_RES_TYPE_IO =1 , +PCI_RES_TYPE_MEM32 =2 , +PCI_RES_TYPE_MEM64 =3 , +PCI_RES_TYPE_PREFMEM32 =6 , +PCI_RES_TYPE_PREFMEM64 =7 +}pci_res_type_t ; +#define PCI_ECAM_BUS_SIZE (1 << 20)#define PCI_ECAM_CONFIG_SIZE (1 << 12) +#define PCI_HEADER_TYPE_NORMAL_NUM_BARS 6/* # of BARs in PCI function */ #endif
Signature · 1 line
1.8.3.1
PATCH 4/11 arm/pci: Allocate and assign memory/io space resources
In reply to Alexander Gordeev--- lib/pci-host-generic.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 69 insertions(+), 2 deletions(-)
lib/pci-host-generic.c +69 -2
@@ -19,6 +19,11 @@ dev >=0 ; \ dev = find_next_dev(pci, dev, &conf)) +static pci_res_type_t flags_to_type (u32 of_flags) +{ +return ((of_flags &0x40000000 ) >>28 ) | ((of_flags >>24 ) &0x03 ); +} +static u8 pci_config_readb (const void *conf,int off) {return readb (conf + off);
@@ -255,11 +260,66 @@ static bool pci_get_bar(void *conf, int bar, pci_res_type_t *type,return true; } +static void pci_set_bar (void *conf,int bar,phys_addr_t addr,bool is64) +{ +int off =PCI_BASE_ADDRESS_0 + (bar *4 ); + +pci_config_writel (addr, conf, off); +if (is64) +pci_config_writel (addr >>32 , conf, off +4 ); +} + +static struct pci_addr_space *pci_find_res (struct pci_host_bridge *host, +pci_res_type_t type) +{ +struct pci_addr_space *as = &host->addr_space[0 ]; +int i; + +for (i =0 ; i < host->nr_addr_spaces; i++, as++) { +if (flags_to_type (as->of_flags) == type) +return as; + } + +return NULL ; +} + +static phys_addr_t pci_align_res_size (phys_addr_t size,pci_res_type_t type) +{ +phys_addr_t mask; + +if (type ==PCI_RES_TYPE_IO ) + mask =PCI_BASE_ADDRESS_IO_MASK ; +else + mask =PCI_BASE_ADDRESS_MEM_MASK ; + +return (size + ~mask) & mask; +} + +static phys_addr_t pci_alloc_res (struct pci_host_bridge *host, +pci_res_type_t type,u64 size) +{ +struct pci_addr_space *res; +phys_addr_t addr; + +assert (res =pci_find_res (host, type)); + + size =pci_align_res_size (size, type); +assert (res->alloc + size <= res->pci_range.size); + + addr = res->pci_range.start + res->alloc; + res->alloc += size; + +return addr; +} +int pci_bus_scan (struct pci *pci) {void *conf; +phys_addr_t addr; +u64 size;pci_res_type_t type;bool is64; +u32 cmd =PCI_COMMAND_SERR ;int bar;int dev;int nr_dev =0 ;
@@ -271,13 +331,20 @@ int pci_bus_scan(struct pci *pci)continue ;for (bar =0 ; bar <PCI_HEADER_TYPE_NORMAL_NUM_BARS ; bar++) { -if (!pci_get_bar (conf, bar, &type,NULL ,NULL , &is64)) +if (!pci_get_bar (conf, bar, &type,NULL , &size, &is64))break ; + addr =pci_alloc_res (pci->sysdata, type, size); +pci_set_bar (conf, bar, addr, is64);if (is64) bar++; + +if (type ==PCI_RES_TYPE_IO ) + cmd |=PCI_COMMAND_IO ; +else + cmd |=PCI_COMMAND_MEMORY ; } -pci_config_writel (PCI_COMMAND_SERR , conf,PCI_COMMAND ); +pci_config_writel (cmd, conf,PCI_COMMAND ); nr_dev++; }
Signature · 1 line
1.8.3.1
PATCH 5/11 arm/pci: Add pci_find_dev() and pci_bar_addr() functions
In reply to Alexander GordeevThis updat is a reminiscence of x86 implementation - to provision a possible future common PCI interface.
Make pcidevaddr_t as int, not u16. There is no good reason to limit it to u16 while omitting properly adjusted bit fields.
--- lib/pci-host-generic.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci.h | 18 +++++++++++++++++ 2 files changed, 72 insertions(+)
lib/pci-host-generic.c +54 -0
@@ -29,6 +29,11 @@ static u8 pci_config_readb(const void *conf, int off)return readb (conf + off); } +static u16 pci_config_readw (const void *conf,int off) +{ +return readw (conf + off); +} +static u32 pci_config_readl (const void *conf,int off) {return readl (conf + off);
@@ -351,6 +356,55 @@ int pci_bus_scan(struct pci *pci)return nr_dev; } +static pcidevaddr_t encode_addr (int bus,int dev,int fn) +{ +assert (bus <256 && dev <32 && fn <8 ); +return bus <<16 | dev <<11 | fn; +} + +static void decode_addr (pcidevaddr_t bdf,int *bus,int *dev,int *fn) +{ + *bus = (bdf >>16 ) &0xff ; + *dev = (bdf >>11 ) &0x1f ; + *fn = (bdf >>8 ) &0x03 ; +} + +pcidevaddr_t pci_find_dev (struct pci *pci,u16 vendor_id,u16 device_id) +{ +void *conf; +int dev; + +for_each_pci_dev (pci, dev, conf) { +if (vendor_id ==pci_config_readw (conf,PCI_VENDOR_ID ) && + device_id ==pci_config_readw (conf,PCI_DEVICE_ID )) +return encode_addr (0 , dev,0 ); + } + +return PCIDEVADDR_INVALID ; +} + +phys_addr_t pci_bar_addr (struct pci *pci,pcidevaddr_t bdf,int bar) +{ +void *conf; +struct pci_addr_space *res; +pci_res_type_t type; +phys_addr_t addr; +bool is64; +int bus, dev, fn; + +decode_addr (bdf, &bus, &dev, &fn); +assert (!bus && !fn);/* We support bus 0 and function 0 only */ + + conf =dev_conf (pci, dev); +assert (pci_config_readb (conf,PCI_HEADER_TYPE ) == +PCI_HEADER_TYPE_NORMAL ); + +assert (pci_get_bar (conf, bar, &type, &addr,NULL , &is64)); +assert (res =pci_find_res (pci->sysdata, type)); + +return res->cpu_range.start + (addr - res->pci_range.start); +} +struct pci *pci_dt_probe (void ) {struct pci_host_bridge *host;
lib/pci.h +18 -0
@@ -7,8 +7,26 @@ struct pci { void *sysdata; }; +/* +* Make it a reminiscence of x86 implementation - to provision a possible +* future merge. +* +* Unlike x86, PCIDEVADDR_INVALID is -1, not 0. PCI bus-device-function +* of 00.00:0 is a legitimate PCI address, so x86 needs a fix. +* +* Also, make pcidevaddr_t int, not u16. There is no good reason to limit +* it to u16 while omitting properly adjusted bit fields. +* +*/ +typedef enum { +PCIDEVADDR_INVALID =-1 +}pcidevaddr_t ; +extern struct pci *pci_dt_probe (void );extern int pci_bus_scan (struct pci *pci);extern void pci_shutdown (struct pci *pci); +extern pcidevaddr_t pci_find_dev (struct pci *pci,u16 vendor_id,u16 device_id); +extern phys_addr_t pci_bar_addr (struct pci *pci,pcidevaddr_t bdf,int bar_nr); +#endif
Signature · 1 line
1.8.3.1
Re: [kvm-unit-tests PATCH 05/11] arm/pci: Add pci_find_dev() and pci_bar_addr() functions
Quoted 2 lines · Alexander Gordeev
This updat is a reminiscence of x86 implementation - to provision a possible future common PCI interface.
This comment inspired me to refactor the x86 code into common code. I'll post that series in a few minutes for you to review. If x86 people are OK with it, then I think we should rebase your series on it. I'm particular interested in learning if the last two patches of this series can be greatly simplified by pretty much just implementing the DT pcie-host-bridge discovery and a lib/arm/asm/pci.h:pci_config_read() function.
Quoted 3 lines
Make
pcidevaddr_tas int, not u16. There is no good reason to limit it to u16 while omitting properly adjusted bit fields.
pcidevaddr_t is OK as a u16, afaict it's just a device index for a single bus (bus=0). So it has a max of 256. You could redefine it to include the bus number too, but I'm not sure it's necessary for simple kvm-unit-tests.
Quoted 85 lines
Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci-host-generic.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci.h | 18 +++++++++++++++++ 2 files changed, 72 insertions(+)lib/pci-host-generic.c +54
-0@@ -29,6 +29,11 @@ static u8 pci_config_readb(const void *conf, int off)return readb (conf + off); } +static u16 pci_config_readw (const void *conf,int off) +{ +return readw (conf + off); +} +static u32 pci_config_readl (const void *conf,int off) {return readl (conf + off);@@ -351,6 +356,55 @@ int pci_bus_scan(struct pci *pci)return nr_dev; } +static pcidevaddr_t encode_addr (int bus,int dev,int fn) +{ +assert (bus <256 && dev <32 && fn <8 ); +return bus <<16 | dev <<11 | fn; +} + +static void decode_addr (pcidevaddr_t bdf,int *bus,int *dev,int *fn) +{ + *bus = (bdf >>16 ) &0xff ; + *dev = (bdf >>11 ) &0x1f ; + *fn = (bdf >>8 ) &0x03 ; +} + +pcidevaddr_t pci_find_dev (struct pci *pci,u16 vendor_id,u16 device_id) +{ +void *conf; +int dev; + +for_each_pci_dev (pci, dev, conf) { +if (vendor_id ==pci_config_readw (conf,PCI_VENDOR_ID ) && + device_id ==pci_config_readw (conf,PCI_DEVICE_ID )) +return encode_addr (0 , dev,0 ); + } + +return PCIDEVADDR_INVALID ; +} + +phys_addr_t pci_bar_addr (struct pci *pci,pcidevaddr_t bdf,int bar) +{ +void *conf; +struct pci_addr_space *res; +pci_res_type_t type; +phys_addr_t addr; +bool is64; +int bus, dev, fn; + +decode_addr (bdf, &bus, &dev, &fn); +assert (!bus && !fn);/* We support bus 0 and function 0 only */ + + conf =dev_conf (pci, dev); +assert (pci_config_readb (conf,PCI_HEADER_TYPE ) == +PCI_HEADER_TYPE_NORMAL ); + +assert (pci_get_bar (conf, bar, &type, &addr,NULL , &is64)); +assert (res =pci_find_res (pci->sysdata, type)); + +return res->cpu_range.start + (addr - res->pci_range.start); +} +struct pci *pci_dt_probe (void ) {struct pci_host_bridge *host;lib/pci.h +0
-0
lib/pci.h struct pci { · line 15
Quoted 9 lines
lib/pci.h +6
-0@@ -7,8 +7,26 @@ struct pci {void *sysdata; }; +/* +* Make it a reminiscence of x86 implementation - to provision a possible +* future merge. +* +* Unlike x86, PCIDEVADDR_INVALID is -1, not 0. PCI bus-device-function +* of 00.00:0 is a legitimate PCI address, so x86 needs a fix.
x86 gets away with this because the pci-testdev is never at addr 00.0, that's where its host bridge is. I agree that it would be cleaner to use an invalid dev-addr (unsigned > 255) for "INVALID" though. How about 0xffff?
Quoted 20 lines
lib/pci.h +12
-0+* +* Also, make pcidevaddr_t int, not u16. There is no good reason to limit +* it to u16 while omitting properly adjusted bit fields. +* +*/ +typedef enum { +PCIDEVADDR_INVALID =-1 +}pcidevaddr_t ; +extern struct pci *pci_dt_probe (void );extern int pci_bus_scan (struct pci *pci);extern void pci_shutdown (struct pci *pci); +extern pcidevaddr_t pci_find_dev (struct pci *pci,u16 vendor_id,u16 device_id); +extern phys_addr_t pci_bar_addr (struct pci *pci,pcidevaddr_t bdf,int bar_nr); +#endif Signature · 1 line
1.8.3.1
PATCH 6/11 arm/pci: PCI testdev existence test
In reply to Alexander Gordeev--- arm/pci-test.c | 3 +++ config/config-arm-common.mak | 1 + lib/pci-testdev.c | 31 +++++++++++++++++++++++++++++++ lib/pci.h | 1 + 4 files changed, 36 insertions(+) create mode 100644 lib/pci-testdev.c
arm/pci-test.c +3 -0
@@ -22,6 +22,9 @@ int main(void) ret =pci_bus_scan (pci);report ("PCI bus scanning detected %d devices" , true, ret); + ret =pci_testdev (pci); +report ("PCI test device" , ret); +pci_shutdown (pci); done:
config/config-arm-common.mak +1 -0
@@ -31,6 +31,7 @@ include config/asm-offsets.mak cflatobjs += lib/alloc.o cflatobjs += lib/devicetree.o cflatobjs += lib/pci-host-generic.o +cflatobjs += lib/pci-testdev.o cflatobjs += lib/virtio.o cflatobjs += lib/virtio-mmio.o cflatobjs += lib/chr-testdev.o
lib/pci-testdev.c new file +31 -0
@@ -0,0 +1,31 @@ +/* +* QEMU "pci-testdev" PCI test device +* +* Copyright (C) 2015, Red Hat Inc, Alexander Gordeev <[email protected]> +* +* This work is licensed under the terms of the GNU LGPL, version 2. +*/ +#include "pci.h" +#include "asm/io.h" + +#define PCI_VENDOR_ID_REDHAT 0x1b36 +#define PCI_DEVICE_ID_REDHAT_TEST 0x0005 + +bool pci_testdev (struct pci *pci) +{ +phys_addr_t addr; +pcidevaddr_t dev =pci_find_dev (pci, +PCI_VENDOR_ID_REDHAT , +PCI_DEVICE_ID_REDHAT_TEST ); + +if (dev ==PCIDEVADDR_INVALID ) +return false; + + addr =pci_bar_addr (pci, dev,0 ); +assert (addr); + + addr =pci_bar_addr (pci, dev,1 ); +assert (addr); + +return true; +}
lib/pci.h +1 -0
@@ -24,6 +24,7 @@ typedef enum {extern struct pci *pci_dt_probe (void );extern int pci_bus_scan (struct pci *pci); +extern bool pci_testdev (struct pci *pci);extern void pci_shutdown (struct pci *pci);extern pcidevaddr_t pci_find_dev (struct pci *pci,u16 vendor_id,u16 device_id);
Signature · 1 line
1.8.3.1
PATCH 7/11 arm/pci: PCI device operation test
In reply to Alexander Gordeev--- arm/pci-test.c | 2 +- lib/pci-testdev.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- lib/pci.h | 2 +- 3 files changed, 138 insertions(+), 5 deletions(-)
arm/pci-test.c +1 -1
@@ -23,7 +23,7 @@ int main(void)report ("PCI bus scanning detected %d devices" , true, ret); ret =pci_testdev (pci); -report ("PCI test device" , ret); +report ("PCI test device passed %d tests" , (ret >=6 ), ret);pci_shutdown (pci);
lib/pci-testdev.c +136 -3
157 lines not on this page · raw message
lib/pci.h +1 -1
@@ -24,7 +24,7 @@ typedef enum {extern struct pci *pci_dt_probe (void );extern int pci_bus_scan (struct pci *pci); -extern bool pci_testdev (struct pci *pci); +extern int pci_testdev (struct pci *pci);extern void pci_shutdown (struct pci *pci);extern pcidevaddr_t pci_find_dev (struct pci *pci,u16 vendor_id,u16 device_id);
Signature · 1 line
1.8.3.1
Re: [kvm-unit-tests PATCH 07/11] arm/pci: PCI device operation test
Quoted 37 lines · Alexander Gordeev
Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- arm/pci-test.c | 2 +- lib/pci-testdev.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- lib/pci.h | 2 +- 3 files changed, 138 insertions(+), 5 deletions(-)arm/pci-test.c +1
-1@@ -23,7 +23,7 @@ int main(void)report ("PCI bus scanning detected %d devices" , true, ret); ret =pci_testdev (pci); -report ("PCI test device" , ret); +report ("PCI test device passed %d tests" , (ret >=6 ), ret);pci_shutdown (pci);lib/pci-testdev.c +7
-1@@ -11,21 +11,154 @@#define PCI_VENDOR_ID_REDHAT 0x1b36#define PCI_DEVICE_ID_REDHAT_TEST 0x0005 -bool pci_testdev (struct pci *pci) +struct pci_test_dev_hdr { +u8 test; +u8 width; +u8 pad0[2 ]; +u32 offset; +u32 data; +u32 count;
lib/pci-testdev.c line 30
Quoted 10 lines
lib/pci-testdev.c +10
-0+u8 name[]; +}; + +struct pci_testdev_ops { +u8 (*read8)(const volatile void *addr); +u16 (*read16)(const volatile void *addr); +u32 (*read32)(const volatile void *addr); +void (*write8)(u8 value,volatile void *addr); +void (*write16)(u16 value,volatile void *addr); +void (*write32)(u32 value,volatile void *addr);
Please use the standard 'b', 'w', 'l' in these names.
Quoted 152 lines
lib/pci-testdev.c +119
-2+}; + +static u8 ioread8 (const volatile void *addr) +{ +return readb (addr); +} + +static u16 ioread16 (const volatile void *addr) +{ +return readw (addr); +} + +static u32 ioread32 (const volatile void *addr) +{ +return readl (addr); +} + +static void iowrite8 (u8 value,volatile void *addr) +{ +writeb (value, addr); +} + +static void iowrite16 (u16 value,volatile void *addr) +{ +writew (value, addr); +} + +static void iowrite32 (u32 value,volatile void *addr) +{ +writel (value, addr); +} + +static struct pci_testdev_ops pci_testdev_io_ops = { + .read8 = ioread8, + .read16 = ioread16, + .read32 = ioread32, + .write8 = iowrite8, + .write16 = iowrite16, + .write32 = iowrite32 +}; + +static u8 memread8 (const volatile void *addr) +{ +return *(const volatile u8 __force *)addr; +} + +static u16 memread16 (const volatile void *addr) +{ +return *(const volatile u16 __force *)addr; +} + +static u32 memread32 (const volatile void *addr) +{ +return *(const volatile u32 __force *)addr; +} + +static void memwrite8 (u8 value,volatile void *addr) +{ + *(volatile u8 __force *)addr = value; +} + +static void memwrite16 (u16 value,volatile void *addr) +{ + *(volatile u16 __force *)addr = value; +} + +static void memwrite32 (u32 value,volatile void *addr) +{ + *(volatile u32 __force *)addr = value; +} + +static struct pci_testdev_ops pci_testdev_mem_ops = { + .read8 = memread8, + .read16 = memread16, + .read32 = memread32, + .write8 = memwrite8, + .write16 = memwrite16, + .write32 = memwrite32 +}; + +static bool pci_testdev_one (struct pci_test_dev_hdr *test, +int test_nr, +struct pci_testdev_ops *ops) +{ +u8 width; + + ops->write8 (test_nr, &test->test); +assert (ops->read32 (&test->count) ==0 ); + + width = ops->read8 (&test->width); +if ((width !=1 ) && (width !=2 ) && (width !=4 )) +return false; + +return true; +} + +static int pci_testdev_all (struct pci_test_dev_hdr *test, +struct pci_testdev_ops *ops) +{ +int i; + +for (i =0 ;; i++) { +if (!pci_testdev_one (test, i, ops)) +break ; + } + +return i; +} + +int pci_testdev (struct pci *pci) {phys_addr_t addr; +void __iomem *mem, *io; +int nr_tests =0 ;pcidevaddr_t dev =pci_find_dev (pci,PCI_VENDOR_ID_REDHAT ,PCI_DEVICE_ID_REDHAT_TEST );if (dev ==PCIDEVADDR_INVALID ) -return false; +return 0 ; addr =pci_bar_addr (pci, dev,0 );assert (addr); + mem =ioremap (addr,0 ); addr =pci_bar_addr (pci, dev,1 );assert (addr); + io = (void *)addr; -return true; + nr_tests +=pci_testdev_all (mem, &pci_testdev_mem_ops); + nr_tests +=pci_testdev_all (io, &pci_testdev_io_ops); + +return nr_tests; }lib/pci.h +1
-1@@ -24,7 +24,7 @@ typedef enum {extern struct pci *pci_dt_probe (void );extern int pci_bus_scan (struct pci *pci); -extern bool pci_testdev (struct pci *pci); +extern int pci_testdev (struct pci *pci);extern void pci_shutdown (struct pci *pci);extern pcidevaddr_t pci_find_dev (struct pci *pci,u16 vendor_id,u16 device_id);Signature · 1 line
1.8.3.1
This patch can be squashed into the last one.
Thanks,
drew
Re: [kvm-unit-tests PATCH 07/11] arm/pci: PCI device operation test
Quoted 9 lines · Andrew Jones
lib/pci-testdev.c line 30
Quoted 7 lines
lib/pci-testdev.c +7
-0+struct pci_testdev_ops { +u8 (*read8)(const volatile void *addr); +u16 (*read16)(const volatile void *addr); +u32 (*read32)(const volatile void *addr); +void (*write8)(u8 value,volatile void *addr); +void (*write16)(u16 value,volatile void *addr); +void (*write32)(u32 value,volatile void *addr);Please use the standard 'b', 'w', 'l' in these names.
I did not use in the first place as it conflicts with standard read*/write* macros. Would [read|write]_[byte|word|long] work for you?
Quoted 2 lines
Thanks,
drew
Re: [kvm-unit-tests PATCH 07/11] arm/pci: PCI device operation test
Quoted 11 lines · Alexander Gordeev
Quoted 9 lines · Andrew Jones
lib/pci-testdev.c line 30
Quoted 7 lines
lib/pci-testdev.c +7
-0+struct pci_testdev_ops { +u8 (*read8)(const volatile void *addr); +u16 (*read16)(const volatile void *addr); +u32 (*read32)(const volatile void *addr); +void (*write8)(u8 value,volatile void *addr); +void (*write16)(u16 value,volatile void *addr); +void (*write32)(u32 value,volatile void *addr);Please use the standard 'b', 'w', 'l' in these names.
Quoted 3 lines
I did not use in the first place as it conflicts with standard read*/write* macros. Would [read|write]_[byte|word|long] work for you?
Oh yeah... dang macros. _byte/word/dword like the pci_config_read/write function in Linux would work.
drew
PATCH 8/11 arm/pci: PCI device read/write test
In reply to Alexander Gordeev--- lib/pci-testdev.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
lib/pci-testdev.c +16 -0
@@ -113,6 +113,9 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test, struct pci_testdev_ops *ops) {u8 width; +u32 sig, off; +const int nr_writes =16 ; +int i; ops->write8 (test_nr, &test->test);assert (ops->read32 (&test->count) ==0 );
@@ -121,6 +124,19 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test,if ((width !=1 ) && (width !=2 ) && (width !=4 ))return false; + sig = ops->read32 (&test->data); + off = ops->read32 (&test->offset); + +for (i =0 ; i < nr_writes; i++) { +switch (width) { +case 1 : ops->write8 (sig, (void *)test + off);break ; +case 2 : ops->write16 (sig, (void *)test + off);break ; +case 4 : ops->write32 (sig, (void *)test + off);break ; + } + } + +assert ((int )ops->read32 (&test->count) == nr_writes); +return true; }
Signature · 1 line
1.8.3.1
Re: [kvm-unit-tests PATCH 08/11] arm/pci: PCI device read/write test
Quoted 26 lines · Alexander Gordeev
Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci-testdev.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)lib/pci-testdev.c +5
-0@@ -113,6 +113,9 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test, struct pci_testdev_ops *ops) {u8 width; +u32 sig, off; +const int nr_writes =16 ; +int i; ops->write8 (test_nr, &test->test);assert (ops->read32 (&test->count) ==0 );@@ -121,6 +124,19 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test,if ((width !=1 ) && (width !=2 ) && (width !=4 ))return false; + sig = ops->read32 (&test->data); + off = ops->read32 (&test->offset);
lib/pci-testdev.c static bool pci_testdev_one(struct pci_test_dev_hdr *test, · line 138
Quoted 10 lines
lib/pci-testdev.c +10
-0+ +for (i =0 ; i < nr_writes; i++) { +switch (width) { +case 1 : ops->write8 (sig, (void *)test + off);break ; +case 2 : ops->write16 (sig, (void *)test + off);break ; +case 4 : ops->write32 (sig, (void *)test + off);break ; + } + } + +assert ((int )ops->read32 (&test->count) == nr_writes);
Are you sure we want to assert here? What about outputting a message and then returning false instead?
Quoted 7 lines
lib/pci-testdev.c +1
-0+return true; }Signature · 1 line
1.8.3.1
Re: [kvm-unit-tests PATCH 08/11] arm/pci: PCI device read/write test
Quoted 25 lines · Andrew Jones
Quoted 24 lines · Alexander Gordeev
Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci-testdev.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)lib/pci-testdev.c +3
-0@@ -113,6 +113,9 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test, struct pci_testdev_ops *ops) {u8 width; +u32 sig, off; +const int nr_writes =16 ; +int i; ops->write8 (test_nr, &test->test);assert (ops->read32 (&test->count) ==0 );@@ -121,6 +124,19 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test,if ((width !=1 ) && (width !=2 ) && (width !=4 ))return false;
Quoted 15 lines
lib/pci-testdev.c static bool pci_testdev_one(struct pci_test_dev_hdr *test, · line 138
Quoted 12 lines
lib/pci-testdev.c +12
-0+ sig = ops->read32 (&test->data); + off = ops->read32 (&test->offset); + +for (i =0 ; i < nr_writes; i++) { +switch (width) { +case 1 : ops->write8 (sig, (void *)test + off);break ; +case 2 : ops->write16 (sig, (void *)test + off);break ; +case 4 : ops->write32 (sig, (void *)test + off);break ; + } + } + +assert ((int )ops->read32 (&test->count) == nr_writes);Are you sure we want to assert here? What about outputting a message and then returning false instead?
Nice catch.
Quoted 7 lines
Quoted 7 lines
lib/pci-testdev.c +1
-0+return true; }Signature · 1 line
1.8.3.1
Re: [kvm-unit-tests PATCH 08/11] arm/pci: PCI device read/write test
In reply to Alexander GordeevQuoted 29 lines · Alexander Gordeev
Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci-testdev.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)lib/pci-testdev.c +8
-0@@ -113,6 +113,9 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test, struct pci_testdev_ops *ops) {u8 width; +u32 sig, off; +const int nr_writes =16 ; +int i; ops->write8 (test_nr, &test->test);assert (ops->read32 (&test->count) ==0 );@@ -121,6 +124,19 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test,if ((width !=1 ) && (width !=2 ) && (width !=4 ))return false; + sig = ops->read32 (&test->data); + off = ops->read32 (&test->offset); + +for (i =0 ; i < nr_writes; i++) { +switch (width) {
lib/pci-testdev.c static bool pci_testdev_one(struct pci_test_dev_hdr *test, · line 141
Quoted 10 lines
lib/pci-testdev.c +8
-0+case 1 : ops->write8 (sig, (void *)test + off);break ; +case 2 : ops->write16 (sig, (void *)test + off);break ; +case 4 : ops->write32 (sig, (void *)test + off);break ; + } + } + +assert ((int )ops->read32 (&test->count) == nr_writes); +return true; }
Forgot to mention that this could also be squashed into the last two patches.
drew
PATCH 9/11 arm/pci: PCI host bridge info printing
In reply to Alexander Gordeev--- lib/pci-host-generic.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
lib/pci-host-generic.c +35 -0
@@ -19,6 +19,15 @@ dev >=0 ; \ dev = find_next_dev(pci, dev, &conf)) +static char *addr_space_desc[] = { + [PCI_RES_TYPE_CONF ] ="CONF" , + [PCI_RES_TYPE_IO ] ="IO" , + [PCI_RES_TYPE_MEM32 ] ="MEM32" , + [PCI_RES_TYPE_MEM64 ] ="MEM64" , + [PCI_RES_TYPE_PREFMEM32 ] ="MEM32/p" , + [PCI_RES_TYPE_PREFMEM64 ] ="MEM64/p" +}; +static pci_res_type_t flags_to_type (u32 of_flags) {return ((of_flags &0x40000000 ) >>28 ) | ((of_flags >>24 ) &0x03 );
@@ -105,6 +114,28 @@ static void pci_host_addr_space_init(struct pci_addr_space as[], int nr_as, } } +static void pci_host_bridge_print (struct pci_host_bridge *host) +{ +printf ("PCIe start %016llx size %016llx " +"bus %02x bus_max %02x #spaces %d\n\n" , + host->cpu_range.start, host->cpu_range.size, + host->bus, host->bus_max, host->nr_addr_spaces); +} + +static void pci_address_spaces_print (struct pci_addr_space as[],int nr_as) +{ +int i; + +for (i =0 ; i < nr_as; i++, as++) { +printf ("%s address space:\n" +"CPU: start %016llx size %016llx\n" +"PCI: start %016llx size %016llx\n\n" , + addr_space_desc[flags_to_type (as->of_flags)], + as->cpu_range.start, as->cpu_range.size, + as->pci_range.start, as->pci_range.size); + } +} +/* * Probe DT for a generic PCI host controller * See kernel Documentation/devicetree/bindings/pci/host-generic-pci.txt
@@ -180,9 +211,13 @@ static struct pci_host_bridge *pci_host_bridge_probe(void) host->bus_max = bus_max; host->nr_addr_spaces = nr_addr_spaces; +pci_host_bridge_print (host); +pci_host_addr_space_init (&host->addr_space[0 ], nr_addr_spaces, (u32 *)prop->data, nr_range_cells); +pci_address_spaces_print (&host->addr_space[0 ], host->nr_addr_spaces); +return host; }
Signature · 1 line
1.8.3.1
Re: [kvm-unit-tests PATCH 09/11] arm/pci: PCI host bridge info printing
Quoted 58 lines · Alexander Gordeev
Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci-host-generic.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)lib/pci-host-generic.c +31
-0@@ -19,6 +19,15 @@ dev >=0 ; \ dev = find_next_dev(pci, dev, &conf)) +static char *addr_space_desc[] = { + [PCI_RES_TYPE_CONF ] ="CONF" , + [PCI_RES_TYPE_IO ] ="IO" , + [PCI_RES_TYPE_MEM32 ] ="MEM32" , + [PCI_RES_TYPE_MEM64 ] ="MEM64" , + [PCI_RES_TYPE_PREFMEM32 ] ="MEM32/p" , + [PCI_RES_TYPE_PREFMEM64 ] ="MEM64/p" +}; +static pci_res_type_t flags_to_type (u32 of_flags) {return ((of_flags &0x40000000 ) >>28 ) | ((of_flags >>24 ) &0x03 );@@ -105,6 +114,28 @@ static void pci_host_addr_space_init(struct pci_addr_space as[], int nr_as, } } +static void pci_host_bridge_print (struct pci_host_bridge *host) +{ +printf ("PCIe start %016llx size %016llx " +"bus %02x bus_max %02x #spaces %d\n\n" , + host->cpu_range.start, host->cpu_range.size, + host->bus, host->bus_max, host->nr_addr_spaces); +} + +static void pci_address_spaces_print (struct pci_addr_space as[],int nr_as) +{ +int i; + +for (i =0 ; i < nr_as; i++, as++) { +printf ("%s address space:\n" +"CPU: start %016llx size %016llx\n" +"PCI: start %016llx size %016llx\n\n" , + addr_space_desc[flags_to_type (as->of_flags)], + as->cpu_range.start, as->cpu_range.size, + as->pci_range.start, as->pci_range.size); + } +} +/* * Probe DT for a generic PCI host controller * See kernel Documentation/devicetree/bindings/pci/host-generic-pci.txt @@ -180,9 +211,13 @@ static struct pci_host_bridge *pci_host_bridge_probe(void) host->bus_max = bus_max; host->nr_addr_spaces = nr_addr_spaces;
lib/pci-host-generic.c static struct pci_host_bridge *pci_host_bridge_probe(void) · line 222
Quoted 10 lines
lib/pci-host-generic.c +4
-0+pci_host_bridge_print (host); +pci_host_addr_space_init (&host->addr_space[0 ], nr_addr_spaces, (u32 *)prop->data, nr_range_cells); +pci_address_spaces_print (&host->addr_space[0 ], host->nr_addr_spaces); +return host; }
Looks good
drew
PATCH 10/11 arm/pci: PCI devices basic info printing
In reply to Alexander Gordeev--- lib/pci-host-generic.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
lib/pci-host-generic.c +34 -0
@@ -352,6 +352,35 @@ static phys_addr_t pci_alloc_res(struct pci_host_bridge *host,return addr; } +static void pci_dev_print (void *conf) +{ +u16 vendor_id =pci_config_readw (conf,PCI_VENDOR_ID ); +u16 device_id =pci_config_readw (conf,PCI_DEVICE_ID ); +u8 header =pci_config_readb (conf,PCI_HEADER_TYPE ); +u8 progif =pci_config_readb (conf,PCI_CLASS_PROG ); +u8 subcl =pci_config_readb (conf,PCI_CLASS_DEVICE ); +u8 class =pci_config_readb (conf,PCI_CLASS_DEVICE +1 ); + +printf ("conf %p vendor_id %04x device_id %04x type %d " +"progif %02x class %02x subcl %02x\n" , + conf, vendor_id, device_id, header, + progif, class, subcl); +} + +static void pci_dev_bar_print (int bar,pci_res_type_t type, +phys_addr_t addr,u64 size,bool is64) +{ +const char *desc = addr_space_desc[type]; + +if (is64) { +printf ("\tBAR#%d,%d [%-7s %02x-%02x]\n" , + bar, bar +1 , desc, addr, addr + size -1 ); + }else { +printf ("\tBAR#%d [%-7s %02x-%02x]\n" , + bar, desc, addr, addr + size -1 ); + } +} +int pci_bus_scan (struct pci *pci) {void *conf;
@@ -365,6 +394,8 @@ int pci_bus_scan(struct pci *pci)int nr_dev =0 ;for_each_pci_dev (pci, dev, conf) { +pci_dev_print (conf); +/* We are only interested in normal PCI devices */ if (pci_config_readb (conf,PCI_HEADER_TYPE ) !=PCI_HEADER_TYPE_NORMAL )
@@ -375,6 +406,9 @@ int pci_bus_scan(struct pci *pci)break ; addr =pci_alloc_res (pci->sysdata, type, size);pci_set_bar (conf, bar, addr, is64); + +pci_dev_bar_print (bar, type, addr, size, is64); +if (is64) bar++;
Signature · 1 line
1.8.3.1
Re: [kvm-unit-tests PATCH 10/11] arm/pci: PCI devices basic info printing
Quoted 68 lines · Alexander Gordeev
Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci-host-generic.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)lib/pci-host-generic.c +34
-0@@ -352,6 +352,35 @@ static phys_addr_t pci_alloc_res(struct pci_host_bridge *host,return addr; } +static void pci_dev_print (void *conf) +{ +u16 vendor_id =pci_config_readw (conf,PCI_VENDOR_ID ); +u16 device_id =pci_config_readw (conf,PCI_DEVICE_ID ); +u8 header =pci_config_readb (conf,PCI_HEADER_TYPE ); +u8 progif =pci_config_readb (conf,PCI_CLASS_PROG ); +u8 subcl =pci_config_readb (conf,PCI_CLASS_DEVICE ); +u8 class =pci_config_readb (conf,PCI_CLASS_DEVICE +1 ); + +printf ("conf %p vendor_id %04x device_id %04x type %d " +"progif %02x class %02x subcl %02x\n" , + conf, vendor_id, device_id, header, + progif, class, subcl); +} + +static void pci_dev_bar_print (int bar,pci_res_type_t type, +phys_addr_t addr,u64 size,bool is64) +{ +const char *desc = addr_space_desc[type]; + +if (is64) { +printf ("\tBAR#%d,%d [%-7s %02x-%02x]\n" , + bar, bar +1 , desc, addr, addr + size -1 ); + }else { +printf ("\tBAR#%d [%-7s %02x-%02x]\n" , + bar, desc, addr, addr + size -1 ); + } +} +int pci_bus_scan (struct pci *pci) {void *conf;@@ -365,6 +394,8 @@ int pci_bus_scan(struct pci *pci)int nr_dev =0 ;for_each_pci_dev (pci, dev, conf) { +pci_dev_print (conf); +/* We are only interested in normal PCI devices */ if (pci_config_readb (conf,PCI_HEADER_TYPE ) !=PCI_HEADER_TYPE_NORMAL )@@ -375,6 +406,9 @@ int pci_bus_scan(struct pci *pci)break ; addr =pci_alloc_res (pci->sysdata, type, size);pci_set_bar (conf, bar, addr, is64); + +pci_dev_bar_print (bar, type, addr, size, is64); +if (is64) bar++;Signature · 1 line
1.8.3.1
I don't think we'll want to print on every bus scan. We should have a special pci_dump_devices or something that can be called if needed.
Otherwise looks good
drew
PATCH 11/11 arm/pci: PCI testdev test flavour printing
In reply to Alexander Gordeev--- lib/pci-testdev.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
lib/pci-testdev.c +18 -0
@@ -140,6 +140,23 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test,return true; } +void pci_testdev_print (struct pci_test_dev_hdr *test, +struct pci_testdev_ops *ops) +{ +bool io = (ops == &pci_testdev_io_ops); +int i; + +printf ("pci-testdev %3s: " , io ?"io" :"mem" ); +for (i =0 ;; ++i) { +char c = ops->read8 (&test->name[i]); +if (!c) +break ; +printf ("%c" , c); + } +printf ("\n" ); + +} +static int pci_testdev_all (struct pci_test_dev_hdr *test,struct pci_testdev_ops *ops) {
@@ -148,6 +165,7 @@ static int pci_testdev_all(struct pci_test_dev_hdr *test,for (i =0 ;; i++) {if (!pci_testdev_one (test, i, ops))break ; +pci_testdev_print (test, ops); }return i;
Signature · 1 line
1.8.3.1
Re: [kvm-unit-tests PATCH 11/11] arm/pci: PCI testdev test flavour printing
Quoted 45 lines · Alexander Gordeev
Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci-testdev.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)lib/pci-testdev.c +18
-0@@ -140,6 +140,23 @@ static bool pci_testdev_one(struct pci_test_dev_hdr *test,return true; } +void pci_testdev_print (struct pci_test_dev_hdr *test, +struct pci_testdev_ops *ops) +{ +bool io = (ops == &pci_testdev_io_ops); +int i; + +printf ("pci-testdev %3s: " , io ?"io" :"mem" ); +for (i =0 ;; ++i) { +char c = ops->read8 (&test->name[i]); +if (!c) +break ; +printf ("%c" , c); + } +printf ("\n" ); + +} +static int pci_testdev_all (struct pci_test_dev_hdr *test,struct pci_testdev_ops *ops) {@@ -148,6 +165,7 @@ static int pci_testdev_all(struct pci_test_dev_hdr *test,for (i =0 ;; i++) {if (!pci_testdev_one (test, i, ops))break ; +pci_testdev_print (test, ops); }return i;Signature · 1 line
1.8.3.1
This can be squashed into 6/11-8/11
Thanks,
drew
Quoted 17 lines · Alexander Gordeev
This series extends the kvm-unit-tests/arm framework to support PCI.
Cc Andrew Jones <[email protected]>Alexander Gordeev (11):
arm/pci: Device tree PCI probing
arm/pci: PCI bus scanning
arm/pci: Read devices BARs
arm/pci: Allocate and assign memory/io space resources arm/pci: Addpci_find_dev()andpci_bar_addr()functions arm/pci: PCI testdev existence test
arm/pci: PCI device operation test
arm/pci: PCI device read/write test
arm/pci: PCI host bridge info printing
arm/pci: PCI devices basic info printing arm/pci: PCI testdev test flavour printing
Quoted 18 lines
arm/pci-test.c | 32 +++ config/config-arm-common.mak | 6 +- lib/alloc.c | 3 - lib/libcflat.h | 3 + lib/pci-host-generic.c | 502 +++++++++++++++++++++++++++++++++++++++++++ lib/pci-host-generic.h | 37 ++++ lib/pci-testdev.c | 198 +++++++++++++++++ lib/pci.h | 33 +++ 8 files changed, 810 insertions(+), 4 deletions(-) create mode 100644 arm/pci-test.c create mode 100644 lib/pci-host-generic.c create mode 100644 lib/pci-host-generic.h create mode 100644 lib/pci-testdev.c create mode 100644 lib/pci.hSignature · 1 line
1.8.3.1
Nice start! I think we can reduce some of it down a bit though by integrating with x86 right now. I'll post a series that starts that effort. If x86 people like it, then you can rebase on that, do some patch squashing, and post again. Soon we'll have pci-testdev vmexit tests for ARM!
Thanks,
drew