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

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

[Alexander Gordeev]

Hi Andrew,

Here is v2 of the series - it improves BAR validation function and
scatters more calls to that around.

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>

Alexander Gordeev (4):
  pci: Rework pci_bar_is_valid()
  pci: Split and rename pci_bar_is_valid() to pci_bar_exists()
  pci: Ensure BAR validity for public functions
  pci: pci-host-generic: Use INVALID_PHYS_ADDR instead of ~0

 lib/pci-host-generic.c |  6 +++---
 lib/pci-testdev.c      |  2 +-
 lib/pci.c              | 35 ++++++++++++++++++++++++++++++++---
 lib/pci.h              |  5 +++++
 x86/vmexit.c           |  2 +-
 5 files changed, 42 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[kvm-unit-tests PATCH v2 1/4] pci: Rework pci_bar_is_valid()

[Alexander Gordeev]

Currently pci_bar_is_valid() returns the value stored
in a BAR. That is wrong, because the function fails if
the returned value is zero. But that is wrong, because:
  - an address in PIO address space may be zero;
  - an upper part of 64-bit BAR may be zero;

The right way of validating a BAR is checking its size -
for implemented BARs the size is non-zero.

It might look strange, but the function checks not just the
queried BAR, but sanity of all six BARs of the device.

If we checked only requested BAR the function could have
succeeded with inconsistent BAR sizes combinations. I.e.
imagine a corrupted BARs reported by a device:
  BAR 0 - 64 bit lower
  BAR 1 - 64 bit lower
  BAR 2 - 64 bit upper
  ...
If the function is asked about BAR 1 it would succeed, even
though in fact the layout is ambiguous.

As result, the function is expected to be called only with
valid BAR numbers and BARs layout should be consistent.
Otherwise, an assert is raised.

The BAR number parameter bar_num is only allowed to refer
a 32-bit BAR or lower part of a 64-bit BAR, as it does not
make sense checking validity of high part of a 64-bit BAR.
Thus, the caller is expected to know the device BARs layout.

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

diff --git a/lib/pci.c b/lib/pci.c
index fdd88296f0ae..00fba6e07860 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -117,7 +117,24 @@ bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
 
 bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
 {
-	return pci_bar_get(dev, bar_num);
+	bool is64 = false;
+	int i;
+
+	assert(bar_num >= 0 && bar_num < 6);
+
+	for (i = 0; i < 6; i++) {
+		if (is64) {
+			assert(i == bar_num);	/* high part of 64-bit BAR */
+			assert(pci_bar_is64(dev, i));
+
+			is64 = false;
+		} else {
+			is64 = pci_bar_is64(dev, i);
+		}
+	}
+	assert(!is64);				/* incomplete 64-bit BAR */
+
+	return pci_bar_size(dev, bar_num);
 }
 
 bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v2 1/4] pci: Rework pci_bar_is_valid()

[Andrew Jones]

On Mon, Jan 23, 2017 at 08:44:07AM +0100, Alexander Gordeev wrote:
> Currently pci_bar_is_valid() returns the value stored
> in a BAR. That is wrong, because the function fails if
> the returned value is zero. But that is wrong, because:
>   - an address in PIO address space may be zero;
>   - an upper part of 64-bit BAR may be zero;
> 
> The right way of validating a BAR is checking its size -
> for implemented BARs the size is non-zero.
> 
> It might look strange, but the function checks not just the
> queried BAR, but sanity of all six BARs of the device.
> 
> If we checked only requested BAR the function could have
> succeeded with inconsistent BAR sizes combinations. I.e.
> imagine a corrupted BARs reported by a device:
>   BAR 0 - 64 bit lower
>   BAR 1 - 64 bit lower
>   BAR 2 - 64 bit upper
>   ...
> If the function is asked about BAR 1 it would succeed, even
> though in fact the layout is ambiguous.

