Re: [PATCH v8 4/7] PCI: keystone: Add support for PVU-based DMA isolation on AM654

[Jan Kiszka <jan.kiszka@siemens.com>]

On 25.04.25 18:48, Siddharth Vadapalli wrote:
> On Tue, Apr 22, 2025 at 02:14:03PM +0800, huaqian.li@siemens.com wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> The AM654 lacks an IOMMU, thus does not support isolating DMA requests
>> from untrusted PCI devices to selected memory regions this way. Use
>> static PVU-based protection instead. The PVU, when enabled, will only
>> accept DMA requests that address previously configured regions.
>>
>> Use the availability of a restricted-dma-pool memory region as trigger
>> and register it as valid DMA target with the PVU. In addition, enable
>> the mapping of requester IDs to VirtIDs in the PCI RC. Use only a single
>> VirtID so far, catching all devices. This may be extended later on.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>> Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
>> ---
>>  drivers/pci/controller/dwc/pci-keystone.c | 106 ++++++++++++++++++++++
>>  1 file changed, 106 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>> index 76a37368ae4f..ea2d8768e333 100644
>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/mfd/syscon.h>
>>  #include <linux/msi.h>
>>  #include <linux/of.h>
>> +#include <linux/of_address.h>
>>  #include <linux/of_irq.h>
>>  #include <linux/of_pci.h>
>>  #include <linux/phy/phy.h>
>> @@ -26,6 +27,7 @@
>>  #include <linux/regmap.h>
>>  #include <linux/resource.h>
>>  #include <linux/signal.h>
>> +#include <linux/ti-pvu.h>
>>  
>>  #include "../../pci.h"
>>  #include "pcie-designware.h"
>> @@ -111,6 +113,16 @@
>>  
>>  #define PCI_DEVICE_ID_TI_AM654X		0xb00c
>>  
>> +#define KS_PCI_VIRTID			0
>> +
>> +#define PCIE_VMAP_xP_CTRL		0x0
>> +#define PCIE_VMAP_xP_REQID		0x4
>> +#define PCIE_VMAP_xP_VIRTID		0x8
>> +
>> +#define PCIE_VMAP_xP_CTRL_EN		BIT(0)
>> +
>> +#define PCIE_VMAP_xP_VIRTID_VID_MASK	0xfff
>> +
>>  struct ks_pcie_of_data {
>>  	enum dw_pcie_device_mode mode;
>>  	const struct dw_pcie_host_ops *host_ops;
>> @@ -1137,6 +1149,94 @@ static const struct of_device_id ks_pcie_of_match[] = {
>>  	{ },
>>  };
>>  
>> +static int ks_init_vmap(struct platform_device *pdev, const char *vmap_name)
>> +{
>> +	struct resource *res;
>> +	void __iomem *base;
>> +	u32 val;
>> +
>> +	if (!IS_ENABLED(CONFIG_TI_PVU))
>> +		return 0;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, vmap_name);
>> +	base = devm_pci_remap_cfg_resource(&pdev->dev, res);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	writel(0, base + PCIE_VMAP_xP_REQID);
>> +
>> +	val = readl(base + PCIE_VMAP_xP_VIRTID);
>> +	val &= ~PCIE_VMAP_xP_VIRTID_VID_MASK;
>> +	val |= KS_PCI_VIRTID;
> 
> While it has been stated that we are going to start off with a single
> VirtID for now and extend it later on, is there an example for how it may
> be extended? The only option I see is that of associating one VirtID for
> Low-Priority (LP) traffic and another for High-Priority (HP) traffic, in
> which case, it might be better to define them upfront and use them like:
> #define KS_PCI_LP_VIRTID	0
> #define KS_PCI_HP_VIRTID	1

Sorry for the late reply, was just reminded of this question:

When trying to design anything beyond the current use case, I would be
struggling right now with the how, simply because we would have no user
of extended APIs around to make sure that the result would be useful.
Can you envision such use cases? If not, I would rather suggest to
postpone any attempts to broaden the API until we have such users.

> 
>> +	writel(val, base + PCIE_VMAP_xP_VIRTID);
>> +
>> +	val = readl(base + PCIE_VMAP_xP_CTRL);
>> +	val |= PCIE_VMAP_xP_CTRL_EN;
>> +	writel(val, base + PCIE_VMAP_xP_CTRL);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ks_init_restricted_dma(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct of_phandle_iterator it;
>> +	struct resource phys;
>> +	int err;
>> +
>> +	if (!IS_ENABLED(CONFIG_TI_PVU))
>> +		return 0;
>> +
>> +	/* Only process the first restricted DMA pool, more are not allowed */
>> +	of_for_each_phandle(&it, err, dev->of_node, "memory-region",
>> +			    NULL, 0) {
>> +		if (of_device_is_compatible(it.node, "restricted-dma-pool"))
>> +			break;
>> +	}
>> +	if (err)
>> +		return err == -ENOENT ? 0 : err;
>> +
>> +	err = of_address_to_resource(it.node, 0, &phys);
>> +	if (err < 0) {
>> +		dev_err(dev, "failed to parse memory region %pOF: %d\n",
>> +			it.node, err);
>> +		return 0;
>> +	}
>> +
>> +	/* Map all incoming requests on low and high prio port to virtID 0 */
> 
> The question I asked above applies here too. How is it planned to extend
> support for multiple VirtIDs, if not on the basis of assigining one
> VirtID to LP traffic and another to HP traffic? Since we have an option
> of using different VirtIDs for LP and HP traffic, why not use it? Is
> there going to be an issue with using VirtID 0 for LP traffic and VirtID 1
> for HP traffic?
> 
>> +	err = ks_init_vmap(pdev, "vmap_lp");
>> +	if (err)
>> +		return err;
>> +	err = ks_init_vmap(pdev, "vmap_hp");
>> +	if (err)
>> +		return err;
>> +
>> +	/*
>> +	 * Enforce DMA pool usage with the help of the PVU.
>> +	 * Any request outside will be dropped and raise an error at the PVU.
>> +	 */
>> +	return ti_pvu_create_region(KS_PCI_VIRTID, &phys);
> 
> Same as above, we are passing a single VIRTID and not an array, so it
> isn't clear to me as to how it will be extended in the future.
> 
>> +}
>> +
>> +static void ks_release_restricted_dma(struct platform_device *pdev)
>> +{
>> +	struct of_phandle_iterator it;
>> +	struct resource phys;
>> +	int err;
>> +
>> +	if (!IS_ENABLED(CONFIG_TI_PVU))
>> +		return;
>> +
>> +	of_for_each_phandle(&it, err, pdev->dev.of_node, "memory-region",
>> +			    NULL, 0) {
>> +		if (of_device_is_compatible(it.node, "restricted-dma-pool") &&
>> +		    of_address_to_resource(it.node, 0, &phys) == 0) {
>> +			ti_pvu_remove_region(KS_PCI_VIRTID, &phys);
>> +			break;
>> +		}
>> +	}
>> +}
>> +
>>  static int ks_pcie_probe(struct platform_device *pdev)
>>  {
>>  	const struct dw_pcie_host_ops *host_ops;
>> @@ -1285,6 +1385,10 @@ static int ks_pcie_probe(struct platform_device *pdev)
>>  	if (ret < 0)
>>  		goto err_get_sync;
>>  
>> +	ret = ks_init_restricted_dma(pdev);
>> +	if (ret < 0)
>> +		goto err_get_sync;
>> +
> 
> Shouldn't the above be moved into the case for "DW_PCIE_RC_TYPE" below? Or
> is this valid even when the SoC is configured to act as an Endpoint?

Fair remark. We currently have no setup for EP usage thus cannot even
say if it would make sense / would work as intended in that case as
well. Probably better to move then.

Jan

> 
>>  	switch (mode) {
>>  	case DW_PCIE_RC_TYPE:
>>  		if (!IS_ENABLED(CONFIG_PCI_KEYSTONE_HOST)) {
>> @@ -1366,6 +1470,8 @@ static void ks_pcie_remove(struct platform_device *pdev)
>>  	int num_lanes = ks_pcie->num_lanes;
>>  	struct device *dev = &pdev->dev;
>>  
>> +	ks_release_restricted_dma(pdev);
>> +
>>  	pm_runtime_put(dev);
>>  	pm_runtime_disable(dev);
>>  	ks_pcie_disable_phy(ks_pcie);
> 
> Regards,
> Siddharth.


-- 
Siemens AG, Foundational Technologies
Linux Expert Center