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

From: Jason Gunthorpe <jgg@nvidia.com>
To: Mostafa Saleh <smostafa@google.com>
Cc: iommu@lists.linux.dev, Joerg Roedel <joro@8bytes.org>,
	linux-arm-kernel@lists.infradead.org,
	Robin Murphy <robin.murphy@arm.com>,
	Will Deacon <will@kernel.org>, Eric Auger <eric.auger@redhat.com>,
	Moritz Fischer <mdf@kernel.org>,
	Moritz Fischer <moritzf@google.com>,
	Michael Shavit <mshavit@google.com>,
	Nicolin Chen <nicolinc@nvidia.com>,
	patches@lists.linux.dev,
	Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Subject: Re: [PATCH v7 2/9] iommu/arm-smmu-v3: Make CD programming use arm_smmu_write_entry()
Date: Mon, 29 Apr 2024 11:29:05 -0300	[thread overview]
Message-ID: <20240429142905.GF941030@nvidia.com> (raw)
In-Reply-To: <Zi13eaOCQFdMO1ut@google.com>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel