[PATCH 0/3] PCI: Expose resource resizing through sysfs

[PATCH 0/3] PCI: Expose resource resizing through sysfs

[Alex Williamson]

We have a couple graphics drivers making use of PCIe Resizable BARs
now, but I've been trying to figure out how we can make use of such
features for devices assigned to a VM.  This is a proposal for a
rather basic interface in sysfs such that we have the ability to
pre-enable larger BARs before we bind devices to vfio-pci and
attach them to a VM.

Along the way I found a double-free in the error path of creating
resource attributes, that can certainly be pulled separately (1/).

I'm using an RTX6000 for testing, which unexpectedly only supports
REBAR with smaller than default sizes, which led me to question
why we have such heavy requirements for shrinking resources (2/).

The final patch proposes the sysfs interface and I'll leave the
discussion there for whether this is a good approach.  Thanks,

Alex
---

Alex Williamson (3):
      PCI: Fix double-free in resource attribute error path
      PCI: Skip reassigning bridge resources if reducing BAR size
      PCI: Expose PCIe Resizable BAR support via sysfs


 Documentation/ABI/testing/sysfs-bus-pci |  27 +++++
 drivers/pci/pci-sysfs.c                 | 126 +++++++++++++++++++++++-
 drivers/pci/setup-res.c                 |   2 +-
 include/linux/pci.h                     |   1 +
 4 files changed, 154 insertions(+), 2 deletions(-)

[PATCH 1/3] PCI: Fix double-free in resource attribute error path

[Alex Williamson]

If pci_create_attr() fails the remaining resource bin attributes are
removed and freed based on the pointer entries which are not cleared
in the case that creation of the bin attribute fails.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/pci/pci-sysfs.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index fc804e08e3cb..9ac92e6a2397 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1196,8 +1196,14 @@ static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine)
 	res_attr->size = pci_resource_len(pdev, num);
 	res_attr->private = (void *)(unsigned long)num;
 	retval = sysfs_create_bin_file(&pdev->dev.kobj, res_attr);
-	if (retval)
+	if (retval) {
+		if (write_combine)
+			pdev->res_attr_wc[num] = NULL;
+		else
+			pdev->res_attr[num] = NULL;
+
 		kfree(res_attr);
+	}
 
 	return retval;
 }

[RFC PATCH 2/3] PCI: Skip reassigning bridge resources if reducing BAR size

[Alex Williamson]

More work is probably necessary here, pci_reassign_bridge_resources()
has heavy weight requirements that the aperture windows are unused,
without even testing if the requested resize is possible in the
existing aperture.  One case we can clearly skip is when reducing the
size of a BAR.

Cc: Christian König <christian.koenig@amd.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/pci/setup-res.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 439ac5f5907a..7a8e1e4f4d33 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -450,7 +450,7 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
 	res->end = res->start + pci_rebar_size_to_bytes(size) - 1;
 
 	/* Check if the new config works by trying to assign everything. */
