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

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Jan 04, 2024 at 12:52:48AM +0800, Michael Shavit wrote:
> > > 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.
> 
> The different cases (ignoring clean-up) from simplest to least are:
> 1. No change because the STE is already equal to the target.
> 2. Directly writing critical word because that's the only difference.
> 3. Setting unused bits then writing critical word.
> 4. Installing breaking STE, write other words, write critical word.

Right

> Case 2. could potentially be collapsed into 3. if the routine that
> sets unused bits skips over the critical word, so that it's a nop when
> the only change is on that critical word.

Right

> > entry_qwords_used_diff should reflect required changes after setting
> > the unused bits.
> 
> Ohhhhhhh, I see. Your suggestion is essentially to move this block
> into the first call to get_used_qword_diff_indexes:
> > > > > +               /*
> > > > > +                * 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);
> 
> Such that:
> if (hweight8(entry_qwords_used_diff) > 1) => non hitless
> if (hweight8(entry_qwords_used_diff) > 0) => hitless, potentially by
> first setting some unused bits in non-critical qwords.

Yes, sorry it was unclear. Here is the full thing for what I mean:

struct arm_smmu_entry_writer_ops {
	unsigned int num_entry_qwords;
	__le64 v_bit;
	void (*get_used)(const __le64 *entry, __le64 *used);
	void (*sync)(void);
};

enum {
	NUM_ENTRY_QWORDS =
		((sizeof(struct arm_smmu_ste) > sizeof(struct arm_smmu_cd)) ?
			 sizeof(struct arm_smmu_ste) :
			 sizeof(struct arm_smmu_cd)) /
		sizeof(u64)
};

static bool entry_set(const struct arm_smmu_entry_writer_ops *ops,
		      __le64 *entry, const __le64 *target, unsigned int start,
		      unsigned int len)
{
	bool changed = false;

	entry = entry + start;
	target = target + start;
	for (; len != 0; len--, target++, start++) {
		if (*entry != *target) {
			WRITE_ONCE(*entry, *target);
			changed = true;
		}
	}

	if (changed)
		ops->sync();
	return changed;
}

/*
 * Figure out if we can do a hitless update of entry to become target. Returns a
 * bit mask where 1 indicates that qword needs to be set disruptively.
 * unused_update is an intermediate value of entry that has unused bits set to
 * their new values.
 */
static u8 compute_qword_diff(const struct arm_smmu_entry_writer_ops *ops,
			     const __le64 *entry, const __le64 *target,
			     __le64 *unused_update)
{
	__le64 target_used[NUM_ENTRY_QWORDS];
	__le64 cur_used[NUM_ENTRY_QWORDS];
	u8 used_qword_diff = 0;
	unsigned int i;

	ops->get_used(entry, cur_used);
	ops->get_used(target, target_used);

	for (i = 0; i != ops->num_entry_qwords; i++) {
		/*
		 * Masks are up to date, the make functions are not allowed to
		 * set a bit to 1 if the used function doesn't say it is used.
		 */
		WARN_ON_ONCE(target[i] & ~target_used[i]);

		/* Bits can change because they are not currently being used */
		unused_update[i] = (entry[i] & cur_used[i]) |
				   (target[i] & ~cur_used[i]);
		/*
		 * Each bit indicates that a used bit in a qword needs to be
		 * changed after unused_update is applied.
		 */
		if ((unused_update[i] & target_used[i]) !=
		    (target[i] & target_used[i]))
			used_qword_diff |= 1 << i;
	}
	return used_qword_diff;
}

static void arm_smmu_write_entry(const struct arm_smmu_entry_writer_ops *ops,
				 __le64 *entry, const __le64 *target)
{
	__le64 unused_update[NUM_ENTRY_QWORDS];
	u8 used_qword_diff;

	used_qword_diff = compute_qword_diff(ops, entry, target, unused_update);
	if (hweight8(used_qword_diff) > 1) {
		/*
		 * At least two qwords need their used bits to be changed. This
		 * requires a breaking update, zero the V bit, write all qwords
		 * but 0, then set qword 0
		 */
		unused_update[0] = entry[0] & (~ops->v_bit);
		entry_set(ops, entry, unused_update, 0, 1);
		entry_set(ops, entry, target, 1, ops->num_entry_qwords);
		entry_set(ops, entry, target, 0, 1);
	} else if (hweight8(used_qword_diff) == 1) {
		/*
		 * Only one qword needs its used bits to be changed. This is a
		 * hitless update, update all bits the current STE is ignoring
		 * to their new values, then update a single qword to change the
		 * STE and finally 0 and unused bits.
		 */
		entry_set(ops, entry, unused_update, 0, ops->num_entry_qwords);
		entry_set(ops, entry, target, ffs(used_qword_diff) - 1, 1);
		entry_set(ops, entry, target, 0, ops->num_entry_qwords);
	} else {
		/*
		 * If everything is working properly this shouldn't do anything
		 * as unused bits should always be 0 and thus
		 * can't change.
		 */
		WARN_ON_ONCE(entry_set(ops, entry, target, 0,
				       ops->num_entry_qwords));
	}
}

I'm fine with this, if you think it is better please sort out the rest
of the bits and send me a diff and I'll integrate it

Thanks,
Jason