RE: [PATCH 0/5] Share sva domains with all devices bound to a mm

Tian, Kevin posted 5 patches 2 years, 1 month ago
Only 0 patches received!
There is a newer version of this series
RE: [PATCH 0/5] Share sva domains with all devices bound to a mm
Posted by Tian, Kevin 2 years, 1 month ago
> From: Zhang, Tina <tina.zhang@intel.com>
> Sent: Tuesday, August 8, 2023 3:50 PM
> 
> A sva domain's lifetime begins with binding a device to a mm and ends
> by releasing all the bound devices from that sva domain. Technically,
> there could be more than one sva domain identified by the mm PASID for
> the use of bound devices issuing DMA transactions.

Could you elaborate it with some concrete examples which motivate
this change?

> 
> To support mm PASID 1:n with sva domains, each mm needs to keep both a
> reference list of allocated sva domains and the corresponding PASID.
> However, currently, mm struct only has one pasid field for sva usage,
> which is used to keep the info of an assigned PASID. That pasid field
> cannot provide sufficient info to build up the 1:n mapping between PASID
> and sva domains.
> 
> This patch-set fills the gap by adding an mm_iommu field[1], whose type is
> mm_iommu_data struct, to replace the old pasid field. The introduced
> mm_iommu_data struct keeps info of both a reference list of sva domains
> and an assigned PASID.
> 
> 
> [1]: https://lore.kernel.org/linux-iommu/ZIBxPd1%2FJCAle6yP@nvidia.com/
> 
> 
> The RFC version of this patch-set is here:
> https://lore.kernel.org/linux-iommu/20230707013441.365583-1-
> tina.zhang@intel.com/
> 
> Tina Zhang (5):
>   iommu: Add mm_get_pasid() helper function
>   iommu: Call helper function to get assigned pasid value
>   mm: Add structure to keep sva information
>   iommu: Support mm PASID 1:n with sva domains
>   mm: Deprecate pasid field
> 
>  arch/x86/kernel/traps.c                       |  2 +-
>  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 12 ++---
>  drivers/iommu/intel/svm.c                     |  8 +--
>  drivers/iommu/iommu-sva.c                     | 50 ++++++++++++-------
>  include/linux/iommu.h                         | 19 +++++--
>  include/linux/mm_types.h                      |  3 +-
>  kernel/fork.c                                 |  1 -
>  mm/init-mm.c                                  |  3 --
>  8 files changed, 58 insertions(+), 40 deletions(-)
> 
> --
> 2.17.1
Re: [PATCH 0/5] Share sva domains with all devices bound to a mm
Posted by Tina Zhang 2 years, 1 month ago
Hi,

On 8/9/23 17:41, Tian, Kevin wrote:
>> From: Zhang, Tina <tina.zhang@intel.com>
>> Sent: Tuesday, August 8, 2023 3:50 PM
>>
>> A sva domain's lifetime begins with binding a device to a mm and ends
>> by releasing all the bound devices from that sva domain. Technically,
>> there could be more than one sva domain identified by the mm PASID for
>> the use of bound devices issuing DMA transactions.
> 
> Could you elaborate it with some concrete examples which motivate
> this change?
The motivation is to remove the superfluous IOTLB invalidation in 
current VT-d driver.

Currently, in VT-d driver, due to lacking shared sva domain info, in 
intel_flush_svm_range(), both iotlb and dev-tlb invalidation operations 
are performed per-device. However, difference devices could be behind 
one IOMMU (e.g., four devices are behind one IOMMU) and invoking iotlb 
per-device gives us more iotlb invalidation than necessary (4 iotlb 
invalidation instead of 1). This issue may give more performance impact 
when in a virtual machine guest, as currently we have one virtual VT-d 
for in front of those virtual devices.


This patch fixes this issue by attaching shared sva domain information 
to mm, so that it can be utilized in the mm_notifier_ops callbacks.

Regards,
-Tina