-	if (dev->bus->self) {
+	if (size > old && dev->bus->self) {
 		ret = pci_reassign_bridge_resources(dev->bus->self, res->flags);
 		if (ret)
 			goto error_resize;

[RFC PATCH 3/3] PCI: Expose PCIe Resizable BAR support via sysfs

[Alex Williamson]

This proposes a simple sysfs interface to Resizable BAR support,
largely for the purposes of assigning such devices to a VM through
VFIO.  Resizable BARs present a difficult feature to expose to a VM
through emulation, as resizing a BAR is done on the host.  It can
fail, and often does, but we have no means via emulation of a PCIe
REBAR capability to handle the error cases.

A vfio-pci specific ioctl interface is also cumbersome as there are
often multiple devices within the same bridge aperture and handling
them is a challenge.  In the interface proposed here, expanding a
BAR potentially requires such devices to be soft-removed during the
resize operation and rescanned after, in order for all the necessary
resources to be released.  A pci-sysfs interface is also more
universal than a vfio specific interface.

Please see the ABI documentation update for usage.

Cc: Christian König <christian.koenig@amd.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

NB, I realize the read value of the syfs attribute provides two values,
the bitmap of possible sizes and the current size.  There are a number
of ways to determine the current size, including stat(1) on the
resourceN file, but I found this output to be useful while developing
the interface and provides consistency with the store value to the
attribute.  Suggestions welcome for better semantics.

 Documentation/ABI/testing/sysfs-bus-pci |   27 +++++++
 drivers/pci/pci-sysfs.c                 |  118 +++++++++++++++++++++++++++++++
 include/linux/pci.h                     |    1 
 3 files changed, 146 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index 6fc2c2efe8ab..5eea5d89c9f2 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -457,3 +457,30 @@ Description:
 
 		The file is writable if the PF is bound to a driver that
 		implements ->sriov_set_msix_vec_count().
+
+What:		/sys/bus/pci/devices/.../resourceN_resize
+Date:		August 2022
+Contact:	Alex Williamson <alex.williamson@redhat.com>
+Description:
+		These files provide an interface to PCIe Resizable BAR support.
+		A file is created for each BAR resource (N) supported by the
+		PCIe Resizable BAR extended capability of the device.  Reading
+		each file exposes the capability and current setting for the
+		device, ex.
+
+		# cat resource1_resize
+		00000000000001c0:6
+
+		The first field provides the supported sizes bitmap, where
+		bit0 = 1MB, bit1 = 2MB, bit2 = 4MB, etc.  In the above example
+		the devices supports 64MB, 128MB, and 256MB BAR sizes.  The
+		second field provides the current setting, the value 6
+		indicates bit6 is set, which corresponds to 64MB.
+
+		When writing the file, only the latter is used, ex.
+
+		# echo 7 > resource1_resize
+
+		This indicates to set the size value corresponding to bit 7,
+		which is 128MB.  The resulting size is 2 ^ (bit# + 20).  This
+		definition matches the PCIe specification of this capability.
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 9ac92e6a2397..aa59a2de508f 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1143,6 +1143,7 @@ static void pci_remove_resource_files(struct pci_dev *pdev)
 
 	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
 		struct bin_attribute *res_attr;
+		struct dev_ext_attribute *resize_attr;
 
 		res_attr = pdev->res_attr[i];
 		if (res_attr) {
@@ -1155,6 +1156,13 @@ static void pci_remove_resource_files(struct pci_dev *pdev)
 			sysfs_remove_bin_file(&pdev->dev.kobj, res_attr);
 			kfree(res_attr);
 		}
+
+		resize_attr = pdev->res_attr_resize[i];
+		if (resize_attr) {
+			sysfs_remove_file(&pdev->dev.kobj,
+					  &resize_attr->attr.attr);
+			kfree(resize_attr);
+		}
 	}
 }
 
@@ -1208,6 +1216,108 @@ static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine)
 	return retval;
 }
 
