Re: [PATCH v6 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4825 bytes --]

On Mon, Sep 13, 2021 at 06:38:58PM -0700, Nicolin Chen wrote:
> This patch dumps all active mapping entries from pagetable
> to a debugfs directory named "mappings".
> 
> Attaching an example:
> 
> SWGROUP: hc
> as->id: 0
> as->attr: R|W|N
> as->pd_dma: 0x0000000080c03000
> {
>         [index: 1023] 0xf0080c3e (count: 2)
>         {
>                 PTE RANGE      | ATTR | PHYS               | IOVA               | SIZE
>                 [#1022, #1023] | 0x5  | 0x000000010bbf1000 | 0x00000000ffffe000 | 0x2000
>         }
> }
> Total PDE count: 1
> Total PTE count: 2
> 
> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
>  drivers/iommu/tegra-smmu.c | 145 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 145 insertions(+)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 68c34a4a0ecc..aac977e181f6 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -46,6 +46,7 @@ struct tegra_smmu {
>  	struct list_head list;
>  
>  	struct dentry *debugfs;
> +	struct dentry *debugfs_mappings;
>  
>  	struct iommu_device iommu;	/* IOMMU Core code handle */
>  };
> @@ -153,6 +154,9 @@ static inline u32 smmu_readl(struct tegra_smmu *smmu, unsigned long offset)
>  
>  #define SMMU_PDE_ATTR		(SMMU_PDE_READABLE | SMMU_PDE_WRITABLE | \
>  				 SMMU_PDE_NONSECURE)
> +#define SMMU_PTE_ATTR		(SMMU_PTE_READABLE | SMMU_PTE_WRITABLE | \
> +				 SMMU_PTE_NONSECURE)
> +#define SMMU_PTE_ATTR_SHIFT	29
>  
>  static unsigned int iova_pd_index(unsigned long iova)
>  {
> @@ -164,6 +168,12 @@ static unsigned int iova_pt_index(unsigned long iova)
>  	return (iova >> SMMU_PTE_SHIFT) & (SMMU_NUM_PTE - 1);
>  }
>  
> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int pt_index)
> +{
> +	return ((dma_addr_t)pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
> +	       ((dma_addr_t)pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
> +}
> +
>  static bool smmu_dma_addr_valid(struct tegra_smmu *smmu, dma_addr_t addr)
>  {
>  	addr >>= 12;
> @@ -496,6 +506,8 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu,
>  	mutex_unlock(&smmu->lock);
>  }
>  
> +static const struct file_operations tegra_smmu_debugfs_mappings_fops;

Could the implementation be moved up here to avoid the forward
declaration?

> +
>  static void tegra_smmu_attach_as(struct tegra_smmu *smmu,
>  				 struct tegra_smmu_as *as,
>  				 unsigned int swgroup)
> @@ -517,6 +529,12 @@ static void tegra_smmu_attach_as(struct tegra_smmu *smmu,
>  			dev_warn(smmu->dev,
>  				 "overwriting group->as for swgroup: %s\n", swgrp->name);
>  		group->as = as;
> +
> +		if (smmu->debugfs_mappings)
> +			debugfs_create_file(group->swgrp->name, 0444,
> +					    smmu->debugfs_mappings, group,
> +					    &tegra_smmu_debugfs_mappings_fops);
> +
>  		break;
>  	}
>  
> @@ -541,6 +559,12 @@ static void tegra_smmu_detach_as(struct tegra_smmu *smmu,
>  		if (group->swgrp != swgrp)
>  			continue;
>  		group->as = NULL;
> +
> +		if (smmu->debugfs_mappings) {
> +			d = debugfs_lookup(group->swgrp->name, smmu->debugfs_mappings);
> +			debugfs_remove(d);
> +		}
> +
>  		break;
>  	}
>  
> @@ -1124,6 +1148,125 @@ static int tegra_smmu_clients_show(struct seq_file *s, void *data)
>  
>  DEFINE_SHOW_ATTRIBUTE(tegra_smmu_clients);
>  
> +static int tegra_smmu_debugfs_mappings_show(struct seq_file *s, void *data)
> +{
> +	struct tegra_smmu_group *group = s->private;
> +	const struct tegra_smmu_swgroup *swgrp;
> +	struct tegra_smmu_as *as;
> +	struct tegra_smmu *smmu;
> +	unsigned int pd_index;
> +	unsigned int pt_index;
> +	unsigned long flags;
> +	u64 pte_count = 0;
> +	u32 pde_count = 0;
> +	u32 *pd, val;
> +
> +	if (!group || !group->as || !group->swgrp)
> +		return 0;
> +
> +	swgrp = group->swgrp;
> +	smmu = group->smmu;
> +	as = group->as;
> +
> +	mutex_lock(&smmu->lock);
> +
> +	val = smmu_readl(smmu, swgrp->reg) & SMMU_ASID_ENABLE;
> +	if (!val)
> +		goto unlock;
> +
> +	pd = page_address(as->pd);
> +	if (!pd)
> +		goto unlock;
> +
> +	seq_printf(s, "\nSWGROUP: %s\n", swgrp->name);
> +	seq_printf(s, "as->id: %d\nas->attr: %c|%c|%s\nas->pd_dma: %pad\n", as->id,
> +		   as->attr & SMMU_PD_READABLE ? 'R' : '-',
> +		   as->attr & SMMU_PD_WRITABLE ? 'W' : '-',
> +		   as->attr & SMMU_PD_NONSECURE ? "NS" : "S",
> +		   &as->pd_dma);
> +	seq_puts(s, "{\n");

Maybe this can be more compact by putting the name, ID, attributes and
base address onto a single line? Maybe also use "'-' : 'S'" for the
non-secure attribute to keep in line with what you've done for readable
and writable attributes.

Then again, this is going to be very verbose output anyway, so maybe it
isn't worth it.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]