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

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

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
>