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

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Wed, Nov 30, 2016 at 05:45:18PM -0600, Bjorn Helgaas wrote:
>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:
>> >On Wed, Nov 30, 2016 at 10:20:28AM +1100, Gavin Shan wrote:
>> >> On Tue, Nov 29, 2016 at 08:48:26AM -0600, Bjorn Helgaas wrote:
>> >> >On Tue, Nov 29, 2016 at 03:55:46PM +1100, Gavin Shan wrote:
>> >> >> On Mon, Nov 28, 2016 at 10:15:06PM -0600, Bjorn Helgaas wrote:
>> >> >> >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.
>> >
>> >> >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 :)
>> >
>> >> >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!
>

Thanks for your helps actually :-)