On Tue, Mar 07, 2017 at 08:39:06PM +0100, Alexander Gordeev wrote:
> 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@xxxxxxxxxx>
> > > Cc: Andrew Jones <drjones@xxxxxxxxxx>
> > > Cc: Peter Xu <peterx@xxxxxxxxxx>
> > > Signed-off-by: Alexander Gordeev <agordeev@xxxxxxxxxx>
> > > ---
> > > 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.
Of course, by why would any other invalid phys addr be OK? Hmm, maybe
it should be allowed for test case purposes.
>
> > 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?
No answer for this. I don't see the point of an assert that can never
fail.
> >
> > > + }
> > > +
> > > 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.
I'd prefer it be removed. No need for the clutter.
>
> > > 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
