Re: [PATCH v4 3/7] PCI: Separate VF BAR updates from standard BAR updates

On Thu, Dec 01, 2016 at 10:02:54AM +1100, Gavin Shan wrote:

On Tue, Nov 29, 2016 at 06:06:05PM -0600, Bjorn Helgaas wrote:

quoted

On Wed, Nov 30, 2016 at 10:20:28AM +1100, Gavin Shan wrote:

quoted

On Tue, Nov 29, 2016 at 08:48:26AM -0600, Bjorn Helgaas wrote:

quoted

On Tue, Nov 29, 2016 at 03:55:46PM +1100, Gavin Shan wrote:

quoted

On Mon, Nov 28, 2016 at 10:15:06PM -0600, Bjorn Helgaas wrote:

quoted

Previously pci_update_resource() used the same code path for updating
standard BARs and VF BARs in SR-IOV capabilities.

Split the VF BAR update into a new pci_iov_update_resource() internal
interface, which makes it simpler to compute the BAR address (we can get
rid of pci_resource_bar() and pci_iov_resource_bar()).

This patch:

 - Renames pci_update_resource() to pci_std_update_resource(),
 - Adds pci_iov_update_resource(),
 - Makes pci_update_resource() a wrapper that calls the appropriate one,

No functional change intended.

quoted

quoted

However, I don't think this code in pci_update_resource() is obviously
correct:

 new = region.start | (res->flags & PCI_REGION_FLAG_MASK);

PCI_REGION_FLAG_MASK is 0xf.  For memory BARs, bits 0-3 are read-only
property bits.  For I/O BARs, bits 0-1 are read-only and bits 2-3 are
part of the address, so on the face of it, the above could corrupt two
bits of an I/O address.

It's true that decode_bar() initializes flags correctly, using
PCI_BASE_ADDRESS_IO_MASK for I/O BARs and PCI_BASE_ADDRESS_MEM_MASK
for memory BARs, but it would take a little more digging to be sure
that we never set bits 2-3 of flags for an I/O resource elsewhere.

The BAR's property bits are probed from device-tree, not hardware
on some platforms (e.g. pSeries). Also, there is only one (property)
bit if it's a ROM BAR. So more check as below might be needed because
the code (without the enhancement) should also work fine.

Ah, right, I forgot about that.  I didn't do enough digging :)

quoted

quoted

How about this in pci_std_update_resource():

       pcibios_resource_to_bus(dev->bus, &region, res);
       new = region.start;

       if (res->flags & IORESOURCE_IO) {
               mask = (u32)PCI_BASE_ADDRESS_IO_MASK;
               new |= res->flags & ~PCI_BASE_ADDRESS_IO_MASK;
       } else {
               mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
               new |= res->flags & ~PCI_BASE_ADDRESS_MEM_MASK;
       }

	if (res->flags & IORESOURCE_IO) {
		mask = (u32)PCI_BASE_ADDRESS_IO_MASK;
		new |= res->flags & ~PCI_BASE_ADDRESS_IO_MASK;
	} else if (resno < PCI_ROM_RESOURCE) {
		mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
		new |= res->flags & ~PCI_BASE_ADDRESS_MEM_MASK;
	} else if (resno == PCI_ROM_RESOURCE) {
		mask = ~((u32)IORESOURCE_ROM_ENABLE);
		new |= res->flags & IORESOURCE_ROM_ENABLE);
	} else {
		dev_warn(&dev->dev, "BAR#%d out of range\n", resno);
		return;
	}

After this patch, the only thing we OR into a ROM BAR value is
PCI_ROM_ADDRESS_ENABLE, and that's done below, only if the ROM is
already enabled.

I did update the ROM mask (to PCI_ROM_ADDRESS_MASK).  I'm not 100%
sure about doing that -- it follows the spec, but it is a change from
what we've been doing before.  I guess it should be safe because it
means we're checking fewer bits than before (only the top 21 bits for
ROMs, where we used check the top 28), so the only possible difference
is that we might not warn about "error updating" in some case where we
used to.

I'm not really sure about the value of the "error updating" checks to
begin with, though I guess it does help us find broken devices that
put non-BARs where BARs are supposed to be.

Yeah, agree. Bjorn, I don't have more comments. please take your time
to respin the series and maybe applied it. I really want to see the
fixes can be in 4.10 if possible :-)

These will definitely be in v4.10.  Thanks for all your help!