Re: [PATCH v5 04/27] iommu/arm-smmu-v3: Add an ops indirection to the STE code

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Mar 22, 2024 at 06:14:24PM +0000, Mostafa Saleh wrote:
> > @@ -1027,57 +1038,55 @@ static void arm_smmu_get_ste_used(const struct arm_smmu_ste *ent,
> >   * unused_update is an intermediate value of entry that has unused bits set to
> >   * their new values.
> >   */
> > -static u8 arm_smmu_entry_qword_diff(const struct arm_smmu_ste *entry,
> > -				    const struct arm_smmu_ste *target,
> > -				    struct arm_smmu_ste *unused_update)
> > +static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer,
> > +				    const __le64 *entry, const __le64 *target,
> > +				    __le64 *unused_update)
> >  {
> > -	struct arm_smmu_ste target_used = {};
> > -	struct arm_smmu_ste cur_used = {};
> > +	__le64 target_used[NUM_ENTRY_QWORDS] = {};
> > +	__le64 cur_used[NUM_ENTRY_QWORDS] = {};
> This is confusing to me, the function was modified to be generic, so its has
> args are __le64 * instead of struct arm_smmu_ste *.

Right

> But NUM_ENTRY_QWORDS is defined as “(sizeof(struct arm_smmu_ste) / sizeof(u64))”
> and in the same function writer->ops->num_entry_qwords is used
> nterchangeably,

Right

> I understand that this not a constant and the compiler would complain.
> But since for any other num_entry_qwords larger than NUM_ENTRY_QWORDS it fails,
> and we know STEs and CDs both have the same size, we simplify the code and make
> it a constant everywhere.

So you say to get rid of num_entry_qwords and just use the constant?

> I see in the next patch, that this is redefined to be the max between STE and
> CD, but again, this hardware and it never changes, so my opinion is to simplify
> the code, as there is no need to generalize this part.

Yes, we need a constant.

It would look like this, it is a little bit simpler:

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 a54062faccde38..d015f41900d802 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -63,9 +63,9 @@ enum arm_smmu_msi_index {
 	ARM_SMMU_MAX_MSIS,
 };
 
-#define NUM_ENTRY_QWORDS                                                \
-	(max(sizeof(struct arm_smmu_ste), sizeof(struct arm_smmu_cd)) / \
-	 sizeof(u64))
+#define NUM_ENTRY_QWORDS 8
+static_assert(sizeof(struct arm_smmu_ste) == NUM_ENTRY_QWORDS * sizeof(u64));
+static_assert(sizeof(struct arm_smmu_cd) == NUM_ENTRY_QWORDS * sizeof(u64));
 
 static phys_addr_t arm_smmu_msi_cfg[ARM_SMMU_MAX_MSIS][3] = {
 	[EVTQ_MSI_INDEX] = {
@@ -1045,7 +1045,7 @@ static u8 arm_smmu_entry_qword_diff(struct arm_smmu_entry_writer *writer,
 	writer->ops->get_used(entry, cur_used);
 	writer->ops->get_used(target, target_used);
 
-	for (i = 0; i != writer->ops->num_entry_qwords; i++) {
+	for (i = 0; i != NUM_ENTRY_QWORDS; i++) {
 		/*
 		 * Check that 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
@@ -1114,7 +1114,6 @@ static bool entry_set(struct arm_smmu_entry_writer *writer, __le64 *entry,
 void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, __le64 *entry,
 			  const __le64 *target)
 {
-	unsigned int num_entry_qwords = writer->ops->num_entry_qwords;
 	__le64 unused_update[NUM_ENTRY_QWORDS];
 	u8 used_qword_diff;
 
@@ -1137,9 +1136,9 @@ void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, __le64 *entry,
 		 */
 		unused_update[critical_qword_index] =
 			entry[critical_qword_index];
-		entry_set(writer, entry, unused_update, 0, num_entry_qwords);
+		entry_set(writer, entry, unused_update, 0, NUM_ENTRY_QWORDS);
 		entry_set(writer, entry, target, critical_qword_index, 1);
-		entry_set(writer, entry, target, 0, num_entry_qwords);
+		entry_set(writer, entry, target, 0, NUM_ENTRY_QWORDS);
 	} else if (used_qword_diff) {
 		/*
 		 * At least two qwords need their inuse bits to be changed. This
@@ -1148,7 +1147,7 @@ void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, __le64 *entry,
 		 */
 		unused_update[0] = entry[0] & (~writer->ops->v_bit);
 		entry_set(writer, entry, unused_update, 0, 1);
-		entry_set(writer, entry, target, 1, num_entry_qwords - 1);
+		entry_set(writer, entry, target, 1, NUM_ENTRY_QWORDS - 1);
 		entry_set(writer, entry, target, 0, 1);
 	} else {
 		/*
@@ -1157,7 +1156,7 @@ void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, __le64 *entry,
 		 * compute_qword_diff().
 		 */
 		WARN_ON_ONCE(
-			entry_set(writer, entry, target, 0, num_entry_qwords));
+			entry_set(writer, entry, target, 0, NUM_ENTRY_QWORDS));
 	}
 }
 
@@ -1272,7 +1271,6 @@ static const struct arm_smmu_entry_writer_ops arm_smmu_cd_writer_ops = {
 	.sync = arm_smmu_cd_writer_sync_entry,
 	.get_used = arm_smmu_get_cd_used,
 	.v_bit = cpu_to_le64(CTXDESC_CD_0_V),
-	.num_entry_qwords = sizeof(struct arm_smmu_cd) / sizeof(u64),
 };
 
 void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid,
@@ -1460,7 +1458,6 @@ static const struct arm_smmu_entry_writer_ops arm_smmu_ste_writer_ops = {
 	.sync = arm_smmu_ste_writer_sync_entry,
 	.get_used = arm_smmu_get_ste_used,
 	.v_bit = cpu_to_le64(STRTAB_STE_0_V),
-	.num_entry_qwords = sizeof(struct arm_smmu_ste) / sizeof(u64),
 };
 
 static void arm_smmu_write_ste(struct arm_smmu_master *master, u32 sid,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 8ba07b00bf6056..5936dc5f76786a 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -779,7 +779,6 @@ struct arm_smmu_entry_writer {
 };
 
 struct arm_smmu_entry_writer_ops {
-	unsigned int num_entry_qwords;
 	__le64 v_bit;
 	void (*get_used)(const __le64 *entry, __le64 *used);
 	void (*sync)(struct arm_smmu_entry_writer *writer);


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