+static ssize_t pci_bar_resize_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct dev_ext_attribute *resize_attr =
+			container_of(attr, struct dev_ext_attribute, attr);
+	int bar = (int)(long)resize_attr->var;
+	ssize_t ret;
+
+	pci_config_pm_runtime_get(pdev);
+
+	/*
+	 * pci_rebar_get_possible_sizes() only currently reads supported sizes
+	 * from the capability register and therefore returns a u32.  The spec
+	 * allows additional supported bits in the control register, which
+	 * then exceeds 32bit.  Expose a u64 to userspace for future compat.
+	 */
+	ret = sysfs_emit(buf, "%016llx:%d\n",
+			(u64)pci_rebar_get_possible_sizes(pdev, bar),
+			pci_rebar_get_current_size(pdev, bar));
+
+	pci_config_pm_runtime_put(pdev);
+
+	return ret;
+}
+
+static ssize_t pci_bar_resize_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct dev_ext_attribute *resize_attr =
+			container_of(attr, struct dev_ext_attribute, attr);
+	int ret, i, bar = (int)(long)resize_attr->var;
+	unsigned long size, flags;
+	u16 cmd;
+
+	if (kstrtoul(buf, 0, &size) < 0)
+		return -EINVAL;
+
+	device_lock(dev);
+	if (dev->driver) {
+		ret = -EBUSY;
+		goto unlock;
+	}
+
+	pci_config_pm_runtime_get(pdev);
+
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	pci_write_config_word(pdev, PCI_COMMAND, cmd & ~PCI_COMMAND_MEMORY);
+
+	flags = pci_resource_flags(pdev, bar);
+
+	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
+		if (pci_resource_len(pdev, i) &&
+		    pci_resource_flags(pdev, i) == flags)
+			pci_release_resource(pdev, i);
+	}
+
+	ret = pci_resize_resource(pdev, bar, size);
+
+	pci_assign_unassigned_bus_resources(pdev->bus);
+
+	pci_write_config_word(pdev, PCI_COMMAND, cmd);
+
+	pci_config_pm_runtime_put(pdev);
+unlock:
+	device_unlock(dev);
+
+	return ret ? ret : count;
+}
+
+static int pci_create_resize_attr(struct pci_dev *pdev, int num)
+{
+	struct dev_ext_attribute *resize_attr;
+	char *resize_attr_name;
+	int retval;
+
+	resize_attr = kzalloc(sizeof(*resize_attr) + 17, GFP_ATOMIC);
+	if (!resize_attr)
+		return -ENOMEM;
+
+	resize_attr_name = (char *)(resize_attr + 1);
+
+	sysfs_attr_init(&resize_attr->attr.attr);
+	sprintf(resize_attr_name, "resource%d_resize", num);
+	resize_attr->attr.attr.name = resize_attr_name;
+	resize_attr->attr.attr.mode = 0600;
+	resize_attr->attr.show = pci_bar_resize_show;
+	resize_attr->attr.store = pci_bar_resize_store;
+	resize_attr->var = (void *)(long)num;
+
+	retval = sysfs_create_file(&pdev->dev.kobj, &resize_attr->attr.attr);
+	if (retval) {
+		kfree(resize_attr);
+		return retval;
+	}
+
+	pdev->res_attr_resize[num] = resize_attr;
+	return 0;
+}
+
 /**
  * pci_create_resource_files - create resource files in sysfs for @dev
  * @pdev: dev in question
@@ -1235,6 +1345,14 @@ static int pci_create_resource_files(struct pci_dev *pdev)
 			pci_remove_resource_files(pdev);
 			return retval;
 		}
+
+		if (pci_rebar_get_current_size(pdev, i) >= 0) {
+			retval = pci_create_resize_attr(pdev, i);
+			if (retval) {
+				pci_remove_resource_files(pdev);
+				return retval;
+			}
+		}
 	}
 	return 0;
 }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 060af91bafcd..9c4db0c5f215 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -470,6 +470,7 @@ struct pci_dev {
 	int		rom_attr_enabled;	/* Display of ROM attribute enabled? */
 	struct bin_attribute *res_attr[DEVICE_COUNT_RESOURCE]; /* sysfs file for resources */
 	struct bin_attribute *res_attr_wc[DEVICE_COUNT_RESOURCE]; /* sysfs file for WC mapping of resources */
+	struct dev_ext_attribute *res_attr_resize[DEVICE_COUNT_RESOURCE]; /* sysfs file for resizing BAR resources */
 
 #ifdef CONFIG_HOTPLUG_PCI_PCIE
 	unsigned int	broken_cmd_compl:1;	/* No compl for some cmds */

Re: [PATCH 0/3] PCI: Expose resource resizing through sysfs

[Christian König]

Am 16.08.22 um 21:39 schrieb Alex Williamson:
> We have a couple graphics drivers making use of PCIe Resizable BARs
> now, but I've been trying to figure out how we can make use of such
> features for devices assigned to a VM.  This is a proposal for a
> rather basic interface in sysfs such that we have the ability to
> pre-enable larger BARs before we bind devices to vfio-pci and
> attach them to a VM.

