[kvm-unit-tests PATCH 0/4] pci: Various PCI BAR checks

[kvm-unit-tests PATCH 0/4] pci: Various PCI BAR checks

[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 <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>

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(-)

-- 
1.8.3.1

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

[Alexander Gordeev]

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

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/alloc.h            | 2 --
 lib/libcflat.h         | 1 +
 lib/pci-host-generic.c | 2 +-
 3 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/alloc.h b/lib/alloc.h
index c12bd15f7afc..81f5369c9283 100644
--- a/lib/alloc.h
+++ b/lib/alloc.h
@@ -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.
diff --git a/lib/libcflat.h b/lib/libcflat.h
index c622198677c1..05481f948795 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -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);
diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
index 7f72d649144e..4263365e8288 100644
--- a/lib/pci-host-generic.c
+++ b/lib/pci-host-generic.c
@@ -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)
-- 
1.8.3.1

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

[Andrew Jones]

On Tue, Nov 29, 2016 at 03:48:50PM +0100, Alexander Gordeev wrote:
> 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

> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/alloc.h            | 2 --
>  lib/libcflat.h         | 1 +
>  lib/pci-host-generic.c | 2 +-
>  3 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/alloc.h b/lib/alloc.h
> index c12bd15f7afc..81f5369c9283 100644
> --- a/lib/alloc.h
> +++ b/lib/alloc.h
> @@ -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.
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index c622198677c1..05481f948795 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -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);
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> index 7f72d649144e..4263365e8288 100644
> --- a/lib/pci-host-generic.c
> +++ b/lib/pci-host-generic.c
> @@ -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)
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Alexander Gordeev]

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/pci.c b/lib/pci.c
index 6bd54cbac1bb..fdd88296f0ae 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -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)
-- 
1.8.3.1

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

[Andrew Jones]

On Tue, Nov 29, 2016 at 03:48:51PM +0100, Alexander Gordeev wrote:
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index 6bd54cbac1bb..fdd88296f0ae 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -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)
> -- 
> 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

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

[Alexander Gordeev]

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/lib/pci.c b/lib/pci.c
index fdd88296f0ae..953810d14334 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -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)
-- 
1.8.3.1

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

[Andrew Jones]

On Tue, Nov 29, 2016 at 03:48:52PM +0100, Alexander Gordeev wrote:
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index fdd88296f0ae..953810d14334 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -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;

No need to move the assignment of off down.

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

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

[Alexander Gordeev]

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/lib/pci.c b/lib/pci.c
index 953810d14334..cb9fc0d86630 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -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)
-- 
1.8.3.1

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

[Andrew Jones]

On Tue, Nov 29, 2016 at 03:48:53PM +0100, Alexander Gordeev wrote:
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index 953810d14334..cb9fc0d86630 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -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;
> +

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;

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

> +
>  	pci_config_writel(dev, off, (uint32_t)addr);
>  
>  	if (is64)
> -- 
> 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

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

[Andrew Jones]

On Wed, Nov 30, 2016 at 02:47:59PM +0100, Andrew Jones wrote:
> On Tue, Nov 29, 2016 at 03:48:53PM +0100, Alexander Gordeev wrote:
> > Cc: Thomas Huth <thuth@redhat.com>
> > Cc: Andrew Jones <drjones@redhat.com>
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > ---
> >  lib/pci.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/pci.c b/lib/pci.c
> > index 953810d14334..cb9fc0d86630 100644
> > --- a/lib/pci.c
> > +++ b/lib/pci.c
> > @@ -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;
> > +
> 
> 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;
> 
> >  	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.
> 
> > +
> >  	pci_config_writel(dev, off, (uint32_t)addr);
> >  
> >  	if (is64)
> > -- 
> > 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.

> 
> Thanks,
> drew
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andrew Jones]

On Wed, Nov 30, 2016 at 02:52:53PM +0100, Andrew Jones wrote:
> On Wed, Nov 30, 2016 at 02:47:59PM +0100, Andrew Jones wrote:
> > On Tue, Nov 29, 2016 at 03:48:53PM +0100, Alexander Gordeev wrote:
> > > Cc: Thomas Huth <thuth@redhat.com>
> > > Cc: Andrew Jones <drjones@redhat.com>
> > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > > ---
> > >  lib/pci.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/pci.c b/lib/pci.c
> > > index 953810d14334..cb9fc0d86630 100644
> > > --- a/lib/pci.c
> > > +++ b/lib/pci.c
> > > @@ -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;
> > > +
> > 
> > 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;
> > 
> > >  	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.
> > 
> > > +
> > >  	pci_config_writel(dev, off, (uint32_t)addr);
> > >  
> > >  	if (is64)
> > > -- 
> > > 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.

Actually you can just call pci_bar_size_helper from pci_bar_is_valid.

> 
> > 
> > Thanks,
> > drew
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH 0/4] pci: Various PCI BAR checks

[Andrew Jones]

Alex,

are you going to send a v2 of this?

Thanks,
drew

On Tue, Nov 29, 2016 at 03:48:49PM +0100, Alexander Gordeev wrote:
> 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 <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> 
> 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(-)
> 
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html