[v4,4/7] PCI: Don't update VF BARs while VF memory space is enabled

If we update a VF BAR while it's enabled, there are two potential problems:

  1) Any driver that's using the VF has a cached BAR value that is stale
     after the update, and

  2) We can't update 64-bit BARs atomically, so the intermediate state
     (new lower dword with old upper dword) may conflict with another
     device, and an access by a driver unrelated to the VF may cause a bus
     error.

Warn about attempts to update VF BARs while they are enabled.  This is a
programming error, so use dev_WARN() to get a backtrace.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/iov.c |    8 ++++++++
 1 file changed, 8 insertions(+)

On Mon, Nov 28, 2016 at 10:15:21PM -0600, Bjorn Helgaas wrote:
>If we update a VF BAR while it's enabled, there are two potential problems:
>
>  1) Any driver that's using the VF has a cached BAR value that is stale
>     after the update, and
>
>  2) We can't update 64-bit BARs atomically, so the intermediate state
>     (new lower dword with old upper dword) may conflict with another
>     device, and an access by a driver unrelated to the VF may cause a bus
>     error.
>
>Warn about attempts to update VF BARs while they are enabled.  This is a
>programming error, so use dev_WARN() to get a backtrace.
>
>Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

>---
> drivers/pci/iov.c |    8 ++++++++
> 1 file changed, 8 insertions(+)
>
>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>index d00ed5c..a800ba2 100644
>--- a/drivers/pci/iov.c
>+++ b/drivers/pci/iov.c
>@@ -584,6 +584,7 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno)
> 	struct resource *res = dev->resource + resno;
> 	int vf_bar = resno - PCI_IOV_RESOURCES;
> 	struct pci_bus_region region;
>+	u16 cmd;
> 	u32 new;
> 	int reg;
>
>@@ -595,6 +596,13 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno)
> 	if (!iov)
> 		return;
>
>+	pci_read_config_word(dev, iov->pos + PCI_SRIOV_CTRL, &cmd);
>+	if ((cmd & PCI_SRIOV_CTRL_VFE) && (cmd & PCI_SRIOV_CTRL_MSE)) {
>+		dev_WARN(&dev->dev, "can't update enabled VF BAR%d %pR\n",
>+			 vf_bar, res);
>+		return;
>+	}
>+
> 	/*
> 	 * Ignore unimplemented BARs, unused resource slots for 64-bit
> 	 * BARs, and non-movable resources, e.g., those described via
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

From: Bjorn Helgaas

> Sent: 29 November 2016 04:15

> If we update a VF BAR while it's enabled, there are two potential problems:

> 

>   1) Any driver that's using the VF has a cached BAR value that is stale

>      after the update, and

> 

>   2) We can't update 64-bit BARs atomically, so the intermediate state

>      (new lower dword with old upper dword) may conflict with another

>      device, and an access by a driver unrelated to the VF may cause a bus

>      error.


Can the high word be first set to a value that is invalid (ie a 4G block
that has no valid PCIe slaves) before updating the low word and finally
the correct high word.

Note that the address only has to be outside the range that the bridge
forwards onto that specific bus.

	David

[Your response didn't make it to the list, I think because it's some
non-plaintext encoding that is rejected by the list (see
http://vger.kernel.org/majordomo-info.html)]

On Wed, Nov 30, 2016 at 11:56 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Bjorn Helgaas
>> Sent: 29 November 2016 04:15
>> If we update a VF BAR while it's enabled, there are two potential problems:
>>
>>   1) Any driver that's using the VF has a cached BAR value that is stale
>>      after the update, and
>>
>>   2) We can't update 64-bit BARs atomically, so the intermediate state
>>      (new lower dword with old upper dword) may conflict with another
>>      device, and an access by a driver unrelated to the VF may cause a bus
>>      error.
>
> Can the high word be first set to a value that is invalid (ie a 4G block
> that has no valid PCIe slaves) before updating the low word and finally
> the correct high word.
>
> Note that the address only has to be outside the range that the bridge
> forwards onto that specific bus.

Maybe, but I think that's getting way too complicated.  I think we
should just think of it as a higher level bug if we're trying to
update an enabled BAR.  We might have to live with that scenario for
standard BARs because firmware might have enabled it for us, and
things might break if we disable it, but I don't think we should try
to make it work for VF BARs.

Bjorn