[PATCH 0/2] PCI: endpoint: set prefetchable bit

[PATCH 0/2] PCI: endpoint: set prefetchable bit

[Niklas Cassel]

Shradha Todi wanted to add prefetchable BAR support by adding more things
to epc_features:
https://lore.kernel.org/linux-pci/20240228134448.56372-1-shradha.t@samsung.com/T/#t

This series sets the prefetchable bit for all backing memory that the EPF
core allocates for a 64-bit BAR.

So far, only compile tested. Please test.


Kind regards,
Niklas

Niklas Cassel (2):
  PCI: endpoint: Move .only_64bit check to core
  PCI: endpoint: Set prefetch when allocating memory for 64-bit BARs

 drivers/pci/endpoint/functions/pci-epf-test.c | 14 --------------
 drivers/pci/endpoint/pci-epf-core.c           |  8 +++++---
 2 files changed, 5 insertions(+), 17 deletions(-)

-- 
2.44.0

[PATCH 1/2] PCI: endpoint: Move .only_64bit check to core

[Niklas Cassel]

Move the .only_64bit check to pci-epf-core, such that this check does
not need to be duplicated in all endpoint function drivers.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 14 --------------
 drivers/pci/endpoint/pci-epf-core.c           |  7 ++++---
 2 files changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index cd4ffb39dcdc..a7f0b5ebc1e8 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -870,19 +870,6 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
 	return 0;
 }
 
-static void pci_epf_configure_bar(struct pci_epf *epf,
-				  const struct pci_epc_features *epc_features)
-{
-	struct pci_epf_bar *epf_bar;
-	int i;
-
-	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
-		epf_bar = &epf->bar[i];
-		if (epc_features->bar[i].only_64bit)
-			epf_bar->flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
-	}
-}
-
 static int pci_epf_test_bind(struct pci_epf *epf)
 {
 	int ret;
@@ -907,7 +894,6 @@ static int pci_epf_test_bind(struct pci_epf *epf)
 	test_reg_bar = pci_epc_get_first_free_bar(epc_features);
 	if (test_reg_bar < 0)
 		return -EINVAL;
-	pci_epf_configure_bar(epf, epc_features);
 
 	epf_test->test_reg_bar = test_reg_bar;
 	epf_test->epc_features = epc_features;
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 0a28a0b0911b..e7dbbeb1f0de 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -304,9 +304,10 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
 	epf_bar[bar].addr = space;
 	epf_bar[bar].size = size;
 	epf_bar[bar].barno = bar;
-	epf_bar[bar].flags |= upper_32_bits(size) ?
-				PCI_BASE_ADDRESS_MEM_TYPE_64 :
-				PCI_BASE_ADDRESS_MEM_TYPE_32;
+	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;
 
 	return space;
 }
-- 
2.44.0

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

[Niklas Cassel]

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;
 
-- 
2.44.0

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

[Niklas Cassel]

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

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

[Shradha Todi]

> -----Original Message-----
> From: Niklas Cassel <cassel@kernel.org>
> Sent: 01 March 2024 00:08
> To: Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof Wilczyński
> <kw@linux.com>; Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org>; Kishon Vijay Abraham I
> <kishon@kernel.org>; Bjorn Helgaas <bhelgaas@google.com>
> Cc: Shradha Todi <shradha.t@samsung.com>; linux-pci@vger.kernel.org
> 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@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

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?

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

[Niklas Cassel]

Hello Shradha,

On Fri, Mar 01, 2024 at 04:07:21PM +0530, Shradha Todi wrote:
> 
> 
> > -----Original Message-----
> > From: Niklas Cassel <cassel@kernel.org>
> > Sent: 01 March 2024 00:08
> > To: Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof Wilczyński
> > <kw@linux.com>; Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org>; Kishon Vijay Abraham I
> > <kishon@kernel.org>; Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Shradha Todi <shradha.t@samsung.com>; linux-pci@vger.kernel.org
> > 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@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
> 
> Hi Niklas,
> 
> A similar fix should go for pci_epf_test_set_bar() as well, since
> pci_epc_set_bar()
> may change the flags.

I do see that this text is here:
https://github.com/torvalds/linux/blob/v6.8-rc6/drivers/pci/endpoint/functions/pci-epf-test.c#L725-L729

Looking at the orginal author of that text:
https://github.com/torvalds/linux/commit/fca83058753456528bef62579ae2b50799d7a473
(turned out that it was me :p)

I see that the reason was that epc->ops->set_bar() for Cadence EP controller
could set PCI_BASE_ADDRESS_MEM_TYPE_64 even if only 32 bits BAR was requested.

Looking at the Cadence set_bar():
https://github.com/torvalds/linux/blob/master/drivers/pci/controller/cadence/pcie-cadence-ep.c#L107-L108
They do set a 64-bit even if a user only requested a 32-bit BAR,
if the requested BAR size was > 4G.

However, we have this check:
https://github.com/torvalds/linux/commit/f25b5fae29d4e5fe6ae17d3f898a959d72519b6a

So that would never happen. pci_epc_set_bar() would error out before calling
the lower level device driver with such a requested configuration.


Now when we have epc_features, if a BAR requires 64-bits, they should set that
as a hardware requirement in epc_features.

A epc->ops->set_bar() should never be allowed to override whas is being
requested IMO.
(It could give an error if it can't fulfill the requested configuration though.)

So what I think should be done:
1) Clean up the Cadence driver, since that code is dead code.
2) Remove the "pci_epc_set_bar() might have given us a 64-bit BAR",
   since it is not true.


Note that pci_epf_test_set_bar() still needs to do:
https://github.com/torvalds/linux/blob/master/drivers/pci/endpoint/functions/pci-epf-test.c#L730

But that is because pci_epf_alloc_space() could have set
PCI_BASE_ADDRESS_MEM_TYPE_64. But I don't think that we need a comment for this,
just like how the "add =" in pci_epf_test_alloc_space() does not have a comment:
https://github.com/torvalds/linux/blob/v6.8-rc6/drivers/pci/endpoint/functions/pci-epf-test.c#L860


Kind regards,
Niklas