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