Message-ID: <o6swnuf4aplcyd5jpgbyhslxcxuhzt4j6a4oq773eujva6ynqj@wmkorp4mavul>
Date: Tue, 6 Jan 2026 12:09:24 +0900
From: Koichiro Den <den@...inux.co.jp>
To: Niklas Cassel <cassel@...nel.org>
Cc: jingoohan1@...il.com, mani@...nel.org, lpieralisi@...nel.org, 
	kwilczynski@...nel.org, robh@...nel.org, bhelgaas@...gle.com, Frank.Li@....com, 
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/2] PCI: endpoint: BAR subrange mapping support

On Tue, Jan 06, 2026 at 10:52:54AM +0900, Koichiro Den wrote:
> On Mon, Jan 05, 2026 at 05:55:30PM +0100, Niklas Cassel wrote:
> > Hello Koichiro,
> > 
> > On Mon, Jan 05, 2026 at 05:02:12PM +0900, Koichiro Den wrote:
> > > This series proposes support for mapping subranges within a PCIe endpoint
> > > BAR and enables controllers to program inbound address translation for
> > > those subranges.
> > > 
> > > The first patch introduces generic BAR subrange mapping support in the
> > > PCI endpoint core. The second patch adds an implementation for the
> > > DesignWare PCIe endpoint controller using Address Match Mode IB iATU.
> > > 
> > > This series is a spin-off from a larger RFC series posted earlier:
> > > https://lore.kernel.org/all/20251217151609.3162665-4-den@valinux.co.jp/
> > > 
> > > Base:
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git
> > >   branch: controller/dwc
> > >   commit: 68ac85fb42cf ("PCI: dwc: Use cfg0_base as iMSI-RX target address
> > >                          to support 32-bit MSI devices")
> > > 
> > > Thank you for reviewing,
> > > 
> > > Koichiro Den (2):
> > >   PCI: endpoint: Add BAR subrange mapping support
> > >   PCI: dwc: ep: Support BAR subrange inbound mapping via address-match
> > >     iATU
> > 
> > I have nothing against this feature, but the big question here is:
> > where is the user?
> > 
> > Usually, we don't add a new feature without having a single user of said
> > feature.
> > 
> 
> The first user will likely be Remote eDMA-backed NTB transport. An RFC
> series,
> https://lore.kernel.org/all/20251217151609.3162665-4-den@valinux.co.jp/
> referenced in the cover letter relies on Address Match Mode support.
> In that sense, this split series is prerequisite work, and if this gets
> acked, I will post another patch series that utilizes this in the NTB code.
> 
> At least for Renesas R-Car S4, where 64-bit BAR0/BAR2 and 32-bit BAR4 are
> available, exposing the eDMA regsister and LL regions to the host requires
> at least two mappings (one for register and another for a contiguous LL
> memory). Address Match Mode allows a flexible and extensible layout for the
> required regions.
> 
> > 
> > One thing that I would like to see though:
> > stricter verification of the pci_epf_bar_submap array.
> > 
> > For DWC, we know that the minimum size that an iATU can map is stored in:
> > (struct dw_pcie *pci) pci->region_align.
> > 
> > Thus, each element in the pci_epf_bar_submap array has to have a size that
> > is a multiple of pci->region_align.
> > 
> > I don't see that you ever verify this anywhere.
> 
> I missed it, will add the check.

My reply above was wrong, the region_align-related validation is already
handled in dw_pcie_prog_inbound_atu(). I don't think we need to duplicate
the same check at (A) (see below) in dw_pcie_ep_ib_atu_addr(), and would
prefer to keep the code simple as possible since this is not a fast path.

    (The below is the code with this series applied)
    /* Address match mode */
    static int dw_pcie_ep_ib_atu_addr(struct dw_pcie_ep *ep, u8 func_no, int type,
                                      struct pci_epf_bar *epf_bar)
    {
            [...]
            for (i = 0; i < epf_bar->num_submap; i++) {
                    off = submap[i].offset;
                    size = submap[i].size;
                    parent_bus_addr = submap[i].phys_addr;
                    if (!size)
                            continue;
                    # (A)
                    [...]
                    ret = dw_pcie_prog_inbound_atu(pci, free_win, type,
                                                   parent_bus_addr, pci_addr, size); # (B)
                    if (ret) {
                            spin_lock_irqsave(&ep->ib_map_lock, flags);
                            list_del(&new->list);
                            clear_bit(free_win, ep->ib_window_map);
                            spin_unlock_irqrestore(&ep->ib_map_lock, flags);
                            devm_kfree(dev, new);
                            return ret;


Kind regards,
Koichiro

> 
> > 
> > You should also verify that the sum of all the sizes in the pci_epf_bar_submap
> > array adds up to exactly pci_epf_bar->size.
> 
> I didn't think this was a requirement. I experimented with it just now, and
> seems to me that no harm is caused even if the sum of the submap sizes is
> less than the BAR size. Could you point me to any description of this
> requirement in the databook if available?
> 
> > 
> > Also, we currently have code in dw_pcie_prog_inbound_atu() that verifies
> > that the physical memory address is aligned to the size of the BAR, as that
> > is a requirement in BAR match mode, see:
> > 129f6af747b2 ("PCI: dwc: ep: Add 'address' alignment to 'size' check in dw_pcie_prog_ep_inbound_atu()")
> > 
> > This is not a requirement in address match mode, so you probably don't
> > want to do that check in address match mode.
> > (You will want to keep the check that the physical memory address is
> > aligned to pci->region_align though.)
> 
> With this series, the call graph would change as follows:
> 
>   -> dw_pcie_ep_set_bar()
>      # For BAR Match Mode:
>      -> dw_pcie_ep_ib_atu_bar()
>         -> dw_pcie_prog_ep_inbound_atu()
>      # For Address Match Mode:
>      -> dw_pcie_ep_ib_atu_addr()
>         -> dw_pcie_prog_inbound_atu()
> 
> and the size check that was introduced in the commit 129f6af747b2 should
> not run for Address Match Mode in any case.
> 
> Thank you for the review!
> Koichiro
> 
> > 
> > 
> > Kind regards,
> > Niklas

Powered by blists - more mailing lists