pci: Various PCI BAR checks

PATCH Superseded Alexander Gordeev 2 participants 12 messages Nov 29, 2016 – Dec 22, 2016

Subsystems (2)

KVM Library code

lore

Share & tools

lore

Alexander Gordeev

author

2016-11-29 14:48 UTC #0 lore raw

cover pci: Various PCI BAR checks

Hi Andrew,

I would consider making PCI BAR number an unsigned integer in all existing APIs. But for now assert(bar_num >= 0 ...) is checked.

Sources are at https://github.com/a-gordeev/kvm-unit-tests.git pci-fixes

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Alexander Gordeev (4):
Move INVALID_PHYS_ADDR macro to lib/libcflat.h
pci: Assert when PCI bus address can not be translated
pci: Sanity check PCI device BAR numbers
pci: Do not set or get addresses for unimplemented BARs

 lib/alloc.h            |  2 --
 lib/libcflat.h         |  1 +
 lib/pci-host-generic.c |  2 +-
 lib/pci.c              | 34 +++++++++++++++++++++++++++++-----
 4 files changed, 31 insertions(+), 8 deletions(-)

Signature · 1 line

1.8.3.1

Alexander Gordeev

author

2016-11-29 14:48 UTC #1 lore raw

PATCH 1/4 Move INVALID_PHYS_ADDR macro to lib/libcflat.h

It probably should have been done with commit 7d764581 ("Move phys_addr_t type definition to lib/libcflat.h")

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/alloc.h            | 2 --
 lib/libcflat.h         | 1 +
 lib/pci-host-generic.c | 2 +-
 3 files changed, 2 insertions(+), 3 deletions(-)

lib/alloc.h +0 -2

index c12bd15f7afc..81f5369c9283 100644
@@ -58,8 +58,6 @@ static inline void *memalign(size_t alignment, size_t size)
 	return alloc_ops->memalign(alignment, size);
 }
 
-#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/libcflat.h +1 -0

index c622198677c1..05481f948795 100644
@@ -61,6 +61,7 @@ typedef _Bool		bool;
 #define PRIxPTR __PRIPTR_PREFIX	"x"
 
 typedef u64			phys_addr_t;
+#define INVALID_PHYS_ADDR	(~(phys_addr_t)0)
 
 extern void puts(const char *s);
 extern void exit(int code);

lib/pci-host-generic.c +1 -1

index 7f72d649144e..4263365e8288 100644
@@ -271,7 +271,7 @@ phys_addr_t pci_host_bridge_get_paddr(u64 pci_addr)
 		as++;
 	}
 
-	return ~0;
+	return INVALID_PHYS_ADDR;
 }
 
 static void __iomem *pci_get_dev_conf(struct pci_host_bridge *host, int devfn)

Signature · 1 line

1.8.3.1

Andrew Jones

2016-11-30 12:37 UTC #2 lore raw

Re: [kvm-unit-tests PATCH 1/4] Move INVALID_PHYS_ADDR macro to lib/libcflat.h

Quoted 2 lines · Alexander Gordeev

It probably should have been done with commit 7d764581 ("Move phys_addr_t type definition to lib/libcflat.h")

The best way would be one patch for the move and one patch for the change (and no 7d764581). I think we're OK with just squashing this one into 7d764581 (actually 721c18d in arm/next) though. I'll do that.

Thanks,
drew

Quoted 55 lines

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/alloc.h            | 2 --
 lib/libcflat.h         | 1 +
 lib/pci-host-generic.c | 2 +-
 3 files changed, 2 insertions(+), 3 deletions(-)

lib/alloc.h +0 -2

index c12bd15f7afc..81f5369c9283 100644
@@ -58,8 +58,6 @@ static inline void *memalign(size_t alignment, size_t size)
 	return alloc_ops->memalign(alignment, size);
 }
 
-#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/libcflat.h +1 -0

index c622198677c1..05481f948795 100644
@@ -61,6 +61,7 @@ typedef _Bool		bool;
 #define PRIxPTR __PRIPTR_PREFIX	"x"
 
 typedef u64			phys_addr_t;
+#define INVALID_PHYS_ADDR	(~(phys_addr_t)0)
 
 extern void puts(const char *s);
 extern void exit(int code);

lib/pci-host-generic.c +1 -1

