Re: [PATCH 2/2] PCI: endpoint: Set prefetch when allocating memory for 64-bit BARs

[Niklas Cassel <cassel@kernel.org>]

On Thu, Feb 29, 2024 at 11:48:59AM +0100, Niklas Cassel wrote:
> From the PCIe 6.0 base spec:
> "Generally only 64-bit BARs are good candidates, since only Legacy
> Endpoints are permitted to set the Prefetchable bit in 32-bit BARs,
> and most scalable platforms map all 32-bit Memory BARs into
> non-prefetchable Memory Space regardless of the Prefetchable bit value."
> 
> "For a PCI Express Endpoint, 64-bit addressing must be supported for all
> BARs that have the Prefetchable bit Set. 32-bit addressing is permitted
> for all BARs that do not have the Prefetchable bit Set."
> 
> "Any device that has a range that behaves like normal memory should mark
> the range as prefetchable. A linear frame buffer in a graphics device is
> an example of a range that should be marked prefetchable."
> 
> The PCIe spec tells us that we should have the prefetchable bit set for
> 64-bit BARs backed by "normal memory". The backing memory that we allocate
> for a 64-bit BAR using pci_epf_alloc_space() (which calls
> dma_alloc_coherent()) is obviously "normal memory".
> 
> Thus, set the prefetchable bit when allocating backing memory for a 64-bit
> BAR.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/pci/endpoint/pci-epf-core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index e7dbbeb1f0de..10264d662abf 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -305,7 +305,8 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>  	epf_bar[bar].size = 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;
> +		epf_bar[bar].flags |= PCI_BASE_ADDRESS_MEM_TYPE_64 |
> +				      PCI_BASE_ADDRESS_MEM_PREFETCH;
>  	else
>  		epf_bar[bar].flags |= PCI_BASE_ADDRESS_MEM_TYPE_32;

This should probably be:

if (upper_32_bits(size) || epc_features->bar[bar].only_64bit)
	epf_bar[bar].flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
else
	epf_bar[bar].flags |= PCI_BASE_ADDRESS_MEM_TYPE_32;

if (epf_bar[bar].flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
	epf_bar[bar].flags |= PCI_BASE_ADDRESS_MEM_PREFETCH;

so that we set PREFETCH even for a EPF driver that had PCI_BASE_ADDRESS_MEM_TYPE_64
set in flags even before calling pci_epf_alloc_space. Will fix in V2.




I also found a bug in the existing code.
If pci_epf_alloc_space() allocated a 64-bit BAR because of bits in size,
then the increment in pci_epf_test_alloc_space() was incorrect.
(I guess no one uses BARs > 4GB).

--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -865,6 +865,12 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
                        dev_err(dev, "Failed to allocate space for BAR%d\n",
                                bar);
                epf_test->reg[bar] = base;
+
+               /*
+                * pci_epf_alloc_space() might have given us a 64-bit BAR,
+                * even if we only requested a 32-bit BAR.
+                */
+               add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;

Will send a separate fix with the above.


Kind regards,
Niklas