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

Mostafa Saleh
2024-04-27T22:09:20Z

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