Re: [PATCH v4 02/16] iommu/arm-smmu-v3: Consolidate the STE generation for abort/bypass

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Feb 01, 2024 at 11:32:41AM +0000, Mostafa Saleh wrote:
> On Wed, Jan 31, 2024 at 10:47:02AM -0400, Jason Gunthorpe wrote:
> > On Wed, Jan 31, 2024 at 02:40:24PM +0000, Mostafa Saleh wrote:
> > > > +static void arm_smmu_make_bypass_ste(struct arm_smmu_ste *target)
> > > > +{
> > > > +	memset(target, 0, sizeof(*target));
> > > 
> > > I see this can be used with the actual STE. Although this is done at init, but
> > > briefly making the STE abort from “arm_smmu_make_bypass_ste”, seems a bit
> > > fragile to me, in case we use this in the future in different scenarios, it
> > > might break the hitless assumption. But no strong opinion though.
> > 
> > At init time, when that case happens, the STE table hasn't been
> > installed in the HW yet. This is why that specific code path has been
> > directly manipulating the STE and does not call the normal update path
> > with the sync'ing.
> > 
> > It is perhaps subtle that is why it is a different flow.
> > 
> > (this is also why I moved the function order as it was a bit obscure
> > to see that indeed all this stuff was sequenced right and we were not
> > updating a live STE improperly)
> 
> I agree, this is not an issue, but it was just confusing when I first read it
> as “arm_smmu_make_bypass_ste” would make the STE transiently unavailable, and
> I had to go and check the usage. Maybe we can add a comment to clarify how this
> function is expected to be used.

I added this remark:

@@ -1592,6 +1592,10 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
        arm_smmu_write_ste(master, sid, dst, &target);
 }
 
+/*
+ * This can safely directly manipulate the STE memory without a sync sequence
+ * because the STE table has not been installed in the SMMU yet.
+ */
 static void arm_smmu_init_bypass_stes(struct arm_smmu_ste *strtab,
                                      unsigned int nent)
 {
@@ -3971,6 +3975,10 @@ static void arm_smmu_rmr_install_bypass_ste(struct arm_smmu_device *smmu)
                                continue;
                        }
 
+                       /*
+                        * STE table is not programmed to HW, see
+                        * arm_smmu_init_bypass_stes()
+                        */
                        arm_smmu_make_bypass_ste(
                                arm_smmu_get_step_for_sid(smmu, rmr->sids[i]));
                }

Jason