Is it possible to have bars messed up like this? I think I'd
just assume that sane layouts will exist and that we need to
avoid misinterpreting a high bar. So we just need to know a
high bar is a high bar, and in that case it's valid.

> 
> As result, the function is expected to be called only with
> valid BAR numbers and BARs layout should be consistent.
> Otherwise, an assert is raised.
> 
> The BAR number parameter bar_num is only allowed to refer
> a 32-bit BAR or lower part of a 64-bit BAR, as it does not
> make sense checking validity of high part of a 64-bit BAR.
> Thus, the caller is expected to know the device BARs layout.

I think the caller should be allowed to make this call on a
high bar.

> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index fdd88296f0ae..00fba6e07860 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -117,7 +117,24 @@ bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
>  
>  bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
>  {
> -	return pci_bar_get(dev, bar_num);
> +	bool is64 = false;
> +	int i;
> +
> +	assert(bar_num >= 0 && bar_num < 6);
> +
> +	for (i = 0; i < 6; i++) {

We have PCI_BAR_NUM now to use instead of 6, but don't you just want
to loop up to bar_num?

> +		if (is64) {
> +			assert(i == bar_num);	/* high part of 64-bit BAR */

This assert looks wrong. Let's assume we want to check bar_num == 2,
but bar0 is 64-bit. In that case wouldn't you hit it?

> +			assert(pci_bar_is64(dev, i));

Won't this assert always fire on the high bar?

> +
> +			is64 = false;
> +		} else {
> +			is64 = pci_bar_is64(dev, i);
> +		}
> +	}
> +	assert(!is64);				/* incomplete 64-bit BAR */
> +
> +	return pci_bar_size(dev, bar_num);
>  }
>  
>  bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
> -- 
> 1.8.3.1
> 

How about just this instead (untested)

 bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
 {
   assert(bar_num >= 0 && bar_num < PCI_BAR_NUM);
   if (bar_num > 0 && pci_bar_is64(dev, bar_num - 1))
     return pci_bar_is_valid(dev, bar_num - 1);
   return pci_bar_size(dev, bar_num);
 }

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 1/4] pci: Rework pci_bar_is_valid()

[Alexander Gordeev]

On Sat, Jan 28, 2017 at 12:10:41PM +0100, Andrew Jones wrote:
> > If we checked only requested BAR the function could have
> > succeeded with inconsistent BAR sizes combinations. I.e.
> > imagine a corrupted BARs reported by a device:
> >   BAR 0 - 64 bit lower
> >   BAR 1 - 64 bit lower
> >   BAR 2 - 64 bit upper
> >   ...
> > If the function is asked about BAR 1 it would succeed, even
> > though in fact the layout is ambiguous.
> 
> Is it possible to have bars messed up like this? I think I'd

I do not know actually. We spread sanity checks (asserts) here
and there to avoid i.e. DT inconsistency. If we were to propagate
this approach to the rest of the code, then we should assume BARs
could be messed up, right?

It is a design decision, therefore - up to you ;)

> just assume that sane layouts will exist and that we need to
> avoid misinterpreting a high bar. So we just need to know a
> high bar is a high bar, and in that case it's valid.

[...]

> Thanks,
> drew

Re: [kvm-unit-tests PATCH v2 1/4] pci: Rework pci_bar_is_valid()

[Andrew Jones]

On Sat, Jan 28, 2017 at 01:38:43PM +0100, Alexander Gordeev wrote:
> On Sat, Jan 28, 2017 at 12:10:41PM +0100, Andrew Jones wrote:
> > > If we checked only requested BAR the function could have
> > > succeeded with inconsistent BAR sizes combinations. I.e.
> > > imagine a corrupted BARs reported by a device:
> > >   BAR 0 - 64 bit lower
> > >   BAR 1 - 64 bit lower
> > >   BAR 2 - 64 bit upper
> > >   ...
> > > If the function is asked about BAR 1 it would succeed, even
> > > though in fact the layout is ambiguous.
> > 
> > Is it possible to have bars messed up like this? I think I'd
> 
> I do not know actually. We spread sanity checks (asserts) here
> and there to avoid i.e. DT inconsistency. If we were to propagate
> this approach to the rest of the code, then we should assume BARs
> could be messed up, right?
> 
> It is a design decision, therefore - up to you ;)

