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

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Jan 02, 2024 at 04:08:41PM +0800, Michael Shavit wrote:
> On Wed, Dec 27, 2023 at 11:46 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > 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
> 
> This branch is probably a bit more complicated than that. It's a bit more like:
>        if (hweight8(entry_qwords_used_diff) > 1) {
>              compute_staging_entry(...);
>              compute_used_diffs(...staging_entry...)
>              if (hweight(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 {
>                  set(qword_start=0, qword_end=N, staging_entry, entry)
>                  critical = ffs(..);
>                  set(qword_start=critical,qword_end=critical+1);
>                  set(qword_start=0,qword_end=N);
>              }
>       }
> 
> >        } 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);
> 
> And then this branch is the case where you can directly switch to the
> entry without first setting unused bits.

Don't make that a special case, just always set the unused bits. All
the setting functions should skip the sync if they didn't change the
entry, so we don't need to care if we call them needlessly.

There are only three programming sequences.

entry_qwords_used_diff should reflect required changes after setting
the unused bits.

> > > +       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.
> 
> The reason I made get_used_qword_diff_indexes an op is because the
> algorithm needs to compute the used_bits for entries (for the current
> entry, the target entry as well as the melded-staging entry).

Make getting the used bits the op..

Jason

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