> 
>>
>> To support mm PASID 1:n with sva domains, each mm needs to keep both a
>> reference list of allocated sva domains and the corresponding PASID.
>> However, currently, mm struct only has one pasid field for sva usage,
>> which is used to keep the info of an assigned PASID. That pasid field
>> cannot provide sufficient info to build up the 1:n mapping between PASID
>> and sva domains.
>>
>> This patch-set fills the gap by adding an mm_iommu field[1], whose type is
>> mm_iommu_data struct, to replace the old pasid field. The introduced
>> mm_iommu_data struct keeps info of both a reference list of sva domains
>> and an assigned PASID.
>>
>>
>> [1]: https://lore.kernel.org/linux-iommu/ZIBxPd1%2FJCAle6yP@nvidia.com/
>>
>>
>> The RFC version of this patch-set is here:
>> https://lore.kernel.org/linux-iommu/20230707013441.365583-1-
>> tina.zhang@intel.com/
>>
>> Tina Zhang (5):
>>    iommu: Add mm_get_pasid() helper function
>>    iommu: Call helper function to get assigned pasid value
>>    mm: Add structure to keep sva information
>>    iommu: Support mm PASID 1:n with sva domains
>>    mm: Deprecate pasid field
>>
>>   arch/x86/kernel/traps.c                       |  2 +-
>>   .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 12 ++---
>>   drivers/iommu/intel/svm.c                     |  8 +--
>>   drivers/iommu/iommu-sva.c                     | 50 ++++++++++++-------
>>   include/linux/iommu.h                         | 19 +++++--
>>   include/linux/mm_types.h                      |  3 +-
>>   kernel/fork.c                                 |  1 -
>>   mm/init-mm.c                                  |  3 --
>>   8 files changed, 58 insertions(+), 40 deletions(-)
>>
>> --
>> 2.17.1
>
RE: [PATCH 0/5] Share sva domains with all devices bound to a mm
Posted by Tian, Kevin 2 years, 1 month ago
> From: Zhang, Tina <tina.zhang@intel.com>
> Sent: Thursday, August 10, 2023 9:32 AM
> 
> Hi,
> 
> On 8/9/23 17:41, Tian, Kevin wrote:
> >> From: Zhang, Tina <tina.zhang@intel.com>
> >> Sent: Tuesday, August 8, 2023 3:50 PM
> >>
> >> A sva domain's lifetime begins with binding a device to a mm and ends
> >> by releasing all the bound devices from that sva domain. Technically,
> >> there could be more than one sva domain identified by the mm PASID for
> >> the use of bound devices issuing DMA transactions.
> >
> > Could you elaborate it with some concrete examples which motivate
> > this change?
> The motivation is to remove the superfluous IOTLB invalidation in
> current VT-d driver.
> 
> Currently, in VT-d driver, due to lacking shared sva domain info, in
> intel_flush_svm_range(), both iotlb and dev-tlb invalidation operations
> are performed per-device. However, difference devices could be behind
> one IOMMU (e.g., four devices are behind one IOMMU) and invoking iotlb
> per-device gives us more iotlb invalidation than necessary (4 iotlb
> invalidation instead of 1). This issue may give more performance impact
> when in a virtual machine guest, as currently we have one virtual VT-d
> for in front of those virtual devices.
> 
> 
> This patch fixes this issue by attaching shared sva domain information
> to mm, so that it can be utilized in the mm_notifier_ops callbacks.
> 

that is one of the motivations. e.g. another one as Jason suggested
is to cleanup to decouple the common sva logic from enqcmd. Both
should be mentioned in next version cover letter.
Re: [PATCH 0/5] Share sva domains with all devices bound to a mm
Posted by Tina Zhang 2 years, 1 month ago
Hi,

On 8/10/23 15:49, Tian, Kevin wrote:
>> From: Zhang, Tina <tina.zhang@intel.com>
>> Sent: Thursday, August 10, 2023 9:32 AM
>>
>> Hi,
>>
>> On 8/9/23 17:41, Tian, Kevin wrote:
>>>> From: Zhang, Tina <tina.zhang@intel.com>
>>>> Sent: Tuesday, August 8, 2023 3:50 PM
>>>>
>>>> A sva domain's lifetime begins with binding a device to a mm and ends
>>>> by releasing all the bound devices from that sva domain. Technically,
>>>> there could be more than one sva domain identified by the mm PASID for
>>>> the use of bound devices issuing DMA transactions.
>>>
>>> Could you elaborate it with some concrete examples which motivate
>>> this change?
>> The motivation is to remove the superfluous IOTLB invalidation in
>> current VT-d driver.
>>
>> Currently, in VT-d driver, due to lacking shared sva domain info, in
>> intel_flush_svm_range(), both iotlb and dev-tlb invalidation operations
>> are performed per-device. However, difference devices could be behind
>> one IOMMU (e.g., four devices are behind one IOMMU) and invoking iotlb
>> per-device gives us more iotlb invalidation than necessary (4 iotlb
>> invalidation instead of 1). This issue may give more performance impact
>> when in a virtual machine guest, as currently we have one virtual VT-d
>> for in front of those virtual devices.
>>
>>
>> This patch fixes this issue by attaching shared sva domain information
>> to mm, so that it can be utilized in the mm_notifier_ops callbacks.
>>
> 
> that is one of the motivations. e.g. another one as Jason suggested
> is to cleanup to decouple the common sva logic from enqcmd. Both
> should be mentioned in next version cover letter.
Right.

