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

[Mostafa Saleh <smostafa@google.com>]

On Mon, Mar 25, 2024 at 11:11:32AM -0300, Jason Gunthorpe wrote:
> 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?

In my opinion, yes, that looks easier to understand, and avoids the MAX
stuff as there is no reason for the extra generalisation.

> > 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);
> 

Thanks,
Mostafa