index 7f72d649144e..4263365e8288 100644
@@ -271,7 +271,7 @@ phys_addr_t pci_host_bridge_get_paddr(u64 pci_addr)
 		as++;
 	}
 
-	return ~0;
+	return INVALID_PHYS_ADDR;
 }
 
 static void __iomem *pci_get_dev_conf(struct pci_host_bridge *host, int devfn)

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

Alexander Gordeev

author

2016-11-29 14:48 UTC #3 lore raw

PATCH 2/4 pci: Assert when PCI bus address can not be translated

In reply to Alexander Gordeev

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

lib/pci.c +5 -1

index 6bd54cbac1bb..fdd88296f0ae 100644
@@ -43,11 +43,15 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
 	uint32_t bar = pci_bar_get(dev, bar_num);
 	uint32_t mask = pci_bar_mask(bar);
 	uint64_t addr = bar & mask;
+	phys_addr_t phys_addr;
 
 	if (pci_bar_is64(dev, bar_num))
 		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
 
-	return pci_translate_addr(dev, addr);
+	phys_addr = pci_translate_addr(dev, addr);
+	assert(phys_addr != INVALID_PHYS_ADDR);
+
+	return phys_addr;
 }
 
 void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)

Signature · 1 line

1.8.3.1

Andrew Jones

2016-11-30 12:45 UTC #4 lore raw

Re: [kvm-unit-tests PATCH 2/4] pci: Assert when PCI bus address can not be translated

Quoted 31 lines · Alexander Gordeev

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

lib/pci.c +5 -1

index 6bd54cbac1bb..fdd88296f0ae 100644
@@ -43,11 +43,15 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
 	uint32_t bar = pci_bar_get(dev, bar_num);
 	uint32_t mask = pci_bar_mask(bar);
 	uint64_t addr = bar & mask;
+	phys_addr_t phys_addr;
 
 	if (pci_bar_is64(dev, bar_num))
 		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
 
-	return pci_translate_addr(dev, addr);
+	phys_addr = pci_translate_addr(dev, addr);
+	assert(phys_addr != INVALID_PHYS_ADDR);
+
+	return phys_addr;
 }
 
 void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)

Signature · 1 line

1.8.3.1

This patch needs a commit message with the justification for the assert. I'll add

Failing to translate the PCI address obtained from the bar means something isn't right with the PCI setup. As this should never happen, assert when it does.

Thanks,
drew

Alexander Gordeev

author

2016-11-29 14:49 UTC #5 lore raw

PATCH 3/4 pci: Sanity check PCI device BAR numbers

In reply to Alexander Gordeev

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

lib/pci.c +14 -4

index fdd88296f0ae..953810d14334 100644
@@ -35,6 +35,7 @@ uint32_t pci_bar_mask(uint32_t bar)
 
 uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
 {
+	assert(bar_num >= 0 && bar_num < 6);
 	return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
 }

@@ -56,11 +57,12 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
 
 void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
 {
+	bool is64 = pci_bar_is64(dev, bar_num);
 	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
 
 	pci_config_writel(dev, off, (uint32_t)addr);
 
-	if (pci_bar_is64(dev, bar_num))
+	if (is64)
 		pci_config_writel(dev, off + 4, (uint32_t)(addr >> 32));
 }

