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

From: Jason Gunthorpe <jgg@nvidia.com>
To: Nicolin Chen <nicolinc@nvidia.com>
Cc: Michael Shavit <mshavit@google.com>,
	iommu@lists.linux.dev, Joerg Roedel <joro@8bytes.org>,
	linux-arm-kernel@lists.infradead.org,
	Robin Murphy <robin.murphy@arm.com>,
	Will Deacon <will@kernel.org>, Eric Auger <eric.auger@redhat.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Moritz Fischer <mdf@kernel.org>,
	patches@lists.linux.dev,
	Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Subject: Re: [PATCH v5 04/27] iommu/arm-smmu-v3: Add an ops indirection to the STE code
Date: Mon, 18 Mar 2024 15:06:27 -0300	[thread overview]
Message-ID: <20240318180627.GF5825@nvidia.com> (raw)
In-Reply-To: <ZfPNGPhdhcj6Mq3V@nvidia.com>

On Thu, Mar 14, 2024 at 09:22:48PM -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@google.com>
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >  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?

They do, but the structs should reflect the HW and there is nothing in
the HW description that requires these to be the same.

It is true we go through some contortions to keep the sizes seperate,
and it is also true they are unlikely to ever change size.

> > +#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:

Yes, again it should flow from the struct which should reflect the HW
layout. Making them the same size is a SW desire..

Jason

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