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

Michael Shavit mshavit at google.com
Mon Dec 18 04:42:35 PST 2023


On Mon, Dec 18, 2023 at 8:35 PM Michael Shavit <mshavit at google.com> wrote:
> The following should address the bug and optimization you pointed out
> with minimal adjustments:
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 98aa8cc17b58b..1c35599d944d7 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -971,100 +971,6 @@ void arm_smmu_tlb_inv_asid(struct
> arm_smmu_device *smmu, u16 asid)
>         arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
>  }
>
> -/*
> - * This algorithm updates any STE/CD to any value without creating a situation
> - * where the HW can percieve a corrupted entry. HW is only required
> to have a 64
> - * bit atomicity with stores from the CPU, while entries are many 64 bit values
> - * big.
> - *
> - * The algorithm works by evolving the entry toward the target in a series of
> - * steps. Each step synchronizes with the HW so that the HW can not
> see an entry
> - * torn across two steps. Upon each call cur/cur_used reflect the current
> - * synchronized value seen by the HW.
> - *
> - * During each step the HW can observe a torn entry that has any combination of
> - * the step's old/new 64 bit words. The algorithm objective is for the HW
> - * behavior to always be one of current behavior, V=0, or new behavior, during
> - * each step, and across all steps.
> - *
> - * At each step one of three actions is chosen to evolve cur to target:
> - *  - Update all unused bits with their target values.
> - *    This relies on the IGNORED behavior described in the specification
> - *  - Update a single 64-bit value
> - *  - Update all unused bits and set V=0
> - *
> - * The last two actions will cause cur_used to change, which will
> then allow the
> - * first action on the next step.
> - *
> - * In the most general case we can make any update in three steps:
> - *  - Disrupting the entry (V=0)
> - *  - Fill now unused bits, all bits except V
> - *  - Make valid (V=1), single 64 bit store
> - *
> - * However this disrupts the HW while it is happening. There are several
> - * interesting cases where a STE/CD can be updated without disturbing the HW
> - * because only a small number of bits are changing (S1DSS, CONFIG, etc) or
> - * because the used bits don't intersect. We can detect this by calculating how
> - * many 64 bit values need update after adjusting the unused bits and skip the
> - * V=0 process.
> - */
> -static bool arm_smmu_write_entry_next(__le64 *cur, const __le64 *cur_used,
> -                                     const __le64 *target,
> -                                     const __le64 *target_used, __le64 *step,
> -                                     __le64 v_bit, unsigned int len)
> -{
> -       u8 step_used_diff = 0;
> -       u8 step_change = 0;
> -       unsigned int i;
> -
> -       /*
> -        * Compute a step that has all the bits currently unused by HW set to
> -        * their target values.
> -        */
> -       for (i = 0; i != len; i++) {
> -               step[i] = (cur[i] & cur_used[i]) | (target[i] & ~cur_used[i]);
> -               if (cur[i] != step[i])
> -                       step_change |= 1 << i;
> -               /*
> -                * Each bit indicates if the step is incorrect compared to the
> -                * target, considering only the used bits in the target
> -                */
> -               if ((step[i] & target_used[i]) != (target[i] & target_used[i]))
> -                       step_used_diff |= 1 << i;
> -       }
> -
> -       if (hweight8(step_used_diff) > 1) {
> -               /*
> -                * More than 1 qword is mismatched, this cannot be done without
> -                * a break. Clear the V bit and go again.
> -                */
> -               step[0] &= ~v_bit;
> -       } else if (!step_change && step_used_diff) {
> -               /*
> -                * Have exactly one critical qword, all the other qwords are set
> -                * correctly, so we can set this qword now.
> -                */
> -               i = ffs(step_used_diff) - 1;
> -               step[i] = target[i];
> -       } else if (!step_change) {
> -               /* cur == target, so all done */
> -               if (memcmp(cur, target, len * sizeof(*cur)) == 0)
> -                       return true;
> -
> -               /*
> -                * All the used HW bits match, but unused bits are different.
> -                * Set them as well. Technically this isn't necessary but it
> -                * brings the entry to the full target state, so if there are
> -                * bugs in the mask calculation this will obscure them.
> -                */
> -               memcpy(step, target, len * sizeof(*step));
> -       }
> -
> -       for (i = 0; i != len; i++)
> -               WRITE_ONCE(cur[i], step[i]);
> -       return false;
> -}
> -
>  static void arm_smmu_sync_cd(struct arm_smmu_master *master,
>                              int ssid, bool leaf)
>  {
> @@ -1398,39 +1304,135 @@ static void arm_smmu_get_ste_used(const
> struct arm_smmu_ste *ent,
>         }
>  }
>
> -static bool arm_smmu_write_ste_next(struct arm_smmu_ste *cur,
> -                                   const struct arm_smmu_ste *target,
> -                                   const struct arm_smmu_ste *target_used)
> +/*
> + * Make bits of the current ste that aren't in use by the hardware equal to the
> + * target's bits.
> + */
> +static void arm_smmu_ste_set_unused_bits(
> +                                  struct arm_smmu_ste *cur,
> +                                  const struct arm_smmu_ste *target)
>  {
>         struct arm_smmu_ste cur_used;
> -       struct arm_smmu_ste step;
> +       int i =0;
>
>         arm_smmu_get_ste_used(cur, &cur_used);
> -       return arm_smmu_write_entry_next(cur->data, cur_used.data, target->data,
> -                                        target_used->data, step.data,
> -                                        cpu_to_le64(STRTAB_STE_0_V),
> -                                        ARRAY_SIZE(cur->data));
> +       for (i = 0; i < ARRAY_SIZE(cur->data); i++)
> +               cur->data[i] = (cur->data[i] & cur_used.data[i]) |
> +                              (target->data[i] & ~cur_used.data[i]);
>  }
>
> +/*
> + * Update the STE to the target configuration. The transition from the current
> + * STE to the target STE takes place over multiple steps that attempts to make
> + * the transition hitless if possible. This function takes care not to create a
> + * situation where the HW can perceive a corrupted entry. HW is only
> required to
> + * have a 64 bit atomicity with stores from the CPU, while entries are many 64
> + * bit values big.
> + *
> + * The algorithm works by evolving the entry toward the target in a series of
> + * steps. Each step synchronizes with the HW so that the HW can not
> see an entry
> + * torn across two steps. During each step the HW can observe a torn entry that
> + * has any combination of the step's old/new 64 bit words. The algorithm
> + * objective is for the HW behavior to always be one of current behavior, V=0,
> + * or new behavior.
> + *
> + * In the most general case we can make any update in three steps:
> + *  - Disrupting the entry (V=0)
> + *  - Fill now unused bits, all bits except V
> + *  - Make valid (V=1), single 64 bit store
> + *
> + * However this disrupts the HW while it is happening. There are several
> + * interesting cases where a STE/CD can be updated without disturbing the HW
> + * because only a small number of bits are changing (S1DSS, CONFIG, etc) or
> + * because the used bits don't intersect. We can detect this by calculating how
> + * many 64 bit values need update after adjusting the unused bits and skip the
> + * V=0 process. This relies on the IGNORED behavior described in the
> + * specification
> + */
>  static void arm_smmu_write_ste(struct arm_smmu_device *smmu, u32 sid,
>                                struct arm_smmu_ste *ste,
>                                const struct arm_smmu_ste *target)
>  {
> +       struct arm_smmu_ste staging_ste;
>         struct arm_smmu_ste target_used;
> -       int i;
> +       int writes_required = 0;
> +       u8 staging_used_diff = 0;
> +       int i = 0;
>
> +       /*
> +        * Compute a staging ste that has all the bits currently unused by HW
> +        * set to their target values, such that comitting it to the ste table
> +        * woudn't disrupt the hardware.
> +        */
> +       memcpy(&staging_ste, ste, sizeof(staging_ste));
> +       arm_smmu_ste_set_unused_bits(&staging_ste, target);
> +
> +       /*
> +        * Determine if it's possible to reach the target configuration from the
> +        * staged configured in a single qword write (ignoring bits that are
> +        * unused under the target configuration).
> +        */
>         arm_smmu_get_ste_used(target, &target_used);
> -       /* Masks in arm_smmu_get_ste_used() are up to date */
> -       for (i = 0; i != ARRAY_SIZE(target->data); i++)
> -               WARN_ON_ONCE(target->data[i] & ~target_used.data[i]);
> +       /*
> +        * But first sanity check that masks in arm_smmu_get_ste_used() are up
> +        * to date.
> +        */
> +       for (i = 0; i != ARRAY_SIZE(target->data); i++) {
> +               if (WARN_ON_ONCE(target->data[i] & ~target_used.data[i]))
> +                       target_used.data[i] |= target->data[i];
> +       }
>
> -       for (i = 0; true; i++) {
> -               if (arm_smmu_write_ste_next(ste, target, &target_used))
> -                       break;
> -               arm_smmu_sync_ste_for_sid(smmu, sid);
> -               if (WARN_ON(i == 4))
> -                       break;
> +       for (i = 0; i != ARRAY_SIZE(staging_ste.data); i++) {
> +               /*
> +                * Each bit of staging_used_diff indicates the index of a qword
> +                * within the staged ste that is incorrect compared to the
> +                * target, considering only the used bits in the target
> +                */
> +               if ((staging_ste.data[i] &
> +                   target_used.data[i]) != (target->data[i]))
> +                       staging_used_diff |= 1 << i;
> +       }
> +       if (hweight8(staging_used_diff) > 1) {
> +               /*
> +                * More than 1 qword is mismatched and a hitless transition from
> +                * the current ste to the target ste is not possible. Clear the
> +                * V bit and recompute the staging STE.
> +                * Because the V bit is cleared, the staging STE will be equal
> +                * to the target STE except for the first qword.
> +                */
> +               staging_ste.data[0] &= ~cpu_to_le64(STRTAB_STE_0_V);
> +               arm_smmu_ste_set_unused_bits(&staging_ste, target);
> +               staging_used_diff = 1;
> +       }
> +
> +       /*
> +        * Commit the staging STE.
> +        */
> +       for (i = 0; i != ARRAY_SIZE(staging_ste.data); i++)
> +               WRITE_ONCE(ste->data[i], staging_ste.data[i]);
> +       arm_smmu_sync_ste_for_sid(smmu, sid);
> +
> +       /*
> +        * It's now possible to switch to the target configuration with a write
> +        * to a single qword. Make that switch now.
> +        */
> +       i = ffs(staging_used_diff) - 1;
> +       WRITE_ONCE(ste->data[i], target->data[i]);
> +       arm_smmu_sync_ste_for_sid(smmu, sid);
> +
> +       /*
> +        * Some of the bits set under the previous configuration but unused
> +        * under the target configuration might still bit set. Clear them as
> +        * well. Technically this isn't necessary but it brings the entry to
> +        * the full target state, so if there are bugs in the mask calculation
> +        * this will obscure them.
> +        */
> +       for (i = 0; i != ARRAY_SIZE(ste->data); i++) {
> +               if (ste->data[i] != target->data[i]) {
> +                       WRITE_ONCE(ste->data[i], target->data[i]);
> +               }
>         }
> +       arm_smmu_sync_ste_for_sid(smmu, sid);
>
>         /* It's likely that we'll want to use the new STE soon */
>         if (!(smmu->options & ARM_SMMU_OPT_SKIP_PREFETCH)) {

Sorry, that was the old patch. Here's the new one:

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 98aa8cc17b58b..de2e7d9e5919c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -971,100 +971,6 @@ void arm_smmu_tlb_inv_asid(struct
arm_smmu_device *smmu, u16 asid)
 	arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
 }

-/*
- * This algorithm updates any STE/CD to any value without creating a situation
- * where the HW can percieve a corrupted entry. HW is only required
to have a 64
- * bit atomicity with stores from the CPU, while entries are many 64 bit values
- * big.
- *
- * The algorithm works by evolving the entry toward the target in a series of
- * steps. Each step synchronizes with the HW so that the HW can not
see an entry
- * torn across two steps. Upon each call cur/cur_used reflect the current
- * synchronized value seen by the HW.
- *
- * During each step the HW can observe a torn entry that has any combination of
- * the step's old/new 64 bit words. The algorithm objective is for the HW
- * behavior to always be one of current behavior, V=0, or new behavior, during
- * each step, and across all steps.
- *
- * At each step one of three actions is chosen to evolve cur to target:
- *  - Update all unused bits with their target values.
- *    This relies on the IGNORED behavior described in the specification
- *  - Update a single 64-bit value
- *  - Update all unused bits and set V=0
- *
- * The last two actions will cause cur_used to change, which will
then allow the
- * first action on the next step.
- *
- * In the most general case we can make any update in three steps:
- *  - Disrupting the entry (V=0)
- *  - Fill now unused bits, all bits except V
- *  - Make valid (V=1), single 64 bit store
- *
- * However this disrupts the HW while it is happening. There are several
- * interesting cases where a STE/CD can be updated without disturbing the HW
- * because only a small number of bits are changing (S1DSS, CONFIG, etc) or
- * because the used bits don't intersect. We can detect this by calculating how
- * many 64 bit values need update after adjusting the unused bits and skip the
- * V=0 process.
- */
-static bool arm_smmu_write_entry_next(__le64 *cur, const __le64 *cur_used,
-				      const __le64 *target,
-				      const __le64 *target_used, __le64 *step,
-				      __le64 v_bit, unsigned int len)
-{
-	u8 step_used_diff = 0;
-	u8 step_change = 0;
-	unsigned int i;
-
-	/*
-	 * Compute a step that has all the bits currently unused by HW set to
-	 * their target values.
-	 */
-	for (i = 0; i != len; i++) {
-		step[i] = (cur[i] & cur_used[i]) | (target[i] & ~cur_used[i]);
-		if (cur[i] != step[i])
-			step_change |= 1 << i;
-		/*
-		 * Each bit indicates if the step is incorrect compared to the
-		 * target, considering only the used bits in the target
-		 */
-		if ((step[i] & target_used[i]) != (target[i] & target_used[i]))
-			step_used_diff |= 1 << i;
-	}
-
-	if (hweight8(step_used_diff) > 1) {
-		/*
-		 * More than 1 qword is mismatched, this cannot be done without
-		 * a break. Clear the V bit and go again.
-		 */
-		step[0] &= ~v_bit;
-	} else if (!step_change && step_used_diff) {
-		/*
-		 * Have exactly one critical qword, all the other qwords are set
-		 * correctly, so we can set this qword now.
-		 */
-		i = ffs(step_used_diff) - 1;
-		step[i] = target[i];
-	} else if (!step_change) {
-		/* cur == target, so all done */
-		if (memcmp(cur, target, len * sizeof(*cur)) == 0)
-			return true;
-
-		/*
-		 * All the used HW bits match, but unused bits are different.
-		 * Set them as well. Technically this isn't necessary but it
-		 * brings the entry to the full target state, so if there are
-		 * bugs in the mask calculation this will obscure them.
-		 */
-		memcpy(step, target, len * sizeof(*step));
-	}
-
-	for (i = 0; i != len; i++)
-		WRITE_ONCE(cur[i], step[i]);
-	return false;
-}
-
 static void arm_smmu_sync_cd(struct arm_smmu_master *master,
 			     int ssid, bool leaf)
 {
@@ -1398,40 +1304,160 @@ static void arm_smmu_get_ste_used(const
struct arm_smmu_ste *ent,
 	}
 }

-static bool arm_smmu_write_ste_next(struct arm_smmu_ste *cur,
-				    const struct arm_smmu_ste *target,
-				    const struct arm_smmu_ste *target_used)
+/*
+ * Make bits of the current ste that aren't in use by the hardware equal to the
+ * target's bits.
+ */
+static void arm_smmu_ste_set_unused_bits(
+			           struct arm_smmu_ste *cur,
+			           const struct arm_smmu_ste *target)
 {
 	struct arm_smmu_ste cur_used;
-	struct arm_smmu_ste step;
+	int i =0;

 	arm_smmu_get_ste_used(cur, &cur_used);
-	return arm_smmu_write_entry_next(cur->data, cur_used.data, target->data,
-					 target_used->data, step.data,
-					 cpu_to_le64(STRTAB_STE_0_V),
-					 ARRAY_SIZE(cur->data));
+	for (i = 0; i < ARRAY_SIZE(cur->data); i++)
+		cur->data[i] = (cur->data[i] & cur_used.data[i]) |
+			       (target->data[i] & ~cur_used.data[i]);
 }

+/*
+ * Each bit of the return value indicates the index of a qword within the ste
+ * that is incorrect compared to the target, considering only the used bits in
+ * the target
+ */
+static u8 arm_smmu_ste_used_qword_diff_indexes(const struct arm_smmu_ste *ste,
+					       const struct arm_smmu_ste *target)
+{
+	struct arm_smmu_ste target_used;
+	u8 qword_diff_indexes = 0;
+	int i = 0;
+
+	arm_smmu_get_ste_used(target, &target_used);
+	for (i = 0; i < ARRAY_SIZE(ste->data); i++) {
+		if ((ste->data[i] & target_used.data[i]) !=
+		    (target->data[i] & target_used.data[i]))
+			qword_diff_indexes |= 1 << i;
+	}
+	return qword_diff_indexes;
+}
+
+/*
+ * Update the STE to the target configuration. The transition from the current
+ * STE to the target STE takes place over multiple steps that attempts to make
+ * the transition hitless if possible. This function takes care not to create a
+ * situation where the HW can perceive a corrupted entry. HW is only
required to
+ * have a 64 bit atomicity with stores from the CPU, while entries are many 64
+ * bit values big.
+ *
+ * The algorithm works by evolving the entry toward the target in a series of
+ * steps. Each step synchronizes with the HW so that the HW can not
see an entry
+ * torn across two steps. During each step the HW can observe a torn entry that
+ * has any combination of the step's old/new 64 bit words. The algorithm
+ * objective is for the HW behavior to always be one of current behavior, V=0,
+ * or new behavior.
+ *
+ * In the most general case we can make any update in three steps:
+ *  - Disrupting the entry (V=0)
+ *  - Fill now unused bits, all bits except V
+ *  - Make valid (V=1), single 64 bit store
+ *
+ * However this disrupts the HW while it is happening. There are several
+ * interesting cases where a STE/CD can be updated without disturbing the HW
+ * because only a small number of bits are changing (S1DSS, CONFIG, etc) or
+ * because the used bits don't intersect. We can detect this by calculating how
+ * many 64 bit values need update after adjusting the unused bits and skip the
+ * V=0 process. This relies on the IGNORED behavior described in the
+ * specification
+ */
 static void arm_smmu_write_ste(struct arm_smmu_device *smmu, u32 sid,
 			       struct arm_smmu_ste *ste,
 			       const struct arm_smmu_ste *target)
 {
-	struct arm_smmu_ste target_used;
-	int i;
+	bool cleanup_sync_required = false;
+	struct arm_smmu_ste staging_ste;
+	u8 ste_qwords_used_diff = 0;
+	int i = 0;

-	arm_smmu_get_ste_used(target, &target_used);
-	/* Masks in arm_smmu_get_ste_used() are up to date */
-	for (i = 0; i != ARRAY_SIZE(target->data); i++)
-		WARN_ON_ONCE(target->data[i] & ~target_used.data[i]);
+	ste_qwords_used_diff = arm_smmu_ste_used_qword_diff_indexes(ste, target);
+	if (WARN_ON_ONCE(ste_qwords_used_diff == 0))
+		return;

-	for (i = 0; true; i++) {
-		if (arm_smmu_write_ste_next(ste, target, &target_used))
-			break;
+	if (hweight8(ste_qwords_used_diff) > 1) {
+		/*
+		 * If transitioning to the target STE with a single qword write
+		 * isn't possible, then we must first transition to an
+		 * intermediate STE. The intermediate STE may either be an STE
+		 * that melds bits of the target STE into the current STE
+		 * without affecting bits used by the hardware under the current
+		 * configuration; or a breaking STE if a hitless transition to
+		 * the target isn't possible.
+		 */
+
+		/*
+		 * Compute a staging ste that has all the bits currently unused
+		 * by HW set to their target values, such that comitting it to
+		 * the ste table woudn't disrupt the hardware.
+		 */
+		memcpy(&staging_ste, ste, sizeof(staging_ste));
+		arm_smmu_ste_set_unused_bits(&staging_ste, target);
+
+		ste_qwords_used_diff = arm_smmu_ste_used_qword_diff_indexes(ste,
+									    target);
+		if (hweight8(ste_qwords_used_diff) > 1) {
+			/*
+			 * More than 1 qword is mismatched between the staging
+			 * and target STE. A hitless transition to the target
+			 * ste is not possible. Set the staging STE to be equal
+			 * to the target STE, apart from the V bit's qword. As
+			 * long as the V bit is cleared first then writes to the
+			 * subsequent qwords will not further disrupt the
+			 * hardware.
+			 */
+			memcpy(&staging_ste, ste, sizeof(staging_ste));
+			staging_ste.data[0] &= ~cpu_to_le64(STRTAB_STE_0_V);
+			arm_smmu_ste_set_unused_bits(&staging_ste, target);
+			/*
+			 * After comitting the staging STE, only the 0th qword
+			 * will differ from the target.
+			 */
+			ste_qwords_used_diff = 1;
+		}
+
+		/*
+		 * Commit the staging STE. Note that the iteration order
+		 * matters, as we may be comitting a breaking STE in the
+		 * non-hitless case.
+		 */
+		for (i = 0; i != ARRAY_SIZE(staging_ste.data); i++)
+			WRITE_ONCE(ste->data[i], staging_ste.data[i]);
 		arm_smmu_sync_ste_for_sid(smmu, sid);
-		if (WARN_ON(i == 4))
-			break;
 	}

+	/*
+	 * It's now possible to switch to the target configuration with a write
+	 * to a single qword. Make that switch now.
+	 */
+	i = ffs(ste_qwords_used_diff) - 1;
+	WRITE_ONCE(ste->data[i], target->data[i]);
+	arm_smmu_sync_ste_for_sid(smmu, sid);
+
+	/*
+	 * Some of the bits set under the previous configuration but unused
+	 * under the target configuration might still bit set. Clear them as
+	 * well. Technically this isn't necessary but it brings the entry to
+	 * the full target state, so if there are bugs in the mask calculation
+	 * this will obscure them.
+	 */
+	for (i = 0; i != ARRAY_SIZE(ste->data); i++) {
+		if (ste->data[i] != target->data[i]) {
+			WRITE_ONCE(ste->data[i], target->data[i]);
+			cleanup_sync_required = true;
+		}
+	}
+	if (cleanup_sync_required)
+		arm_smmu_sync_ste_for_sid(smmu, sid);
+
 	/* It's likely that we'll want to use the new STE soon */
 	if (!(smmu->options & ARM_SMMU_OPT_SKIP_PREFETCH)) {
 		struct arm_smmu_cmdq_ent



More information about the linux-arm-kernel mailing list