Regards,
-Tina
Re: [PATCH 0/5] Share sva domains with all devices bound to a mm
Posted by Jason Gunthorpe 2 years, 1 month ago
On Thu, Aug 10, 2023 at 07:49:11AM +0000, Tian, Kevin wrote:
> > From: Zhang, Tina <tina.zhang@intel.com>
> > Sent: Thursday, August 10, 2023 9:32 AM
> > 
> > Hi,
> > 
> > On 8/9/23 17:41, Tian, Kevin wrote:
> > >> From: Zhang, Tina <tina.zhang@intel.com>
> > >> Sent: Tuesday, August 8, 2023 3:50 PM
> > >>
> > >> A sva domain's lifetime begins with binding a device to a mm and ends
> > >> by releasing all the bound devices from that sva domain. Technically,
> > >> there could be more than one sva domain identified by the mm PASID for
> > >> the use of bound devices issuing DMA transactions.
> > >
> > > Could you elaborate it with some concrete examples which motivate
> > > this change?
> > The motivation is to remove the superfluous IOTLB invalidation in
> > current VT-d driver.
> > 
> > Currently, in VT-d driver, due to lacking shared sva domain info, in
> > intel_flush_svm_range(), both iotlb and dev-tlb invalidation operations
> > are performed per-device. However, difference devices could be behind
> > one IOMMU (e.g., four devices are behind one IOMMU) and invoking iotlb
> > per-device gives us more iotlb invalidation than necessary (4 iotlb
> > invalidation instead of 1). This issue may give more performance impact
> > when in a virtual machine guest, as currently we have one virtual VT-d
> > for in front of those virtual devices.
> > 
> > 
> > This patch fixes this issue by attaching shared sva domain information
> > to mm, so that it can be utilized in the mm_notifier_ops callbacks.
> > 
> 
> that is one of the motivations. e.g. another one as Jason suggested
> is to cleanup to decouple the common sva logic from enqcmd. Both
> should be mentioned in next version cover letter.

I also want to purge all the de-duplication and refcounting code
around mm's and sva_binds from the drivers. Eg see the mess this makes
of SMMUv3.

Core code provides a single iommu_domain per-mm for SVA. Driver can
rely on this optimization and does not need to de-duplicate.

Single domain tracks all attachments. Driver can optimize using that
information by de-duplicating (eg ASID invalidation vs ATC
invalidation)

After this we need to fix the domain allocation op to add a
'alloc_domain_sva(dev, mm_struct)' op so that the drivers can setup
their SVA domains fully in a nice lock-safe environment.

Jason
Re: [PATCH 0/5] Share sva domains with all devices bound to a mm
Posted by Tina Zhang 2 years, 1 month ago
Hi,

On 8/11/23 00:27, Jason Gunthorpe wrote:
> On Thu, Aug 10, 2023 at 07:49:11AM +0000, Tian, Kevin wrote:
>>> From: Zhang, Tina <tina.zhang@intel.com>
>>> Sent: Thursday, August 10, 2023 9:32 AM
>>>
>>> Hi,
>>>
>>> On 8/9/23 17:41, Tian, Kevin wrote:
>>>>> From: Zhang, Tina <tina.zhang@intel.com>
>>>>> Sent: Tuesday, August 8, 2023 3:50 PM
>>>>>
>>>>> A sva domain's lifetime begins with binding a device to a mm and ends
>>>>> by releasing all the bound devices from that sva domain. Technically,
>>>>> there could be more than one sva domain identified by the mm PASID for
>>>>> the use of bound devices issuing DMA transactions.
>>>>
>>>> Could you elaborate it with some concrete examples which motivate
>>>> this change?
>>> The motivation is to remove the superfluous IOTLB invalidation in
>>> current VT-d driver.
>>>
>>> Currently, in VT-d driver, due to lacking shared sva domain info, in
>>> intel_flush_svm_range(), both iotlb and dev-tlb invalidation operations
>>> are performed per-device. However, difference devices could be behind
>>> one IOMMU (e.g., four devices are behind one IOMMU) and invoking iotlb
>>> per-device gives us more iotlb invalidation than necessary (4 iotlb
>>> invalidation instead of 1). This issue may give more performance impact
>>> when in a virtual machine guest, as currently we have one virtual VT-d
>>> for in front of those virtual devices.
>>>
>>>
>>> This patch fixes this issue by attaching shared sva domain information
>>> to mm, so that it can be utilized in the mm_notifier_ops callbacks.
>>>
>>
>> that is one of the motivations. e.g. another one as Jason suggested
>> is to cleanup to decouple the common sva logic from enqcmd. Both
>> should be mentioned in next version cover letter.
> 
> I also want to purge all the de-duplication and refcounting code
> around mm's and sva_binds from the drivers. Eg see the mess this makes
> of SMMUv3.
> 
> Core code provides a single iommu_domain per-mm for SVA. Driver can
> rely on this optimization and does not need to de-duplicate.
> 
> Single domain tracks all attachments. Driver can optimize using that
> information by de-duplicating (eg ASID invalidation vs ATC
> invalidation)
> 
> After this we need to fix the domain allocation op to add a
> 'alloc_domain_sva(dev, mm_struct)' op so that the drivers can setup
> their SVA domains fully in a nice lock-safe environment.
Agree. These can be added to the to-do list.

Regards,
-Tina
> 
> Jason