linux-kernel - Re: [PATCH v8 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: <20241124073239.5yl5zsmrrcrhmibh@thinkpad>
Date: Sun, 24 Nov 2024 13:02:39 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Frank Li <Frank.Li@....com>
Cc: 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, Niklas Cassel <cassel@...nel.org>,
	dlemoal@...nel.org, maz@...nel.org, tglx@...utronix.de,
	jdmason@...zu.us
Subject: Re: [PATCH v8 3/6] PCI: endpoint: Add pci_epf_align_addr() helper
 for address alignment

On Sat, Nov 16, 2024 at 09:40:43AM -0500, Frank Li wrote:
> Introduce the helper function pci_epf_align_addr() to adjust addresses

pci_epf_align_inbound_addr()?

> according to PCI BAR alignment requirements, converting addresses into base
> and offset values.
> 
> Signed-off-by: Frank Li <Frank.Li@....com>
> ---
> change from v7 to v8
> - change name to pci_epf_align_inbound_addr()
> - update comment said only need for memory, which not allocated by
> pci_epf_alloc_space().
> 
> change from v6 to v7
> - new patch
> ---
>  drivers/pci/endpoint/pci-epf-core.c | 45 +++++++++++++++++++++++++++++++++++++
>  include/linux/pci-epf.h             | 14 ++++++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index 8fa2797d4169a..4dfc218ebe20b 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -464,6 +464,51 @@ struct pci_epf *pci_epf_create(const char *name)
>  }
>  EXPORT_SYMBOL_GPL(pci_epf_create);
>  
> +/**
> + * pci_epf_align_inbound_addr() - Get base address and offset that match bar's

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

Below, you are updating 'base' only if it is not NULL. Why would anyone call
this API with 'base' and 'offset' set to NULL?

> + * @off: return offset, nothing return if NULL
> + *
> + * Helper function to convert input 'addr' to base and offset, which match
> + * BAR's alignment requirement.
> + *
> + * The pci_epf_alloc_space() function already accounts for alignment. This is
> + * primarily intended for use with other memory regions not allocated by
> + * pci_epf_alloc_space(), such as peripheral register spaces or the trigger
> + * address for a platform MSI controller.
> + */
> +int pci_epf_align_inbound_addr(struct pci_epf *epf, enum pci_barno bar,
> +			       u64 addr, u64 *base, size_t *off)
> +{
> +	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_inbound_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..eff73ccb5e702 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -238,6 +238,20 @@ 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_inbound_addr(struct pci_epf *epf, enum pci_barno bar,
> +			       u64 addr, u64 *base, size_t *off);
> +static inline int pci_epf_align_inbound_addr_lo_hi(struct pci_epf *epf, enum pci_barno bar,
> +						   u32 low, u32 high, u64 *base, size_t *off)

Why can't you just use pci_epf_align_inbound_addr() directly? Or the caller
could pass u64 address directly.

- Mani

> +{
> +	u64 addr = high;
> +
> +	addr <<= 32;
> +	addr |= low;
> +
> +	return pci_epf_align_inbound_addr(epf, bar, addr, base, off);
> +}
> +
>  int pci_epf_bind(struct pci_epf *epf);
>  void pci_epf_unbind(struct pci_epf *epf);
>  int pci_epf_add_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
> 
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்