linux-kernel - Re: [PATCH v6 5/6] PCI: Allow drivers to control VF BAR size

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5de3951c-01f1-3892-09e1-f7d30a4e048d@linux.intel.com>
Date: Wed, 2 Apr 2025 15:04:44 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Michał Winiarski <michal.winiarski@...el.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, 2 Apr 2025, Michał Winiarski wrote:

> 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>


> > > +/**
> > > + * 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.

I have small grammar error in that:

"can be accomodate" -> "can accomodate"

> > > + * 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);

Yes, good point, 64-bit division is required.

-- 
 i.