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

[Christian König]

Re-sending this as text from my private mail account since the AMD 
servers now seem to convert everything to HTML ^^.

Christian.

Am 11.10.24 um 10:57 schrieb Christian König:
> Am 10.10.24 um 10:59 schrieb Michał Winiarski:
> > On Fri, Sep 20, 2024 at 12:07:34PM +0200, Christian König wrote:
> > > Am 20.09.24 um 00:35 schrieb Michał Winiarski:
> > > > [SNIP]
> > > > @@ -487,6 +567,11 @@ static ssize_t sriov_numvfs_store(struct device *dev,
> > > >    		goto exit;
> > > >    	}
> > > > +	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > > > +		if (pdev->sriov->rebar_extend[i])
> > > > +			pci_iov_resource_do_extend(pdev, i + PCI_IOV_RESOURCES, num_vfs);
> > > > +	}
> > > > +
> > > That sounds like a really bad idea to me.
> > >
> > > Basically the suggestion is here that the PCI subsystem should silently
> > > extend and shrink the VF BARs when the number of VFs change?
> > Why do you think it's a bad idea? Everything is under PCI subsystem
> > control and the driver in charge has to explicitly opt-in to this
> > behavior on a per-BAR basis.
>
> And exactly that's a bad idea. The PCI subsystem shouldn't control 
> this, the driver should.
>
> At least for some devices we have tons of interactions with ACPI and 
> EFI. Only the driver does know for example when platform drivers which 
> might be in the way for a resize have been unloaded.
>
> From the past experience BAR resize should only be triggered by the 
> driver and never from the PCI subsystem while scanning the bus or 
> probing devices.
>
> > > Bjorn has the last word on that but I would say that instead the driver
> > > owning the PCIe device as hypervisor should resize the VF BARs to a desired
> > > size and that in turn restricts the number of VFs you can enable.
> > Then the PCI subsystem would silently change the driver_max_VFs (or new
> > variable, as driver_max_VFs is under PF control, so it's either new var
> > or checking VF BAR size in pci_sriov_set_totalvfs).
>
> Nope, the PCI subsystem should not magically adjust anything.
>
> What should happen instead is that the driver would call 
> pci_enable_sriov() with the number of virtual functions to enable and 
> the PCI subsystem then validates that number and return -EINVAL or 
> -ENOSPC if it won't work.
>
> > It also means that we have to do the maths to calculate the new VF limit
> > in both PCI subsystem and the caller.
>
> Well the point is that those calculations are different.
>
> What the subsystem does is to validate if with the number of requested 
> virtual functions the necessary resources will fit into the allocate 
> space.
>
> What the driver does previously is to either change the allocate space 
> or calculate the other way around and determine the maximum virtual 
> functions from the space available.
>
> > We can go this route as well - I just think it's cleaner to keep this
> > all under PCI subsystem control.
>
> I think that would be much cleaner, especially the PCI subsystem 
> shouldn't adjust any values given from the driver or even more general 
> overrule decisions the driver made.
>
> Instead proper error codes should be returned if some values don't 
> make sense or the subsystem isn't able to move around BARs currently 
> in use etc...
>
> Regards,
> Christian.
>
> > I'll keep the current behavior in v3, but I'm open to changing it.
> >
> > Thanks,
> > -Michał
> >
> > > Regards,
> > > Christian.
> > >
> > > >    	ret = pdev->driver->sriov_configure(pdev, num_vfs);
> > > >    	if (ret < 0)
> > > >    		goto exit;
> > > > @@ -881,8 +966,13 @@ static int sriov_init(struct pci_dev *dev, int pos)
> > > >    static void sriov_release(struct pci_dev *dev)
> > > >    {
> > > > +	int i;
> > > > +
> > > >    	BUG_ON(dev->sriov->num_VFs);
> > > > +	for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
> > > > +		pci_iov_resource_do_restore(dev, i + PCI_IOV_RESOURCES);
> > > > +
> > > >    	if (dev != dev->sriov->dev)
> > > >    		pci_dev_put(dev->sriov->dev);
> > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > > index e763b3fd4c7a2..47ed2633232aa 100644
> > > > --- a/drivers/pci/pci.h
> > > > +++ b/drivers/pci/pci.h
> > > > @@ -385,6 +385,7 @@ struct pci_sriov {
> > > >    	u16		subsystem_vendor; /* VF subsystem vendor */
> > > >    	u16		subsystem_device; /* VF subsystem device */
> > > >    	resource_size_t	barsz[PCI_SRIOV_NUM_BARS];	/* VF BAR size */
> > > > +	bool		rebar_extend[PCI_SRIOV_NUM_BARS];	/* Resize VF BAR */
> > > >    	bool		drivers_autoprobe; /* Auto probing of VFs by driver */
> > > >    };
> > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > index 4cf89a4b4cbcf..c007119da7b3d 100644
> > > > --- a/include/linux/pci.h
> > > > +++ b/include/linux/pci.h
> > > > @@ -2364,6 +2364,7 @@ int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
> > > >    int pci_sriov_get_totalvfs(struct pci_dev *dev);
> > > >    int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn);
> > > >    resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
> > > > +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable);
> > > >    void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
> > > >    /* Arch may override these (weak) */
> > > > @@ -2416,6 +2417,8 @@ static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
> > > >    #define pci_sriov_configure_simple	NULL
> > > >    static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
> > > >    { return 0; }
> > > > +static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable)
> > > > +{ return -ENODEV; }
> > > >    static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { }
> > > >    #endif