| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZnLhn3DvnEWaa6I+@daire-X570>
Date: Wed, 19 Jun 2024 14:48:15 +0100
From: Daire McNamara <daire.mcnamara@...rochip.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC: <linux-pci@...r.kernel.org>, <devicetree@...r.kernel.org>,
<conor.dooley@...rochip.com>, <lpieralisi@...nel.org>, <kw@...ux.com>,
<robh@...nel.org>, <bhelgaas@...gle.com>, <linux-kernel@...r.kernel.org>,
<linux-riscv@...ts.infradead.org>, <krzk+dt@...nel.org>,
<conor+dt@...nel.org>
Subject: Re: [PATCH v3 2/3] PCI: microchip: Fix inbound address translation
tables
On Fri, Jun 14, 2024 at 04:29:47PM +0300, Ilpo Järvinen wrote:
> On Wed, 12 Jun 2024, daire.mcnamara@...rochip.com wrote:
>
> > From: Daire McNamara <daire.mcnamara@...rochip.com>
> >
> > On Microchip PolarFire SoC the PCIe Root Port can be behind one of three
> > general purpose Fabric Interface Controller (FIC) buses that encapsulates
> > an AXI-S bus. Depending on which FIC(s) the Root Port is connected
> > through to CPU space, and what address translation is done by that FIC,
> > the Root Port driver's inbound address translation may vary.
> >
> > For all current supported designs and all future expected designs,
> > inbound address translation done by a FIC on PolarFire SoC varies
> > depending on whether PolarFire SoC in operating in dma-coherent mode or
> > dma-noncoherent mode.
> >
> > The setup of the outbound address translation tables in the root port
> > driver only needs to handle these two cases.
> >
> > Setup the inbound address translation tables to one of two address
> > translations, depending on whether the rootport is marked as dma-coherent or
> > dma-noncoherent.
> >
> > Fixes: 6f15a9c9f941 ("PCI: microchip: Add Microchip Polarfire PCIe controller driver")
> >
> > Signed-off-by: Daire McNamara <daire.mcnamara@...rochip.com>
> > ---
> > drivers/pci/controller/pcie-microchip-host.c | 97 +++++++++++++++++++-
> > 1 file changed, 92 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> > index 853adce24492..d5021333e2aa 100644
> > --- a/drivers/pci/controller/pcie-microchip-host.c
> > +++ b/drivers/pci/controller/pcie-microchip-host.c
> > @@ -30,6 +30,9 @@
> > #define MC_PCIE_BRIDGE_ADDR (MC_PCIE1_BRIDGE_ADDR)
> > #define MC_PCIE_CTRL_ADDR (MC_PCIE1_CTRL_ADDR)
> >
> > +#define MC_MAX_NUM_INBOUND_WINDOWS 8
> > +#define MPFS_NC_BOUNCE_ADDR 0x80000000
> > +
> > /* PCIe Bridge Phy Regs */
> > #define PCIE_PCI_IRQ_DW0 0xa8
> > #define MSIX_CAP_MASK BIT(31)
> > @@ -105,6 +108,7 @@
> > #define ATR0_AXI4_SLV0_TRSL_PARAM 0x810u
> > #define PCIE_TX_RX_INTERFACE 0x00000000u
> > #define PCIE_CONFIG_INTERFACE 0x00000001u
> > +#define TRSL_ID_AXI4_MASTER_0 0x00000004u
> >
> > #define ATR_ENTRY_SIZE 32
> >
> > @@ -931,6 +935,89 @@ static int mc_pcie_init_irq_domains(struct mc_pcie *port)
> > return mc_allocate_msi_domains(port);
> > }
> >
> > +static void mc_pcie_setup_inbound_atr(int window_index, u64 axi_addr, u64 pcie_addr, size_t size)
> > +{
> > + void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> > + u32 table_offset = window_index * ATR_ENTRY_SIZE;
> > + u32 atr_sz;
> > + u32 val;
> > +
> > + atr_sz = ilog2(size) - 1;
> > + atr_sz &= GENMASK(5, 0);
> > + val = lower_32_bits(pcie_addr) & GENMASK(31, 12);
>
> ALIGN_DOWN(, SZ_xx) ?
Nice, I'll pop that into v4
>
> > + val |= (atr_sz << ATR_SIZE_SHIFT);
>
> This looks like a named define + FIELD_PREP() would be more appropriate
> here.
And, also nice. Again, I'll pop that into v4.
>
> > + val |= ATR_IMPL_ENABLE;
> > + writel(val, bridge_base_addr + table_offset + ATR0_PCIE_WIN0_SRCADDR_PARAM);
> > +
> > + writel(upper_32_bits(pcie_addr), bridge_base_addr + table_offset +
> > + ATR0_PCIE_WIN0_SRC_ADDR);
> > +
> > + writel(lower_32_bits(axi_addr), bridge_base_addr + table_offset +
> > + ATR0_PCIE_WIN0_TRSL_ADDR_LSB);
> > + writel(upper_32_bits(axi_addr), bridge_base_addr + table_offset +
> > + ATR0_PCIE_WIN0_TRSL_ADDR_UDW);
> > +
> > + writel(TRSL_ID_AXI4_MASTER_0, bridge_base_addr + table_offset +
> > + ATR0_PCIE_WIN0_TRSL_PARAM);
>
> Having a table_addr local variable instead would make these much less
> repetitive and shorter.
Agreed. v4
>
> > +}
> > +
> > +static int mc_pcie_setup_inbound_ranges(struct platform_device *pdev, struct mc_pcie *port)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *dn = dev->of_node;
> > + struct of_range_parser parser;
> > + struct of_range range;
> > + int atr_index = 0;
> > +
> > + /*
> > + * MPFS PCIe root port is 32-bit only, behind a Fabric Interface
> > + * Controller FPGA logic block which contains the AXI-S interface.
> > + *
> > + * From the point of view of the PCIe root port, There are only
> > + * two supported Root Port configurations
> > + *
> > + * Configuration 1: for use with fully coherent designs; supports a
> > + * window from 0x0 (CPU space) to specified PCIe space.
> > + *
> > + * Configuration 2: for use with non-coherent designs; supports two
> > + * 1 Gb wide windows to CPU space; one mapping cpu space 0 to pcie
> > + * space 0x80000000 and mapping cpu space 0x40000000 to pcie
> > + * space 0xc0000000. This cfg needs two windows because of how
> > + * the MSI space is allocated in the AXI-S range on MPFS.
> > + *
> > + * The FIC interface outside the PCIe block *must* complete the inbound
> > + * address translation as per MCHP MPFS FPGA design guidelines.
> > + */
> > + if (device_property_read_bool(dev, "dma-noncoherent")) {
> > + /*
> > + * Always need same two tables in this case. Need two tables
> > + * due to hardware interactions between address and size.
> > + */
> > + mc_pcie_setup_inbound_atr(0, 0, MPFS_NC_BOUNCE_ADDR, SZ_1G);
> > + mc_pcie_setup_inbound_atr(1, SZ_1G, MPFS_NC_BOUNCE_ADDR + SZ_1G, SZ_1G);
> > + } else {
> > + /* Find any dma-ranges */
> > + if (of_pci_dma_range_parser_init(&parser, dn)) {
> > + /* No dma-range property - setup default */
> > + mc_pcie_setup_inbound_atr(0, 0, 0, SZ_4G);
> > + return 0;
> > + }
> > +
> > + for_each_of_range(&parser, &range) {
> > + if (atr_index >= MC_MAX_NUM_INBOUND_WINDOWS) {
> > + dev_err(dev, "too many inbound ranges; %d available tables\n",
> > + MC_MAX_NUM_INBOUND_WINDOWS);
> > + return -EINVAL;
>
> You don't need to rollback anything when this error is encountered?
This one, I can't see how the driver can do anything more useful than just give up - the
address translation tables are now no better nor no worse than they were before; i.e. the
root port won't work properly.
I can't see any deploment-time recovery of the root port from this.
For general end-users of PolarFire systems, I'd expect they're going to see a working system
that has been brought up/debugged by someone doing a bsp.
So, in my mind, this is a case only a developer bringing up a new bsp on
PolarFire SoC needs to know about and all the driver can do is a notify that their proposed
address translations won't work as they imagine (as they've run out of tables).
>
> > + }
> > + mc_pcie_setup_inbound_atr(atr_index, 0, range.pci_addr, range.size);
> > + atr_index++;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static void mc_pcie_setup_window(void __iomem *bridge_base_addr, u32 index,
> > phys_addr_t axi_addr, phys_addr_t pci_addr,
> > u64 size)
> > @@ -962,11 +1049,6 @@ static void mc_pcie_setup_window(void __iomem *bridge_base_addr, u32 index,
> > val = upper_32_bits(pci_addr);
> > writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) +
> > ATR0_AXI4_SLV0_TRSL_ADDR_UDW);
> > -
> > - val = readl(bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM);
> > - val |= (ATR0_PCIE_ATR_SIZE << ATR0_PCIE_ATR_SIZE_SHIFT);
> > - writel(val, bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM);
> > - writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR);
> > }
> >
> > static int mc_pcie_setup_windows(struct platform_device *pdev,
> > @@ -1129,6 +1211,11 @@ static int mc_platform_init(struct pci_config_window *cfg)
> > if (ret)
> > return ret;
> >
> > + /* Configure inbound translation tables */
>
> IMO, this comment adds 0 value over what the code tells all by itself so
> it would be best to drop it.
Dropped
>
> > + ret = mc_pcie_setup_inbound_ranges(pdev, port);
> > + if (ret)
> > + return ret;
> > +
> > /* Address translation is up; safe to enable interrupts */
> > ret = mc_init_interrupts(pdev, port);
> > if (ret)
> >
>
> --
> i.
>
>
Powered by blists - more mailing lists