On Wed, Nov 30, 2016 at 02:47:59PM +0100, Andrew Jones wrote:
> 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.
Oops, can't call the new validity function from pci_bar_size. That
would recurse. Please add a comment to pci_bar_size stating that.
>
> 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
--
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
