Skip to contentSkip to conversation

Enable PASID for DMA API users

PATCH In review Jacob Pan 5 participants 35 messages May 18, 2022 – Jun 1, 2022

Subsystems (5)
dmaengine IOMMU Intel IOMMU (VT-d) IOMMU DMA-API layer +1 more subsystems
lore
Share & tools
lore

Jacob Pan

author
#0 lore raw

v4 cover Enable PASID for DMA API users

Some modern accelerators such as Intel's Data Streaming Accelerator (DSA) require PASID in DMA requests to be operational. Specifically, the work submissions with ENQCMD on shared work queues require PASIDs. The use cases include both user DMA with shared virtual addressing (SVA) and in-kernel DMA similar to legacy DMA w/o PASID. Here we address the latter.

DMA mapping API is the de facto standard for in-kernel DMA. However, it operates on a per device or Requester ID(RID) basis which is not PASID-aware. To leverage DMA API for devices relies on PASIDs, this patchset introduces the following APIs

1. A driver facing API that enables DMA API PASID usage:
iommu_attach_dma_pasid(struct device *dev, ioasid_t &pasid);
2. VT-d driver default domain op that allows attaching device-domain-PASID

Once PASID DMA is enabled and attached to the appropriate IOMMU domain, device drivers can continue to use DMA APIs as-is. There is no difference in terms of mapping in dma_handle between without PASID and with PASID. The DMA mapping performed by IOMMU will be identical for both requests, let it be IOVA or PA in case of pass-through.

