Re: [PATCH v5 02/25] iommu/sva: Manage process address spaces
On Mon, Apr 20, 2020 at 11:14:37AM -0700, Fenghua Yu wrote: > > Agreed, perhaps Fenghua can consider that in his patchset. It would > > help align life cycles as well. > > https://lkml.org/lkml/2020/3/30/910> > > Seems we depend on each other: my patch defines pasid in mm_struct. > I can free PASID in your detach() function. Looks like this should go into the same series. I also don't see any good reason to have the pasid in the x86-specific context vs the common mm_struct. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 02/25] iommu/sva: Manage process address spaces
On Mon, Apr 20, 2020 at 10:48:50AM -0700, Jacob Pan wrote: > On Mon, 20 Apr 2020 10:57:27 -0300 > Jason Gunthorpe wrote: > > > On Mon, Apr 20, 2020 at 09:42:13AM +0200, Jean-Philippe Brucker wrote: > > > On Thu, Apr 16, 2020 at 05:13:31AM -0700, Christoph Hellwig wrote: > > > > On Thu, Apr 16, 2020 at 10:54:02AM +0200, Jean-Philippe Brucker > > > > wrote: > > > > > On Thu, Apr 16, 2020 at 12:28:52AM -0700, Christoph Hellwig > > > > > wrote: > > > > > > > + rcu_read_lock(); > > > > > > > + hlist_for_each_entry_rcu(bond, _mm->devices, > > > > > > > mm_node) > > > > > > > + io_mm->ops->invalidate(bond->sva.dev, > > > > > > > io_mm->pasid, io_mm->ctx, > > > > > > > +start, end - start); > > > > > > > + rcu_read_unlock(); > > > > > > > +} > > > > > > > > > > > > What is the reason that the devices don't register their own > > > > > > notifiers? This kinds of multiplexing is always rather messy, > > > > > > and you do it for all the methods. > > > > > > > > > > This sends TLB and ATC invalidations through the IOMMU, it > > > > > doesn't go through device drivers > > > > > > > > I don't think we mean the same thing, probably because of my > > > > rather imprecise use of the word device. > > > > > > > > What I mean is that the mmu_notifier should not be embedded into > > > > the io_mm structure (whch btw, seems to have a way to generic > > > > name, just like all other io_* prefixed names), but instead into > > > > the iommu_bond structure. That avoid the whole multiplexing > > > > layer. > > > > > > Right, I can see the appeal. I still like having a single mmu > > > notifier per mm because it ensures we allocate a single PASID per > > > mm (as required by x86). I suppose one alternative is to maintain a > > > hashtable of mm->pasid, to avoid iterating over all bonds during > > > allocation. > > > > I've been getting rid of hash tables like this.. Adding it to the > > mm_struct does seem reasonable, I think PASID is a pretty broad > > concept now. > > > Agreed, perhaps Fenghua can consider that in his patchset. It would > help align life cycles as well. > https://lkml.org/lkml/2020/3/30/910> Seems we depend on each other: my patch defines pasid in mm_struct. I can free PASID in your detach() function. Thanks. -Fenghua ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 02/25] iommu/sva: Manage process address spaces
Am 2020-04-20 um 1:44 p.m. schrieb Jacob Pan: >> The bottom line is, when you allocate a PASID for a context, you want >> to know how small it needs to be for all the devices that want to use >> it. If you make it too big, some device will not be able to use it. >> If you make it too small, you waste precious PASIDs that could be >> used for other contexts that need them. >> > So for AMD, system-wide PASID allocation works with the > restriction/optimization above? > Yes for KFD. On multi-GPU systems we allocate one PASID for the whole process and use it on all GPUs. For AMDGPU graphics contexts, we allocate one PASID for each per-GPU context. But they're allocated from a single global PASID namespace managed by the AMDGPU driver and shared with KFD. So we're wasting PASIDs here, but we are compatible with a single system-wide PASID namespace. Regards, Felix ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 02/25] iommu/sva: Manage process address spaces
On Mon, 20 Apr 2020 10:57:27 -0300 Jason Gunthorpe wrote: > On Mon, Apr 20, 2020 at 09:42:13AM +0200, Jean-Philippe Brucker wrote: > > On Thu, Apr 16, 2020 at 05:13:31AM -0700, Christoph Hellwig wrote: > > > On Thu, Apr 16, 2020 at 10:54:02AM +0200, Jean-Philippe Brucker > > > wrote: > > > > On Thu, Apr 16, 2020 at 12:28:52AM -0700, Christoph Hellwig > > > > wrote: > > > > > > + rcu_read_lock(); > > > > > > + hlist_for_each_entry_rcu(bond, _mm->devices, > > > > > > mm_node) > > > > > > + io_mm->ops->invalidate(bond->sva.dev, > > > > > > io_mm->pasid, io_mm->ctx, > > > > > > + start, end - start); > > > > > > + rcu_read_unlock(); > > > > > > +} > > > > > > > > > > What is the reason that the devices don't register their own > > > > > notifiers? This kinds of multiplexing is always rather messy, > > > > > and you do it for all the methods. > > > > > > > > This sends TLB and ATC invalidations through the IOMMU, it > > > > doesn't go through device drivers > > > > > > I don't think we mean the same thing, probably because of my > > > rather imprecise use of the word device. > > > > > > What I mean is that the mmu_notifier should not be embedded into > > > the io_mm structure (whch btw, seems to have a way to generic > > > name, just like all other io_* prefixed names), but instead into > > > the iommu_bond structure. That avoid the whole multiplexing > > > layer. > > > > Right, I can see the appeal. I still like having a single mmu > > notifier per mm because it ensures we allocate a single PASID per > > mm (as required by x86). I suppose one alternative is to maintain a > > hashtable of mm->pasid, to avoid iterating over all bonds during > > allocation. > > I've been getting rid of hash tables like this.. Adding it to the > mm_struct does seem reasonable, I think PASID is a pretty broad > concept now. > Agreed, perhaps Fenghua can consider that in his patchset. It would help align life cycles as well. https://lkml.org/lkml/2020/3/30/910 > Jason [Jacob Pan] ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 02/25] iommu/sva: Manage process address spaces
On Mon, 20 Apr 2020 11:00:28 -0400 Felix Kuehling wrote: > Am 2020-04-20 um 8:40 a.m. schrieb Christian König: > > Am 20.04.20 um 13:55 schrieb Christoph Hellwig: > >> On Mon, Apr 20, 2020 at 01:44:56PM +0200, Christian König wrote: > >>> Am 20.04.20 um 10:10 schrieb Christoph Hellwig: > On Mon, Apr 20, 2020 at 09:42:13AM +0200, Jean-Philippe Brucker > wrote: > > Right, I can see the appeal. I still like having a single mmu > > notifier per > > mm because it ensures we allocate a single PASID per mm (as > > required by > > x86). I suppose one alternative is to maintain a hashtable of > > mm->pasid, > > to avoid iterating over all bonds during allocation. > Given that the PASID is a pretty generic and important concept > can we just add it directly to the mm_struct and allocate it > lazily once we first need it? > >>> Well the problem is that the PASID might as well be device > >>> specific. E.g. > >>> some devices use 16bit PASIDs, some 15bit, some other only 12bit. > >>> > >>> So what could (at least in theory) happen is that you need to > >>> allocate different PASIDs for the same process because different > >>> devices need one. > >> This directly contradicts the statement from Jean-Philippe above > >> that x86 requires a single PASID per mm_struct. If we may need > >> different PASIDs for different devices and can actually support > >> this just allocating one per [device, mm_struct] would make most > >> sense of me, as it doesn't couple otherwise disjoint state. > > > > Well I'm not an expert on this topic. Felix can probably tell you a > > bit more about that. > > > > Maybe it is sufficient to keep the allocated PASIDs as small as > > possible and return an appropriate error if a device can't deal with > > the allocated number. > > > > If a device can only deal with 12bit PASIDs and more than 2^12 try > > to use it there isn't much else we can do than returning an error > > anyway. > > I'm probably missing some context. But let me try giving a useful > reply. > > The hardware allows you to have different PASIDs for each device > referring to the same address space. But I think it's OK for software > to choose not to do that. If Linux wants to manage one PASID > namespace for all devices, that's a reasonable choice IMO. > On VT-d, system wide PASID namespace is required. Here is a section of the documentation I am working on. Namespaces IOASIDs are limited system resources that default to 20 bits in size. Since each device has its own table, theoretically the namespace can be per device also. However, VT-d also supports shared workqueue and ENQCMD[1] where one IOASID could be used to submit work on multiple devices. This requires IOASID to be system-wide on Intel VT-d platforms. This is also the reason why guest must use emulated virtual command interface to allocate IOASID from the host. On VT-d, storage of IOASID table is at per device while the granularity of assignment is per IOASID. Even though, each guest IOASID must have a backing host IOASID, guest IOASID can be different than its host IOASID. The namespace of guest IOASID is controlled by VMM, which decideds whether identity mapping of G-H IOASIDs is necessary. 1. https://software.intel.com/sites/default/files/managed/c5/15/architecture-instruction-set-extensions-programming-reference.pdf For the per mm_struct PASID question by Christian, we are proposing that in x86 context and a lazy free. https://lkml.org/lkml/2020/3/30/910 > Different devices have different limits for the size of PASID they can > support. For example AMD GPUs support 16-bits but the IOMMU supports > less. So on APUs we use small PASIDs for contexts that want to use > IOMMUv2 to access memory, but bigger PASIDs for contexts that do not. > > I choose the word "context" deliberately, because the amdgpu driver > uses PASIDs even when we're not using IOMMUv2, and we're using them to > identify GPU virtual address spaces. There can be more than one per > process. In practice you can have two, one for graphics (not SVM, > doesn't use IOMMUv2) and one for KFD compute (SVM, can use IOMMUv2 on > APUs). > > Because the IOMMUv2 supports only smaller PASIDs, we want to avoid > exhausting that space with PASID allocations that don't use the > IOMMUv2. So our PASID allocation function has a "size" parameter, and > we try to allocated a PASID as big as possible in order to leave more > precious smaller PASIDs for contexts that need them. > > The bottom line is, when you allocate a PASID for a context, you want > to know how small it needs to be for all the devices that want to use > it. If you make it too big, some device will not be able to use it. > If you make it too small, you waste precious PASIDs that could be > used for other contexts that need them. > So for AMD, system-wide PASID allocation works with the restriction/optimization above? >
Re: [PATCH v5 02/25] iommu/sva: Manage process address spaces
Am 20.04.20 um 13:55 schrieb Christoph Hellwig: On Mon, Apr 20, 2020 at 01:44:56PM +0200, Christian König wrote: Am 20.04.20 um 10:10 schrieb Christoph Hellwig: On Mon, Apr 20, 2020 at 09:42:13AM +0200, Jean-Philippe Brucker wrote: Right, I can see the appeal. I still like having a single mmu notifier per mm because it ensures we allocate a single PASID per mm (as required by x86). I suppose one alternative is to maintain a hashtable of mm->pasid, to avoid iterating over all bonds during allocation. Given that the PASID is a pretty generic and important concept can we just add it directly to the mm_struct and allocate it lazily once we first need it? Well the problem is that the PASID might as well be device specific. E.g. some devices use 16bit PASIDs, some 15bit, some other only 12bit. So what could (at least in theory) happen is that you need to allocate different PASIDs for the same process because different devices need one. This directly contradicts the statement from Jean-Philippe above that x86 requires a single PASID per mm_struct. If we may need different PASIDs for different devices and can actually support this just allocating one per [device, mm_struct] would make most sense of me, as it doesn't couple otherwise disjoint state. Well I'm not an expert on this topic. Felix can probably tell you a bit more about that. Maybe it is sufficient to keep the allocated PASIDs as small as possible and return an appropriate error if a device can't deal with the allocated number. If a device can only deal with 12bit PASIDs and more than 2^12 try to use it there isn't much else we can do than returning an error anyway. Regards, Christian. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 02/25] iommu/sva: Manage process address spaces
Am 2020-04-20 um 8:40 a.m. schrieb Christian König: > Am 20.04.20 um 13:55 schrieb Christoph Hellwig: >> On Mon, Apr 20, 2020 at 01:44:56PM +0200, Christian König wrote: >>> Am 20.04.20 um 10:10 schrieb Christoph Hellwig: On Mon, Apr 20, 2020 at 09:42:13AM +0200, Jean-Philippe Brucker wrote: > Right, I can see the appeal. I still like having a single mmu > notifier per > mm because it ensures we allocate a single PASID per mm (as > required by > x86). I suppose one alternative is to maintain a hashtable of > mm->pasid, > to avoid iterating over all bonds during allocation. Given that the PASID is a pretty generic and important concept can we just add it directly to the mm_struct and allocate it lazily once we first need it? >>> Well the problem is that the PASID might as well be device specific. >>> E.g. >>> some devices use 16bit PASIDs, some 15bit, some other only 12bit. >>> >>> So what could (at least in theory) happen is that you need to allocate >>> different PASIDs for the same process because different devices need >>> one. >> This directly contradicts the statement from Jean-Philippe above that >> x86 requires a single PASID per mm_struct. If we may need different >> PASIDs for different devices and can actually support this just >> allocating one per [device, mm_struct] would make most sense of me, as >> it doesn't couple otherwise disjoint state. > > Well I'm not an expert on this topic. Felix can probably tell you a > bit more about that. > > Maybe it is sufficient to keep the allocated PASIDs as small as > possible and return an appropriate error if a device can't deal with > the allocated number. > > If a device can only deal with 12bit PASIDs and more than 2^12 try to > use it there isn't much else we can do than returning an error anyway. I'm probably missing some context. But let me try giving a useful reply. The hardware allows you to have different PASIDs for each device referring to the same address space. But I think it's OK for software to choose not to do that. If Linux wants to manage one PASID namespace for all devices, that's a reasonable choice IMO. Different devices have different limits for the size of PASID they can support. For example AMD GPUs support 16-bits but the IOMMU supports less. So on APUs we use small PASIDs for contexts that want to use IOMMUv2 to access memory, but bigger PASIDs for contexts that do not. I choose the word "context" deliberately, because the amdgpu driver uses PASIDs even when we're not using IOMMUv2, and we're using them to identify GPU virtual address spaces. There can be more than one per process. In practice you can have two, one for graphics (not SVM, doesn't use IOMMUv2) and one for KFD compute (SVM, can use IOMMUv2 on APUs). Because the IOMMUv2 supports only smaller PASIDs, we want to avoid exhausting that space with PASID allocations that don't use the IOMMUv2. So our PASID allocation function has a "size" parameter, and we try to allocated a PASID as big as possible in order to leave more precious smaller PASIDs for contexts that need them. The bottom line is, when you allocate a PASID for a context, you want to know how small it needs to be for all the devices that want to use it. If you make it too big, some device will not be able to use it. If you make it too small, you waste precious PASIDs that could be used for other contexts that need them. Regards, Felix ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 02/25] iommu/sva: Manage process address spaces
On Mon, Apr 20, 2020 at 09:42:13AM +0200, Jean-Philippe Brucker wrote: > On Thu, Apr 16, 2020 at 05:13:31AM -0700, Christoph Hellwig wrote: > > On Thu, Apr 16, 2020 at 10:54:02AM +0200, Jean-Philippe Brucker wrote: > > > On Thu, Apr 16, 2020 at 12:28:52AM -0700, Christoph Hellwig wrote: > > > > > + rcu_read_lock(); > > > > > + hlist_for_each_entry_rcu(bond, _mm->devices, mm_node) > > > > > + io_mm->ops->invalidate(bond->sva.dev, io_mm->pasid, > > > > > io_mm->ctx, > > > > > +start, end - start); > > > > > + rcu_read_unlock(); > > > > > +} > > > > > > > > What is the reason that the devices don't register their own notifiers? > > > > This kinds of multiplexing is always rather messy, and you do it for > > > > all the methods. > > > > > > This sends TLB and ATC invalidations through the IOMMU, it doesn't go > > > through device drivers > > > > I don't think we mean the same thing, probably because of my rather > > imprecise use of the word device. > > > > What I mean is that the mmu_notifier should not be embedded into the > > io_mm structure (whch btw, seems to have a way to generic name, just > > like all other io_* prefixed names), but instead into the > > iommu_bond structure. That avoid the whole multiplexing layer. > > Right, I can see the appeal. I still like having a single mmu notifier per > mm because it ensures we allocate a single PASID per mm (as required by > x86). I suppose one alternative is to maintain a hashtable of mm->pasid, > to avoid iterating over all bonds during allocation. I've been getting rid of hash tables like this.. Adding it to the mm_struct does seem reasonable, I think PASID is a pretty broad concept now. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 02/25] iommu/sva: Manage process address spaces
Am 20.04.20 um 10:10 schrieb Christoph Hellwig: On Mon, Apr 20, 2020 at 09:42:13AM +0200, Jean-Philippe Brucker wrote: Right, I can see the appeal. I still like having a single mmu notifier per mm because it ensures we allocate a single PASID per mm (as required by x86). I suppose one alternative is to maintain a hashtable of mm->pasid, to avoid iterating over all bonds during allocation. Given that the PASID is a pretty generic and important concept can we just add it directly to the mm_struct and allocate it lazily once we first need it? Well the problem is that the PASID might as well be device specific. E.g. some devices use 16bit PASIDs, some 15bit, some other only 12bit. So what could (at least in theory) happen is that you need to allocate different PASIDs for the same process because different devices need one. Regards, Christian. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 02/25] iommu/sva: Manage process address spaces
On Mon, Apr 20, 2020 at 01:44:56PM +0200, Christian König wrote: > Am 20.04.20 um 10:10 schrieb Christoph Hellwig: > > On Mon, Apr 20, 2020 at 09:42:13AM +0200, Jean-Philippe Brucker wrote: > > > Right, I can see the appeal. I still like having a single mmu notifier per > > > mm because it ensures we allocate a single PASID per mm (as required by > > > x86). I suppose one alternative is to maintain a hashtable of mm->pasid, > > > to avoid iterating over all bonds during allocation. > > Given that the PASID is a pretty generic and important concept can > > we just add it directly to the mm_struct and allocate it lazily once > > we first need it? > > Well the problem is that the PASID might as well be device specific. E.g. > some devices use 16bit PASIDs, some 15bit, some other only 12bit. > > So what could (at least in theory) happen is that you need to allocate > different PASIDs for the same process because different devices need one. This directly contradicts the statement from Jean-Philippe above that x86 requires a single PASID per mm_struct. If we may need different PASIDs for different devices and can actually support this just allocating one per [device, mm_struct] would make most sense of me, as it doesn't couple otherwise disjoint state. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 02/25] iommu/sva: Manage process address spaces
On Mon, Apr 20, 2020 at 09:42:13AM +0200, Jean-Philippe Brucker wrote: > Right, I can see the appeal. I still like having a single mmu notifier per > mm because it ensures we allocate a single PASID per mm (as required by > x86). I suppose one alternative is to maintain a hashtable of mm->pasid, > to avoid iterating over all bonds during allocation. Given that the PASID is a pretty generic and important concept can we just add it directly to the mm_struct and allocate it lazily once we first need it? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 02/25] iommu/sva: Manage process address spaces
On Thu, Apr 16, 2020 at 05:13:31AM -0700, Christoph Hellwig wrote: > On Thu, Apr 16, 2020 at 10:54:02AM +0200, Jean-Philippe Brucker wrote: > > On Thu, Apr 16, 2020 at 12:28:52AM -0700, Christoph Hellwig wrote: > > > > + rcu_read_lock(); > > > > + hlist_for_each_entry_rcu(bond, _mm->devices, mm_node) > > > > + io_mm->ops->invalidate(bond->sva.dev, io_mm->pasid, > > > > io_mm->ctx, > > > > + start, end - start); > > > > + rcu_read_unlock(); > > > > +} > > > > > > What is the reason that the devices don't register their own notifiers? > > > This kinds of multiplexing is always rather messy, and you do it for > > > all the methods. > > > > This sends TLB and ATC invalidations through the IOMMU, it doesn't go > > through device drivers > > I don't think we mean the same thing, probably because of my rather > imprecise use of the word device. > > What I mean is that the mmu_notifier should not be embedded into the > io_mm structure (whch btw, seems to have a way to generic name, just > like all other io_* prefixed names), but instead into the > iommu_bond structure. That avoid the whole multiplexing layer. Right, I can see the appeal. I still like having a single mmu notifier per mm because it ensures we allocate a single PASID per mm (as required by x86). I suppose one alternative is to maintain a hashtable of mm->pasid, to avoid iterating over all bonds during allocation. Thanks, Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 02/25] iommu/sva: Manage process address spaces
On Thu, Apr 16, 2020 at 10:54:02AM +0200, Jean-Philippe Brucker wrote: > On Thu, Apr 16, 2020 at 12:28:52AM -0700, Christoph Hellwig wrote: > > > + rcu_read_lock(); > > > + hlist_for_each_entry_rcu(bond, _mm->devices, mm_node) > > > + io_mm->ops->invalidate(bond->sva.dev, io_mm->pasid, io_mm->ctx, > > > +start, end - start); > > > + rcu_read_unlock(); > > > +} > > > > What is the reason that the devices don't register their own notifiers? > > This kinds of multiplexing is always rather messy, and you do it for > > all the methods. > > This sends TLB and ATC invalidations through the IOMMU, it doesn't go > through device drivers I don't think we mean the same thing, probably because of my rather imprecise use of the word device. What I mean is that the mmu_notifier should not be embedded into the io_mm structure (whch btw, seems to have a way to generic name, just like all other io_* prefixed names), but instead into the iommu_bond structure. That avoid the whole multiplexing layer. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 02/25] iommu/sva: Manage process address spaces
On Thu, Apr 16, 2020 at 12:28:52AM -0700, Christoph Hellwig wrote: > > + rcu_read_lock(); > > + hlist_for_each_entry_rcu(bond, _mm->devices, mm_node) > > + io_mm->ops->invalidate(bond->sva.dev, io_mm->pasid, io_mm->ctx, > > + start, end - start); > > + rcu_read_unlock(); > > +} > > What is the reason that the devices don't register their own notifiers? > This kinds of multiplexing is always rather messy, and you do it for > all the methods. This sends TLB and ATC invalidations through the IOMMU, it doesn't go through device drivers Thanks, Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 02/25] iommu/sva: Manage process address spaces
> + rcu_read_lock(); > + hlist_for_each_entry_rcu(bond, _mm->devices, mm_node) > + io_mm->ops->invalidate(bond->sva.dev, io_mm->pasid, io_mm->ctx, > +start, end - start); > + rcu_read_unlock(); > +} What is the reason that the devices don't register their own notifiers? This kinds of multiplexing is always rather messy, and you do it for all the methods. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
