On Fri, Mar 28, 2025 at 06:39:29PM +0200, Ilpo Järvinen wrote:
> On Wed, 26 Mar 2025, Ilpo Järvinen wrote:
>
> > On Thu, 20 Mar 2025, Michał Winiarski wrote:
> >
> > > When the resource representing VF MMIO BAR reservation is created, its
> > > size is always large enough to accommodate the BAR of all SR-IOV Virtual
> > > Functions that can potentially be created (total VFs). If for whatever
> > > reason it's not possible to accommodate all VFs - the resource is not
> > > assigned and no VFs can be created.
> > >
> > > The following patch will allow VF BAR size to be modified by drivers at
> >
> > "The following patch" sounds to be like you're referring to patch that
> > follows this description, ie., the patch below. "An upcoming change" is
> > alternative that doesn't suffer from the same problem.
> >
> > > a later point in time, which means that the check for resource
> > > assignment is no longer sufficient.
> > >
> > > Add an additional check that verifies that VF BAR for all enabled VFs
> > > fits within the underlying reservation resource.
> >
> > So this does not solve the case where the initial size was too large to
> > fix and such VF BARs remain unassigned, right?
> >
> > > Signed-off-by: Michał Winiarski <michal.winiar...@intel.com>
> > > ---
> > > drivers/pci/iov.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > > index cbf335725d4fb..861273ad9a580 100644
> > > --- a/drivers/pci/iov.c
> > > +++ b/drivers/pci/iov.c
> > > @@ -646,8 +646,13 @@ static int sriov_enable(struct pci_dev *dev, int
> > > nr_virtfn)
> > >
> > > nres = 0;
> > > for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> > > + resource_size_t vf_bar_sz =
> > > + pci_iov_resource_size(dev,
> > > + pci_resource_num_from_vf_bar(i));
> >
> > Please add int idx = pci_resource_num_from_vf_bar(i);
> >
> > > bars |= (1 << pci_resource_num_from_vf_bar(i));
> > > res = &dev->resource[pci_resource_num_from_vf_bar(i)];
> > > + if (vf_bar_sz * nr_virtfn > resource_size(res))
> > > + continue;
> >
> > Not directly related to this patch, I suspect this could actually try to
> > assign an unassigned resource by doing something like this (perhaps in own
> > patch, it doesn't even need to be part of this series but can be sent
> > later if you find the suggestion useful):
> >
> > /* Retry assignment if the initial size didn't fit */
> > if (!res->parent && pci_assign_resource(res, idx))
> > continue;
> >
> > Although I suspect reset_resource() might have been called for the
> > resource and IIRC it breaks the resource somehow but it could have been
> > that IOV resources can be resummoned from that state though thanks to
> > their size not being stored into the resource itself but comes from iov
> > structures.
>
> I realized reset_resource() will zero the flags so it won't work without
> getting rid of reset_resource() calls first which I've not yet completed.
>
> And once I get the rebar sizes included into bridge window sizing
> algorithm, the default size could possibly be shrunk by the resource
> fitting/assignment code so the resource assignment should no longer fail
> just because the initial size was too large. So it shouldn't be necessary
> after that.
Yeah - and even if something fails in the resource constrained
environment, I think the flow used to reassign the PF resource (and
bring back the ability to create VFs) should involve remove -> rescan
(not just VF enabling).
Thanks,
-Michał
>
> > > if (res->parent)
> > > nres++;
> > > }
> > >
> >
> >
>
> --
> i.