In addition, this set converts DSA driver in-kernel DMA with PASID from SVA lib to DMA API. There have been security and functional issues with the kernel SVA approach:
(https://lore.kernel.org/linux-iommu/[email protected]/)
The highlights are as the following:
- The lack of IOTLB synchronization upon kernel page table updates. (vmalloc, module/BPF loading, CONFIG_DEBUG_PAGEALLOC etc.)
- Other than slight more protection, using kernel virtual address (KVA)
has little advantage over physical address. There are also no use cases yet where DMA engines need kernel virtual addresses for in-kernel DMA.

Subsequently, cleanup is done around the usage of sva_bind_device() for in-kernel DMA. Removing special casing code in VT-d driver and tightening SVA lib API.

This work and idea behind it is a collaboration with many people, many thanks to Baolu Lu, Jason Gunthorpe, Dave Jiang, and others.

ChangeLog:
v4
- Rebased on "Baolu's SVA and IOPF refactoring" series v6.
(https://github.com/LuBaolu/intel-iommu/commits/iommu-sva-refactoring-v6)
- Fixed locking for protecting iommu domain PASID data
- Use iommu_attach_device_pasid() API instead of calling domain ops directly. This will leverage the common pasid_array that replaces driver specific storage in device_domain_info.
- Added a helper function to do look up in pasid_array from domain

v3
- Rebased on "Baolu's SVA and IOPF refactoring" series v5.
(https://github.com/LuBaolu/intel-iommu/commits/iommu-sva-refactoring-v5)
This version is significantly simplified by leveraging IOMMU domain ops, attach_dev_pasid() op is implemented differently on a DMA domain than on a SVA domain.
We currently have no need to support multiple PASIDs per DMA domain.
(https://lore.kernel.org/lkml/[email protected]/).
Removed PASID-device list from V2, a PASID field is introduced to struct iommu_domain instead. It is intended for DMA requests with PASID by all devices attached to the domain.

v2
- Do not reserve a special PASID for DMA API usage. Use IOASID allocation instead.
- Introduced a generic device-pasid-domain attachment IOMMU op. Replaced the DMA API only IOMMU op.
- Removed supervisor SVA support in VT-d
- Removed unused sva_bind_device parameters
- Use IOMMU specific data instead of struct device to store PASID info

*** SUBJECT HERE ***

*** BLURB HERE ***

Jacob Pan (6):
iommu: Add a per domain PASID for DMA API
iommu: Add a helper to do PASID lookup from domain iommu/vt-d: Implement domain ops for attach_dev_pasid
iommu: Add PASID support for DMA mapping API users
dmaengine: idxd: Use DMA API for in-kernel DMA with PASID iommu/vt-d: Delete unused SVM flag

 drivers/dma/idxd/idxd.h     |   1 -
 drivers/dma/idxd/init.c     |  34 +++--------
 drivers/dma/idxd/sysfs.c    |   7 ---
 drivers/iommu/dma-iommu.c   | 114 ++++++++++++++++++++++++++++++++++++
 drivers/iommu/intel/iommu.c |  72 ++++++++++++++++++++++-
 drivers/iommu/intel/svm.c   |   2 +-
 drivers/iommu/iommu.c       |  22 +++++++
 include/linux/dma-iommu.h   |   3 +
 include/linux/intel-svm.h   |  13 ----
 include/linux/iommu.h       |   8 ++-
 10 files changed, 226 insertions(+), 50 deletions(-)
Signature · 1 line
2.25.1
author
#1 lore raw

PATCH v4 1/6 iommu: Add a per domain PASID for DMA API

DMA requests tagged with PASID can target individual IOMMU domains. Introduce a domain-wide PASID for DMA API, it will be used on the same mapping as legacy DMA without PASID. Let it be IOVA or PA in case of identity domain.

Signed-off-by Jacob Pan <[email protected]>
---
 include/linux/iommu.h | 2 ++
 1 file changed, 2 insertions(+)
include/linux/iommu.h +2 -0
index 9405034e3013..36ad007084cc 100644
@@ -106,6 +106,8 @@ struct iommu_domain {
 	enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
 						      void *data);
 	void *fault_data;
+	ioasid_t dma_pasid;		/* Used for DMA requests with PASID */
+	atomic_t dma_pasid_users;
 };
 
 static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
Signature · 6 lines
2.25.1

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
maintainer
#2 lore raw

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

Quoted 15 lines · Jacob Pan

DMA requests tagged with PASID can target individual IOMMU domains. Introduce a domain-wide PASID for DMA API, it will be used on the same mapping as legacy DMA without PASID. Let it be IOVA or PA in case of identity domain.

Signed-off-by Jacob Pan <[email protected]>
---
  include/linux/iommu.h | 2 ++
  1 file changed, 2 insertions(+)
include/linux/iommu.h +0 -0
index 9405034e3013..36ad007084cc 100644

include/linux/iommu.h struct iommu_domain { · line 109

Quoted 4 lines
include/linux/iommu.h +1 -0
@@ -106,6 +106,8 @@ struct iommu_domain {
  	enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
  						      void *data);
  	void *fault_data;
+	ioasid_t dma_pasid;		/* Used for DMA requests with PASID */

This looks more suitable for iommu_dma_cookie?

include/linux/iommu.h struct iommu_domain { · line 113

Quoted 4 lines
include/linux/iommu.h +1 -0
+	atomic_t dma_pasid_users;
  };
 
  static inline bool iommu_is_dma_domain(struct iommu_domain *domain)

Best regards,
baolu

#3 lore raw

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

In reply to Jacob Pan
Quoted 4 lines · Jacob Pan

DMA requests tagged with PASID can target individual IOMMU domains. Introduce a domain-wide PASID for DMA API, it will be used on the same mapping as legacy DMA without PASID. Let it be IOVA or PA in case of identity domain.

Huh? I can't understand what this is trying to say or why this patch makes sense.

We really should not have pasid's like this attached to the domains..

Jason
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

author
#4 lore raw

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

Hi Jason,

Quoted 11 lines · Jason Gunthorpe
Quoted 4 lines · Jacob Pan

DMA requests tagged with PASID can target individual IOMMU domains. Introduce a domain-wide PASID for DMA API, it will be used on the same mapping as legacy DMA without PASID. Let it be IOVA or PA in case of identity domain.

Huh? I can't understand what this is trying to say or why this patch makes sense.

We really should not have pasid's like this attached to the domains..

This is the same "DMA API global PASID" you reviewed in v3, I just singled it out as a standalone patch and renamed it. Here is your previous review comment.

include/linux/iommu.h struct iommu_domain { · line 109

Quoted 7 lines

+++ b/include/linux/iommu.h

include/linux/iommu.h +2 -0
@@ -105,6 +105,8 @@ struct iommu_domain {
 	enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
 						      void *data);
 	void *fault_data;
+	ioasid_t pasid;		/* Used for DMA requests with PASID */
+	atomic_t pasid_users;

These are poorly named, this is really the DMA API global PASID and shouldn't be used for other things.

Perhaps I misunderstood, do you mind explaining more?

Thanks,

Jacob

#5 lore raw

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

Quoted 32 lines · Jacob Pan

Hi Jason,

Quoted 11 lines · Jason Gunthorpe
Quoted 4 lines · Jacob Pan

DMA requests tagged with PASID can target individual IOMMU domains. Introduce a domain-wide PASID for DMA API, it will be used on the same mapping as legacy DMA without PASID. Let it be IOVA or PA in case of identity domain.

Huh? I can't understand what this is trying to say or why this patch makes sense.

We really should not have pasid's like this attached to the domains..

This is the same "DMA API global PASID" you reviewed in v3, I just singled it out as a standalone patch and renamed it. Here is your previous review comment.

include/linux/iommu.h struct iommu_domain { · line 109

Quoted 7 lines

+++ b/include/linux/iommu.h

include/linux/iommu.h +2 -0
@@ -105,6 +105,8 @@ struct iommu_domain {
 	enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
 						      void *data);
 	void *fault_data;
+	ioasid_t pasid;		/* Used for DMA requests with PASID */
+	atomic_t pasid_users;

These are poorly named, this is really the DMA API global PASID and shouldn't be used for other things.

Quoted 1 line

Perhaps I misunderstood, do you mind explaining more?

You still haven't really explained what this is for in this patch, maybe it just needs a better commit message, or maybe something is wrong.

I keep saying the DMA API usage is not special, so why do we need to create a new global pasid and refcount? Realistically this is only going to be used by IDXD, why can't we just allocate a PASID and return it to the driver every time a driver asks for DMA API on PASI mode? Why does the core need to do anything special?

Jason

#6 lore raw

RE: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

Quoted 45 lines

From: Jason Gunthorpe <[email protected]>
Sent: Monday, May 30, 2022 8:23 PM

Quoted 3 lines · Jacob Pan

Hi Jason,

On Tue, 24 May 2022 10:50:34 -0300, Jason Gunthorpe <[email protected]>

Quoted 4 lines · wrote:
Quoted 4 lines · Jacob Pan
Quoted 2 lines

DMA requests tagged with PASID can target individual IOMMU domains. Introduce a domain-wide PASID for DMA API, it will be used on the

same

Quoted 8 lines
Quoted 8 lines
Quoted 2 lines

mapping as legacy DMA without PASID. Let it be IOVA or PA in case of identity domain.

Huh? I can't understand what this is trying to say or why this patch makes sense.

We really should not have pasid's like this attached to the domains..

Quoted 7 lines

This is the same "DMA API global PASID" you reviewed in v3, I just singled it out as a standalone patch and renamed it. Here is your previous review comment.

Review struct iommu_domain { · line 105

Quoted 3 lines

+++ b/include/linux/iommu.h

(quoted diff) +0 -0
@@ -105,6 +105,8 @@ struct iommu_domain {
 	enum iommu_page_response_code (*iopf_handler)(struct

iommu_fault *fault,

Quoted 11 lines

include/linux/iommu.h struct iommu_domain {

Quoted 4 lines
include/linux/iommu.h +2 -0
 						      void *data);
 	void *fault_data;
+	ioasid_t pasid;		/* Used for DMA requests with PASID */
+	atomic_t pasid_users;

These are poorly named, this is really the DMA API global PASID and shouldn't be used for other things.

Perhaps I misunderstood, do you mind explaining more?

You still haven't really explained what this is for in this patch, maybe it just needs a better commit message, or maybe something is wrong.

Quoted 6 lines

I keep saying the DMA API usage is not special, so why do we need to create a new global pasid and refcount? Realistically this is only going to be used by IDXD, why can't we just allocate a PASID and return it to the driver every time a driver asks for DMA API on PASI mode? Why does the core need to do anything special?

Agree. I guess it was a mistake caused by treating ENQCMD as the only user although the actual semantics of the invented interfaces have already evolved to be quite general.

This is very similar to what we have been discussing for iommufd. a PASID is just an additional routing info when attaching a device to an I/O address space (DMA API in this context) and by default it should be a per-device resource except when ENQCMD is explicitly opt in.

Hence it's right time for us to develop common facility working for both this DMA API usage and iommufd, i.e.:

for normal PASID attach to a domain, driver:

	allocates a local pasid from device local space;
	attaches the local pasid to a domain;

for PASID attach in particular for ENQCMD, driver:

	allocates a global pasid in system-wide;
	attaches the global pasid to a domain;
	set the global pasid in PASID_MSR;

In both cases the pasid is stored in the attach data instead of the domain.

DMA API pasid is no special from above except it needs to allow one device attached to the same domain twice (one with RID and the other with RID+PASID).

for iommufd those operations are initiated by userspace via iommufd uAPI.

Thanks
Kevin

maintainer
#7 lore raw

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

Quoted 51 lines · Kevin
Quoted 18 lines
Quoted 3 lines

Review struct iommu_domain { · line 105

Quoted 3 lines

+++ b/include/linux/iommu.h

(quoted diff) +0 -0
@@ -105,6 +105,8 @@ struct iommu_domain {
  	enum iommu_page_response_code (*iopf_handler)(struct

iommu_fault *fault,

Quoted 10 lines

include/linux/iommu.h struct iommu_domain {

Quoted 4 lines
include/linux/iommu.h +2 -0
  						      void *data);
  	void *fault_data;
+	ioasid_t pasid;		/* Used for DMA requests with PASID */
+	atomic_t pasid_users;

These are poorly named, this is really the DMA API global PASID and shouldn't be used for other things.

Perhaps I misunderstood, do you mind explaining more?

You still haven't really explained what this is for in this patch, maybe it just needs a better commit message, or maybe something is wrong.

Quoted 6 lines

I keep saying the DMA API usage is not special, so why do we need to create a new global pasid and refcount? Realistically this is only going to be used by IDXD, why can't we just allocate a PASID and return it to the driver every time a driver asks for DMA API on PASI mode? Why does the core need to do anything special?

Agree. I guess it was a mistake caused by treating ENQCMD as the only user although the actual semantics of the invented interfaces have already evolved to be quite general.

This is very similar to what we have been discussing for iommufd. a PASID is just an additional routing info when attaching a device to an I/O address space (DMA API in this context) and by default it should be a per-device resource except when ENQCMD is explicitly opt in.

Hence it's right time for us to develop common facility working for both this DMA API usage and iommufd, i.e.:

for normal PASID attach to a domain, driver:

	allocates a local pasid from device local space;
	attaches the local pasid to a domain;

for PASID attach in particular for ENQCMD, driver:

	allocates a global pasid in system-wide;
	attaches the global pasid to a domain;
	set the global pasid in PASID_MSR;

In both cases the pasid is stored in the attach data instead of the domain.

Quoted 6 lines

DMA API pasid is no special from above except it needs to allow one device attached to the same domain twice (one with RID and the other with RID+PASID).

for iommufd those operations are initiated by userspace via iommufd uAPI.

My understanding is that device driver owns its PASID policy. If ENQCMD is supported on the device, the PASIDs should be allocated through ioasid_alloc(). Otherwise, the whole PASID pool is managed by the device driver.

For kernel DMA w/ PASID, after the driver has a PASID for this purpose, it can just set the default domain to the PASID on device. There's no need for enable/disable() interfaces.

Best regards,
baolu

#8 lore raw

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

Quoted 1 line · Baolu Lu

My understanding is that device driver owns its PASID policy.

I'm not sure this is actually useful, the core code should provide the allocator as I can't think of any device that actually needs a special allocator.

Quoted 3 lines

If ENQCMD is supported on the device, the PASIDs should be allocated through ioasid_alloc(). Otherwise, the whole PASID pool is managed by the device driver.

This is OK only if we know in-advance that the device needs a global PASID. ENQCMD is one reason, maybe there are others down the road.

Better to make this core code policy.

Jason

author
#9 lore raw

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

In reply to Baolu Lu

Hi Baolu,

Quoted 20 lines · Baolu Lu
Quoted 19 lines · Kevin
Quoted 19 lines
Quoted 3 lines

Review struct iommu_domain { · line 105

Quoted 3 lines

+++ b/include/linux/iommu.h

(quoted diff) +0 -0
@@ -105,6 +105,8 @@ struct iommu_domain {
  	enum iommu_page_response_code (*iopf_handler)(struct

iommu_fault *fault,

Quoted 11 lines

Review

Quoted 5 lines
include/linux/iommu.h +1 -0
  						      void *data);
  	void *fault_data;
+	ioasid_t pasid;		/* Used for DMA requests

with PASID */

(quoted diff) +1 -0
+	atomic_t pasid_users;

These are poorly named, this is really the DMA API global PASID and shouldn't be used for other things.

Perhaps I misunderstood, do you mind explaining more?

You still haven't really explained what this is for in this patch, maybe it just needs a better commit message, or maybe something is wrong.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

I keep saying the DMA API usage is not special, so why do we need to create a new global pasid and refcount? Realistically this is only going to be used by IDXD, why can't we just allocate a PASID and return it to the driver every time a driver asks for DMA API on PASI mode? Why does the core need to do anything special?

The reason why I store PASID at IOMMU domain is for IOTLB flush within the domain. Device driver is not aware of domain level IOTLB flush. We also have iova_cookie for each domain which essentially is for RIDPASID.

Quoted 20 lines
Quoted 20 lines

Agree. I guess it was a mistake caused by treating ENQCMD as the only user although the actual semantics of the invented interfaces have already evolved to be quite general.

This is very similar to what we have been discussing for iommufd. a PASID is just an additional routing info when attaching a device to an I/O address space (DMA API in this context) and by default it should be a per-device resource except when ENQCMD is explicitly opt in.

Hence it's right time for us to develop common facility working for both this DMA API usage and iommufd, i.e.:

for normal PASID attach to a domain, driver:

	allocates a local pasid from device local space;
	attaches the local pasid to a domain;

for PASID attach in particular for ENQCMD, driver:

Quoted 7 lines
Quoted 7 lines
	allocates a global pasid in system-wide;
	attaches the global pasid to a domain;
	set the global pasid in PASID_MSR;

In both cases the pasid is stored in the attach data instead of the domain.

So during IOTLB flush for the domain, do we loop through the attach data?

Quoted 4 lines
Quoted 4 lines

DMA API pasid is no special from above except it needs to allow one device attached to the same domain twice (one with RID and the other with RID+PASID).

Quoted 8 lines
Quoted 2 lines

for iommufd those operations are initiated by userspace via iommufd uAPI.

My understanding is that device driver owns its PASID policy. If ENQCMD is supported on the device, the PASIDs should be allocated through ioasid_alloc(). Otherwise, the whole PASID pool is managed by the device driver.

It seems the changes we want for this patchset are:
1. move ioasid_alloc() from the core to device (allocation scope will be based on whether ENQCMD is intended or not)
2. store pasid in the attach data
3. use the same iommufd api to attach/set pasid on its default domain Am I summarizing correctly?

Quoted 6 lines

For kernel DMA w/ PASID, after the driver has a PASID for this purpose, it can just set the default domain to the PASID on device. There's no need for enable/disable() interfaces.

Best regards,
baolu

Thanks,

Jacob

#10 lore raw

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

Quoted 3 lines · Jacob Pan

The reason why I store PASID at IOMMU domain is for IOTLB flush within the domain. Device driver is not aware of domain level IOTLB flush. We also have iova_cookie for each domain which essentially is for RIDPASID.

You need to make the PASID stuff work generically.

The domain needs to hold a list of all the places it needs to flush and that list needs to be maintained during attach/detach.

A single PASID on the domain is obviously never going to work generically.

Jason

author
#11 lore raw

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

Hi Jason,

Quoted 7 lines · Jason Gunthorpe
Quoted 4 lines · Jacob Pan

The reason why I store PASID at IOMMU domain is for IOTLB flush within the domain. Device driver is not aware of domain level IOTLB flush. We also have iova_cookie for each domain which essentially is for RIDPASID.

Quoted 8 lines

You need to make the PASID stuff work generically.

The domain needs to hold a list of all the places it needs to flush and that list needs to be maintained during attach/detach.

A single PASID on the domain is obviously never going to work generically.

I agree, I did it this way really meant to be part of iommu_domain's iommu_dma_cookie, not meant to be global. But for the lack of common storage between identity domain and dma domain, I put it here as global.

Then should we also extract RIDPASID to become part of the generic API? i.e. set pasid, flush IOTLB etc. Right? RIDPASID is not in group's pasid_array today.

Thanks,

Jacob
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

#12 lore raw

RE: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

Quoted 23 lines

From: Jacob Pan <[email protected]>
Sent: Wednesday, June 1, 2022 4:44 AM

Hi Jason,

Quoted 7 lines · Jason Gunthorpe
Quoted 4 lines · Jacob Pan

The reason why I store PASID at IOMMU domain is for IOTLB flush within the domain. Device driver is not aware of domain level IOTLB flush. We also have iova_cookie for each domain which essentially is for RIDPASID.

Quoted 8 lines

You need to make the PASID stuff work generically.

The domain needs to hold a list of all the places it needs to flush and that list needs to be maintained during attach/detach.

A single PASID on the domain is obviously never going to work generically.

Quoted 8 lines

I agree, I did it this way really meant to be part of iommu_domain's iommu_dma_cookie, not meant to be global. But for the lack of common storage between identity domain and dma domain, I put it here as global.

Then should we also extract RIDPASID to become part of the generic API? i.e. set pasid, flush IOTLB etc. Right? RIDPASID is not in group's pasid_array today.

RIDPASID is just an alias to RID in the PASID table (similar to pasid#0 on other platforms). it's reserved and not exposed outside the intel-iommu driver. So I don't think it should be managed via the generic iommu API. But probably you can generalize it with other pasids within intel-iommu driver if doing so can simplify the logic there.
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

#13 lore raw

RE: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

In reply to Jacob Pan
Quoted 7 lines

From: Jacob Pan <[email protected]>
Sent: Wednesday, June 1, 2022 1:30 AM

Quoted 4 lines
Quoted 4 lines

In both cases the pasid is stored in the attach data instead of the domain.

So during IOTLB flush for the domain, do we loop through the attach data?

Yes and it's required.

Quoted 8 lines
Quoted 8 lines
Quoted 7 lines

DMA API pasid is no special from above except it needs to allow one device attached to the same domain twice (one with RID and the other with RID+PASID).

for iommufd those operations are initiated by userspace via iommufd uAPI.

Quoted 8 lines
Quoted 5 lines

My understanding is that device driver owns its PASID policy. If ENQCMD is supported on the device, the PASIDs should be allocated through ioasid_alloc(). Otherwise, the whole PASID pool is managed by the device driver.

It seems the changes we want for this patchset are:
1. move ioasid_alloc() from the core to device (allocation scope will be based on whether ENQCMD is intended or not)

yes, and the driver can specify whether the allocation is system-wide or per-device.

Quoted 2 lines

2. store pasid in the attach data
3. use the same iommufd api to attach/set pasid on its default domain

s/iommufd/iommu/

maintainer
#14 lore raw

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

Quoted 7 lines · Kevin
Quoted 6 lines

From: Jacob Pan<[email protected]>
Sent: Wednesday, June 1, 2022 1:30 AM

Quoted 3 lines
Quoted 3 lines

In both cases the pasid is stored in the attach data instead of the domain.

So during IOTLB flush for the domain, do we loop through the attach data?

Yes and it's required.

What does the attach data mean here? Do you mean group->pasid_array?

Why not tracking the {device, pasid} info in the iommu driver when setting domain to {device, pasid}? We have tracked device in a list when setting a domain to device.

Best regards,
baolu
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

#15 lore raw

RE: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

Quoted 6 lines

From: Baolu Lu <[email protected]>
Sent: Wednesday, June 1, 2022 5:37 PM

Quoted 2 lines · Kevin
Quoted 2 lines

From: Jacob Pan<[email protected]>
Sent: Wednesday, June 1, 2022 1:30 AM

Quoted 7 lines
Quoted 5 lines
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines

In both cases the pasid is stored in the attach data instead of the domain.

So during IOTLB flush for the domain, do we loop through the attach data?

Yes and it's required.

What does the attach data mean here? Do you mean group->pasid_array?

any structure describing the attaching relationship from {device, pasid} to a domain

Quoted 5 lines

Why not tracking the {device, pasid} info in the iommu driver when setting domain to {device, pasid}? We have tracked device in a list when setting a domain to device.

Yes, that tracking structure is the attach data. 😊
_______________________________________________
iommu mailing list [email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

author
#16 lore raw

PATCH v4 2/6 iommu: Add a helper to do PASID lookup from domain

In reply to Jacob Pan

IOMMU group maintains a PASID array which stores the associated IOMMU domains. This patch introduces a helper function to do domain to PASID look up. It will be used by TLB flush and device-PASID attach verification.

Signed-off-by Jacob Pan <[email protected]>
---
 drivers/iommu/iommu.c | 22 ++++++++++++++++++++++
 include/linux/iommu.h |  6 +++++-
 2 files changed, 27 insertions(+), 1 deletion(-)
drivers/iommu/iommu.c +22 -0
index 00d0262a1fe9..22f44833db64 100644
@@ -3199,3 +3199,25 @@ struct iommu_domain *iommu_get_domain_for_iopf(struct device *dev,
 
 	return domain;
 }
+
+ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)
+{
+	struct iommu_domain *tdomain;
+	struct iommu_group *group;
+	unsigned long index;
+	ioasid_t pasid = INVALID_IOASID;
+
+	group = iommu_group_get(dev);
+	if (!group)
+		return pasid;
+
+	xa_for_each(&group->pasid_array, index, tdomain) {
+		if (domain == tdomain) {
+			pasid = index;
+			break;
+		}
+	}
+	iommu_group_put(group);
+
+	return pasid;
+}
include/linux/iommu.h +5 -1
index 36ad007084cc..c0440a4be699 100644
@@ -694,7 +694,7 @@ void iommu_detach_device_pasid(struct iommu_domain *domain,
 			       struct device *dev, ioasid_t pasid);
 struct iommu_domain *
 iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid);
-
+ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain);
 #else /* CONFIG_IOMMU_API */
 
 struct iommu_ops {};

@@ -1070,6 +1070,10 @@ iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid)
 {
 	return NULL;
 }
+static ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)
+{
+	return INVALID_IOASID;
+}
 #endif /* CONFIG_IOMMU_API */
 
 #ifdef CONFIG_IOMMU_SVA
Signature · 1 line
2.25.1
maintainer
#17 lore raw

Re: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

Quoted 3 lines · Jacob Pan

IOMMU group maintains a PASID array which stores the associated IOMMU domains. This patch introduces a helper function to do domain to PASID look up. It will be used by TLB flush and device-PASID attach verification.

Do you really need this?

The IOMMU driver has maintained a device tracking list for each domain. It has been used for cache invalidation when unmap() is called against dma domain.

Best regards,
baolu

Quoted 51 lines
Signed-off-by Jacob Pan <[email protected]>
---
  drivers/iommu/iommu.c | 22 ++++++++++++++++++++++
  include/linux/iommu.h |  6 +++++-
  2 files changed, 27 insertions(+), 1 deletion(-)
drivers/iommu/iommu.c +22 -0
index 00d0262a1fe9..22f44833db64 100644
@@ -3199,3 +3199,25 @@ struct iommu_domain *iommu_get_domain_for_iopf(struct device *dev,
 
  	return domain;
  }
+
+ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)
+{
+	struct iommu_domain *tdomain;
+	struct iommu_group *group;
+	unsigned long index;
+	ioasid_t pasid = INVALID_IOASID;
+
+	group = iommu_group_get(dev);
+	if (!group)
+		return pasid;
+
+	xa_for_each(&group->pasid_array, index, tdomain) {
+		if (domain == tdomain) {
+			pasid = index;
+			break;
+		}
+	}
+	iommu_group_put(group);
+
+	return pasid;
+}
include/linux/iommu.h +1 -1
index 36ad007084cc..c0440a4be699 100644
@@ -694,7 +694,7 @@ void iommu_detach_device_pasid(struct iommu_domain *domain,
  			       struct device *dev, ioasid_t pasid);
  struct iommu_domain *
  iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid);
-
+ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain);
  #else /* CONFIG_IOMMU_API */
 
  struct iommu_ops {};

include/linux/iommu.h iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid) · line 1079

Quoted 10 lines
include/linux/iommu.h +4 -0
@@ -1070,6 +1070,10 @@ iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid)
  {
  	return NULL;
  }
+static ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)
+{
+	return INVALID_IOASID;
+}
  #endif /* CONFIG_IOMMU_API */
 
  #ifdef CONFIG_IOMMU_SVA

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

author
#18 lore raw

Re: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

Hi Baolu,

Quoted 10 lines · Baolu Lu
Quoted 4 lines

IOMMU group maintains a PASID array which stores the associated IOMMU domains. This patch introduces a helper function to do domain to PASID look up. It will be used by TLB flush and device-PASID attach verification.

Do you really need this?

The IOMMU driver has maintained a device tracking list for each domain. It has been used for cache invalidation when unmap() is called against dma domain.

Yes, I am aware of the device list. In v3, I stored DMA API PASID in device list of device_domain_info. Since we already have a pasid_array, Jason suggested to share the storage with the code. This helper is needed to reverse look up the DMA PASID based on the domain attached. Discussions here:
https://lore.kernel.org/lkml/[email protected]/t/#mf7cb7d54d89e6e732a020dc22435260da0a49580

Thanks,

Jacob

author
#20 lore raw

Re: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

Hi Christoph,

Quoted 5 lines · Christoph Hellwig
Quoted 2 lines · Jacob Pan

+ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)

Overly long line here.

will fix,

Thanks,

Jacob

#21 lore raw

RE: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

In reply to Jacob Pan
Quoted 31 lines

From: Jacob Pan <[email protected]>
Sent: Thursday, May 19, 2022 2:21 AM

IOMMU group maintains a PASID array which stores the associated IOMMU domains. This patch introduces a helper function to do domain to PASID look up. It will be used by TLB flush and device-PASID attach verification.

Signed-off-by Jacob Pan <[email protected]>
---
 drivers/iommu/iommu.c | 22 ++++++++++++++++++++++
 include/linux/iommu.h |  6 +++++-
 2 files changed, 27 insertions(+), 1 deletion(-)
drivers/iommu/iommu.c +2 -0
index 00d0262a1fe9..22f44833db64 100644
@@ -3199,3 +3199,25 @@ struct iommu_domain
 *iommu_get_domain_for_iopf(struct device *dev,
 
 	return domain;
 }
+
+ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct

iommu_domain *domain)

drivers/iommu/iommu.c +6 -0
+{
+	struct iommu_domain *tdomain;
+	struct iommu_group *group;
+	unsigned long index;
+	ioasid_t pasid = INVALID_IOASID;
+

drivers/iommu/iommu.c struct iommu_domain *iommu_get_domain_for_iopf(struct device *dev, · line 3219

Quoted 10 lines
drivers/iommu/iommu.c +10 -0
+	group = iommu_group_get(dev);
+	if (!group)
+		return pasid;
+
+	xa_for_each(&group->pasid_array, index, tdomain) {
+		if (domain == tdomain) {
+			pasid = index;
+			break;
+		}
+	}

Don't we need to acquire the group lock here?

Btw the intention of this function is a bit confusing. Patch01 already stores the pasid under domain hence it's redundant to get it indirectly from xarray index. You could simply introduce a flag bit (e.g. dma_pasid_enabled) in device_domain_info and then directly use domain->dma_pasid once the flag is true.

Quoted 31 lines
drivers/iommu/iommu.c +4 -0
+	iommu_group_put(group);
+
+	return pasid;
+}
include/linux/iommu.h +1 -1
index 36ad007084cc..c0440a4be699 100644
@@ -694,7 +694,7 @@ void iommu_detach_device_pasid(struct
 iommu_domain *domain,
 			       struct device *dev, ioasid_t pasid);
 struct iommu_domain *
 iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid);
-
+ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct
 iommu_domain *domain);
 #else /* CONFIG_IOMMU_API */

struct iommu_ops {};

include/linux/iommu.h +4 -0
@@ -1070,6 +1070,10 @@ iommu_get_domain_for_iopf(struct device *dev,
 ioasid_t pasid)
 {
 	return NULL;
 }
+static ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct
 iommu_domain *domain)
+{
+	return INVALID_IOASID;
+}
 #endif /* CONFIG_IOMMU_API */
Quoted 3 lines

#ifdef CONFIG_IOMMU_SVA
--
2.25.1

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

#22 lore raw

RE: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

In reply to Jacob Pan
Quoted 25 lines

From: Tian, Kevin
Sent: Monday, May 23, 2022 3:55 PM

Quoted 9 lines

From: Jacob Pan <[email protected]> +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)