@@ -76,9 +78,12 @@ void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
  */
 static uint32_t pci_bar_size_helper(pcidevaddr_t dev, int bar_num)
 {
-	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
+	int off;
 	uint32_t bar, val;
 
+	assert(bar_num >= 0 && bar_num < 6);
+
+	off = PCI_BASE_ADDRESS_0 + bar_num * 4;
 	bar = pci_config_readl(dev, off);
 	pci_config_writel(dev, off, ~0u);
 	val = pci_config_readl(dev, off);

@@ -127,8 +132,13 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
 	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
 		return false;
 
-	return (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
-		      PCI_BASE_ADDRESS_MEM_TYPE_64;
+	if ((bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
+		   PCI_BASE_ADDRESS_MEM_TYPE_64) {
+		assert(bar_num < 5);
+		return true;
+	}
+
+	return false;
 }
 
 void pci_bar_print(pcidevaddr_t dev, int bar_num)

Signature · 1 line

1.8.3.1

Andrew Jones

2016-11-30 13:26 UTC #6 lore raw

Re: [kvm-unit-tests PATCH 3/4] pci: Sanity check PCI device BAR numbers

Quoted 34 lines · Alexander Gordeev

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

lib/pci.c +3 -1

index fdd88296f0ae..953810d14334 100644
@@ -35,6 +35,7 @@ uint32_t pci_bar_mask(uint32_t bar)
 
 uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
 {
+	assert(bar_num >= 0 && bar_num < 6);
 	return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
 }

@@ -56,11 +57,12 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
 
 void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
 {
+	bool is64 = pci_bar_is64(dev, bar_num);
 	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
 
 	pci_config_writel(dev, off, (uint32_t)addr);
 
-	if (pci_bar_is64(dev, bar_num))
+	if (is64)
 		pci_config_writel(dev, off + 4, (uint32_t)(addr >> 32));
 }

lib/pci.c void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr) · line 86

Quoted 10 lines

lib/pci.c +4 -1

@@ -76,9 +78,12 @@ void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
  */
 static uint32_t pci_bar_size_helper(pcidevaddr_t dev, int bar_num)
 {
-	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
+	int off;
 	uint32_t bar, val;
 
+	assert(bar_num >= 0 && bar_num < 6);
+
+	off = PCI_BASE_ADDRESS_0 + bar_num * 4;

No need to move the assignment of off down.

Quoted 22 lines

lib/pci.c +7 -2

 	bar = pci_config_readl(dev, off);
 	pci_config_writel(dev, off, ~0u);
 	val = pci_config_readl(dev, off);

@@ -127,8 +132,13 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
 	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
 		return false;
 
-	return (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
-		      PCI_BASE_ADDRESS_MEM_TYPE_64;
+	if ((bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
+		   PCI_BASE_ADDRESS_MEM_TYPE_64) {
+		assert(bar_num < 5);
+		return true;
+	}
+
+	return false;
 }
 
 void pci_bar_print(pcidevaddr_t dev, int bar_num)

Signature · 1 line

1.8.3.1

I think I'd rather leave pci_bar_is64() alone and instead teach pci_bar_is_valid() more about validating bars. Then scatter more calls to that around. pci_bar_is_valid doesn't look right to me anyway. It currently just says non-zero valid, zero not valid. What if you're querying the second bar of a 64-bit bar? Or what if the PCI base address is supposed to be zero, like it is for the PIO region?

How about the following instead?

bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)

 {
     assert(bar_num >= 0 && bar_num < 6);
     assert(!(bar_num == 5 && pci_bar_is64(dev, bar_num)));

     if (bar_num > 0 && pci_bar_is64(dev, bar_num - 1)) {
         assert(!pci_bar_is64(dev, bar_num));
         return pci_bar_is_valid(dev, bar_num - 1);
     }

     return pci_bar_size(dev, bar_num);

}

Thanks,
drew

Alexander Gordeev

author

2016-11-29 14:49 UTC #7 lore raw

PATCH 4/4 pci: Do not set or get addresses for unimplemented BARs

In reply to Alexander Gordeev

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

lib/pci.c +11 -1

index 953810d14334..cb9fc0d86630 100644
@@ -44,11 +44,16 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
 	uint32_t bar = pci_bar_get(dev, bar_num);
 	uint32_t mask = pci_bar_mask(bar);
 	uint64_t addr = bar & mask;
-	phys_addr_t phys_addr;
+	phys_addr_t phys_addr, size;
 
 	if (pci_bar_is64(dev, bar_num))
 		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
 
+	size = pci_bar_size(dev, bar_num);
+	assert(size);
+	if (!size)
+		return INVALID_PHYS_ADDR;
+
 	phys_addr = pci_translate_addr(dev, addr);
 	assert(phys_addr != INVALID_PHYS_ADDR);

@@ -58,8 +63,13 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
 void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
 {
 	bool is64 = pci_bar_is64(dev, bar_num);
+	phys_addr_t size = pci_bar_size(dev, bar_num);
 	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
 
+	assert(size);
+	if (!size)
+		return;
+
 	pci_config_writel(dev, off, (uint32_t)addr);
 
 	if (is64)

Signature · 1 line

1.8.3.1

Andrew Jones

2016-11-30 13:48 UTC #8 lore raw

Re: [kvm-unit-tests PATCH 4/4] pci: Do not set or get addresses for unimplemented BARs

Quoted 16 lines · Alexander Gordeev

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

lib/pci.c +0 -1

index 953810d14334..cb9fc0d86630 100644
@@ -44,11 +44,16 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
 	uint32_t bar = pci_bar_get(dev, bar_num);
 	uint32_t mask = pci_bar_mask(bar);
 	uint64_t addr = bar & mask;
-	phys_addr_t phys_addr;

lib/pci.c phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num) · line 56

Quoted 10 lines

lib/pci.c +6 -0

+	phys_addr_t phys_addr, size;
 
 	if (pci_bar_is64(dev, bar_num))
 		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
 
+	size = pci_bar_size(dev, bar_num);
+	assert(size);
+	if (!size)
+		return INVALID_PHYS_ADDR;
+

You're both asserting size is non-zero and returning something when it is... You only want the later. It's quite feasible that a unit test would want to probe bars by attempting get_addrs on each, checking for a valid one.

So, with the new pci_bar_is_valid() I propose, you should just do

if (!pci_bar_is_valid(dev, bar_num))

      return INVALID_PHYS_ADDR;

lib/pci.c phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num) · line 71

Quoted 13 lines

lib/pci.c +4 -0

 	phys_addr = pci_translate_addr(dev, addr);
 	assert(phys_addr != INVALID_PHYS_ADDR);

@@ -58,8 +63,13 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
 void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
 {
 	bool is64 = pci_bar_is64(dev, bar_num);
+	phys_addr_t size = pci_bar_size(dev, bar_num);
 	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
 
+	assert(size);
+	if (!size)
+		return;

Again, both an assert and a return. Here I think asserting that the bar is valid is reasonable.

Quoted 7 lines

lib/pci.c +1 -0

+
 	pci_config_writel(dev, off, (uint32_t)addr);
 
 	if (is64)

Signature · 1 line

1.8.3.1

There are a few other functions (pci_alloc_resource, pci_bar_print, pci_bar_size) that do validity checking by looking for zero size. We should replace those with the new, improved validity check as well.

Thanks,
drew

Andrew Jones

2016-11-30 13:52 UTC #9 lore raw

Re: [kvm-unit-tests PATCH 4/4] pci: Do not set or get addresses for unimplemented BARs

Quoted 58 lines · Andrew Jones

Quoted 16 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>
Cc Andrew Jones <[email protected]>
Signed-off-by Alexander Gordeev <[email protected]>
---
 lib/pci.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
lib/pci.c +0 -1
index 953810d14334..cb9fc0d86630 100644
@@ -44,11 +44,16 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
 	uint32_t bar = pci_bar_get(dev, bar_num);
 	uint32_t mask = pci_bar_mask(bar);
 	uint64_t addr = bar & mask;
-	phys_addr_t phys_addr;
lib/pci.c phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num) · line 56
Quoted 10 lines
lib/pci.c +6 -0
+	phys_addr_t phys_addr, size;
 
 	if (pci_bar_is64(dev, bar_num))
 		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
 
+	size = pci_bar_size(dev, bar_num);
+	assert(size);
+	if (!size)
+		return INVALID_PHYS_ADDR;
+
You're both asserting size is non-zero and returning something when it is... You only want the later. It's quite feasible that a unit test would want to probe bars by attempting get_addrs on each, checking for a valid one.
So, with the new pci_bar_is_valid() I propose, you should just do
if (!pci_bar_is_valid(dev, bar_num))
      return INVALID_PHYS_ADDR;
lib/pci.c phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num) · line 71
Quoted 13 lines
lib/pci.c +4 -0
 	phys_addr = pci_translate_addr(dev, addr);
 	assert(phys_addr != INVALID_PHYS_ADDR);
 
