On Tue, Nov 29, 2016 at 03:48:53PM +0100, Alexander Gordeev wrote:
> Cc: Thomas Huth <thuth@xxxxxxxxxx>
> Cc: Andrew Jones <drjones@xxxxxxxxxx>
> Signed-off-by: Alexander Gordeev <agordeev@xxxxxxxxxx>
> ---
> lib/pci.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/lib/pci.c b/lib/pci.c
> index 953810d14334..cb9fc0d86630 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -44,11 +44,16 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
> uint32_t bar = pci_bar_get(dev, bar_num);
> uint32_t mask = pci_bar_mask(bar);
> uint64_t addr = bar & mask;
> - phys_addr_t phys_addr;
> + phys_addr_t phys_addr, size;
>
> if (pci_bar_is64(dev, bar_num))
> addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
>
> + size = pci_bar_size(dev, bar_num);
> + assert(size);
> + if (!size)
> + return INVALID_PHYS_ADDR;
> +
You're both asserting size is non-zero and returning something when
it is... You only want the later. It's quite feasible that a unit
test would want to probe bars by attempting get_addrs on each,
checking for a valid one.
So, with the new pci_bar_is_valid() I propose, you should just do
if (!pci_bar_is_valid(dev, bar_num))
return INVALID_PHYS_ADDR;
> phys_addr = pci_translate_addr(dev, addr);
> assert(phys_addr != INVALID_PHYS_ADDR);
>
> @@ -58,8 +63,13 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
> void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
> {
> bool is64 = pci_bar_is64(dev, bar_num);
> + phys_addr_t size = pci_bar_size(dev, bar_num);
> int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
>
> + assert(size);
> + if (!size)
> + return;
Again, both an assert and a return. Here I think asserting that the
bar is valid is reasonable.
> +
> pci_config_writel(dev, off, (uint32_t)addr);
>
> if (is64)
> --
> 1.8.3.1
>
There are a few other functions (pci_alloc_resource, pci_bar_print,
pci_bar_size) that do validity checking by looking for zero size. We
should replace those with the new, improved validity check as well.
Thanks,
drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