Ah, yes please.

I was considering doing this myself just for testing while adding the 
rebar support for the GFX drivers, but then just implementing it on the 
GFX side was simpler.

I would just add a warning that resizing BARs can easily crash the 
system even when no driver directly claimed the resource or PCIe device.

It literally took me weeks to figure out that I need to kick out the EFI 
framebuffer driver before trying to resize the BAR or otherwise I just 
get a hung system.

> Along the way I found a double-free in the error path of creating
> resource attributes, that can certainly be pulled separately (1/).
>
> I'm using an RTX6000 for testing, which unexpectedly only supports
> REBAR with smaller than default sizes, which led me to question
> why we have such heavy requirements for shrinking resources (2/).

Oh, that's easy. You got tons of ARM boards with less than 512MiB of 
address space per root PCIe complex.

If you want to get a GPU working on those you need to decrease the BAR 
size or otherwise you won't be able to fit 256MiB VRAM BAR + register 
BAR into the same hole for the PCIe root complex.

An alternative explanation is that at least AMD produced some boards 
with a messed up resize configuration word. But on those you only got 
256MiB, 512MiB and 1GiB potential BAR sizes IIRC.

Anyway, with an appropriate warning added to the sysfs documentation the 
patch #2 and #3 are Acked-by: Christian König <christian.koenig@amd.com>

Regards,
Christian.

>
> The final patch proposes the sysfs interface and I'll leave the
> discussion there for whether this is a good approach.  Thanks,
>
> Alex
> ---
>
> Alex Williamson (3):
>        PCI: Fix double-free in resource attribute error path
>        PCI: Skip reassigning bridge resources if reducing BAR size
>        PCI: Expose PCIe Resizable BAR support via sysfs
>
>
>   Documentation/ABI/testing/sysfs-bus-pci |  27 +++++
>   drivers/pci/pci-sysfs.c                 | 126 +++++++++++++++++++++++-
>   drivers/pci/setup-res.c                 |   2 +-
>   include/linux/pci.h                     |   1 +
>   4 files changed, 154 insertions(+), 2 deletions(-)
>

Re: [PATCH 0/3] PCI: Expose resource resizing through sysfs

[Alex Williamson]

On Wed, 17 Aug 2022 12:10:44 +0200
Christian König <christian.koenig@amd.com> wrote:

> Am 16.08.22 um 21:39 schrieb Alex Williamson:
> > We have a couple graphics drivers making use of PCIe Resizable BARs
> > now, but I've been trying to figure out how we can make use of such
> > features for devices assigned to a VM.  This is a proposal for a
> > rather basic interface in sysfs such that we have the ability to
> > pre-enable larger BARs before we bind devices to vfio-pci and
> > attach them to a VM.  
> 
> Ah, yes please.
> 
> I was considering doing this myself just for testing while adding the 
> rebar support for the GFX drivers, but then just implementing it on the 
> GFX side was simpler.
> 
> I would just add a warning that resizing BARs can easily crash the 
> system even when no driver directly claimed the resource or PCIe device.
> 
> It literally took me weeks to figure out that I need to kick out the EFI 
> framebuffer driver before trying to resize the BAR or otherwise I just 
> get a hung system.

