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

[Mostafa Saleh]

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