Re: [kvm-unit-tests PATCH v9 10/12] pci: Add generic ECAM host support

[Alexander Gordeev <agordeev@redhat.com>]

On Fri, Nov 04, 2016 at 06:07:41PM +0100, Andrew Jones wrote:
> > > +bool pci_probe(void)
> > > +{
> > > +	pcidevaddr_t dev;
> > > +	u8 header;
> > > +	u32 cmd;
> > > +	int i;
> > > +
> > > +	assert(!pci_host_bridge);
> > > +	pci_host_bridge = pci_dt_probe();
> > > +	if (!pci_host_bridge)
> > > +		return false;
> > > +
> > > +	for (dev = 0; dev < 256; dev++) {
> > > +		if (!pci_dev_exists(dev))
> > > +			continue;
> > > +
> > > +		/* We are only interested in normal PCI devices */
> > > +		header = pci_config_readb(dev, PCI_HEADER_TYPE);
> > > +		if ((header & PCI_HEADER_TYPE_MASK) != PCI_HEADER_TYPE_NORMAL)
> > > +			continue;
> > > +
> > > +		cmd = PCI_COMMAND_SERR;
> > > +
> > > +		for (i = 0; i < 6; i++) {
> > > +			u64 addr;
> > > +
> > > +			if (pci_alloc_resource(dev, i, &addr)) {
> > > +				pci_bar_set_addr(dev, i, addr);
> > > +
> > > +				if (pci_bar_is_memory(dev, i))
> > > +					cmd |= PCI_COMMAND_MEMORY;
> > > +				else
> > > +					cmd |= PCI_COMMAND_IO;
> > > +			}
> > > +
> > > +			if (pci_bar_is64(dev, i))
> > > +				i++;
> > > +		}
> > > +
> > > +		pci_config_writel(dev, PCI_COMMAND, cmd);
> > 
> > As I asked in the last review, is this PCI_COMMAND_SERR safe/desired
> > to write, even when no resources are allocated? If not, then above
> > we need to capture the return value of pci_alloc_resource, add
> > an 'else break', and then check the return value here before doing
> > the write.
> 
> Another note on this PCI_COMMAND write. Why is it safe [correct?] to
> use writel? This is a 16-bit register according to the comment in
> lib/linux/pci_regs.h. Don't we need to introduce a pci_config_writew
> and use that? If so, then you should grab Peter Xu's patch that
> introduces pci_config_writeb/w.

You are right, this is cleanly a bug.

However, I can not put Peter's patch neither before nor after mine,
since pci_config_write*() are both introduced and used in this patch.
As Peter's patch needs a refreshment anyway (see my comment on it)
I will add pci_config_writew() to this patch. Hope, that works for
everyone.

> > 
> > > +	}
> > > +
> > > +	return true;
> > > +}

> > Thanks,
> > drew