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

[Nicolin Chen]

On Thu, Mar 14, 2024 at 09:23:00PM -0700, Nicolin Chen wrote:
> Hi Michael/Jason,
> 
> On Mon, Mar 04, 2024 at 07:43:52PM -0400, Jason Gunthorpe wrote:
> > Prepare to put the CD code into the same mechanism. Add an ops indirection
> > around all the STE specific code and make the worker functions independent
> > of the entry content being processed.
> > 
> > get_used and sync ops are provided to hook the correct code.
> > 
> > Signed-off-by: Michael Shavit <mshavit at google.com>
> > Signed-off-by: Jason Gunthorpe <jgg at nvidia.com>

With the following trivial comments being sent previously, I've
also retested this version with SVA cases covering two different
S1DSS configurations.

This seems to be the only patch not tagged with Tested-by. So,

Tested-by: Nicolin Chen <nicolinc at nvidia.com>

Thanks
Nicolin

> > ---
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 172 ++++++++++++--------
> >  1 file changed, 104 insertions(+), 68 deletions(-)
> > 
> > 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 c60b067c1f553e..b7f947e36f596f 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -48,8 +48,20 @@ enum arm_smmu_msi_index {
> >  	ARM_SMMU_MAX_MSIS,
> >  };
> >  
> > -static void arm_smmu_sync_ste_for_sid(struct arm_smmu_device *smmu,
> > -				      ioasid_t sid);
> > +struct arm_smmu_entry_writer_ops;
> > +struct arm_smmu_entry_writer {
> > +	const struct arm_smmu_entry_writer_ops *ops;
> > +	struct arm_smmu_master *master;
> > +};
> > +
> > +struct arm_smmu_entry_writer_ops {
> > +	unsigned int num_entry_qwords;
> 
> I vaguely remember some related discussion, yet can't find it
> out. So sorry for questioning this, if it's already discussed.
> Aren't CD and STE having the same num_entry_qwords in terms of
> their values? Feels like we can just use NUM_ENTRY_QWORDS?
> 
> > +	__le64 v_bit;
> > +	void (*get_used)(const __le64 *entry, __le64 *used);
> > +	void (*sync)(struct arm_smmu_entry_writer *writer);
> > +};
> > +
> > +#define NUM_ENTRY_QWORDS (sizeof(struct arm_smmu_ste) / sizeof(u64))
> 
> And this seems to be just a fixed "8"? Since both are defined
> straightforwardly:
> 
> struct arm_smmu_ste {
> 	__le64 data[8];
> };
> ...
> struct arm_smmu_cd {
> 	__le64 data[8];
> };
> 
> Might be a bit nitpicking, yet maybe the other way around?
> 
> #define NUM_ENTRY_QWORDS 8
> ...
> struct arm_smmu_ste {
> 	__le64 data[NUM_ENTRY_QWORDS];
> };
> ...
> struct arm_smmu_cd {
> 	__le64 data[NUM_ENTRY_QWORDS];
> };
> 
> Thanks
> Nicolin