Re: [PATCH v4 2/7] PCI: endpoint: Add pci_epc_bar_size_to_rebar_cap()

[Niklas Cassel <cassel@kernel.org>]

On Wed, Feb 19, 2025 at 11:49:27AM -0600, Bjorn Helgaas wrote:
> On Wed, Feb 19, 2025 at 06:24:10PM +0100, Niklas Cassel wrote:
> > On Tue, Feb 18, 2025 at 10:48:04AM -0600, Bjorn Helgaas wrote:
> > > On Fri, Jan 31, 2025 at 07:29:51PM +0100, Niklas Cassel wrote:
> > > > Add a helper function to convert a size to the representation used by the
> > > > Resizable BAR Capability Register.
> > > 
> > > > +/**
> > > > + * pci_epc_bar_size_to_rebar_cap() - convert a size to the representation used
> > > > + *				     by the Resizable BAR Capability Register
> > > > + * @size: the size to convert
> > > > + * @cap: where to store the result
> > > > + *
> > > > + * Returns 0 on success and a negative error code in case of error.
> > > > + */
> > > > +int pci_epc_bar_size_to_rebar_cap(size_t size, u32 *cap)
> > > > +{
> > > > +	/*
> > > > +	 * According to PCIe base spec, min size for a resizable BAR is 1 MB,
> > > > +	 * thus disallow a requested BAR size smaller than 1 MB.
> > > > +	 * Disallow a requested BAR size larger than 128 TB.
> > > > +	 */
> > > > +	if (size < SZ_1M || (u64)size > (SZ_128G * 1024))
> > > > +		return -EINVAL;
> > > > +
> > > > +	*cap = ilog2(size) - ilog2(SZ_1M);
> > > > +
> > > > +	/* Sizes in REBAR_CAP start at BIT(4). */
> > > > +	*cap = BIT(*cap + 4);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(pci_epc_bar_size_to_rebar_cap);
> > > 
> > > This doesn't seem specific to EPC.  It looks basically similar to
> > > pci_rebar_bytes_to_size().  Can we consolidate anything there?
> > 
> > This function is to convert a size to
> > "Resizable BAR Capability Register", which includes one or more supported
> > BAR sizes.
> > 
> > This register should only ever by written by a PCI endpoint function,
> > so I think its current home in drivers/pci/endpoint/ is correct.
> > 
> > pci_rebar_bytes_to_size() is used to convert a size to
> > "Resizable BAR Control Register", specifically the field
> > "BAR Size".
> > 
> > This "BAR Size" field in the "Resizable BAR Control Register" can be
> > written by the host side (or endpoint side), to select one of the
> > supported bar sizes. So I think it makes sense for
> > pci_rebar_bytes_to_size() to live in pci.h.
> 
> Thanks, I agree.
> 
> It looks like pci_epc_bar_size_to_rebar_cap() is only called once per
> BAR.  Does that mean an endpoint driver can only set a single
> supported size for a Resizable BAR in the Capability register?

Yes, that is the current design.

(Which is an improvement to before this series was merged, where a Resizable
BAR had to be forced to 1 MB size, so EPF drivers could not configure a size
larger/different than 1 MB.)



It is possible to allow the endpoint framework to allow multiple Resizable
BAR sizes to be supported in the future.

On e.g. rk3588, the EP gets an IRQ when the host side writes the "BAR Size"
field in the "Resizable BAR Control Register".

So one can image that the drivers could call some function in the EPC
framework that allocates + copies + frees new backing memory for the BARs.

But right now, I don't have a need for such a use-case, so let's defer that
until someone actually wants/needs that, since it will undoubtedly also add
more code complexity to the EPC framework.


Kind regards,
Niklas