I like sanity checking asserts, but a check for bar layout
sanity only needs to be made once, not each time any bar is
checked for existence. If you want to confirm the layout of
bars is sane, then pci_scan_bars is probably the right place.

> 
> > just assume that sane layouts will exist and that we need to
> > avoid misinterpreting a high bar. So we just need to know a
> > high bar is a high bar, and in that case it's valid.
> 
> [...]
> 
> > Thanks,
> > drew

Re: [kvm-unit-tests PATCH v2 1/4] pci: Rework pci_bar_is_valid()

[Alexander Gordeev]

On Sat, Jan 28, 2017 at 12:10:41PM +0100, Andrew Jones wrote:
> How about just this instead (untested)
> 
>  bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
>  {
>    assert(bar_num >= 0 && bar_num < PCI_BAR_NUM);
>    if (bar_num > 0 && pci_bar_is64(dev, bar_num - 1))
>      return pci_bar_is_valid(dev, bar_num - 1);
>    return pci_bar_size(dev, bar_num);
>  }

This one can not distinguish between PCI_BASE_ADDRESS_* flags
and address bits of high part of a 64 bit address.

However, it seems a newly introduced pci_dev::resource[] can
be used for that.

> Thanks,
> drew

Re: [kvm-unit-tests PATCH v2 1/4] pci: Rework pci_bar_is_valid()

[Alexander Gordeev]

On Wed, Feb 08, 2017 at 01:49:20PM +0100, Alexander Gordeev wrote:
> On Sat, Jan 28, 2017 at 12:10:41PM +0100, Andrew Jones wrote:
> > How about just this instead (untested)
> > 
> >  bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
> >  {
> >    assert(bar_num >= 0 && bar_num < PCI_BAR_NUM);
> >    if (bar_num > 0 && pci_bar_is64(dev, bar_num - 1))
> >      return pci_bar_is_valid(dev, bar_num - 1);
> >    return pci_bar_size(dev, bar_num);
> >  }
> 
> This one can not distinguish between PCI_BASE_ADDRESS_* flags
> and address bits of high part of a 64 bit address.
> 
> However, it seems a newly introduced pci_dev::resource[] can
> be used for that.

Hi Andrew,

Although using pci_dev::resource[] is possible I do not like a
result for various reasons. I.e. I anticipate these:

  - pci_bar_get/set_addr() logic will get quite cryptic;
  - mismatch between pci_dev::resource[] 64 bit size and 
    32 bit BAR size. While it is okay right now it will
    be confusing once pci_dev::resource[] are reused as
    BAR validity flags;
  - architecture specific pci_probe() reworked to seed
    pci_dev::resource[] (and being invoked on all archs);

Overall, the current compact implementetion is going to turn
into a framework with not so obvious logic.

I suggest just polish v1 or drop the idea of checking BARs
altogether.

Thoughts?

> > Thanks,
> > drew

Re: [kvm-unit-tests PATCH v2 1/4] pci: Rework pci_bar_is_valid()

[Andrew Jones]

On Mon, Feb 13, 2017 at 10:44:21AM +0100, Alexander Gordeev wrote:
> On Wed, Feb 08, 2017 at 01:49:20PM +0100, Alexander Gordeev wrote:
> > On Sat, Jan 28, 2017 at 12:10:41PM +0100, Andrew Jones wrote:
> > > How about just this instead (untested)
> > > 
> > >  bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
> > >  {
> > >    assert(bar_num >= 0 && bar_num < PCI_BAR_NUM);
> > >    if (bar_num > 0 && pci_bar_is64(dev, bar_num - 1))
> > >      return pci_bar_is_valid(dev, bar_num - 1);
> > >    return pci_bar_size(dev, bar_num);
> > >  }
> > 
> > This one can not distinguish between PCI_BASE_ADDRESS_* flags
> > and address bits of high part of a 64 bit address.
> > 
> > However, it seems a newly introduced pci_dev::resource[] can
> > be used for that.
> 
> Hi Andrew,
> 
> Although using pci_dev::resource[] is possible I do not like a
> result for various reasons. I.e. I anticipate these:
> 
>   - pci_bar_get/set_addr() logic will get quite cryptic;
>   - mismatch between pci_dev::resource[] 64 bit size and 
>     32 bit BAR size. While it is okay right now it will
>     be confusing once pci_dev::resource[] are reused as
>     BAR validity flags;
>   - architecture specific pci_probe() reworked to seed
>     pci_dev::resource[] (and being invoked on all archs);
> 
> Overall, the current compact implementetion is going to turn
> into a framework with not so obvious logic.
> 
> I suggest just polish v1 or drop the idea of checking BARs
> altogether.

We definitely need to improve pci_bar_is_valid(), so let's see
how a v2 of this series looks.

Thanks,
drew

> 
> Thoughts?
> 
> > > Thanks,
> > > drew

[kvm-unit-tests PATCH v2 2/4] pci: Split and rename pci_bar_is_valid() to pci_bar_exists()

[Alexander Gordeev]

Function pci_bar_is_valid() does two logical steps:
  1. Checks BAR validity in the context of all device BARs;
  2. Checks if the BAR is implemented (AKA exists);

In some cases checking BAR existence (step 2) is redundant.
So reduce function pci_bar_is_valid() to only checking BAR
validity(step 1).

In most cases we do need to check BAR existence, but we must
ensure the BAR validity first (steps 1 and 2). For such cases
pci_bar_exists() function is introduced. In essence it does
what pci_bar_is_valid() used to do before this change.

Cc: Thomas Huth <thuth@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
 lib/pci-host-generic.c |  4 ++--
 lib/pci-testdev.c      |  2 +-
 lib/pci.c              | 15 +++++++++------
 lib/pci.h              |  5 +++++
 x86/vmexit.c           |  2 +-
 5 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
index 4263365e8288..a881ec5a9d4f 100644
--- a/lib/pci-host-generic.c
+++ b/lib/pci-host-generic.c
@@ -175,8 +175,7 @@ static bool pci_alloc_resource(pcidevaddr_t dev, int bar_num, u64 *addr)
 
 	*addr = ~0;
 
-	size = pci_bar_size(dev, bar_num);
-	if (!size)
+	if (!pci_bar_exists(dev, bar_num))
 		return false;
 
 	bar = pci_bar_get(dev, bar_num);
@@ -199,6 +198,7 @@ static bool pci_alloc_resource(pcidevaddr_t dev, int bar_num, u64 *addr)
 		return false;
 	}
 
+	size = pci_bar_size(dev, bar_num);
 	pci_addr = ALIGN(as->pci_start + as->allocated, size);
 	size += pci_addr - (as->pci_start + as->allocated);
 	assert(as->allocated + size <= as->size);
diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
index ad482d3291c7..712ada5c5860 100644
--- a/lib/pci-testdev.c
+++ b/lib/pci-testdev.c
@@ -176,7 +176,7 @@ int pci_testdev(void)
 		return -1;
 	}
 
-	ret = pci_bar_is_valid(dev, 0) && pci_bar_is_valid(dev, 1);
+	ret = pci_bar_exists(dev, 0) && pci_bar_exists(dev, 1);
 	assert(ret);
 
 	addr = pci_bar_get_addr(dev, 0);
