Hello Shradha, On Fri, Mar 01, 2024 at 04:07:21PM +0530, Shradha Todi wrote: > > > > -----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. 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
