On 10/08/2023 19:55, Jason Gunthorpe wrote: > 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. > My earlier point was more about not what 'domain_default_ops' represents but that it's a pointer. Shared by everyone (devices and domains alike). But you sort of made it clear that it's OK to duplicate it to not have dirty tracking. The duplication is what I felt a little odd. > 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 wasn't quite bodging, just trying to parallelize what was bus cleanup could be tackling domain/device-independent ops without them being global. Maybe I read too much into it hence my previous question. > I think the domain_alloc_user patches are in good enough shape you can > rely on them. > I have been using them. Just needs a flags arg and I have a alternative to the snip I pasted earlier with domain_alloc_user already there > Return the IOMMU_CAP_DIRTY as generic data in the new GET_INFO I have this one here: https://lore.kernel.org/linux-iommu/20230518204650.14541-14-joao.m.martins@xxxxxxxxxx/ I can rework to GET_HW_INFO but it really needs to be generic bits of data and not iommu hw specific e.g. that translates into device_iommu_capable() cap checking. The moment it stays hw specific and having userspace to decode what different hw specific bits for general simple capability checking (e.g. do we support nesting, do we support dirty tracking, etc), it breaks the point of a generic API. With exception I guess of going into the weeds of nesting specific formats as there's hardware formats intimate to the userspace space process emulation for iommu model specific thing i.e. things that can't be made generic. Hopefully my expectation here matches yours for cap checking? > 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 refuses to attach the dirty enabled domain to places that do > dirty tracking. > This is already done, and so far I have an unsigned long flags to domain_alloc_user() and probably be kept defining it as iommu-domain-feature bit (unless it's better to follow similar direction as hwpt_type like in domain_alloc_user). And gets stored as iommu_domain::flags, like this series had. Though if majority of driver rejection flows via alloc_domain_user only (which has a struct device), perhaps it's not even needed to store as a new iommu_domain::flags > Driver returns a domain with the right ops 'XXX_domain_ops_dirty_paging'. alright, you look OK with this -- I'll go with it then
