PCI bus support
Subsystems (2)
cover PCI bus support
Hi Andrew,
This is PCI test RFC. The code is far from done, but all (re)work suggestions are present and up to comments.
One major idea we discussed is missing from this RFC. That is switching from 'pcidevaddr_t' to 'struct pci_dev *' for accessing PCI devices (and further down the chain - busses and bridges). The reasons are:
- the amount of altered code is quite big already and I would like first to check if the suggested changes are reasonable, before introducing further changes;
- the current implementation with 'pcidevaddr_t' does not seem likable to me, but it is the minimalistic indeed. So I would like to take the last chance to assess the trade-off between 'nice' and 'small' implementations;
Few highlights regarding the new implementation:
- many, but not all your previous comments were addressed -
I tried to fix most important ones;
- patch 'Factor out generic architecture code' most likely is not this series specific. It seems architectures do contain a code that could be factored out Linux-style;
- I did not put much thought into generic and x86 reworks. I only focused on PCI changes while putting off possible following decent implementation;
- an all-in-one helper:
bool pci_get_bar(void *conf, int bar, pci_res_type_t *type,
u64 *addr, u64 *size, bool *is64);
got replaced with a set of interfaces, which are less handy,
but seems what you were after:
phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num);
phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);- a need to translate PCI bus address to CPU physical address led to introduction of a new arch-specific interface:
phys_addr_t pci_xlate_addr(pcidevaddr_t dev, uint64_t pci_addr);
An arch would have to override this one in case PCI addresses and CPU physical addresses are not identical. Linux does it once - when creates a list of PCI resources. By contrast, this RFC calls pci_xlate_addr() each time pci_bar_addr() is called. That lets to avoid extra data structures;
Alexander Gordeev (15):
Update ioremap() prototype to conform to the Linux one
x86: Add basic ioremap() implementation x86/vmexit: Make use of ioremap()
pci: Fix indentation in generic PCI files pci/x86: Rename pci_config_read() to pci_config_readl() pci/x86: Add remaining PCI configuration space accessors
pci: Add pci_probe() and pci_shutdown()
pci: Rework pci_bar_addr()
pci: Add pci_bar_set()
pci: Add pci_print() and pci_type_desc() pci/x86: Adopt PCI framework changes
Factor out generic architecture code
pci/arm: Add generic ECAM host support
pci: Add pci-testdev PCI bus test device pci/arm: Add pci-testdev PCI device operation test
arm/Makefile.common | 7 +- arm/pci-test.c | 27 +++++ arm/run | 10 +- lib/alloc.h | 7 -- lib/arm64/asm/pci.h | 29 +++++ lib/asm-generic/io.h | 59 ++++++++- lib/asm-generic/pci.h | 29 ++++- lib/libcflat.h | 7 ++ lib/pci-host-generic.c | 321 +++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci-host-generic.h | 57 +++++++++ lib/pci-testdev.c | 188 +++++++++++++++++++++++++++++ lib/pci.c | 182 +++++++++++++++++++++++++--- lib/pci.h | 19 ++- lib/x86/asm/page.h | 1 + lib/x86/asm/pci.h | 27 ++++- lib/x86/io.c | 12 ++ lib/x86/io.h | 41 +++++-- lib/x86/smp.h | 4 - lib/x86/vm.c | 1 - lib/x86/vm.h | 12 +- x86/eventinj.c | 7 +- x86/kvmclock.c | 1 + x86/vmexit.c | 16 ++- 23 files changed, 980 insertions(+), 84 deletions(-) create mode 100644 arm/pci-test.c create mode 100644 lib/arm64/asm/pci.h 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/x86/asm/page.h
Signature · 1 line
1.8.3.1
RFC PATCH 1/15 Update ioremap() prototype to conform to the Linux one
--- lib/alloc.h | 7 ------- lib/asm-generic/io.h | 4 ++-- lib/libcflat.h | 7 +++++++ 3 files changed, 9 insertions(+), 9 deletions(-)
lib/alloc.h +0 -7
@@ -58,13 +58,6 @@ static inline void *memalign(size_t alignment, size_t size)return alloc_ops->memalign (alignment, size); } -#ifdef PHYS32 -typedef u32 phys_addr_t ; -#else -typedef u64 phys_addr_t ; -#endif -#define INVALID_PHYS_ADDR (~(phys_addr_t)0) -/* * phys_alloc is a very simple allocator which allows physical memory * to be partitioned into regions until all memory is allocated.
lib/asm-generic/io.h +2 -2
@@ -153,10 +153,10 @@ static inline u64 __bswap64(u64 x) ({ wmb(); __raw_writeq(cpu_to_le64(b), addr); })#ifndef ioremap -static inline void *ioremap(u64 phys_addr, size_t size __unused) +static inline void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused) {assert (sizeof (long ) ==8 || !(phys_addr >>32 )); -return (void *)(unsigned long )phys_addr; +return (void __iomem *)phys_addr; }#endif
lib/libcflat.h +7 -0
@@ -48,6 +48,13 @@ typedef _Bool bool;#define false 0#define true 1 +#ifdef PHYS32 +typedef u32 phys_addr_t ; +#else +typedef u64 phys_addr_t ; +#endif +#define INVALID_PHYS_ADDR (~(phys_addr_t)0) +#if __SIZEOF_LONG__ ==8 # define __PRI64_PREFIX "l"# define __PRIPTR_PREFIX "l"
Signature · 1 line
1.8.3.1
Re: [PATCH RFC 01/15] Update ioremap() prototype to conform to the Linux one
Quoted 49 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/alloc.h | 7 ------- lib/asm-generic/io.h | 4 ++-- lib/libcflat.h | 7 +++++++ 3 files changed, 9 insertions(+), 9 deletions(-)lib/alloc.h +0
-7@@ -58,13 +58,6 @@ static inline void *memalign(size_t alignment, size_t size)return alloc_ops->memalign (alignment, size); } -#ifdef PHYS32 -typedef u32 phys_addr_t ; -#else -typedef u64 phys_addr_t ; -#endif -#define INVALID_PHYS_ADDR (~(phys_addr_t)0) -/* * phys_alloc is a very simple allocator which allows physical memory * to be partitioned into regions until all memory is allocated. lib/asm-generic/io.h +2
-2@@ -153,10 +153,10 @@ static inline u64 __bswap64(u64 x) ({ wmb(); __raw_writeq(cpu_to_le64(b), addr); })#ifndef ioremap -static inline void *ioremap(u64 phys_addr, size_t size __unused) +static inline void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused) {assert (sizeof (long ) ==8 || !(phys_addr >>32 )); -return (void *)(unsigned long )phys_addr; +return (void __iomem *)phys_addr; }#endif lib/libcflat.h +0
-0
lib/libcflat.h typedef _Bool bool; · line 52
Quoted 5 lines
lib/libcflat.h +2
-0@@ -48,6 +48,13 @@ typedef _Bool bool;#define false 0#define true 1 +#ifdef PHYS32 +typedef u32 phys_addr_t ;
You can drop the #ifdef PHYS32 part. It's unused and unnecessary. Moving this typedef is good, and can lead to some cleanups of removing '#include <alloc.h>'s from several places that only included alloc.h to get the typedef.
Quoted 10 lines
lib/libcflat.h +5
-0+#else +typedef u64 phys_addr_t ; +#endif +#define INVALID_PHYS_ADDR (~(phys_addr_t)0) +#if __SIZEOF_LONG__ ==8 # define __PRI64_PREFIX "l"# define __PRIPTR_PREFIX "l"Signature · 1 line
1.8.3.1
Thanks,
drew
RFC PATCH 2/15 x86: Add basic ioremap() implementation
In reply to Alexander GordeevMake ioremap() remap just one page for now
--- lib/x86/io.c | 12 ++++++++++++ lib/x86/io.h | 5 +++++ 2 files changed, 17 insertions(+)
lib/x86/io.c +12 -0
@@ -1,6 +1,7 @@#include "libcflat.h" #include "smp.h" #include "io.h" +#include "vm.h" #ifndef USE_SERIAL #define USE_SERIAL #endif
@@ -81,3 +82,14 @@ void exit(int code) asmvolatile ("out %0, %1" : :"a" (code),"d" ((short )0xf4 ));#endif } + +void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused) +{ +phys_addr_t base =ALIGN (phys_addr,PAGE_SIZE ); +phys_addr_t offset = phys_addr - base; +void *page =alloc_vpages (1 ); + +install_page ((void *)read_cr3 (), base, page); + +return page + offset; +}
lib/x86/io.h +5 -0
@@ -1,6 +1,8 @@#ifndef IO_H #define IO_H +#define __iomem +static inline unsigned char inb (unsigned short port) {unsigned char value;
@@ -37,4 +39,7 @@ static inline void outl(unsigned int value, unsigned short port) asmvolatile ("outl %0, %w1" : :"a" (value),"Nd" (port)); } +#define ioremap ioremap +void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused); +#endif
Signature · 1 line
1.8.3.1
Re: [PATCH RFC 02/15] x86: Add basic ioremap() implementation
Quoted 1 line · Alexander Gordeev
Make
ioremap()remap just one page for now
Why restrict it to just one page? Should be able to round size up to next page multiple, right? And then actually use the size argument.
Quoted 60 lines
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/x86/io.c | 12 ++++++++++++ lib/x86/io.h | 5 +++++ 2 files changed, 17 insertions(+)lib/x86/io.c +12
-0@@ -1,6 +1,7 @@#include "libcflat.h" #include "smp.h" #include "io.h" +#include "vm.h" #ifndef USE_SERIAL #define USE_SERIAL #endif @@ -81,3 +82,14 @@ void exit(int code) asmvolatile ("out %0, %1" : :"a" (code),"d" ((short )0xf4 ));#endif } + +void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused) +{ +phys_addr_t base =ALIGN (phys_addr,PAGE_SIZE ); +phys_addr_t offset = phys_addr - base; +void *page =alloc_vpages (1 ); + +install_page ((void *)read_cr3 (), base, page); + +return page + offset; +}lib/x86/io.h +5
-0@@ -1,6 +1,8 @@#ifndef IO_H #define IO_H +#define __iomem +static inline unsigned char inb (unsigned short port) {unsigned char value;@@ -37,4 +39,7 @@ static inline void outl(unsigned int value, unsigned short port) asmvolatile ("outl %0, %w1" : :"a" (value),"Nd" (port)); } +#define ioremap ioremap +void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused); +#endif Signature · 1 line
1.8.3.1
Thanks,
drew
RFC PATCH 3/15 x86/vmexit: Make use of ioremap()
In reply to Alexander Gordeev--- x86/vmexit.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
x86/vmexit.c +3 -9
@@ -372,8 +372,7 @@ int main(int ac, char **av) {struct fadt_descriptor_rev1 *fadt;int i; -unsigned long membar =0 , base, offset; -void *m; +uint64_t membar =0 ;pcidevaddr_t pcidev;smp_init ();
@@ -395,17 +394,12 @@ int main(int ac, char **av) }if (pci_bar_is_memory (pcidev, i)) { membar =pci_bar_addr (pcidev, i); - base = membar & ~4095 ; - offset = membar - base; - m =alloc_vpages (1 ); - -install_page ((void *)read_cr3 (), base, m); - pci_test.memaddr = m + offset; + pci_test.memaddr =ioremap (membar,0 ); }else { pci_test.iobar =pci_bar_addr (pcidev, i); } } -printf ("pci-testdev at 0x%x membar %lx iobar %x\n" , +printf ("pci-testdev at 0x%x membar %" PRIx64" iobar %x\n" , pcidev, membar, pci_test.iobar); }
Signature · 1 line
1.8.3.1
Re: [PATCH RFC 03/15] x86/vmexit: Make use of ioremap()
Quoted 12 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- x86/vmexit.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)x86/vmexit.c +0
-0
x86/vmexit.c int main(int ac, char **av) · line 375
Quoted 6 lines
x86/vmexit.c +1
-2@@ -372,8 +372,7 @@ int main(int ac, char **av) {struct fadt_descriptor_rev1 *fadt;int i; -unsigned long membar =0 , base, offset; -void *m; +uint64_t membar =0 ;
Why change membar to u64? pci_bar_addr still returns a ulong.
Quoted 4 lines
x86/vmexit.c +0
-0pcidevaddr_t pcidev;smp_init ();
x86/vmexit.c int main(int ac, char **av) · line 397
Quoted 10 lines
x86/vmexit.c +1
-6@@ -395,17 +394,12 @@ int main(int ac, char **av) }if (pci_bar_is_memory (pcidev, i)) { membar =pci_bar_addr (pcidev, i); - base = membar & ~4095 ; - offset = membar - base; - m =alloc_vpages (1 ); - -install_page ((void *)read_cr3 (), base, m); - pci_test.memaddr = m + offset; + pci_test.memaddr =ioremap (membar,0 );
Having ioremap's size argument be useful and passing a '1' here would be much nicer.
Quoted 16 lines
x86/vmexit.c +1
-1}else { pci_test.iobar =pci_bar_addr (pcidev, i); } } -printf ("pci-testdev at 0x%x membar %lx iobar %x\n" , +printf ("pci-testdev at 0x%x membar %" PRIx64" iobar %x\n" , pcidev, membar, pci_test.iobar); }Signature · 6 lines
1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 03/15] x86/vmexit: Make use of ioremap()
Quoted 8 lines · Andrew Jones
Quoted 7 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- x86/vmexit.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
Quoted 13 lines
x86/vmexit.c int main(int ac, char **av) · line 375
Quoted 11 lines
x86/vmexit.c +1
-2@@ -372,8 +372,7 @@ int main(int ac, char **av) {struct fadt_descriptor_rev1 *fadt;int i; -unsigned long membar =0 , base, offset; -void *m; +uint64_t membar =0 ;Why change membar to u64?
pci_bar_addrstill returns a ulong.
In the end it should be neither, but rather phys_addr_t, unless I am missing something. But at this point pci_bar_addr() indeed returns a ulong, so changing it in this patch is probably jumping the gun.
Quoted 34 lines
Quoted 4 lines
x86/vmexit.c +0
-0pcidevaddr_t pcidev;smp_init ();x86/vmexit.c int main(int ac, char **av) · line 397
Quoted 10 lines
x86/vmexit.c +1
-6@@ -395,17 +394,12 @@ int main(int ac, char **av) }if (pci_bar_is_memory (pcidev, i)) { membar =pci_bar_addr (pcidev, i); - base = membar & ~4095 ; - offset = membar - base; - m =alloc_vpages (1 ); - -install_page ((void *)read_cr3 (), base, m); - pci_test.memaddr = m + offset; + pci_test.memaddr =ioremap (membar,0 );Having ioremap's size argument be useful and passing a '1' here would be much nicer.
Quoted 16 lines
x86/vmexit.c +1
-1}else { pci_test.iobar =pci_bar_addr (pcidev, i); } } -printf ("pci-testdev at 0x%x membar %lx iobar %x\n" , +printf ("pci-testdev at 0x%x membar %" PRIx64" iobar %x\n" , pcidev, membar, pci_test.iobar); }Signature · 6 lines
1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC PATCH 4/15 pci: Fix indentation in generic PCI files
In reply to Alexander Gordeev--- lib/pci.c | 36 ++++++++++++++++++------------------ lib/pci.h | 2 +- 2 files changed, 19 insertions(+), 19 deletions(-)
lib/pci.c +18 -18
@@ -10,34 +10,34 @@/* Scan bus look for a specific device. Only bus 0 scanned for now. */ pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id) { -unsigned dev; -for (dev =0 ; dev <256 ; ++dev) { -uint32_t id =pci_config_read (dev,0 ); -if ((id &0xFFFF ) == vendor_id && (id >>16 ) == device_id) { -return dev; - } - } -return PCIDEVADDR_INVALID ; +unsigned dev; +for (dev =0 ; dev <256 ; ++dev) { +uint32_t id =pci_config_read (dev,0 ); +if ((id &0xFFFF ) == vendor_id && (id >>16 ) == device_id) { +return dev; + } + } +return PCIDEVADDR_INVALID ; }unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num) { -uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); -if (bar &PCI_BASE_ADDRESS_SPACE_IO ) { -return bar &PCI_BASE_ADDRESS_IO_MASK ; - }else { -return bar &PCI_BASE_ADDRESS_MEM_MASK ; - } +uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); +if (bar &PCI_BASE_ADDRESS_SPACE_IO ) { +return bar &PCI_BASE_ADDRESS_IO_MASK ; + }else { +return bar &PCI_BASE_ADDRESS_MEM_MASK ; + } }bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num) { -uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); -return !(bar &PCI_BASE_ADDRESS_SPACE_IO ); +uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); +return !(bar &PCI_BASE_ADDRESS_SPACE_IO ); }bool pci_bar_is_valid (pcidevaddr_t dev,int bar_num) { -uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); -return bar; +uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); +return bar; }
lib/pci.h +1 -1
@@ -12,7 +12,7 @@typedef uint16_t pcidevaddr_t ;enum { -PCIDEVADDR_INVALID =0xffff , +PCIDEVADDR_INVALID =0xffff , };pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id);unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num);
Signature · 1 line
1.8.3.1
Re: [PATCH RFC 04/15] pci: Fix indentation in generic PCI files
Quoted 15 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci.c | 36 ++++++++++++++++++------------------ lib/pci.h | 2 +- 2 files changed, 19 insertions(+), 19 deletions(-)lib/pci.c +0
-0@@ -10,34 +10,34 @@/* Scan bus look for a specific device. Only bus 0 scanned for now. */ pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id)
lib/pci.c line 13
Quoted 10 lines
lib/pci.c +1
-8{ -unsigned dev; -for (dev =0 ; dev <256 ; ++dev) { -uint32_t id =pci_config_read (dev,0 ); -if ((id &0xFFFF ) == vendor_id && (id >>16 ) == device_id) { -return dev; - } - } -return PCIDEVADDR_INVALID ; +unsigned dev;
While at it add an empty line here.
lib/pci.c line 18
Quoted 5 lines
lib/pci.c +5
-0+for (dev =0 ; dev <256 ; ++dev) { +uint32_t id =pci_config_read (dev,0 ); +if ((id &0xFFFF ) == vendor_id && (id >>16 ) == device_id) { +return dev; + }
Can drop these {}
lib/pci.c line 25
Quoted 13 lines
lib/pci.c +3
-6+ } +return PCIDEVADDR_INVALID ; }unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num) { -uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); -if (bar &PCI_BASE_ADDRESS_SPACE_IO ) { -return bar &PCI_BASE_ADDRESS_IO_MASK ; - }else { -return bar &PCI_BASE_ADDRESS_MEM_MASK ; - } +uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 );
Need empty line here.
lib/pci.c line 30
Quoted 5 lines
lib/pci.c +5
-0+if (bar &PCI_BASE_ADDRESS_SPACE_IO ) { +return bar &PCI_BASE_ADDRESS_IO_MASK ; + }else { +return bar &PCI_BASE_ADDRESS_MEM_MASK ; + }
Can drop the {} in the if and else here too.
Quoted 37 lines
lib/pci.c +4
-4}bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num) { -uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); -return !(bar &PCI_BASE_ADDRESS_SPACE_IO ); +uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); +return !(bar &PCI_BASE_ADDRESS_SPACE_IO ); }bool pci_bar_is_valid (pcidevaddr_t dev,int bar_num) { -uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); -return bar; +uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); +return bar; }lib/pci.h +1
-1@@ -12,7 +12,7 @@typedef uint16_t pcidevaddr_t ;enum { -PCIDEVADDR_INVALID =0xffff , +PCIDEVADDR_INVALID =0xffff , };pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id);unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num);Signature · 6 lines
1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 04/15] pci: Fix indentation in generic PCI files
Quoted 20 lines · Andrew Jones
Quoted 19 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci.c | 36 ++++++++++++++++++------------------ lib/pci.h | 2 +- 2 files changed, 19 insertions(+), 19 deletions(-)lib/pci.c +0
-3@@ -10,34 +10,34 @@/* Scan bus look for a specific device. Only bus 0 scanned for now. */ pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id) { -unsigned dev; -for (dev =0 ; dev <256 ; ++dev) { -uint32_t id =pci_config_read (dev,0 );
Quoted 8 lines
lib/pci.c line 13
Quoted 6 lines
lib/pci.c +1
-5-if ((id &0xFFFF ) == vendor_id && (id >>16 ) == device_id) { -return dev; - } - } -return PCIDEVADDR_INVALID ; +unsigned dev;While at it add an empty line here.
This and likely following such suggestions are addressed in a following rework patch. Both patches seem good to me for fixing the style - just let me know whichever you prefer ;)
Quoted 69 lines
lib/pci.c line 18
Quoted 5 lines
lib/pci.c +5
-0+for (dev =0 ; dev <256 ; ++dev) { +uint32_t id =pci_config_read (dev,0 ); +if ((id &0xFFFF ) == vendor_id && (id >>16 ) == device_id) { +return dev; + }Can drop these {}
lib/pci.c line 25
Quoted 13 lines
lib/pci.c +3
-6+ } +return PCIDEVADDR_INVALID ; }unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num) { -uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); -if (bar &PCI_BASE_ADDRESS_SPACE_IO ) { -return bar &PCI_BASE_ADDRESS_IO_MASK ; - }else { -return bar &PCI_BASE_ADDRESS_MEM_MASK ; - } +uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 );Need empty line here.
lib/pci.c line 30
Quoted 5 lines
lib/pci.c +5
-0+if (bar &PCI_BASE_ADDRESS_SPACE_IO ) { +return bar &PCI_BASE_ADDRESS_IO_MASK ; + }else { +return bar &PCI_BASE_ADDRESS_MEM_MASK ; + }Can drop the {} in the if and else here too.
Quoted 37 lines
lib/pci.c +4
-4}bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num) { -uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); -return !(bar &PCI_BASE_ADDRESS_SPACE_IO ); +uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); +return !(bar &PCI_BASE_ADDRESS_SPACE_IO ); }bool pci_bar_is_valid (pcidevaddr_t dev,int bar_num) { -uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); -return bar; +uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); +return bar; }lib/pci.h +1
-1@@ -12,7 +12,7 @@typedef uint16_t pcidevaddr_t ;enum { -PCIDEVADDR_INVALID =0xffff , +PCIDEVADDR_INVALID =0xffff , };pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id);unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num);Signature · 6 lines
1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 04/15] pci: Fix indentation in generic PCI files
Quoted 30 lines · Alexander Gordeev
Quoted 20 lines · Andrew Jones
Quoted 19 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci.c | 36 ++++++++++++++++++------------------ lib/pci.h | 2 +- 2 files changed, 19 insertions(+), 19 deletions(-)lib/pci.c +0
-3@@ -10,34 +10,34 @@/* Scan bus look for a specific device. Only bus 0 scanned for now. */ pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id) { -unsigned dev; -for (dev =0 ; dev <256 ; ++dev) { -uint32_t id =pci_config_read (dev,0 );Quoted 8 lines
lib/pci.c line 13
Quoted 6 lines
lib/pci.c +1
-5-if ((id &0xFFFF ) == vendor_id && (id >>16 ) == device_id) { -return dev; - } - } -return PCIDEVADDR_INVALID ; +unsigned dev;While at it add an empty line here.
Quoted 3 lines
This and likely following such suggestions are addressed in a following rework patch. Both patches seem good to me for fixing the style - just let me know whichever you prefer ;)
I'd prefer this patch to do all the code style fixes, and then not mix code style changes into the other patches. The subject of this patch should be updated too.
Thanks,
drew
RFC PATCH 5/15 pci/x86: Rename pci_config_read() to pci_config_readl()
In reply to Alexander GordeevThis is needed to facilitate other-sized PCI configuration space accessors, not just 32 bit one.
--- lib/pci.c | 8 ++++---- lib/x86/asm/pci.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-)
lib/pci.c +4 -4
@@ -12,7 +12,7 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id) {unsigned dev;for (dev =0 ; dev <256 ; ++dev) { -uint32_t id =pci_config_read (dev,0 ); +uint32_t id =pci_config_readl (dev,0 );if ((id &0xFFFF ) == vendor_id && (id >>16 ) == device_id) {return dev; }
@@ -22,7 +22,7 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num) { -uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); +uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 );if (bar &PCI_BASE_ADDRESS_SPACE_IO ) {return bar &PCI_BASE_ADDRESS_IO_MASK ; }else {
@@ -32,12 +32,12 @@ unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num) { -uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); +uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 );return !(bar &PCI_BASE_ADDRESS_SPACE_IO ); }bool pci_bar_is_valid (pcidevaddr_t dev,int bar_num) { -uint32_t bar =pci_config_read (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); +uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 );return bar; }
lib/x86/asm/pci.h +1 -1
@@ -9,7 +9,7 @@#include "pci.h" #include "x86/io.h" -static inline uint32_t pci_config_read (pcidevaddr_t dev,uint8_t reg) +static inline uint32_t pci_config_readl (pcidevaddr_t dev,uint8_t reg) {uint32_t index = reg | (dev <<8 ) | (0x1 <<31 );outl (index,0xCF8 );
Signature · 1 line
1.8.3.1
Re: [PATCH RFC 05/15] pci/x86: Rename pci_config_read() to pci_config_readl()
Quoted 10 lines · Alexander Gordeev
This is needed to facilitate other-sized PCI configuration space accessors, not just 32 bit one.
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci.c | 8 ++++---- lib/x86/asm/pci.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-)
Re: [PATCH RFC 05/15] pci/x86: Rename pci_config_read() to pci_config_readl()
In reply to Alexander GordeevQuoted 10 lines · Alexander Gordeev
This is needed to facilitate other-sized PCI configuration space accessors, not just 32 bit one.
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci.c | 8 ++++---- lib/x86/asm/pci.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-)
...
lib/x86/asm/pci.h line 15
Quoted 13 lines
lib/x86/asm/pci.h +1
-1@@ -9,7 +9,7 @@#include "pci.h" #include "x86/io.h" -static inline uint32_t pci_config_read (pcidevaddr_t dev,uint8_t reg) +static inline uint32_t pci_config_readl (pcidevaddr_t dev,uint8_t reg) {uint32_t index = reg | (dev <<8 ) | (0x1 <<31 );outl (index,0xCF8 );
Did you consider "pci_config_read32" instead? ... that would be even more clear about the size. Anyway, I'm also fine with "readl", so:
Re: [PATCH RFC 05/15] pci/x86: Rename pci_config_read() to pci_config_readl()
Quoted 20 lines · Thomas Huth
Quoted 10 lines · Alexander Gordeev
This is needed to facilitate other-sized PCI configuration space accessors, not just 32 bit one.
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci.c | 8 ++++---- lib/x86/asm/pci.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-)...
Quoted 8 lines
lib/x86/asm/pci.h +0
-0@@ -9,7 +9,7 @@#include "pci.h" #include "x86/io.h"
Quoted 8 lines
lib/x86/asm/pci.h line 15
Quoted 5 lines
lib/x86/asm/pci.h +1
-1-static inline uint32_t pci_config_read (pcidevaddr_t dev,uint8_t reg) +static inline uint32_t pci_config_readl (pcidevaddr_t dev,uint8_t reg) {uint32_t index = reg | (dev <<8 ) | (0x1 <<31 );outl (index,0xCF8 );Did you consider "
pci_config_read32" instead? ... that would be even more clear about the size. Anyway, I'm also fine with "readl", so:
Actually, yes. I considered mimicking Linux ones:
int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val); int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val); int pci_read_config_dword(const struct pci_dev *dev, int where, u32 *val); int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val); int pci_write_config_word(const struct pci_dev *dev, int where, u16 val); int pci_write_config_dword(const struct pci_dev *dev, int where, u32 val);
But these are (a) take 'struct pci_dev *' as a parameter and (b) can report a failure. So I was indecisive wrt naming - at least until the trade-off between 'struct pci_dev *' vs pcidevaddr_t is finalized.
For now, the PCI accessor names mimick "seabios" ones and kind of correlate vs IO accessors: pci_config_readl <-> readl etc.
Quoted 2 lines
Reviewed-by Thomas Huth <[email protected]>
RFC PATCH 6/15 pci/x86: Add remaining PCI configuration space accessors
In reply to Alexander Gordeev--- lib/x86/asm/pci.h | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
lib/x86/asm/pci.h +21 -2
@@ -9,11 +9,30 @@#include "pci.h" #include "x86/io.h" +#define PCI_CONF1_ADDRESS (dev, reg) ((0x1 << 31) | (dev << 8) | reg) +static inline uint32_t pci_config_readl (pcidevaddr_t dev,uint8_t reg) { -uint32_t index = reg | (dev <<8 ) | (0x1 <<31 ); -outl (index,0xCF8 ); +outl (PCI_CONF1_ADDRESS (dev, reg),0xCF8 );return inl (0xCFC ); } +static inline uint8_t pci_config_readb (pcidevaddr_t dev,uint8_t reg) +{ +outl (PCI_CONF1_ADDRESS (dev, reg),0xCF8 ); +return inb (0xCFC ); +} + +static inline uint16_t pci_config_readw (pcidevaddr_t dev,uint8_t reg) +{ +outl (PCI_CONF1_ADDRESS (dev, reg),0xCF8 ); +return inw (0xCFC ); +} + +static inline void pci_config_writel (pcidevaddr_t dev,uint8_t reg,uint32_t val) +{ +outl (PCI_CONF1_ADDRESS (dev, reg),0xCF8 ); +outl (val,0xCFC ); +} +#endif
Signature · 1 line
1.8.3.1
Re: [PATCH RFC 06/15] pci/x86: Add remaining PCI configuration space accessors
Quoted 44 lines · Alexander Gordeev
Signed-off-by Alexander Gordeev <[email protected]>--- lib/x86/asm/pci.h | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)lib/x86/asm/pci.h +21
-2@@ -9,11 +9,30 @@#include "pci.h" #include "x86/io.h" +#define PCI_CONF1_ADDRESS (dev, reg) ((0x1 << 31) | (dev << 8) | reg) +static inline uint32_t pci_config_readl (pcidevaddr_t dev,uint8_t reg) { -uint32_t index = reg | (dev <<8 ) | (0x1 <<31 ); -outl (index,0xCF8 ); +outl (PCI_CONF1_ADDRESS (dev, reg),0xCF8 );return inl (0xCFC ); } +static inline uint8_t pci_config_readb (pcidevaddr_t dev,uint8_t reg) +{ +outl (PCI_CONF1_ADDRESS (dev, reg),0xCF8 ); +return inb (0xCFC ); +} + +static inline uint16_t pci_config_readw (pcidevaddr_t dev,uint8_t reg) +{ +outl (PCI_CONF1_ADDRESS (dev, reg),0xCF8 ); +return inw (0xCFC ); +} + +static inline void pci_config_writel (pcidevaddr_t dev,uint8_t reg,uint32_t val) +{ +outl (PCI_CONF1_ADDRESS (dev, reg),0xCF8 ); +outl (val,0xCFC ); +} +#endif Signature · 1 line
1.8.3.1
RFC PATCH 7/15 pci: Add pci_probe() and pci_shutdown()
In reply to Alexander Gordeev--- lib/asm-generic/pci.h | 15 ++++++++++++++- lib/pci.c | 1 - lib/pci.h | 2 ++ 3 files changed, 16 insertions(+), 2 deletions(-)
lib/asm-generic/pci.h +14 -1
@@ -1,4 +1,17 @@#ifndef _ASM_GENERIC_PCI_H_#define _ASM_GENERIC_PCI_H_ -#error need architecture specific asm/pci.h + +#ifndef pci_probe +static inline bool pci_probe (void ) +{ +return true; +} +#endif + +#ifndef pci_shutdown +static inline void pci_shutdown (void ) +{ +} +#endif +#endif
lib/pci.c +0 -1
@@ -5,7 +5,6 @@*/ #include <linux/pci_regs.h> #include "pci.h" -#include "asm/pci.h" /* Scan bus look for a specific device. Only bus 0 scanned for now. */ pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id)
lib/pci.h +2 -0
@@ -39,4 +39,6 @@ struct pci_test_dev_hdr { uint8_t name[]; }; +#include "asm/pci.h" +#endif /* PCI_H */
Signature · 1 line
1.8.3.1
Re: [PATCH RFC 07/15] pci: Add pci_probe() and pci_shutdown()
Quoted 22 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/pci.h | 15 ++++++++++++++- lib/pci.c | 1 - lib/pci.h | 2 ++ 3 files changed, 16 insertions(+), 2 deletions(-)lib/asm-generic/pci.h +5
-1@@ -1,4 +1,17 @@ #ifndef _ASM_GENERIC_PCI_H_ #define _ASM_GENERIC_PCI_H_ -#error need architecture specific asm/pci.h + +#ifndef pci_probe +static inline bool pci_probe(void) +{ + return true;
lib/asm-generic/pci.h line 17
Quoted 10 lines
lib/asm-generic/pci.h +9
-0+} +#endif + +#ifndef pci_shutdown +static inline void pci_shutdown (void ) +{ +} +#endif +#endif
What are these functions supposed to do exactly? (and why is the default implementation empty?) ... some comments would be really helpful here.
Thomas
Re: [PATCH RFC 07/15] pci: Add pci_probe() and pci_shutdown()
Quoted 28 lines · Thomas Huth
Quoted 27 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/pci.h | 15 ++++++++++++++- lib/pci.c | 1 - lib/pci.h | 2 ++ 3 files changed, 16 insertions(+), 2 deletions(-)lib/asm-generic/pci.h +10
-1@@ -1,4 +1,17 @@#ifndef _ASM_GENERIC_PCI_H_#define _ASM_GENERIC_PCI_H_ -#error need architecture specific asm/pci.h + +#ifndef pci_probe +static inline bool pci_probe (void ) +{ +return true; +} +#endif + +#ifndef pci_shutdown +static inline void pci_shutdown (void )
Quoted 8 lines
lib/asm-generic/pci.h line 17
Quoted 5 lines
lib/asm-generic/pci.h +4
-0+{ +} +#endif +#endif What are these functions supposed to do exactly? (and why is the default implementation empty?) ... some comments would be really helpful here.
These two are expected to init/scan and shutdown PCI busses - something a firmware or bios do. Only when pci_probe() succeeded all pci_* public functions are expected functional.
They are empty, because this is default implementation. Architectures that need initialize PCI bus should override these functions.
Quoted 2 lines
Thomas
Re: [PATCH RFC 07/15] pci: Add pci_probe() and pci_shutdown()
Quoted 38 lines · Alexander Gordeev
Quoted 28 lines · Thomas Huth
Quoted 27 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/pci.h | 15 ++++++++++++++- lib/pci.c | 1 - lib/pci.h | 2 ++ 3 files changed, 16 insertions(+), 2 deletions(-)lib/asm-generic/pci.h +10
-1@@ -1,4 +1,17 @@#ifndef _ASM_GENERIC_PCI_H_#define _ASM_GENERIC_PCI_H_ -#error need architecture specific asm/pci.h + +#ifndef pci_probe +static inline bool pci_probe (void ) +{ +return true; +} +#endif + +#ifndef pci_shutdown +static inline void pci_shutdown (void )Quoted 8 lines
lib/asm-generic/pci.h line 17
Quoted 5 lines
lib/asm-generic/pci.h +4
-0+{ +} +#endif +#endif What are these functions supposed to do exactly? (and why is the default implementation empty?) ... some comments would be really helpful here.
Quoted 6 lines
These two are expected to init/scan and shutdown PCI busses - something a firmware or bios do. Only when
pci_probe()succeeded all pci_* public functions are expected functional.They are empty, because this is default implementation. Architectures that need initialize PCI bus should override these functions.
I think a function like pci_probe would simply call probe functions for specific pci controller / host bridges that it knows about. For starters it'll only know about one. Even the probe code of that host bridge will be general (i.e. not arch-specific), but it will call arch specific functions. IOW, these functions should just be in lib/pci.[ch], and only introduced when they have non-empty bodies. And we should leave the #error in lib/asm-generic/pci.h
Thanks,
drew
Quoted 7 lines
Quoted 3 lines
Thomas
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC PATCH 8/15 pci: Rework pci_bar_addr()
In reply to Alexander GordeevThis update makes pci_bar_addr() interface 64 bit BARs aware and introduces a concept of PCI address translation. In cases PCI bus and CPU bus physical addresses are not indentical the architecutre should override pci_xslate_addr() interface to provide necessary mapping between the two.
--- lib/asm-generic/pci.h | 8 +++++ lib/pci.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++----- lib/pci.h | 4 ++- 3 files changed, 87 insertions(+), 9 deletions(-)
lib/asm-generic/pci.h +8 -0
@@ -14,4 +14,12 @@ static inline void pci_shutdown(void) }#endif +#ifndef pci_xslate_addr +static inline +phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) +{ +return addr; +} +#endif +#endif
lib/pci.c +76 -8
102 lines not on this page · raw message
lib/pci.h +3 -1
@@ -15,7 +15,9 @@ enum { PCIDEVADDR_INVALID = 0xffff, };pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id); -unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num); +phys_addr_t pci_bar_addr (pcidevaddr_t dev,int bar_num); +phys_addr_t pci_bar_size (pcidevaddr_t dev,int bar_num); +bool pci_bar_is64 (pcidevaddr_t dev,int bar_num);bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num);bool pci_bar_is_valid (pcidevaddr_t dev,int bar_num);
Signature · 1 line
1.8.3.1
Re: [PATCH RFC 08/15] pci: Rework pci_bar_addr()
Quoted 60 lines · Alexander Gordeev
This update makes
pci_bar_addr()interface 64 bit BARs aware and introduces a concept of PCI address translation. In cases PCI bus and CPU bus physical addresses are not indentical the architecutre should overridepci_xslate_addr()interface to provide necessary mapping between the two.Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/pci.h | 8 +++++ lib/pci.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++----- lib/pci.h | 4 ++- 3 files changed, 87 insertions(+), 9 deletions(-)lib/asm-generic/pci.h +8
-0@@ -14,4 +14,12 @@ static inline void pci_shutdown(void) }#endif +#ifndef pci_xslate_addr +static inline +phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) +{ +return addr; +} +#endif +#endif lib/pci.c +9
-5@@ -9,23 +9,78 @@/* Scan bus look for a specific device. Only bus 0 scanned for now. */ pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id) { -unsigned dev; -for (dev =0 ; dev <256 ; ++dev) { -uint32_t id =pci_config_readl (dev,0 ); -if ((id &0xFFFF ) == vendor_id && (id >>16 ) == device_id) { +pcidevaddr_t dev; + +for (dev =0 ; dev <256 ; dev++) { +if (pci_config_readw (dev,PCI_VENDOR_ID ) == vendor_id && +pci_config_readw (dev,PCI_DEVICE_ID ) == device_id) {return dev; } } +return PCIDEVADDR_INVALID ; } -unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num) +static phys_addr_t pci_bar_mask (uint32_t bar) +{ + return (bar & PCI_BASE_ADDRESS_SPACE_IO) ?
lib/pci.c line 36
Quoted 10 lines
lib/pci.c +10
-0+ PCI_BASE_ADDRESS_IO_MASK :PCI_BASE_ADDRESS_MEM_MASK ; +} + +phys_addr_t pci_bar_addr (pcidevaddr_t dev,int bar_num) +{ +int off; +uint32_t bar; +uint64_t addr, mask; + + off =PCI_BASE_ADDRESS_0 + bar_num *4 ;
Looks like you also added now support for 64-bit bars below ... but is the above calculation still valid if there is a 64-bit bar inbetween? E.g. what is this function supposed to do if the first bar is a 64-bit bar? Is the caller supposed to use bar_num = 2 for the second bar, or bar_num = 1? ... maybe a comment in front of the function would be good here.
Quoted 42 lines
lib/pci.c +38
-2+ bar =pci_config_readl (dev, off); + mask =pci_bar_mask (bar); + +if (pci_bar_is64 (dev, bar_num)) { +uint32_t addr_high =pci_config_readl (dev, off +4 ); + addr = (((uint64_t )addr_high <<32 ) | bar) & mask; + }else { + addr = bar & mask; + } + +return pci_xlate_addr (dev, addr); +} + +/* +* 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. +* +* The following pci_bar_size32() and pci_bar_size() functions do +* the described algorithm. +*/ +static uint32_t pci_bar_size32 (pcidevaddr_t dev,int bar_num) +{ +int off =PCI_BASE_ADDRESS_0 + (bar_num *4 ); +uint32_t bar, size; + + bar =pci_config_readl (dev, off); +pci_config_writel (dev, off, ~0u ); + size =pci_config_readl (dev, off); +pci_config_writel (dev, off, bar); + +return size; +} + +phys_addr_t pci_bar_size (pcidevaddr_t dev,int bar_num) {uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); -if (bar &PCI_BASE_ADDRESS_SPACE_IO ) { -return bar &PCI_BASE_ADDRESS_IO_MASK ; +uint32_t size =pci_bar_size32 (dev, bar_num);
lib/pci.c line 85
Quoted 10 lines
lib/pci.c +6
-1+phys_addr_t mask =pci_bar_mask (bar); + +if (pci_bar_is64 (dev, bar_num)) { +uint32_t size_high =pci_bar_size32 (dev, bar_num +1 ); +return (~((((phys_addr_t )size_high <<32 ) | size) & mask)) +1 ; }else { -return bar &PCI_BASE_ADDRESS_MEM_MASK ; +return (~((phys_addr_t )size & mask)) +1 ; } }
I think you could slightly simplify that function, something like:
phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
{
uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
phys_addr_t size = pci_bar_size32(dev, bar_num);
phys_addr_t mask = pci_bar_mask(bar);
if (pci_bar_is64(dev, bar_num)) {
size |= ((phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32;
}
return (~((phys_addr_t)size & mask)) + 1;
}Quoted 6 lines
lib/pci.c +2
-0@@ -40,3 +95,16 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 );return bar; } + +bool pci_bar_is64 (pcidevaddr_t dev,int bar_num)
lib/pci.c bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num) · line 109
Quoted 10 lines
lib/pci.c +10
-0+{ +uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); + +if (bar &PCI_BASE_ADDRESS_SPACE_IO ) +return false; +else if ((bar &PCI_BASE_ADDRESS_MEM_TYPE_MASK ) == +PCI_BASE_ADDRESS_MEM_TYPE_64 ) +return true; +else +return false;
More simple:
else return ((bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64);
?
Quoted 6 lines
lib/pci.c +1
-0+}lib/pci.h +0
-0
lib/pci.h enum { · line 22
Quoted 9 lines
lib/pci.h +3
-1@@ -15,7 +15,9 @@ enum { PCIDEVADDR_INVALID = 0xffff, };pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id); -unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num); +phys_addr_t pci_bar_addr (pcidevaddr_t dev,int bar_num); +phys_addr_t pci_bar_size (pcidevaddr_t dev,int bar_num); +bool pci_bar_is64 (pcidevaddr_t dev,int bar_num);bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num);bool pci_bar_is_valid (pcidevaddr_t dev,int bar_num);
Thomas
Re: [PATCH RFC 08/15] pci: Rework pci_bar_addr()
Quoted 69 lines · Thomas Huth
Quoted 68 lines · Alexander Gordeev
This update makes
pci_bar_addr()interface 64 bit BARs aware and introduces a concept of PCI address translation. In cases PCI bus and CPU bus physical addresses are not indentical the architecutre should overridepci_xslate_addr()interface to provide necessary mapping between the two.Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/pci.h | 8 +++++ lib/pci.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++----- lib/pci.h | 4 ++- 3 files changed, 87 insertions(+), 9 deletions(-)lib/asm-generic/pci.h +8
-0@@ -14,4 +14,12 @@ static inline void pci_shutdown(void) }#endif +#ifndef pci_xslate_addr +static inline +phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) +{ +return addr; +} +#endif +#endif lib/pci.c +17
-5@@ -9,23 +9,78 @@/* Scan bus look for a specific device. Only bus 0 scanned for now. */ pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id) { -unsigned dev; -for (dev =0 ; dev <256 ; ++dev) { -uint32_t id =pci_config_readl (dev,0 ); -if ((id &0xFFFF ) == vendor_id && (id >>16 ) == device_id) { +pcidevaddr_t dev; + +for (dev =0 ; dev <256 ; dev++) { +if (pci_config_readw (dev,PCI_VENDOR_ID ) == vendor_id && +pci_config_readw (dev,PCI_DEVICE_ID ) == device_id) {return dev; } } +return PCIDEVADDR_INVALID ; } -unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num) +static phys_addr_t pci_bar_mask (uint32_t bar) +{ +return (bar &PCI_BASE_ADDRESS_SPACE_IO ) ? +PCI_BASE_ADDRESS_IO_MASK :PCI_BASE_ADDRESS_MEM_MASK ; +} + +phys_addr_t pci_bar_addr (pcidevaddr_t dev,int bar_num) +{ +int off; +uint32_t bar; +uint64_t addr, mask;
Quoted 8 lines
lib/pci.c line 36
Quoted 2 lines
lib/pci.c +2
-0+ + off =PCI_BASE_ADDRESS_0 + bar_num *4 ;Looks like you also added now support for 64-bit bars below ... but is the above calculation still valid if there is a 64-bit bar inbetween? E.g. what is this function supposed to do if the first bar is a 64-bit bar? Is the caller supposed to use
bar_num= 2 for the second bar, orbar_num= 1? ... maybe a comment in front of the function would be good here.
That is a good point. I tend not to complicate this function which would have report a failure in case of "inbetween" access and nailing "bar_num" to 32 bits. I do not like this idea much, but struggling with an alternate idea. Otherwise it should be something like this:
bool pci_bar_addr(pcidevaddr_t dev, int bar_num, phys_addr_t *addr);
But in early versions of this series Andrew did not like it ;) The code iterated through all 6 BARs decoding each one-by-one.
Quoted 56 lines
Quoted 42 lines
lib/pci.c +38
-2+ bar =pci_config_readl (dev, off); + mask =pci_bar_mask (bar); + +if (pci_bar_is64 (dev, bar_num)) { +uint32_t addr_high =pci_config_readl (dev, off +4 ); + addr = (((uint64_t )addr_high <<32 ) | bar) & mask; + }else { + addr = bar & mask; + } + +return pci_xlate_addr (dev, addr); +} + +/* +* 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. +* +* The following pci_bar_size32() and pci_bar_size() functions do +* the described algorithm. +*/ +static uint32_t pci_bar_size32 (pcidevaddr_t dev,int bar_num) +{ +int off =PCI_BASE_ADDRESS_0 + (bar_num *4 ); +uint32_t bar, size; + + bar =pci_config_readl (dev, off); +pci_config_writel (dev, off, ~0u ); + size =pci_config_readl (dev, off); +pci_config_writel (dev, off, bar); + +return size; +} + +phys_addr_t pci_bar_size (pcidevaddr_t dev,int bar_num) {uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); -if (bar &PCI_BASE_ADDRESS_SPACE_IO ) { -return bar &PCI_BASE_ADDRESS_IO_MASK ; +uint32_t size =pci_bar_size32 (dev, bar_num);lib/pci.c line 85
Quoted 10 lines
lib/pci.c +6
-1+phys_addr_t mask =pci_bar_mask (bar); + +if (pci_bar_is64 (dev, bar_num)) { +uint32_t size_high =pci_bar_size32 (dev, bar_num +1 ); +return (~((((phys_addr_t )size_high <<32 ) | size) & mask)) +1 ; }else { -return bar &PCI_BASE_ADDRESS_MEM_MASK ; +return (~((phys_addr_t )size & mask)) +1 ; } }I think you could slightly simplify that function, something like:
phys_addr_tpci_bar_size(pcidevaddr_t dev, int bar_num)
Quoted 11 lines
{
uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
phys_addr_t size = pci_bar_size32(dev, bar_num);
phys_addr_t mask = pci_bar_mask(bar);
if (pci_bar_is64(dev, bar_num)) {
size |= ((phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32;
}
return (~((phys_addr_t)size & mask)) + 1;
}Yep, looks better.
Quoted 19 lines
Quoted 6 lines
lib/pci.c +2
-0@@ -40,3 +95,16 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 );return bar; } + +bool pci_bar_is64 (pcidevaddr_t dev,int bar_num)lib/pci.c bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num) · line 109
Quoted 10 lines
lib/pci.c +10
-0+{ +uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); + +if (bar &PCI_BASE_ADDRESS_SPACE_IO ) +return false; +else if ((bar &PCI_BASE_ADDRESS_MEM_TYPE_MASK ) == +PCI_BASE_ADDRESS_MEM_TYPE_64 ) +return true; +else +return false;More simple:
Quoted 5 lines
else return ((bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64);?
I personally find it less readable, but can not explain it ;)
Quoted 19 lines
Quoted 6 lines
lib/pci.c +1
-0+}lib/pci.h +0
-0lib/pci.h enum { · line 22
Quoted 9 lines
lib/pci.h +3
-1@@ -15,7 +15,9 @@ enum { PCIDEVADDR_INVALID = 0xffff, };pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id); -unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num); +phys_addr_t pci_bar_addr (pcidevaddr_t dev,int bar_num); +phys_addr_t pci_bar_size (pcidevaddr_t dev,int bar_num); +bool pci_bar_is64 (pcidevaddr_t dev,int bar_num);bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num);bool pci_bar_is_valid (pcidevaddr_t dev,int bar_num);Thomas
Re: [PATCH RFC 08/15] pci: Rework pci_bar_addr()
Quoted 179 lines · Alexander Gordeev
Quoted 69 lines · Thomas Huth
Quoted 68 lines · Alexander Gordeev
This update makes
pci_bar_addr()interface 64 bit BARs aware and introduces a concept of PCI address translation. In cases PCI bus and CPU bus physical addresses are not indentical the architecutre should overridepci_xslate_addr()interface to provide necessary mapping between the two.Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/pci.h | 8 +++++ lib/pci.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++----- lib/pci.h | 4 ++- 3 files changed, 87 insertions(+), 9 deletions(-)lib/asm-generic/pci.h +8
-0@@ -14,4 +14,12 @@ static inline void pci_shutdown(void) }#endif +#ifndef pci_xslate_addr +static inline +phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) +{ +return addr; +} +#endif +#endif lib/pci.c +17
-5@@ -9,23 +9,78 @@/* Scan bus look for a specific device. Only bus 0 scanned for now. */ pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id) { -unsigned dev; -for (dev =0 ; dev <256 ; ++dev) { -uint32_t id =pci_config_readl (dev,0 ); -if ((id &0xFFFF ) == vendor_id && (id >>16 ) == device_id) { +pcidevaddr_t dev; + +for (dev =0 ; dev <256 ; dev++) { +if (pci_config_readw (dev,PCI_VENDOR_ID ) == vendor_id && +pci_config_readw (dev,PCI_DEVICE_ID ) == device_id) {return dev; } } +return PCIDEVADDR_INVALID ; } -unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num) +static phys_addr_t pci_bar_mask (uint32_t bar) +{ +return (bar &PCI_BASE_ADDRESS_SPACE_IO ) ? +PCI_BASE_ADDRESS_IO_MASK :PCI_BASE_ADDRESS_MEM_MASK ; +} + +phys_addr_t pci_bar_addr (pcidevaddr_t dev,int bar_num) +{ +int off; +uint32_t bar; +uint64_t addr, mask;Quoted 8 lines
lib/pci.c line 36
Quoted 2 lines
lib/pci.c +2
-0+ + off =PCI_BASE_ADDRESS_0 + bar_num *4 ;Looks like you also added now support for 64-bit bars below ... but is the above calculation still valid if there is a 64-bit bar inbetween? E.g. what is this function supposed to do if the first bar is a 64-bit bar? Is the caller supposed to use
bar_num= 2 for the second bar, orbar_num= 1? ... maybe a comment in front of the function would be good here.That is a good point. I tend not to complicate this function which would have report a failure in case of "inbetween" access and nailing "
bar_num" to 32 bits. I do not like this idea much, but struggling with an alternate idea. Otherwise it should be something like this:bool pci_bar_addr(pcidevaddr_t dev, int bar_num, phys_addr_t *addr);But in early versions of this series Andrew did not like it ;) The code iterated through all 6 BARs decoding each one-by-one.
Quoted 56 lines
Quoted 42 lines
lib/pci.c +38
-2+ bar =pci_config_readl (dev, off); + mask =pci_bar_mask (bar); + +if (pci_bar_is64 (dev, bar_num)) { +uint32_t addr_high =pci_config_readl (dev, off +4 ); + addr = (((uint64_t )addr_high <<32 ) | bar) & mask; + }else { + addr = bar & mask; + } + +return pci_xlate_addr (dev, addr); +} + +/* +* 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. +* +* The following pci_bar_size32() and pci_bar_size() functions do +* the described algorithm. +*/ +static uint32_t pci_bar_size32 (pcidevaddr_t dev,int bar_num) +{ +int off =PCI_BASE_ADDRESS_0 + (bar_num *4 ); +uint32_t bar, size; + + bar =pci_config_readl (dev, off); +pci_config_writel (dev, off, ~0u ); + size =pci_config_readl (dev, off); +pci_config_writel (dev, off, bar); + +return size; +} + +phys_addr_t pci_bar_size (pcidevaddr_t dev,int bar_num) {uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); -if (bar &PCI_BASE_ADDRESS_SPACE_IO ) { -return bar &PCI_BASE_ADDRESS_IO_MASK ; +uint32_t size =pci_bar_size32 (dev, bar_num);lib/pci.c line 85
Quoted 10 lines
lib/pci.c +6
-1+phys_addr_t mask =pci_bar_mask (bar); + +if (pci_bar_is64 (dev, bar_num)) { +uint32_t size_high =pci_bar_size32 (dev, bar_num +1 ); +return (~((((phys_addr_t )size_high <<32 ) | size) & mask)) +1 ; }else { -return bar &PCI_BASE_ADDRESS_MEM_MASK ; +return (~((phys_addr_t )size & mask)) +1 ; } }I think you could slightly simplify that function, something like:
phys_addr_tpci_bar_size(pcidevaddr_t dev, int bar_num)Quoted 11 lines
{ uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4); phys_addr_t size = pci_bar_size32(dev, bar_num); phys_addr_t mask = pci_bar_mask(bar); if (pci_bar_is64(dev, bar_num)) { size |= ((phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32; } return (~((phys_addr_t)size & mask)) + 1; }Yep, looks better.
Quoted 19 lines
Quoted 6 lines
lib/pci.c +2
-0@@ -40,3 +95,16 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 );return bar; } + +bool pci_bar_is64 (pcidevaddr_t dev,int bar_num)lib/pci.c bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num) · line 109
Quoted 10 lines
lib/pci.c +10
-0+{ +uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); + +if (bar &PCI_BASE_ADDRESS_SPACE_IO ) +return false; +else if ((bar &PCI_BASE_ADDRESS_MEM_TYPE_MASK ) == +PCI_BASE_ADDRESS_MEM_TYPE_64 ) +return true; +else +return false;More simple:
Quoted 7 lines
Quoted 5 lines
else return ((bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64);?
I personally find it less readable, but can not explain it ;)
I prefer Thomas' return the result of the condition. You can also drop the 'else' :-)
Quoted 24 lines
Quoted 10 lines
Quoted 10 lines
lib/pci.c +1
-0+}lib/pci.h +0
-0@@ -15,7 +15,9 @@ enum { PCIDEVADDR_INVALID = 0xffff, };pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id);Quoted 10 lines
lib/pci.h enum { · line 22
Quoted 6 lines
lib/pci.h +3
-1-unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num); +phys_addr_t pci_bar_addr (pcidevaddr_t dev,int bar_num); +phys_addr_t pci_bar_size (pcidevaddr_t dev,int bar_num); +bool pci_bar_is64 (pcidevaddr_t dev,int bar_num);bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num);bool pci_bar_is_valid (pcidevaddr_t dev,int bar_num);Thomas
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 08/15] pci: Rework pci_bar_addr()
In reply to Alexander GordeevQuoted 20 lines · Alexander Gordeev
This update makes
pci_bar_addr()interface 64 bit BARs aware and introduces a concept of PCI address translation. In cases PCI bus and CPU bus physical addresses are not indentical the architecutre should overridepci_xslate_addr()interface to provide necessary mapping between the two.Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/pci.h | 8 +++++ lib/pci.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++----- lib/pci.h | 4 ++- 3 files changed, 87 insertions(+), 9 deletions(-)lib/asm-generic/pci.h +0
-0
lib/asm-generic/pci.h static inline void pci_shutdown(void) · line 23
Quoted 10 lines
lib/asm-generic/pci.h +7
-0@@ -14,4 +14,12 @@ static inline void pci_shutdown(void) }#endif +#ifndef pci_xslate_addr +static inline +phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) +{ +return addr; +} +#endif
No need for this generic function. Like pci_config_read() is now, just leave it to the arch to implement it. Also please spell out translate, it's only 4 more characters and looks oh so much better :-)
Quoted 128 lines
lib/asm-generic/pci.h +1
-0+#endif lib/pci.c +76
-8@@ -9,23 +9,78 @@/* Scan bus look for a specific device. Only bus 0 scanned for now. */ pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id) { -unsigned dev; -for (dev =0 ; dev <256 ; ++dev) { -uint32_t id =pci_config_readl (dev,0 ); -if ((id &0xFFFF ) == vendor_id && (id >>16 ) == device_id) { +pcidevaddr_t dev; + +for (dev =0 ; dev <256 ; dev++) { +if (pci_config_readw (dev,PCI_VENDOR_ID ) == vendor_id && +pci_config_readw (dev,PCI_DEVICE_ID ) == device_id) {return dev; } } +return PCIDEVADDR_INVALID ; } -unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num) +static phys_addr_t pci_bar_mask (uint32_t bar) +{ +return (bar &PCI_BASE_ADDRESS_SPACE_IO ) ? +PCI_BASE_ADDRESS_IO_MASK :PCI_BASE_ADDRESS_MEM_MASK ; +} + +phys_addr_t pci_bar_addr (pcidevaddr_t dev,int bar_num) +{ +int off; +uint32_t bar; +uint64_t addr, mask; + + off =PCI_BASE_ADDRESS_0 + bar_num *4 ; + bar =pci_config_readl (dev, off); + mask =pci_bar_mask (bar); + +if (pci_bar_is64 (dev, bar_num)) { +uint32_t addr_high =pci_config_readl (dev, off +4 ); + addr = (((uint64_t )addr_high <<32 ) | bar) & mask; + }else { + addr = bar & mask; + } + +return pci_xlate_addr (dev, addr); +} + +/* +* 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. +* +* The following pci_bar_size32() and pci_bar_size() functions do +* the described algorithm. +*/ +static uint32_t pci_bar_size32 (pcidevaddr_t dev,int bar_num) +{ +int off =PCI_BASE_ADDRESS_0 + (bar_num *4 ); +uint32_t bar, size; + + bar =pci_config_readl (dev, off); +pci_config_writel (dev, off, ~0u ); + size =pci_config_readl (dev, off); +pci_config_writel (dev, off, bar); + +return size; +} + +phys_addr_t pci_bar_size (pcidevaddr_t dev,int bar_num) {uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); -if (bar &PCI_BASE_ADDRESS_SPACE_IO ) { -return bar &PCI_BASE_ADDRESS_IO_MASK ; +uint32_t size =pci_bar_size32 (dev, bar_num); +phys_addr_t mask =pci_bar_mask (bar); + +if (pci_bar_is64 (dev, bar_num)) { +uint32_t size_high =pci_bar_size32 (dev, bar_num +1 ); +return (~((((phys_addr_t )size_high <<32 ) | size) & mask)) +1 ; }else { -return bar &PCI_BASE_ADDRESS_MEM_MASK ; +return (~((phys_addr_t )size & mask)) +1 ; } }@@ -40,3 +95,16 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 );return bar; } + +bool pci_bar_is64 (pcidevaddr_t dev,int bar_num) +{ +uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); + +if (bar &PCI_BASE_ADDRESS_SPACE_IO ) +return false; +else if ((bar &PCI_BASE_ADDRESS_MEM_TYPE_MASK ) == +PCI_BASE_ADDRESS_MEM_TYPE_64 ) +return true; +else +return false; +}lib/pci.h +3
-1@@ -15,7 +15,9 @@ enum { PCIDEVADDR_INVALID = 0xffff, };pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id); -unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num); +phys_addr_t pci_bar_addr (pcidevaddr_t dev,int bar_num); +phys_addr_t pci_bar_size (pcidevaddr_t dev,int bar_num); +bool pci_bar_is64 (pcidevaddr_t dev,int bar_num);bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num);bool pci_bar_is_valid (pcidevaddr_t dev,int bar_num);Signature · 1 line
1.8.3.1
thanks,
drew
Re: [PATCH RFC 08/15] pci: Rework pci_bar_addr()
In reply to Alexander GordeevQuoted 43 lines · Alexander Gordeev
This update makes
pci_bar_addr()interface 64 bit BARs aware and introduces a concept of PCI address translation. In cases PCI bus and CPU bus physical addresses are not indentical the architecutre should overridepci_xslate_addr()interface to provide necessary mapping between the two.Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/pci.h | 8 +++++ lib/pci.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++----- lib/pci.h | 4 ++- 3 files changed, 87 insertions(+), 9 deletions(-)lib/asm-generic/pci.h +8
-0@@ -14,4 +14,12 @@ static inline void pci_shutdown(void) }#endif +#ifndef pci_xslate_addr +static inline +phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) +{ +return addr; +} +#endif +#endif lib/pci.c +0
-3@@ -9,23 +9,78 @@/* Scan bus look for a specific device. Only bus 0 scanned for now. */ pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id) { -unsigned dev; -for (dev =0 ; dev <256 ; ++dev) { -uint32_t id =pci_config_readl (dev,0 );
lib/pci.c line 20
Quoted 10 lines
lib/pci.c +6
-1-if ((id &0xFFFF ) == vendor_id && (id >>16 ) == device_id) { +pcidevaddr_t dev; + +for (dev =0 ; dev <256 ; dev++) { +if (pci_config_readw (dev,PCI_VENDOR_ID ) == vendor_id && +pci_config_readw (dev,PCI_DEVICE_ID ) == device_id) {return dev; } } +
One other note. This patch is also mixing in cleanups. Please do all cleanups in separate patches first, then apply your changes onto the clean code.
Thanks,
drew
Quoted 109 lines
lib/pci.c +70
-4return PCIDEVADDR_INVALID ; } -unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num) +static phys_addr_t pci_bar_mask (uint32_t bar) +{ +return (bar &PCI_BASE_ADDRESS_SPACE_IO ) ? +PCI_BASE_ADDRESS_IO_MASK :PCI_BASE_ADDRESS_MEM_MASK ; +} + +phys_addr_t pci_bar_addr (pcidevaddr_t dev,int bar_num) +{ +int off; +uint32_t bar; +uint64_t addr, mask; + + off =PCI_BASE_ADDRESS_0 + bar_num *4 ; + bar =pci_config_readl (dev, off); + mask =pci_bar_mask (bar); + +if (pci_bar_is64 (dev, bar_num)) { +uint32_t addr_high =pci_config_readl (dev, off +4 ); + addr = (((uint64_t )addr_high <<32 ) | bar) & mask; + }else { + addr = bar & mask; + } + +return pci_xlate_addr (dev, addr); +} + +/* +* 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. +* +* The following pci_bar_size32() and pci_bar_size() functions do +* the described algorithm. +*/ +static uint32_t pci_bar_size32 (pcidevaddr_t dev,int bar_num) +{ +int off =PCI_BASE_ADDRESS_0 + (bar_num *4 ); +uint32_t bar, size; + + bar =pci_config_readl (dev, off); +pci_config_writel (dev, off, ~0u ); + size =pci_config_readl (dev, off); +pci_config_writel (dev, off, bar); + +return size; +} + +phys_addr_t pci_bar_size (pcidevaddr_t dev,int bar_num) {uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); -if (bar &PCI_BASE_ADDRESS_SPACE_IO ) { -return bar &PCI_BASE_ADDRESS_IO_MASK ; +uint32_t size =pci_bar_size32 (dev, bar_num); +phys_addr_t mask =pci_bar_mask (bar); + +if (pci_bar_is64 (dev, bar_num)) { +uint32_t size_high =pci_bar_size32 (dev, bar_num +1 ); +return (~((((phys_addr_t )size_high <<32 ) | size) & mask)) +1 ; }else { -return bar &PCI_BASE_ADDRESS_MEM_MASK ; +return (~((phys_addr_t )size & mask)) +1 ; } }@@ -40,3 +95,16 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 );return bar; } + +bool pci_bar_is64 (pcidevaddr_t dev,int bar_num) +{ +uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 ); + +if (bar &PCI_BASE_ADDRESS_SPACE_IO ) +return false; +else if ((bar &PCI_BASE_ADDRESS_MEM_TYPE_MASK ) == +PCI_BASE_ADDRESS_MEM_TYPE_64 ) +return true; +else +return false; +}lib/pci.h +3
-1@@ -15,7 +15,9 @@ enum { PCIDEVADDR_INVALID = 0xffff, };pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id); -unsigned long pci_bar_addr (pcidevaddr_t dev,int bar_num); +phys_addr_t pci_bar_addr (pcidevaddr_t dev,int bar_num); +phys_addr_t pci_bar_size (pcidevaddr_t dev,int bar_num); +bool pci_bar_is64 (pcidevaddr_t dev,int bar_num);bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num);bool pci_bar_is_valid (pcidevaddr_t dev,int bar_num);Signature · 6 lines
1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC PATCH 9/15 pci: Add pci_bar_set()
In reply to Alexander Gordeev--- lib/pci.c | 8 ++++++++ lib/pci.h | 1 + 2 files changed, 9 insertions(+)
lib/pci.c +8 -0
@@ -84,6 +84,14 @@ phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num) } } +void pci_bar_set (pcidevaddr_t dev,int bar_num,uint64_t addr) +{ +int off =PCI_BASE_ADDRESS_0 + bar_num *4 ; +pci_config_writel (dev, off, (uint32_t )addr); +if (pci_bar_is64 (dev, bar_num)) +pci_config_writel (dev, (uint32_t )(addr >>32 ), off +4 ); +} +bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num) {uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 );
lib/pci.h +1 -0
@@ -15,6 +15,7 @@ enum { PCIDEVADDR_INVALID = 0xffff, };pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id); +void pci_bar_set (pcidevaddr_t dev,int bar_num,uint64_t addr);phys_addr_t pci_bar_addr (pcidevaddr_t dev,int bar_num);phys_addr_t pci_bar_size (pcidevaddr_t dev,int bar_num);bool pci_bar_is64 (pcidevaddr_t dev,int bar_num);
Signature · 1 line
1.8.3.1
Re: [PATCH RFC 09/15] pci: Add pci_bar_set()
Quoted 33 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci.c | 8 ++++++++ lib/pci.h | 1 + 2 files changed, 9 insertions(+)lib/pci.c +8
-0@@ -84,6 +84,14 @@ phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num) } } +void pci_bar_set (pcidevaddr_t dev,int bar_num,uint64_t addr) +{ +int off =PCI_BASE_ADDRESS_0 + bar_num *4 ; +pci_config_writel (dev, off, (uint32_t )addr); +if (pci_bar_is64 (dev, bar_num)) +pci_config_writel (dev, (uint32_t )(addr >>32 ), off +4 ); +} +bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num) {uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 );lib/pci.h +0
-0
lib/pci.h enum { · line 21
Quoted 7 lines
lib/pci.h +1
-0@@ -15,6 +15,7 @@ enum { PCIDEVADDR_INVALID = 0xffff, };pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id); +void pci_bar_set (pcidevaddr_t dev,int bar_num,uint64_t addr);phys_addr_t pci_bar_addr (pcidevaddr_t dev,int bar_num);phys_addr_t pci_bar_size (pcidevaddr_t dev,int bar_num);bool pci_bar_is64 (pcidevaddr_t dev,int bar_num);
Re: [PATCH RFC 09/15] pci: Add pci_bar_set()
In reply to Alexander GordeevQuoted 13 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci.c | 8 ++++++++ lib/pci.h | 1 + 2 files changed, 9 insertions(+)lib/pci.c +0
-0
lib/pci.c phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num) · line 89
Quoted 6 lines
lib/pci.c +3
-0@@ -84,6 +84,14 @@ phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num) } } +void pci_bar_set (pcidevaddr_t dev,int bar_num,uint64_t addr) +{ +int off =PCI_BASE_ADDRESS_0 + bar_num *4 ;
Blank line here please. Please run the kernel's checkpatch script on all your patches. We strive to follow the kernel coding style. (Well, don't look at the legacy code :-)
lib/pci.c phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num) · line 92
Quoted 3 lines
lib/pci.c +3
-0+pci_config_writel (dev, off, (uint32_t )addr); +if (pci_bar_is64 (dev, bar_num)) +pci_config_writel (dev, (uint32_t )(addr >>32 ), off +4 );
This pci_config_writel() call is using offset as the 3rd parameter, but it should be the 2nd.
Also, where do you introduce pci_config_writel()? I haven't closely reviewed all the previous patches, but so far I only see uses of it, no introduction of it. Does this series compile one patch at a time? We should ensure it does to maintain bisectability.
Thanks,
drew
Quoted 24 lines
lib/pci.c +2
-0+} +bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num) {uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 );lib/pci.h +1
-0@@ -15,6 +15,7 @@ enum { PCIDEVADDR_INVALID = 0xffff, };pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id); +void pci_bar_set (pcidevaddr_t dev,int bar_num,uint64_t addr);phys_addr_t pci_bar_addr (pcidevaddr_t dev,int bar_num);phys_addr_t pci_bar_size (pcidevaddr_t dev,int bar_num);bool pci_bar_is64 (pcidevaddr_t dev,int bar_num);Signature · 6 lines
1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 09/15] pci: Add pci_bar_set()
Quoted 25 lines · Andrew Jones
Quoted 13 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci.c | 8 ++++++++ lib/pci.h | 1 + 2 files changed, 9 insertions(+)lib/pci.c +0
-0lib/pci.c phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num) · line 89
Quoted 6 lines
lib/pci.c +3
-0@@ -84,6 +84,14 @@ phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num) } } +void pci_bar_set (pcidevaddr_t dev,int bar_num,uint64_t addr) +{ +int off =PCI_BASE_ADDRESS_0 + bar_num *4 ;Blank line here please. Please run the kernel's checkpatch script on all your patches. We strive to follow the kernel coding style. (Well, don't look at the legacy code :-)
Quoted 6 lines
lib/pci.c phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num) · line 92
Quoted 3 lines
lib/pci.c +3
-0+pci_config_writel (dev, off, (uint32_t )addr); +if (pci_bar_is64 (dev, bar_num)) +pci_config_writel (dev, (uint32_t )(addr >>32 ), off +4 );This
pci_config_writel()call is using offset as the 3rd parameter, but it should be the 2nd.
Bummer.
Quoted 4 lines
Also, where do you introduce
pci_config_writel()? I haven't closely reviewed all the previous patches, but so far I only see uses of it, no introduction of it. Does this series compile one patch at a time? We should ensure it does to maintain bisectability.
06/15 "pci/x86: Add remaining PCI configuration space accessors"
pci_config_read*/write* accessors are not expected implemented on archs that do not include lib/pci.c. Once lib/pci.c added to an arch accessors must be introduced to lib/asm/pci.h as well.
Also, no generic implementation introduced as there is no such thing as generic PCI access.
If it makes sense?
Quoted 27 lines
Thanks,
drewQuoted 24 lines
lib/pci.c +2
-0+} +bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num) {uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 );lib/pci.h +1
-0@@ -15,6 +15,7 @@ enum { PCIDEVADDR_INVALID = 0xffff, };pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id); +void pci_bar_set (pcidevaddr_t dev,int bar_num,uint64_t addr);phys_addr_t pci_bar_addr (pcidevaddr_t dev,int bar_num);phys_addr_t pci_bar_size (pcidevaddr_t dev,int bar_num);bool pci_bar_is64 (pcidevaddr_t dev,int bar_num);Signature · 6 lines
1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 09/15] pci: Add pci_bar_set()
Quoted 42 lines · Alexander Gordeev
Quoted 25 lines · Andrew Jones
Quoted 13 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci.c | 8 ++++++++ lib/pci.h | 1 + 2 files changed, 9 insertions(+)lib/pci.c +0
-0lib/pci.c phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num) · line 89
Quoted 6 lines
lib/pci.c +3
-0@@ -84,6 +84,14 @@ phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num) } } +void pci_bar_set (pcidevaddr_t dev,int bar_num,uint64_t addr) +{ +int off =PCI_BASE_ADDRESS_0 + bar_num *4 ;Blank line here please. Please run the kernel's checkpatch script on all your patches. We strive to follow the kernel coding style. (Well, don't look at the legacy code :-)
Quoted 6 lines
lib/pci.c phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num) · line 92
Quoted 3 lines
lib/pci.c +3
-0+pci_config_writel (dev, off, (uint32_t )addr); +if (pci_bar_is64 (dev, bar_num)) +pci_config_writel (dev, (uint32_t )(addr >>32 ), off +4 );This
pci_config_writel()call is using offset as the 3rd parameter, but it should be the 2nd.Bummer.
Quoted 4 lines
Also, where do you introduce
pci_config_writel()? I haven't closely reviewed all the previous patches, but so far I only see uses of it, no introduction of it. Does this series compile one patch at a time? We should ensure it does to maintain bisectability.06/15 "pci/x86: Add remaining PCI configuration space accessors"
Quoted 8 lines
pci_config_read*/write* accessors are not expected implemented on archs that do not include
lib/pci.c. Oncelib/pci.cadded to an arch accessors must be introduced tolib/asm/pci.has well.Also, no generic implementation introduced as there is no such thing as generic PCI access.
If it makes sense?
It does. And now I see why I didn't see it. You didn't CC me on 06/15, so that message didn't land in my "to review" virtual folder, and I didn't notice that the series jumped from 5 to 7 when I was looking it over.
drew
Quoted 32 lines
Quoted 24 lines
Thanks,
drewQuoted 20 lines
lib/pci.c +2
-0+} +bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num) {uint32_t bar =pci_config_readl (dev,PCI_BASE_ADDRESS_0 + bar_num *4 );lib/pci.h +1
-0@@ -15,6 +15,7 @@ enum { PCIDEVADDR_INVALID = 0xffff, };pcidevaddr_t pci_find_dev (uint16_t vendor_id,uint16_t device_id); +void pci_bar_set (pcidevaddr_t dev,int bar_num,uint64_t addr);phys_addr_t pci_bar_addr (pcidevaddr_t dev,int bar_num);phys_addr_t pci_bar_size (pcidevaddr_t dev,int bar_num);bool pci_bar_is64 (pcidevaddr_t dev,int bar_num);Signature · 1 line
1.8.3.1Quoted 4 lines
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC PATCH 10/15 pci: Add pci_print() and pci_type_desc()
In reply to Alexander Gordeev--- lib/asm-generic/pci.h | 6 +++++ lib/pci.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci.h | 3 +++ 3 files changed, 76 insertions(+)
lib/asm-generic/pci.h +6 -0
@@ -22,4 +22,10 @@ phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) }#endif +#ifndef pci_print_arch +static inline void pci_print_arch (void ) +{ +} +#endif +#endif
lib/pci.c +67 -0
70 lines not on this page · raw message
lib/pci.h +3 -0
@@ -22,6 +22,9 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num);bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num);bool pci_bar_is_valid (pcidevaddr_t dev,int bar_num); +void pci_type_desc (int type,char *desc,int len); +void pci_print (void ); +/* * pci-testdev is a driver for the pci-testdev qemu pci device. The * device enables testing mmio and portio exits, and measuring their
Signature · 1 line
1.8.3.1
Re: [PATCH RFC 10/15] pci: Add pci_print() and pci_type_desc()
Quoted 29 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/pci.h | 6 +++++ lib/pci.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci.h | 3 +++ 3 files changed, 76 insertions(+)lib/asm-generic/pci.h +6
-0@@ -22,4 +22,10 @@ phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) }#endif +#ifndef pci_print_arch +static inline void pci_print_arch (void ) +{ +} +#endif +#endif lib/pci.c +0
-0
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 123
Quoted 8 lines
lib/pci.c +5
-0@@ -116,3 +116,70 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num)else return false; } + +void pci_type_desc (int type,char *desc,int len) +{ +if ((type &PCI_BASE_ADDRESS_SPACE ) ==PCI_BASE_ADDRESS_SPACE_IO ) { +strcpy (desc,"PIO" );/* strncpy() would be better */
Maybe that's a good point in time now to introduce strncpy to kvm-unit-tests? (or maybe even a better variant like strlcpy?)
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 125
Quoted 2 lines
lib/pci.c +2
-0+ }else { +static char *str[] = {"32" ,"1M" ,"64" };
Since you're depending on external values as array index below, I'd play safe here and add a fourth entry like "??" or "RES" or so.
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 136
Quoted 11 lines
lib/pci.c +11
-0+int idx = (type &PCI_BASE_ADDRESS_MEM_TYPE_MASK ) >>1 ; +int pfetch = type &PCI_BASE_ADDRESS_MEM_PREFETCH ; +snprintf (desc, len,"MEM%s%s" , str[idx], pfetch ?"/p" :"" ); + } +} + +static void pci_dev_print (pcidevaddr_t dev) +{ +uint16_t vendor_id =pci_config_readw (dev,PCI_VENDOR_ID ); +uint16_t device_id =pci_config_readw (dev,PCI_DEVICE_ID ); +uint8_t header =pci_config_readb (dev,PCI_HEADER_TYPE );
I think you should mask away the uppermost bit of the header type byte - otherwise your code does not work with multi-function devices.
Quoted 25 lines
lib/pci.c +25
-0+uint8_t progif =pci_config_readb (dev,PCI_CLASS_PROG ); +uint8_t subclass =pci_config_readb (dev,PCI_CLASS_DEVICE ); +uint8_t class =pci_config_readb (dev,PCI_CLASS_DEVICE +1 ); +int bar; + +printf ("dev %2d fn %d vendor_id %04x device_id %04x type %02x " +"progif %02x class %02x subclass %02x\n" , + dev /8 , dev %8 , vendor_id, device_id, header, + progif, class, subclass); + +if (header !=PCI_HEADER_TYPE_NORMAL ) +return ; + +for (bar =0 ; bar <6 ; bar++) { +phys_addr_t start, end; +char desc[8 ]; + +if (!pci_bar_is_valid (dev, bar)) +break ; + + start =pci_bar_addr (dev, bar); + end = start +pci_bar_size (dev, bar) -1 ; + +pci_type_desc (bar, desc,sizeof (desc)); +
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 171
Quoted 10 lines
lib/pci.c +10
-0+if (pci_bar_is64 (dev, bar)) { +printf ("\tBAR#%d,%d [%-7s %" PRIx64"-%" PRIx64"]\n" , + bar, bar +1 , desc, start, end); + bar++; + }else { +printf ("\tBAR#%d [%-7s %02x-%02x]\n" , + bar, desc, (uint32_t )start, (uint32_t )end); + } + } +}
Thomas
Re: [PATCH RFC 10/15] pci: Add pci_print() and pci_type_desc()
Quoted 33 lines · Thomas Huth
Quoted 32 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/pci.h | 6 +++++ lib/pci.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci.h | 3 +++ 3 files changed, 76 insertions(+)lib/asm-generic/pci.h +6
-0@@ -22,4 +22,10 @@ phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) }#endif +#ifndef pci_print_arch +static inline void pci_print_arch (void ) +{ +} +#endif +#endif lib/pci.c +0
-0@@ -116,3 +116,70 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num)else return false; }
Quoted 8 lines
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 123
Quoted 5 lines
lib/pci.c +5
-0+ +void pci_type_desc (int type,char *desc,int len) +{ +if ((type &PCI_BASE_ADDRESS_SPACE ) ==PCI_BASE_ADDRESS_SPACE_IO ) { +strcpy (desc,"PIO" );/* strncpy() would be better */ Maybe that's a good point in time now to introduce strncpy to kvm-unit-tests? (or maybe even a better variant like strlcpy?)
Andrew?
Quoted 5 lines
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 125
Quoted 2 lines
lib/pci.c +2
-0+ }else { +static char *str[] = {"32" ,"1M" ,"64" };Since you're depending on external values as array index below, I'd play safe here and add a fourth entry like "??" or "RES" or so.
As I got from previous Andrew's explanations we would rather crash in case of incorrect access.
Quoted 6 lines
Quoted 6 lines
lib/pci.c +6
-0+int idx = (type &PCI_BASE_ADDRESS_MEM_TYPE_MASK ) >>1 ; +int pfetch = type &PCI_BASE_ADDRESS_MEM_PREFETCH ; +snprintf (desc, len,"MEM%s%s" , str[idx], pfetch ?"/p" :"" ); + } +} +
Quoted 8 lines
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 136
Quoted 5 lines
lib/pci.c +5
-0+static void pci_dev_print (pcidevaddr_t dev) +{ +uint16_t vendor_id =pci_config_readw (dev,PCI_VENDOR_ID ); +uint16_t device_id =pci_config_readw (dev,PCI_DEVICE_ID ); +uint8_t header =pci_config_readb (dev,PCI_HEADER_TYPE );I think you should mask away the uppermost bit of the header type byte - otherwise your code does not work with multi-function devices.
See below.
Quoted 4 lines
Quoted 4 lines
lib/pci.c +4
-0+uint8_t progif =pci_config_readb (dev,PCI_CLASS_PROG ); +uint8_t subclass =pci_config_readb (dev,PCI_CLASS_DEVICE ); +uint8_t class =pci_config_readb (dev,PCI_CLASS_DEVICE +1 ); +int bar;
Quoted 8 lines
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 148
Quoted 8 lines
lib/pci.c +8
-0+ +printf ("dev %2d fn %d vendor_id %04x device_id %04x type %02x " +"progif %02x class %02x subclass %02x\n" , + dev /8 , dev %8 , vendor_id, device_id, header, + progif, class, subclass); + +if (header !=PCI_HEADER_TYPE_NORMAL ) +return ;
if ((header & 0x7f) != PCI_HEADER_TYPE_NORMAL) /* mask multi-function bit */ return;
I would rather masked it here.
?
Quoted 26 lines
Quoted 13 lines
lib/pci.c +13
-0+ +for (bar =0 ; bar <6 ; bar++) { +phys_addr_t start, end; +char desc[8 ]; + +if (!pci_bar_is_valid (dev, bar)) +break ; + + start =pci_bar_addr (dev, bar); + end = start +pci_bar_size (dev, bar) -1 ; + +pci_type_desc (bar, desc,sizeof (desc)); +lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 171
Quoted 10 lines
lib/pci.c +10
-0+if (pci_bar_is64 (dev, bar)) { +printf ("\tBAR#%d,%d [%-7s %" PRIx64"-%" PRIx64"]\n" , + bar, bar +1 , desc, start, end); + bar++; + }else { +printf ("\tBAR#%d [%-7s %02x-%02x]\n" , + bar, desc, (uint32_t )start, (uint32_t )end); + } + } +}Thomas
Re: [PATCH RFC 10/15] pci: Add pci_print() and pci_type_desc()
Quoted 45 lines · Alexander Gordeev
Quoted 33 lines · Thomas Huth
Quoted 32 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/pci.h | 6 +++++ lib/pci.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci.h | 3 +++ 3 files changed, 76 insertions(+)lib/asm-generic/pci.h +6
-0@@ -22,4 +22,10 @@ phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) }#endif +#ifndef pci_print_arch +static inline void pci_print_arch (void ) +{ +} +#endif +#endif lib/pci.c +0
-0@@ -116,3 +116,70 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num)else return false; }Quoted 8 lines
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 123
Quoted 5 lines
lib/pci.c +5
-0+ +void pci_type_desc (int type,char *desc,int len) +{ +if ((type &PCI_BASE_ADDRESS_SPACE ) ==PCI_BASE_ADDRESS_SPACE_IO ) { +strcpy (desc,"PIO" );/* strncpy() would be better */ Maybe that's a good point in time now to introduce strncpy to kvm-unit-tests? (or maybe even a better variant like strlcpy?)
Andrew?
Quoted 8 lines
Quoted 5 lines
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 125
Quoted 2 lines
lib/pci.c +2
-0+ }else { +static char *str[] = {"32" ,"1M" ,"64" };Since you're depending on external values as array index below, I'd play safe here and add a fourth entry like "??" or "RES" or so.
As I got from previous Andrew's explanations we would rather crash in case of incorrect access.
Well, but then please rather "crash" with an assert statement or something similar. Otherwise, it depends on a random value that resides there in memory. And at least on powerpc, we do not use the MMU (yet), so this would likely end up in a very random, unreadable string, instead of an immediate crash.
Quoted 33 lines
Quoted 6 lines
Quoted 6 lines
lib/pci.c +6
-0+int idx = (type &PCI_BASE_ADDRESS_MEM_TYPE_MASK ) >>1 ; +int pfetch = type &PCI_BASE_ADDRESS_MEM_PREFETCH ; +snprintf (desc, len,"MEM%s%s" , str[idx], pfetch ?"/p" :"" ); + } +} +Quoted 8 lines
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 136
Quoted 5 lines
lib/pci.c +5
-0+static void pci_dev_print (pcidevaddr_t dev) +{ +uint16_t vendor_id =pci_config_readw (dev,PCI_VENDOR_ID ); +uint16_t device_id =pci_config_readw (dev,PCI_DEVICE_ID ); +uint8_t header =pci_config_readb (dev,PCI_HEADER_TYPE );I think you should mask away the uppermost bit of the header type byte - otherwise your code does not work with multi-function devices.
See below.
Quoted 4 lines
Quoted 4 lines
lib/pci.c +4
-0+uint8_t progif =pci_config_readb (dev,PCI_CLASS_PROG ); +uint8_t subclass =pci_config_readb (dev,PCI_CLASS_DEVICE ); +uint8_t class =pci_config_readb (dev,PCI_CLASS_DEVICE +1 ); +int bar;Quoted 8 lines
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 148
Quoted 8 lines
lib/pci.c +8
-0+ +printf ("dev %2d fn %d vendor_id %04x device_id %04x type %02x " +"progif %02x class %02x subclass %02x\n" , + dev /8 , dev %8 , vendor_id, device_id, header, + progif, class, subclass); + +if (header !=PCI_HEADER_TYPE_NORMAL ) +return ;if ((header & 0x7f) != PCI_HEADER_TYPE_NORMAL) /* mask multi-function bit */ return;
Quoted 2 lines
I would rather masked it here.
?
That's fine for me, too.
Thomas
Re: [PATCH RFC 10/15] pci: Add pci_print() and pci_type_desc()
Quoted 44 lines · Thomas Huth
Quoted 43 lines · Alexander Gordeev
Quoted 41 lines · Thomas Huth
Quoted 29 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/pci.h | 6 +++++ lib/pci.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci.h | 3 +++ 3 files changed, 76 insertions(+)lib/asm-generic/pci.h +6
-0@@ -22,4 +22,10 @@ phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) }#endif +#ifndef pci_print_arch +static inline void pci_print_arch (void ) +{ +} +#endif +#endif lib/pci.c +0
-0lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 123
Quoted 8 lines
lib/pci.c +5
-0@@ -116,3 +116,70 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num)else return false; } + +void pci_type_desc (int type,char *desc,int len) +{ +if ((type &PCI_BASE_ADDRESS_SPACE ) ==PCI_BASE_ADDRESS_SPACE_IO ) { +strcpy (desc,"PIO" );/* strncpy() would be better */ Maybe that's a good point in time now to introduce strncpy to kvm-unit-tests? (or maybe even a better variant like strlcpy?)
Quoted 1 line
Quoted 1 line
Andrew?
Fine by me, if needed. Typically we've been pretty safe using strcpy in kvm-unit-tests, particularly when combined with some input sanity test asserting. What's the concern about desc here? Would it make more sense to check that concern with an assert?
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 125
Quoted 3 lines
lib/pci.c +2
-0+ }else { +static char *str[] = {"32" ,"1M" ,"64" };Since you're depending on external values as array index below, I'd play safe here and add a fourth entry like "??" or "RES" or so.
Quoted 8 lines
Quoted 2 lines
As I got from previous Andrew's explanations we would rather crash in case of incorrect access.
Well, but then please rather "crash" with an assert statement or something similar. Otherwise, it depends on a random value that resides there in memory. And at least on powerpc, we do not use the MMU (yet), so this would likely end up in a very random, unreadable string, instead of an immediate crash.
Yes. I prefer asserting for any unexpected lib api use, i.e. we should sanity check with asserts all external inputs, after which we don't need any error paths for those cases. But using asserts is necessary, we can't just assume going off in the weeds will lead to an understandable "crash".
Thanks,
drew
Quoted 45 lines
Quoted 34 lines
Quoted 15 lines
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 136
Quoted 12 lines
lib/pci.c +11
-0+int idx = (type &PCI_BASE_ADDRESS_MEM_TYPE_MASK ) >>1 ; +int pfetch = type &PCI_BASE_ADDRESS_MEM_PREFETCH ; +snprintf (desc, len,"MEM%s%s" , str[idx], pfetch ?"/p" :"" ); + } +} + +static void pci_dev_print (pcidevaddr_t dev) +{ +uint16_t vendor_id =pci_config_readw (dev,PCI_VENDOR_ID ); +uint16_t device_id =pci_config_readw (dev,PCI_DEVICE_ID ); +uint8_t header =pci_config_readb (dev,PCI_HEADER_TYPE );I think you should mask away the uppermost bit of the header type byte - otherwise your code does not work with multi-function devices.
See below.
Quoted 4 lines
Quoted 4 lines
lib/pci.c +4
-0+uint8_t progif =pci_config_readb (dev,PCI_CLASS_PROG ); +uint8_t subclass =pci_config_readb (dev,PCI_CLASS_DEVICE ); +uint8_t class =pci_config_readb (dev,PCI_CLASS_DEVICE +1 ); +int bar;Quoted 8 lines
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 148
Quoted 8 lines
lib/pci.c +8
-0+ +printf ("dev %2d fn %d vendor_id %04x device_id %04x type %02x " +"progif %02x class %02x subclass %02x\n" , + dev /8 , dev %8 , vendor_id, device_id, header, + progif, class, subclass); + +if (header !=PCI_HEADER_TYPE_NORMAL ) +return ;if ((header & 0x7f) != PCI_HEADER_TYPE_NORMAL) /* mask multi-function bit */ return;Quoted 2 lines
I would rather masked it here.
?That's fine for me, too.
Thomas
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 10/15] pci: Add pci_print() and pci_type_desc()
In reply to Alexander GordeevQuoted 14 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/pci.h | 6 +++++ lib/pci.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci.h | 3 +++ 3 files changed, 76 insertions(+)lib/asm-generic/pci.h +0
-0
lib/asm-generic/pci.h phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) · line 29
Quoted 8 lines
lib/asm-generic/pci.h +5
-0@@ -22,4 +22,10 @@ phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) }#endif +#ifndef pci_print_arch +static inline void pci_print_arch (void ) +{ +} +#endif
What's this function for? I think pci_print() should be enough. Or is this for the host bridge? If so, then there should be a non-arch specific pci_host_bridge_print() call written that does everything it can with arch-neutral code, and just use arch calls as needed.
Quoted 12 lines
lib/asm-generic/pci.h +1
-0+#endif lib/pci.c +2
-0@@ -116,3 +116,70 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num)else return false; } + +void pci_type_desc (int type,char *desc,int len)
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 130
Quoted 10 lines
lib/pci.c +10
-0+{ +if ((type &PCI_BASE_ADDRESS_SPACE ) ==PCI_BASE_ADDRESS_SPACE_IO ) { +strcpy (desc,"PIO" );/* strncpy() would be better */ + }else { +static char *str[] = {"32" ,"1M" ,"64" }; +int idx = (type &PCI_BASE_ADDRESS_MEM_TYPE_MASK ) >>1 ; +int pfetch = type &PCI_BASE_ADDRESS_MEM_PREFETCH ; +snprintf (desc, len,"MEM%s%s" , str[idx], pfetch ?"/p" :"" ); + } +}
What else, besides pci_dev_print, would call this function? If only one function needs to output these descs, then we don't need the strcpy, snprintf stuff, we just need printf at the call site.
Oh, I see. It's because desc shows up in two printfs below. Well, I'd still just repeat the printfs, or maybe create a macro.
Quoted 41 lines
lib/pci.c +41
-0+ +static void pci_dev_print (pcidevaddr_t dev) +{ +uint16_t vendor_id =pci_config_readw (dev,PCI_VENDOR_ID ); +uint16_t device_id =pci_config_readw (dev,PCI_DEVICE_ID ); +uint8_t header =pci_config_readb (dev,PCI_HEADER_TYPE ); +uint8_t progif =pci_config_readb (dev,PCI_CLASS_PROG ); +uint8_t subclass =pci_config_readb (dev,PCI_CLASS_DEVICE ); +uint8_t class =pci_config_readb (dev,PCI_CLASS_DEVICE +1 ); +int bar; + +printf ("dev %2d fn %d vendor_id %04x device_id %04x type %02x " +"progif %02x class %02x subclass %02x\n" , + dev /8 , dev %8 , vendor_id, device_id, header, + progif, class, subclass); + +if (header !=PCI_HEADER_TYPE_NORMAL ) +return ; + +for (bar =0 ; bar <6 ; bar++) { +phys_addr_t start, end; +char desc[8 ]; + +if (!pci_bar_is_valid (dev, bar)) +break ; + + start =pci_bar_addr (dev, bar); + end = start +pci_bar_size (dev, bar) -1 ; + +pci_type_desc (bar, desc,sizeof (desc)); + +if (pci_bar_is64 (dev, bar)) { +printf ("\tBAR#%d,%d [%-7s %" PRIx64"-%" PRIx64"]\n" , + bar, bar +1 , desc, start, end); + bar++; + }else { +printf ("\tBAR#%d [%-7s %02x-%02x]\n" , + bar, desc, (uint32_t )start, (uint32_t )end); + } + } +}
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 181
Quoted 10 lines
lib/pci.c +10
-0+ +void pci_print (void ) +{ +pcidevaddr_t dev; + +pci_print_arch (); + +for (dev =0 ; dev <256 ; ++dev) { +if (pci_config_readw (dev,PCI_VENDOR_ID ) != (uint16_t )~0 && +pci_config_readw (dev,PCI_DEVICE_ID ) != (uint16_t )~0 ) {
You could create a pci_dev_valid() function that checks vendor/device id.
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 183
Quoted 2 lines
lib/pci.c +2
-0+pci_dev_print (dev); + }
nit: no need for {} with a single line.
Quoted 19 lines
lib/pci.c +2
-0+ } +}lib/pci.h +3
-0@@ -22,6 +22,9 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num);bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num);bool pci_bar_is_valid (pcidevaddr_t dev,int bar_num); +void pci_type_desc (int type,char *desc,int len); +void pci_print (void ); +/* * pci-testdev is a driver for the pci-testdev qemu pci device. The * device enables testing mmio and portio exits, and measuring their Signature · 1 line
1.8.3.1
Thanks,
drew
Re: [PATCH RFC 10/15] pci: Add pci_print() and pci_type_desc()
Quoted 18 lines · Andrew Jones
Quoted 17 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/pci.h | 6 +++++ lib/pci.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci.h | 3 +++ 3 files changed, 76 insertions(+)lib/asm-generic/pci.h +0
-0@@ -22,4 +22,10 @@ phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) }#endif
Quoted 10 lines
lib/asm-generic/pci.h phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) · line 29
Quoted 5 lines
lib/asm-generic/pci.h +5
-0+#ifndef pci_print_arch +static inline void pci_print_arch (void ) +{ +} +#endif What's this function for? I think
pci_print()should be enough. Or is this for the host bridge? If so, then there should be a non-arch specificpci_host_bridge_print()call written that does everything it can with arch-neutral code, and just use arch calls as needed.
It is not only for the host bridge. My intention is printing any PCI specifics that an architecture may have. So in case of the current ARM and PPC it would be the host bridge. In general case there could be few host bridges or irregular PCI controller or something like that.
Currently ARM pci_print_arch() will print PCI memory space layout. That is quite auxiliary info and frankly, pci_print_arch() is not necessary at all. Up to you.
Quoted 27 lines
Quoted 12 lines
lib/asm-generic/pci.h +1
-0+#endif lib/pci.c +2
-0@@ -116,3 +116,70 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num)else return false; } + +void pci_type_desc (int type,char *desc,int len)lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 130
Quoted 10 lines
lib/pci.c +10
-0+{ +if ((type &PCI_BASE_ADDRESS_SPACE ) ==PCI_BASE_ADDRESS_SPACE_IO ) { +strcpy (desc,"PIO" );/* strncpy() would be better */ + }else { +static char *str[] = {"32" ,"1M" ,"64" }; +int idx = (type &PCI_BASE_ADDRESS_MEM_TYPE_MASK ) >>1 ; +int pfetch = type &PCI_BASE_ADDRESS_MEM_PREFETCH ; +snprintf (desc, len,"MEM%s%s" , str[idx], pfetch ?"/p" :"" ); + } +}What else, besides
pci_dev_print, would call this function? If only one function needs to output these descs, then we don't need the strcpy, snprintf stuff, we just need printf at the call site.
Quoted 2 lines
Oh, I see. It's because desc shows up in two printfs below. Well, I'd still just repeat the printfs, or maybe create a macro.
Also, a follow-up patch would use pci_type_desc() to describe host bridge's memory regions.
Quoted 45 lines
Quoted 45 lines
lib/pci.c +45
-0+static void pci_dev_print (pcidevaddr_t dev) +{ +uint16_t vendor_id =pci_config_readw (dev,PCI_VENDOR_ID ); +uint16_t device_id =pci_config_readw (dev,PCI_DEVICE_ID ); +uint8_t header =pci_config_readb (dev,PCI_HEADER_TYPE ); +uint8_t progif =pci_config_readb (dev,PCI_CLASS_PROG ); +uint8_t subclass =pci_config_readb (dev,PCI_CLASS_DEVICE ); +uint8_t class =pci_config_readb (dev,PCI_CLASS_DEVICE +1 ); +int bar; + +printf ("dev %2d fn %d vendor_id %04x device_id %04x type %02x " +"progif %02x class %02x subclass %02x\n" , + dev /8 , dev %8 , vendor_id, device_id, header, + progif, class, subclass); + +if (header !=PCI_HEADER_TYPE_NORMAL ) +return ; + +for (bar =0 ; bar <6 ; bar++) { +phys_addr_t start, end; +char desc[8 ]; + +if (!pci_bar_is_valid (dev, bar)) +break ; + + start =pci_bar_addr (dev, bar); + end = start +pci_bar_size (dev, bar) -1 ; + +pci_type_desc (bar, desc,sizeof (desc)); + +if (pci_bar_is64 (dev, bar)) { +printf ("\tBAR#%d,%d [%-7s %" PRIx64"-%" PRIx64"]\n" , + bar, bar +1 , desc, start, end); + bar++; + }else { +printf ("\tBAR#%d [%-7s %02x-%02x]\n" , + bar, desc, (uint32_t )start, (uint32_t )end); + } + } +} + +void pci_print (void ) +{ +pcidevaddr_t dev; +
Quoted 8 lines
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 181
Quoted 5 lines
lib/pci.c +5
-0+pci_print_arch (); + +for (dev =0 ; dev <256 ; ++dev) { +if (pci_config_readw (dev,PCI_VENDOR_ID ) != (uint16_t )~0 && +pci_config_readw (dev,PCI_DEVICE_ID ) != (uint16_t )~0 ) {You could create a
pci_dev_valid()function that checks vendor/device id.
What about pci_dev_exists() instead of pci_dev_valid()?
Quoted 27 lines
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 182
Quoted 2 lines
lib/pci.c +2
-0+pci_dev_print (dev); + }nit: no need for {} with a single line.
Quoted 19 lines
lib/pci.c +2
-0+ } +}lib/pci.h +3
-0@@ -22,6 +22,9 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num);bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num);bool pci_bar_is_valid (pcidevaddr_t dev,int bar_num); +void pci_type_desc (int type,char *desc,int len); +void pci_print (void ); +/* * pci-testdev is a driver for the pci-testdev qemu pci device. The * device enables testing mmio and portio exits, and measuring their Signature · 1 line
1.8.3.1Thanks,
drew
Re: [PATCH RFC 10/15] pci: Add pci_print() and pci_type_desc()
Quoted 30 lines · Alexander Gordeev
Quoted 18 lines · Andrew Jones
Quoted 17 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/pci.h | 6 +++++ lib/pci.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci.h | 3 +++ 3 files changed, 76 insertions(+)lib/asm-generic/pci.h +0
-0@@ -22,4 +22,10 @@ phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) }#endif Quoted 10 lines
lib/asm-generic/pci.h phys_addr_t pci_xlate_addr(pcidevaddr_t __unused dev, uint64_t addr) · line 29
Quoted 5 lines
lib/asm-generic/pci.h +5
-0+#ifndef pci_print_arch +static inline void pci_print_arch (void ) +{ +} +#endif What's this function for? I think
pci_print()should be enough. Or is this for the host bridge? If so, then there should be a non-arch specificpci_host_bridge_print()call written that does everything it can with arch-neutral code, and just use arch calls as needed.
Quoted 8 lines
It is not only for the host bridge. My intention is printing any PCI specifics that an architecture may have. So in case of the current ARM and PPC it would be the host bridge. In general case there could be few host bridges or irregular PCI controller or something like that.
Currently ARM
pci_print_arch()will print PCI memory space layout. That is quite auxiliary info and frankly,pci_print_arch()is not necessary at all. Up to you.
Let's drop it for now.
Quoted 28 lines
Quoted 28 lines
Quoted 13 lines
lib/asm-generic/pci.h +1
-0+#endif lib/pci.c +2
-0@@ -116,3 +116,70 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num)else return false; } + +void pci_type_desc (int type,char *desc,int len)lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 130
Quoted 10 lines
lib/pci.c +10
-0+{ +if ((type &PCI_BASE_ADDRESS_SPACE ) ==PCI_BASE_ADDRESS_SPACE_IO ) { +strcpy (desc,"PIO" );/* strncpy() would be better */ + }else { +static char *str[] = {"32" ,"1M" ,"64" }; +int idx = (type &PCI_BASE_ADDRESS_MEM_TYPE_MASK ) >>1 ; +int pfetch = type &PCI_BASE_ADDRESS_MEM_PREFETCH ; +snprintf (desc, len,"MEM%s%s" , str[idx], pfetch ?"/p" :"" ); + } +}What else, besides
pci_dev_print, would call this function? If only one function needs to output these descs, then we don't need the strcpy, snprintf stuff, we just need printf at the call site.
Quoted 5 lines
Quoted 2 lines
Oh, I see. It's because desc shows up in two printfs below. Well, I'd still just repeat the printfs, or maybe create a macro.
Also, a follow-up patch would use
pci_type_desc()to describe host bridge's memory regions.
OK, but rather than using snprintf, you could just use an enum/switch, allowing you to return, e.g. "PIO", "MEM32/p", etc. I think that would be cleaner.
Quoted 55 lines
Quoted 46 lines
Quoted 46 lines
lib/pci.c +45
-0+static void pci_dev_print (pcidevaddr_t dev) +{ +uint16_t vendor_id =pci_config_readw (dev,PCI_VENDOR_ID ); +uint16_t device_id =pci_config_readw (dev,PCI_DEVICE_ID ); +uint8_t header =pci_config_readb (dev,PCI_HEADER_TYPE ); +uint8_t progif =pci_config_readb (dev,PCI_CLASS_PROG ); +uint8_t subclass =pci_config_readb (dev,PCI_CLASS_DEVICE ); +uint8_t class =pci_config_readb (dev,PCI_CLASS_DEVICE +1 ); +int bar; + +printf ("dev %2d fn %d vendor_id %04x device_id %04x type %02x " +"progif %02x class %02x subclass %02x\n" , + dev /8 , dev %8 , vendor_id, device_id, header, + progif, class, subclass); + +if (header !=PCI_HEADER_TYPE_NORMAL ) +return ; + +for (bar =0 ; bar <6 ; bar++) { +phys_addr_t start, end; +char desc[8 ]; + +if (!pci_bar_is_valid (dev, bar)) +break ; + + start =pci_bar_addr (dev, bar); + end = start +pci_bar_size (dev, bar) -1 ; + +pci_type_desc (bar, desc,sizeof (desc)); + +if (pci_bar_is64 (dev, bar)) { +printf ("\tBAR#%d,%d [%-7s %" PRIx64"-%" PRIx64"]\n" , + bar, bar +1 , desc, start, end); + bar++; + }else { +printf ("\tBAR#%d [%-7s %02x-%02x]\n" , + bar, desc, (uint32_t )start, (uint32_t )end); + } + } +} + +void pci_print (void ) +{ +pcidevaddr_t dev; +Quoted 8 lines
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 181
Quoted 5 lines
lib/pci.c +5
-0+pci_print_arch (); + +for (dev =0 ; dev <256 ; ++dev) { +if (pci_config_readw (dev,PCI_VENDOR_ID ) != (uint16_t )~0 && +pci_config_readw (dev,PCI_DEVICE_ID ) != (uint16_t )~0 ) {You could create a
pci_dev_valid()function that checks vendor/device id.
Quoted 1 line
What about
pci_dev_exists()instead ofpci_dev_valid()?
Whatever terminology matches the spec.
Quoted 32 lines
Quoted 16 lines
lib/pci.c bool pci_bar_is64(pcidevaddr_t dev, int bar_num) · line 182
Quoted 3 lines
lib/pci.c +2
-0+pci_dev_print (dev); + }nit: no need for {} with a single line.
Quoted 10 lines
lib/pci.c +2
-0+ } +}lib/pci.h +0
-0@@ -22,6 +22,9 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num);bool pci_bar_is_memory (pcidevaddr_t dev,int bar_num);bool pci_bar_is_valid (pcidevaddr_t dev,int bar_num);Quoted 12 lines
Quoted 9 lines
lib/pci.h +3
-0+void pci_type_desc (int type,char *desc,int len); +void pci_print (void ); +/* * pci-testdev is a driver for the pci-testdev qemu pci device. The * device enables testing mmio and portio exits, and measuring their Signature · 1 line
1.8.3.1Thanks,
drew--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC PATCH 11/15 pci/x86: Adopt PCI framework changes
In reply to Alexander Gordeev--- lib/x86/asm/pci.h | 2 ++ x86/vmexit.c | 4 ++++ 2 files changed, 6 insertions(+)
lib/x86/asm/pci.h +2 -0
@@ -35,4 +35,6 @@ static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val outl(val, 0xCFC); } +#include <asm-generic/pci.h> +#endif
x86/vmexit.c +4 -0
@@ -386,6 +386,8 @@ int main(int ac, char **av) pm_tmr_blk = fadt->pm_tmr_blk;printf ("PM timer port is %x\n" , pm_tmr_blk); +pci_probe (); + pcidev =pci_find_dev (PCI_VENDOR_ID_REDHAT ,PCI_DEVICE_ID_REDHAT_TEST );if (pcidev !=PCIDEVADDR_INVALID ) {for (i =0 ; i <PCI_TESTDEV_NUM_BARS ; i++) {
@@ -407,5 +409,7 @@ int main(int ac, char **av)if (test_wanted (&tests[i], av +1 , ac -1 ))while (do_test (&tests[i])) {} +pci_shutdown (); +return 0 ; }
Signature · 1 line
1.8.3.1
Re: [PATCH RFC 11/15] pci/x86: Adopt PCI framework changes
Quoted 7 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/x86/asm/pci.h | 2 ++ x86/vmexit.c | 4 ++++ 2 files changed, 6 insertions(+)
Let's drop this patch. It doesn't do anything now, and won't do anything (probably) ever. It only clutters the x86 code.
thanks,
drew
Quoted 40 lines
lib/x86/asm/pci.h +2
-0@@ -35,4 +35,6 @@ static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val outl(val, 0xCFC); } +#include <asm-generic/pci.h> +#endif x86/vmexit.c +4
-0@@ -386,6 +386,8 @@ int main(int ac, char **av) pm_tmr_blk = fadt->pm_tmr_blk;printf ("PM timer port is %x\n" , pm_tmr_blk); +pci_probe (); + pcidev =pci_find_dev (PCI_VENDOR_ID_REDHAT ,PCI_DEVICE_ID_REDHAT_TEST );if (pcidev !=PCIDEVADDR_INVALID ) {for (i =0 ; i <PCI_TESTDEV_NUM_BARS ; i++) {@@ -407,5 +409,7 @@ int main(int ac, char **av)if (test_wanted (&tests[i], av +1 , ac -1 ))while (do_test (&tests[i])) {} +pci_shutdown (); +return 0 ; }Signature · 6 lines
1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC PATCH 12/15 Factor out generic architecture code
In reply to Alexander GordeevThis rework is a prerequisite for the forthcoming pci-testdev implementation. Basically, only generic port IO accessors are needed, but it turned out touching io/smp/mm files is needed. This update should likely be more comprehensive and split into several commits.
--- lib/asm-generic/io.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++++---- lib/x86/asm/page.h | 1 + lib/x86/io.h | 36 ++++++++++++++++++++++------------ lib/x86/smp.h | 4 ---- lib/x86/vm.c | 1 - lib/x86/vm.h | 12 +----------- x86/eventinj.c | 7 +------ x86/kvmclock.c | 1 + 8 files changed, 79 insertions(+), 38 deletions(-) create mode 100644 lib/x86/asm/page.h
lib/asm-generic/io.h +51 -4
@@ -127,6 +127,9 @@ static inline u64 __bswap64(u64 x) ({ u64 __r = !__cpu_is_be() ? __bswap64(x) : ((u64)x); __r; })#define cpu_to_be64 be64_to_cpu +#ifndef mb +#define mb () do { } while (0) +#endif #ifndef rmb#define rmb () do { } while (0)#endif
@@ -152,6 +155,48 @@ static inline u64 __bswap64(u64 x)#define writeq (b, addr) \ ({ wmb(); __raw_writeq(cpu_to_le64(b), addr); }) +#ifndef inb +static inline uint8_t inb (unsigned long port) +{ +return readb ((const volatile void __iomem *)port); +} +#endif + +#ifndef inw +static inline uint16_t inw (unsigned long port) +{ +return readw ((const volatile void __iomem *)port); +} +#endif + +#ifndef inl +static inline uint32_t inl (unsigned long port) +{ +return readl ((const volatile void __iomem *)port); +} +#endif + +#ifndef outb +static inline void outb (uint8_t value,unsigned long port) +{ +writeb (value, (volatile void __iomem *)port); +} +#endif + +#ifndef outw +static inline void outw (uint16_t value,unsigned long port) +{ +writew (value, (volatile void __iomem *)port); +} +#endif + +#ifndef outl +static inline void outl (uint32_t value,unsigned long port) +{ +writel (value, (volatile void __iomem *)port); +} +#endif +#ifndef ioremap static inline void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused) {
@@ -161,14 +206,16 @@ static inline void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused)#endif #ifndef virt_to_phys -static inline unsigned long virt_to_phys (volatile void *address) +static inline phys_addr_t virt_to_phys (volatile void *virt) { -return __pa ((unsigned long )address); +return (phys_addr_t )virt; } +#endif -static inline void *phys_to_virt (unsigned long address) +#ifndef phys_to_virt +static inline void *phys_to_virt (phys_addr_t phys) { -return __va (address); +return (void *)phys; }#endif
lib/x86/asm/page.h new file +1 -0
@@ -0,0 +1 @@ +#include <asm-generic/page.h>
lib/x86/io.h +24 -12
@@ -3,43 +3,55 @@#define __iomem -static inline unsigned char inb (unsigned short port) +#define mb () asm volatile("mfence":::"memory") +#define rmb () asm volatile("lfence":::"memory") +#define wmb () asm volatile("sfence" ::: "memory") + +#define inb inb +static inline uint8_t inb (unsigned long port) {unsigned char value; - asmvolatile ("inb %w1, %0" :"=a" (value) :"Nd" (port)); + asmvolatile ("inb %w1, %0" :"=a" (value) :"Nd" ((unsigned short )port));return value; } -static inline unsigned short inw (unsigned short port) +#define inw inw +static inline uint16_t inw (unsigned long port) {unsigned short value; - asmvolatile ("inw %w1, %0" :"=a" (value) :"Nd" (port)); + asmvolatile ("inw %w1, %0" :"=a" (value) :"Nd" ((unsigned short )port));return value; } -static inline unsigned int inl (unsigned short port) +#define inl inl +static inline uint32_t inl (unsigned long port) {unsigned int value; - asmvolatile ("inl %w1, %0" :"=a" (value) :"Nd" (port)); + asmvolatile ("inl %w1, %0" :"=a" (value) :"Nd" ((unsigned short )port));return value; } -static inline void outb (unsigned char value,unsigned short port) +#define outb outb +static inline void outb (uint8_t value,unsigned long port) { - asmvolatile ("outb %b0, %w1" : :"a" (value),"Nd" (port)); + asmvolatile ("outb %b0, %w1" : :"a" (value),"Nd" ((unsigned short )port)); } -static inline void outw (unsigned short value,unsigned short port) +#define outw outw +static inline void outw (uint16_t value,unsigned long port) { - asmvolatile ("outw %w0, %w1" : :"a" (value),"Nd" (port)); + asmvolatile ("outw %w0, %w1" : :"a" (value),"Nd" ((unsigned short )port)); } -static inline void outl (unsigned int value,unsigned short port) +#define outl outl +static inline void outl (uint32_t value,unsigned long port) { - asmvolatile ("outl %0, %w1" : :"a" (value),"Nd" (port)); + asmvolatile ("outl %0, %w1" : :"a" (value),"Nd" ((unsigned short )port)); }#define ioremap ioremap void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused); +#include <asm-generic/io.h> +#endif
lib/x86/smp.h +0 -4
@@ -2,10 +2,6 @@#define __SMP_H#include <asm/spinlock.h> -#define mb () asm volatile("mfence":::"memory") -#define rmb () asm volatile("lfence":::"memory") -#define wmb () asm volatile("sfence" ::: "memory") -void smp_init (void );int cpu_count (void );
lib/x86/vm.c +0 -1
@@ -2,7 +2,6 @@#include "vm.h" #include "libcflat.h" -#define PAGE_SIZE 4096ul#ifdef __x86_64__#define LARGE_PAGE_SIZE (512 * PAGE_SIZE)#else
lib/x86/vm.h +1 -11
@@ -2,8 +2,8 @@#define VM_H #include "processor.h" +#include "io.h" -#define PAGE_SIZE 4096ul#ifdef __x86_64__#define LARGE_PAGE_SIZE (512 * PAGE_SIZE)#else
@@ -39,14 +39,4 @@ unsigned long *install_large_page(unsigned long *cr3,unsigned long phys, void *virt);unsigned long *install_page (unsigned long *cr3,unsigned long phys,void *virt); -static inline unsigned long virt_to_phys (const void *virt) -{ -return (unsigned long )virt; -} - -static inline void *phys_to_virt (unsigned long phys) -{ -return (void *)phys; -} -#endif
x86/eventinj.c +1 -6
@@ -16,11 +16,6 @@ static inline void io_delay(void) { } -static inline void outl (int addr,int val) -{ - asmvolatile ("outl %1, %w0" : :"d" (addr),"a" (val)); -} -void apic_self_ipi (u8 v) { apic_icr_write(APIC_DEST_SELF | APIC_DEST_PHYSICAL | APIC_DM_FIXED |
@@ -32,7 +27,7 @@ void apic_self_nmi(void)apic_icr_write (APIC_DEST_PHYSICAL |APIC_DM_NMI |APIC_INT_ASSERT ,0 ); } -#define flush_phys_addr (__s) outl(0xe4, __s) +#define flush_phys_addr (__s) outl(__s, 0xe4)#define flush_stack () do { \ int __l; \ flush_phys_addr(virt_to_phys(&__l)); \
x86/kvmclock.c +1 -0
@@ -1,4 +1,5 @@#include "libcflat.h" +#include "io.h" #include "smp.h" #include "atomic.h" #include "processor.h"
Signature · 1 line
1.8.3.1
Re: [PATCH RFC 12/15] Factor out generic architecture code
Quoted 26 lines · Alexander Gordeev
This rework is a prerequisite for the forthcoming pci-testdev implementation. Basically, only generic port IO accessors are needed, but it turned out touching io/smp/mm files is needed. This update should likely be more comprehensive and split into several commits.
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/io.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++++---- lib/x86/asm/page.h | 1 + lib/x86/io.h | 36 ++++++++++++++++++++++------------ lib/x86/smp.h | 4 ---- lib/x86/vm.c | 1 - lib/x86/vm.h | 12 +----------- x86/eventinj.c | 7 +------ x86/kvmclock.c | 1 + 8 files changed, 79 insertions(+), 38 deletions(-) create mode 100644 lib/x86/asm/page.hlib/asm-generic/io.h +0
-0
lib/asm-generic/io.h static inline u64 __bswap64(u64 x) · line 132
Quoted 6 lines
lib/asm-generic/io.h +3
-0@@ -127,6 +127,9 @@ static inline u64 __bswap64(u64 x) ({ u64 __r = !__cpu_is_be() ? __bswap64(x) : ((u64)x); __r; })#define cpu_to_be64 be64_to_cpu +#ifndef mb +#define mb () do { } while (0) +#endif
I think it would be better to at least use a
asm volatile ("":::"memory")
here to make sure that the compiler does not try to optimize memory access before and behind the barrier?
Thomas
Re: [PATCH RFC 12/15] Factor out generic architecture code
Quoted 30 lines · Thomas Huth
Quoted 29 lines · Alexander Gordeev
This rework is a prerequisite for the forthcoming pci-testdev implementation. Basically, only generic port IO accessors are needed, but it turned out touching io/smp/mm files is needed. This update should likely be more comprehensive and split into several commits.
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/io.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++++---- lib/x86/asm/page.h | 1 + lib/x86/io.h | 36 ++++++++++++++++++++++------------ lib/x86/smp.h | 4 ---- lib/x86/vm.c | 1 - lib/x86/vm.h | 12 +----------- x86/eventinj.c | 7 +------ x86/kvmclock.c | 1 + 8 files changed, 79 insertions(+), 38 deletions(-) create mode 100644 lib/x86/asm/page.hlib/asm-generic/io.h +0
-0@@ -127,6 +127,9 @@ static inline u64 __bswap64(u64 x) ({ u64 __r = !__cpu_is_be() ? __bswap64(x) : ((u64)x); __r; })#define cpu_to_be64 be64_to_cpu
Quoted 8 lines
lib/asm-generic/io.h static inline u64 __bswap64(u64 x) · line 132
Quoted 3 lines
lib/asm-generic/io.h +3
-0+#ifndef mb +#define mb () do { } while (0) +#endif I think it would be better to at least use a
asm volatile ("":::"memory")
here to make sure that the compiler does not try to optimize memory access before and behind the barrier?
This is a blind copy of existing rmb() and wmb() in the current implementation.
This particular patch is rather to allow the following changes and may be to get some suggestions on how to rework the existing generic code in a better way.
I should have clarified this more clearly.
Quoted 2 lines
Thomas
Re: [PATCH RFC 12/15] Factor out generic architecture code
In reply to Thomas HuthQuoted 30 lines · Thomas Huth
Quoted 29 lines · Alexander Gordeev
This rework is a prerequisite for the forthcoming pci-testdev implementation. Basically, only generic port IO accessors are needed, but it turned out touching io/smp/mm files is needed. This update should likely be more comprehensive and split into several commits.
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/io.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++++---- lib/x86/asm/page.h | 1 + lib/x86/io.h | 36 ++++++++++++++++++++++------------ lib/x86/smp.h | 4 ---- lib/x86/vm.c | 1 - lib/x86/vm.h | 12 +----------- x86/eventinj.c | 7 +------ x86/kvmclock.c | 1 + 8 files changed, 79 insertions(+), 38 deletions(-) create mode 100644 lib/x86/asm/page.hlib/asm-generic/io.h +0
-0@@ -127,6 +127,9 @@ static inline u64 __bswap64(u64 x) ({ u64 __r = !__cpu_is_be() ? __bswap64(x) : ((u64)x); __r; })#define cpu_to_be64 be64_to_cpu
Quoted 8 lines
lib/asm-generic/io.h static inline u64 __bswap64(u64 x) · line 132
Quoted 3 lines
lib/asm-generic/io.h +3
-0+#ifndef mb +#define mb () do { } while (0) +#endif I think it would be better to at least use a
asm volatile ("":::"memory")
here to make sure that the compiler does not try to optimize memory access before and behind the barrier?
Agreed, but we should do it as a separate patch.
thanks,
drew
Quoted 7 lines
Thomas
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 12/15] Factor out generic architecture code
Quoted 40 lines · Andrew Jones
Quoted 30 lines · Thomas Huth
Quoted 29 lines · Alexander Gordeev
This rework is a prerequisite for the forthcoming pci-testdev implementation. Basically, only generic port IO accessors are needed, but it turned out touching io/smp/mm files is needed. This update should likely be more comprehensive and split into several commits.
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/io.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++++---- lib/x86/asm/page.h | 1 + lib/x86/io.h | 36 ++++++++++++++++++++++------------ lib/x86/smp.h | 4 ---- lib/x86/vm.c | 1 - lib/x86/vm.h | 12 +----------- x86/eventinj.c | 7 +------ x86/kvmclock.c | 1 + 8 files changed, 79 insertions(+), 38 deletions(-) create mode 100644 lib/x86/asm/page.hlib/asm-generic/io.h +0
-0@@ -127,6 +127,9 @@ static inline u64 __bswap64(u64 x) ({ u64 __r = !__cpu_is_be() ? __bswap64(x) : ((u64)x); __r; })#define cpu_to_be64 be64_to_cpuQuoted 8 lines
lib/asm-generic/io.h static inline u64 __bswap64(u64 x) · line 132
Quoted 3 lines
lib/asm-generic/io.h +3
-0+#ifndef mb +#define mb () do { } while (0) +#endif I think it would be better to at least use a
asm volatile ("":::"memory")
here to make sure that the compiler does not try to optimize memory access before and behind the barrier?
Quoted 1 line
Agreed, but we should do it as a separate patch.
Basically, the series a posted earlier this week "x86: Cleanup low-level arch code" is *this* update implemented in a a proper way (well, apart from those embarrassing flaws you spotted).
Quoted 10 lines
thanks,
drewQuoted 7 lines
Thomas
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 12/15] Factor out generic architecture code
In reply to Alexander GordeevQuoted 5 lines · Alexander Gordeev
This rework is a prerequisite for the forthcoming pci-testdev implementation. Basically, only generic port IO accessors are needed, but it turned out touching io/smp/mm files is needed. This update should likely be more comprehensive and split into several commits.
Yes. It should be split into more commits. And this commit message
Quoted 66 lines
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/io.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++++---- lib/x86/asm/page.h | 1 + lib/x86/io.h | 36 ++++++++++++++++++++++------------ lib/x86/smp.h | 4 ---- lib/x86/vm.c | 1 - lib/x86/vm.h | 12 +----------- x86/eventinj.c | 7 +------ x86/kvmclock.c | 1 + 8 files changed, 79 insertions(+), 38 deletions(-) create mode 100644 lib/x86/asm/page.hlib/asm-generic/io.h +35
-0@@ -127,6 +127,9 @@ static inline u64 __bswap64(u64 x) ({ u64 __r = !__cpu_is_be() ? __bswap64(x) : ((u64)x); __r; })#define cpu_to_be64 be64_to_cpu +#ifndef mb +#define mb () do { } while (0) +#endif #ifndef rmb#define rmb () do { } while (0)#endif @@ -152,6 +155,48 @@ static inline u64 __bswap64(u64 x)#define writeq (b, addr) \ ({ wmb(); __raw_writeq(cpu_to_le64(b), addr); }) +#ifndef inb +static inline uint8_t inb (unsigned long port) +{ +return readb ((const volatile void __iomem *)port); +} +#endif + +#ifndef inw +static inline uint16_t inw (unsigned long port) +{ +return readw ((const volatile void __iomem *)port); +} +#endif + +#ifndef inl +static inline uint32_t inl (unsigned long port) +{ +return readl ((const volatile void __iomem *)port); +} +#endif + +#ifndef outb +static inline void outb (uint8_t value,unsigned long port) +{ +writeb (value, (volatile void __iomem *)port); +} +#endif + +#ifndef outw +static inline void outw (uint16_t value,unsigned long port) +{ +writew (value, (volatile void __iomem *)port);
lib/asm-generic/io.h static inline u64 __bswap64(u64 x) · line 199
Quoted 10 lines
lib/asm-generic/io.h +10
-0+} +#endif + +#ifndef outl +static inline void outl (uint32_t value,unsigned long port) +{ +writel (value, (volatile void __iomem *)port); +} +#endif +
What are all the above for? Are we going to base arch-neutral pci code on these io-accessors? If so, why?
Quoted 13 lines
lib/asm-generic/io.h +2
-2#ifndef ioremap static inline void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused) {@@ -161,14 +206,16 @@ static inline void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused)#endif #ifndef virt_to_phys -static inline unsigned long virt_to_phys (volatile void *address) +static inline phys_addr_t virt_to_phys (volatile void *virt) { -return __pa ((unsigned long )address); +return (phys_addr_t )virt; }
lib/asm-generic/io.h static inline void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused) · line 220
Quoted 10 lines
lib/asm-generic/io.h +4
-2+#endif -static inline void *phys_to_virt (unsigned long address) +#ifndef phys_to_virt +static inline void *phys_to_virt (phys_addr_t phys) { -return __va (address); +return (void *)phys; }#endif
Why change the generic virt_to_phys/phys_to_virt? I think all architectures will want to define their own, in which case we don't really need these anyway, but they're here now, and they match Linux's asm-generic implementations.
Quoted 68 lines
lib/x86/asm/page.h new file +1
-0@@ -0,0 +1 @@ +#include <asm-generic/page.h> lib/x86/io.h +20
-11@@ -3,43 +3,55 @@#define __iomem -static inline unsigned char inb (unsigned short port) +#define mb () asm volatile("mfence":::"memory") +#define rmb () asm volatile("lfence":::"memory") +#define wmb () asm volatile("sfence" ::: "memory") + +#define inb inb +static inline uint8_t inb (unsigned long port) {unsigned char value; - asmvolatile ("inb %w1, %0" :"=a" (value) :"Nd" (port)); + asmvolatile ("inb %w1, %0" :"=a" (value) :"Nd" ((unsigned short )port));return value; } -static inline unsigned short inw (unsigned short port) +#define inw inw +static inline uint16_t inw (unsigned long port) {unsigned short value; - asmvolatile ("inw %w1, %0" :"=a" (value) :"Nd" (port)); + asmvolatile ("inw %w1, %0" :"=a" (value) :"Nd" ((unsigned short )port));return value; } -static inline unsigned int inl (unsigned short port) +#define inl inl +static inline uint32_t inl (unsigned long port) {unsigned int value; - asmvolatile ("inl %w1, %0" :"=a" (value) :"Nd" (port)); + asmvolatile ("inl %w1, %0" :"=a" (value) :"Nd" ((unsigned short )port));return value; } -static inline void outb (unsigned char value,unsigned short port) +#define outb outb +static inline void outb (uint8_t value,unsigned long port) { - asmvolatile ("outb %b0, %w1" : :"a" (value),"Nd" (port)); + asmvolatile ("outb %b0, %w1" : :"a" (value),"Nd" ((unsigned short )port)); } -static inline void outw (unsigned short value,unsigned short port) +#define outw outw +static inline void outw (uint16_t value,unsigned long port) { - asmvolatile ("outw %w0, %w1" : :"a" (value),"Nd" (port)); + asmvolatile ("outw %w0, %w1" : :"a" (value),"Nd" ((unsigned short )port)); } -static inline void outl (unsigned int value,unsigned short port) +#define outl outl
lib/x86/io.h line 55
Quoted 10 lines
lib/x86/io.h +3
-1+static inline void outl (uint32_t value,unsigned long port) { - asmvolatile ("outl %0, %w1" : :"a" (value),"Nd" (port)); + asmvolatile ("outl %0, %w1" : :"a" (value),"Nd" ((unsigned short )port)); }#define ioremap ioremap void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused); +#include <asm-generic/io.h>
let's avoid doing this until x86 needs something from asm-generic/io.h
Quoted 34 lines
lib/x86/io.h +1
-0+#endif lib/x86/smp.h +0
-4@@ -2,10 +2,6 @@#define __SMP_H#include <asm/spinlock.h> -#define mb () asm volatile("mfence":::"memory") -#define rmb () asm volatile("lfence":::"memory") -#define wmb () asm volatile("sfence" ::: "memory") -void smp_init (void );int cpu_count (void );lib/x86/vm.c +0
-1@@ -2,7 +2,6 @@#include "vm.h" #include "libcflat.h" -#define PAGE_SIZE 4096ul#ifdef __x86_64__#define LARGE_PAGE_SIZE (512 * PAGE_SIZE)#else lib/x86/vm.h +0
-0
lib/x86/vm.h line 6
Quoted 6 lines
lib/x86/vm.h +1
-1@@ -2,8 +2,8 @@#define VM_H #include "processor.h" +#include "io.h" -#define PAGE_SIZE 4096ul
Strange. You've removed two definitions of PAGE_SIZE. I think that removes them all. How does x86 still compile after this patch?
Quoted 7 lines
lib/x86/vm.h +0
-0#ifdef __x86_64__#define LARGE_PAGE_SIZE (512 * PAGE_SIZE)#else @@ -39,14 +39,4 @@ unsigned long *install_large_page(unsigned long *cr3,unsigned long phys, void *virt);unsigned long *install_page (unsigned long *cr3,unsigned long phys,void *virt);
lib/x86/vm.h unsigned long *install_large_page(unsigned long *cr3,unsigned long phys, · line 51
Quoted 10 lines
lib/x86/vm.h +0
-10-static inline unsigned long virt_to_phys (const void *virt) -{ -return (unsigned long )virt; -} - -static inline void *phys_to_virt (unsigned long phys) -{ -return (void *)phys; -} -
Uh oh. You've now changed x86's virt_to_phys/phys_to_virt to using phys_addr_t. The unit tests may not be happy!
Quoted 31 lines
lib/x86/vm.h +0
-0#endif x86/eventinj.c +1
-6@@ -16,11 +16,6 @@ static inline void io_delay(void) { } -static inline void outl (int addr,int val) -{ - asmvolatile ("outl %1, %w0" : :"d" (addr),"a" (val)); -} -void apic_self_ipi (u8 v) { apic_icr_write(APIC_DEST_SELF | APIC_DEST_PHYSICAL | APIC_DM_FIXED |@@ -32,7 +27,7 @@ void apic_self_nmi(void)apic_icr_write (APIC_DEST_PHYSICAL |APIC_DM_NMI |APIC_INT_ASSERT ,0 ); } -#define flush_phys_addr (__s) outl(0xe4, __s) +#define flush_phys_addr (__s) outl(__s, 0xe4)#define flush_stack () do { \ int __l; \ flush_phys_addr(virt_to_phys(&__l)); \x86/kvmclock.c +0
-0
x86/kvmclock.c line 2
Quoted 2 lines
x86/kvmclock.c +1
-0@@ -1,4 +1,5 @@#include "libcflat.h" +#include "io.h"
why add this include?
Quoted 6 lines
x86/kvmclock.c +0
-0#include "smp.h" #include "atomic.h" #include "processor.h" Signature · 1 line
1.8.3.1
I guess I'll see in the next patches what the motivation for this one is, but at the moment I'd say just completely drop this one.
thanks,
drew
Re: [PATCH RFC 12/15] Factor out generic architecture code
Quoted 80 lines · Andrew Jones
Quoted 5 lines · Alexander Gordeev
This rework is a prerequisite for the forthcoming pci-testdev implementation. Basically, only generic port IO accessors are needed, but it turned out touching io/smp/mm files is needed. This update should likely be more comprehensive and split into several commits.
Yes. It should be split into more commits. And this commit message
Quoted 71 lines
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/asm-generic/io.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++++---- lib/x86/asm/page.h | 1 + lib/x86/io.h | 36 ++++++++++++++++++++++------------ lib/x86/smp.h | 4 ---- lib/x86/vm.c | 1 - lib/x86/vm.h | 12 +----------- x86/eventinj.c | 7 +------ x86/kvmclock.c | 1 + 8 files changed, 79 insertions(+), 38 deletions(-) create mode 100644 lib/x86/asm/page.hlib/asm-generic/io.h +40
-0@@ -127,6 +127,9 @@ static inline u64 __bswap64(u64 x) ({ u64 __r = !__cpu_is_be() ? __bswap64(x) : ((u64)x); __r; })#define cpu_to_be64 be64_to_cpu +#ifndef mb +#define mb () do { } while (0) +#endif #ifndef rmb#define rmb () do { } while (0)#endif @@ -152,6 +155,48 @@ static inline u64 __bswap64(u64 x)#define writeq (b, addr) \ ({ wmb(); __raw_writeq(cpu_to_le64(b), addr); }) +#ifndef inb +static inline uint8_t inb (unsigned long port) +{ +return readb ((const volatile void __iomem *)port); +} +#endif + +#ifndef inw +static inline uint16_t inw (unsigned long port) +{ +return readw ((const volatile void __iomem *)port); +} +#endif + +#ifndef inl +static inline uint32_t inl (unsigned long port) +{ +return readl ((const volatile void __iomem *)port); +} +#endif + +#ifndef outb +static inline void outb (uint8_t value,unsigned long port) +{ +writeb (value, (volatile void __iomem *)port); +} +#endif + +#ifndef outw +static inline void outw (uint16_t value,unsigned long port) +{ +writew (value, (volatile void __iomem *)port); +} +#endif + +#ifndef outl +static inline void outl (uint32_t value,unsigned long port)
Quoted 8 lines
lib/asm-generic/io.h static inline u64 __bswap64(u64 x) · line 199
Quoted 5 lines
lib/asm-generic/io.h +5
-0+{ +writel (value, (volatile void __iomem *)port); +} +#endif +What are all the above for? Are we going to base arch-neutral pci code on these io-accessors? If so, why?
I am not sure I got your question, but I will answer as if I did :)
We need in[bwl]/out[bwl] to access devices' port IO space. Out of all archs in kvm-unit-tests it is only x86 that uses dedicated instructions and overrides the generic ones. All others do it as Linux IO generics do - using read[bwl]/write[bwl].
Quoted 15 lines
Quoted 15 lines
lib/asm-generic/io.h +3
-2#ifndef ioremap static inline void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused) {@@ -161,14 +206,16 @@ static inline void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused)#endif #ifndef virt_to_phys -static inline unsigned long virt_to_phys (volatile void *address) +static inline phys_addr_t virt_to_phys (volatile void *virt) { -return __pa ((unsigned long )address); +return (phys_addr_t )virt; } +#endif
Quoted 13 lines
lib/asm-generic/io.h static inline void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused) · line 220
Quoted 8 lines
lib/asm-generic/io.h +3
-2-static inline void *phys_to_virt (unsigned long address) +#ifndef phys_to_virt +static inline void *phys_to_virt (phys_addr_t phys) { -return __va (address); +return (void *)phys; }#endif Why change the generic virt_to_phys/phys_to_virt? I think all architectures will want to define their own, in which case we don't really need these anyway, but they're here now, and they match Linux's asm-generic implementations.
Right. In my new version I preserved these once by just adding missing __va/__pa to x86 in "x86: Cleanup low-level arch code" series.
Quoted 189 lines
Quoted 68 lines
lib/x86/asm/page.h new file +1
-0@@ -0,0 +1 @@ +#include <asm-generic/page.h> lib/x86/io.h +20
-11@@ -3,43 +3,55 @@#define __iomem -static inline unsigned char inb (unsigned short port) +#define mb () asm volatile("mfence":::"memory") +#define rmb () asm volatile("lfence":::"memory") +#define wmb () asm volatile("sfence" ::: "memory") + +#define inb inb +static inline uint8_t inb (unsigned long port) {unsigned char value; - asmvolatile ("inb %w1, %0" :"=a" (value) :"Nd" (port)); + asmvolatile ("inb %w1, %0" :"=a" (value) :"Nd" ((unsigned short )port));return value; } -static inline unsigned short inw (unsigned short port) +#define inw inw +static inline uint16_t inw (unsigned long port) {unsigned short value; - asmvolatile ("inw %w1, %0" :"=a" (value) :"Nd" (port)); + asmvolatile ("inw %w1, %0" :"=a" (value) :"Nd" ((unsigned short )port));return value; } -static inline unsigned int inl (unsigned short port) +#define inl inl +static inline uint32_t inl (unsigned long port) {unsigned int value; - asmvolatile ("inl %w1, %0" :"=a" (value) :"Nd" (port)); + asmvolatile ("inl %w1, %0" :"=a" (value) :"Nd" ((unsigned short )port));return value; } -static inline void outb (unsigned char value,unsigned short port) +#define outb outb +static inline void outb (uint8_t value,unsigned long port) { - asmvolatile ("outb %b0, %w1" : :"a" (value),"Nd" (port)); + asmvolatile ("outb %b0, %w1" : :"a" (value),"Nd" ((unsigned short )port)); } -static inline void outw (unsigned short value,unsigned short port) +#define outw outw +static inline void outw (uint16_t value,unsigned long port) { - asmvolatile ("outw %w0, %w1" : :"a" (value),"Nd" (port)); + asmvolatile ("outw %w0, %w1" : :"a" (value),"Nd" ((unsigned short )port)); } -static inline void outl (unsigned int value,unsigned short port) +#define outl outllib/x86/io.h line 55
Quoted 10 lines
lib/x86/io.h +3
-1+static inline void outl (uint32_t value,unsigned long port) { - asmvolatile ("outl %0, %w1" : :"a" (value),"Nd" (port)); + asmvolatile ("outl %0, %w1" : :"a" (value),"Nd" ((unsigned short )port)); }#define ioremap ioremap void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused); +#include <asm-generic/io.h> let's avoid doing this until x86 needs something from
asm-generic/io.hQuoted 34 lines
lib/x86/io.h +1
-0+#endif lib/x86/smp.h +0
-4@@ -2,10 +2,6 @@#define __SMP_H#include <asm/spinlock.h> -#define mb () asm volatile("mfence":::"memory") -#define rmb () asm volatile("lfence":::"memory") -#define wmb () asm volatile("sfence" ::: "memory") -void smp_init (void );int cpu_count (void );lib/x86/vm.c +0
-1@@ -2,7 +2,6 @@#include "vm.h" #include "libcflat.h" -#define PAGE_SIZE 4096ul#ifdef __x86_64__#define LARGE_PAGE_SIZE (512 * PAGE_SIZE)#else lib/x86/vm.h +0
-0lib/x86/vm.h line 6
Quoted 6 lines
lib/x86/vm.h +1
-1@@ -2,8 +2,8 @@#define VM_H #include "processor.h" +#include "io.h" -#define PAGE_SIZE 4096ulStrange. You've removed two definitions of
PAGE_SIZE. I think that removes them all. How does x86 still compile after this patch?Quoted 7 lines
lib/x86/vm.h +0
-0#ifdef __x86_64__#define LARGE_PAGE_SIZE (512 * PAGE_SIZE)#else @@ -39,14 +39,4 @@ unsigned long *install_large_page(unsigned long *cr3,unsigned long phys, void *virt);unsigned long *install_page (unsigned long *cr3,unsigned long phys,void *virt);lib/x86/vm.h unsigned long *install_large_page(unsigned long *cr3,unsigned long phys, · line 51
Quoted 10 lines
lib/x86/vm.h +0
-10-static inline unsigned long virt_to_phys (const void *virt) -{ -return (unsigned long )virt; -} - -static inline void *phys_to_virt (unsigned long phys) -{ -return (void *)phys; -} -Uh oh. You've now changed x86's virt_to_phys/phys_to_virt to using
phys_addr_t. The unit tests may not be happy!Quoted 31 lines
lib/x86/vm.h +0
-0#endif x86/eventinj.c +1
-6@@ -16,11 +16,6 @@ static inline void io_delay(void) { } -static inline void outl (int addr,int val) -{ - asmvolatile ("outl %1, %w0" : :"d" (addr),"a" (val)); -} -void apic_self_ipi (u8 v) { apic_icr_write(APIC_DEST_SELF | APIC_DEST_PHYSICAL | APIC_DM_FIXED |@@ -32,7 +27,7 @@ void apic_self_nmi(void)apic_icr_write (APIC_DEST_PHYSICAL |APIC_DM_NMI |APIC_INT_ASSERT ,0 ); } -#define flush_phys_addr (__s) outl(0xe4, __s) +#define flush_phys_addr (__s) outl(__s, 0xe4)#define flush_stack () do { \ int __l; \ flush_phys_addr(virt_to_phys(&__l)); \x86/kvmclock.c +0
-0x86/kvmclock.c line 2
Quoted 2 lines
x86/kvmclock.c +1
-0@@ -1,4 +1,5 @@#include "libcflat.h" +#include "io.h" why add this include?
Quoted 6 lines
x86/kvmclock.c +0
-0#include "smp.h" #include "atomic.h" #include "processor.h" Signature · 1 line
1.8.3.1
Quoted 2 lines
I guess I'll see in the next patches what the motivation for this one is, but at the moment I'd say just completely drop this one.
Yes, sorry I did not make it even clearer. This patch is to enable following changes. I hoped to gather some ideas on proper approach, but it is completely superceded by the series "x86: Cleanup low-level arch code".
Quoted 2 lines
thanks,
drew
Re: [PATCH RFC 12/15] Factor out generic architecture code
In reply to Andrew JonesQuoted 5 lines · Andrew Jones
Quoted 5 lines
lib/x86/vm.h +0
-5-static inline unsigned long virt_to_phys (const void *virt) -{ -return (unsigned long )virt; -} -
Quoted 8 lines
lib/x86/vm.h unsigned long *install_large_page(unsigned long *cr3,unsigned long phys, · line 51
Quoted 5 lines
lib/x86/vm.h +0
-5-static inline void *phys_to_virt (unsigned long phys) -{ -return (void *)phys; -} -Uh oh. You've now changed x86's virt_to_phys/phys_to_virt to using
phys_addr_t. The unit tests may not be happy!
What might go wrong with phys_addr_t?
Quoted 2 lines
thanks,
drew
Re: [PATCH RFC 12/15] Factor out generic architecture code
Quoted 15 lines · Alexander Gordeev
Quoted 5 lines · Andrew Jones
Quoted 5 lines
lib/x86/vm.h +0
-5-static inline unsigned long virt_to_phys (const void *virt) -{ -return (unsigned long )virt; -} -Quoted 8 lines
lib/x86/vm.h unsigned long *install_large_page(unsigned long *cr3,unsigned long phys, · line 51
Quoted 5 lines
lib/x86/vm.h +0
-5-static inline void *phys_to_virt (unsigned long phys) -{ -return (void *)phys; -} -Uh oh. You've now changed x86's virt_to_phys/phys_to_virt to using
phys_addr_t. The unit tests may not be happy!
Quoted 1 line
What might go wrong with
phys_addr_t?
Compilation errors.
Quoted 7 lines
Quoted 3 lines
thanks,
drew--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC PATCH 13/15 pci/arm: Add generic ECAM host support
In reply to Alexander GordeevUnlike x86, ARM and PPC kvm-unit-tests do not have a luxury of PCI bus initialized by a BIOS and ready to use at start. Thus, we need allocate and assign resources to all devices.
There is no any sort of resource management for memory and io spaces, since only one-per-BAR allocations are expected between calls to pci_probe() and pci_shutdown(), and no deallocations at all.
--- arm/Makefile.common | 6 +- arm/pci-test.c | 23 ++++ lib/arm64/asm/pci.h | 29 +++++ lib/pci-host-generic.c | 321 +++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci-host-generic.h | 57 +++++++++ 5 files changed, 435 insertions(+), 1 deletion(-) create mode 100644 arm/pci-test.c create mode 100644 lib/arm64/asm/pci.h create mode 100644 lib/pci-host-generic.c create mode 100644 lib/pci-host-generic.h
arm/Makefile.common +5 -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
@@ -30,6 +31,8 @@ include scripts/asm-offsets.mak cflatobjs += lib/util.o cflatobjs += lib/alloc.o cflatobjs += lib/devicetree.o +cflatobjs += lib/pci.o +cflatobjs += lib/pci-host-generic.o cflatobjs += lib/virtio.o cflatobjs += lib/virtio-mmio.o cflatobjs += lib/chr-testdev.o
@@ -73,3 +76,4 @@ $(TEST_DIR)/selftest.elf: $(cstart.o) $(TEST_DIR)/selftest.o $(TEST_DIR)/spinlock-test.elf: $(cstart.o) $(TEST_DIR)/spinlock-test.o $(TEST_DIR)/selftest.o $(cstart.o): $(asm-offsets) +$(TEST_DIR)/pci-test.elf: $(cstart.o) $(TEST_DIR)/pci-test.o
arm/pci-test.c new file +23 -0
@@ -0,0 +1,23 @@ +/* +* PCI bus operation test +* +* Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]> +* +* This work is licensed under the terms of the GNU LGPL, version 2. +*/ +#include <libcflat.h> +#include <pci.h> + +int main (void ) +{ +int ret; + + ret =pci_probe (); +report ("PCI bus probing" , ret); + +pci_print (); + +pci_shutdown (); + +return report_summary (); +}
lib/arm64/asm/pci.h new file +29 -0
@@ -0,0 +1,29 @@ +#ifndef ASM_PCI_H +#define ASM_PCI_H +/* +* Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]> +* +* This work is licensed under the terms of the GNU LGPL, version 2. +*/ +#include "libcflat.h" + +uint8_t pci_config_readb (pcidevaddr_t dev,uint8_t reg); +uint16_t pci_config_readw (pcidevaddr_t dev,uint8_t reg); +uint32_t pci_config_readl (pcidevaddr_t dev,uint8_t reg); +void pci_config_writel (pcidevaddr_t dev,uint8_t reg,uint32_t val); + +#define pci_xslate_addr pci_xlate_addr +phys_addr_t pci_xlate_addr (pcidevaddr_t dev,uint64_t addr); + +#define pci_print_arch pci_print_arch +void pci_print_arch (void ); + +#define pci_probe pci_probe +bool pci_probe (void ); + +#define pci_shutdown pci_shutdown +void pci_shutdown (void ); + +#include <asm-generic/pci.h> + +#endif
lib/pci-host-generic.c new file +321 -0
321 lines not on this page · raw message
lib/pci-host-generic.h new file +57 -0
57 lines not on this page · raw message
Signature · 1 line
1.8.3.1
Re: [PATCH RFC 13/15] pci/arm: Add generic ECAM host support
nit: I'd prefer $SUBJECT to have prefixes like 'arm/arm64: pci:' rather than pci/arm.
Quoted 68 lines · Alexander Gordeev
Unlike x86, ARM and PPC kvm-unit-tests do not have a luxury of PCI bus initialized by a BIOS and ready to use at start. Thus, we need allocate and assign resources to all devices.
There is no any sort of resource management for memory and io spaces, since only one-per-BAR allocations are expected between calls to
pci_probe()andpci_shutdown(), and no deallocations at all.Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- arm/Makefile.common | 6 +- arm/pci-test.c | 23 ++++ lib/arm64/asm/pci.h | 29 +++++ lib/pci-host-generic.c | 321 +++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci-host-generic.h | 57 +++++++++ 5 files changed, 435 insertions(+), 1 deletion(-) create mode 100644 arm/pci-test.c create mode 100644 lib/arm64/asm/pci.h create mode 100644 lib/pci-host-generic.c create mode 100644 lib/pci-host-generic.harm/Makefile.common +5
-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@@ -30,6 +31,8 @@ include scripts/asm-offsets.mak cflatobjs += lib/util.o cflatobjs += lib/alloc.o cflatobjs += lib/devicetree.o +cflatobjs += lib/pci.o +cflatobjs += lib/pci-host-generic.o cflatobjs += lib/virtio.o cflatobjs += lib/virtio-mmio.o cflatobjs += lib/chr-testdev.o@@ -73,3 +76,4 @@ $(TEST_DIR)/selftest.elf: $(cstart.o) $(TEST_DIR)/selftest.o $(TEST_DIR)/spinlock-test.elf: $(cstart.o) $(TEST_DIR)/spinlock-test.o $(TEST_DIR)/selftest.o $(cstart.o): $(asm-offsets) +$(TEST_DIR)/pci-test.elf: $(cstart.o) $(TEST_DIR)/pci-test.oarm/pci-test.c new file +10
-0@@ -0,0 +1,23 @@ +/* +* PCI bus operation test +* +* Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]> +* +* This work is licensed under the terms of the GNU LGPL, version 2. +*/ +#include <libcflat.h> +#include <pci.h> +
arm/pci-test.c line 20
Quoted 10 lines
arm/pci-test.c new file +10
-0+int main (void ) +{ +int ret; + + ret =pci_probe (); +report ("PCI bus probing" , ret); + +pci_print (); + +pci_shutdown ();
I still think having a pci_shutdown() is overkill for kvm-unit-tests, but if it's already implemented then why not. Maybe it could be useful in a pci-related unit test?
lib/arm64/asm/pci.h line 2
Quoted 11 lines
arm/pci-test.c new file +3
-0+ +return report_summary (); +}lib/arm64/asm/pci.h new file +2
-0@@ -0,0 +1,29 @@ +#ifndef ASM_PCI_H +#define ASM_PCI_H
nit: I'd prefer _ASMARM64_PCI_H_
Quoted 33 lines
lib/arm64/asm/pci.h new file +27
-0+/* +* Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]> +* +* This work is licensed under the terms of the GNU LGPL, version 2. +*/ +#include "libcflat.h" + +uint8_t pci_config_readb (pcidevaddr_t dev,uint8_t reg); +uint16_t pci_config_readw (pcidevaddr_t dev,uint8_t reg); +uint32_t pci_config_readl (pcidevaddr_t dev,uint8_t reg); +void pci_config_writel (pcidevaddr_t dev,uint8_t reg,uint32_t val); + +#define pci_xslate_addr pci_xlate_addr +phys_addr_t pci_xlate_addr (pcidevaddr_t dev,uint64_t addr); + +#define pci_print_arch pci_print_arch +void pci_print_arch (void ); + +#define pci_probe pci_probe +bool pci_probe (void ); + +#define pci_shutdown pci_shutdown +void pci_shutdown (void ); + +#include <asm-generic/pci.h> + +#endif lib/pci-host-generic.c new file +0
-0
lib/pci-host-generic.c line 2
Quoted 2 lines
lib/pci-host-generic.c new file +2
-0@@ -0,0 +1,321 @@ +/* +* PCI bus operation test
Probably the wrong description. It sure isn't talking about host bridges.
lib/pci-host-generic.c line 15
Quoted 13 lines
lib/pci-host-generic.c new file +13
-0+* +* Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]> +* +* This work is licensed under the terms of the GNU LGPL, version 2. +*/ +#include "libcflat.h" +#include "devicetree.h" +#include "asm/io.h" +#include "pci.h" +#include "pci-host-generic.h" +#include <linux/pci_regs.h> + +static struct pci_host_bridge *pci_host_bridge =NULL ;
No need for the explicit '= NULL'.
lib/pci-host-generic.c line 19
Quoted 4 lines
lib/pci-host-generic.c new file +4
-0+ +static void __iomem *pci_get_dev_conf (struct pci_host_bridge *host,int dev) +{ +return (void __iomem *)host->start + dev * (1 <<PCI_ECAM_DEVICE_SHIFT );
Let's remove the multiplication: ... + (dev << PCI_ECAM_DEVICE_SHIFT)
Quoted 9 lines
lib/pci-host-generic.c new file +9
-0+} + +static int of_flags_to_pci_type (u32 of_flags) +{ +static int type_map[] = { + [1 ] =PCI_BASE_ADDRESS_SPACE_IO , + [2 ] =PCI_BASE_ADDRESS_MEM_TYPE_32 , + [3 ] =PCI_BASE_ADDRESS_MEM_TYPE_64 + };
lib/pci-host-generic.c line 38
Quoted 10 lines
lib/pci-host-generic.c new file +10
-0+int res = type_map[(of_flags >>24 ) &0x03 ]; + +if (of_flags &0x40000000 ) + res |=PCI_BASE_ADDRESS_MEM_PREFETCH ; + +return res; +} + +static void pci_host_addr_space_init (struct pci_addr_space as[],int nr_as, +fdt32_t rcells[],int nr_rcells)
This is a DT specific function so should be named as such. Actually I don't really think we need it anyway. There's only one caller, so why not just opencode it at the callsite?
Quoted 59 lines
lib/pci-host-generic.c new file +59
-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 +* 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++, rcells += nr_rcells) { + as->type =of_flags_to_pci_type (fdt32_to_cpu (rcells[0 ])); + as->pci_start = ((u64 )fdt32_to_cpu (rcells[1 ]) <<32 ) | +fdt32_to_cpu (rcells[2 ]); + +if (nr_rcells ==6 ) { + as->start =fdt32_to_cpu (rcells[3 ]); + as->size = ((u64 )fdt32_to_cpu (rcells[4 ]) <<32 ) | +fdt32_to_cpu (rcells[5 ]); + }else { + as->start = ((u64 )fdt32_to_cpu (rcells[3 ]) <<32 ) | +fdt32_to_cpu (rcells[4 ]); + as->size = ((u64 )fdt32_to_cpu (rcells[5 ]) <<32 ) | +fdt32_to_cpu (rcells[6 ]); + } + } +} + +/* +* Probe DT for a generic PCI host controller +* See kernel Documentation/devicetree/bindings/pci/host-generic-pci.txt +* and function gen_pci_probe() in drivers/pci/host/pci-host-generic.c +*/ +static struct pci_host_bridge *pci_dt_probe (void ) +{ +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; + +if (!dt_available ()) { +printf ("No device tree found" ); +return NULL ; + } + +dt_bus_init_defaults (&dt_bus); +dt_device_init (&dt_dev, &dt_bus,NULL ); +
lib/pci-host-generic.c line 107
Quoted 10 lines
lib/pci-host-generic.c new file +10
-0+ node =fdt_path_offset (fdt,"/" ); +assert (node >=0 ); + + ret =dt_get_nr_cells (node, &nac_root, &nsc_root); +assert (ret ==0 ); +assert (nac_root ==1 || nac_root ==2 ); + + node =fdt_subnode_offset (fdt, node,"pcie" ); +if (node == -FDT_ERR_NOTFOUND ) { +printf ("No PCIe controller found" );
missing '\n'
lib/pci-host-generic.c line 115
Quoted 8 lines
lib/pci-host-generic.c new file +8
-0+return NULL ; + } +assert (node >=0 ); + + prop =fdt_get_property (fdt, node,"device_type" , &len); +if (!prop || len !=4 ||strcmp ((char *)prop->data,"pci" )) { +printf ("PCIe controller device_type is not \"pci\"" ); +return NULL ;
Documentation/devicetree/bindings/pci/host-generic-pci.txt says that device_type must be "pci", so this is an assert() case, not a return NULL case.
lib/pci-host-generic.c line 118
Quoted 3 lines
lib/pci-host-generic.c new file +3
-0+ } + + ret =fdt_node_check_compatible (fdt, node,"pci-host-ecam-generic" );
This check should be done before the device_type property check.
Actually, we might as well skip the fdt_subnode_offset(fdt, node, "pcie") search, and just search by this compatibility string, i.e. use fdt_node_offset_by_compatible() instead. If it's not found, return NULL, if it is found, but device_type != pci, then assert.
Not your fault. The code I gave you for this function had it just like this, but now on review, it looks like there's room for improvement :-)
Quoted 40 lines
lib/pci-host-generic.c new file +40
-0+assert (ret >=0 ); +if (ret !=0 ) { +printf ("PCIe controller is not ECAM compatible" ); +return NULL ; + } + +dt_device_bind_node (&dt_dev, node); + ret =dt_pbus_get_base (&dt_dev, &base); +assert (ret ==0 ); + + prop =fdt_get_property (fdt, node,"bus-range" , &len); +if (prop ==NULL ) { +assert (len == -FDT_ERR_NOTFOUND ); + bus =0x00 ; + bus_max =0xff ; + }else { +fdt32_t *data = (fdt32_t *)prop->data; + bus =fdt32_to_cpu (data[0 ]); + bus_max =fdt32_to_cpu (data[1 ]); +assert (bus <= bus_max); + } +assert (bus_max < base.size / (1 <<PCI_ECAM_BUS_SHIFT )); + + ret =dt_get_nr_cells (node, &nac, &nsc); +assert (ret ==0 ); +assert (nac ==3 && nsc ==2 ); + + prop =fdt_get_property (fdt, node,"ranges" , &len); +assert (prop !=NULL ); + + nr_range_cells = nac + nsc + nac_root; + nr_addr_spaces = (len /4 ) / nr_range_cells; + + host =malloc (sizeof (*host) + +sizeof (host->addr_space[0 ]) * nr_addr_spaces); +assert (host !=NULL ); + + host->start = base.addr; + host->size = base.size; + host->bus = bus;
lib/pci-host-generic.c line 168
Quoted 10 lines
lib/pci-host-generic.c new file +10
-0+ host->bus_max = bus_max; + host->nr_addr_spaces = nr_addr_spaces; + +pci_host_addr_space_init (&host->addr_space[0 ], nr_addr_spaces, + (fdt32_t *)prop->data, nr_range_cells); + +return host; +} + +static u64 pci_alloc_res (struct pci_host_bridge *host,int type,u64 size)
No reason to take 'host' as an argument. It's global and this function is not.
lib/pci-host-generic.c line 178
Quoted 10 lines
lib/pci-host-generic.c new file +10
-0+{ +struct pci_addr_space *as = &host->addr_space[0 ]; +phys_addr_t addr, mask; +int i; + +for (i =0 ; i < host->nr_addr_spaces; i++, as++) { +if (as->type == type) +break ; + } +assert (i < host->nr_addr_spaces);
I think this failure may justify an error message.
lib/pci-host-generic.c line 189
Quoted 11 lines
lib/pci-host-generic.c new file +11
-0+ +if (type &PCI_BASE_ADDRESS_SPACE_IO ) + mask =PCI_BASE_ADDRESS_IO_MASK ; +else + mask =PCI_BASE_ADDRESS_MEM_MASK ; + + size =ALIGN (size, mask); +assert (as->free + size <= as->size); + + addr = as->pci_start + as->free; + as->free += size;
Hmm, the name 'free' made me think it was pointing at the start of free memory, but it's just an offset. Maybe a name like free_offset would be better? Or initialize it to as->pci_start and use it as a free pointer? Whatever though, not a big deal.
lib/pci-host-generic.c line 194
Quoted 5 lines
lib/pci-host-generic.c new file +5
-0+ +return addr; +} + +static void pci_bus_scan (struct pci_host_bridge *host)
Same as above. No need for host to be a parameter.
lib/pci-host-generic.c line 205
Quoted 11 lines
lib/pci-host-generic.c new file +11
-0+{ +u32 cmd =PCI_COMMAND_SERR ; +int dev; +int bar; + +for (dev =0 ; dev <256 ; dev++) { +void __iomem *conf =pci_get_dev_conf (host, dev); + +/* We are only interested in normal PCI devices */ +if (readb (conf +PCI_HEADER_TYPE ) !=PCI_HEADER_TYPE_NORMAL ) +continue ;
you need to initialize cmd to PCI_COMMAND_SERR here, not above
Quoted 33 lines
lib/pci-host-generic.c new file +33
-0+ +for (bar =0 ; bar <6 ; bar++) { +u64 addr, size; +u32 type; + + size =pci_bar_size (dev, bar); +if (!size) +break ; + + type =readl (conf +PCI_BASE_ADDRESS_0 + (bar *4 )); + addr =pci_alloc_res (host, type, size); +pci_bar_set (dev, bar, addr); + +if (pci_bar_is_memory (dev, bar)) + cmd |=PCI_COMMAND_MEMORY ; +else + cmd |=PCI_COMMAND_IO ; + +if (pci_bar_is64 (dev, bar)) + bar++; + } + +writel (cmd, conf +PCI_COMMAND ); + } +} + +bool pci_probe (void ) +{ +assert (!pci_host_bridge); + pci_host_bridge =pci_dt_probe (); +if (!pci_host_bridge) +return false; +
lib/pci-host-generic.c line 248
Quoted 10 lines
lib/pci-host-generic.c new file +10
-0+pci_bus_scan (pci_host_bridge); + +return true; +} + +void pci_shutdown () +{ +int dev; + +assert (pci_host_bridge);
Instead of an assert I think
if (!pci_host_bridge)
return;
is ok here.
lib/pci-host-generic.c line 253
Quoted 5 lines
lib/pci-host-generic.c new file +5
-0+ +for (dev =0 ; dev <256 ; dev++) { +void __iomem *conf =pci_get_dev_conf (pci_host_bridge, dev); +if (readw (conf +PCI_VENDOR_ID ) == (u16 )~0 && +readw (conf +PCI_DEVICE_ID ) == (u16 )~0 )
here's where that dev-valid function would be nice again
lib/pci-host-generic.c line 262
Quoted 9 lines
lib/pci-host-generic.c new file +9
-0+continue ; +writel (PCI_COMMAND_INTX_DISABLE , conf +PCI_COMMAND ); + } + +free (pci_host_bridge); + pci_host_bridge =NULL ; +} + +void pci_print_arch ()
There's nothing arch-specific about this function. It should just be named pci_host_bridge_print()
Quoted 15 lines
lib/pci-host-generic.c new file +15
-0+{ +struct pci_host_bridge *host; +struct pci_addr_space *as; +char desc[8 ]; +int i; + +assert (pci_host_bridge); + host = pci_host_bridge; + as = &host->addr_space[0 ]; + +printf ("PCIe start %" PRIx64" size %" PRIx64" " +"bus %02x bus_max %02x #spaces %d\n\n" , + host->start, host->size, + host->bus, host->bus_max, host->nr_addr_spaces); +
lib/pci-host-generic.c line 287
Quoted 10 lines
lib/pci-host-generic.c new file +10
-0+for (i =0 ; i < host->nr_addr_spaces; i++, as++) { +pci_type_desc (as->type, desc,sizeof (desc)); +printf ("%s address space:\n" +"CPU start %" PRIx64" " +"PCI start %" PRIx64" size %" PRIx64"\n\n" , + desc, as->start, as->pci_start, as->size); + } +} + +phys_addr_t pci_xslate_addr(pcidevaddr_t __unused dev, u64 pci_addr)
This is pci_host_bridge_get_paddr(), or some such, also not arch specific, but rather host bridge specific.
Quoted 23 lines
lib/pci-host-generic.c new file +23
-0+{ +struct pci_host_bridge *host = pci_host_bridge; +struct pci_addr_space *as = &host->addr_space[0 ]; +int i; + +for (i =0 ; i < host->nr_addr_spaces; i++, as++) { +if (pci_addr >= as->pci_start && + pci_addr < as->pci_start + as->size) { +return as->start + (pci_addr - as->pci_start); + } + } + +return 0 ; +} + +#define PCI_OP_READ (type, op) \ +type pci_config_##op(pcidevaddr_t dev, u8 off) \ +{ \ + void __iomem *conf = pci_get_dev_conf(pci_host_bridge, dev); \ + return op(conf + off); \ +} + +#define PCI_OP_WRITE (type, op) \
lib/pci-host-generic.c line 320
Quoted 10 lines
lib/pci-host-generic.c new file +10
-0+void pci_config_##op(pcidevaddr_t dev, u8 off, type val) \ +{ \ + void __iomem *conf = pci_get_dev_conf(pci_host_bridge, dev); \ + op(val, conf + off); \ +} + +PCI_OP_READ(u8, readb) +PCI_OP_READ(u16, readw) +PCI_OP_READ(u32, readl) +PCI_OP_WRITE (u32, writel)
OK, but I'm not a big fan of function generation like this. If there's tons to do, it makes sense, but for a handful, I prefer duplication. The reason I don't like it is that you can't grep/cscope for the function name.
Quoted 50 lines
lib/pci-host-generic.c new file +1
-0+lib/pci-host-generic.h new file +43
-0@@ -0,0 +1,57 @@ +#ifndef PCI_HOST_GENERIC_H +#define PCI_HOST_GENERIC_H +/* +* PCI host bridge supporting structures and constants +* +* Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]> +* +* This work is licensed under the terms of the GNU LGPL, version 2. +*/ +#include "libcflat.h" + +struct pci_addr_space { +phys_addr_t pci_start; +phys_addr_t start; +phys_addr_t size; +phys_addr_t free; +int type; +}; + +struct pci_host_bridge { +phys_addr_t start; +phys_addr_t size; +int bus; +int bus_max; +int nr_addr_spaces; +struct pci_addr_space addr_space[]; +}; + +/* +* The following constants are derived from Linux, see this source: +* +* drivers/pci/host/pci-host-generic.c +* struct gen_pci_cfg_bus_ops::bus_shift +* int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) +* +* Documentation/devicetree/bindings/pci/host-generic-pci.txt excerpt: +* +* Configuration Space is assumed to be memory-mapped (as opposed to being +* accessed via an ioport) and laid out with a direct correspondence to the +* geography of a PCI bus address by concatenating the various components to +* form an offset. +* +* For CAM, this 24-bit offset is:
lib/pci-host-generic.h line 53
Quoted 10 lines
lib/pci-host-generic.h new file +10
-0+* +* cfg_offset(bus, device, function, register) = +* bus << 16 | device << 11 | function << 8 | register +* +* Whilst ECAM extends this by 4 bits to accommodate 4k of function space: +* +* cfg_offset(bus, device, function, register) = +* bus << 20 | device << 15 | function << 12 | register +* +*/
I like documentation, but the above could probably be condensed.
lib/pci-host-generic.h line 55
Quoted 2 lines
lib/pci-host-generic.h new file +2
-0+#define PCI_ECAM_BUS_SHIFT 20 +#define PCI_ECAM_DEVICE_SHIFT 12
Hmm, so you've named the function shift PCI_ECAM_*DEVICE*_SHIFT? Linux's gen_pci_map_cfg_bus_ecam() shifts by 12, like you do in pci_get_dev_conf(), but it doesn't shift something called dev, like you do, it shifts something called 'devfn'.
Thanks,
drew
Quoted 9 lines
lib/pci-host-generic.h new file +2
-0+ +#endif Signature · 6 lines
1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 13/15] pci/arm: Add generic ECAM host support
Quoted 1 line · Andrew Jones
Quoted 1 line
+static u64
pci_alloc_res(struct pci_host_bridge *host, int type, u64 size)
[...]
Quoted 10 lines
lib/pci-host-generic.c line 189
Quoted 5 lines
lib/pci-host-generic.c new file +5
-0+ size =ALIGN (size, mask); +assert (as->free + size <= as->size); + + addr = as->pci_start + as->free; + as->free += size;Hmm, the name 'free' made me think it was pointing at the start of free memory, but it's just an offset. Maybe a name like
free_offsetwould be better? Or initialize it toas->pci_startand use it as a free pointer? Whatever though, not a big deal.
It is also not an offset per se. It is rather number of already allocated bytes. What about "allocated"?
Quoted 6 lines
lib/pci-host-generic.c line 192
Quoted 3 lines
lib/pci-host-generic.c new file +3
-0+ +return addr; +}Thanks,
drew
Re: [PATCH RFC 13/15] pci/arm: Add generic ECAM host support
Quoted 16 lines · Alexander Gordeev
Quoted 1 line · Andrew Jones
Quoted 1 line
+static u64
pci_alloc_res(struct pci_host_bridge *host, int type, u64 size)[...]
Quoted 10 lines
lib/pci-host-generic.c line 189
Quoted 5 lines
lib/pci-host-generic.c new file +5
-0+ size =ALIGN (size, mask); +assert (as->free + size <= as->size); + + addr = as->pci_start + as->free; + as->free += size;Hmm, the name 'free' made me think it was pointing at the start of free memory, but it's just an offset. Maybe a name like
free_offsetwould be better? Or initialize it toas->pci_startand use it as a free pointer? Whatever though, not a big deal.
Quoted 2 lines
It is also not an offset per se. It is rather number of already allocated bytes. What about "allocated"?
allocated is better
Quoted 11 lines
Quoted 7 lines
lib/pci-host-generic.c line 192
Quoted 4 lines
lib/pci-host-generic.c new file +3
-0+ +return addr; +}Thanks,
drew--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 13/15] pci/arm: Add generic ECAM host support
In reply to Alexander GordeevQuoted 5 lines · Alexander Gordeev
arm/Makefile.common | 6 +- arm/pci-test.c | 23 ++++ lib/arm64/asm/pci.h | 29 +++++ lib/pci-host-generic.c | 321 +++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci-host-generic.h | 57 +++++++++
I thought may be it would be better to split this patch in two? First, introduce the host bridge and then enable it on ARM?
Thanks!
Quoted 1 line
Re: [PATCH RFC 13/15] pci/arm: Add generic ECAM host support
Quoted 9 lines · Alexander Gordeev
Quoted 5 lines · Alexander Gordeev
arm/Makefile.common | 6 +- arm/pci-test.c | 23 ++++ lib/arm64/asm/pci.h | 29 +++++ lib/pci-host-generic.c | 321 +++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci-host-generic.h | 57 +++++++++I thought may be it would be better to split this patch in two? First, introduce the host bridge and then enable it on ARM?
Agreed
drew
Quoted 8 lines
Thanks!
Quoted 1 line
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC PATCH 14/15 pci: Add pci-testdev PCI bus test device
In reply to Alexander Gordeev--- lib/pci-testdev.c | 188 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci.h | 7 ++ 2 files changed, 195 insertions(+) create mode 100644 lib/pci-testdev.c
lib/pci-testdev.c new file +188 -0
@@ -0,0 +1,188 @@ +/* +* QEMU "pci-testdev" PCI test device +* +* Copyright (C) 2016, 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" + +struct pci_testdev_ops { +u8 (*io_readb)(const volatile void *addr); +u16 (*io_readw)(const volatile void *addr); +u32 (*io_readl)(const volatile void *addr); +void (*io_writeb)(u8 value,volatile void *addr); +void (*io_writew)(u16 value,volatile void *addr); +void (*io_writel)(u32 value,volatile void *addr); +}; + +static u8 pio_readb (const volatile void *addr) +{ +return inb ((unsigned long )addr); +} + +static u16 pio_readw (const volatile void *addr) +{ +return inw ((unsigned long )addr); +} + +static u32 pio_readl (const volatile void *addr) +{ +return inl ((unsigned long )addr); +} + +static void pio_writeb (u8 value,volatile void *addr) +{ +outb (value, (unsigned long )addr); +} + +static void pio_writew (u16 value,volatile void *addr) +{ +outw (value, (unsigned long )addr); +} + +static void pio_writel (u32 value,volatile void *addr) +{ +outl (value, (unsigned long )addr); +} + +static struct pci_testdev_ops pci_testdev_io_ops = { + .io_readb = pio_readb, + .io_readw = pio_readw, + .io_readl = pio_readl, + .io_writeb = pio_writeb, + .io_writew = pio_writew, + .io_writel = pio_writel +}; + +static u8 mmio_readb (const volatile void *addr) +{ +return *(const volatile u8 __force *)addr; +} + +static u16 mmio_readw (const volatile void *addr) +{ +return *(const volatile u16 __force *)addr; +} + +static u32 mmio_readl (const volatile void *addr) +{ +return *(const volatile u32 __force *)addr; +} + +static void mmio_writeb (u8 value,volatile void *addr) +{ + *(volatile u8 __force *)addr = value; +} + +static void mmio_writew (u16 value,volatile void *addr) +{ + *(volatile u16 __force *)addr = value; +} + +static void mmio_writel (u32 value,volatile void *addr) +{ + *(volatile u32 __force *)addr = value; +} + +static struct pci_testdev_ops pci_testdev_mem_ops = { + .io_readb = mmio_readb, + .io_readw = mmio_readw, + .io_readl = mmio_readl, + .io_writeb = mmio_writeb, + .io_writew = mmio_writew, + .io_writel = mmio_writel +}; + +static bool pci_testdev_one (struct pci_test_dev_hdr *test, +int test_nr, +struct pci_testdev_ops *ops) +{ +u8 width; +u32 count, sig, off; +const int nr_writes =16 ; +int i; + + ops->io_writeb (test_nr, &test->test); + count = ops->io_readl (&test->count); +if (count !=0 ) +return false; + + width = ops->io_readb (&test->width); +if ((width !=1 ) && (width !=2 ) && (width !=4 )) +return false; + + sig = ops->io_readl (&test->data); + off = ops->io_readl (&test->offset); + +for (i =0 ; i < nr_writes; i++) { +switch (width) { +case 1 : ops->io_writeb (sig, (void *)test + off);break ; +case 2 : ops->io_writew (sig, (void *)test + off);break ; +case 4 : ops->io_writel (sig, (void *)test + off);break ; + } + } + +if ((int )ops->io_readl (&test->count) != nr_writes) +return false; + +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->io_readb (&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) +{ +int i; + +for (i =0 ;; i++) { +if (!pci_testdev_one (test, i, ops)) +break ; +pci_testdev_print (test, ops); + } + +return i; +} + +int pci_testdev (void ) +{ +phys_addr_t addr; +void __iomem *mem, *io; +pcidevaddr_t dev; +int nr_tests =0 ; + + dev =pci_find_dev (PCI_VENDOR_ID_REDHAT ,PCI_DEVICE_ID_REDHAT_TEST ); +if (dev ==PCIDEVADDR_INVALID ) +return -1 ; + +if (!pci_bar_is_valid (dev,0 ) || !pci_bar_is_valid (dev,1 )) +return -1 ; + + addr =pci_bar_addr (dev,1 ); + io = (void *)addr; + + addr =pci_bar_addr (dev,0 ); + mem =ioremap (addr,0 ); + + 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 +7 -0
@@ -25,6 +25,8 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);void pci_type_desc (int type,char *desc,int len);void pci_print (void ); +int pci_testdev (void ); +/* * pci-testdev is a driver for the pci-testdev qemu pci device. The * device enables testing mmio and portio exits, and measuring their
@@ -33,7 +35,12 @@ void pci_print(void);#define PCI_VENDOR_ID_REDHAT 0x1b36#define PCI_DEVICE_ID_REDHAT_TEST 0x0005 +/* +* pci-testdev supports at least three types of tests (via mmio and +* portio BARs): no-eventfd, wildcard-eventfd and datamatch-eventfd +*/ #define PCI_TESTDEV_NUM_BARS 2 +#define PCI_TESTDEV_NUM_TESTS 3struct pci_test_dev_hdr {uint8_t test;
Signature · 1 line
1.8.3.1
Re: [PATCH RFC 14/15] pci: Add pci-testdev PCI bus test device
Quoted 53 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci-testdev.c | 188 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci.h | 7 ++ 2 files changed, 195 insertions(+) create mode 100644 lib/pci-testdev.clib/pci-testdev.c new file +38
-0@@ -0,0 +1,188 @@ +/* +* QEMU "pci-testdev" PCI test device +* +* Copyright (C) 2016, 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" + +struct pci_testdev_ops { +u8 (*io_readb)(const volatile void *addr); +u16 (*io_readw)(const volatile void *addr); +u32 (*io_readl)(const volatile void *addr); +void (*io_writeb)(u8 value,volatile void *addr); +void (*io_writew)(u16 value,volatile void *addr); +void (*io_writel)(u32 value,volatile void *addr); +}; + +static u8 pio_readb (const volatile void *addr) +{ +return inb ((unsigned long )addr); +} + +static u16 pio_readw (const volatile void *addr) +{ +return inw ((unsigned long )addr); +} + +static u32 pio_readl (const volatile void *addr) +{ +return inl ((unsigned long )addr); +} + +static void pio_writeb (u8 value,volatile void *addr) +{ +outb (value, (unsigned long )addr); +}
lib/pci-testdev.c line 48
Quoted 10 lines
lib/pci-testdev.c new file +10
-0+ +static void pio_writew (u16 value,volatile void *addr) +{ +outw (value, (unsigned long )addr); +} + +static void pio_writel (u32 value,volatile void *addr) +{ +outl (value, (unsigned long )addr); +}
Ah, so the above is why you were attempting to add the io-accessor wrappers around mmio-accessors to asm-generic. Rather than do that, I think we can just allow the unit test to populate pci_testdev_io_ops. E.g. a unit test on ARM would populate it (I think) like
.io_readb = __raw_readb,
.io_readw = __raw_readw,
.io_readl = __raw_readl,
.io_writeb = __raw_writeb,
.io_writew = __raw_writew,
.io_writel = __raw_writel,while an x86 unit test would use ins and outs.
You could maybe even still provide the ops structs pre-populated, but you'll need to use some #ifdef __arm__ type of stuff then.
Quoted 72 lines
lib/pci-testdev.c new file +72
-0+ +static struct pci_testdev_ops pci_testdev_io_ops = { + .io_readb = pio_readb, + .io_readw = pio_readw, + .io_readl = pio_readl, + .io_writeb = pio_writeb, + .io_writew = pio_writew, + .io_writel = pio_writel +}; + +static u8 mmio_readb (const volatile void *addr) +{ +return *(const volatile u8 __force *)addr; +} + +static u16 mmio_readw (const volatile void *addr) +{ +return *(const volatile u16 __force *)addr; +} + +static u32 mmio_readl (const volatile void *addr) +{ +return *(const volatile u32 __force *)addr; +} + +static void mmio_writeb (u8 value,volatile void *addr) +{ + *(volatile u8 __force *)addr = value; +} + +static void mmio_writew (u16 value,volatile void *addr) +{ + *(volatile u16 __force *)addr = value; +} + +static void mmio_writel (u32 value,volatile void *addr) +{ + *(volatile u32 __force *)addr = value; +} + +static struct pci_testdev_ops pci_testdev_mem_ops = { + .io_readb = mmio_readb, + .io_readw = mmio_readw, + .io_readl = mmio_readl, + .io_writeb = mmio_writeb, + .io_writew = mmio_writew, + .io_writel = mmio_writel +}; + +static bool pci_testdev_one (struct pci_test_dev_hdr *test, +int test_nr, +struct pci_testdev_ops *ops) +{ +u8 width; +u32 count, sig, off; +const int nr_writes =16 ; +int i; + + ops->io_writeb (test_nr, &test->test); + count = ops->io_readl (&test->count); +if (count !=0 ) +return false; + + width = ops->io_readb (&test->width); +if ((width !=1 ) && (width !=2 ) && (width !=4 )) +return false; + + sig = ops->io_readl (&test->data); + off = ops->io_readl (&test->offset); + +for (i =0 ; i < nr_writes; i++) { +switch (width) {
lib/pci-testdev.c line 130
Quoted 10 lines
lib/pci-testdev.c new file +10
-0+case 1 : ops->io_writeb (sig, (void *)test + off);break ; +case 2 : ops->io_writew (sig, (void *)test + off);break ; +case 4 : ops->io_writel (sig, (void *)test + off);break ; + } + } + +if ((int )ops->io_readl (&test->count) != nr_writes) +return false; + +return true;
Or just
return (int)ops->io_readl(&test->count) != nr_writes;
I know you like it :-)
lib/pci-testdev.c line 136
Quoted 6 lines
lib/pci-testdev.c new file +6
-0+} + +void pci_testdev_print (struct pci_test_dev_hdr *test, +struct pci_testdev_ops *ops) +{ +bool io = (ops == &pci_testdev_io_ops);
nice :)
lib/pci-testdev.c line 147
Quoted 11 lines
lib/pci-testdev.c new file +11
-0+int i; + +printf ("pci-testdev %3s: " , io ?"io" :"mem" ); +for (i =0 ;; ++i) { +char c = ops->io_readb (&test->name[i]); +if (!c) +break ; +printf ("%c" , c); + } +printf ("\n" ); +
extra blank line here
Quoted 55 lines
lib/pci-testdev.c new file +41
-0+} + +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 ; +pci_testdev_print (test, ops); + } + +return i; +} + +int pci_testdev (void ) +{ +phys_addr_t addr; +void __iomem *mem, *io; +pcidevaddr_t dev; +int nr_tests =0 ; + + dev =pci_find_dev (PCI_VENDOR_ID_REDHAT ,PCI_DEVICE_ID_REDHAT_TEST ); +if (dev ==PCIDEVADDR_INVALID ) +return -1 ; + +if (!pci_bar_is_valid (dev,0 ) || !pci_bar_is_valid (dev,1 )) +return -1 ; + + addr =pci_bar_addr (dev,1 ); + io = (void *)addr; + + addr =pci_bar_addr (dev,0 ); + mem =ioremap (addr,0 ); + + 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 +2
-0@@ -25,6 +25,8 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);void pci_type_desc (int type,char *desc,int len);void pci_print (void ); +int pci_testdev (void ); +/* * pci-testdev is a driver for the pci-testdev qemu pci device. The * device enables testing mmio and portio exits, and measuring their
lib/pci.h void pci_print(void); · line 43
Quoted 9 lines
lib/pci.h +5
-0@@ -33,7 +35,12 @@ void pci_print(void);#define PCI_VENDOR_ID_REDHAT 0x1b36#define PCI_DEVICE_ID_REDHAT_TEST 0x0005 +/* +* pci-testdev supports at least three types of tests (via mmio and +* portio BARs): no-eventfd, wildcard-eventfd and datamatch-eventfd +*/ #define PCI_TESTDEV_NUM_BARS 2 +#define PCI_TESTDEV_NUM_TESTS 3
you don't seem to use this define anywhere. maybe the next patch
thanks,
drew
Quoted 10 lines
lib/pci.h +0
-0struct pci_test_dev_hdr {uint8_t test;Signature · 6 lines
1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 14/15] pci: Add pci-testdev PCI bus test device
Quoted 59 lines · Andrew Jones
Quoted 58 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci-testdev.c | 188 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci.h | 7 ++ 2 files changed, 195 insertions(+) create mode 100644 lib/pci-testdev.clib/pci-testdev.c new file +43
-0@@ -0,0 +1,188 @@ +/* +* QEMU "pci-testdev" PCI test device +* +* Copyright (C) 2016, 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" + +struct pci_testdev_ops { +u8 (*io_readb)(const volatile void *addr); +u16 (*io_readw)(const volatile void *addr); +u32 (*io_readl)(const volatile void *addr); +void (*io_writeb)(u8 value,volatile void *addr); +void (*io_writew)(u16 value,volatile void *addr); +void (*io_writel)(u32 value,volatile void *addr); +}; + +static u8 pio_readb (const volatile void *addr) +{ +return inb ((unsigned long )addr); +} + +static u16 pio_readw (const volatile void *addr) +{ +return inw ((unsigned long )addr); +} + +static u32 pio_readl (const volatile void *addr) +{ +return inl ((unsigned long )addr); +} + +static void pio_writeb (u8 value,volatile void *addr) +{ +outb (value, (unsigned long )addr); +} + +static void pio_writew (u16 value,volatile void *addr) +{ +outw (value, (unsigned long )addr); +}
Quoted 8 lines
lib/pci-testdev.c line 48
Quoted 5 lines
lib/pci-testdev.c new file +5
-0+ +static void pio_writel (u32 value,volatile void *addr) +{ +outl (value, (unsigned long )addr); +}Ah, so the above is why you were attempting to add the io-accessor wrappers around mmio-accessors to asm-generic. Rather than do that,
I also tried to unify io-accessors across archs Linux style.
Quoted 11 lines
I think we can just allow the unit test to populate
pci_testdev_io_ops. E.g. a unit test on ARM would populate it (I think) like.io_readb = __raw_readb, .io_readw = __raw_readw, .io_readl = __raw_readl, .io_writeb = __raw_writeb, .io_writew = __raw_writew, .io_writel = __raw_writel,
Quoted 4 lines
while an x86 unit test would use ins and outs.
You could maybe even still provide the ops structs pre-populated, but you'll need to use some #ifdef
__arm__type of stuff then.
Out of these two options the latter makes more sense to me, since type of access to PCI resources is derived from that resource type rather than from unit test parameters. And that deriving could be done "automatically".
So I would do the #ifdefs, but that will look like a worsened version of generic io-accessors ;)
So what is your preference here?
Quoted 190 lines
Quoted 71 lines
lib/pci-testdev.c new file +71
-0+static struct pci_testdev_ops pci_testdev_io_ops = { + .io_readb = pio_readb, + .io_readw = pio_readw, + .io_readl = pio_readl, + .io_writeb = pio_writeb, + .io_writew = pio_writew, + .io_writel = pio_writel +}; + +static u8 mmio_readb (const volatile void *addr) +{ +return *(const volatile u8 __force *)addr; +} + +static u16 mmio_readw (const volatile void *addr) +{ +return *(const volatile u16 __force *)addr; +} + +static u32 mmio_readl (const volatile void *addr) +{ +return *(const volatile u32 __force *)addr; +} + +static void mmio_writeb (u8 value,volatile void *addr) +{ + *(volatile u8 __force *)addr = value; +} + +static void mmio_writew (u16 value,volatile void *addr) +{ + *(volatile u16 __force *)addr = value; +} + +static void mmio_writel (u32 value,volatile void *addr) +{ + *(volatile u32 __force *)addr = value; +} + +static struct pci_testdev_ops pci_testdev_mem_ops = { + .io_readb = mmio_readb, + .io_readw = mmio_readw, + .io_readl = mmio_readl, + .io_writeb = mmio_writeb, + .io_writew = mmio_writew, + .io_writel = mmio_writel +}; + +static bool pci_testdev_one (struct pci_test_dev_hdr *test, +int test_nr, +struct pci_testdev_ops *ops) +{ +u8 width; +u32 count, sig, off; +const int nr_writes =16 ; +int i; + + ops->io_writeb (test_nr, &test->test); + count = ops->io_readl (&test->count); +if (count !=0 ) +return false; + + width = ops->io_readb (&test->width); +if ((width !=1 ) && (width !=2 ) && (width !=4 )) +return false; + + sig = ops->io_readl (&test->data); + off = ops->io_readl (&test->offset); + +for (i =0 ; i < nr_writes; i++) { +switch (width) {lib/pci-testdev.c line 130
Quoted 10 lines
lib/pci-testdev.c new file +10
-0+case 1 : ops->io_writeb (sig, (void *)test + off);break ; +case 2 : ops->io_writew (sig, (void *)test + off);break ; +case 4 : ops->io_writel (sig, (void *)test + off);break ; + } + } + +if ((int )ops->io_readl (&test->count) != nr_writes) +return false; + +return true;Or just
return (int)ops->io_readl(&test->count) != nr_writes;I know you like it :-)
lib/pci-testdev.c line 136
Quoted 6 lines
lib/pci-testdev.c new file +6
-0+} + +void pci_testdev_print (struct pci_test_dev_hdr *test, +struct pci_testdev_ops *ops) +{ +bool io = (ops == &pci_testdev_io_ops);nice :)
lib/pci-testdev.c line 147
Quoted 11 lines
lib/pci-testdev.c new file +11
-0+int i; + +printf ("pci-testdev %3s: " , io ?"io" :"mem" ); +for (i =0 ;; ++i) { +char c = ops->io_readb (&test->name[i]); +if (!c) +break ; +printf ("%c" , c); + } +printf ("\n" ); +extra blank line here
Quoted 55 lines
lib/pci-testdev.c new file +41
-0+} + +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 ; +pci_testdev_print (test, ops); + } + +return i; +} + +int pci_testdev (void ) +{ +phys_addr_t addr; +void __iomem *mem, *io; +pcidevaddr_t dev; +int nr_tests =0 ; + + dev =pci_find_dev (PCI_VENDOR_ID_REDHAT ,PCI_DEVICE_ID_REDHAT_TEST ); +if (dev ==PCIDEVADDR_INVALID ) +return -1 ; + +if (!pci_bar_is_valid (dev,0 ) || !pci_bar_is_valid (dev,1 )) +return -1 ; + + addr =pci_bar_addr (dev,1 ); + io = (void *)addr; + + addr =pci_bar_addr (dev,0 ); + mem =ioremap (addr,0 ); + + 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 +2
-0@@ -25,6 +25,8 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);void pci_type_desc (int type,char *desc,int len);void pci_print (void ); +int pci_testdev (void ); +/* * pci-testdev is a driver for the pci-testdev qemu pci device. The * device enables testing mmio and portio exits, and measuring their lib/pci.h void pci_print(void); · line 43
Quoted 9 lines
lib/pci.h +5
-0@@ -33,7 +35,12 @@ void pci_print(void);#define PCI_VENDOR_ID_REDHAT 0x1b36#define PCI_DEVICE_ID_REDHAT_TEST 0x0005 +/* +* pci-testdev supports at least three types of tests (via mmio and +* portio BARs): no-eventfd, wildcard-eventfd and datamatch-eventfd +*/ #define PCI_TESTDEV_NUM_BARS 2 +#define PCI_TESTDEV_NUM_TESTS 3you don't seem to use this define anywhere. maybe the next patch
thanks,
drewQuoted 10 lines
lib/pci.h +0
-0struct pci_test_dev_hdr {uint8_t test;Signature · 6 lines
1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 14/15] pci: Add pci-testdev PCI bus test device
Quoted 69 lines · Alexander Gordeev
Quoted 59 lines · Andrew Jones
Quoted 58 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- lib/pci-testdev.c | 188 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ lib/pci.h | 7 ++ 2 files changed, 195 insertions(+) create mode 100644 lib/pci-testdev.clib/pci-testdev.c new file +43
-0@@ -0,0 +1,188 @@ +/* +* QEMU "pci-testdev" PCI test device +* +* Copyright (C) 2016, 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" + +struct pci_testdev_ops { +u8 (*io_readb)(const volatile void *addr); +u16 (*io_readw)(const volatile void *addr); +u32 (*io_readl)(const volatile void *addr); +void (*io_writeb)(u8 value,volatile void *addr); +void (*io_writew)(u16 value,volatile void *addr); +void (*io_writel)(u32 value,volatile void *addr); +}; + +static u8 pio_readb (const volatile void *addr) +{ +return inb ((unsigned long )addr); +} + +static u16 pio_readw (const volatile void *addr) +{ +return inw ((unsigned long )addr); +} + +static u32 pio_readl (const volatile void *addr) +{ +return inl ((unsigned long )addr); +} + +static void pio_writeb (u8 value,volatile void *addr) +{ +outb (value, (unsigned long )addr); +} + +static void pio_writew (u16 value,volatile void *addr) +{ +outw (value, (unsigned long )addr); +}Quoted 8 lines
lib/pci-testdev.c line 48
Quoted 5 lines
lib/pci-testdev.c new file +5
-0+ +static void pio_writel (u32 value,volatile void *addr) +{ +outl (value, (unsigned long )addr); +}Ah, so the above is why you were attempting to add the io-accessor wrappers around mmio-accessors to asm-generic. Rather than do that,
Quoted 1 line
I also tried to unify io-accessors across archs Linux style.
Yeah, but not everything Linux does is nice :-)
Quoted 22 lines
Quoted 12 lines
I think we can just allow the unit test to populate
pci_testdev_io_ops. E.g. a unit test on ARM would populate it (I think) like.io_readb = __raw_readb, .io_readw = __raw_readw, .io_readl = __raw_readl, .io_writeb = __raw_writeb, .io_writew = __raw_writew, .io_writel = __raw_writel,Quoted 4 lines
while an x86 unit test would use ins and outs.
You could maybe even still provide the ops structs pre-populated, but you'll need to use some #ifdef
__arm__type of stuff then.Out of these two options the latter makes more sense to me, since type of access to PCI resources is derived from that resource type rather than from unit test parameters. And that deriving could be done "automatically".
Quoted 4 lines
So I would do the #ifdefs, but that will look like a worsened version of generic io-accessors ;)
So what is your preference here?
My latter suggestion, and I think I was wrong about needing ifdefs. Anyway, let's see how it turns out.
Quoted 195 lines
Quoted 179 lines
Quoted 72 lines
lib/pci-testdev.c new file +71
-0+static struct pci_testdev_ops pci_testdev_io_ops = { + .io_readb = pio_readb, + .io_readw = pio_readw, + .io_readl = pio_readl, + .io_writeb = pio_writeb, + .io_writew = pio_writew, + .io_writel = pio_writel +}; + +static u8 mmio_readb (const volatile void *addr) +{ +return *(const volatile u8 __force *)addr; +} + +static u16 mmio_readw (const volatile void *addr) +{ +return *(const volatile u16 __force *)addr; +} + +static u32 mmio_readl (const volatile void *addr) +{ +return *(const volatile u32 __force *)addr; +} + +static void mmio_writeb (u8 value,volatile void *addr) +{ + *(volatile u8 __force *)addr = value; +} + +static void mmio_writew (u16 value,volatile void *addr) +{ + *(volatile u16 __force *)addr = value; +} + +static void mmio_writel (u32 value,volatile void *addr) +{ + *(volatile u32 __force *)addr = value; +} + +static struct pci_testdev_ops pci_testdev_mem_ops = { + .io_readb = mmio_readb, + .io_readw = mmio_readw, + .io_readl = mmio_readl, + .io_writeb = mmio_writeb, + .io_writew = mmio_writew, + .io_writel = mmio_writel +}; + +static bool pci_testdev_one (struct pci_test_dev_hdr *test, +int test_nr, +struct pci_testdev_ops *ops) +{ +u8 width; +u32 count, sig, off; +const int nr_writes =16 ; +int i; + + ops->io_writeb (test_nr, &test->test); + count = ops->io_readl (&test->count); +if (count !=0 ) +return false; + + width = ops->io_readb (&test->width); +if ((width !=1 ) && (width !=2 ) && (width !=4 )) +return false; + + sig = ops->io_readl (&test->data); + off = ops->io_readl (&test->offset); + +for (i =0 ; i < nr_writes; i++) { +switch (width) {lib/pci-testdev.c line 130
Quoted 10 lines
lib/pci-testdev.c new file +10
-0+case 1 : ops->io_writeb (sig, (void *)test + off);break ; +case 2 : ops->io_writew (sig, (void *)test + off);break ; +case 4 : ops->io_writel (sig, (void *)test + off);break ; + } + } + +if ((int )ops->io_readl (&test->count) != nr_writes) +return false; + +return true;Or just
return (int)ops->io_readl(&test->count) != nr_writes;I know you like it :-)
lib/pci-testdev.c line 136
Quoted 6 lines
lib/pci-testdev.c new file +6
-0+} + +void pci_testdev_print (struct pci_test_dev_hdr *test, +struct pci_testdev_ops *ops) +{ +bool io = (ops == &pci_testdev_io_ops);nice :)
lib/pci-testdev.c line 147
Quoted 11 lines
lib/pci-testdev.c new file +11
-0+int i; + +printf ("pci-testdev %3s: " , io ?"io" :"mem" ); +for (i =0 ;; ++i) { +char c = ops->io_readb (&test->name[i]); +if (!c) +break ; +printf ("%c" , c); + } +printf ("\n" ); +extra blank line here
Quoted 55 lines
lib/pci-testdev.c new file +41
-0+} + +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 ; +pci_testdev_print (test, ops); + } + +return i; +} + +int pci_testdev (void ) +{ +phys_addr_t addr; +void __iomem *mem, *io; +pcidevaddr_t dev; +int nr_tests =0 ; + + dev =pci_find_dev (PCI_VENDOR_ID_REDHAT ,PCI_DEVICE_ID_REDHAT_TEST ); +if (dev ==PCIDEVADDR_INVALID ) +return -1 ; + +if (!pci_bar_is_valid (dev,0 ) || !pci_bar_is_valid (dev,1 )) +return -1 ; + + addr =pci_bar_addr (dev,1 ); + io = (void *)addr; + + addr =pci_bar_addr (dev,0 ); + mem =ioremap (addr,0 ); + + 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 +2
-0@@ -25,6 +25,8 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);void pci_type_desc (int type,char *desc,int len);void pci_print (void ); +int pci_testdev (void ); +/* * pci-testdev is a driver for the pci-testdev qemu pci device. The * device enables testing mmio and portio exits, and measuring their lib/pci.h void pci_print(void); · line 43
Quoted 9 lines
lib/pci.h +5
-0@@ -33,7 +35,12 @@ void pci_print(void);#define PCI_VENDOR_ID_REDHAT 0x1b36#define PCI_DEVICE_ID_REDHAT_TEST 0x0005 +/* +* pci-testdev supports at least three types of tests (via mmio and +* portio BARs): no-eventfd, wildcard-eventfd and datamatch-eventfd +*/ #define PCI_TESTDEV_NUM_BARS 2 +#define PCI_TESTDEV_NUM_TESTS 3you don't seem to use this define anywhere. maybe the next patch
Quoted 12 lines
thanks,
drewQuoted 10 lines
lib/pci.h +0
-0struct pci_test_dev_hdr {uint8_t test;Signature · 6 lines
1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 14/15] pci: Add pci-testdev PCI bus test device
Quoted 34 lines · Andrew Jones
Quoted 9 lines
Quoted 7 lines
lib/pci-testdev.c line 48
Quoted 4 lines
lib/pci-testdev.c new file +4
-0+static void pio_writel (u32 value,volatile void *addr) +{ +outl (value, (unsigned long )addr); +}Ah, so the above is why you were attempting to add the io-accessor wrappers around mmio-accessors to asm-generic. Rather than do that,
I also tried to unify io-accessors across archs Linux style.
Yeah, but not everything Linux does is nice :-)
Quoted 22 lines
Quoted 12 lines
I think we can just allow the unit test to populate
pci_testdev_io_ops. E.g. a unit test on ARM would populate it (I think) like.io_readb = __raw_readb, .io_readw = __raw_readw, .io_readl = __raw_readl, .io_writeb = __raw_writeb, .io_writew = __raw_writew, .io_writel = __raw_writel,Quoted 4 lines
while an x86 unit test would use ins and outs.
You could maybe even still provide the ops structs pre-populated, but you'll need to use some #ifdef
__arm__type of stuff then.Out of these two options the latter makes more sense to me, since type of access to PCI resources is derived from that resource type rather than from unit test parameters. And that deriving could be done "automatically".
Quoted 7 lines
Quoted 4 lines
So I would do the #ifdefs, but that will look like a worsened version of generic io-accessors ;)
So what is your preference here?
My latter suggestion, and I think I was wrong about needing ifdefs. Anyway, let's see how it turns out.
I am struggling to make it any better. Ifdefs would look less than nice indeed, but the only alternative I can think of is putting pci-testdev io accessors to <arch>/asm. But pci-testdev io accessors in asm would be just ugly, because there is nothing special about pci-testdev specific io-accessors. They are still arch's io accessors. So why not just make io accessors unified and Linux-style?
Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
lib/pci.h +2
-0@@ -25,6 +25,8 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);void pci_type_desc (int type,char *desc,int len);void pci_print (void ); +int pci_testdev (void ); +/* * pci-testdev is a driver for the pci-testdev qemu pci device. The * device enables testing mmio and portio exits, and measuring their @@ -33,7 +35,12 @@ void pci_print(void);#define PCI_VENDOR_ID_REDHAT 0x1b36#define PCI_DEVICE_ID_REDHAT_TEST 0x0005
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
lib/pci.h void pci_print(void); · line 43
Quoted 6 lines
lib/pci.h +5
-0+/* +* pci-testdev supports at least three types of tests (via mmio and +* portio BARs): no-eventfd, wildcard-eventfd and datamatch-eventfd +*/ #define PCI_TESTDEV_NUM_BARS 2 +#define PCI_TESTDEV_NUM_TESTS 3you don't seem to use this define anywhere. maybe the next patch
Yeah, PCI_TESTDEV_NUM_TESTS is used by the next patch. But it looks cleaner to me to introduce it here - as this patch kind of makes pci-testdev "complete". While the next patch is just pci-testdev enabler for ARM.
Leave it here or move?
Thanks!
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
thanks,
drew
Re: [PATCH RFC 14/15] pci: Add pci-testdev PCI bus test device
Quoted 43 lines · Alexander Gordeev
Quoted 34 lines · Andrew Jones
Quoted 9 lines
Quoted 7 lines
lib/pci-testdev.c line 48
Quoted 4 lines
lib/pci-testdev.c new file +4
-0+static void pio_writel (u32 value,volatile void *addr) +{ +outl (value, (unsigned long )addr); +}Ah, so the above is why you were attempting to add the io-accessor wrappers around mmio-accessors to asm-generic. Rather than do that,
I also tried to unify io-accessors across archs Linux style.
Yeah, but not everything Linux does is nice :-)
Quoted 22 lines
Quoted 12 lines
I think we can just allow the unit test to populate
pci_testdev_io_ops. E.g. a unit test on ARM would populate it (I think) like.io_readb = __raw_readb, .io_readw = __raw_readw, .io_readl = __raw_readl, .io_writeb = __raw_writeb, .io_writew = __raw_writew, .io_writel = __raw_writel,Quoted 4 lines
while an x86 unit test would use ins and outs.
You could maybe even still provide the ops structs pre-populated, but you'll need to use some #ifdef
__arm__type of stuff then.Out of these two options the latter makes more sense to me, since type of access to PCI resources is derived from that resource type rather than from unit test parameters. And that deriving could be done "automatically".
Quoted 7 lines
Quoted 4 lines
So I would do the #ifdefs, but that will look like a worsened version of generic io-accessors ;)
So what is your preference here?
My latter suggestion, and I think I was wrong about needing ifdefs. Anyway, let's see how it turns out.
Quoted 6 lines
I am struggling to make it any better. Ifdefs would look less than nice indeed, but the only alternative I can think of is putting pci-testdev io accessors to <arch>/asm. But pci-testdev io accessors in asm would be just ugly, because there is nothing special about pci-testdev specific io-accessors. They are still arch's io accessors. So why not just make io accessors unified and Linux-style?
OK, let's give the io function wrappers another round.
Quoted 27 lines
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
lib/pci.h +2
-0@@ -25,6 +25,8 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);void pci_type_desc (int type,char *desc,int len);void pci_print (void ); +int pci_testdev (void ); +/* * pci-testdev is a driver for the pci-testdev qemu pci device. The * device enables testing mmio and portio exits, and measuring their @@ -33,7 +35,12 @@ void pci_print(void);#define PCI_VENDOR_ID_REDHAT 0x1b36#define PCI_DEVICE_ID_REDHAT_TEST 0x0005Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
lib/pci.h void pci_print(void); · line 43
Quoted 6 lines
lib/pci.h +5
-0+/* +* pci-testdev supports at least three types of tests (via mmio and +* portio BARs): no-eventfd, wildcard-eventfd and datamatch-eventfd +*/ #define PCI_TESTDEV_NUM_BARS 2 +#define PCI_TESTDEV_NUM_TESTS 3you don't seem to use this define anywhere. maybe the next patch
Quoted 6 lines
Yeah,
PCI_TESTDEV_NUM_TESTSis used by the next patch. But it looks cleaner to me to introduce it here - as this patch kind of makes pci-testdev "complete". While the next patch is just pci-testdev enabler for ARM.Leave it here or move?
It's fine here.
Thanks,
drew
Quoted 9 lines
Thanks!
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
thanks,
drew--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC PATCH 15/15 pci/arm: Add pci-testdev PCI device operation test
In reply to Alexander Gordeev--- arm/Makefile.common | 1 + arm/pci-test.c | 4 ++++ arm/run | 10 +++++++++- 3 files changed, 14 insertions(+), 1 deletion(-)
arm/Makefile.common +1 -0
@@ -33,6 +33,7 @@ cflatobjs += lib/alloc.o cflatobjs += lib/devicetree.o cflatobjs += lib/pci.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
arm/pci-test.c +4 -0
@@ -17,6 +17,10 @@ int main(void)pci_print (); + ret =pci_testdev (); +report ("PCI test device passed %d tests" , + ret >=PCI_TESTDEV_NUM_BARS *PCI_TESTDEV_NUM_TESTS , ret); +pci_shutdown ();return report_summary ();
arm/run +9 -1
@@ -67,8 +67,16 @@ fi chr_testdev='-device virtio-serial-device' chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' +if ! $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then + pci_testdev="-device pci-testdev" + echo "$qpath doesn't support pci-testdev. Exiting." + exit 2 +fi + +pci_testdev='-device pci-testdev' + M+=",accel=$ACCEL" -command="$qemu $M -cpu $processor $chr_testdev" +command="$qemu $M -cpu $processor $chr_testdev $pci_testdev" command+=" -display none -serial stdio -kernel" command="$(timeout_cmd) $command" echo $command "$@"
Signature · 1 line
1.8.3.1
Re: [PATCH RFC 15/15] pci/arm: Add pci-testdev PCI device operation test
Quoted 41 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>Cc Andrew Jones <[email protected]>Signed-off-by Alexander Gordeev <[email protected]>--- arm/Makefile.common | 1 + arm/pci-test.c | 4 ++++ arm/run | 10 +++++++++- 3 files changed, 14 insertions(+), 1 deletion(-)arm/Makefile.common +1
-0@@ -33,6 +33,7 @@ cflatobjs += lib/alloc.o cflatobjs += lib/devicetree.o cflatobjs += lib/pci.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.oarm/pci-test.c +4
-0@@ -17,6 +17,10 @@ int main(void)pci_print (); + ret =pci_testdev (); +report ("PCI test device passed %d tests" , + ret >=PCI_TESTDEV_NUM_BARS *PCI_TESTDEV_NUM_TESTS , ret); +pci_shutdown ();return report_summary ();arm/run +0
-0
arm/run fi · line 70
Quoted 4 lines
arm/run +1
-0@@ -67,8 +67,16 @@ fi chr_testdev='-device virtio-serial-device' chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' +if ! $qemu $M -device '?' 2>&1 | grep pci-testdev > /dev/null; then
^extra space
arm/run fi · line 71
Quoted 1 line
arm/run +1
-0+ pci_testdev="-device pci-testdev"
pointless assignment (*IF* we want to bail out here, but...)
arm/run fi · line 73
Quoted 2 lines
arm/run +2
-0+ echo "$qpath doesn't support pci-testdev. Exiting." + exit 2
Why bail out? (The code I sent you didn't)
Users may want to run kvm-unit-tests with a QEMU that doesn't support pci-testdev. There will still be plenty (someday) of tests that don't require it, so we should proceed. Only the pci-test should fail, and it should fail with an informative message. Actually the unit test should probably make use of xfail, like this
have_testdev = pci_testdev_probe();
ret = pci_testdev_run();
report_xfail("PCI test device passed...", !have_testdev, ret, ...);Quoted 17 lines
arm/run +5
-1+fi + +pci_testdev='-device pci-testdev' + M+=",accel=$ACCEL" -command="$qemu $M -cpu $processor $chr_testdev" +command="$qemu $M -cpu $processor $chr_testdev $pci_testdev" command+=" -display none -serial stdio -kernel" command="$(timeout_cmd) $command" echo $command "$@"Signature · 6 lines
1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
thanks,
drew
Re: [PATCH RFC 15/15] pci/arm: Add pci-testdev PCI device operation test
Quoted 9 lines · Andrew Jones
Users may want to run kvm-unit-tests with a QEMU that doesn't support pci-testdev. There will still be plenty (someday) of tests that don't require it, so we should proceed. Only the pci-test should fail, and it should fail with an informative message. Actually the unit test should probably make use of xfail, like this
have_testdev = pci_testdev_probe(); ret = pci_testdev_run(); report_xfail("PCI test device passed...", !have_testdev, ret, ...);
A problem with this piece is pci_testdev_run() is called even when pci_testdev_probe() failed. What about this one instead?
int main(void)
{
int ret = pci_probe();
report("PCI bus probing", ret);
if (!ret)
goto done;
pci_print();
if (pci_find_dev(PCI_VENDOR_ID_REDHAT,
PCI_DEVICE_ID_REDHAT_TEST) == PCIDEVADDR_INVALID)
goto done;
ret = pci_testdev();
report("PCI test device passed %d tests",
ret >= PCI_TESTDEV_NUM_BARS * PCI_TESTDEV_NUM_TESTS, ret);done:
return report_summary(); }
Quoted 2 lines
thanks,
drew
Re: [PATCH RFC 15/15] pci/arm: Add pci-testdev PCI device operation test
Quoted 32 lines · Alexander Gordeev
Quoted 9 lines · Andrew Jones
Users may want to run kvm-unit-tests with a QEMU that doesn't support pci-testdev. There will still be plenty (someday) of tests that don't require it, so we should proceed. Only the pci-test should fail, and it should fail with an informative message. Actually the unit test should probably make use of xfail, like this
have_testdev = pci_testdev_probe(); ret = pci_testdev_run(); report_xfail("PCI test device passed...", !have_testdev, ret, ...);A problem with this piece is
pci_testdev_run()is called even whenpci_testdev_probe()failed. What about this one instead?int main(void)
{ int ret = pci_probe(); report("PCI bus probing", ret); if (!ret) goto done; pci_print(); if (pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST) == PCIDEVADDR_INVALID) goto done; ret = pci_testdev(); report("PCI test device passed %d tests", ret >= PCI_TESTDEV_NUM_BARS * PCI_TESTDEV_NUM_TESTS, ret);
Quoted 3 lines
done:
return report_summary(); }
Yup, that's fine.
drew
Quoted 7 lines
Quoted 3 lines
thanks,
drew--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 15/15] pci/arm: Add pci-testdev PCI device operation test
Quoted 9 lines · Andrew Jones
Quoted 9 lines
int main(void)
{ int ret = pci_probe(); report("PCI bus probing", ret); if (!ret) goto done; pci_print();
Would you like to make this dump optional?
I.e. with --verbose command line argument?
Quoted 15 lines
Quoted 11 lines
if (pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST) == PCIDEVADDR_INVALID) goto done; ret = pci_testdev(); report("PCI test device passed %d tests", ret >= PCI_TESTDEV_NUM_BARS * PCI_TESTDEV_NUM_TESTS, ret);done:
return report_summary(); }Yup, that's fine.
drew
Re: [PATCH RFC 15/15] pci/arm: Add pci-testdev PCI device operation test
Quoted 5 lines · Alexander Gordeev
Quoted 4 lines · Andrew Jones
Quoted 4 lines
int main(void)
{ int ret = pci_probe();
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
report("PCI bus probing", ret); if (!ret) goto done; pci_print();Would you like to make this dump optional?
I.e. with --verbose command line argument?
Not sure yet. We can change it later.
Quoted 20 lines
Quoted 9 lines
Quoted 9 lines
if (pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST) == PCIDEVADDR_INVALID) goto done; ret = pci_testdev(); report("PCI test device passed %d tests", ret >= PCI_TESTDEV_NUM_BARS * PCI_TESTDEV_NUM_TESTS, ret);Quoted 7 lines
Quoted 3 lines
done:
return report_summary(); }Yup, that's fine.
drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html