lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fnisbg2bng3f5rkcoc7duzi34g7hghcqgzzehc5v6yb772kdj4@rcjs4mftf7s6>
Date: Wed, 2 Apr 2025 12:43:48 +0200
From: Michał Winiarski <michal.winiarski@...el.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC: <linux-pci@...r.kernel.org>, <intel-xe@...ts.freedesktop.org>,
	<dri-devel@...ts.freedesktop.org>, LKML <linux-kernel@...r.kernel.org>,
	"Bjorn Helgaas" <bhelgaas@...gle.com>, Christian König
	<christian.koenig@....com>, Krzysztof Wilczyński
	<kw@...ux.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 v6 5/6] PCI: Allow drivers to control VF BAR size

On Wed, Mar 26, 2025 at 05:22:50PM +0200, Ilpo Järvinen wrote:
> On Thu, 20 Mar 2025, Michał Winiarski wrote:
> 
> > Drivers could leverage the fact that the VF BAR MMIO reservation is
> > created for total number of VFs supported by the device by resizing the
> > BAR to larger size when smaller number of VFs is enabled.
> > 
> > Add a pci_iov_vf_bar_set_size() function to control the size and a
> > pci_iov_vf_bar_get_sizes() helper to get the VF BAR sizes that will
> > allow up to num_vfs to be successfully enabled with the current
> > underlying reservation size.
> > 
> > Signed-off-by: Michał Winiarski <michal.winiarski@...el.com>
> > ---
> >  drivers/pci/iov.c   | 78 +++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/pci.h |  6 ++++
> >  2 files changed, 84 insertions(+)
> > 
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index 861273ad9a580..751eef232685c 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -1291,3 +1291,81 @@ int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn)
> >  	return nr_virtfn;
> >  }
> >  EXPORT_SYMBOL_GPL(pci_sriov_configure_simple);
> > +
> > +/**
> > + * pci_iov_vf_bar_set_size - set a new size for a VF BAR
> > + * @dev: the PCI device
> > + * @resno: the resource number
> > + * @size: new size as defined in the spec (0=1MB, 31=128TB)
> > + *
> > + * Set the new size of a VF BAR that supports VF resizable BAR capability.
> > + * Unlike pci_resize_resource(), this does not cause the resource that
> > + * reserves the MMIO space (originally up to total_VFs) to be resized, which
> > + * means that following calls to pci_enable_sriov() can fail if the resources
> > + * no longer fit.
> > + *
> > + * Returns 0 on success, or negative on failure.
> 
> Return: is the correct kernel doc style.

Yeah, I just blindly followed the style from surrounding docs. I'll
change it here.

> 
> > + */
> > +int pci_iov_vf_bar_set_size(struct pci_dev *dev, int resno, int size)
> > +{
> > +	int ret;
> > +	u32 sizes;
> > +
> > +	if (!pci_resource_is_iov(resno))
> > +		return -EINVAL;
> > +
> > +	if (pci_iov_is_memory_decoding_enabled(dev))
> > +		return -EBUSY;
> > +
> > +	sizes = pci_rebar_get_possible_sizes(dev, resno);
> > +	if (!sizes)
> > +		return -ENOTSUPP;
> > +
> > +	if (!(sizes & BIT(size)))
> > +		return -EINVAL;
> > +
> > +	ret = pci_rebar_set_size(dev, resno, size);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pci_iov_resource_set_size(dev, resno, pci_rebar_size_to_bytes(size));
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_iov_vf_bar_set_size);
> > +
> > +/**
> > + * pci_iov_vf_bar_get_sizes - get VF BAR sizes allowing to create up to num_vfs
> > + * @dev: the PCI device
> > + * @resno: the resource number
> > + * @num_vfs: number of VFs
> > + *
> > + * Get the sizes of a VF resizable BAR that can be accommodated within the
> > + * resource that reserves the MMIO space if num_vfs are enabled.
> 
> I'd rephrase to:
> 
> Get the sizes of a VF resizable BAR that can be accommodate @num_vfs 
> within the currently assigned size of the resource @resno.

Ok.

> 
> > + *
> > + * Returns 0 if BAR isn't resizable, otherwise returns a bitmask in format
> 
> Return:
> 
> a bitmask of sizes

Ok.

> 
> > + * defined in the spec (bit 0=1MB, bit 31=128TB).
> > + */
> > +u32 pci_iov_vf_bar_get_sizes(struct pci_dev *dev, int resno, int num_vfs)
> > +{
> > +	resource_size_t size;
> > +	u32 sizes;
> > +	int i;
> > +
> > +	sizes = pci_rebar_get_possible_sizes(dev, resno);
> > +	if (!sizes)
> > +		return 0;
> > +
> > +	while (sizes > 0) {
> > +		i = __fls(sizes);
> > +		size = pci_rebar_size_to_bytes(i);
> > +
> > +		if (size * num_vfs <= pci_resource_len(dev, resno))
> > +			break;
> > +
> > +		sizes &= ~BIT(i);
> > +	}
> 
> Couldn't this be handled without a loop:
> 
> 	bar_sizes = (round_up(pci_resource_len(dev, resno) / num_vfs) - 1) >>
> 		    ilog2(SZ_1M);
> 
> 	sizes &= bar_sizes;
> 
> (Just to given an idea, I wrote this into the email so it might contain 
> some off-by-one errors or like).

I think the division will need to be wrapped with something like do_div
(because IIUC, we have 32bit architectures where resource_size_t is
u64).

But yeah, we can drop the loop, turning it into something like this:

	vf_len = pci_resource_len(dev, resno);
	do_div(vf_len, num_vfs);
	sizes = (roundup_pow_of_two(vf_len + 1) - 1) >> ilog2(SZ_1M);

Thanks,
-Michał

> 
> > +
> > +	return sizes;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_iov_vf_bar_get_sizes);
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 0e8e3fd77e967..c8708f3749757 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -2389,6 +2389,8 @@ 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_vf_bar_set_size(struct pci_dev *dev, int resno, int size);
> > +u32 pci_iov_vf_bar_get_sizes(struct pci_dev *dev, int resno, int num_vfs);
> >  void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
> >  
> >  /* Arch may override these (weak) */
> > @@ -2441,6 +2443,10 @@ 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 int pci_iov_vf_bar_set_size(struct pci_dev *dev, int resno, int size)
> > +{ return -ENODEV; }
> > +static inline u32 pci_iov_vf_bar_get_sizes(struct pci_dev *dev, int resno, int num_vfs)
> > +{ return 0; }
> >  static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { }
> >  #endif
> >  
> > 
> 
> -- 
>  i.


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