On Wed, Nov 30, 2016 at 02:52:53PM +0100, Andrew Jones wrote:
> 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.
Actually you can just call pci_bar_size_helper from pci_bar_is_valid.
>
> >
> > 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
--
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
