Re: [PATCH v5] iommu/tegra-smmu: Add pagetable mappings to debugfs
On Tue, Mar 16, 2021 at 12:16:43PM +0100, Thierry Reding wrote:
> > +struct tegra_smmu_group_debug {
> > + const struct tegra_smmu_swgroup *group;
> > + void *priv;
>
> This always stores the address space, so why not make this:
>
> struct tegra_smmu_as *as;
>
> ? While at it, perhaps throw in a const to make sure we don't modify
> this structure in the debugfs code.
OK. I will try to change that.
> > @@ -334,7 +350,7 @@ static void tegra_smmu_domain_free(struct iommu_domain
> > *domain)
> > }
> >
> > static const struct tegra_smmu_swgroup *
> > -tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup)
> > +tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup, int
> > *index)
> > {
> > const struct tegra_smmu_swgroup *group = NULL;
> > unsigned int i;
> > @@ -342,6 +358,8 @@ tegra_smmu_find_swgroup(struct tegra_smmu *smmu,
> > unsigned int swgroup)
> > for (i = 0; i < smmu->soc->num_swgroups; i++) {
> > if (smmu->soc->swgroups[i].swgroup == swgroup) {
> > group = >soc->swgroups[i];
> > + if (index)
> > + *index = i;
>
> This doesn't look like the right place for this. And this also makes
> things hard to follow because it passes out-of-band data in the index
> parameter.
>
> I'm thinking that this could benefit from a bit of refactoring where
> we could for example embed struct tegra_smmu_group_debug into struct
> tegra_smmu_group and then reference that when necessary instead of
> carrying all that data in an orthogonal array. That should also make
> it easier to track this.
>
> Come to think of it, everything that's currently in your new struct
> tegra_smmu_group_debug would be useful in struct tegra_smmu_group,
> irrespective of debugfs support.
Will try to embed it or see what I can do following the suggestion.
> > +static int tegra_smmu_mappings_show(struct seq_file *s, void *data)
> > +{
> > + seq_printf(s, "\nSWGROUP: %s\nASID: %d\nreg: 0x%x\n",
> > + group->name, as->id, group->reg);
>
> Is group->reg really that useful here?
Can drop it.
> > +
> > + smmu_writel(smmu, as->id & 0x7f, SMMU_PTB_ASID);
> > + ptb_reg = smmu_readl(smmu, SMMU_PTB_DATA);
> > +
> > + seq_printf(s, "PTB_ASID: 0x%x\nas->pd_dma: %pad\n",
> > + ptb_reg, >pd_dma);
>
> This looks somewhat redundant because as->pd_dma is already part of the
> PTB_ASID register value. Instead, perhaps decode the upper bits of that
> register and simply output as->pdma so that we don't duplicate the base
> address of the page table?
That's a good idea. Will change that too.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5] iommu/tegra-smmu: Add pagetable mappings to debugfs
16.03.2021 14:16, Thierry Reding пишет: >> +seq_puts(s, "}\n"); >> +seq_printf(s, "Total PDE count: %u\n", pde_count); >> +seq_printf(s, "Total PTE count: %llu\n", pte_count); > Some of the above looks like it wouldn't be very easily consumed by > scripts. Is that something we want to do? Or is this targetted primarily > at human consumption? Output should be parsable using a simple regex. Could you please clarify what exactly isn't easy? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5] iommu/tegra-smmu: Add pagetable mappings to debugfs
15.03.2021 23:36, Nicolin Chen пишет:
> +static int tegra_smmu_mappings_show(struct seq_file *s, void *data)
> +{
> + struct tegra_smmu_group_debug *group_debug = s->private;
> + const struct tegra_smmu_swgroup *group;
> + 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 val, ptb_reg;
> + u32 *pd;
> +
> + if (!group_debug || !group_debug->priv || !group_debug->group)
> + return 0;
I'm also now curious how difficult would be to read out the actual h/w
state, i.e. check whether ASID is enabled and then dynamically map the
pointed pages instead of using pages allocated by driver. This will show
us the real h/w state. For example this may show mappings left after
bootloader or after reboot/kexec, which could be handy to see.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5] iommu/tegra-smmu: Add pagetable mappings to debugfs
15.03.2021 23:36, Nicolin Chen пишет:
> +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;
> +}
Looking at this again, I'm now wondering whether will be better to
replace dma_addr_t with u32 everywhere since SMMU only supports 32bits
for IOVA.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5] iommu/tegra-smmu: Add pagetable mappings to debugfs
On Mon, Mar 15, 2021 at 01:36:31PM -0700, Nicolin Chen wrote:
> This patch dumps all active mapping entries from pagetable
> to a debugfs directory named "mappings".
>
> Attaching an example:
>
> SWGROUP: hc
> ASID: 0
> reg: 0x250
> PTB_ASID: 0xe0080004
> as->pd_dma: 0x80004000
> {
> [1023] 0xf008000b (1)
> {
> PTE RANGE | ATTR | PHYS | IOVA
> | SIZE
> [#1023, #1023] | 0x5 | 0x000111a8d000 |
> 0xf000 | 0x1000
> }
> }
> Total PDE count: 1
> Total PTE count: 1
>
> Tested-by: Dmitry Osipenko
> Reviewed-by: Dmitry Osipenko
> Signed-off-by: Nicolin Chen
> ---
>
> Changelog
> v5:
> * Fixed a typo in commit message
> * Splitted a long line into two lines
> * Rearranged variable defines by length
> * Added Tested-by and Reviewed-by from Dmitry
> v4: https://lkml.org/lkml/2021/3/14/429
> * Changed %d to %u for unsigned variables
> * Fixed print format mismatch warnings on ARM32
> v3: https://lkml.org/lkml/2021/3/14/30
> * Fixed PHYS and IOVA print formats
> * Changed variables to unsigned int type
> * Changed the table outputs to be compact
> v2: https://lkml.org/lkml/2021/3/9/1382
> * Expanded mutex range to the entire function
> * Added as->lock to protect pagetable walkthrough
> * Replaced devm_kzalloc with devm_kcalloc for group_debug
> * Added "PTE RANGE" and "SIZE" columns to group contiguous mappings
> * Dropped as->count check; added WARN_ON when as->count mismatches
> pd[pd_index]
> v1: https://lkml.org/lkml/2020/9/26/70
>
> drivers/iommu/tegra-smmu.c | 181 -
> 1 file changed, 176 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 97eb62f667d2..b728cae63314 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -19,6 +19,11 @@
> #include
> #include
>
> +struct tegra_smmu_group_debug {
> + const struct tegra_smmu_swgroup *group;
> + void *priv;
This always stores the address space, so why not make this:
struct tegra_smmu_as *as;
? While at it, perhaps throw in a const to make sure we don't modify
this structure in the debugfs code.
> +};
> +
> struct tegra_smmu_group {
> struct list_head list;
> struct tegra_smmu *smmu;
> @@ -47,6 +52,8 @@ struct tegra_smmu {
> struct dentry *debugfs;
>
> struct iommu_device iommu; /* IOMMU Core code handle */
> +
> + struct tegra_smmu_group_debug *group_debug;
> };
>
> struct tegra_smmu_as {
> @@ -152,6 +159,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)
No need for the parentheses here.
>
> static unsigned int iova_pd_index(unsigned long iova)
> {
> @@ -163,6 +173,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;
> @@ -334,7 +350,7 @@ static void tegra_smmu_domain_free(struct iommu_domain
> *domain)
> }
>
> static const struct tegra_smmu_swgroup *
> -tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup)
> +tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup, int
> *index)
> {
> const struct tegra_smmu_swgroup *group = NULL;
> unsigned int i;
> @@ -342,6 +358,8 @@ tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned
> int swgroup)
> for (i = 0; i < smmu->soc->num_swgroups; i++) {
> if (smmu->soc->swgroups[i].swgroup == swgroup) {
> group = >soc->swgroups[i];
> + if (index)
> + *index = i;
This doesn't look like the right place for this. And this also makes
things hard to follow because it passes out-of-band data in the index
parameter.
I'm thinking that this could benefit from a bit of refactoring where
we could for example embed struct tegra_smmu_group_debug into struct
tegra_smmu_group and then reference that when necessary instead of
carrying all that data in an orthogonal array. That should also make
it easier to track this.
Come to think of it, everything that's currently in your new struct
tegra_smmu_group_debug would be useful in struct tegra_smmu_group,
irrespective of debugfs support.
>
[PATCH v5] iommu/tegra-smmu: Add pagetable mappings to debugfs
This patch dumps all active mapping entries from pagetable
to a debugfs directory named "mappings".
Attaching an example:
SWGROUP: hc
ASID: 0
reg: 0x250
PTB_ASID: 0xe0080004
as->pd_dma: 0x80004000
{
[1023] 0xf008000b (1)
{
PTE RANGE | ATTR | PHYS | IOVA
| SIZE
[#1023, #1023] | 0x5 | 0x000111a8d000 | 0xf000
| 0x1000
}
}
Total PDE count: 1
Total PTE count: 1
Tested-by: Dmitry Osipenko
Reviewed-by: Dmitry Osipenko
Signed-off-by: Nicolin Chen
---
Changelog
v5:
* Fixed a typo in commit message
* Splitted a long line into two lines
* Rearranged variable defines by length
* Added Tested-by and Reviewed-by from Dmitry
v4: https://lkml.org/lkml/2021/3/14/429
* Changed %d to %u for unsigned variables
* Fixed print format mismatch warnings on ARM32
v3: https://lkml.org/lkml/2021/3/14/30
* Fixed PHYS and IOVA print formats
* Changed variables to unsigned int type
* Changed the table outputs to be compact
v2: https://lkml.org/lkml/2021/3/9/1382
* Expanded mutex range to the entire function
* Added as->lock to protect pagetable walkthrough
* Replaced devm_kzalloc with devm_kcalloc for group_debug
* Added "PTE RANGE" and "SIZE" columns to group contiguous mappings
* Dropped as->count check; added WARN_ON when as->count mismatches pd[pd_index]
v1: https://lkml.org/lkml/2020/9/26/70
drivers/iommu/tegra-smmu.c | 181 -
1 file changed, 176 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 97eb62f667d2..b728cae63314 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -19,6 +19,11 @@
#include
#include
+struct tegra_smmu_group_debug {
+ const struct tegra_smmu_swgroup *group;
+ void *priv;
+};
+
struct tegra_smmu_group {
struct list_head list;
struct tegra_smmu *smmu;
@@ -47,6 +52,8 @@ struct tegra_smmu {
struct dentry *debugfs;
struct iommu_device iommu; /* IOMMU Core code handle */
+
+ struct tegra_smmu_group_debug *group_debug;
};
struct tegra_smmu_as {
@@ -152,6 +159,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)
{
@@ -163,6 +173,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;
@@ -334,7 +350,7 @@ static void tegra_smmu_domain_free(struct iommu_domain
*domain)
}
static const struct tegra_smmu_swgroup *
-tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup)
+tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup, int
*index)
{
const struct tegra_smmu_swgroup *group = NULL;
unsigned int i;
@@ -342,6 +358,8 @@ tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned
int swgroup)
for (i = 0; i < smmu->soc->num_swgroups; i++) {
if (smmu->soc->swgroups[i].swgroup == swgroup) {
group = >soc->swgroups[i];
+ if (index)
+ *index = i;
break;
}
}
@@ -350,19 +368,22 @@ tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned
int swgroup)
}
static void tegra_smmu_enable(struct tegra_smmu *smmu, unsigned int swgroup,
- unsigned int asid)
+ struct tegra_smmu_as *as)
{
const struct tegra_smmu_swgroup *group;
+ unsigned int asid = as->id;
unsigned int i;
u32 value;
- group = tegra_smmu_find_swgroup(smmu, swgroup);
+ group = tegra_smmu_find_swgroup(smmu, swgroup, );
if (group) {
value = smmu_readl(smmu, group->reg);
value &= ~SMMU_ASID_MASK;
value |= SMMU_ASID_VALUE(asid);
value |= SMMU_ASID_ENABLE;
smmu_writel(smmu, value, group->reg);
+ if (smmu->group_debug)
+ smmu->group_debug[i].priv = as;
} else {
pr_warn("%s group from swgroup %u not found\n", __func__,
swgroup);
@@ -389,13 +410,15 @@ static void tegra_smmu_disable(struct tegra_smmu *smmu,
