* [PATCH 0/3] PCI endpoint additional pci_epc_set_bar() checks
@ 2024-11-14 11:03 Niklas Cassel
2024-11-14 11:03 ` [PATCH 1/3] PCI: artpec6: Implement dw_pcie_ep operation get_features Niklas Cassel
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Niklas Cassel @ 2024-11-14 11:03 UTC (permalink / raw)
To: Jesper Nilsson, Lorenzo Pieralisi, Krzysztof Wilczyński,
Manivannan Sadhasivam, Rob Herring, Bjorn Helgaas, Jingoo Han,
Kishon Vijay Abraham I
Cc: Damien Le Moal, Frank Li, Niklas Cassel, linux-arm-kernel,
linux-pci
Hello all,
This series adds some extra checks to ensure that can not program the iATU
with an address that we did not intend to use.
These checks would have given resulting in an error when testing some of
the earlier revisions of Frank's doorbell patches (which did not handle
fixed BARs properly).
With these checks in place, we will hopefully avoid unnecessary debugging
in the future.
Kind regards,
Niklas
Niklas Cassel (3):
PCI: artpec6: Implement dw_pcie_ep operation get_features
PCI: endpoint: Add size check for fixed size BARs in pci_epc_set_bar()
PCI: dwc: ep: Improve alignment check in dw_pcie_prog_ep_inbound_atu()
drivers/pci/controller/dwc/pcie-artpec6.c | 13 +++++++++++++
drivers/pci/controller/dwc/pcie-designware-ep.c | 8 +++++---
drivers/pci/controller/dwc/pcie-designware.c | 5 +++--
drivers/pci/controller/dwc/pcie-designware.h | 2 +-
drivers/pci/endpoint/pci-epc-core.c | 11 +++++++++--
5 files changed, 31 insertions(+), 8 deletions(-)
--
2.47.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] PCI: artpec6: Implement dw_pcie_ep operation get_features
2024-11-14 11:03 [PATCH 0/3] PCI endpoint additional pci_epc_set_bar() checks Niklas Cassel
@ 2024-11-14 11:03 ` Niklas Cassel
2024-11-14 17:04 ` Frank Li
2024-11-14 11:03 ` [PATCH 2/3] PCI: endpoint: Add size check for fixed size BARs in pci_epc_set_bar() Niklas Cassel
2024-11-14 11:03 ` [PATCH 3/3] PCI: dwc: ep: Improve alignment check in dw_pcie_prog_ep_inbound_atu() Niklas Cassel
2 siblings, 1 reply; 8+ messages in thread
From: Niklas Cassel @ 2024-11-14 11:03 UTC (permalink / raw)
To: Jesper Nilsson, Lorenzo Pieralisi, Krzysztof Wilczyński,
Manivannan Sadhasivam, Rob Herring, Bjorn Helgaas
Cc: Damien Le Moal, Frank Li, Niklas Cassel, linux-arm-kernel,
linux-pci
All non-DWC EPC drivers implement (struct pci_epc *)->ops->get_features().
All DWC EPC drivers implement (struct dw_pcie_ep *)->ops->get_features(),
except for pcie-artpec6.c.
epc_features has been required in pci-epf-test.c since commit 6613bc2301ba
("PCI: endpoint: Fix NULL pointer dereference for ->get_features()").
A follow-up commit will make further use of epc_features in EPC core code.
Implement epc_features in the only EPC driver where it is currently not
implemented.
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
drivers/pci/controller/dwc/pcie-artpec6.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-artpec6.c b/drivers/pci/controller/dwc/pcie-artpec6.c
index f8e7283dacd47..234c8cbcae3af 100644
--- a/drivers/pci/controller/dwc/pcie-artpec6.c
+++ b/drivers/pci/controller/dwc/pcie-artpec6.c
@@ -369,9 +369,22 @@ static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
return 0;
}
+static const struct pci_epc_features artpec6_pcie_epc_features = {
+ .linkup_notifier = false,
+ .msi_capable = true,
+ .msix_capable = false,
+};
+
+static const struct pci_epc_features *
+artpec6_pcie_get_features(struct dw_pcie_ep *ep)
+{
+ return &artpec6_pcie_epc_features;
+}
+
static const struct dw_pcie_ep_ops pcie_ep_ops = {
.init = artpec6_pcie_ep_init,
.raise_irq = artpec6_pcie_raise_irq,
+ .get_features = artpec6_pcie_get_features,
};
static int artpec6_pcie_probe(struct platform_device *pdev)
--
2.47.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] PCI: endpoint: Add size check for fixed size BARs in pci_epc_set_bar()
2024-11-14 11:03 [PATCH 0/3] PCI endpoint additional pci_epc_set_bar() checks Niklas Cassel
2024-11-14 11:03 ` [PATCH 1/3] PCI: artpec6: Implement dw_pcie_ep operation get_features Niklas Cassel
@ 2024-11-14 11:03 ` Niklas Cassel
2024-11-14 17:06 ` Frank Li
2024-11-14 11:03 ` [PATCH 3/3] PCI: dwc: ep: Improve alignment check in dw_pcie_prog_ep_inbound_atu() Niklas Cassel
2 siblings, 1 reply; 8+ messages in thread
From: Niklas Cassel @ 2024-11-14 11:03 UTC (permalink / raw)
To: Manivannan Sadhasivam, Krzysztof Wilczyński,
Kishon Vijay Abraham I, Bjorn Helgaas
Cc: Damien Le Moal, Frank Li, Jesper Nilsson, Niklas Cassel,
linux-pci
A BAR of type BAR_FIXED has a fixed BAR size (the size cannot be changed).
When using pci_epf_alloc_space() to allocate backing memory for a BAR,
pci_epf_alloc_space() will always set the size to the fixed BAR size if
the BAR type is BAR_FIXED (and will give an error if you the requested size
is larger than the fixed BAR size).
However, some drivers might not call pci_epf_alloc_space() before calling
pci_epc_set_bar(), so add a check in pci_epc_set_bar() to ensure that an
EPF driver cannot set a size different from the fixed BAR size, if the BAR
type is BAR_FIXED.
The pci_epc_function_is_valid() check is removed because this check is now
done by pci_epc_get_features().
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
drivers/pci/endpoint/pci-epc-core.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index bed7c7d1fe3c3..c69c133701c92 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -609,10 +609,17 @@ EXPORT_SYMBOL_GPL(pci_epc_clear_bar);
int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
struct pci_epf_bar *epf_bar)
{
- int ret;
+ const struct pci_epc_features *epc_features;
+ enum pci_barno bar = epf_bar->barno;
int flags = epf_bar->flags;
+ int ret;
- if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
+ epc_features = pci_epc_get_features(epc, func_no, vfunc_no);
+ if (!epc_features)
+ return -EINVAL;
+
+ if (epc_features->bar[bar].type == BAR_FIXED &&
+ (epc_features->bar[bar].fixed_size != epf_bar->size))
return -EINVAL;
if ((epf_bar->barno == BAR_5 && flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ||
--
2.47.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] PCI: dwc: ep: Improve alignment check in dw_pcie_prog_ep_inbound_atu()
2024-11-14 11:03 [PATCH 0/3] PCI endpoint additional pci_epc_set_bar() checks Niklas Cassel
2024-11-14 11:03 ` [PATCH 1/3] PCI: artpec6: Implement dw_pcie_ep operation get_features Niklas Cassel
2024-11-14 11:03 ` [PATCH 2/3] PCI: endpoint: Add size check for fixed size BARs in pci_epc_set_bar() Niklas Cassel
@ 2024-11-14 11:03 ` Niklas Cassel
2024-11-14 17:21 ` Frank Li
2 siblings, 1 reply; 8+ messages in thread
From: Niklas Cassel @ 2024-11-14 11:03 UTC (permalink / raw)
To: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas
Cc: Damien Le Moal, Frank Li, Jesper Nilsson, Niklas Cassel,
linux-pci
dw_pcie_prog_ep_inbound_atu() is used to program an inbound iATU in
"BAR Match Mode".
While a memory address returned by kmalloc() is guaranteed to be naturally
aligned (aligned to the size of the allocation), it is not guaranteed that
the address that is supplied to pci_epc_set_bar() (and thus the addres that
is supplied to dw_pcie_prog_ep_inbound_atu()) is naturally aligned.
See the register description for IATU_LWR_TARGET_ADDR_OFF_INBOUND_i,
specifically fields LWR_TARGET_RW and LWR_TARGET_HW in the DWC Databook.
"Field size depends on log2(BAR_MASK+1) in BAR match mode."
I.e. only the upper bits are writable, and the number of writable bits is
dependent on the configured BAR_MASK.
Add a check to ensure that the physical address programmed in the iATU is
aligned to the size of the BAR (BAR_MASK+1), as without this, we can get
hard to debug errors, as we could write to bits that are read-only (without
getting a write error), which could cause the iATU to end up redirecting to
a physical address that is different from the address that we intended.
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
drivers/pci/controller/dwc/pcie-designware-ep.c | 8 +++++---
drivers/pci/controller/dwc/pcie-designware.c | 5 +++--
drivers/pci/controller/dwc/pcie-designware.h | 2 +-
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 507e40bd18c8f..4ad6ebd2ea320 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -128,7 +128,8 @@ static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
}
static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
- dma_addr_t cpu_addr, enum pci_barno bar)
+ dma_addr_t cpu_addr, enum pci_barno bar,
+ size_t size)
{
int ret;
u32 free_win;
@@ -145,7 +146,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
}
ret = dw_pcie_prog_ep_inbound_atu(pci, func_no, free_win, type,
- cpu_addr, bar);
+ cpu_addr, bar, size);
if (ret < 0) {
dev_err(pci->dev, "Failed to program IB window\n");
return ret;
@@ -229,7 +230,8 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
else
type = PCIE_ATU_TYPE_IO;
- ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
+ ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar,
+ size);
if (ret)
return ret;
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 6d6cbc8b5b2c6..3c683b6119c39 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -597,11 +597,12 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
}
int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
- int type, u64 cpu_addr, u8 bar)
+ int type, u64 cpu_addr, u8 bar, size_t size)
{
u32 retries, val;
- if (!IS_ALIGNED(cpu_addr, pci->region_align))
+ if (!IS_ALIGNED(cpu_addr, pci->region_align) ||
+ !IS_ALIGNED(cpu_addr, size))
return -EINVAL;
dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LOWER_TARGET,
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 347ab74ac35aa..fc08727116725 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -491,7 +491,7 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
u64 cpu_addr, u64 pci_addr, u64 size);
int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
- int type, u64 cpu_addr, u8 bar);
+ int type, u64 cpu_addr, u8 bar, size_t size);
void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index);
void dw_pcie_setup(struct dw_pcie *pci);
void dw_pcie_iatu_detect(struct dw_pcie *pci);
--
2.47.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] PCI: artpec6: Implement dw_pcie_ep operation get_features
2024-11-14 11:03 ` [PATCH 1/3] PCI: artpec6: Implement dw_pcie_ep operation get_features Niklas Cassel
@ 2024-11-14 17:04 ` Frank Li
0 siblings, 0 replies; 8+ messages in thread
From: Frank Li @ 2024-11-14 17:04 UTC (permalink / raw)
To: Niklas Cassel
Cc: Jesper Nilsson, Lorenzo Pieralisi, Krzysztof Wilczyński,
Manivannan Sadhasivam, Rob Herring, Bjorn Helgaas, Damien Le Moal,
linux-arm-kernel, linux-pci
On Thu, Nov 14, 2024 at 12:03:27PM +0100, Niklas Cassel wrote:
> All non-DWC EPC drivers implement (struct pci_epc *)->ops->get_features().
> All DWC EPC drivers implement (struct dw_pcie_ep *)->ops->get_features(),
> except for pcie-artpec6.c.
>
> epc_features has been required in pci-epf-test.c since commit 6613bc2301ba
> ("PCI: endpoint: Fix NULL pointer dereference for ->get_features()").
>
> A follow-up commit will make further use of epc_features in EPC core code.
>
> Implement epc_features in the only EPC driver where it is currently not
> implemented.
>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
> drivers/pci/controller/dwc/pcie-artpec6.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-artpec6.c b/drivers/pci/controller/dwc/pcie-artpec6.c
> index f8e7283dacd47..234c8cbcae3af 100644
> --- a/drivers/pci/controller/dwc/pcie-artpec6.c
> +++ b/drivers/pci/controller/dwc/pcie-artpec6.c
> @@ -369,9 +369,22 @@ static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> return 0;
> }
>
> +static const struct pci_epc_features artpec6_pcie_epc_features = {
> + .linkup_notifier = false,
> + .msi_capable = true,
> + .msix_capable = false,
> +};
> +
> +static const struct pci_epc_features *
> +artpec6_pcie_get_features(struct dw_pcie_ep *ep)
> +{
> + return &artpec6_pcie_epc_features;
> +}
> +
> static const struct dw_pcie_ep_ops pcie_ep_ops = {
> .init = artpec6_pcie_ep_init,
> .raise_irq = artpec6_pcie_raise_irq,
> + .get_features = artpec6_pcie_get_features,
> };
>
> static int artpec6_pcie_probe(struct platform_device *pdev)
> --
> 2.47.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] PCI: endpoint: Add size check for fixed size BARs in pci_epc_set_bar()
2024-11-14 11:03 ` [PATCH 2/3] PCI: endpoint: Add size check for fixed size BARs in pci_epc_set_bar() Niklas Cassel
@ 2024-11-14 17:06 ` Frank Li
0 siblings, 0 replies; 8+ messages in thread
From: Frank Li @ 2024-11-14 17:06 UTC (permalink / raw)
To: Niklas Cassel
Cc: Manivannan Sadhasivam, Krzysztof Wilczyński,
Kishon Vijay Abraham I, Bjorn Helgaas, Damien Le Moal,
Jesper Nilsson, linux-pci
On Thu, Nov 14, 2024 at 12:03:28PM +0100, Niklas Cassel wrote:
> A BAR of type BAR_FIXED has a fixed BAR size (the size cannot be changed).
>
> When using pci_epf_alloc_space() to allocate backing memory for a BAR,
> pci_epf_alloc_space() will always set the size to the fixed BAR size if
> the BAR type is BAR_FIXED (and will give an error if you the requested size
> is larger than the fixed BAR size).
>
> However, some drivers might not call pci_epf_alloc_space() before calling
> pci_epc_set_bar(), so add a check in pci_epc_set_bar() to ensure that an
> EPF driver cannot set a size different from the fixed BAR size, if the BAR
> type is BAR_FIXED.
>
> The pci_epc_function_is_valid() check is removed because this check is now
> done by pci_epc_get_features().
>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
> drivers/pci/endpoint/pci-epc-core.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index bed7c7d1fe3c3..c69c133701c92 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -609,10 +609,17 @@ EXPORT_SYMBOL_GPL(pci_epc_clear_bar);
> int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> struct pci_epf_bar *epf_bar)
> {
> - int ret;
> + const struct pci_epc_features *epc_features;
> + enum pci_barno bar = epf_bar->barno;
> int flags = epf_bar->flags;
> + int ret;
>
> - if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
> + epc_features = pci_epc_get_features(epc, func_no, vfunc_no);
> + if (!epc_features)
> + return -EINVAL;
> +
> + if (epc_features->bar[bar].type == BAR_FIXED &&
> + (epc_features->bar[bar].fixed_size != epf_bar->size))
> return -EINVAL;
>
> if ((epf_bar->barno == BAR_5 && flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ||
> --
> 2.47.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] PCI: dwc: ep: Improve alignment check in dw_pcie_prog_ep_inbound_atu()
2024-11-14 11:03 ` [PATCH 3/3] PCI: dwc: ep: Improve alignment check in dw_pcie_prog_ep_inbound_atu() Niklas Cassel
@ 2024-11-14 17:21 ` Frank Li
2024-11-15 8:15 ` Niklas Cassel
0 siblings, 1 reply; 8+ messages in thread
From: Frank Li @ 2024-11-14 17:21 UTC (permalink / raw)
To: Niklas Cassel
Cc: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Damien Le Moal, Jesper Nilsson, linux-pci
On Thu, Nov 14, 2024 at 12:03:29PM +0100, Niklas Cassel wrote:
according to patch, subject look like
"Add 'address' alignment to 'size' check in dw_pcie_prog_ep_inbound_atu()."
> dw_pcie_prog_ep_inbound_atu() is used to program an inbound iATU in
> "BAR Match Mode".
>
> While a memory address returned by kmalloc() is guaranteed to be naturally
> aligned (aligned to the size of the allocation), it is not guaranteed that
> the address that is supplied to pci_epc_set_bar() (and thus the addres that
> is supplied to dw_pcie_prog_ep_inbound_atu()) is naturally aligned.
short sentence may be better
The memory address returned by kmalloc() is guaranteed to be naturally
aligned (aligned to the size of the allocation), but it may not align when
pass to pci_epc_set_bar().
>
> See the register description for IATU_LWR_TARGET_ADDR_OFF_INBOUND_i,
> specifically fields LWR_TARGET_RW and LWR_TARGET_HW in the DWC Databook.
>
> "Field size depends on log2(BAR_MASK+1) in BAR match mode."
>
> I.e. only the upper bits are writable, and the number of writable bits is
> dependent on the configured BAR_MASK.
>
> Add a check to ensure that the physical address programmed in the iATU is
> aligned to the size of the BAR (BAR_MASK+1), as without this, we can get
> hard to debug errors, as we could write to bits that are read-only (without
> getting a write error), which could cause the iATU to end up redirecting to
> a physical address that is different from the address that we intended.
>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
> drivers/pci/controller/dwc/pcie-designware-ep.c | 8 +++++---
> drivers/pci/controller/dwc/pcie-designware.c | 5 +++--
> drivers/pci/controller/dwc/pcie-designware.h | 2 +-
> 3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 507e40bd18c8f..4ad6ebd2ea320 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -128,7 +128,8 @@ static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> }
>
> static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> - dma_addr_t cpu_addr, enum pci_barno bar)
> + dma_addr_t cpu_addr, enum pci_barno bar,
> + size_t size)
> {
> int ret;
> u32 free_win;
> @@ -145,7 +146,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> }
>
> ret = dw_pcie_prog_ep_inbound_atu(pci, func_no, free_win, type,
> - cpu_addr, bar);
> + cpu_addr, bar, size);
> if (ret < 0) {
> dev_err(pci->dev, "Failed to program IB window\n");
> return ret;
> @@ -229,7 +230,8 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> else
> type = PCIE_ATU_TYPE_IO;
>
> - ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
> + ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar,
> + size);
> if (ret)
> return ret;
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 6d6cbc8b5b2c6..3c683b6119c39 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -597,11 +597,12 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> }
>
> int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> - int type, u64 cpu_addr, u8 bar)
> + int type, u64 cpu_addr, u8 bar, size_t size)
> {
> u32 retries, val;
>
> - if (!IS_ALIGNED(cpu_addr, pci->region_align))
> + if (!IS_ALIGNED(cpu_addr, pci->region_align) ||
> + !IS_ALIGNED(cpu_addr, size))
> return -EINVAL;
>
> dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LOWER_TARGET,
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 347ab74ac35aa..fc08727116725 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -491,7 +491,7 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> u64 cpu_addr, u64 pci_addr, u64 size);
> int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> - int type, u64 cpu_addr, u8 bar);
> + int type, u64 cpu_addr, u8 bar, size_t size);
> void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index);
> void dw_pcie_setup(struct dw_pcie *pci);
> void dw_pcie_iatu_detect(struct dw_pcie *pci);
> --
> 2.47.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] PCI: dwc: ep: Improve alignment check in dw_pcie_prog_ep_inbound_atu()
2024-11-14 17:21 ` Frank Li
@ 2024-11-15 8:15 ` Niklas Cassel
0 siblings, 0 replies; 8+ messages in thread
From: Niklas Cassel @ 2024-11-15 8:15 UTC (permalink / raw)
To: Frank Li
Cc: Jingoo Han, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Damien Le Moal, Jesper Nilsson, linux-pci
On Thu, Nov 14, 2024 at 12:21:54PM -0500, Frank Li wrote:
> On Thu, Nov 14, 2024 at 12:03:29PM +0100, Niklas Cassel wrote:
>
> according to patch, subject look like
>
> "Add 'address' alignment to 'size' check in dw_pcie_prog_ep_inbound_atu()."
Sure.
>
> > dw_pcie_prog_ep_inbound_atu() is used to program an inbound iATU in
> > "BAR Match Mode".
> >
> > While a memory address returned by kmalloc() is guaranteed to be naturally
> > aligned (aligned to the size of the allocation), it is not guaranteed that
> > the address that is supplied to pci_epc_set_bar() (and thus the addres that
> > is supplied to dw_pcie_prog_ep_inbound_atu()) is naturally aligned.
>
> short sentence may be better
>
> The memory address returned by kmalloc() is guaranteed to be naturally
> aligned (aligned to the size of the allocation), but it may not align when
> pass to pci_epc_set_bar().
I do not think that this is better.
The memory address returned by kmalloc() is naturally aligned, and will still
be naturally aligned when passed to pci_epc_set_bar().
The problem is if the address supplied to pci_epc_set_bar() did not come from
something that was kmalloc():ed. E.g. the ITS address.
I can rephrase this sentence to make that clearer in v2.
Kind regards,
Niklas
>
> >
> > See the register description for IATU_LWR_TARGET_ADDR_OFF_INBOUND_i,
> > specifically fields LWR_TARGET_RW and LWR_TARGET_HW in the DWC Databook.
> >
> > "Field size depends on log2(BAR_MASK+1) in BAR match mode."
> >
> > I.e. only the upper bits are writable, and the number of writable bits is
> > dependent on the configured BAR_MASK.
> >
> > Add a check to ensure that the physical address programmed in the iATU is
> > aligned to the size of the BAR (BAR_MASK+1), as without this, we can get
> > hard to debug errors, as we could write to bits that are read-only (without
> > getting a write error), which could cause the iATU to end up redirecting to
> > a physical address that is different from the address that we intended.
> >
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > ---
> > drivers/pci/controller/dwc/pcie-designware-ep.c | 8 +++++---
> > drivers/pci/controller/dwc/pcie-designware.c | 5 +++--
> > drivers/pci/controller/dwc/pcie-designware.h | 2 +-
> > 3 files changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 507e40bd18c8f..4ad6ebd2ea320 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -128,7 +128,8 @@ static int dw_pcie_ep_write_header(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> > }
> >
> > static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> > - dma_addr_t cpu_addr, enum pci_barno bar)
> > + dma_addr_t cpu_addr, enum pci_barno bar,
> > + size_t size)
> > {
> > int ret;
> > u32 free_win;
> > @@ -145,7 +146,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> > }
> >
> > ret = dw_pcie_prog_ep_inbound_atu(pci, func_no, free_win, type,
> > - cpu_addr, bar);
> > + cpu_addr, bar, size);
> > if (ret < 0) {
> > dev_err(pci->dev, "Failed to program IB window\n");
> > return ret;
> > @@ -229,7 +230,8 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> > else
> > type = PCIE_ATU_TYPE_IO;
> >
> > - ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar);
> > + ret = dw_pcie_ep_inbound_atu(ep, func_no, type, epf_bar->phys_addr, bar,
> > + size);
> > if (ret)
> > return ret;
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 6d6cbc8b5b2c6..3c683b6119c39 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -597,11 +597,12 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> > }
> >
> > int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > - int type, u64 cpu_addr, u8 bar)
> > + int type, u64 cpu_addr, u8 bar, size_t size)
> > {
> > u32 retries, val;
> >
> > - if (!IS_ALIGNED(cpu_addr, pci->region_align))
> > + if (!IS_ALIGNED(cpu_addr, pci->region_align) ||
> > + !IS_ALIGNED(cpu_addr, size))
> > return -EINVAL;
> >
> > dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LOWER_TARGET,
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 347ab74ac35aa..fc08727116725 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -491,7 +491,7 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> > u64 cpu_addr, u64 pci_addr, u64 size);
> > int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > - int type, u64 cpu_addr, u8 bar);
> > + int type, u64 cpu_addr, u8 bar, size_t size);
> > void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index);
> > void dw_pcie_setup(struct dw_pcie *pci);
> > void dw_pcie_iatu_detect(struct dw_pcie *pci);
> > --
> > 2.47.0
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-11-15 8:15 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-14 11:03 [PATCH 0/3] PCI endpoint additional pci_epc_set_bar() checks Niklas Cassel
2024-11-14 11:03 ` [PATCH 1/3] PCI: artpec6: Implement dw_pcie_ep operation get_features Niklas Cassel
2024-11-14 17:04 ` Frank Li
2024-11-14 11:03 ` [PATCH 2/3] PCI: endpoint: Add size check for fixed size BARs in pci_epc_set_bar() Niklas Cassel
2024-11-14 17:06 ` Frank Li
2024-11-14 11:03 ` [PATCH 3/3] PCI: dwc: ep: Improve alignment check in dw_pcie_prog_ep_inbound_atu() Niklas Cassel
2024-11-14 17:21 ` Frank Li
2024-11-15 8:15 ` Niklas Cassel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox