Re: [PATCH v2 2/3] PCI: Allow extending VF BAR within original resource boundary

[Michał Winiarski]

On Fri, Sep 20, 2024 at 02:30:00PM +0300, Ilpo Järvinen wrote:
> On Fri, 20 Sep 2024, Michał Winiarski wrote:
> 
> > VF MMIO resource reservation, either created by system firmware and
> > inherited by Linux PCI subsystem or created by the subsystem itself,
> > contains enough space to fit the BAR of all SR-IOV Virtual Functions
> > that can potentially be created (total VFs supported by the device).
> > This can be leveraged when the device is exposing lower than optimal BAR
> > size as a default, allowing access to the entire resource when lower
> > number of VFs are created.
> > It is achieved by dynamically resizing the BAR to largest possible value
> > that allows to fit all newly created VFs within the original resource
> > boundary.
> > 
> > Signed-off-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > ---
> >  drivers/pci/iov.c   | 92 ++++++++++++++++++++++++++++++++++++++++++++-
> >  drivers/pci/pci.h   |  1 +
> >  include/linux/pci.h |  3 ++
> >  3 files changed, 95 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index e8ccd2ae0f024..d88efbfa70e42 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -181,6 +181,86 @@ bool pci_iov_memory_decoding_enabled(struct pci_dev *dev)
> >  	return cmd & PCI_SRIOV_CTRL_MSE;
> >  }
> >  
> > +static void pci_iov_resource_do_extend(struct pci_dev *dev, int resno, u16 num_vfs)
> > +{
> > +	resource_size_t size;
> > +	int ret, old, i;
> > +	u32 sizes;
> > +
> > +	pci_config_pm_runtime_get(dev);
> > +
> > +	if (pci_iov_memory_decoding_enabled(dev)) {
> > +		ret = -EBUSY;
> > +		goto err;
> > +	}
> > +
> > +	sizes = pci_rebar_get_possible_sizes(dev, resno);
> > +	if (!sizes) {
> > +		ret = -ENOTSUPP;
> > +		goto err;
> > +	}
> > +
> > +	old = pci_rebar_get_current_size(dev, resno);
> > +	if (old < 0) {
> > +		ret = old;
> > +		goto err;
> > +	}
> > +
> > +	while (sizes > 0) {
> > +		i = __fls(sizes);
> > +		size = pci_rebar_size_to_bytes(i);
> > +		if (size * num_vfs <= pci_resource_len(dev, resno)) {
> > +			if (i != old) {
> > +				ret = pci_rebar_set_size(dev, resno, size);
> > +				if (ret)
> > +					goto err;
> > +
> > +				pci_iov_resource_set_size(dev, resno, size);
> > +				pci_iov_update_resource(dev, resno);
> > +			}
> > +			break;
> > +		}
> > +		sizes &= ~BIT(i);
> > +	}
> > +
> > +	pci_config_pm_runtime_put(dev);
> > +
> > +	return;
> > +
> > +err:
> > +	dev_WARN(&dev->dev, "Failed to extend %s: %d\n",
> > +		 pci_resource_name(dev, resno), ret);
> 
> Why do you use dev_WARN()? (analoguous to WARN_ON() / friends).
> 
> I suppose you'd want to use pci_warn() instead.

This should never happen - but fair point, with something like
panic_on_warn it would have more negative consequences. Let's downgrade
it to pci_warn().

> 
> > +	pci_config_pm_runtime_put(dev);
> > +}
> > +
> > +static void pci_iov_resource_do_restore(struct pci_dev *dev, int resno)
> > +{
> > +	if (dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES])
> > +		pci_iov_resource_do_extend(dev, resno, dev->sriov->total_VFs);
> > +}
> > +
> > +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable)
> > +{
> > +	if (!pci_resource_is_iov(dev, resno)) {
> > +		dev_WARN(&dev->dev, "%s is not an IOV resource\n",
> > +			 pci_resource_name(dev, resno));
> 
> pci_warn() ?

And this one is programmer error. But same as above, let's use
pci_warn().

Thanks
-Michał