linux-kernel - Re: [PATCH v7 3/6] PCI: endpoint: Add pci_epf_align

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: <Zzd6Kp1mliKGW2m0@lizhi-Precision-Tower-5810>
Date: Fri, 15 Nov 2024 11:43:22 -0500
From: Frank Li <Frank.li@....com>
To: Niklas Cassel <cassel@...nel.org>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>, Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	imx@...ts.linux.dev, dlemoal@...nel.org, maz@...nel.org,
	tglx@...utronix.de, jdmason@...zu.us
Subject: Re: [PATCH v7 3/6] PCI: endpoint: Add pci_epf_align_addr() helper
 for address alignment

On Fri, Nov 15, 2024 at 10:53:07AM +0100, Niklas Cassel wrote:
> On Thu, Nov 14, 2024 at 05:52:39PM -0500, Frank Li wrote:
> > Introduce the helper function pci_epf_align_addr() to adjust addresses
> > according to PCI BAR alignment requirements, converting addresses into base
> > and offset values.
> >
> > Signed-off-by: Frank Li <Frank.Li@....com>
> > ---
> > change from v6 to v7
> > - new patch
> > ---
> >  drivers/pci/endpoint/pci-epf-core.c | 39 +++++++++++++++++++++++++++++++++++++
> >  include/linux/pci-epf.h             | 13 +++++++++++++
> >  2 files changed, 52 insertions(+)
> >
> > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > index 8fa2797d4169a..a3f172cc786e9 100644
> > --- a/drivers/pci/endpoint/pci-epf-core.c
> > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > @@ -464,6 +464,45 @@ struct pci_epf *pci_epf_create(const char *name)
> >  }
> >  EXPORT_SYMBOL_GPL(pci_epf_create);
> >
> > +/**
> > + * pci_epf_align_addr() - Get base address and offset that match bar's
> > + *			  alignment requirement
> > + * @epf: the EPF device
> > + * @addr: the address of the memory
> > + * @bar: the BAR number corresponding to map addr
> > + * @base: return base address, which match BAR's alignment requirement, nothing
> > + *	  return if NULL
> > + * @off: return offset, nothing return if NULL
> > + *
> > + * Helper function to convert input 'addr' to base and offset, which match
> > + * BAR's alignment requirement.
> > + */
> > +int pci_epf_align_addr(struct pci_epf *epf, enum pci_barno bar, u64 addr, u64 *base, size_t *off)
>
> Nit: perhaps rename this function to:
> pci_epf_align_ib_addr()
> or
> pci_epf_align_inbound_addr()
>
> to more clearly not confuse this with:
> if (epc->ops->align_addr)
> .align_addr()
> (Ideally those functions should have been named align_ob_addr(),
> or align_outbound_addr())
>
>
> > +{
> > +	const struct pci_epc_features *epc_features;
> > +	u64 align;
> > +
> > +	epc_features = pci_epc_get_features(epf->epc, epf->func_no, epf->vfunc_no);
> > +	if (!epc_features) {
> > +		dev_err(&epf->dev, "epc_features not implemented\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	align = epc_features->align;
> > +	align = align ? align : 128;
> > +	if (epc_features->bar[bar].type == BAR_FIXED)
> > +		align = max(epc_features->bar[bar].fixed_size, align);
> > +
> > +	if (base)
> > +		*base = round_down(addr, align);
> > +
> > +	if (off)
> > +		*off = addr & (align - 1);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_epf_align_addr);
> > +
> >  static void pci_epf_dev_release(struct device *dev)
> >  {
> >  	struct pci_epf *epf = to_pci_epf(dev);
> > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > index 5374e6515ffa0..20f4f31ba9b36 100644
> > --- a/include/linux/pci-epf.h
> > +++ b/include/linux/pci-epf.h
> > @@ -238,6 +238,19 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> >  			  enum pci_epc_interface_type type);
> >  void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
> >  			enum pci_epc_interface_type type);
> > +
> > +int pci_epf_align_addr(struct pci_epf *epf, enum pci_barno bar, u64 addr, u64 *base, size_t *off);
> > +static inline int pci_epf_align_addr_lo_hi(struct pci_epf *epf, enum pci_barno bar,
> > +					   u32 low, u32 high, u64 *base, size_t *off)
> > +{
> > +	u64 addr = high;
> > +
> > +	addr <<= 32;
> > +	addr |= low;
> > +
> > +	return pci_epf_align_addr(epf, bar, addr, base, off);
> > +}
>
> I'm not sure if this function deserves to live :)
> Can't the caller just do this before calling pci_epf_align_addr() ?

Ideally, kernel should have macro to combine 32bit macro to a 64bit, but
I have not found it.

It is quite easy to make error or warning by simple
(high << 32 | low)

It needs ((u64) high << 32 | low) at least. I just want to avoid everyone
to struggle simple issue.

And msi_msg use lo and hi. pci_function_test actually just demostrate how
to use doorbell. if other function driver use doorbell in future, avoid
"(u64) high << 32 | low" copy to everywhere.

Maybe like upper_32_bits(), add global helper macro

#define low32_high32_to_64bit(l, h) ((u64)(h) << 32 | low)

Frank
>
>
> Kind regards,
> Niklas