| Message ID | 20241114110326.1891331-8-cassel@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI endpoint additional pci_epc_set_bar() checks | expand |
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 >
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 > >
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);
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(-)