All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexander Gordeev <agordeev@redhat.com>
To: Andrew Jones <drjones@redhat.com>
Cc: kvm@vger.kernel.org, Thomas Huth <thuth@redhat.com>,
	Peter Xu <peterx@redhat.com>
Subject: Re: [kvm-unit-tests PATCH v4 6/5] pci: Add BAR sanity checks
Date: Tue, 7 Mar 2017 20:39:06 +0100	[thread overview]
Message-ID: <20170307193905.GA27018@agordeev.lab.eng.brq.redhat.com> (raw)
In-Reply-To: <20170307142253.o6f4etsnd23m2f7o@hawk.localdomain>

On Tue, Mar 07, 2017 at 03:22:53PM +0100, Andrew Jones wrote:
> Hi Alex,
> 
> This should be its own patch, the series you've added it to was
> already committed.
> 
> On Mon, Mar 06, 2017 at 09:06:23PM +0100, Alexander Gordeev wrote:
> > Cc: Thomas Huth <thuth@redhat.com>
> > Cc: Andrew Jones <drjones@redhat.com>
> > Cc: Peter Xu <peterx@redhat.com>
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > ---
> >  lib/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++--------------
> >  lib/pci.h | 21 +++++++++++++--------
> >  2 files changed, 53 insertions(+), 22 deletions(-)
> > 
> > diff --git a/lib/pci.c b/lib/pci.c
> > index daf398100b7e..98fc3849d297 100644
> > --- a/lib/pci.c
> > +++ b/lib/pci.c
> > @@ -110,13 +110,14 @@ uint32_t pci_bar_mask(uint32_t bar)
> >  		PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
> >  }
> >  
> > -uint32_t pci_bar_get(struct pci_dev *dev, int bar_num)
> > +uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num)
> >  {
> > -	return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 +
> > -				bar_num * 4);
> > +	CHECK_BAR_NUM(bar_num);
> > +
> > +	return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 + bar_num * 4);
> >  }
> >  
> > -static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num)
> > +static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num)
> >  {
> >  	uint32_t bar = pci_bar_get(dev, bar_num);
> >  	uint32_t mask = pci_bar_mask(bar);
> > @@ -132,15 +133,26 @@ static phys_addr_t __pci_bar_get_addr(struct pci_dev *dev, int bar_num)
> >  	return phys_addr;
> >  }
> >  
> > -phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num)
> > +phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num)
> >  {
> > +	CHECK_BAR_NUM(bar_num);
> > +
> >  	return dev->resource[bar_num];
> >  }
> >  
> > -void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
> > +void pci_bar_set_addr(struct pci_dev *dev,
> > +		      unsigned int bar_num, phys_addr_t addr)
> >  {
> >  	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
> >  
> > +	CHECK_BAR_NUM(bar_num);
> > +	assert(addr != INVALID_PHYS_ADDR);
> 
> Shouldn't this be something like
> 
>  assert((addr & PHYS_MASK) == addr)

Nope. At this stage PCI bus is scanned and pci_dev::resource[]
is initialized with proper values. A value of INVALID_PHYS_ADDR in
dev->resource[bar_num] indicates BAR #bar_num is not implemented.
Thus, we do not want to screw up this assumption with a test case
trying to write INVALID_PHYS_ADDR to a the BAR.

> But that means we need to ensure all architectures define PHYS_MASK...
> 
> > +	assert(dev->resource[bar_num] != INVALID_PHYS_ADDR);
> > +	if (pci_bar_is64(dev, bar_num)) {
> > +		assert(bar_num + 1 < PCI_BAR_NUM);
> 
> Use CHECK_BAR_NUM(bar_num + 1)
> 
> > +		assert(dev->resource[bar_num] == dev->resource[bar_num + 1]);
> 
> Can this ever happen without the unit test explicitly messing things up?
> 
> > +	}
> > +
> >  	pci_config_writel(dev->bdf, off, (uint32_t)addr);
> >  	dev->resource[bar_num] = addr;
> >  
> > @@ -161,7 +173,7 @@ void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
> >   * The following pci_bar_size_helper() and pci_bar_size() functions
> >   * implement the algorithm.
> >   */
> > -static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num)
> > +static uint32_t pci_bar_size_helper(struct pci_dev *dev, unsigned int bar_num)
> >  {
> >  	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
> >  	uint16_t bdf = dev->bdf;
> > @@ -175,10 +187,12 @@ static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num)
> >  	return val;
> >  }
> >  
> > -phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num)
> > +phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num)
> >  {
> >  	uint32_t bar, size;
> >  
> > +	CHECK_BAR_NUM(bar_num);
> > +
> >  	size = pci_bar_size_helper(dev, bar_num);
> >  	if (!size)
> >  		return 0;
> > @@ -196,21 +210,31 @@ phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num)
> >  	}
> >  }
> >  
> > -bool pci_bar_is_memory(struct pci_dev *dev, int bar_num)
> > +bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num)
> >  {
> > -	uint32_t bar = pci_bar_get(dev, bar_num);
> > +	uint32_t bar;
> > +
> > +	CHECK_BAR_NUM(bar_num);
> > +
> > +	bar = pci_bar_get(dev, bar_num);
> 
> This hunk is unnecessary, pci_bar_get already calls CHECK_BAR_NUM

Right, it is superfluous. I put it here intentionally to avoid
the check to go away in case pci_bar_get() is changed somehow.
Pure paranoia. I can remove it if you want here and elsewhere.

> >  	return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
> >  }
> >  
> > -bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)
> > +bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num)
> >  {
> > +	CHECK_BAR_NUM(bar_num);
> > +
> >  	return dev->resource[bar_num] != INVALID_PHYS_ADDR;
> >  }
> >  
> > -bool pci_bar_is64(struct pci_dev *dev, int bar_num)
> > +bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num)
> >  {
> > -	uint32_t bar = pci_bar_get(dev, bar_num);
> > +	uint32_t bar;
> > +
> > +	CHECK_BAR_NUM(bar_num);
> > +
> > +	bar = pci_bar_get(dev, bar_num);
> 
> Same as above (unnecessary hunk)
> 
> >  
> >  	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
> >  		return false;
> > @@ -219,11 +243,13 @@ bool pci_bar_is64(struct pci_dev *dev, int bar_num)
> >  		      PCI_BASE_ADDRESS_MEM_TYPE_64;
> >  }
> >  
> > -void pci_bar_print(struct pci_dev *dev, int bar_num)
> > +void pci_bar_print(struct pci_dev *dev, unsigned int bar_num)
> >  {
> >  	phys_addr_t size, start, end;
> >  	uint32_t bar;
> >  
> > +	CHECK_BAR_NUM(bar_num);
> > +
> 
> Unnecessary, pci_bar_is_valid already checks
> 
> >  	if (!pci_bar_is_valid(dev, bar_num))
> >  		return;
> >  
> > diff --git a/lib/pci.h b/lib/pci.h
> > index 03cc0a72d48d..c770def840da 100644
> > --- a/lib/pci.h
> > +++ b/lib/pci.h
> > @@ -18,6 +18,9 @@ enum {
> >  #define PCI_BAR_NUM                     6
> >  #define PCI_DEVFN_MAX                   256
> >  
> > +#define CHECK_BAR_NUM(bar_num)	\
> > +	do { assert(bar_num < PCI_BAR_NUM); } while (0)
> 
> Just add 'bar_num >= 0 &&' to this macro and we don't need to
> change bar_num to unsigned everywhere
> 
> > +
> >  #define PCI_BDF_GET_DEVFN(x)            ((x) & 0xff)
> >  #define PCI_BDF_GET_BUS(x)              (((x) >> 8) & 0xff)
> >  
> > @@ -54,15 +57,17 @@ extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
> >   * It is expected the caller is aware of the device BAR layout and never
> >   * tries to address the middle of a 64-bit register.
> >   */
> > -extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num);
> > -extern void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr);
> > -extern phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num);
> > -extern uint32_t pci_bar_get(struct pci_dev *dev, int bar_num);
> > +
> > +extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, unsigned int bar_num);
> > +extern void pci_bar_set_addr(struct pci_dev *dev, unsigned int bar_num,
> > +			     phys_addr_t addr);
> > +extern phys_addr_t pci_bar_size(struct pci_dev *dev, unsigned int bar_num);
> > +extern uint32_t pci_bar_get(struct pci_dev *dev, unsigned int bar_num);
> >  extern uint32_t pci_bar_mask(uint32_t bar);
> > -extern bool pci_bar_is64(struct pci_dev *dev, int bar_num);
> > -extern bool pci_bar_is_memory(struct pci_dev *dev, int bar_num);
> > -extern bool pci_bar_is_valid(struct pci_dev *dev, int bar_num);
> > -extern void pci_bar_print(struct pci_dev *dev, int bar_num);
> > +extern bool pci_bar_is64(struct pci_dev *dev, unsigned int bar_num);
> > +extern bool pci_bar_is_memory(struct pci_dev *dev, unsigned int bar_num);
> > +extern bool pci_bar_is_valid(struct pci_dev *dev, unsigned int bar_num);
> > +extern void pci_bar_print(struct pci_dev *dev, unsigned int bar_num);
> >  extern void pci_dev_print_id(struct pci_dev *dev);
> >  extern void pci_dev_print(struct pci_dev *dev);
> >  extern uint8_t pci_intx_line(struct pci_dev *dev);
> > -- 
> > 1.8.3.1
> > 
> 
> Thanks,
> drew

  reply	other threads:[~2017-03-07 20:25 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-28 18:08 [kvm-unit-tests PATCH v4 0/5] pci: Complete conversion of PCI API to struct pci_dev Alexander Gordeev
2017-02-28 18:08 ` [kvm-unit-tests PATCH v4 1/5] pci: pci-host-generic: Use INVALID_PHYS_ADDR instead of ~0 Alexander Gordeev
2017-02-28 18:08 ` [kvm-unit-tests PATCH v4 2/5] pci: Accomodate 64 bit BARs in pci_dev::resource[] Alexander Gordeev
2017-02-28 18:08 ` [kvm-unit-tests PATCH v4 3/5] pci: Turn struct pci_dev into device handle for PCI functions Alexander Gordeev
2017-02-28 18:08 ` [kvm-unit-tests PATCH v4 4/5] pci: Rework pci_bar_is_valid() Alexander Gordeev
2017-02-28 18:08 ` [kvm-unit-tests PATCH v4 5/5] pci: Make PCI API consistent wrt using struct pci_dev Alexander Gordeev
2017-03-02 21:24 ` [kvm-unit-tests PATCH v4 0/5] pci: Complete conversion of PCI API to " Radim Krčmář
2017-03-06 20:06 ` [kvm-unit-tests PATCH v4 6/5] pci: Add BAR sanity checks Alexander Gordeev
2017-03-07 14:22   ` Andrew Jones
2017-03-07 19:39     ` Alexander Gordeev [this message]
2017-03-08 10:10       ` Andrew Jones
2017-03-08 11:40         ` Alexander Gordeev
2017-03-08 11:45           ` Andrew Jones

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170307193905.GA27018@agordeev.lab.eng.brq.redhat.com \
    --to=agordeev@redhat.com \
    --cc=drjones@redhat.com \
    --cc=kvm@vger.kernel.org \
    --cc=peterx@redhat.com \
    --cc=thuth@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.