Good point, I think maybe we can avoid crashing the system though if we
use the new aperture support to remove conflicting drivers from all VGA
class devices, similar to d17378062079 ("vfio/pci: Remove console
drivers").  A note in the ABI documentation about removing console
drivers from the device when resizing resources would definitely be in
order.

> > Along the way I found a double-free in the error path of creating
> > resource attributes, that can certainly be pulled separately (1/).
> >
> > I'm using an RTX6000 for testing, which unexpectedly only supports
> > REBAR with smaller than default sizes, which led me to question
> > why we have such heavy requirements for shrinking resources (2/).  
> 
> Oh, that's easy. You got tons of ARM boards with less than 512MiB of 
> address space per root PCIe complex.
> 
> If you want to get a GPU working on those you need to decrease the
> BAR size or otherwise you won't be able to fit 256MiB VRAM BAR +
> register BAR into the same hole for the PCIe root complex.
> 
> An alternative explanation is that at least AMD produced some boards 
> with a messed up resize configuration word. But on those you only got 
> 256MiB, 512MiB and 1GiB potential BAR sizes IIRC.

An aspect of shrinking BARs that maybe I'm not giving enough
consideration to is that we might be shrinking a BAR on one device in
order to release MMIO space from a bridge aperture, that we might then
use to expand a BAR elsewhere.  The RTX6000 case only frees a rather
modest amount of MMIO space, but I could imagine more substantial
configurations.  Maybe this justifies resizing the bridge aperture even
in the shrinking case?

> Anyway, with an appropriate warning added to the sysfs documentation
> the patch #2 and #3 are Acked-by: Christian König
> <christian.koenig@amd.com>

Thanks!

Alex

Re: [PATCH 0/3] PCI: Expose resource resizing through sysfs

[Christian König]

Am 17.08.22 um 16:02 schrieb Alex Williamson:
> On Wed, 17 Aug 2022 12:10:44 +0200
> Christian König <christian.koenig@amd.com> wrote:
>
>> Am 16.08.22 um 21:39 schrieb Alex Williamson:
>>> We have a couple graphics drivers making use of PCIe Resizable BARs
>>> now, but I've been trying to figure out how we can make use of such
>>> features for devices assigned to a VM.  This is a proposal for a
>>> rather basic interface in sysfs such that we have the ability to
>>> pre-enable larger BARs before we bind devices to vfio-pci and
>>> attach them to a VM.
>> Ah, yes please.
>>
>> I was considering doing this myself just for testing while adding the
>> rebar support for the GFX drivers, but then just implementing it on the
>> GFX side was simpler.
>>
>> I would just add a warning that resizing BARs can easily crash the
>> system even when no driver directly claimed the resource or PCIe device.
>>
>> It literally took me weeks to figure out that I need to kick out the EFI
>> framebuffer driver before trying to resize the BAR or otherwise I just
>> get a hung system.
> Good point, I think maybe we can avoid crashing the system though if we
> use the new aperture support to remove conflicting drivers from all VGA
> class devices, similar to d17378062079 ("vfio/pci: Remove console
> drivers").  A note in the ABI documentation about removing console
> drivers from the device when resizing resources would definitely be in
> order.
>
>>> Along the way I found a double-free in the error path of creating
>>> resource attributes, that can certainly be pulled separately (1/).
>>>
>>> I'm using an RTX6000 for testing, which unexpectedly only supports
>>> REBAR with smaller than default sizes, which led me to question
>>> why we have such heavy requirements for shrinking resources (2/).
>> Oh, that's easy. You got tons of ARM boards with less than 512MiB of
>> address space per root PCIe complex.
>>
>> If you want to get a GPU working on those you need to decrease the
>> BAR size or otherwise you won't be able to fit 256MiB VRAM BAR +
>> register BAR into the same hole for the PCIe root complex.
>>
>> An alternative explanation is that at least AMD produced some boards
>> with a messed up resize configuration word. But on those you only got
>> 256MiB, 512MiB and 1GiB potential BAR sizes IIRC.
> An aspect of shrinking BARs that maybe I'm not giving enough
> consideration to is that we might be shrinking a BAR on one device in
> order to release MMIO space from a bridge aperture, that we might then
> use to expand a BAR elsewhere.  The RTX6000 case only frees a rather
> modest amount of MMIO space, but I could imagine more substantial
> configurations.  Maybe this justifies resizing the bridge aperture even
> in the shrinking case?

That was the original idea why I've kept that in there, yes.

But I never really seen a case where that really mattered.

So far making BARs smaller was something only the BIOS does.

Christian.

>
>> Anyway, with an appropriate warning added to the sysfs documentation
>> the patch #2 and #3 are Acked-by: Christian König
>> <christian.koenig@amd.com>
> Thanks!
>
> Alex
>