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