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

[Niklas Cassel <cassel@kernel.org>]

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