[PATCH v2 1/1] PCI: Fix BAR resizing when VF BARs are assigned

[PATCH v2 1/1] PCI: Fix BAR resizing when VF BARs are assigned

[Ilpo Järvinen]

__resource_resize_store() attempts to release all resources of the
device before attempting the resize. The loop, however, only covers
standard BARs (< PCI_STD_NUM_BARS). If a device has VF BARs that are
assigned, pci_reassign_bridge_resources() finds the bridge window still
has some assigned child resources and returns -NOENT which makes
pci_resize_resource() to detect an error and abort the resize.

Change the release loop to cover all resources up to VF BARs which
allows the resize operation to release the bridge windows and attempt
to assigned them again with the different size.

As __resource_resize_store() checks first that no driver is bound to
the PCI device before resizing is allowed, SR-IOV cannot be enabled
during resize so it is safe to release also the IOV resources.

Fixes: 91fa127794ac ("PCI: Expose PCIe Resizable BAR support via sysfs")
Reported-by: Michał Winiarski <michal.winiarski@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

v2:
- Removed language about expansion ROMs

 drivers/pci/pci-sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index b46ce1a2c554..0c16751bab40 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1578,7 +1578,7 @@ static ssize_t __resource_resize_store(struct device *dev, int n,
 
 	pci_remove_resource_files(pdev);
 
-	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
+	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
 		if (pci_resource_len(pdev, i) &&
 		    pci_resource_flags(pdev, i) == flags)
 			pci_release_resource(pdev, i);

base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
-- 
2.39.5

Re: [PATCH v2 1/1] PCI: Fix BAR resizing when VF BARs are assigned

[Alex Williamson]

On Fri,  7 Mar 2025 16:03:49 +0200
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> __resource_resize_store() attempts to release all resources of the
> device before attempting the resize. The loop, however, only covers
> standard BARs (< PCI_STD_NUM_BARS). If a device has VF BARs that are
> assigned, pci_reassign_bridge_resources() finds the bridge window still
> has some assigned child resources and returns -NOENT which makes
> pci_resize_resource() to detect an error and abort the resize.
> 
> Change the release loop to cover all resources up to VF BARs which
> allows the resize operation to release the bridge windows and attempt
> to assigned them again with the different size.
> 
> As __resource_resize_store() checks first that no driver is bound to
> the PCI device before resizing is allowed, SR-IOV cannot be enabled
> during resize so it is safe to release also the IOV resources.

Is this true?  pci-pf-stub doesn't teardown SR-IOV on release, which I
understand is done intentionally.  Thanks,

Alex
 
> Fixes: 91fa127794ac ("PCI: Expose PCIe Resizable BAR support via sysfs")
> Reported-by: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> 
> v2:
> - Removed language about expansion ROMs
> 
>  drivers/pci/pci-sysfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index b46ce1a2c554..0c16751bab40 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1578,7 +1578,7 @@ static ssize_t __resource_resize_store(struct device *dev, int n,
>  
>  	pci_remove_resource_files(pdev);
>  
> -	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> +	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
>  		if (pci_resource_len(pdev, i) &&
>  		    pci_resource_flags(pdev, i) == flags)
>  			pci_release_resource(pdev, i);
> 
> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b

Re: [PATCH v2 1/1] PCI: Fix BAR resizing when VF BARs are assigned

[Michał Winiarski]

On Fri, Mar 14, 2025 at 08:56:49AM -0600, Alex Williamson wrote:
> On Fri,  7 Mar 2025 16:03:49 +0200
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > __resource_resize_store() attempts to release all resources of the
> > device before attempting the resize. The loop, however, only covers
> > standard BARs (< PCI_STD_NUM_BARS). If a device has VF BARs that are
> > assigned, pci_reassign_bridge_resources() finds the bridge window still
> > has some assigned child resources and returns -NOENT which makes
> > pci_resize_resource() to detect an error and abort the resize.
> > 
> > Change the release loop to cover all resources up to VF BARs which
> > allows the resize operation to release the bridge windows and attempt
> > to assigned them again with the different size.
> > 
> > As __resource_resize_store() checks first that no driver is bound to
> > the PCI device before resizing is allowed, SR-IOV cannot be enabled
> > during resize so it is safe to release also the IOV resources.
> 
> Is this true?  pci-pf-stub doesn't teardown SR-IOV on release, which I
> understand is done intentionally.  Thanks,

Is that really intentional?
PCI warns when that scenario occurs:
https://elixir.bootlin.com/linux/v6.13.7/source/drivers/pci/iov.c#L936

I thought that the usecase is binding pci-pf-stub, creating VFs, and
letting the driver be.
But unbinding after creating VFs? What's the goal of that?
Perhaps we're just missing .remove() in pci-pf-stub?

Thanks,
-Michał

> 
> Alex
>  
> > Fixes: 91fa127794ac ("PCI: Expose PCIe Resizable BAR support via sysfs")
> > Reported-by: Michał Winiarski <michal.winiarski@intel.com>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> > 
> > v2:
> > - Removed language about expansion ROMs
> > 
> >  drivers/pci/pci-sysfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index b46ce1a2c554..0c16751bab40 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -1578,7 +1578,7 @@ static ssize_t __resource_resize_store(struct device *dev, int n,
> >  
> >  	pci_remove_resource_files(pdev);
> >  
> > -	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> > +	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
> >  		if (pci_resource_len(pdev, i) &&
> >  		    pci_resource_flags(pdev, i) == flags)
> >  			pci_release_resource(pdev, i);
> > 
> > base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
> 

Re: [PATCH v2 1/1] PCI: Fix BAR resizing when VF BARs are assigned

[Alex Williamson]

On Mon, 17 Mar 2025 19:18:03 +0100
Michał Winiarski <michal.winiarski@intel.com> wrote:

> On Fri, Mar 14, 2025 at 08:56:49AM -0600, Alex Williamson wrote:
> > On Fri,  7 Mar 2025 16:03:49 +0200
> > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> >   
> > > __resource_resize_store() attempts to release all resources of the
> > > device before attempting the resize. The loop, however, only covers
> > > standard BARs (< PCI_STD_NUM_BARS). If a device has VF BARs that are
> > > assigned, pci_reassign_bridge_resources() finds the bridge window still
> > > has some assigned child resources and returns -NOENT which makes
> > > pci_resize_resource() to detect an error and abort the resize.
> > > 
> > > Change the release loop to cover all resources up to VF BARs which
> > > allows the resize operation to release the bridge windows and attempt
> > > to assigned them again with the different size.
> > > 
> > > As __resource_resize_store() checks first that no driver is bound to
> > > the PCI device before resizing is allowed, SR-IOV cannot be enabled
> > > during resize so it is safe to release also the IOV resources.  
> > 
> > Is this true?  pci-pf-stub doesn't teardown SR-IOV on release, which I
> > understand is done intentionally.  Thanks,  
> 
> Is that really intentional?
> PCI warns when that scenario occurs:
> https://elixir.bootlin.com/linux/v6.13.7/source/drivers/pci/iov.c#L936

Yep, it warns.  It doesn't prevent it from happening though.

> I thought that the usecase is binding pci-pf-stub, creating VFs, and
> letting the driver be.
> But unbinding after creating VFs? What's the goal of that?
> Perhaps we're just missing .remove() in pci-pf-stub?

I guess I don't actually know that leaving SR-IOV enabled was
intentional, maybe it was an oversight.  The original commit only
mentions the case of a device that requires nothing but this shim as
the PF driver.  A pci_warn() isn't much disincentive, the system might
already have taints.  If it's something that we really want to show as
broken, it'd probably need to be a WARN_ON.  Thanks,

Alex

Re: [PATCH v2 1/1] PCI: Fix BAR resizing when VF BARs are assigned

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2661 bytes --]

+ Jakub
+ Alexander

On Mon, 17 Mar 2025, Alex Williamson wrote:
> On Mon, 17 Mar 2025 19:18:03 +0100
> Michał Winiarski <michal.winiarski@intel.com> wrote:
> > On Fri, Mar 14, 2025 at 08:56:49AM -0600, Alex Williamson wrote:
> > > On Fri,  7 Mar 2025 16:03:49 +0200
> > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> > >   
> > > > __resource_resize_store() attempts to release all resources of the
> > > > device before attempting the resize. The loop, however, only covers
> > > > standard BARs (< PCI_STD_NUM_BARS). If a device has VF BARs that are
> > > > assigned, pci_reassign_bridge_resources() finds the bridge window still
> > > > has some assigned child resources and returns -NOENT which makes
> > > > pci_resize_resource() to detect an error and abort the resize.
> > > > 
> > > > Change the release loop to cover all resources up to VF BARs which
> > > > allows the resize operation to release the bridge windows and attempt
> > > > to assigned them again with the different size.
> > > > 
> > > > As __resource_resize_store() checks first that no driver is bound to
> > > > the PCI device before resizing is allowed, SR-IOV cannot be enabled
> > > > during resize so it is safe to release also the IOV resources.  
> > > 
> > > Is this true?  pci-pf-stub doesn't teardown SR-IOV on release, which I
> > > understand is done intentionally.  Thanks,  

Thanks for reviewing. I'm sorry I just took Michał's word on this for 
granted so I didn't check it myself.

I could amend __resource_resize_store() to return -EBUSY if SR-IOV is 
there despite no driver being present, but lets hear if Alexander or Jakub 
has some input on this.

> > Is that really intentional?
> > PCI warns when that scenario occurs:
> > https://elixir.bootlin.com/linux/v6.13.7/source/drivers/pci/iov.c#L936
> 
> Yep, it warns.  It doesn't prevent it from happening though.
> 
> > I thought that the usecase is binding pci-pf-stub, creating VFs, and
> > letting the driver be.
> > But unbinding after creating VFs? What's the goal of that?
> > Perhaps we're just missing .remove() in pci-pf-stub?
> 
> I guess I don't actually know that leaving SR-IOV enabled was
> intentional, maybe it was an oversight.  The original commit only
> mentions the case of a device that requires nothing but this shim as
> the PF driver.  A pci_warn() isn't much disincentive, the system might
> already have taints.  If it's something that we really want to show as
> broken, it'd probably need to be a WARN_ON. 

Added Alexander and Jakub, perhaps they remember something or if there are 
caveats going either way.

-- 
 i.

Re: [PATCH v2 1/1] PCI: Fix BAR resizing when VF BARs are assigned

[Philipp Stanner]

On Fri, 2025-03-07 at 16:03 +0200, Ilpo Järvinen wrote:
> __resource_resize_store() attempts to release all resources of the
> device before attempting the resize. The loop, however, only covers
> standard BARs (< PCI_STD_NUM_BARS). If a device has VF BARs that are
> assigned, pci_reassign_bridge_resources() finds the bridge window
> still
> has some assigned child resources and returns -NOENT which makes
> pci_resize_resource() to detect an error and abort the resize.
> 
> Change the release loop to cover all resources up to VF BARs which
> allows the resize operation to release the bridge windows and attempt
> to assigned them again with the different size.
> 
> As __resource_resize_store() checks first that no driver is bound to
> the PCI device before resizing is allowed, SR-IOV cannot be enabled
> during resize so it is safe to release also the IOV resources.
> 
> Fixes: 91fa127794ac ("PCI: Expose PCIe Resizable BAR support via
> sysfs")
> Reported-by: Michał Winiarski <michal.winiarski@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> 
> v2:
> - Removed language about expansion ROMs
> 
>  drivers/pci/pci-sysfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index b46ce1a2c554..0c16751bab40 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1578,7 +1578,7 @@ static ssize_t __resource_resize_store(struct
> device *dev, int n,
>  
>  	pci_remove_resource_files(pdev);
>  
> -	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> +	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {

I, just recently, fixed a similar bug (assuming it is one in this
particular case here) [1].

I think it would be great if someone who has the knowledge could
improve the documentation in linux/pci.h where those constants are
defined.

PCI_STD_NUM_BARS is even defined in a different header, hinting at a
different background.


P.

[1] https://lore.kernel.org/all/20250312080634.13731-2-phasta@kernel.org/


>  		if (pci_resource_len(pdev, i) &&
>  		    pci_resource_flags(pdev, i) == flags)
>  			pci_release_resource(pdev, i);
> 
> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b

Re: [PATCH v2 1/1] PCI: Fix BAR resizing when VF BARs are assigned

[Alex Williamson]

On Tue, 18 Mar 2025 13:42:57 +0200 (EET)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> + Jakub
> + Alexander
> 
> On Mon, 17 Mar 2025, Alex Williamson wrote:
> > On Mon, 17 Mar 2025 19:18:03 +0100
> > Michał Winiarski <michal.winiarski@intel.com> wrote:  
> > > On Fri, Mar 14, 2025 at 08:56:49AM -0600, Alex Williamson wrote:  
> > > > On Fri,  7 Mar 2025 16:03:49 +0200
> > > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> > > >     
> > > > > __resource_resize_store() attempts to release all resources of the
> > > > > device before attempting the resize. The loop, however, only covers
> > > > > standard BARs (< PCI_STD_NUM_BARS). If a device has VF BARs that are
> > > > > assigned, pci_reassign_bridge_resources() finds the bridge window still
> > > > > has some assigned child resources and returns -NOENT which makes
> > > > > pci_resize_resource() to detect an error and abort the resize.
> > > > > 
> > > > > Change the release loop to cover all resources up to VF BARs which
> > > > > allows the resize operation to release the bridge windows and attempt
> > > > > to assigned them again with the different size.
> > > > > 
> > > > > As __resource_resize_store() checks first that no driver is bound to
> > > > > the PCI device before resizing is allowed, SR-IOV cannot be enabled
> > > > > during resize so it is safe to release also the IOV resources.    
> > > > 
> > > > Is this true?  pci-pf-stub doesn't teardown SR-IOV on release, which I
> > > > understand is done intentionally.  Thanks,    
> 
> Thanks for reviewing. I'm sorry I just took Michał's word on this for 
> granted so I didn't check it myself.
> 
> I could amend __resource_resize_store() to return -EBUSY if SR-IOV is 
> there despite no driver being present

I probably never really considered resizing BARs for an SR-IOV capable
device when adding this support originally, but it seems valid to me
that if we extend releasing resources to the SR-IOV BARs that we simply
need to assert that SR-IOV is disabled and fail otherwise.  Thanks,

Alex

Re: [PATCH v2 1/1] PCI: Fix BAR resizing when VF BARs are assigned

[Alex Williamson]

On Tue, 18 Mar 2025 09:01:57 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Tue, 18 Mar 2025 13:42:57 +0200 (EET)
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > + Jakub
> > + Alexander
> > 
> > On Mon, 17 Mar 2025, Alex Williamson wrote:  
> > > On Mon, 17 Mar 2025 19:18:03 +0100
> > > Michał Winiarski <michal.winiarski@intel.com> wrote:    
> > > > On Fri, Mar 14, 2025 at 08:56:49AM -0600, Alex Williamson wrote:    
> > > > > On Fri,  7 Mar 2025 16:03:49 +0200
> > > > > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> > > > >       
> > > > > > __resource_resize_store() attempts to release all resources of the
> > > > > > device before attempting the resize. The loop, however, only covers
> > > > > > standard BARs (< PCI_STD_NUM_BARS). If a device has VF BARs that are
> > > > > > assigned, pci_reassign_bridge_resources() finds the bridge window still
> > > > > > has some assigned child resources and returns -NOENT which makes
> > > > > > pci_resize_resource() to detect an error and abort the resize.
> > > > > > 
> > > > > > Change the release loop to cover all resources up to VF BARs which
> > > > > > allows the resize operation to release the bridge windows and attempt
> > > > > > to assigned them again with the different size.
> > > > > > 
> > > > > > As __resource_resize_store() checks first that no driver is bound to
> > > > > > the PCI device before resizing is allowed, SR-IOV cannot be enabled
> > > > > > during resize so it is safe to release also the IOV resources.      
> > > > > 
> > > > > Is this true?  pci-pf-stub doesn't teardown SR-IOV on release, which I
> > > > > understand is done intentionally.  Thanks,      
> > 
> > Thanks for reviewing. I'm sorry I just took Michał's word on this for 
> > granted so I didn't check it myself.
> > 
> > I could amend __resource_resize_store() to return -EBUSY if SR-IOV is 
> > there despite no driver being present  
> 
> I probably never really considered resizing BARs for an SR-IOV capable
> device when adding this support originally, but it seems valid to me
> that if we extend releasing resources to the SR-IOV BARs that we simply
> need to assert that SR-IOV is disabled and fail otherwise.  Thanks,

Also, I've never seen it, but I'm under the impression that it's
possible for pre-boot to hand-off a device with SR-IOV already enabled.
Therefore I think this would be a worthwhile addition, regardless of the
behavior of any given in-kernel driver.  Thanks,

Alex