drivers/iommu/iommu.c +6 -0
+{
+	struct iommu_domain *tdomain;
+	struct iommu_group *group;
+	unsigned long index;
+	ioasid_t pasid = INVALID_IOASID;
+

drivers/iommu/iommu.c struct iommu_domain *iommu_get_domain_for_iopf(struct device *dev, · line 3219

Quoted 10 lines
drivers/iommu/iommu.c +10 -0
+	group = iommu_group_get(dev);
+	if (!group)
+		return pasid;
+
+	xa_for_each(&group->pasid_array, index, tdomain) {
+		if (domain == tdomain) {
+			pasid = index;
+			break;
+		}
+	}

Don't we need to acquire the group lock here?

Quoted 6 lines

Btw the intention of this function is a bit confusing. Patch01 already stores the pasid under domain hence it's redundant to get it indirectly from xarray index. You could simply introduce a flag bit (e.g. dma_pasid_enabled) in device_domain_info and then directly use domain->dma_pasid once the flag is true.

Just saw your discussion with Jason about v3. While it makes sense to not specialize DMA domain in iommu driver, the use of this function should only be that when the call chain doesn't pass down a pasid value e.g. when doing cache invalidation for domain map/unmap. If the upper interface already carries a pasid e.g. in detach_dev_pasid() iommu driver can simply verify that the corresponding pasid xarray entry points to the specified domain instead of using this function to loop xarray and then verify the returned pasid (as done in patch03/04).

Thanks
Kevin
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

author
#23 lore raw

Re: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

Hi Kevin,

Quoted 17 lines · Kevin
Quoted 17 lines

From: Tian, Kevin
Sent: Monday, May 23, 2022 3:55 PM

Quoted 14 lines

From: Jacob Pan <[email protected]> +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)

drivers/iommu/iommu.c +11 -0
+{
+	struct iommu_domain *tdomain;
+	struct iommu_group *group;
+	unsigned long index;
+	ioasid_t pasid = INVALID_IOASID;
+
+	group = iommu_group_get(dev);
+	if (!group)
+		return pasid;
+
+	xa_for_each(&group->pasid_array, index, tdomain) {
Quoted 8 lines
Quoted 8 lines

drivers/iommu/iommu.c struct iommu_domain *iommu_get_domain_for_iopf(struct device *dev, · line 3219

Quoted 5 lines
drivers/iommu/iommu.c +5 -0
+		if (domain == tdomain) {
+			pasid = index;
+			break;
+		}
+	}

Don't we need to acquire the group lock here?

pasid_array is under RCU read lock so it is protected though may have stale data. It also used in atomic context for TLB flush, cannot take the group mutex. If the caller does detach_dev_pasid while doing TLB flush, it could result in extra flush but harmless.

Quoted 7 lines
Quoted 6 lines

Btw the intention of this function is a bit confusing. Patch01 already stores the pasid under domain hence it's redundant to get it indirectly from xarray index. You could simply introduce a flag bit (e.g. dma_pasid_enabled) in device_domain_info and then directly use domain->dma_pasid once the flag is true.

Quoted 8 lines

Just saw your discussion with Jason about v3. While it makes sense to not specialize DMA domain in iommu driver, the use of this function should only be that when the call chain doesn't pass down a pasid value e.g. when doing cache invalidation for domain map/unmap. If the upper interface already carries a pasid e.g. in detach_dev_pasid() iommu driver can simply verify that the corresponding pasid xarray entry points to the specified domain instead of using this function to loop xarray and then verify the returned pasid (as done in patch03/04).

Excellent point, I could just use xa_load(pasid) to compare the domain instead of loop through xa.
I will add another helper.

bool iommu_is_pasid_domain_attached(struct device *dev, struct iommu_domain *domain, ioasid_t pasid)

{
	struct iommu_group *group;
	bool ret = false;

	group = iommu_group_get(dev);
	if (WARN_ON(!group))
		return false;

	if (domain == xa_load(&group->pasid_array, pasid))
		ret = true;

	iommu_group_put(group);

	return ret;
}

Thanks,

Jacob
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

author
#24 lore raw

PATCH v4 3/6 iommu/vt-d: Implement domain ops for attach_dev_pasid

In reply to Jacob Pan

On VT-d platforms with scalable mode enabled, devices issue DMA requests with PASID need to attach PASIDs to given IOMMU domains. The attach operation involves the following:
- Programming the PASID into the device's PASID table
- Tracking device domain and the PASID relationship
- Managing IOTLB and device TLB invalidations

This patch add attach_dev_pasid functions to the default domain ops which is used by DMA and identity domain types. It could be extended to support other domain types whenever necessary.

Signed-off-by Lu Baolu <[email protected]>
Signed-off-by Jacob Pan <[email protected]>
---
 drivers/iommu/intel/iommu.c | 72 +++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 2 deletions(-)
drivers/iommu/intel/iommu.c +70 -2
index 1c2c92b657c7..75615c105fdf 100644
@@ -1556,12 +1556,18 @@ static void __iommu_flush_dev_iotlb(struct device_domain_info *info,
 				    u64 addr, unsigned int mask)
 {
 	u16 sid, qdep;
+	ioasid_t pasid;
 
 	if (!info || !info->ats_enabled)
 		return;
 
 	sid = info->bus << 8 | info->devfn;
 	qdep = info->ats_qdep;
+	pasid = iommu_get_pasid_from_domain(info->dev, &info->domain->domain);
+	if (pasid != INVALID_IOASID) {
+		qi_flush_dev_iotlb_pasid(info->iommu, sid, info->pfsid,
+					 pasid, qdep, addr, mask);
+	}
 	qi_flush_dev_iotlb(info->iommu, sid, info->pfsid,
 			   qdep, addr, mask);
 }

@@ -1591,6 +1597,7 @@ static void iommu_flush_iotlb_psi(struct intel_iommu *iommu,
 	unsigned int mask = ilog2(aligned_pages);
 	uint64_t addr = (uint64_t)pfn << VTD_PAGE_SHIFT;
 	u16 did = domain->iommu_did[iommu->seq_id];
+	struct iommu_domain *iommu_domain = &domain->domain;
 
 	BUG_ON(pages == 0);
 

@@ -1599,6 +1606,9 @@ static void iommu_flush_iotlb_psi(struct intel_iommu *iommu,
 
 	if (domain_use_first_level(domain)) {
 		qi_flush_piotlb(iommu, did, PASID_RID2PASID, addr, pages, ih);
+		/* flush additional kernel DMA PASIDs attached */
+		if (iommu_domain->dma_pasid)
+			qi_flush_piotlb(iommu, did, iommu_domain->dma_pasid, addr, pages, ih);
 	} else {
 		unsigned long bitmask = aligned_pages - 1;
 

@@ -4255,6 +4265,7 @@ static void __dmar_remove_one_dev_info(struct device_domain_info *info)
 	struct dmar_domain *domain;
 	struct intel_iommu *iommu;
 	unsigned long flags;
+	ioasid_t pasid;
 
 	assert_spin_locked(&device_domain_lock);
 

@@ -4265,10 +4276,15 @@ static void __dmar_remove_one_dev_info(struct device_domain_info *info)
 	domain = info->domain;
 
 	if (info->dev && !dev_is_real_dma_subdevice(info->dev)) {
-		if (dev_is_pci(info->dev) && sm_supported(iommu))
+		if (dev_is_pci(info->dev) && sm_supported(iommu)) {
 			intel_pasid_tear_down_entry(iommu, info->dev,
 					PASID_RID2PASID, false);
-
+			pasid = iommu_get_pasid_from_domain(info->dev,
+							    &info->domain->domain);
+			if (pasid != INVALID_IOASID)
+				intel_pasid_tear_down_entry(iommu, info->dev,
+							    pasid, false);
+		}
 		iommu_disable_dev_iotlb(info);
 		domain_context_clear(info);
 		intel_pasid_free_table(info->dev);

@@ -4904,6 +4920,56 @@ static void intel_iommu_iotlb_sync_map(struct iommu_domain *domain,
 	}
 }
 
+static int intel_iommu_attach_dev_pasid(struct iommu_domain *domain,
+					struct device *dev,
+					ioasid_t pasid)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	struct intel_iommu *iommu = info->iommu;
+	unsigned long flags;
+	int ret = 0;
+
+	if (!sm_supported(iommu) || !info)
+		return -ENODEV;
+
+	if (WARN_ON(pasid == PASID_RID2PASID))
+		return -EINVAL;
+
+	spin_lock_irqsave(&device_domain_lock, flags);
+	spin_lock(&iommu->lock);
+	if (hw_pass_through && domain_type_is_si(dmar_domain))
+		ret = intel_pasid_setup_pass_through(iommu, dmar_domain,
+						     dev, pasid);
+	else if (domain_use_first_level(dmar_domain))
+		ret = domain_setup_first_level(iommu, dmar_domain,
+					       dev, pasid);
+	else
+		ret = intel_pasid_setup_second_level(iommu, dmar_domain,
+						     dev, pasid);
+
+	spin_unlock(&iommu->lock);
+	spin_unlock_irqrestore(&device_domain_lock, flags);
+
+	return ret;
+}
+
+static void intel_iommu_detach_dev_pasid(struct iommu_domain *domain,
+					struct device *dev,
+					ioasid_t pasid)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct intel_iommu *iommu = info->iommu;
+	unsigned long flags;
+
+	if (pasid != iommu_get_pasid_from_domain(info->dev, &info->domain->domain))
+		return;
+
+	spin_lock_irqsave(&iommu->lock, flags);
+	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
+	spin_unlock_irqrestore(&iommu->lock, flags);
+}
+
 const struct iommu_ops intel_iommu_ops = {
 	.capable		= intel_iommu_capable,
 	.domain_alloc		= intel_iommu_domain_alloc,

@@ -4932,6 +4998,8 @@ const struct iommu_ops intel_iommu_ops = {
 		.iova_to_phys		= intel_iommu_iova_to_phys,
 		.free			= intel_iommu_domain_free,
 		.enforce_cache_coherency = intel_iommu_enforce_cache_coherency,
+		.attach_dev_pasid	= intel_iommu_attach_dev_pasid,
+		.detach_dev_pasid	= intel_iommu_detach_dev_pasid,
 	}
 };
Signature · 6 lines
2.25.1

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
#25 lore raw

Re: [PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

Quoted 21 lines · Jacob Pan

On VT-d platforms with scalable mode enabled, devices issue DMA requests with PASID need to attach PASIDs to given IOMMU domains. The attach operation involves the following:
- Programming the PASID into the device's PASID table
- Tracking device domain and the PASID relationship
- Managing IOTLB and device TLB invalidations

This patch add attach_dev_pasid functions to the default domain ops which is used by DMA and identity domain types. It could be extended to support other domain types whenever necessary.

Signed-off-by Lu Baolu <[email protected]>
Signed-off-by Jacob Pan <[email protected]>
 drivers/iommu/intel/iommu.c | 72 +++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 2 deletions(-)
drivers/iommu/intel/iommu.c +1 -0
index 1c2c92b657c7..75615c105fdf 100644
+++ b/drivers/iommu/intel/iommu.c

@@ -1556,12 +1556,18 @@ static void __iommu_flush_dev_iotlb(struct device_domain_info *info,
 				    u64 addr, unsigned int mask)

drivers/iommu/intel/iommu.c static void __iommu_flush_dev_iotlb(struct device_domain_info *info, · line 1566

Quoted 10 lines
drivers/iommu/intel/iommu.c +2 -0
 {
 	u16 sid, qdep;
+	ioasid_t pasid;
 
 	if (!info || !info->ats_enabled)
 		return;
 
 	sid = info->bus << 8 | info->devfn;
 	qdep = info->ats_qdep;
+	pasid = iommu_get_pasid_from_domain(info->dev, &info->domain->domain);

No, a simgple domain can be attached to multiple pasids, all need to be flushed.

This whole API isn't suitable.

Jason
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

author
#26 lore raw

Re: [PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

Hi Jason,

Quoted 29 lines · Jason Gunthorpe
Quoted 28 lines · Jacob Pan

On VT-d platforms with scalable mode enabled, devices issue DMA requests with PASID need to attach PASIDs to given IOMMU domains. The attach operation involves the following:
- Programming the PASID into the device's PASID table
- Tracking device domain and the PASID relationship
- Managing IOTLB and device TLB invalidations

This patch add attach_dev_pasid functions to the default domain ops which is used by DMA and identity domain types. It could be extended to support other domain types whenever necessary.

Signed-off-by Lu Baolu <[email protected]>
Signed-off-by Jacob Pan <[email protected]>
 drivers/iommu/intel/iommu.c | 72 +++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 2 deletions(-)
drivers/iommu/intel/iommu.c +2 -0
index 1c2c92b657c7..75615c105fdf 100644
+++ b/drivers/iommu/intel/iommu.c

@@ -1556,12 +1556,18 @@ static void __iommu_flush_dev_iotlb(struct
 device_domain_info *info, u64 addr, unsigned int mask)
 {
 	u16 sid, qdep;
+	ioasid_t pasid;
 
 	if (!info || !info->ats_enabled)
 		return;
Quoted 8 lines

drivers/iommu/intel/iommu.c static void __iommu_flush_dev_iotlb(struct device_domain_info *info,

Quoted 4 lines
drivers/iommu/intel/iommu.c +1 -0
 	sid = info->bus << 8 | info->devfn;
 	qdep = info->ats_qdep;
+	pasid = iommu_get_pasid_from_domain(info->dev,
 &info->domain->domain);

No, a simgple domain can be attached to multiple pasids, all need to be flushed.

Here is device TLB flush, why would I want to flush PASIDs other than my own device attached?

At one level up, we do have a list of device to be flushed.

	list_for_each_entry(info, &domain->devices, link)
		__iommu_flush_dev_iotlb(info, addr, mask);

Note that RID2PASID is not in the pasid_array, its DEVTLB flush also needs special handling in that the device is doing DMA w/o PASID, thus not aware of RID2PASID.

Quoted 3 lines

This whole API isn't suitable.

Jason

Thanks,

Jacob
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

#27 lore raw

Re: [PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

Quoted 41 lines · Jacob Pan

Hi Jason,

Quoted 29 lines · Jason Gunthorpe
Quoted 28 lines · Jacob Pan

On VT-d platforms with scalable mode enabled, devices issue DMA requests with PASID need to attach PASIDs to given IOMMU domains. The attach operation involves the following:
- Programming the PASID into the device's PASID table
- Tracking device domain and the PASID relationship
- Managing IOTLB and device TLB invalidations

This patch add attach_dev_pasid functions to the default domain ops which is used by DMA and identity domain types. It could be extended to support other domain types whenever necessary.

Signed-off-by Lu Baolu <[email protected]>
Signed-off-by Jacob Pan <[email protected]>
 drivers/iommu/intel/iommu.c | 72 +++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 2 deletions(-)
drivers/iommu/intel/iommu.c +2 -0
index 1c2c92b657c7..75615c105fdf 100644
+++ b/drivers/iommu/intel/iommu.c

@@ -1556,12 +1556,18 @@ static void __iommu_flush_dev_iotlb(struct
 device_domain_info *info, u64 addr, unsigned int mask)
 {
 	u16 sid, qdep;
+	ioasid_t pasid;
 
 	if (!info || !info->ats_enabled)
 		return;
Quoted 8 lines

drivers/iommu/intel/iommu.c static void __iommu_flush_dev_iotlb(struct device_domain_info *info,

Quoted 4 lines
drivers/iommu/intel/iommu.c +1 -0
 	sid = info->bus << 8 | info->devfn;
 	qdep = info->ats_qdep;
+	pasid = iommu_get_pasid_from_domain(info->dev,
 &info->domain->domain);

No, a simgple domain can be attached to multiple pasids, all need to be flushed.

Quoted 2 lines

Here is device TLB flush, why would I want to flush PASIDs other than my own device attached?

Again, a domain can be attached to multiple PASID's *on the same device*

The idea that there is only one PASID per domain per device is not right.

Jason

author
#28 lore raw

Re: [PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

Hi Jason,

Quoted 43 lines · Jason Gunthorpe
Quoted 42 lines · Jacob Pan

Hi Jason,

Quoted 38 lines · Jason Gunthorpe
Quoted 23 lines · Jacob Pan

On VT-d platforms with scalable mode enabled, devices issue DMA requests with PASID need to attach PASIDs to given IOMMU domains. The attach operation involves the following:
- Programming the PASID into the device's PASID table
- Tracking device domain and the PASID relationship
- Managing IOTLB and device TLB invalidations

This patch add attach_dev_pasid functions to the default domain ops which is used by DMA and identity domain types. It could be extended to support other domain types whenever necessary.

Signed-off-by Lu Baolu <[email protected]>
Signed-off-by Jacob Pan <[email protected]>
 drivers/iommu/intel/iommu.c | 72

+++++++++++++++++++++++++++++++++++-- 1 file changed, 70 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/iommu/intel/iommu.c index 1c2c92b657c7..75615c105fdf 100644 +++ b/drivers/iommu/intel/iommu.c

drivers/iommu/intel/iommu.c +0 -0
@@ -1556,12 +1556,18 @@ static void __iommu_flush_dev_iotlb(struct
 device_domain_info *info, u64 addr, unsigned int mask)
 {

drivers/iommu/intel/iommu.c static void __iommu_flush_dev_iotlb(struct · line 1567

Quoted 10 lines
drivers/iommu/intel/iommu.c +2 -0
 	u16 sid, qdep;
+	ioasid_t pasid;
 
 	if (!info || !info->ats_enabled)
 		return;
 
 	sid = info->bus << 8 | info->devfn;
 	qdep = info->ats_qdep;
+	pasid = iommu_get_pasid_from_domain(info->dev,
 &info->domain->domain);

No, a simgple domain can be attached to multiple pasids, all need to be flushed.

Quoted 9 lines
Quoted 2 lines

Here is device TLB flush, why would I want to flush PASIDs other than my own device attached?

Again, a domain can be attached to multiple PASID's *on the same device*

The idea that there is only one PASID per domain per device is not right.

Got you, I was under the impression that there is no use case yet for multiple PASIDs per device-domain based on our early discussion.
https://lore.kernel.org/lkml/[email protected]/

Perhaps I misunderstood. I will make the API more future proof and search through the pasid_array xa for *all* domain-device matches. Like you suggested earlier, may need to retrieve the xa in the first place and use xas_for_each to get a faster search.

Thanks,

Jacob

#29 lore raw

Re: [PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

Quoted 5 lines · Jacob Pan
Quoted 3 lines

The idea that there is only one PASID per domain per device is not right.

Got you, I was under the impression that there is no use case yet for multiple PASIDs per device-domain based on our early discussion.

The key word there is "in-kernel" and "DMA API" - iommufd userspace will do whatever it likes.

I wish you guys would organize your work so adding generic PASID support to IOMMU was its own series with its own purpose so everything stops becoming confused with SVA and DMA API ideas that are not general.

Jason

author
#30 lore raw

PATCH v4 4/6 iommu: Add PASID support for DMA mapping API users

In reply to Jacob Pan

DMA mapping API is the de facto standard for in-kernel DMA. It operates on a per device/RID basis which is not PASID-aware.

Some modern devices such as Intel Data Streaming Accelerator, PASID is required for certain work submissions. To allow such devices use DMA mapping API, we need the following functionalities:
1. Provide device a way to retrieve a PASID for work submission within the kernel
2. Enable the kernel PASID on the IOMMU for the device
3. Attach the kernel PASID to the device's default DMA domain, let it be IOVA or physical address in case of pass-through.

This patch introduces a driver facing API that enables DMA API PASID usage. Once enabled, device drivers can continue to use DMA APIs as is. There is no difference in dma_handle between without PASID and with PASID.

Signed-off-by Jacob Pan <[email protected]>
---
 drivers/iommu/dma-iommu.c | 114 ++++++++++++++++++++++++++++++++++++++
 include/linux/dma-iommu.h |   3 +
 2 files changed, 117 insertions(+)
drivers/iommu/dma-iommu.c +114 -0
index 1ca85d37eeab..6ad7ba619ef0 100644
@@ -34,6 +34,8 @@ struct iommu_dma_msi_page {
 	phys_addr_t		phys;
 };
 
+static DECLARE_IOASID_SET(iommu_dma_pasid);
+
 enum iommu_dma_cookie_type {
 	IOMMU_DMA_IOVA_COOKIE,
 	IOMMU_DMA_MSI_COOKIE,

@@ -370,6 +372,118 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
 	domain->iova_cookie = NULL;
 }
 
+/* Protect iommu_domain DMA PASID data */
+static DEFINE_MUTEX(dma_pasid_lock);
+/**
+ * iommu_attach_dma_pasid --Attach a PASID for in-kernel DMA. Use the device's
+ * DMA domain.
+ * @dev: Device to be enabled
+ * @pasid: The returned kernel PASID to be used for DMA
+ *
+ * DMA request with PASID will be mapped the same way as the legacy DMA.
+ * If the device is in pass-through, PASID will also pass-through. If the
+ * device is in IOVA, the PASID will point to the same IOVA page table.
+ *
+ * @return err code or 0 on success
+ */
+int iommu_attach_dma_pasid(struct device *dev, ioasid_t *pasid)
+{
+	struct iommu_domain *dom;
+	ioasid_t id, max;
+	int ret = 0;
+
+	dom = iommu_get_domain_for_dev(dev);
+	if (!dom || !dom->ops || !dom->ops->attach_dev_pasid)
+		return -ENODEV;
+
+	/* Only support domain types that DMA API can be used */
+	if (dom->type == IOMMU_DOMAIN_UNMANAGED ||
+	    dom->type == IOMMU_DOMAIN_BLOCKED) {
+		dev_warn(dev, "Invalid domain type %d", dom->type);
+		return -EPERM;
+	}
+
+	mutex_lock(&dma_pasid_lock);
+	id = dom->dma_pasid;
+	if (!id) {
+		/*
+		 * First device to use PASID in its DMA domain, allocate
+		 * a single PASID per DMA domain is all we need, it is also
+		 * good for performance when it comes down to IOTLB flush.
+		 */
+		max = 1U << dev->iommu->pasid_bits;
+		if (!max) {
+			ret = -EINVAL;
+			goto done_unlock;
+		}
+
+		id = ioasid_alloc(&iommu_dma_pasid, 1, max, dev);
+		if (id == INVALID_IOASID) {
+			ret = -ENOMEM;
+			goto done_unlock;
+		}
+
+		dom->dma_pasid = id;
+		atomic_set(&dom->dma_pasid_users, 1);
+	}
+
+	ret = iommu_attach_device_pasid(dom, dev, id);
+	if (!ret) {
+		*pasid = id;
+		atomic_inc(&dom->dma_pasid_users);
+		goto done_unlock;
+	}
+
+	if (atomic_dec_and_test(&dom->dma_pasid_users)) {
+		ioasid_free(id);
+		dom->dma_pasid = 0;
+	}
+done_unlock:
+	mutex_unlock(&dma_pasid_lock);
+	return ret;
+}
+EXPORT_SYMBOL(iommu_attach_dma_pasid);
+
+/**
+ * iommu_detach_dma_pasid --Disable in-kernel DMA request with PASID
+ * @dev:	Device's PASID DMA to be disabled
+ *
+ * It is the device driver's responsibility to ensure no more incoming DMA
+ * requests with the kernel PASID before calling this function. IOMMU driver
+ * ensures PASID cache, IOTLBs related to the kernel PASID are cleared and
+ * drained.
+ *
+ */
+void iommu_detach_dma_pasid(struct device *dev)
+{
+	struct iommu_domain *dom;
+	ioasid_t pasid;
+
+	dom = iommu_get_domain_for_dev(dev);
+	if (WARN_ON(!dom || !dom->ops || !dom->ops->detach_dev_pasid))
+		return;
+
+	/* Only support DMA API managed domain type */
+	if (WARN_ON(dom->type == IOMMU_DOMAIN_UNMANAGED ||
+		    dom->type == IOMMU_DOMAIN_BLOCKED))
+		return;
+
+	mutex_lock(&dma_pasid_lock);
+	pasid = iommu_get_pasid_from_domain(dev, dom);
+	if (!pasid || pasid == INVALID_IOASID) {
+		dev_err(dev, "No valid DMA PASID attached\n");
+		mutex_unlock(&dma_pasid_lock);
+		return;
+	}
+	iommu_detach_device_pasid(dom, dev, pasid);
+	if (atomic_dec_and_test(&dom->dma_pasid_users)) {
+		ioasid_free(pasid);
+		dom->dma_pasid = 0;
+	}
+	mutex_unlock(&dma_pasid_lock);
+}
+EXPORT_SYMBOL(iommu_detach_dma_pasid);
+
 /**
  * iommu_dma_get_resv_regions - Reserved region driver helper
  * @dev: Device from iommu_get_resv_regions()
include/linux/dma-iommu.h +3 -0
index 24607dc3c2ac..538650b9cb75 100644
@@ -18,6 +18,9 @@ int iommu_get_dma_cookie(struct iommu_domain *domain);
 int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base);
 void iommu_put_dma_cookie(struct iommu_domain *domain);
 
+int iommu_attach_dma_pasid(struct device *dev, ioasid_t *pasid);
+void iommu_detach_dma_pasid(struct device *dev);
+
 /* Setup call for arch DMA mapping code */
 void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit);
 int iommu_dma_init_fq(struct iommu_domain *domain);
Signature · 1 line
2.25.1
#31 lore raw

RE: [PATCH v4 4/6] iommu: Add PASID support for DMA mapping API users

Quoted 53 lines

From: Jacob Pan <[email protected]>
Sent: Thursday, May 19, 2022 2:21 AM

DMA mapping API is the de facto standard for in-kernel DMA. It operates on a per device/RID basis which is not PASID-aware.

Some modern devices such as Intel Data Streaming Accelerator, PASID is required for certain work submissions. To allow such devices use DMA mapping API, we need the following functionalities:
1. Provide device a way to retrieve a PASID for work submission within the kernel
2. Enable the kernel PASID on the IOMMU for the device
3. Attach the kernel PASID to the device's default DMA domain, let it be IOVA or physical address in case of pass-through.

This patch introduces a driver facing API that enables DMA API PASID usage. Once enabled, device drivers can continue to use DMA APIs as is. There is no difference in dma_handle between without PASID and with PASID.

Signed-off-by Jacob Pan <[email protected]>
---
 drivers/iommu/dma-iommu.c | 114
++++++++++++++++++++++++++++++++++++++
 include/linux/dma-iommu.h |   3 +
 2 files changed, 117 insertions(+)
drivers/iommu/dma-iommu.c +6 -0
index 1ca85d37eeab..6ad7ba619ef0 100644
@@ -34,6 +34,8 @@ struct iommu_dma_msi_page {
 	phys_addr_t		phys;
 };
 
+static DECLARE_IOASID_SET(iommu_dma_pasid);
+
 enum iommu_dma_cookie_type {
 	IOMMU_DMA_IOVA_COOKIE,
 	IOMMU_DMA_MSI_COOKIE,

@@ -370,6 +372,118 @@ void iommu_put_dma_cookie(struct
 iommu_domain *domain)
 	domain->iova_cookie = NULL;
 }
 
+/* Protect iommu_domain DMA PASID data */
+static DEFINE_MUTEX(dma_pasid_lock);
+/**
+ * iommu_attach_dma_pasid --Attach a PASID for in-kernel DMA. Use the

device's
+ * DMA domain.
+ * @dev: Device to be enabled
+ * @pasid: The returned kernel PASID to be used for DMA

Quoted 8 lines

+ *
+ * DMA request with PASID will be mapped the same way as the legacy DMA.
+ * If the device is in pass-through, PASID will also pass-through. If the
+ * device is in IOVA, the PASID will point to the same IOVA page table.
+ *
+ * @return err code or 0 on success
+ */
+int iommu_attach_dma_pasid(struct device *dev, ioasid_t *pasid)

iommu_attach_dma_domain_pasid? 'dma_pasid' is too broad from a API p.o.v.

Quoted 5 lines
drivers/iommu/dma-iommu.c +5 -0
+{
+	struct iommu_domain *dom;
+	ioasid_t id, max;
+	int ret = 0;
+

drivers/iommu/dma-iommu.c void iommu_put_dma_cookie(struct iommu_domain *domain) · line 404

Quoted 10 lines
drivers/iommu/dma-iommu.c +10 -0
+	dom = iommu_get_domain_for_dev(dev);
+	if (!dom || !dom->ops || !dom->ops->attach_dev_pasid)
+		return -ENODEV;
+
+	/* Only support domain types that DMA API can be used */
+	if (dom->type == IOMMU_DOMAIN_UNMANAGED ||
+	    dom->type == IOMMU_DOMAIN_BLOCKED) {
+		dev_warn(dev, "Invalid domain type %d", dom->type);
+		return -EPERM;
+	}

WARN_ON.

and probably we can just check whether domain is default domain here.

Quoted 13 lines
drivers/iommu/dma-iommu.c +13 -0
+
+	mutex_lock(&dma_pasid_lock);
+	id = dom->dma_pasid;
+	if (!id) {
+		/*
+		 * First device to use PASID in its DMA domain, allocate
+		 * a single PASID per DMA domain is all we need, it is also
+		 * good for performance when it comes down to IOTLB flush.
+		 */
+		max = 1U << dev->iommu->pasid_bits;
+		if (!max) {
+			ret = -EINVAL;
+			goto done_unlock;

drivers/iommu/dma-iommu.c void iommu_put_dma_cookie(struct iommu_domain *domain) · line 427

Quoted 10 lines
drivers/iommu/dma-iommu.c +10 -0
+		}
+
+		id = ioasid_alloc(&iommu_dma_pasid, 1, max, dev);
+		if (id == INVALID_IOASID) {
+			ret = -ENOMEM;
+			goto done_unlock;
+		}
+
+		dom->dma_pasid = id;
+		atomic_set(&dom->dma_pasid_users, 1);

this is always accessed with lock held hence no need to be atomic.

Quoted 40 lines
drivers/iommu/dma-iommu.c +40 -0
+	}
+
+	ret = iommu_attach_device_pasid(dom, dev, id);
+	if (!ret) {
+		*pasid = id;
+		atomic_inc(&dom->dma_pasid_users);
+		goto done_unlock;
+	}
+
+	if (atomic_dec_and_test(&dom->dma_pasid_users)) {
+		ioasid_free(id);
+		dom->dma_pasid = 0;
+	}
+done_unlock:
+	mutex_unlock(&dma_pasid_lock);
+	return ret;
+}
+EXPORT_SYMBOL(iommu_attach_dma_pasid);
+
+/**
+ * iommu_detach_dma_pasid --Disable in-kernel DMA request with PASID
+ * @dev:	Device's PASID DMA to be disabled
+ *
+ * It is the device driver's responsibility to ensure no more incoming DMA
+ * requests with the kernel PASID before calling this function. IOMMU driver
+ * ensures PASID cache, IOTLBs related to the kernel PASID are cleared and
+ * drained.
+ *
+ */
+void iommu_detach_dma_pasid(struct device *dev)
+{
+	struct iommu_domain *dom;
+	ioasid_t pasid;
+
+	dom = iommu_get_domain_for_dev(dev);
+	if (WARN_ON(!dom || !dom->ops || !dom->ops->detach_dev_pasid))
+		return;
+
+	/* Only support DMA API managed domain type */
+	if (WARN_ON(dom->type == IOMMU_DOMAIN_UNMANAGED ||

drivers/iommu/dma-iommu.c void iommu_put_dma_cookie(struct iommu_domain *domain) · line 477

Quoted 10 lines
drivers/iommu/dma-iommu.c +10 -0
+		    dom->type == IOMMU_DOMAIN_BLOCKED))
+		return;
+
+	mutex_lock(&dma_pasid_lock);
+	pasid = iommu_get_pasid_from_domain(dev, dom);
+	if (!pasid || pasid == INVALID_IOASID) {
+		dev_err(dev, "No valid DMA PASID attached\n");
+		mutex_unlock(&dma_pasid_lock);
+		return;
+	}

here just use dom->dma_pasid and let iommu driver to figure out underlying whether this device has been attached to the domain with the said pasid.

Quoted 26 lines
drivers/iommu/dma-iommu.c +9 -0
+	iommu_detach_device_pasid(dom, dev, pasid);
+	if (atomic_dec_and_test(&dom->dma_pasid_users)) {
+		ioasid_free(pasid);
+		dom->dma_pasid = 0;
+	}
+	mutex_unlock(&dma_pasid_lock);
+}
+EXPORT_SYMBOL(iommu_detach_dma_pasid);
+
 /**
  * iommu_dma_get_resv_regions - Reserved region driver helper
  * @dev: Device from iommu_get_resv_regions()
include/linux/dma-iommu.h +3 -0
index 24607dc3c2ac..538650b9cb75 100644
@@ -18,6 +18,9 @@ int iommu_get_dma_cookie(struct iommu_domain
 *domain);
 int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t
 base);
 void iommu_put_dma_cookie(struct iommu_domain *domain);
 
+int iommu_attach_dma_pasid(struct device *dev, ioasid_t *pasid);
+void iommu_detach_dma_pasid(struct device *dev);
+
 /* Setup call for arch DMA mapping code */
Quoted 5 lines

void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64
dma_limit);
int iommu_dma_init_fq(struct iommu_domain *domain);
--
2.25.1

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

author
#32 lore raw

Re: [PATCH v4 4/6] iommu: Add PASID support for DMA mapping API users

Hi Kevin,

Quoted 58 lines · Kevin
Quoted 58 lines

From: Jacob Pan <[email protected]>
Sent: Thursday, May 19, 2022 2:21 AM

DMA mapping API is the de facto standard for in-kernel DMA. It operates on a per device/RID basis which is not PASID-aware.

Some modern devices such as Intel Data Streaming Accelerator, PASID is required for certain work submissions. To allow such devices use DMA mapping API, we need the following functionalities:
1. Provide device a way to retrieve a PASID for work submission within the kernel
2. Enable the kernel PASID on the IOMMU for the device
3. Attach the kernel PASID to the device's default DMA domain, let it be IOVA or physical address in case of pass-through.

This patch introduces a driver facing API that enables DMA API PASID usage. Once enabled, device drivers can continue to use DMA APIs as is. There is no difference in dma_handle between without PASID and with PASID.

Signed-off-by Jacob Pan <[email protected]>
---
 drivers/iommu/dma-iommu.c | 114
++++++++++++++++++++++++++++++++++++++
 include/linux/dma-iommu.h |   3 +
 2 files changed, 117 insertions(+)
drivers/iommu/dma-iommu.c +6 -0
index 1ca85d37eeab..6ad7ba619ef0 100644
@@ -34,6 +34,8 @@ struct iommu_dma_msi_page {
 	phys_addr_t		phys;
 };
 
+static DECLARE_IOASID_SET(iommu_dma_pasid);
+
 enum iommu_dma_cookie_type {
 	IOMMU_DMA_IOVA_COOKIE,
 	IOMMU_DMA_MSI_COOKIE,

@@ -370,6 +372,118 @@ void iommu_put_dma_cookie(struct
 iommu_domain *domain)
 	domain->iova_cookie = NULL;
 }
 
+/* Protect iommu_domain DMA PASID data */
+static DEFINE_MUTEX(dma_pasid_lock);
+/**
+ * iommu_attach_dma_pasid --Attach a PASID for in-kernel DMA. Use the

device's
+ * DMA domain.
+ * @dev: Device to be enabled
+ * @pasid: The returned kernel PASID to be used for DMA
+ *
+ * DMA request with PASID will be mapped the same way as the legacy DMA.
+ * If the device is in pass-through, PASID will also pass-through. If the

Quoted 9 lines
Quoted 5 lines

+ * device is in IOVA, the PASID will point to the same IOVA page table.
+ *
+ * @return err code or 0 on success
+ */
+int iommu_attach_dma_pasid(struct device *dev, ioasid_t *pasid)

iommu_attach_dma_domain_pasid? 'dma_pasid' is too broad from a API p.o.v.

I agree dma_pasid is too broad, technically it is dma_api_pasid but seems too long.
My concern with dma_domain_pasid is that the pasid can also be used for identity domain.

Quoted 18 lines
Quoted 5 lines
drivers/iommu/dma-iommu.c +5 -0
+{
+	struct iommu_domain *dom;
+	ioasid_t id, max;
+	int ret = 0;
+

drivers/iommu/dma-iommu.c void iommu_put_dma_cookie(struct iommu_domain *domain) · line 404

Quoted 10 lines
drivers/iommu/dma-iommu.c +10 -0
+	dom = iommu_get_domain_for_dev(dev);
+	if (!dom || !dom->ops || !dom->ops->attach_dev_pasid)
+		return -ENODEV;
+
+	/* Only support domain types that DMA API can be used */
+	if (dom->type == IOMMU_DOMAIN_UNMANAGED ||
+	    dom->type == IOMMU_DOMAIN_BLOCKED) {
+		dev_warn(dev, "Invalid domain type %d", dom->type);
+		return -EPERM;
+	}

WARN_ON.

Quoted 2 lines

and probably we can just check whether domain is default domain here.

good point, I will just use
struct iommu_domain *def_domain = iommu_get_dma_domain(dev);

Quoted 20 lines
Quoted 20 lines
drivers/iommu/dma-iommu.c +6 -0
+
+	mutex_lock(&dma_pasid_lock);
+	id = dom->dma_pasid;
+	if (!id) {
+		/*
+		 * First device to use PASID in its DMA domain,

allocate

drivers/iommu/dma-iommu.c +1 -0
+		 * a single PASID per DMA domain is all we need, it is

also

drivers/iommu/dma-iommu.c +1 -0
+		 * good for performance when it comes down to IOTLB

flush.

drivers/iommu/dma-iommu.c +9 -0
+		 */
+		max = 1U << dev->iommu->pasid_bits;
+		if (!max) {
+			ret = -EINVAL;
+			goto done_unlock;
+		}
+
+		id = ioasid_alloc(&iommu_dma_pasid, 1, max, dev);
+		if (id == INVALID_IOASID) {
Quoted 9 lines

drivers/iommu/dma-iommu.c void iommu_put_dma_cookie(struct iommu_domain *domain) · line 427

Quoted 6 lines
drivers/iommu/dma-iommu.c +6 -0
+			ret = -ENOMEM;
+			goto done_unlock;
+		}
+
+		dom->dma_pasid = id;
+		atomic_set(&dom->dma_pasid_users, 1);

this is always accessed with lock held hence no need to be atomic.

good catch, will fix

Quoted 49 lines
Quoted 49 lines
drivers/iommu/dma-iommu.c +24 -0
+	}
+
+	ret = iommu_attach_device_pasid(dom, dev, id);
+	if (!ret) {
+		*pasid = id;
+		atomic_inc(&dom->dma_pasid_users);
+		goto done_unlock;
+	}
+
+	if (atomic_dec_and_test(&dom->dma_pasid_users)) {
+		ioasid_free(id);
+		dom->dma_pasid = 0;
+	}
+done_unlock:
+	mutex_unlock(&dma_pasid_lock);
+	return ret;
+}
+EXPORT_SYMBOL(iommu_attach_dma_pasid);
+
+/**
+ * iommu_detach_dma_pasid --Disable in-kernel DMA request with PASID
+ * @dev:	Device's PASID DMA to be disabled
+ *
+ * It is the device driver's responsibility to ensure no more incoming

DMA
+ * requests with the kernel PASID before calling this function. IOMMU driver
+ * ensures PASID cache, IOTLBs related to the kernel PASID are cleared and
+ * drained.
+ *
+ */

drivers/iommu/dma-iommu.c +17 -0
+void iommu_detach_dma_pasid(struct device *dev)
+{
+	struct iommu_domain *dom;
+	ioasid_t pasid;
+
+	dom = iommu_get_domain_for_dev(dev);
+	if (WARN_ON(!dom || !dom->ops || !dom->ops->detach_dev_pasid))
+		return;
+
+	/* Only support DMA API managed domain type */
+	if (WARN_ON(dom->type == IOMMU_DOMAIN_UNMANAGED ||
+		    dom->type == IOMMU_DOMAIN_BLOCKED))
+		return;
+
+	mutex_lock(&dma_pasid_lock);
+	pasid = iommu_get_pasid_from_domain(dev, dom);
+	if (!pasid || pasid == INVALID_IOASID) {
Quoted 9 lines

drivers/iommu/dma-iommu.c void iommu_put_dma_cookie(struct iommu_domain *domain) · line 477

Quoted 4 lines
drivers/iommu/dma-iommu.c +4 -0
+		dev_err(dev, "No valid DMA PASID attached\n");
+		mutex_unlock(&dma_pasid_lock);
+		return;
+	}

here just use dom->dma_pasid and let iommu driver to figure out underlying whether this device has been attached to the domain with the said pasid.

Yeah, I am checking the pasid matching in the iommu driver. My thinking is that here is a quick sanity check in the common code to rule out invalid value.

Thanks a lot!

Jacob

author
#33 lore raw

PATCH v4 5/6 dmaengine: idxd: Use DMA API for in-kernel DMA with PASID

In reply to Jacob Pan

The current in-kernel supervisor PASID support is based on the SVM/SVA machinery in SVA lib. The binding between a kernel PASID and kernel mapping has many flaws. See discussions in the link below.

This patch enables in-kernel DMA by switching from SVA lib to the standard DMA mapping APIs. Since both DMA requests with and without PASIDs are mapped identically, there is no change to how DMA APIs are used after the kernel PASID is enabled.

Link https://lore.kernel.org/linux-iommu/[email protected]/ open here
Signed-off-by Jacob Pan <[email protected]>
---
 drivers/dma/idxd/idxd.h  |  1 -
 drivers/dma/idxd/init.c  | 34 +++++++++-------------------------
 drivers/dma/idxd/sysfs.c |  7 -------
 3 files changed, 9 insertions(+), 33 deletions(-)
drivers/dma/idxd/idxd.h +0 -1
index ccbefd0be617..190b08bd7c08 100644
@@ -277,7 +277,6 @@ struct idxd_device {
 	struct idxd_wq **wqs;
 	struct idxd_engine **engines;
 
-	struct iommu_sva *sva;
 	unsigned int pasid;
 
 	int num_groups;
drivers/dma/idxd/init.c +9 -25
index e1b5d1e4a949..e2e1c0eae6d6 100644
@@ -16,6 +16,7 @@
 #include <linux/idr.h>
 #include <linux/intel-svm.h>
 #include <linux/iommu.h>
+#include <linux/dma-iommu.h>
 #include <uapi/linux/idxd.h>
 #include <linux/dmaengine.h>
 #include "../dmaengine.h"

@@ -466,36 +467,22 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d
 
 static int idxd_enable_system_pasid(struct idxd_device *idxd)
 {
-	int flags;
-	unsigned int pasid;
-	struct iommu_sva *sva;
+	u32 pasid;
+	int ret;
 
-	flags = SVM_FLAG_SUPERVISOR_MODE;
-
-	sva = iommu_sva_bind_device(&idxd->pdev->dev, NULL, &flags);
-	if (IS_ERR(sva)) {
-		dev_warn(&idxd->pdev->dev,
-			 "iommu sva bind failed: %ld\n", PTR_ERR(sva));
-		return PTR_ERR(sva);
-	}
-
-	pasid = iommu_sva_get_pasid(sva);
-	if (pasid == IOMMU_PASID_INVALID) {
-		iommu_sva_unbind_device(sva);
-		return -ENODEV;
+	ret = iommu_attach_dma_pasid(&idxd->pdev->dev, &pasid);
+	if (ret) {
+		dev_err(&idxd->pdev->dev, "No DMA PASID %d\n", ret);
+		return ret;
 	}
-
-	idxd->sva = sva;
 	idxd->pasid = pasid;
-	dev_dbg(&idxd->pdev->dev, "system pasid: %u\n", pasid);
+
 	return 0;
 }
 
 static void idxd_disable_system_pasid(struct idxd_device *idxd)
 {
-
-	iommu_sva_unbind_device(idxd->sva);
-	idxd->sva = NULL;
+	iommu_detach_dma_pasid(&idxd->pdev->dev);
 }
 
 static int idxd_probe(struct idxd_device *idxd)

@@ -527,10 +514,7 @@ static int idxd_probe(struct idxd_device *idxd)
 			else
 				set_bit(IDXD_FLAG_PASID_ENABLED, &idxd->flags);
 		}
-	} else if (!sva) {
-		dev_warn(dev, "User forced SVA off via module param.\n");
 	}
-
 	idxd_read_caps(idxd);
 	idxd_read_table_offsets(idxd);
drivers/dma/idxd/sysfs.c +0 -7
index dfd549685c46..a48928973bd4 100644
@@ -839,13 +839,6 @@ static ssize_t wq_name_store(struct device *dev,
 	if (strlen(buf) > WQ_NAME_SIZE || strlen(buf) == 0)
 		return -EINVAL;
 
-	/*
-	 * This is temporarily placed here until we have SVM support for
-	 * dmaengine.
-	 */
-	if (wq->type == IDXD_WQT_KERNEL && device_pasid_enabled(wq->idxd))
-		return -EOPNOTSUPP;
-
 	memset(wq->name, 0, WQ_NAME_SIZE + 1);
 	strncpy(wq->name, buf, WQ_NAME_SIZE);
 	strreplace(wq->name, '\n', '\0');
Signature · 1 line
2.25.1
author
#34 lore raw

PATCH v4 6/6 iommu/vt-d: Delete unused SVM flag

In reply to Jacob Pan

Supervisor PASID for SVA/SVM is no longer supported, delete the unused flag.

Signed-off-by Jacob Pan <[email protected]>
---
 drivers/iommu/intel/svm.c |  2 +-
 include/linux/intel-svm.h | 13 -------------
 2 files changed, 1 insertion(+), 14 deletions(-)
drivers/iommu/intel/svm.c +1 -1
index 44331db060e4..5b220d464218 100644
@@ -750,7 +750,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 			 * to unbind the mm while any page faults are outstanding.
 			 */
 			svm = pasid_private_find(req->pasid);
-			if (IS_ERR_OR_NULL(svm) || (svm->flags & SVM_FLAG_SUPERVISOR_MODE))
+			if (IS_ERR_OR_NULL(svm))
 				goto bad_req;
 		}
include/linux/intel-svm.h +0 -13
index b3b125b332aa..6835a665c195 100644
@@ -13,17 +13,4 @@
 #define PRQ_RING_MASK	((0x1000 << PRQ_ORDER) - 0x20)
 #define PRQ_DEPTH	((0x1000 << PRQ_ORDER) >> 5)
 
-/*
- * The SVM_FLAG_SUPERVISOR_MODE flag requests a PASID which can be used only
- * for access to kernel addresses. No IOTLB flushes are automatically done
- * for kernel mappings; it is valid only for access to the kernel's static
- * 1:1 mapping of physical memory — not to vmalloc or even module mappings.
- * A future API addition may permit the use of such ranges, by means of an
- * explicit IOTLB flush call (akin to the DMA API's unmap method).
- *
- * It is unlikely that we will ever hook into flush_tlb_kernel_range() to
- * do such IOTLB flushes automatically.
- */
-#define SVM_FLAG_SUPERVISOR_MODE	BIT(0)
-
 #endif /* __INTEL_SVM_H__ */
Signature · 1 line
2.25.1