On Thu, Aug 10, 2023 at 07:23:11PM +0100, Joao Martins wrote: > On 19/05/2023 14:29, Jason Gunthorpe wrote: > > On Fri, May 19, 2023 at 12:56:19PM +0100, Joao Martins wrote: > >> On 19/05/2023 12:51, Jason Gunthorpe wrote: > >>> On Fri, May 19, 2023 at 12:47:24PM +0100, Joao Martins wrote: > >>>> In practice it is done as soon after the domain is created but I understand what > >>>> you mean that both should be together; I have this implemented like that as my > >>>> first take as a domain_alloc passed flags, but I was a little undecided because > >>>> we are adding another domain_alloc() op for the user-managed pagetable and after > >>>> having another one we would end up with 3 ways of creating iommu domain -- but > >>>> maybe that's not an issue > >>> > >>> It should ride on the same user domain alloc op as some generic flags, > >> > >> OK, I suppose that makes sense specially with this being tied in HWPT_ALLOC > >> where all this new user domain alloc does. > > > > Yes, it should be easy. > > > > Then do what Robin said and make the domain ops NULL if the user > > didn't ask for dirty tracking and then attach can fail if there are > > domain incompatibility's. > > > > Since alloc_user (or whatever it settles into) will have the struct > > device * argument this should be easy enough with out getting mixed > > with the struct bus cleanup. > > Taking a step back, the iommu domain ops are a shared global pointer in all > iommu domains AFAIU at least in all three iommu implementations I was targetting > with this -- init-ed from iommu_ops::domain_default_ops. Not something we can > "just" clear part of it as that's the same global pointer shared with every > other domain. We would have to duplicate for every vendor two domain ops: one > with dirty and another without dirty tracking; though the general sentiment > behind clearing makes sense Yes, the "domain_default_ops" is basically a transitional hack to help migrate to narrowly defined per-usage domain ops. eg things like blocking and identity should not have mapping ops. Things that don't support dirty tracking should not have dirty tracking ops in the first place. So the simplest version of this is that by default all domain allocations do not support dirty tracking. This ensures maximum cross-instance/device domain re-use. If userspace would like to use dirty tracking it signals it to iommufd, probably using the user domain alloc path. The driver, if it supports it, returns a dirty capable domain with matching dirty enabled ops. A dirty capable domain can only be attached to a device/instance that is compatible and continues to provide dirty tracking. This allows HW that has special restrictions to be properly supported. eg maybe HW can only support dirty on a specific page table format. It can select that format during alloc. > This is a bit simpler and as a bonus it avoids getting dependent on the > domain_alloc_user() nesting infra and no core iommu domain changes; We have to start tackling some of this and not just bodging on top of bodges :\ I think the domain_alloc_user patches are in good enough shape you can rely on them. Return the IOMMU_CAP_DIRTY as generic data in the new GET_INFO Accept some generic flag in the alloc_hwpt requesting dirty Pass generic flags down to the driver. Reject set flags and drivers that don't implement alloc_domain_user. Driver returns a domain with the right ops 'XXX_domain_ops_dirty_paging'. Driver refuses to attach the dirty enabled domain to places that do dirty tracking. Jason