@@ -58,8 +63,13 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
 void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
 {
 	bool is64 = pci_bar_is64(dev, bar_num);
+	phys_addr_t size = pci_bar_size(dev, bar_num);
 	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
 
+	assert(size);
+	if (!size)
+		return;
Again, both an assert and a return. Here I think asserting that the bar is valid is reasonable.
Quoted 3 lines
lib/pci.c +1 -0
+
 	pci_config_writel(dev, off, (uint32_t)addr);
 

Quoted 8 lines

Quoted 4 lines
lib/pci.c +0 -0
 	if (is64)
Signature · 1 line
1.8.3.1
There are a few other functions (pci_alloc_resource, pci_bar_print, pci_bar_size) that do validity checking by looking for zero size. We should replace those with the new, improved validity check as well.

Oops, can't call the new validity function from pci_bar_size. That would recurse. Please add a comment to pci_bar_size stating that.

Quoted 7 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

Andrew Jones

2016-11-30 13:57 UTC #10 lore raw

Re: [kvm-unit-tests PATCH 4/4] pci: Do not set or get addresses for unimplemented BARs

Quoted 64 lines · Andrew Jones

Quoted 63 lines · Andrew Jones
Quoted 16 lines · Alexander Gordeev
Cc Thomas Huth <[email protected]>
Cc Andrew Jones <[email protected]>
Signed-off-by Alexander Gordeev <[email protected]>
---
 lib/pci.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
lib/pci.c +0 -1
index 953810d14334..cb9fc0d86630 100644
@@ -44,11 +44,16 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
 	uint32_t bar = pci_bar_get(dev, bar_num);
 	uint32_t mask = pci_bar_mask(bar);
 	uint64_t addr = bar & mask;
-	phys_addr_t phys_addr;
lib/pci.c phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num) · line 56
Quoted 10 lines
lib/pci.c +6 -0
+	phys_addr_t phys_addr, size;
 
 	if (pci_bar_is64(dev, bar_num))
 		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
 
