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@xxxxxxxxxx>
> > ---
> > 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
> >
