| Message ID | 20240229104900.894695-3-cassel@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: endpoint: set prefetchable bit | expand |
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
> -----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?
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
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;
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(-)