[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241028165604.GA1105091@bhelgaas>
Date: Mon, 28 Oct 2024 11:56:04 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Michał Winiarski <michal.winiarski@...el.com>
Cc: linux-pci@...r.kernel.org, intel-xe@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
Bjorn Helgaas <bhelgaas@...gle.com>,
Christian König <christian.koenig@....com>,
Krzysztof Wilczyński <kw@...ux.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>,
Michal Wajdeczko <michal.wajdeczko@...el.com>,
Lucas De Marchi <lucas.demarchi@...el.com>,
Thomas Hellström <thomas.hellstrom@...ux.intel.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Matt Roper <matthew.d.roper@...el.com>
Subject: Re: [PATCH v4 5/7] PCI/IOV: Check that VF BAR fits within the
reservation
On Fri, Oct 25, 2024 at 11:50:36PM +0200, Michał Winiarski wrote:
> VF MMIO resource reservation, either created by system firmware and
> inherited by Linux PCI subsystem or created by the subsystem itself,
> should contain enough space to fit the BAR of all SR-IOV Virtual
> Functions that can potentially be created (total VFs supported by the
> device).
I don't think "VF resource reservation ... should contain enough
space" is really accurate or actionable. It would be *nice* if the PF
BAR is large enough to accommodate the largest supported VF BARs for
all possible VFs, but if it doesn't, it's not really an error. It's
just a reflection of the fact that resource space is limited.
> However, that assumption only holds in an environment where VF BAR size
> can't be modified.
There's no reason to assume anything about how many VF BARs fit. The
existing code should avoid enabling the requested nr_virtfn VFs if the
PF doesn't have enough space -- I think that's what the "if
(res->parent)" is supposed to be checking.
The fact that you need a change here makes me suspect that we're
missing some resource claim (and corresponding res->parent update)
elsewhere when resizing the VF BAR.
> Add an additional check that verifies that VF BAR for all enabled VFs
> fits within the underlying reservation resource.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@...el.com>
> ---
> drivers/pci/iov.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 79143c1bc7bb4..5de828e5a26ea 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -645,10 +645,14 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>
> nres = 0;
> for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> + int vf_bar_sz = pci_iov_resource_size(dev,
> + pci_resource_to_iov(i));
> bars |= (1 << pci_resource_to_iov(i));
> res = &dev->resource[pci_resource_to_iov(i)];
> - if (res->parent)
> - nres++;
> + if (!res->parent || vf_bar_sz * nr_virtfn > resource_size(res))
> + continue;
> +
> + nres++;
> }
> if (nres != iov->nres) {
> pci_err(dev, "not enough MMIO resources for SR-IOV\n");
> --
> 2.47.0
>
Powered by blists - more mailing lists