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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2714 bytes --]

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@intel.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.