[v2,12/12] iommu: Add helper to set iopf handler for domain

  To avoid open code everywhere.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h | 11 ++++++++++-
 drivers/iommu/iommu.c | 20 ++++++++++++++++++--
 2 files changed, 28 insertions(+), 3 deletions(-)
  

On Thu, Jul 27, 2023 at 01:48:37PM +0800, Lu Baolu wrote:
> To avoid open code everywhere.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  include/linux/iommu.h | 11 ++++++++++-
>  drivers/iommu/iommu.c | 20 ++++++++++++++++++--
>  2 files changed, 28 insertions(+), 3 deletions(-)

Seems like overkill at this point..

Also, I think this is probably upside down.

We want to create the domains as fault enabled in the first place.

A fault enabled domain should never be attached to something that
cannot support faults. It should also not support changing the fault
handler while it exists.

Thus at the creation point would be the time to supply the fault handler
as part of requesting faulting.

Taking an existing domain and making it faulting enabled is going to
be really messy in all the corner cases.

My advice (and Robin will probably hate me), is to define a new op:

struct domain_alloc_paging_args {
       struct fault_handler *fault_handler;
       void *fault_data
};

struct iommu_domain *domain_alloc_paging2(struct device *dev, struct
       domain_alloc_paging_args *args)

The point would be to leave the majority of drivers using the
simplified, core assisted, domain_alloc_paging() interface and they
just don't have to touch any of this stuff at all.

Obviously if handler is given then the domain will be initialized as
faulting.

Jason
  

On 2023/8/11 3:18, Jason Gunthorpe wrote:
> On Thu, Jul 27, 2023 at 01:48:37PM +0800, Lu Baolu wrote:
>> To avoid open code everywhere.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   include/linux/iommu.h | 11 ++++++++++-
>>   drivers/iommu/iommu.c | 20 ++++++++++++++++++--
>>   2 files changed, 28 insertions(+), 3 deletions(-)
> 
> Seems like overkill at this point..
> 
> Also, I think this is probably upside down.
> 
> We want to create the domains as fault enabled in the first place.
> 
> A fault enabled domain should never be attached to something that
> cannot support faults. It should also not support changing the fault
> handler while it exists.
> 
> Thus at the creation point would be the time to supply the fault handler
> as part of requesting faulting.
> 
> Taking an existing domain and making it faulting enabled is going to
> be really messy in all the corner cases.

Yes. Agreed.

> 
> My advice (and Robin will probably hate me), is to define a new op:
> 
> struct domain_alloc_paging_args {
>         struct fault_handler *fault_handler;
>         void *fault_data
> };
> 
> struct iommu_domain *domain_alloc_paging2(struct device *dev, struct
>         domain_alloc_paging_args *args)
> 
> The point would be to leave the majority of drivers using the
> simplified, core assisted, domain_alloc_paging() interface and they
> just don't have to touch any of this stuff at all.
> 
> Obviously if handler is given then the domain will be initialized as
> faulting.

Perhaps we also need an internal helper for iommu drivers to check the
iopf capability of the domain.

Best regards,
baolu
  

On Fri, Aug 11, 2023 at 10:40:15AM +0800, Baolu Lu wrote:
> On 2023/8/11 3:18, Jason Gunthorpe wrote:
> > On Thu, Jul 27, 2023 at 01:48:37PM +0800, Lu Baolu wrote:
> > > To avoid open code everywhere.
> > > 
> > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > > ---
> > >   include/linux/iommu.h | 11 ++++++++++-
> > >   drivers/iommu/iommu.c | 20 ++++++++++++++++++--
> > >   2 files changed, 28 insertions(+), 3 deletions(-)
> > 
> > Seems like overkill at this point..
> > 
> > Also, I think this is probably upside down.
> > 
> > We want to create the domains as fault enabled in the first place.
> > 
> > A fault enabled domain should never be attached to something that
> > cannot support faults. It should also not support changing the fault
> > handler while it exists.
> > 
> > Thus at the creation point would be the time to supply the fault handler
> > as part of requesting faulting.
> > 
> > Taking an existing domain and making it faulting enabled is going to
> > be really messy in all the corner cases.
> 
> Yes. Agreed.
> 
> > 
> > My advice (and Robin will probably hate me), is to define a new op:
> > 
> > struct domain_alloc_paging_args {
> >         struct fault_handler *fault_handler;
> >         void *fault_data
> > };
> > 
> > struct iommu_domain *domain_alloc_paging2(struct device *dev, struct
> >         domain_alloc_paging_args *args)
> > 
> > The point would be to leave the majority of drivers using the
> > simplified, core assisted, domain_alloc_paging() interface and they
> > just don't have to touch any of this stuff at all.
> > 
> > Obviously if handler is given then the domain will be initialized as
> > faulting.
> 
> Perhaps we also need an internal helper for iommu drivers to check the
> iopf capability of the domain.

Yeah, maybe.

I've been mulling over this for a a bit here

Robin suggested to wrap everything in a arg to domain_alloc and build
a giant super multiplexor

I don't really like that because it makes it quite complicated for the
driver and multiplexor APIs are rarely good.

So for simple drivers I like the 'domain_alloc_paging' as the only op
they implement and it is obviously simple and hard to implement
wrong. Most drivers would do this.

We also need a:

struct iommu_domain *domain_alloc_sva(struct device *dev, struct mm_struct *mm)

So SVA can be fully setup at allocation time. SVA doesn't have any
legal permutations so it can be kept simple.

Then we need something to bundle:
 - Dirty tracking yes/no
 - The iommufd user space blob
 - Fault handling yes/no

For complex drivers.

So maybe we should just have a 3rd option

// I'm a complex driver and many people checked that I implemented
// this right:
struct domain_alloc_args {
       struct device *dev;
       unsigned int flags; // For requesting dirty tracking

       // alloc_domain_user interface
       struct iommu_domain *parent;
       void *user_data;
       size_t user_len;

       // Faulting
       struct fault_handler *fault_handler;
       void *fault_data;	
};
struct iommu_domain *domain_alloc(struct domain_alloc_args *args);

?

IDK, multiplexor APIs are rarely good, but maybe this is the right
direction?

Jason