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

[Mostafa Saleh <smostafa@google.com>]

On Mon, Apr 22, 2024 at 10:29:54AM -0300, Jason Gunthorpe wrote:
> On Fri, Apr 19, 2024 at 09:07:19PM +0000, Mostafa Saleh wrote:
> > > -	cdptr = arm_smmu_get_cd_ptr(master, ssid);
> > > -	if (!cdptr)
> > > +	cd_table_entry = arm_smmu_get_cd_ptr(master, ssid);
> > > +	if (!cd_table_entry)
> > >  		return -ENOMEM;
> > >  
> > > +	target = *cd_table_entry;
> > 
> > As this changes the logic where all CD manipulation is not on the actual
> > CD, I believe a comment would be helpful here.
> 
> This is all deleted in a few patches, doesn't seem worth it to
> me. These steps exist only for bisection.
> 
> > > @@ -1299,18 +1357,14 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
> > >  		if (cd_table->stall_enabled)
> > >  			val |= CTXDESC_CD_0_S;
> > >  	}
> > > -
> > > +	cdptr->data[0] = cpu_to_le64(val);
> > >  	/*
> > > -	 * The SMMU accesses 64-bit values atomically. See IHI0070Ca 3.21.3
> > > -	 * "Configuration structures and configuration invalidation completion"
> > > -	 *
> > > -	 *   The size of single-copy atomic reads made by the SMMU is
> > > -	 *   IMPLEMENTATION DEFINED but must be at least 64 bits. Any single
> > > -	 *   field within an aligned 64-bit span of a structure can be altered
> > > -	 *   without first making the structure invalid.
> > > +	 * Since the above is updating the CD entry based on the current value
> > > +	 * without zeroing unused bits it needs fixing before being passed to
> > > +	 * the programming logic.
> > >  	 */
> > > -	WRITE_ONCE(cdptr->data[0], cpu_to_le64(val));
> > > -	arm_smmu_sync_cd(master, ssid, true);
> > > +	arm_smmu_clean_cd_entry(&target);
> > 
> > I am not sure I understand the logic here, is that only needed for entry[0]
> > As I see the other entries are set and not reused.
> 
> I'm not sure what you are asking?
> 
> 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
	} 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.
	}

As I  don’t think the right approach is to populate the CD incorrectly
and then clear the parts not needed for EPD0.
Also, TTB0 is ignored anyway in that case, no?

Thanks,
Mostafa

> > If so, I think it’d be better to make that clear, also as used_bits
> > are always 0xff for all cases, I believe the EPD0 logic should be
> > integrated in populating the CD so it is correct by construction, as
> > this looks like a hack to me.
> 
> Yes, this is what happens, in a few more steps. We have to go and
> build the missing make functions first.
> 
> There is a bit of a circular problem here: the new scheme expects that
> the CD is only programmed by the new scheme and follows the rules - eg
> no unused bits set. While the old scheme doesn't follow the rules.
> 
> So this patch makes the old scheme follow the rules and be compatible
> with the new scheme then we go place by place and convert to the new
> scheme. Then we remove the old scheme entirely. Look at the "Move the
> CD generation for SVA into a function" patch.
> 
> Yes, this is a minimal hack to let the next few patches work out
> correctly without breaking bisection.
> 
> How about a new commit message:
> 
> iommu/arm-smmu-v3: Make CD programming use arm_smmu_write_entry()
> 
> CD table entries and STE's have the same essential programming sequence,
> just with different types. Use the new ops indirection to link CD
> programming to the common writer.
> 
> In a few more patches all CD writers will call an appropriate make
> function and then directly call arm_smmu_write_cd_entry().
> arm_smmu_write_ctx_desc() will be removed.
> 
> Until then lightly tweak arm_smmu_write_ctx_desc() to also use the new
> programmer by using the same logic as right now to build the target CD on
> the stack, sanitizing it to meet the used rules, and then using the
> writer.
> 
> This is necessary because the writer expects that the currently programmed
> CD follows the used rules. Next patches add new make functions and new
> direct calls to arm_smmu_write_cd_entry() which will require this.
> 
> Jason