On Wed, Mar 08, 2017 at 11:10:26AM +0100, Andrew Jones wrote:
> > > > -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)
> >
> > Nope. At this stage PCI bus is scanned and pci_dev::resource[]
> > is initialized with proper values. A value of INVALID_PHYS_ADDR in
> > dev->resource[bar_num] indicates BAR #bar_num is not implemented.
> > Thus, we do not want to screw up this assumption with a test case
> > trying to write INVALID_PHYS_ADDR to a the BAR.
>
> Of course, by why would any other invalid phys addr be OK? Hmm, maybe
> it should be allowed for test case purposes.
Yeah, I think it should be allowed.
> > > 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?
No, unless there is really something wrong with the code.
> No answer for this. I don't see the point of an assert that can never
> fail.
So this is code integrity assert, not test case input check.
Do you want me to remove it?
> > > Thanks,
> > > drew
