Re: [PATCH 04/19] iommu/arm-smmu-v3: Make STE programming independent of the callers

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Dec 19, 2023 at 09:42:27PM +0800, Michael Shavit wrote:

> +static void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer,
> +                                __le64 *cur, const __le64 *target,
> +                                __le64 *staging_entry)
> +{
> +       bool cleanup_sync_required = false;
> +       u8 entry_qwords_used_diff = 0;
> +       int i = 0;
> +
> +       entry_qwords_used_diff =
> +               writer->ops.get_used_qword_diff_indexes(cur, target);
> +       if (WARN_ON_ONCE(entry_qwords_used_diff == 0))
> +               return;

A no change update is actually API legal, eg we can set the same
domain twice in a row. It should just do nothing.

If the goal is to improve readability I'd split this into smaller
functions and have the main function look like this:

       compute_used(..)
       if (hweight8(entry_qwords_used_diff) > 1) {
             set_v_0(..);
             set(qword_start=1,qword_end=N);
	     set(qword_start=0,qword_end=1); // V=1
       } else if (hweight8(entry_qwords_used_diff) == 1) {
             set_unused(..);
	     critical = ffs(..);
             set(qword_start=critical,qword_end=critical+1);
             set(qword_start=0,qword_end=N);
       } else { // hweight8 == 0
             set(qword_start=0,qword_end=N);
       }

Then the three different programming algorithms are entirely clear in
code. Make the generic set() function skip the sync if nothing
changed.

> +       if (hweight8(entry_qwords_used_diff) > 1) {
> +               /*
> +                * If transitioning to the target entry with a single qword
> +                * write isn't possible, then we must first transition to an
> +                * intermediate entry. The intermediate entry may either be an
> +                * entry that melds bits of the target entry into the current
> +                * entry without disrupting the hardware, or a breaking entry if
> +                * a hitless transition to the target is impossible.
> +                */
> +
> +               /*
> +                * Compute a staging entry that has all the bits currently
> +                * unused by HW set to their target values, such that comitting
> +                * it to the entry table woudn't disrupt the hardware.
> +                */
> +               memcpy(staging_entry, cur, writer->entry_length);
> +               writer->ops.set_unused_bits(staging_entry, target);
> +
> +               entry_qwords_used_diff =
> +                       writer->ops.get_used_qword_diff_indexes(staging_entry,
> +                                                               target);

Put the number qwords directly in the ops struct and don't make this
an op.  Above will need N=number of qwords as well.

Regrads,
Jason