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

Mostafa Saleh
2024-04-29T15:31:28Z

On Mon, Apr 29, 2024 at 11:29:05AM -0300, Jason Gunthorpe wrote:
> 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.

I understand, my point was why don’t we introduce a new logic to construct it
correctly, instead of hacking the old one, as it is much easier to reason
about (at least from my point of view)

>
> > } 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?
>

That’s similar to how I imagined it.

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

I agree, if it's deleted anyway we shouldn't put much time, I haven't
looked at the SVA patch yet.

> > 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.

Makes sense.

Thanks,
Mostafa
> Jason