linux-kernel - Re: [PATCH v4 1/2] PCI: endpoint: improve fixed

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] [day] [month] [year] [list]
Message-ID: <aAjsUXiK5PK7S6Jd@ryzen>
Date: Wed, 23 Apr 2025 15:34:09 +0200
From: Niklas Cassel <cassel@...nel.org>
To: Jerome Brunet <jbrunet@...libre.com>
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>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Jon Mason <jdmason@...zu.us>, Dave Jiang <dave.jiang@...el.com>,
	Allen Hubbe <allenbh@...il.com>, Christoph Hellwig <hch@....de>,
	Sagi Grimberg <sagi@...mberg.me>,
	Chaitanya Kulkarni <kch@...dia.com>,
	Marek Vasut <marek.vasut+renesas@...il.com>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	Yuya Hamamachi <yuya.hamamachi.sx@...esas.com>,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	ntb@...ts.linux.dev, linux-nvme@...ts.infradead.org
Subject: Re: [PATCH v4 1/2] PCI: endpoint: improve fixed_size bar handling
 when allocating space

On Tue, Apr 22, 2025 at 04:54:19PM +0200, Jerome Brunet wrote:
> When trying to allocate space for an endpoint function on a BAR with a
> fixed size, the size saved in the 'struct pci_epf_bar' should be the fixed
> size. This is expected by pci_epc_set_bar().
> 
> However, if the fixed_size is smaller that the alignment, the size saved
> in the 'struct pci_epf_bar' matches the alignment and it is a problem for
> pci_epc_set_bar().
> 
> To solve this, continue to allocate space that match the iATU alignment
> requirement, saving it as .aligned_size, then save the size that matches
> what is present in the BAR.
> 
> Fixes: 2a9a801620ef ("PCI: endpoint: Add support to specify alignment for buffers allocated to BARs")
> Signed-off-by: Jerome Brunet <jbrunet@...libre.com>
> ---
>  drivers/pci/endpoint/pci-epf-core.c | 21 ++++++++++++++-------
>  include/linux/pci-epf.h             |  3 +++
>  2 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index 394395c7f8decfa2010469655a4bd58a002993fd..982db6c1fbe77653f6a74a31df5c4e997507d2d8 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -236,12 +236,13 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
>  	}
>  
>  	dev = epc->dev.parent;
> -	dma_free_coherent(dev, epf_bar[bar].size, addr,
> +	dma_free_coherent(dev, epf_bar[bar].aligned_size, addr,
>  			  epf_bar[bar].phys_addr);
>  
>  	epf_bar[bar].phys_addr = 0;
>  	epf_bar[bar].addr = NULL;
>  	epf_bar[bar].size = 0;
> +	epf_bar[bar].aligned_size = 0;
>  	epf_bar[bar].barno = 0;
>  	epf_bar[bar].flags = 0;
>  }
> @@ -264,7 +265,7 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>  			  enum pci_epc_interface_type type)
>  {
>  	u64 bar_fixed_size = epc_features->bar[bar].fixed_size;
> -	size_t align = epc_features->align;
> +	size_t aligned_size, align = epc_features->align;
>  	struct pci_epf_bar *epf_bar;
>  	dma_addr_t phys_addr;
>  	struct pci_epc *epc;
> @@ -285,12 +286,17 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>  			return NULL;
>  		}
>  		size = bar_fixed_size;
> +	} else {
> +		/* BAR size must be power of two */
> +		size = roundup_pow_of_two(size);
>  	}
>  
> -	if (align)
> -		size = ALIGN(size, align);
> -	else
> -		size = roundup_pow_of_two(size);
> +	/*
> +	 * Allocate enough memory to accommodate the iATU alignment requirement.
> +	 * In most cases, this will be the same as .size but it might be different
> +	 * if, for example, the fixed size of a BAR is smaller than align.
> +	 */
> +	aligned_size = align ? ALIGN(size, align) : size;
>  
>  	if (type == PRIMARY_INTERFACE) {
>  		epc = epf->epc;
> @@ -301,7 +307,7 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>  	}
>  
>  	dev = epc->dev.parent;
> -	space = dma_alloc_coherent(dev, size, &phys_addr, GFP_KERNEL);
> +	space = dma_alloc_coherent(dev, aligned_size, &phys_addr, GFP_KERNEL);
>  	if (!space) {
>  		dev_err(dev, "failed to allocate mem space\n");
>  		return NULL;
> @@ -310,6 +316,7 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>  	epf_bar[bar].phys_addr = phys_addr;
>  	epf_bar[bar].addr = space;
>  	epf_bar[bar].size = size;
> +	epf_bar[bar].aligned_size = aligned_size;
>  	epf_bar[bar].barno = bar;
>  	if (upper_32_bits(size) || epc_features->bar[bar].only_64bit)
>  		epf_bar[bar].flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index 879d19cebd4fc6d8df9d724e3a52fa7fbd61e535..23b0878c2665db1c21e6e83543c33149ab1e0009 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -114,6 +114,8 @@ struct pci_epf_driver {
>   * @phys_addr: physical address that should be mapped to the BAR
>   * @addr: virtual address corresponding to the @phys_addr
>   * @size: the size of the address space present in BAR
> + * @aligned_size: the size actually allocated to accommodate the iATU alignment
> + *                requirement.

Nit: Since none of the other lines end with a full stop.
Perhaps add one for all lines, or continue not having it for all lines.

Regardless:
Reviewed-by: Niklas Cassel <cassel@...nel.org>