diff --git a/lib/pci.c b/lib/pci.c
index 00fba6e07860..7a3e505ab314 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -124,17 +124,20 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
 
 	for (i = 0; i < 6; i++) {
 		if (is64) {
-			assert(i == bar_num);	/* high part of 64-bit BAR */
-			assert(pci_bar_is64(dev, i));
+			if (i == bar_num)	/* high part of 64-bit BAR */
+				return false;
+			if (pci_bar_is64(dev, i))
+				return false;
 
 			is64 = false;
 		} else {
 			is64 = pci_bar_is64(dev, i);
 		}
 	}
-	assert(!is64);				/* incomplete 64-bit BAR */
+	if (is64)				/* incomplete 64-bit BAR */
+		return false;
 
-	return pci_bar_size(dev, bar_num);
+	return true;
 }
 
 bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
@@ -153,12 +156,12 @@ void pci_bar_print(pcidevaddr_t dev, int bar_num)
 	phys_addr_t size, start, end;
 	uint32_t bar;
 
-	size = pci_bar_size(dev, bar_num);
-	if (!size)
+	if (!pci_bar_exists(dev, bar_num))
 		return;
 
 	bar = pci_bar_get(dev, bar_num);
 	start = pci_bar_get_addr(dev, bar_num);
+	size = pci_bar_size(dev, bar_num);
 	end = start + size - 1;
 
 	if (pci_bar_is64(dev, bar_num)) {
diff --git a/lib/pci.h b/lib/pci.h
index 30f538110610..0b07036f27ac 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -43,6 +43,11 @@ extern bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
 extern void pci_bar_print(pcidevaddr_t dev, int bar_num);
 extern void pci_dev_print_id(pcidevaddr_t dev);
 
+static inline bool pci_bar_exists(pcidevaddr_t dev, int bar_num)
+{
+	return pci_bar_size(dev, bar_num);
+}
+
 int pci_testdev(void);
 
 /*
diff --git a/x86/vmexit.c b/x86/vmexit.c
index 2d99d5fdf1c2..32132667b617 100644
--- a/x86/vmexit.c
+++ b/x86/vmexit.c
@@ -388,7 +388,7 @@ int main(int ac, char **av)
 	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++) {
-			if (!pci_bar_is_valid(pcidev, i)) {
+			if (!pci_bar_exists(pcidev, i)) {
 				continue;
 			}
 			if (pci_bar_is_memory(pcidev, i)) {
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v2 2/4] pci: Split and rename pci_bar_is_valid() to pci_bar_exists()

[Andrew Jones]

If you take my suggestion for patch 1, then this patch can be
dropped.

On Mon, Jan 23, 2017 at 08:44:08AM +0100, Alexander Gordeev wrote:
> Function pci_bar_is_valid() does two logical steps:
>   1. Checks BAR validity in the context of all device BARs;
>   2. Checks if the BAR is implemented (AKA exists);
> 
> In some cases checking BAR existence (step 2) is redundant.
> So reduce function pci_bar_is_valid() to only checking BAR
> validity(step 1).
> 
> In most cases we do need to check BAR existence, but we must
> ensure the BAR validity first (steps 1 and 2). For such cases
> pci_bar_exists() function is introduced. In essence it does
> what pci_bar_is_valid() used to do before this change.
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci-host-generic.c |  4 ++--
>  lib/pci-testdev.c      |  2 +-
>  lib/pci.c              | 15 +++++++++------
>  lib/pci.h              |  5 +++++
>  x86/vmexit.c           |  2 +-
>  5 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> index 4263365e8288..a881ec5a9d4f 100644
> --- a/lib/pci-host-generic.c
> +++ b/lib/pci-host-generic.c
> @@ -175,8 +175,7 @@ static bool pci_alloc_resource(pcidevaddr_t dev, int bar_num, u64 *addr)
>  
>  	*addr = ~0;
>  
> -	size = pci_bar_size(dev, bar_num);
> -	if (!size)
> +	if (!pci_bar_exists(dev, bar_num))
>  		return false;
>  
>  	bar = pci_bar_get(dev, bar_num);
> @@ -199,6 +198,7 @@ static bool pci_alloc_resource(pcidevaddr_t dev, int bar_num, u64 *addr)
>  		return false;
>  	}
>  
> +	size = pci_bar_size(dev, bar_num);
>  	pci_addr = ALIGN(as->pci_start + as->allocated, size);
>  	size += pci_addr - (as->pci_start + as->allocated);
>  	assert(as->allocated + size <= as->size);
> diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
> index ad482d3291c7..712ada5c5860 100644
> --- a/lib/pci-testdev.c
> +++ b/lib/pci-testdev.c
> @@ -176,7 +176,7 @@ int pci_testdev(void)
>  		return -1;
>  	}
>  
> -	ret = pci_bar_is_valid(dev, 0) && pci_bar_is_valid(dev, 1);
> +	ret = pci_bar_exists(dev, 0) && pci_bar_exists(dev, 1);
>  	assert(ret);
>  
>  	addr = pci_bar_get_addr(dev, 0);
> diff --git a/lib/pci.c b/lib/pci.c
> index 00fba6e07860..7a3e505ab314 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -124,17 +124,20 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
>  
>  	for (i = 0; i < 6; i++) {
>  		if (is64) {
> -			assert(i == bar_num);	/* high part of 64-bit BAR */
> -			assert(pci_bar_is64(dev, i));
> +			if (i == bar_num)	/* high part of 64-bit BAR */
> +				return false;
> +			if (pci_bar_is64(dev, i))
> +				return false;
>  
>  			is64 = false;
>  		} else {
>  			is64 = pci_bar_is64(dev, i);
>  		}
>  	}
> -	assert(!is64);				/* incomplete 64-bit BAR */
> +	if (is64)				/* incomplete 64-bit BAR */
> +		return false;
>  
> -	return pci_bar_size(dev, bar_num);
> +	return true;
>  }
>  
>  bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
> @@ -153,12 +156,12 @@ void pci_bar_print(pcidevaddr_t dev, int bar_num)
>  	phys_addr_t size, start, end;
>  	uint32_t bar;
>  
> -	size = pci_bar_size(dev, bar_num);
> -	if (!size)
> +	if (!pci_bar_exists(dev, bar_num))
>  		return;
>  
>  	bar = pci_bar_get(dev, bar_num);
>  	start = pci_bar_get_addr(dev, bar_num);
> +	size = pci_bar_size(dev, bar_num);
>  	end = start + size - 1;
>  
>  	if (pci_bar_is64(dev, bar_num)) {
> diff --git a/lib/pci.h b/lib/pci.h
> index 30f538110610..0b07036f27ac 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -43,6 +43,11 @@ extern bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
>  extern void pci_bar_print(pcidevaddr_t dev, int bar_num);
>  extern void pci_dev_print_id(pcidevaddr_t dev);
>  
> +static inline bool pci_bar_exists(pcidevaddr_t dev, int bar_num)
> +{
> +	return pci_bar_size(dev, bar_num);
> +}
> +
>  int pci_testdev(void);
>  
>  /*
> diff --git a/x86/vmexit.c b/x86/vmexit.c
> index 2d99d5fdf1c2..32132667b617 100644
> --- a/x86/vmexit.c
> +++ b/x86/vmexit.c
> @@ -388,7 +388,7 @@ int main(int ac, char **av)
>  	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++) {
> -			if (!pci_bar_is_valid(pcidev, i)) {
> +			if (!pci_bar_exists(pcidev, i)) {
>  				continue;
>  			}
>  			if (pci_bar_is_memory(pcidev, i)) {
> -- 
> 1.8.3.1
> 

[kvm-unit-tests PATCH v2 3/4] pci: Ensure BAR validity for public functions

[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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/pci.c b/lib/pci.c
index 7a3e505ab314..144bec755c8d 100644
--- a/lib/pci.c
+++ b/lib/pci.c
@@ -35,6 +35,8 @@ 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);
 }
 
@@ -45,6 +47,9 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
 	uint64_t addr = bar & mask;
 	phys_addr_t phys_addr;
 
+	if (!pci_bar_exists(dev, bar_num))
+		return INVALID_PHYS_ADDR;
+
 	if (pci_bar_is64(dev, bar_num))
 		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
 
@@ -58,6 +63,8 @@ void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
 {
 	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
 
+	assert(pci_bar_exists(dev, bar_num));
+
 	pci_config_writel(dev, off, (uint32_t)addr);
 
 	if (pci_bar_is64(dev, bar_num))
@@ -91,6 +98,8 @@ phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
 {
 	uint32_t bar, size;
 
+	assert(pci_bar_is_valid(dev, bar_num));
+
 	size = pci_bar_size_helper(dev, bar_num);
 	if (!size)
 		return 0;
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v2 3/4] pci: Ensure BAR validity for public functions

[Andrew Jones]

On Mon, Jan 23, 2017 at 08:44:09AM +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 | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/lib/pci.c b/lib/pci.c
> index 7a3e505ab314..144bec755c8d 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -35,6 +35,8 @@ 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);
>  }
>  
> @@ -45,6 +47,9 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
>  	uint64_t addr = bar & mask;
>  	phys_addr_t phys_addr;
>  
> +	if (!pci_bar_exists(dev, bar_num))
> +		return INVALID_PHYS_ADDR;
> +
>  	if (pci_bar_is64(dev, bar_num))
>  		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
>  
> @@ -58,6 +63,8 @@ void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
>  {
>  	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
>  
> +	assert(pci_bar_exists(dev, bar_num));
> +
>  	pci_config_writel(dev, off, (uint32_t)addr);
>  
>  	if (pci_bar_is64(dev, bar_num))
> @@ -91,6 +98,8 @@ phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
>  {
>  	uint32_t bar, size;
>  
> +	assert(pci_bar_is_valid(dev, bar_num));

If dropping patch 2, then we can replace this condition with
(bar_num == 0 || !pci_bar_is64(dev, bar_num - 1))

> +
>  	size = pci_bar_size_helper(dev, bar_num);
>  	if (!size)
>  		return 0;
> -- 
> 1.8.3.1
> 

[kvm-unit-tests PATCH v2 4/4] pci: pci-host-generic: Use INVALID_PHYS_ADDR instead of ~0

[Alexander Gordeev]

This update is better be squashed into commit b8b9fcf56253b
("pci: Make all ones invalid translate address")

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

diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
index a881ec5a9d4f..4008fed153b6 100644
--- a/lib/pci-host-generic.c
+++ b/lib/pci-host-generic.c
@@ -173,7 +173,7 @@ static bool pci_alloc_resource(pcidevaddr_t dev, int bar_num, u64 *addr)
 	u64 size, pci_addr;
 	int type, i;
 
-	*addr = ~0;
+	*addr = INVALID_PHYS_ADDR;
 
 	if (!pci_bar_exists(dev, bar_num))
 		return false;
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH v2 4/4] pci: pci-host-generic: Use INVALID_PHYS_ADDR instead of ~0

[Andrew Jones]

On Mon, Jan 23, 2017 at 08:44:10AM +0100, Alexander Gordeev wrote:
> This update is better be squashed into commit b8b9fcf56253b
> ("pci: Make all ones invalid translate address")
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> ---
>  lib/pci-host-generic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Andrew Jones <drjones@redhat.com>

> 
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> index a881ec5a9d4f..4008fed153b6 100644
> --- a/lib/pci-host-generic.c
> +++ b/lib/pci-host-generic.c
> @@ -173,7 +173,7 @@ static bool pci_alloc_resource(pcidevaddr_t dev, int bar_num, u64 *addr)
>  	u64 size, pci_addr;
>  	int type, i;
>  
> -	*addr = ~0;
> +	*addr = INVALID_PHYS_ADDR;
>  
>  	if (!pci_bar_exists(dev, bar_num))
>  		return false;
> -- 
> 1.8.3.1
>