+	size = pci_bar_size(dev, bar_num);
+	assert(size);
+	if (!size)
+		return INVALID_PHYS_ADDR;
+
You're both asserting size is non-zero and returning something when it is... You only want the later. It's quite feasible that a unit test would want to probe bars by attempting get_addrs on each, checking for a valid one.
So, with the new pci_bar_is_valid() I propose, you should just do
if (!pci_bar_is_valid(dev, bar_num))
      return INVALID_PHYS_ADDR;
lib/pci.c phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num) · line 71
Quoted 13 lines
lib/pci.c +4 -0
 	phys_addr = pci_translate_addr(dev, addr);
 	assert(phys_addr != INVALID_PHYS_ADDR);
 
@@ -58,8 +63,13 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
 void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
 {
 	bool is64 = pci_bar_is64(dev, bar_num);
+	phys_addr_t size = pci_bar_size(dev, bar_num);
 	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
 
+	assert(size);
+	if (!size)
+		return;
Again, both an assert and a return. Here I think asserting that the bar is valid is reasonable.
Quoted 7 lines
lib/pci.c +1 -0
+
 	pci_config_writel(dev, off, (uint32_t)addr);
 
 	if (is64)
Signature · 1 line
1.8.3.1

Quoted 6 lines

Quoted 3 lines
There are a few other functions (pci_alloc_resource, pci_bar_print, pci_bar_size) that do validity checking by looking for zero size. We should replace those with the new, improved validity check as well.
Oops, can't call the new validity function from pci_bar_size. That would recurse. Please add a comment to pci_bar_size stating that.

Actually you can just call pci_bar_size_helper from pci_bar_is_valid.

Quoted 12 lines

Quoted 8 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
--
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

Andrew Jones

2016-12-22 13:05 UTC #11 lore raw

In reply to Alexander Gordeev

Alex,

are you going to send a v2 of this?

Thanks,
drew

Quoted 29 lines · Alexander Gordeev

Hi Andrew,
I would consider making PCI BAR number an unsigned integer in all existing APIs. But for now assert(bar_num >= 0 ...) is checked.
Sources are at https://github.com/a-gordeev/kvm-unit-tests.git pci-fixes
Cc Thomas Huth <[email protected]>
Cc Andrew Jones <[email protected]>
Alexander Gordeev (4):
Move INVALID_PHYS_ADDR macro to lib/libcflat.h
pci: Assert when PCI bus address can not be translated
pci: Sanity check PCI device BAR numbers
pci: Do not set or get addresses for unimplemented BARs
 lib/alloc.h            |  2 --
 lib/libcflat.h         |  1 +
 lib/pci-host-generic.c |  2 +-
 lib/pci.c              | 34 +++++++++++++++++++++++++++++-----
 4 files changed, 31 insertions(+), 8 deletions(-)
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