> -----Original Message-----
> From: Niklas Cassel <cassel@xxxxxxxxxx>
> Sent: 01 March 2024 00:08
> To: Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx>; Krzysztof Wilczyński
> <kw@xxxxxxxxx>; Manivannan Sadhasivam
> <manivannan.sadhasivam@xxxxxxxxxx>; Kishon Vijay Abraham I
> <kishon@xxxxxxxxxx>; Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Cc: Shradha Todi <shradha.t@xxxxxxxxxxx>; linux-pci@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/2] PCI: endpoint: Set prefetch when allocating memory
for
> 64-bit BARs
>
> 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@xxxxxxxxxx>
> > ---
> > 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
Hi Niklas,
A similar fix should go for pci_epf_test_set_bar() as well, since
pci_epc_set_bar()
may change the flags.
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c
b/drivers/pci/endpoint/functions/pci-epf-test.c
index cd4ffb39dcdc..3bf4340e9d7b 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -741,6 +741,8 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
if (bar == test_reg_bar)
return ret;
}
+
+ add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
}
return 0;
Do you want to add it as part of the same patch? Or should I post another patch
for this?
