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

[Jason Gunthorpe]

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