On 27.03.2017 15:22, 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 | 14 +++++++++++++-
> lib/pci.h | 3 +++
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/lib/pci.c b/lib/pci.c
> index daf398100b7e..17ce75322547 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -112,6 +112,8 @@ uint32_t pci_bar_mask(uint32_t bar)
>
> uint32_t pci_bar_get(struct pci_dev *dev, int bar_num)
> {
> + CHECK_BAR_NUM(bar_num);
> +
> return pci_config_readl(dev->bdf, PCI_BASE_ADDRESS_0 +
> bar_num * 4);
> }
> @@ -134,6 +136,8 @@ static 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, int bar_num)
> {
> + CHECK_BAR_NUM(bar_num);
> +
> return dev->resource[bar_num];
> }
>
> @@ -141,11 +145,19 @@ void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
> {
> int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
>
> + assert(addr != INVALID_PHYS_ADDR);
> + assert(dev->resource[bar_num] != INVALID_PHYS_ADDR);
> +
> + CHECK_BAR_NUM(bar_num);
> + if (pci_bar_is64(dev, bar_num))
> + CHECK_BAR_NUM(bar_num + 1);
> + else
> + assert((addr >> 32) == 0);
> +
> pci_config_writel(dev->bdf, off, (uint32_t)addr);
> dev->resource[bar_num] = addr;
>
> if (pci_bar_is64(dev, bar_num)) {
> - assert(bar_num + 1 < PCI_BAR_NUM);
> pci_config_writel(dev->bdf, off + 4, (uint32_t)(addr >> 32));
> dev->resource[bar_num + 1] = dev->resource[bar_num];
> }
> diff --git a/lib/pci.h b/lib/pci.h
> index 03cc0a72d48d..2b104b598ff1 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 >= 0 && bar_num < PCI_BAR_NUM); } while (0)
> +
> #define PCI_BDF_GET_DEVFN(x) ((x) & 0xff)
> #define PCI_BDF_GET_BUS(x) (((x) >> 8) & 0xff)
>
>
Looks fine to me. I'd maybe rather call it ASSERT_BAR_NUM instead of
CHECK_BAR_NUM, but that's just cosmetics...
Reviewed-by: Thomas Huth <thuth@xxxxxxxxxx>
