Re: [PATCH v7 2/9] iommu/arm-smmu-v3: Make CD programming use arm_smmu_write_entry()

Jason Gunthorpe
2024-04-29T14:29:30Z

On Sat, Apr 27, 2024 at 10:08:57PM +0000, Mostafa Saleh wrote:
> > The issue is the old logic constructs the new CD by manipulating the
> > existing CD in various ways "in place" that ends up creating CDs that
> > don't meet the requirements for the new programmer. For instance EPD0
> > will be set and the TTB0 will also be left programmed.
> >
>
> I see, but what I don’t understand is why doesn't the function construct
> the CD correctly, as from

Why? Because it never had to before. It made minimal edits to minimize
the code.

> } else if (cd == &quiet_cd) { /* (4) */
> if (!(smmu->features & ARM_SMMU_FEAT_STALL_FORCE))
> val &= ~(CTXDESC_CD_0_S | CTXDESC_CD_0_R);
> val |= CTXDESC_CD_0_TCR_EPD0;
> // populate the rest of the CD correctly here.
> }

What you are asking for is this:

cd_live = !!(val & CTXDESC_CD_0_V);

if (!cd) { /* (5) */
+ memset(cdptr, 0, sizeof(*cdptr));
val = 0;
} else if (cd == &quiet_cd) { /* (4) */
+ val &= ~(CTXDESC_CD_0_TCR_T0SZ | CTXDESC_CD_0_TCR_TG0 |
+ CTXDESC_CD_0_TCR_IRGN0 | CTXDESC_CD_0_TCR_ORGN0 |
+ CTXDESC_CD_0_TCR_SH0);
if (!(smmu->features & ARM_SMMU_FEAT_STALL_FORCE))
val &= ~(CTXDESC_CD_0_S | CTXDESC_CD_0_R);
val |= CTXDESC_CD_0_TCR_EPD0;
+ cdptr->data[1] &= ~cpu_to_le64(CTXDESC_CD_1_TTB0_MASK);
} else if (cd_live) { /* (3) */
val &= ~CTXDESC_CD_0_ASID;
val |= FIELD_PREP(CTXDESC_CD_0_ASID, cd->asid);

I think.. I've been staring at this a while now and I *think* it
covers all the cases and we won't hit the WARN_ON?

So sure, lets do it that way, the code is all deleted anyhow ..

> As I don’t think the right approach is to populate the CD incorrectly
> and then clear the parts not needed for EPD0.

It is very easy to see that such a simple algorithm will not trigger
the WARN_ON. The above is somewhat trickier.

> Also, TTB0 is ignored anyway in that case, no?

Only by HW, there is a protective WARN_ON that will trigger in the
programmer, that is what this is trying to avoid. For bisection.

Jason