Re: [PATCH v4 09/12] iommu/vt-d: Add iotlb flush for nested domain

[Nicolin Chen <nicolinc@nvidia.com>]

On Wed, Aug 09, 2023 at 04:19:01PM -0300, Jason Gunthorpe wrote:
> On Wed, Aug 09, 2023 at 12:12:25PM -0700, Nicolin Chen wrote:
> > On Wed, Aug 09, 2023 at 01:24:56PM -0300, Jason Gunthorpe wrote:
> > > On Wed, Aug 09, 2023 at 09:30:12AM +0000, Liu, Yi L wrote:
> > > 
> > > > Yeah, adding new structures to ucmd_buffer may increase the size as
> > > > well if the new one is larger. While for an array, if there is new entry,
> > > > it is for sure to increase the size. I remember there is one tricky thing
> > > > when handling the selftest type. E.g. it is defined as 0xbadbeef, if using
> > > > it to index array, it would expire. So we have some special handling on
> > > > it. If defining the things in iommu_ops, it is simpler. Selftest may be
> > > > not so critical to determining the direction though.
> > > 
> > > Maybe we are trying too hard to make it "easy" on the driver.
> > > 
> > > Can't we just have the driver invoke some:
> > > 
> > > driver_iommufd_invalidate_op(??? *opaque)
> > > {
> > > 	struct driver_base_struct args;
> > > 
> > >         rc = iommufd_get_args(opaque, &args, sizeof(args),
> > > 	     offsetof(args, last));
> > 
> > OK. So, IIUIC, the opaque should be:
> > 
> > struct iommu_user_data {
> > 	void __user *data_uptr;
> > 	size_t data_len;
> > }user_data;
> > 
> > And core does basic sanity of data_uptr != NULL and data_len !=0
> > before passing this to driver, and then do a full sanity during
> > the iommufd_get_args (or iommufd_get_user_data?) call.
> 
> Don't even need to check datA_uptr and data_len, the helper should
> check the size and null is caught by copy from user

I see. I was worried about the alloc path since its data input is
optional upon IOMMU_DOMAIN_UNMANAGED. But this helper should work
for that also.

In that case, we might not even need to define the union with all
structures, in iommu.h.

> > > Similarly for managing the array of invalidation commands.
> > 
> > You mean an embedded uptr inside a driver user data struct right?
> > Sure, that should go through the new helper too.
> 
> If we are committed that all drivers have to process an array then put
> the array in the top level struct and pass it in the same user_data
> struct and use another helper to allow the driver to iterate through
> it.

I see. Both VTD and SMMU pass uptr to the arrays of invalidation
commands/requests. The only difference is that SMMU's array is a
ring buffer other than a plain one indexing from the beginning.
But the helper could take two index inputs, which should work for
VTD case too. If another IOMMU driver only supports one request,
rather than a array of requests, we can treat that as a single-
entry array.

Then, the driver-specific data structure will be the array entry
level only.

@Yi,
This seems to be a bigger rework than the top level struct. Along
with Jason's request for fail_nth below, we'd need to bisect the
workload between us, or can just continue each other's daily work.
Let me know which one you prefer.
https://lore.kernel.org/linux-iommu/ZNPCtPTcHvITt6fk@nvidia.com/

Thanks!
Nic