Re: [PATCH RFC 10/11] iommu: Make IOPF handling framework generic
On 2022/3/21 20:50, Jason Gunthorpe wrote:
On Sun, Mar 20, 2022 at 02:40:29PM +0800, Lu Baolu wrote:
+static enum iommu_page_response_code
+iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
+{
+ vm_fault_t ret;
+ struct mm_struct *mm;
+ struct vm_area_struct *vma;
+ unsigned int access_flags = 0;
+ struct iommu_domain *domain = data;
Why is the iommu_domain not passed in as a fully typed object? I would
think data should some opaque value used by non-sva cases.
The "data" is set together with the fault handler when the caller
installs a fault handler for an iommu domain. We will add a generic
interface to install fault handler for an iommu domain later when a real
non-sva case comes.
What is the lifetime model here anyhow?
I simply thought that the device driver should guarantee that there are
no pending faults after sva_unbind(). This is insufficient. I need to
rework this.
+ unsigned int fault_flags = FAULT_FLAG_REMOTE;
+ struct iommu_fault_page_request *prm = >prm;
+ enum iommu_page_response_code status = IOMMU_PAGE_RESP_INVALID;
+
+ if (!(prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID))
+ return status;
+
+ mm = domain->sva_cookie;
+ if (IS_ERR_OR_NULL(mm))
Do not use this function
Do not store err pointers in structs.
Sure.
+out_put_mm:
+ mmap_read_unlock(mm);
+ mmput(mm);
mm structs are weird, they have two refcounts.
The 'sva_cookie' should hold a mmgrab/mmdrop() refcount to keep the
pointer alive but to touch the mmap lock you have to upgrade it to a
refcount that prevents destruction using mmget_not_zero() just for
this short period.
Yes. Will look into it.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 10/11] iommu: Make IOPF handling framework generic
On 2022/3/21 19:39, Jean-Philippe Brucker wrote:
On Sun, Mar 20, 2022 at 02:40:29PM +0800, Lu Baolu wrote:
The existing IOPF handling framework only handles the I/O page faults for
SVA. Ginven that we are able to link iommu domain with each I/O page fault,
we can now make the I/O page fault handling framework more general for
more types of page faults.
Signed-off-by: Lu Baolu
---
include/linux/iommu.h | 4 +++
drivers/iommu/io-pgfault.c| 67 ++-
drivers/iommu/iommu-sva-lib.c | 59 ++
3 files changed, 73 insertions(+), 57 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 803e7b07605e..11c65a7bed88 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -50,6 +50,8 @@ struct iommu_dma_cookie;
typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
struct device *, unsigned long, int, void *);
typedef int (*iommu_dev_fault_handler_t)(struct iommu_fault *, void *);
+typedef enum iommu_page_response_code (*iommu_domain_iopf_handler_t)
+ (struct iommu_fault *, void *);
struct iommu_domain_geometry {
dma_addr_t aperture_start; /* First address that can be mapped*/
@@ -101,6 +103,8 @@ struct iommu_domain {
struct iommu_domain_geometry geometry;
struct iommu_dma_cookie *iova_cookie;
struct mm_struct *sva_cookie;
+ iommu_domain_iopf_handler_t fault_handler;
+ void *fault_data;
};
static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 1df8c1dcae77..dad0e40cd8d2 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -69,62 +69,6 @@ static int iopf_complete_group(struct device *dev, struct
iopf_fault *iopf,
return iommu_page_response(dev, );
}
-static enum iommu_page_response_code
-iopf_handle_single(struct iopf_fault *iopf)
-{
- vm_fault_t ret;
- struct mm_struct *mm;
- struct vm_area_struct *vma;
- unsigned int access_flags = 0;
- unsigned int fault_flags = FAULT_FLAG_REMOTE;
- struct iommu_fault_page_request *prm = >fault.prm;
- enum iommu_page_response_code status = IOMMU_PAGE_RESP_INVALID;
-
- if (!(prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID))
- return status;
-
- mm = iommu_sva_find(prm->pasid);
- if (IS_ERR_OR_NULL(mm))
- return status;
-
- mmap_read_lock(mm);
-
- vma = find_extend_vma(mm, prm->addr);
- if (!vma)
- /* Unmapped area */
- goto out_put_mm;
-
- if (prm->perm & IOMMU_FAULT_PERM_READ)
- access_flags |= VM_READ;
-
- if (prm->perm & IOMMU_FAULT_PERM_WRITE) {
- access_flags |= VM_WRITE;
- fault_flags |= FAULT_FLAG_WRITE;
- }
-
- if (prm->perm & IOMMU_FAULT_PERM_EXEC) {
- access_flags |= VM_EXEC;
- fault_flags |= FAULT_FLAG_INSTRUCTION;
- }
-
- if (!(prm->perm & IOMMU_FAULT_PERM_PRIV))
- fault_flags |= FAULT_FLAG_USER;
-
- if (access_flags & ~vma->vm_flags)
- /* Access fault */
- goto out_put_mm;
-
- ret = handle_mm_fault(vma, prm->addr, fault_flags, NULL);
- status = ret & VM_FAULT_ERROR ? IOMMU_PAGE_RESP_INVALID :
- IOMMU_PAGE_RESP_SUCCESS;
-
-out_put_mm:
- mmap_read_unlock(mm);
- mmput(mm);
-
- return status;
-}
-
static void iopf_handle_group(struct work_struct *work)
{
struct iopf_group *group;
@@ -134,12 +78,21 @@ static void iopf_handle_group(struct work_struct *work)
group = container_of(work, struct iopf_group, work);
list_for_each_entry_safe(iopf, next, >faults, list) {
+ struct iommu_domain *domain;
+
+ domain = iommu_get_domain_for_dev_pasid(group->dev,
+ iopf->fault.prm.pasid);
Do we have a guarantee that the domain is not freed while we handle the
fault? We could prevent unbind() while there are pending faults on this
bond. But a refcount on SVA domains could defer freeing, and would also
help with keeping the semantics where bind() returns a single refcounted
bond for any {dev, mm}.
Given that this path is full of circular locking pitfalls, and to keep the
fault handler efficient (well, at least not make it worse), we should
probably keep a getter like iommu_sva_find() that does not require
locking.
Agreed. We need a mechanism to ensure concurrency. I will look into it.
+
+ if (!domain || !domain->fault_handler)
+ status = IOMMU_PAGE_RESP_INVALID;
+
/*
* For the moment, errors are sticky: don't handle subsequent
* faults in the group if there is an error.
*/
if (status
Re: [PATCH RFC 10/11] iommu: Make IOPF handling framework generic
On 2022/3/21 20:43, Jason Gunthorpe wrote: On Mon, Mar 21, 2022 at 11:42:16AM +, Jean-Philippe Brucker wrote: I tend to disagree with that last part. The fault is caused by a specific device accessing shared page tables. We should keep that device information throughout the fault handling, so that we can report it to the driver when things go wrong. SVA faults should never be reported to drivers?? When things go wrong, the corresponding response code will be responded to the device through iommu_page_response(). The hardware should then report the failure to the device driver and the device driver will handle it in the device-specific way. There's no need to propagate the I/O page faults to the device driver in any case. Do I understand it right? Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 09/11] iommu: Add iommu_get_domain_for_dev_pasid()
On 2022/3/21 20:40, Jason Gunthorpe wrote:
On Sun, Mar 20, 2022 at 02:40:28PM +0800, Lu Baolu wrote:
@@ -3098,7 +3101,16 @@ int iommu_attach_device_pasid(struct iommu_domain
*domain,
if (iommu_group_device_count(group) != 1)
goto out_unlock;
+ xa_lock(>pasid_array);
+ curr = __xa_cmpxchg(>pasid_array, pasid, NULL,
+ domain, GFP_KERNEL);
+ xa_unlock(>pasid_array);
+ if (curr)
curr can be an xa_err that should be propogated.
Yes, should check xa_err().
+ goto out_unlock;
+
ret = domain->ops->attach_dev_pasid(domain, dev, pasid);
+ if (ret)
+ xa_erase(>pasid_array, pasid);
out_unlock:
mutex_unlock(>mutex);
@@ -3118,6 +3130,25 @@ void iommu_detach_device_pasid(struct iommu_domain
*domain,
mutex_lock(>mutex);
domain->ops->detach_dev_pasid(domain, dev, pasid);
+ xa_erase(>pasid_array, pasid);
+ mutex_unlock(>mutex);
+ iommu_group_put(group);
+}
+
+struct iommu_domain *
+iommu_get_domain_for_dev_pasid(struct device *dev, ioasid_t pasid)
+{
+ struct iommu_domain *domain;
+ struct iommu_group *group;
+
+ group = iommu_group_get(dev);
+ if (!group)
+ return NULL;
+
+ mutex_lock(>mutex);
+ domain = xa_load(>pasid_array, pasid);
mutex_unlock(>mutex);
iommu_group_put(group);
This whole API seems sketchy - what is the lifecycle of the returned
iommu_domain and what prevents it from being concurrently freed after
unlocking?
Agreed. The domain could be used in page fault handling thread, hence
need a mechanism to guarantee the concurrence.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 06/11] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On 2022/3/21 20:05, Jason Gunthorpe wrote: On Sun, Mar 20, 2022 at 02:40:25PM +0800, Lu Baolu wrote: +/** + * iommu_sva_bind_device() - Bind a process address space to a device + * @dev: the device + * @mm: the mm to bind, caller must hold a reference to it + * @drvdata: opaque data pointer to pass to bind callback + * + * Create a bond between device and address space, allowing the device to access + * the mm using the returned PASID. If a bond already exists between @device and + * @mm, it is returned and an additional reference is taken. Caller must call + * iommu_sva_unbind_device() to release each reference. + * + * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to + * initialize the required SVA features. + * + * On error, returns an ERR_PTR value. + */ +struct iommu_sva * +iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata) The drvdata is never used Yes. It is cleaned up in Jacob's series. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 06/11] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On 2022/3/21 19:33, Jean-Philippe Brucker wrote:
On Sun, Mar 20, 2022 at 02:40:25PM +0800, Lu Baolu wrote:
diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
index 106506143896..47cf98e661ff 100644
--- a/drivers/iommu/iommu-sva-lib.c
+++ b/drivers/iommu/iommu-sva-lib.c
@@ -3,6 +3,8 @@
* Helpers for IOMMU drivers implementing SVA
*/
#include
+#include
+#include
#include
#include "iommu-sva-lib.h"
@@ -69,3 +71,101 @@ struct mm_struct *iommu_sva_find(ioasid_t pasid)
return ioasid_find(_sva_pasid, pasid, __mmget_not_zero);
}
EXPORT_SYMBOL_GPL(iommu_sva_find);
+
+static struct iommu_domain *iommu_sva_domain_alloc(struct device *dev)
+{
+ struct bus_type *bus = dev->bus;
+ struct iommu_domain *domain;
+
+ if (!bus || !bus->iommu_ops)
+ return NULL;
+
+ domain = bus->iommu_ops->domain_alloc(IOMMU_DOMAIN_SVA);
+ if (domain)
+ domain->type = IOMMU_DOMAIN_SVA;
+
+ return domain;
+}
+
+/**
+ * iommu_sva_bind_device() - Bind a process address space to a device
+ * @dev: the device
+ * @mm: the mm to bind, caller must hold a reference to it
+ * @drvdata: opaque data pointer to pass to bind callback
+ *
+ * Create a bond between device and address space, allowing the device to
access
+ * the mm using the returned PASID. If a bond already exists between @device
and
+ * @mm, it is returned and an additional reference is taken.
This is not true anymore, we return a different structure for each call.
Caller must call
+ * iommu_sva_unbind_device() to release each reference.
+ *
+ * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to
+ * initialize the required SVA features.
+ *
+ * On error, returns an ERR_PTR value.
+ */
+struct iommu_sva *
+iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
+{
+ int ret = -EINVAL;
+ struct iommu_sva *handle;
+ struct iommu_domain *domain;
+
+ handle = kzalloc(sizeof(*handle), GFP_KERNEL);
+ if (!handle)
+ return ERR_PTR(-ENOMEM);
+
+ ret = iommu_sva_alloc_pasid(mm, 1, (1U << dev->iommu->pasid_bits) - 1);
+ if (ret)
+ goto out;
+
+ domain = iommu_sva_domain_alloc(dev);
+ if (!domain) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ domain->sva_cookie = mm;
+
+ ret = iommu_attach_device_pasid(domain, dev, mm->pasid);
+ if (ret)
+ goto out_free_domain;
+
+ handle->dev = dev;
+ handle->domain = domain;
+ handle->pasid = mm->pasid;
+
+ return handle;
+
+out_free_domain:
+ iommu_domain_free(domain);
+out:
+ kfree(handle);
+
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(iommu_sva_bind_device);
+
+/**
+ * iommu_sva_unbind_device() - Remove a bond created with iommu_sva_bind_device
+ * @handle: the handle returned by iommu_sva_bind_device()
+ *
+ * Put reference to a bond between device and address space.
Same here. But I'd prefer keeping the old behavior so device drivers don't
have to keep track of {dev, mm} pairs themselves.
Okay. Thank you for pointing this out. Let me figure it out in the next
version.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 04/11] iommu/vt-d: Add SVA domain support
On 2022/3/21 19:56, Jason Gunthorpe wrote:
On Sun, Mar 20, 2022 at 02:40:23PM +0800, Lu Baolu wrote:
Add support for SVA domain allocation and provide an SVA-specific
iommu_domain_ops.
Signed-off-by: Lu Baolu
include/linux/intel-iommu.h | 1 +
drivers/iommu/intel/iommu.c | 12
drivers/iommu/intel/svm.c | 34 ++
3 files changed, 47 insertions(+)
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 2f9891cb3d00..c14283137fb5 100644
+++ b/include/linux/intel-iommu.h
@@ -744,6 +744,7 @@ void intel_svm_unbind(struct iommu_sva *handle);
u32 intel_svm_get_pasid(struct iommu_sva *handle);
int intel_svm_page_response(struct device *dev, struct iommu_fault_event *evt,
struct iommu_page_response *msg);
+extern const struct iommu_domain_ops intel_svm_domain_ops;
struct intel_svm_dev {
struct list_head list;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index c1b91bce1530..d55dca3eacf8 100644
+++ b/drivers/iommu/intel/iommu.c
@@ -4318,6 +4318,18 @@ static struct iommu_domain
*intel_iommu_domain_alloc(unsigned type)
return domain;
case IOMMU_DOMAIN_IDENTITY:
return _domain->domain;
+#ifdef CONFIG_INTEL_IOMMU_SVM
+ case IOMMU_DOMAIN_SVA:
+ dmar_domain = alloc_domain(type);
+ if (!dmar_domain) {
+ pr_err("Can't allocate sva domain\n");
Don't put random pr_err's/etc in drivers. At least try to use dev_err
+ return NULL;
+ }
+ domain = _domain->domain;
+ domain->ops = _svm_domain_ops;
+
+ return domain;
+#endif /* CONFIG_INTEL_IOMMU_SVM */
default:
return NULL;
}
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index ee5ecde5b318..b9f4dd7057d1 100644
+++ b/drivers/iommu/intel/svm.c
@@ -932,3 +932,37 @@ int intel_svm_page_response(struct device *dev,
mutex_unlock(_mutex);
return ret;
}
+
+static int intel_svm_attach_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ struct device_domain_info *info = dev_iommu_priv_get(dev);
+ struct mm_struct *mm = domain->sva_cookie;
+ struct intel_iommu *iommu = info->iommu;
+ struct iommu_sva *sva;
+
+ mutex_lock(_mutex);
+ sva = intel_svm_bind_mm(iommu, dev, mm);
+ mutex_unlock(_mutex);
+
+ return IS_ERR_OR_NULL(sva);
Never use IS_ERR_OR_NULL(), fix whatever is wrong in intel_svm_bind_mm()
that it can return NULL on failure.
+const struct iommu_domain_ops intel_svm_domain_ops = {
+ .attach_dev_pasid = intel_svm_attach_dev_pasid,
+ .detach_dev_pasid = intel_svm_detach_dev_pasid,
Lets have consistent language either this is called SVA or SVM but not
both.
Thanks a lot for above comments. All make sense to me.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC 10/11] iommu: Make IOPF handling framework generic
> From: Jean-Philippe Brucker > Sent: Monday, March 21, 2022 7:42 PM > > Hi Kevin, > > On Mon, Mar 21, 2022 at 08:09:36AM +, Tian, Kevin wrote: > > > From: Lu Baolu > > > Sent: Sunday, March 20, 2022 2:40 PM > > > > > > The existing IOPF handling framework only handles the I/O page faults for > > > SVA. Ginven that we are able to link iommu domain with each I/O page > fault, > > > we can now make the I/O page fault handling framework more general > for > > > more types of page faults. > > > > "make ... generic" in subject line is kind of confusing. Reading this patch > > I > > think you really meant changing from per-device fault handling to per- > domain > > fault handling. This is more accurate in concept since the fault is caused > > by > > the domain page table. > > I tend to disagree with that last part. The fault is caused by a specific > device accessing shared page tables. We should keep that device > information throughout the fault handling, so that we can report it to the > driver when things go wrong. A process can have multiple threads bound to > different devices, they share the same mm so if the driver wanted to > signal a misbehaving thread, similarly to a SEGV on the CPU side, it would > need the device information to precisely report it to userspace. > iommu driver can include the device information in the fault data. But in concept the IOPF should be reported per domain. and I agree with Jason that at most we can send SEGV to the entire thread group since there is no way to associate a DMA back to a thread which initiates the DMA. Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 02/11] iommu: Add iommu_domain type for SVA
On 2022/3/21 19:47, Jason Gunthorpe wrote:
On Sun, Mar 20, 2022 at 02:40:21PM +0800, Lu Baolu wrote:
Add a new iommu domain type IOMMU_DOMAIN_SVA to represent an I/O page
table which is shared from CPU host VA. Add a sva_cookie field in the
iommu_domain structure to save the mm_struct which represent the CPU
memory page table.
Signed-off-by: Lu Baolu
include/linux/iommu.h | 6 ++
1 file changed, 6 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 36f43af0af53..3e179b853380 100644
+++ b/include/linux/iommu.h
@@ -64,6 +64,9 @@ struct iommu_domain_geometry {
#define __IOMMU_DOMAIN_PT (1U << 2) /* Domain is identity mapped */
#define __IOMMU_DOMAIN_DMA_FQ (1U << 3) /* DMA-API uses flush queue*/
+#define __IOMMU_DOMAIN_SHARED (1U << 4) /* Page table shared from CPU */
+#define __IOMMU_DOMAIN_HOST_VA (1U << 5) /* Host CPU virtual address */
+
/*
* This are the possible domain-types
*
@@ -86,6 +89,8 @@ struct iommu_domain_geometry {
#define IOMMU_DOMAIN_DMA_FQ (__IOMMU_DOMAIN_PAGING |\
__IOMMU_DOMAIN_DMA_API | \
__IOMMU_DOMAIN_DMA_FQ)
+#define IOMMU_DOMAIN_SVA (__IOMMU_DOMAIN_SHARED |\
+__IOMMU_DOMAIN_HOST_VA)
Is there any use for this in the core code? I feel like flags should
only be created if the core code needs to test them in some way.
flags are some attributes of the domain and the combination of multiple
flags forms a domain type.
@@ -95,6 +100,7 @@ struct iommu_domain {
void *handler_token;
struct iommu_domain_geometry geometry;
struct iommu_dma_cookie *iova_cookie;
+ struct mm_struct *sva_cookie;
Don't call a mm_struct a cookie please
Sure.
And why do we need this in core code?
Need to connect an SVA domain with mm.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 01/11] iommu: Add pasid_bits field in struct dev_iommu
On 2022/3/22 8:26, Tian, Kevin wrote:
From: Lu Baolu
Sent: Monday, March 21, 2022 6:22 PM
- if (features >= 0)
+ if (features >= 0) {
info->pasid_supported = features | 1;
+ dev->iommu->pasid_bits =
+ fls(pci_max_pasids(pdev)) - 1;
Original intel_svm_alloc_pasid() covers both PCI and non-PCI devices:
ioasid_t max_pasid = dev_is_pci(dev) ?
pci_max_pasids(to_pci_dev(dev)) : intel_pasid_max_id;
though I'm not sure whether non-PCI SVA has been supported indeed, this
patch implies a functional change here.
The info->pasid_supported is only set for PCI devices. So the status is
that non-PCI SVA hasn't been supported. No functional change here from
this point of view.
Then this information should be included in the commit msg.
Sure.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 01/11] iommu: Add pasid_bits field in struct dev_iommu
On 2022/3/21 19:22, Jean-Philippe Brucker wrote: Hi Baolu, On Sun, Mar 20, 2022 at 02:40:20PM +0800, Lu Baolu wrote: diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 627a3ed5ee8f..8e262210b5ad 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2812,6 +2812,7 @@ static int arm_smmu_dev_enable_feature(struct device *dev, master->iopf_enabled = true; return 0; case IOMMU_DEV_FEAT_SVA: + dev->iommu->pasid_bits = master->ssid_bits; This would be better in arm_smmu_probe_device() Sure. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC 08/11] iommu: Handle IO page faults directly
> From: Jean-Philippe Brucker
> Sent: Monday, March 21, 2022 7:36 PM
>
> On Sun, Mar 20, 2022 at 02:40:27PM +0800, Lu Baolu wrote:
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index c0966fc9b686..4f90b71c6f6e 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -27,6 +27,8 @@
> > #include
> > #include
> >
> > +#include "iommu-sva-lib.h"
> > +
> > static struct kset *iommu_group_kset;
> > static DEFINE_IDA(iommu_group_ida);
> >
> > @@ -1177,10 +1179,9 @@ int iommu_report_device_fault(struct device
> *dev, struct iommu_fault_event *evt)
> > if (!param || !evt)
> > return -EINVAL;
> >
> > - /* we only report device fault if there is a handler registered */
> > mutex_lock(>lock);
> > fparam = param->fault_param;
> > - if (!fparam || !fparam->handler) {
> > + if (!fparam) {
> > ret = -EINVAL;
> > goto done_unlock;
> > }
> > @@ -1198,7 +1199,11 @@ int iommu_report_device_fault(struct device
> *dev, struct iommu_fault_event *evt)
> > mutex_unlock(>lock);
> > }
> >
> > - ret = fparam->handler(>fault, fparam->data);
> > + if (fparam->handler)
> > + ret = fparam->handler(>fault, fparam->data);
> > + else
> > + ret = iommu_queue_iopf(>fault, fparam->data);
> > +
>
> I like the change, but we'll need to consolidate this, because now if the
> driver registers a fault handler it disables IOPF. We could instead
> prevent registration if an IOPF param is present. We could also just merge
> fparam->handler but eventually I'd like to make IOPF fall back to the
> fault handler registered by device driver, in case of invalid page faults.
> I have a couple patches for this but am still missing some bits.
>
Probably we need two different fault reporting interfaces:
iommu_report_device_fault()
iommu_report_domain_fault()
iommu driver knows the type of faults and just calls individual API
accordingly. fallback is also managed by iommu driver if necessary.
Thanks
Kevin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC 01/11] iommu: Add pasid_bits field in struct dev_iommu
> From: Lu Baolu
> Sent: Monday, March 21, 2022 6:22 PM
> >> - if (features >= 0)
> >> + if (features >= 0) {
> >>info->pasid_supported = features | 1;
> >> + dev->iommu->pasid_bits =
> >> + fls(pci_max_pasids(pdev)) - 1;
> > Original intel_svm_alloc_pasid() covers both PCI and non-PCI devices:
> >
> > ioasid_t max_pasid = dev_is_pci(dev) ?
> > pci_max_pasids(to_pci_dev(dev)) : intel_pasid_max_id;
> >
> > though I'm not sure whether non-PCI SVA has been supported indeed, this
> > patch implies a functional change here.
> >
>
> The info->pasid_supported is only set for PCI devices. So the status is
> that non-PCI SVA hasn't been supported. No functional change here from
> this point of view.
>
Then this information should be included in the commit msg.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 14/32] iommu: introduce iommu_domain_alloc_type and the KVM type
On Sat, Mar 19, 2022 at 07:51:31AM +, Tian, Kevin wrote: > > From: Jason Gunthorpe > > Sent: Friday, March 18, 2022 10:13 PM > > > > On Fri, Mar 18, 2022 at 02:23:57AM +, Tian, Kevin wrote: > > > > > Yes, that is another major part work besides the iommufd work. And > > > it is not compatible with KVM features which rely on the dynamic > > > manner of EPT. Though It is a bit questionable whether it's worthy of > > > doing so just for saving memory footprint while losing other capabilities, > > > it is a requirement for some future security extension in Intel trusted > > > computing architecture. And KVM has been pinning pages for SEV/TDX/etc. > > > today thus some facilities can be reused. But I agree it is not a simple > > > task thus we need start discussion early to explore various gaps in > > > iommu and kvm. > > > > Yikes. IMHO this might work better going the other way, have KVM > > import the iommu_domain and use that as the KVM page table than vice > > versa. > > > > The semantics are a heck of a lot clearer, and it is really obvious > > that alot of KVM becomes disabled if you do this. > > > > This is an interesting angle to look at it. But given pinning is already > required in KVM to support SEV/TDX even w/o assigned device, those > restrictions have to be understood by KVM MMU code which makes > a KVM-managed page table under such restrictions closer to be > sharable with IOMMU. I thought the SEV/TDX stuff wasn't being done with pinning but via a memfd in a special mode that does sort of pin under the covers, but it is not necessarily a DMA pin. (it isn't even struct page memory, so I'm not even sure what pin means) Certainly, there is no inherent problem with SEV/TDX having movable memory and KVM could concievably handle this - but iommu cannot. I would not make an equivilance with SEV/TDX and iommu at least.. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2 2/2] thunderbolt: Make iommu_dma_protection more accurate
[Public] > -Original Message- > From: Robin Murphy > Sent: Monday, March 21, 2022 06:12 > To: mika.westerb...@linux.intel.com; Limonciello, Mario > > Cc: j...@8bytes.org; baolu...@linux.intel.com; andreas.noe...@gmail.com; > michael.ja...@intel.com; yehezkel...@gmail.com; iommu@lists.linux- > foundation.org; linux-...@vger.kernel.org; linux-ker...@vger.kernel.org; > h...@lst.de > Subject: Re: [PATCH v2 2/2] thunderbolt: Make iommu_dma_protection > more accurate > > On 2022-03-21 10:58, mika.westerb...@linux.intel.com wrote: > > Hi Mario, > > > > On Fri, Mar 18, 2022 at 10:29:59PM +, Limonciello, Mario wrote: > >> [Public] > >> > >>> Between me trying to get rid of iommu_present() and Mario wanting to > >>> support the AMD equivalent of DMAR_PLATFORM_OPT_IN, scrutiny has > >>> shown > >>> that the iommu_dma_protection attribute is being far too optimistic. > >>> Even if an IOMMU might be present for some PCI segment in the > system, > >>> that doesn't necessarily mean it provides translation for the device(s) > >>> we care about. Furthermore, all that DMAR_PLATFORM_OPT_IN really > does > >>> is tell us that memory was protected before the kernel was loaded, and > >>> prevent the user from disabling the intel-iommu driver entirely. While > >>> that lets us assume kernel integrity, what matters for actual runtime > >>> DMA protection is whether we trust individual devices, based on the > >>> "external facing" property that we expect firmware to describe for > >>> Thunderbolt ports. > >>> > >>> It's proven challenging to determine the appropriate ports accurately > >>> given the variety of possible topologies, so while still not getting a > >>> perfect answer, by putting enough faith in firmware we can at least get > >>> a good bit closer. If we can see that any device near a Thunderbolt NHI > >>> has all the requisites for Kernel DMA Protection, chances are that it > >>> *is* a relevant port, but moreover that implies that firmware is playing > >>> the game overall, so we'll use that to assume that all Thunderbolt ports > >>> should be correctly marked and thus will end up fully protected. > >>> > >> > >> This approach looks generally good to me. I do worry a little bit about > older > >> systems that didn't set ExternalFacingPort in the FW but were previously > setting > >> iommu_dma_protection, but I think that those could be treated on a > quirk > >> basis to set PCI IDs for those root ports as external facing if/when they > come > >> up. > > > > There are no such systems out there AFAICT. > > And even if there are, as above they've never actually been fully > protected and still won't be, so it's arguably a good thing for them to > stop thinking so. I was meaning that if this situation comes up that we could tag the PCI IDs for those root ports as ExternalFacing in drivers/pci/quirks.c so that the protection "is" enacted for them even if it was missing from the firmware. > > >> I'll send up a follow up patch that adds the AMD ACPI table check. > >> If it looks good can roll it into your series for v3, or if this series > >> goes > >> as is for v2 it can come on its own. > >> > >>> CC: Mario Limonciello > >>> Signed-off-by: Robin Murphy > >>> --- > >>> > >>> v2: Give up trying to look for specific devices, just look for evidence > >>> that firmware cares at all. > >> > >> I still do think you could know exactly which devices to use if you're in > >> SW CM mode, but I guess the consensus is to not bifurcate the way this > >> can be checked. > > > > Indeed. > > > > The patch looks good to me now. I will give it a try on a couple of > > systems later today or tomorrow and let you guys know how it went. I > > don't expect any problems but let's see. > > > > Thanks a lot Robin for working on this :) > > Heh, let's just hope the other half-dozen or so subsystems I need to > touch for this IOMMU cleanup aren't all quite as involved as this turned > out to be :) Thanks a lot for this effort! > > Cheers, > Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 10/11] iommu: Make IOPF handling framework generic
On Sun, Mar 20, 2022 at 02:40:29PM +0800, Lu Baolu wrote:
> +static enum iommu_page_response_code
> +iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
> +{
> + vm_fault_t ret;
> + struct mm_struct *mm;
> + struct vm_area_struct *vma;
> + unsigned int access_flags = 0;
> + struct iommu_domain *domain = data;
Why is the iommu_domain not passed in as a fully typed object? I would
think data should some opaque value used by non-sva cases.
What is the lifetime model here anyhow?
> + unsigned int fault_flags = FAULT_FLAG_REMOTE;
> + struct iommu_fault_page_request *prm = >prm;
> + enum iommu_page_response_code status = IOMMU_PAGE_RESP_INVALID;
> +
> + if (!(prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID))
> + return status;
> +
> + mm = domain->sva_cookie;
> + if (IS_ERR_OR_NULL(mm))
Do not use this function
Do not store err pointers in structs.
> +out_put_mm:
> + mmap_read_unlock(mm);
> + mmput(mm);
mm structs are weird, they have two refcounts.
The 'sva_cookie' should hold a mmgrab/mmdrop() refcount to keep the
pointer alive but to touch the mmap lock you have to upgrade it to a
refcount that prevents destruction using mmget_not_zero() just for
this short period.
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 10/11] iommu: Make IOPF handling framework generic
On Mon, Mar 21, 2022 at 11:42:16AM +, Jean-Philippe Brucker wrote: > I tend to disagree with that last part. The fault is caused by a specific > device accessing shared page tables. We should keep that device > information throughout the fault handling, so that we can report it to the > driver when things go wrong. SVA faults should never be reported to drivers?? > A process can have multiple threads bound to different devices, they > share the same mm so if the driver wanted to signal a misbehaving > thread, similarly to a SEGV on the CPU side, it would need the > device information to precisely report it to userspace. I'm not sure I understand this - we can't match DMAs to executing CPUs. On fault we fail the DMA and let the process keep running or SIGSEGV the whole thread group. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 09/11] iommu: Add iommu_get_domain_for_dev_pasid()
On Sun, Mar 20, 2022 at 02:40:28PM +0800, Lu Baolu wrote:
> @@ -3098,7 +3101,16 @@ int iommu_attach_device_pasid(struct iommu_domain
> *domain,
> if (iommu_group_device_count(group) != 1)
> goto out_unlock;
>
> + xa_lock(>pasid_array);
> + curr = __xa_cmpxchg(>pasid_array, pasid, NULL,
> + domain, GFP_KERNEL);
> + xa_unlock(>pasid_array);
> + if (curr)
curr can be an xa_err that should be propogated.
> + goto out_unlock;
> +
> ret = domain->ops->attach_dev_pasid(domain, dev, pasid);
> + if (ret)
> + xa_erase(>pasid_array, pasid);
>
> out_unlock:
> mutex_unlock(>mutex);
> @@ -3118,6 +3130,25 @@ void iommu_detach_device_pasid(struct iommu_domain
> *domain,
>
> mutex_lock(>mutex);
> domain->ops->detach_dev_pasid(domain, dev, pasid);
> + xa_erase(>pasid_array, pasid);
> + mutex_unlock(>mutex);
> + iommu_group_put(group);
> +}
> +
> +struct iommu_domain *
> +iommu_get_domain_for_dev_pasid(struct device *dev, ioasid_t pasid)
> +{
> + struct iommu_domain *domain;
> + struct iommu_group *group;
> +
> + group = iommu_group_get(dev);
> + if (!group)
> + return NULL;
> +
> + mutex_lock(>mutex);
> + domain = xa_load(>pasid_array, pasid);
> mutex_unlock(>mutex);
> iommu_group_put(group);
This whole API seems sketchy - what is the lifecycle of the returned
iommu_domain and what prevents it from being concurrently freed after
unlocking?
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 06/11] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On Sun, Mar 20, 2022 at 02:40:25PM +0800, Lu Baolu wrote: > +/** > + * iommu_sva_bind_device() - Bind a process address space to a device > + * @dev: the device > + * @mm: the mm to bind, caller must hold a reference to it > + * @drvdata: opaque data pointer to pass to bind callback > + * > + * Create a bond between device and address space, allowing the device to > access > + * the mm using the returned PASID. If a bond already exists between @device > and > + * @mm, it is returned and an additional reference is taken. Caller must call > + * iommu_sva_unbind_device() to release each reference. > + * > + * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to > + * initialize the required SVA features. > + * > + * On error, returns an ERR_PTR value. > + */ > +struct iommu_sva * > +iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void > *drvdata) The drvdata is never used Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 06/11] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On Mon, Mar 21, 2022 at 07:01:45PM +0800, Lu Baolu wrote: > > one domain can be attached by multiple devices, so this should not be > > a blind alloc. > > Indeed. Perhaps we could associate the SVA domain with the mm->pasid and > add a user counter inside the domain. This has the same problem as VFIO it needs to keep a list of automatically created iommu domains in the mm_struct and try to re-use them every time a device is bound. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 05/11] arm-smmu-v3/sva: Add SVA domain support
On Mon, Mar 21, 2022 at 11:31:19AM +, Jean-Philippe Brucker wrote: > For now we could just return a naked struct iommu_domain. Sanity checks in > arm_smmu_attach_dev() would be good too, a SVA domain can't be attached as > a parent domain. Now that we have per-domain ops the 'standard' arm_smmu_attach_dev() cannot be called on a SVA iommu_domain already. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 04/11] iommu/vt-d: Add SVA domain support
On Sun, Mar 20, 2022 at 02:40:23PM +0800, Lu Baolu wrote:
> Add support for SVA domain allocation and provide an SVA-specific
> iommu_domain_ops.
>
> Signed-off-by: Lu Baolu
> include/linux/intel-iommu.h | 1 +
> drivers/iommu/intel/iommu.c | 12
> drivers/iommu/intel/svm.c | 34 ++
> 3 files changed, 47 insertions(+)
>
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index 2f9891cb3d00..c14283137fb5 100644
> +++ b/include/linux/intel-iommu.h
> @@ -744,6 +744,7 @@ void intel_svm_unbind(struct iommu_sva *handle);
> u32 intel_svm_get_pasid(struct iommu_sva *handle);
> int intel_svm_page_response(struct device *dev, struct iommu_fault_event
> *evt,
> struct iommu_page_response *msg);
> +extern const struct iommu_domain_ops intel_svm_domain_ops;
>
> struct intel_svm_dev {
> struct list_head list;
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index c1b91bce1530..d55dca3eacf8 100644
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4318,6 +4318,18 @@ static struct iommu_domain
> *intel_iommu_domain_alloc(unsigned type)
> return domain;
> case IOMMU_DOMAIN_IDENTITY:
> return _domain->domain;
> +#ifdef CONFIG_INTEL_IOMMU_SVM
> + case IOMMU_DOMAIN_SVA:
> + dmar_domain = alloc_domain(type);
> + if (!dmar_domain) {
> + pr_err("Can't allocate sva domain\n");
Don't put random pr_err's/etc in drivers. At least try to use dev_err
> + return NULL;
> + }
> + domain = _domain->domain;
> + domain->ops = _svm_domain_ops;
> +
> + return domain;
> +#endif /* CONFIG_INTEL_IOMMU_SVM */
> default:
> return NULL;
> }
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index ee5ecde5b318..b9f4dd7057d1 100644
> +++ b/drivers/iommu/intel/svm.c
> @@ -932,3 +932,37 @@ int intel_svm_page_response(struct device *dev,
> mutex_unlock(_mutex);
> return ret;
> }
> +
> +static int intel_svm_attach_dev_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t pasid)
> +{
> + struct device_domain_info *info = dev_iommu_priv_get(dev);
> + struct mm_struct *mm = domain->sva_cookie;
> + struct intel_iommu *iommu = info->iommu;
> + struct iommu_sva *sva;
> +
> + mutex_lock(_mutex);
> + sva = intel_svm_bind_mm(iommu, dev, mm);
> + mutex_unlock(_mutex);
> +
> + return IS_ERR_OR_NULL(sva);
Never use IS_ERR_OR_NULL(), fix whatever is wrong in intel_svm_bind_mm()
that it can return NULL on failure.
> +const struct iommu_domain_ops intel_svm_domain_ops = {
> + .attach_dev_pasid = intel_svm_attach_dev_pasid,
> + .detach_dev_pasid = intel_svm_detach_dev_pasid,
Lets have consistent language either this is called SVA or SVM but not
both.
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 03/11] iommu: Add attach/detach_dev_pasid domain ops
On Mon, Mar 21, 2022 at 07:13:49AM +, Tian, Kevin wrote: > /* >* To keep things simple, SVA currently doesn't support IOMMU groups >* with more than one device. Existing SVA-capable systems are not >* affected by the problems that required IOMMU groups (lack of ACS >* isolation, device ID aliasing and other hardware issues). >*/ > if (iommu_group_device_count(group) != 1) > goto out_unlock; > > btw I didn't see any safeguard on above assumption in device hotplug path > to a group which already has SVA enabled... This is because using device_count is always wrong for these kinds of tests. The exclusion needs to be built up from the new owner mechanism. Once the device is in SVA mode it is the same as having exclusive ownership against any other probes() Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 02/11] iommu: Add iommu_domain type for SVA
On Sun, Mar 20, 2022 at 02:40:21PM +0800, Lu Baolu wrote:
> Add a new iommu domain type IOMMU_DOMAIN_SVA to represent an I/O page
> table which is shared from CPU host VA. Add a sva_cookie field in the
> iommu_domain structure to save the mm_struct which represent the CPU
> memory page table.
>
> Signed-off-by: Lu Baolu
> include/linux/iommu.h | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 36f43af0af53..3e179b853380 100644
> +++ b/include/linux/iommu.h
> @@ -64,6 +64,9 @@ struct iommu_domain_geometry {
> #define __IOMMU_DOMAIN_PT(1U << 2) /* Domain is identity mapped */
> #define __IOMMU_DOMAIN_DMA_FQ(1U << 3) /* DMA-API uses flush queue
> */
>
> +#define __IOMMU_DOMAIN_SHARED(1U << 4) /* Page table shared from
> CPU */
> +#define __IOMMU_DOMAIN_HOST_VA (1U << 5) /* Host CPU virtual address
> */
> +
> /*
> * This are the possible domain-types
> *
> @@ -86,6 +89,8 @@ struct iommu_domain_geometry {
> #define IOMMU_DOMAIN_DMA_FQ (__IOMMU_DOMAIN_PAGING |\
>__IOMMU_DOMAIN_DMA_API | \
>__IOMMU_DOMAIN_DMA_FQ)
> +#define IOMMU_DOMAIN_SVA (__IOMMU_DOMAIN_SHARED |\
> + __IOMMU_DOMAIN_HOST_VA)
Is there any use for this in the core code? I feel like flags should
only be created if the core code needs to test them in some way.
> @@ -95,6 +100,7 @@ struct iommu_domain {
> void *handler_token;
> struct iommu_domain_geometry geometry;
> struct iommu_dma_cookie *iova_cookie;
> + struct mm_struct *sva_cookie;
Don't call a mm_struct a cookie please
And why do we need this in core code?
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 10/11] iommu: Make IOPF handling framework generic
Hi Kevin, On Mon, Mar 21, 2022 at 08:09:36AM +, Tian, Kevin wrote: > > From: Lu Baolu > > Sent: Sunday, March 20, 2022 2:40 PM > > > > The existing IOPF handling framework only handles the I/O page faults for > > SVA. Ginven that we are able to link iommu domain with each I/O page fault, > > we can now make the I/O page fault handling framework more general for > > more types of page faults. > > "make ... generic" in subject line is kind of confusing. Reading this patch I > think you really meant changing from per-device fault handling to per-domain > fault handling. This is more accurate in concept since the fault is caused by > the domain page table. I tend to disagree with that last part. The fault is caused by a specific device accessing shared page tables. We should keep that device information throughout the fault handling, so that we can report it to the driver when things go wrong. A process can have multiple threads bound to different devices, they share the same mm so if the driver wanted to signal a misbehaving thread, similarly to a SEGV on the CPU side, it would need the device information to precisely report it to userspace. Thanks, Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 10/11] iommu: Make IOPF handling framework generic
On Sun, Mar 20, 2022 at 02:40:29PM +0800, Lu Baolu wrote:
> The existing IOPF handling framework only handles the I/O page faults for
> SVA. Ginven that we are able to link iommu domain with each I/O page fault,
> we can now make the I/O page fault handling framework more general for
> more types of page faults.
>
> Signed-off-by: Lu Baolu
> ---
> include/linux/iommu.h | 4 +++
> drivers/iommu/io-pgfault.c| 67 ++-
> drivers/iommu/iommu-sva-lib.c | 59 ++
> 3 files changed, 73 insertions(+), 57 deletions(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 803e7b07605e..11c65a7bed88 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -50,6 +50,8 @@ struct iommu_dma_cookie;
> typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
> struct device *, unsigned long, int, void *);
> typedef int (*iommu_dev_fault_handler_t)(struct iommu_fault *, void *);
> +typedef enum iommu_page_response_code (*iommu_domain_iopf_handler_t)
> + (struct iommu_fault *, void *);
>
> struct iommu_domain_geometry {
> dma_addr_t aperture_start; /* First address that can be mapped*/
> @@ -101,6 +103,8 @@ struct iommu_domain {
> struct iommu_domain_geometry geometry;
> struct iommu_dma_cookie *iova_cookie;
> struct mm_struct *sva_cookie;
> + iommu_domain_iopf_handler_t fault_handler;
> + void *fault_data;
> };
>
> static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> index 1df8c1dcae77..dad0e40cd8d2 100644
> --- a/drivers/iommu/io-pgfault.c
> +++ b/drivers/iommu/io-pgfault.c
> @@ -69,62 +69,6 @@ static int iopf_complete_group(struct device *dev, struct
> iopf_fault *iopf,
> return iommu_page_response(dev, );
> }
>
> -static enum iommu_page_response_code
> -iopf_handle_single(struct iopf_fault *iopf)
> -{
> - vm_fault_t ret;
> - struct mm_struct *mm;
> - struct vm_area_struct *vma;
> - unsigned int access_flags = 0;
> - unsigned int fault_flags = FAULT_FLAG_REMOTE;
> - struct iommu_fault_page_request *prm = >fault.prm;
> - enum iommu_page_response_code status = IOMMU_PAGE_RESP_INVALID;
> -
> - if (!(prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID))
> - return status;
> -
> - mm = iommu_sva_find(prm->pasid);
> - if (IS_ERR_OR_NULL(mm))
> - return status;
> -
> - mmap_read_lock(mm);
> -
> - vma = find_extend_vma(mm, prm->addr);
> - if (!vma)
> - /* Unmapped area */
> - goto out_put_mm;
> -
> - if (prm->perm & IOMMU_FAULT_PERM_READ)
> - access_flags |= VM_READ;
> -
> - if (prm->perm & IOMMU_FAULT_PERM_WRITE) {
> - access_flags |= VM_WRITE;
> - fault_flags |= FAULT_FLAG_WRITE;
> - }
> -
> - if (prm->perm & IOMMU_FAULT_PERM_EXEC) {
> - access_flags |= VM_EXEC;
> - fault_flags |= FAULT_FLAG_INSTRUCTION;
> - }
> -
> - if (!(prm->perm & IOMMU_FAULT_PERM_PRIV))
> - fault_flags |= FAULT_FLAG_USER;
> -
> - if (access_flags & ~vma->vm_flags)
> - /* Access fault */
> - goto out_put_mm;
> -
> - ret = handle_mm_fault(vma, prm->addr, fault_flags, NULL);
> - status = ret & VM_FAULT_ERROR ? IOMMU_PAGE_RESP_INVALID :
> - IOMMU_PAGE_RESP_SUCCESS;
> -
> -out_put_mm:
> - mmap_read_unlock(mm);
> - mmput(mm);
> -
> - return status;
> -}
> -
> static void iopf_handle_group(struct work_struct *work)
> {
> struct iopf_group *group;
> @@ -134,12 +78,21 @@ static void iopf_handle_group(struct work_struct *work)
> group = container_of(work, struct iopf_group, work);
>
> list_for_each_entry_safe(iopf, next, >faults, list) {
> + struct iommu_domain *domain;
> +
> + domain = iommu_get_domain_for_dev_pasid(group->dev,
> + iopf->fault.prm.pasid);
Do we have a guarantee that the domain is not freed while we handle the
fault? We could prevent unbind() while there are pending faults on this
bond. But a refcount on SVA domains could defer freeing, and would also
help with keeping the semantics where bind() returns a single refcounted
bond for any {dev, mm}.
Given that this path is full of circular locking pitfalls, and to keep the
fault handler efficient (well, at least not make it worse), we should
probably keep a getter like iommu_sva_find() that does not require
locking.
> +
> + if (!domain || !domain->fault_handler)
> + status = IOMMU_PAGE_RESP_INVALID;
> +
> /*
>* For the moment, errors are sticky: don't handle subsequent
>* faults in the group if there is an error.
>*/
> if (status ==
Re: [PATCH RFC 08/11] iommu: Handle IO page faults directly
On Sun, Mar 20, 2022 at 02:40:27PM +0800, Lu Baolu wrote:
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index c0966fc9b686..4f90b71c6f6e 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -27,6 +27,8 @@
> #include
> #include
>
> +#include "iommu-sva-lib.h"
> +
> static struct kset *iommu_group_kset;
> static DEFINE_IDA(iommu_group_ida);
>
> @@ -1177,10 +1179,9 @@ int iommu_report_device_fault(struct device *dev,
> struct iommu_fault_event *evt)
> if (!param || !evt)
> return -EINVAL;
>
> - /* we only report device fault if there is a handler registered */
> mutex_lock(>lock);
> fparam = param->fault_param;
> - if (!fparam || !fparam->handler) {
> + if (!fparam) {
> ret = -EINVAL;
> goto done_unlock;
> }
> @@ -1198,7 +1199,11 @@ int iommu_report_device_fault(struct device *dev,
> struct iommu_fault_event *evt)
> mutex_unlock(>lock);
> }
>
> - ret = fparam->handler(>fault, fparam->data);
> + if (fparam->handler)
> + ret = fparam->handler(>fault, fparam->data);
> + else
> + ret = iommu_queue_iopf(>fault, fparam->data);
> +
I like the change, but we'll need to consolidate this, because now if the
driver registers a fault handler it disables IOPF. We could instead
prevent registration if an IOPF param is present. We could also just merge
fparam->handler but eventually I'd like to make IOPF fall back to the
fault handler registered by device driver, in case of invalid page faults.
I have a couple patches for this but am still missing some bits.
Thanks,
Jean
> if (ret && evt_pending) {
> mutex_lock(>lock);
> list_del(_pending->list);
> --
> 2.25.1
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 06/11] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On Sun, Mar 20, 2022 at 02:40:25PM +0800, Lu Baolu wrote:
> diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
> index 106506143896..47cf98e661ff 100644
> --- a/drivers/iommu/iommu-sva-lib.c
> +++ b/drivers/iommu/iommu-sva-lib.c
> @@ -3,6 +3,8 @@
> * Helpers for IOMMU drivers implementing SVA
> */
> #include
> +#include
> +#include
> #include
>
> #include "iommu-sva-lib.h"
> @@ -69,3 +71,101 @@ struct mm_struct *iommu_sva_find(ioasid_t pasid)
> return ioasid_find(_sva_pasid, pasid, __mmget_not_zero);
> }
> EXPORT_SYMBOL_GPL(iommu_sva_find);
> +
> +static struct iommu_domain *iommu_sva_domain_alloc(struct device *dev)
> +{
> + struct bus_type *bus = dev->bus;
> + struct iommu_domain *domain;
> +
> + if (!bus || !bus->iommu_ops)
> + return NULL;
> +
> + domain = bus->iommu_ops->domain_alloc(IOMMU_DOMAIN_SVA);
> + if (domain)
> + domain->type = IOMMU_DOMAIN_SVA;
> +
> + return domain;
> +}
> +
> +/**
> + * iommu_sva_bind_device() - Bind a process address space to a device
> + * @dev: the device
> + * @mm: the mm to bind, caller must hold a reference to it
> + * @drvdata: opaque data pointer to pass to bind callback
> + *
> + * Create a bond between device and address space, allowing the device to
> access
> + * the mm using the returned PASID. If a bond already exists between @device
> and
> + * @mm, it is returned and an additional reference is taken.
This is not true anymore, we return a different structure for each call.
> Caller must call
> + * iommu_sva_unbind_device() to release each reference.
> + *
> + * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to
> + * initialize the required SVA features.
> + *
> + * On error, returns an ERR_PTR value.
> + */
> +struct iommu_sva *
> +iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void
> *drvdata)
> +{
> + int ret = -EINVAL;
> + struct iommu_sva *handle;
> + struct iommu_domain *domain;
> +
> + handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> + if (!handle)
> + return ERR_PTR(-ENOMEM);
> +
> + ret = iommu_sva_alloc_pasid(mm, 1, (1U << dev->iommu->pasid_bits) - 1);
> + if (ret)
> + goto out;
> +
> + domain = iommu_sva_domain_alloc(dev);
> + if (!domain) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + domain->sva_cookie = mm;
> +
> + ret = iommu_attach_device_pasid(domain, dev, mm->pasid);
> + if (ret)
> + goto out_free_domain;
> +
> + handle->dev = dev;
> + handle->domain = domain;
> + handle->pasid = mm->pasid;
> +
> + return handle;
> +
> +out_free_domain:
> + iommu_domain_free(domain);
> +out:
> + kfree(handle);
> +
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(iommu_sva_bind_device);
> +
> +/**
> + * iommu_sva_unbind_device() - Remove a bond created with
> iommu_sva_bind_device
> + * @handle: the handle returned by iommu_sva_bind_device()
> + *
> + * Put reference to a bond between device and address space.
Same here. But I'd prefer keeping the old behavior so device drivers don't
have to keep track of {dev, mm} pairs themselves.
Thanks,
Jean
> The device should
> + * not be issuing any more transaction for this PASID. All outstanding page
> + * requests for this PASID must have been flushed to the IOMMU.
> + */
> +void iommu_sva_unbind_device(struct iommu_sva *handle)
> +{
> + struct device *dev = handle->dev;
> + struct iommu_domain *domain = handle->domain;
> + struct mm_struct *mm = domain->sva_cookie;
> +
> + iommu_detach_device_pasid(domain, dev, mm->pasid);
> + iommu_domain_free(domain);
> + kfree(handle);
> +}
> +EXPORT_SYMBOL_GPL(iommu_sva_unbind_device);
> +
> +u32 iommu_sva_get_pasid(struct iommu_sva *handle)
> +{
> + return handle->pasid;
> +}
> +EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 05/11] arm-smmu-v3/sva: Add SVA domain support
On Sun, Mar 20, 2022 at 02:40:24PM +0800, Lu Baolu wrote:
> Add support for SVA domain allocation and provide an SVA-specific
> iommu_domain_ops.
>
> Signed-off-by: Lu Baolu
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 14 ++
> .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 45 +++
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 13 +-
> 3 files changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index cd48590ada30..7631c00fdcbd 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -759,6 +759,10 @@ struct iommu_sva *arm_smmu_sva_bind(struct device *dev,
> struct mm_struct *mm,
> void arm_smmu_sva_unbind(struct iommu_sva *handle);
> u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle);
> void arm_smmu_sva_notifier_synchronize(void);
> +int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t id);
> +void arm_smmu_sva_detach_dev_pasid(struct iommu_domain *domain,
> +struct device *dev, ioasid_t id);
> #else /* CONFIG_ARM_SMMU_V3_SVA */
> static inline bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
> {
> @@ -804,5 +808,15 @@ static inline u32 arm_smmu_sva_get_pasid(struct
> iommu_sva *handle)
> }
>
> static inline void arm_smmu_sva_notifier_synchronize(void) {}
> +
> +static inline int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t id)
> +{
> + return -ENODEV;
> +}
> +
> +static inline void arm_smmu_sva_detach_dev_pasid(struct iommu_domain *domain,
> + struct device *dev,
> + ioasid_t id) {}
> #endif /* CONFIG_ARM_SMMU_V3_SVA */
> #endif /* _ARM_SMMU_V3_H */
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> index 22ddd05bbdcd..1e114b9dc17f 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -534,3 +534,48 @@ void arm_smmu_sva_notifier_synchronize(void)
>*/
> mmu_notifier_synchronize();
> }
> +
> +int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t id)
> +{
> + int ret = 0;
> + struct iommu_sva *handle;
> + struct mm_struct *mm = domain->sva_cookie;
> + struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> +
> + if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1 ||
This check is for the parent domain, iommu_get_domain_for_dev(dev)
> + domain->type != IOMMU_DOMAIN_SVA || !mm)
> + return -EINVAL;
> +
> + mutex_lock(_lock);
> + handle = __arm_smmu_sva_bind(dev, mm);
> + if (IS_ERR_OR_NULL(handle))
> + ret = PTR_ERR(handle);
> + mutex_unlock(_lock);
> +
> + return ret;
> +}
> +
> +void arm_smmu_sva_detach_dev_pasid(struct iommu_domain *domain,
> +struct device *dev, ioasid_t id)
> +{
> + struct arm_smmu_bond *bond = NULL, *t;
> + struct mm_struct *mm = domain->sva_cookie;
> + struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> +
> + mutex_lock(_lock);
> + list_for_each_entry(t, >bonds, list) {
> + if (t->mm == mm) {
> + bond = t;
> + break;
> + }
> + }
> +
> + if (!WARN_ON(!bond) && refcount_dec_and_test(>refs)) {
> + list_del(>list);
> + arm_smmu_mmu_notifier_put(bond->smmu_mn);
> + iommu_sva_free_pasid(bond->mm);
Can be dropped since iommu.c does PASID allocation (also the one in
__arm_smmu_sva_bind() as a cleanup patch)
> + kfree(bond);
> + }
> + mutex_unlock(_lock);
> +}
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 8e262210b5ad..2e9d3cd30510 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -88,6 +88,8 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
> { 0, NULL},
> };
>
> +static void arm_smmu_domain_free(struct iommu_domain *domain);
> +
> static void parse_driver_options(struct arm_smmu_device *smmu)
> {
> int i = 0;
> @@ -1995,6 +1997,12 @@ static bool arm_smmu_capable(enum iommu_cap cap)
> }
> }
>
> +static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
> + .attach_dev_pasid = arm_smmu_sva_attach_dev_pasid,
> + .detach_dev_pasid = arm_smmu_sva_detach_dev_pasid,
> + .free = arm_smmu_domain_free,
> +};
> +
> static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> {
>
Re: [PATCH RFC 01/11] iommu: Add pasid_bits field in struct dev_iommu
Hi Baolu, On Sun, Mar 20, 2022 at 02:40:20PM +0800, Lu Baolu wrote: > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > index 627a3ed5ee8f..8e262210b5ad 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2812,6 +2812,7 @@ static int arm_smmu_dev_enable_feature(struct device > *dev, > master->iopf_enabled = true; > return 0; > case IOMMU_DEV_FEAT_SVA: > + dev->iommu->pasid_bits = master->ssid_bits; This would be better in arm_smmu_probe_device() Thanks, Jean > return arm_smmu_master_enable_sva(master); > default: > return -EINVAL; ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/2] thunderbolt: Make iommu_dma_protection more accurate
On 2022-03-21 10:58, mika.westerb...@linux.intel.com wrote: Hi Mario, On Fri, Mar 18, 2022 at 10:29:59PM +, Limonciello, Mario wrote: [Public] Between me trying to get rid of iommu_present() and Mario wanting to support the AMD equivalent of DMAR_PLATFORM_OPT_IN, scrutiny has shown that the iommu_dma_protection attribute is being far too optimistic. Even if an IOMMU might be present for some PCI segment in the system, that doesn't necessarily mean it provides translation for the device(s) we care about. Furthermore, all that DMAR_PLATFORM_OPT_IN really does is tell us that memory was protected before the kernel was loaded, and prevent the user from disabling the intel-iommu driver entirely. While that lets us assume kernel integrity, what matters for actual runtime DMA protection is whether we trust individual devices, based on the "external facing" property that we expect firmware to describe for Thunderbolt ports. It's proven challenging to determine the appropriate ports accurately given the variety of possible topologies, so while still not getting a perfect answer, by putting enough faith in firmware we can at least get a good bit closer. If we can see that any device near a Thunderbolt NHI has all the requisites for Kernel DMA Protection, chances are that it *is* a relevant port, but moreover that implies that firmware is playing the game overall, so we'll use that to assume that all Thunderbolt ports should be correctly marked and thus will end up fully protected. This approach looks generally good to me. I do worry a little bit about older systems that didn't set ExternalFacingPort in the FW but were previously setting iommu_dma_protection, but I think that those could be treated on a quirk basis to set PCI IDs for those root ports as external facing if/when they come up. There are no such systems out there AFAICT. And even if there are, as above they've never actually been fully protected and still won't be, so it's arguably a good thing for them to stop thinking so. I'll send up a follow up patch that adds the AMD ACPI table check. If it looks good can roll it into your series for v3, or if this series goes as is for v2 it can come on its own. CC: Mario Limonciello Signed-off-by: Robin Murphy --- v2: Give up trying to look for specific devices, just look for evidence that firmware cares at all. I still do think you could know exactly which devices to use if you're in SW CM mode, but I guess the consensus is to not bifurcate the way this can be checked. Indeed. The patch looks good to me now. I will give it a try on a couple of systems later today or tomorrow and let you guys know how it went. I don't expect any problems but let's see. Thanks a lot Robin for working on this :) Heh, let's just hope the other half-dozen or so subsystems I need to touch for this IOMMU cleanup aren't all quite as involved as this turned out to be :) Cheers, Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 06/11] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
On 2022/3/21 16:04, Tian, Kevin wrote:
From: Lu Baolu
Sent: Sunday, March 20, 2022 2:40 PM
+struct iommu_sva *
+iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void
*drvdata)
+{
+ int ret = -EINVAL;
+ struct iommu_sva *handle;
+ struct iommu_domain *domain;
+
+ handle = kzalloc(sizeof(*handle), GFP_KERNEL);
+ if (!handle)
+ return ERR_PTR(-ENOMEM);
+
+ ret = iommu_sva_alloc_pasid(mm, 1, (1U << dev->iommu->pasid_bits)
- 1);
+ if (ret)
+ goto out;
+
+ domain = iommu_sva_domain_alloc(dev);
+ if (!domain) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ domain->sva_cookie = mm;
one domain can be attached by multiple devices, so this should not be
a blind alloc.
Indeed. Perhaps we could associate the SVA domain with the mm->pasid and
add a user counter inside the domain.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/2] thunderbolt: Make iommu_dma_protection more accurate
Hi Mario, On Fri, Mar 18, 2022 at 10:29:59PM +, Limonciello, Mario wrote: > [Public] > > > Between me trying to get rid of iommu_present() and Mario wanting to > > support the AMD equivalent of DMAR_PLATFORM_OPT_IN, scrutiny has > > shown > > that the iommu_dma_protection attribute is being far too optimistic. > > Even if an IOMMU might be present for some PCI segment in the system, > > that doesn't necessarily mean it provides translation for the device(s) > > we care about. Furthermore, all that DMAR_PLATFORM_OPT_IN really does > > is tell us that memory was protected before the kernel was loaded, and > > prevent the user from disabling the intel-iommu driver entirely. While > > that lets us assume kernel integrity, what matters for actual runtime > > DMA protection is whether we trust individual devices, based on the > > "external facing" property that we expect firmware to describe for > > Thunderbolt ports. > > > > It's proven challenging to determine the appropriate ports accurately > > given the variety of possible topologies, so while still not getting a > > perfect answer, by putting enough faith in firmware we can at least get > > a good bit closer. If we can see that any device near a Thunderbolt NHI > > has all the requisites for Kernel DMA Protection, chances are that it > > *is* a relevant port, but moreover that implies that firmware is playing > > the game overall, so we'll use that to assume that all Thunderbolt ports > > should be correctly marked and thus will end up fully protected. > > > > This approach looks generally good to me. I do worry a little bit about older > systems that didn't set ExternalFacingPort in the FW but were previously > setting > iommu_dma_protection, but I think that those could be treated on a quirk > basis to set PCI IDs for those root ports as external facing if/when they come > up. There are no such systems out there AFAICT. > I'll send up a follow up patch that adds the AMD ACPI table check. > If it looks good can roll it into your series for v3, or if this series goes > as is for v2 it can come on its own. > > > CC: Mario Limonciello > > Signed-off-by: Robin Murphy > > --- > > > > v2: Give up trying to look for specific devices, just look for evidence > > that firmware cares at all. > > I still do think you could know exactly which devices to use if you're in > SW CM mode, but I guess the consensus is to not bifurcate the way this > can be checked. Indeed. The patch looks good to me now. I will give it a try on a couple of systems later today or tomorrow and let you guys know how it went. I don't expect any problems but let's see. Thanks a lot Robin for working on this :) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 2/2] PCI: ACPI: Support Microsoft's "DmaProperty"
On Sat, Mar 19, 2022 at 11:29:06PM -0700, Rajat Jain wrote: > The "DmaProperty" is supported and documented by Microsoft here: > https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports > They use this property for DMA protection: > https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt > > Support the "DmaProperty" with the same semantics. This is useful for > internal PCI devices that do not hang off a PCIe rootport, but offer > an attack surface for DMA attacks (e.g. internal network devices). > > Signed-off-by: Rajat Jain Reviewed-by: Mika Westerberg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/2] PCI: Rename "pci_dev->untrusted" to "pci_dev->poses_dma_risk"
On Sat, Mar 19, 2022 at 11:29:05PM -0700, Rajat Jain wrote: > Rename the field to make it more clear, that the device can execute DMA > attacks on the system, and thus the system may need protection from > such attacks from this device. > > No functional change intended. > > Signed-off-by: Rajat Jain Reviewed-by: Mika Westerberg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 04/11] iommu/vt-d: Add SVA domain support
On 2022/3/21 15:45, Tian, Kevin wrote:
From: Lu Baolu
Sent: Sunday, March 20, 2022 2:40 PM
Add support for SVA domain allocation and provide an SVA-specific
iommu_domain_ops.
Signed-off-by: Lu Baolu
---
include/linux/intel-iommu.h | 1 +
drivers/iommu/intel/iommu.c | 12
drivers/iommu/intel/svm.c | 34 ++
3 files changed, 47 insertions(+)
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 2f9891cb3d00..c14283137fb5 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -744,6 +744,7 @@ void intel_svm_unbind(struct iommu_sva *handle);
u32 intel_svm_get_pasid(struct iommu_sva *handle);
int intel_svm_page_response(struct device *dev, struct iommu_fault_event
*evt,
struct iommu_page_response *msg);
+extern const struct iommu_domain_ops intel_svm_domain_ops;
struct intel_svm_dev {
struct list_head list;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index c1b91bce1530..d55dca3eacf8 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4318,6 +4318,18 @@ static struct iommu_domain
*intel_iommu_domain_alloc(unsigned type)
return domain;
case IOMMU_DOMAIN_IDENTITY:
return _domain->domain;
+#ifdef CONFIG_INTEL_IOMMU_SVM
+ case IOMMU_DOMAIN_SVA:
+ dmar_domain = alloc_domain(type);
+ if (!dmar_domain) {
+ pr_err("Can't allocate sva domain\n");
+ return NULL;
+ }
+ domain = _domain->domain;
+ domain->ops = _svm_domain_ops;
+
+ return domain;
+#endif /* CONFIG_INTEL_IOMMU_SVM */
default:
return NULL;
}
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index ee5ecde5b318..b9f4dd7057d1 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -932,3 +932,37 @@ int intel_svm_page_response(struct device *dev,
mutex_unlock(_mutex);
return ret;
}
+
+static int intel_svm_attach_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ struct device_domain_info *info = dev_iommu_priv_get(dev);
+ struct mm_struct *mm = domain->sva_cookie;
+ struct intel_iommu *iommu = info->iommu;
+ struct iommu_sva *sva;
+
+ mutex_lock(_mutex);
+ sva = intel_svm_bind_mm(iommu, dev, mm);
+ mutex_unlock(_mutex);
+
I'm not sure whether this is the right implementation of this callback.
In last patch you said it will be used for multiple usages but here it
is fixed to mm binding.
It is for svm domain ops so it should be mm binding only. For other
usages, they should have different domain ops.
Also the pasid argument is not used at all
and instead it is retrieved from mm struct implicitly.
The pasid is not used here because it has already been stored in
mm->pasid. Since it's only for SVM domain, I think it's okay.
Basically SVA requires three steps:
1) alloc a SVA-type domain;
2) construct the domain to wrap mm;
3) attach the domain to a PASID;
If we aim .attach_dev_pasid to be generic it may suggest that 1) and 2)
should be done before .attach_dev_pasid then within this callback it
just deals with domain/pasid attach in a generic way.
You are right. This code does have room for further cleanup. I would
like to put that in a separated series so that we could focus on the
generic SVA and IOPF refactoring.
+ return IS_ERR_OR_NULL(sva);
+}
+
+static void intel_svm_detach_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ mutex_lock(_mutex);
+ intel_svm_unbind_mm(dev, pasid);
+ mutex_unlock(_mutex);
+}
+
+static void intel_svm_domain_free(struct iommu_domain *domain)
+{
+ kfree(domain);
+}
+
+const struct iommu_domain_ops intel_svm_domain_ops = {
+ .attach_dev_pasid = intel_svm_attach_dev_pasid,
+ .detach_dev_pasid = intel_svm_detach_dev_pasid,
+ .free = intel_svm_domain_free,
+};
--
2.25.1
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 03/11] iommu: Add attach/detach_dev_pasid domain ops
On 2022/3/21 15:13, Tian, Kevin wrote:
From: Lu Baolu
Sent: Sunday, March 20, 2022 2:40 PM
Attaching an IOMMU domain to a PASID of a device is a generic operation
for modern IOMMU drivers which support PASID-granular DMA address
translation. Currently visible usage scenarios include (but not limited):
- SVA
- kernel DMA with PASID
- hardware-assist mediated device
This adds a pair of common domain ops for this purpose and implements a
couple of wrapper helpers for in-kernel usage.
Signed-off-by: Lu Baolu
---
include/linux/iommu.h | 22 ++
drivers/iommu/iommu.c | 41
+
2 files changed, 63 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 3e179b853380..e51845b9a146 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -268,6 +268,8 @@ struct iommu_ops {
* struct iommu_domain_ops - domain specific operations
* @attach_dev: attach an iommu domain to a device
* @detach_dev: detach an iommu domain from a device
+ * @attach_dev_pasid: attach an iommu domain to a pasid of device
+ * @detach_dev_pasid: detach an iommu domain from a pasid of device
* @map: map a physically contiguous memory region to an iommu domain
* @map_pages: map a physically contiguous set of pages of the same size
to
* an iommu domain.
@@ -285,6 +287,10 @@ struct iommu_ops {
struct iommu_domain_ops {
int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
void (*detach_dev)(struct iommu_domain *domain, struct device
*dev);
+ int (*attach_dev_pasid)(struct iommu_domain *domain,
+ struct device *dev, ioasid_t id);
+ void (*detach_dev_pasid)(struct iommu_domain *domain,
+struct device *dev, ioasid_t id);
int (*map)(struct iommu_domain *domain, unsigned long iova,
phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
@@ -678,6 +684,11 @@ int iommu_group_claim_dma_owner(struct
iommu_group *group, void *owner);
void iommu_group_release_dma_owner(struct iommu_group *group);
bool iommu_group_dma_owner_claimed(struct iommu_group *group);
+int iommu_attach_device_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid);
+void iommu_detach_device_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid);
+
#else /* CONFIG_IOMMU_API */
struct iommu_ops {};
@@ -1046,6 +1057,17 @@ static inline bool
iommu_group_dma_owner_claimed(struct iommu_group *group)
{
return false;
}
+
+static inline int iommu_attach_device_pasid(struct iommu_domain
*domain,
+ struct device *dev, ioasid_t pasid)
+{
+ return -ENODEV;
+}
+
+static inline void iommu_detach_device_pasid(struct iommu_domain
*domain,
+struct device *dev, ioasid_t pasid)
+{
+}
#endif /* CONFIG_IOMMU_API */
/**
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0c42ece25854..78c71ee15f36 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3167,3 +3167,44 @@ bool iommu_group_dma_owner_claimed(struct
iommu_group *group)
return user;
}
EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
+
+int iommu_attach_device_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ struct iommu_group *group;
+ int ret = -EINVAL;
+
+ if (!domain->ops->attach_dev_pasid)
+ return -EINVAL;
+
+ group = iommu_group_get(dev);
+ if (!group)
+ return -ENODEV;
+
+ mutex_lock(>mutex);
+ if (iommu_group_device_count(group) != 1)
+ goto out_unlock;
Need move the reason of above limitation from iommu_sva_bind_device()
to here:
/*
* To keep things simple, SVA currently doesn't support IOMMU groups
* with more than one device. Existing SVA-capable systems are not
* affected by the problems that required IOMMU groups (lack of ACS
* isolation, device ID aliasing and other hardware issues).
*/
if (iommu_group_device_count(group) != 1)
goto out_unlock;
Yes. We need a comment around this code. But it's not only for SVA but
also for all pasid attachment feature. I need more inputs to judge
whether this limitation is reasonable.
btw I didn't see any safeguard on above assumption in device hotplug path
to a group which already has SVA enabled...
Agreed.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 02/11] iommu: Add iommu_domain type for SVA
On 2022/3/21 15:06, Tian, Kevin wrote:
From: Lu Baolu
Sent: Sunday, March 20, 2022 2:40 PM
Add a new iommu domain type IOMMU_DOMAIN_SVA to represent an I/O
page
table which is shared from CPU host VA. Add a sva_cookie field in the
iommu_domain structure to save the mm_struct which represent the CPU
memory page table.
Signed-off-by: Lu Baolu
---
include/linux/iommu.h | 6 ++
1 file changed, 6 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 36f43af0af53..3e179b853380 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -64,6 +64,9 @@ struct iommu_domain_geometry {
#define __IOMMU_DOMAIN_PT (1U << 2) /* Domain is identity
mapped */
#define __IOMMU_DOMAIN_DMA_FQ (1U << 3) /* DMA-API uses
flush queue*/
+#define __IOMMU_DOMAIN_SHARED (1U << 4) /* Page table shared from
CPU */
+#define __IOMMU_DOMAIN_HOST_VA (1U << 5) /* Host CPU virtual
address */
suppose the SHARED bit will be also used for KVM page table sharing and
HOST_VA bit is to differentiate mm sharing from the latter?
Yes, that's my intention.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 01/11] iommu: Add pasid_bits field in struct dev_iommu
On 2022/3/21 15:01, Tian, Kevin wrote:
From: Lu Baolu
Sent: Sunday, March 20, 2022 2:40 PM
Use this field to save the pasid/ssid bits that a device is able to
support with its IOMMU hardware. It is a generic attribute of a device
and lifting it into the per-device dev_iommu struct makes it possible
to allocate a PASID for device without calls into the IOMMU drivers.
Any iommu driver which suports PASID related features should set this
field before features are enabled on the devices.
Signed-off-by: Lu Baolu
---
include/linux/iommu.h | 1 +
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 +
drivers/iommu/intel/iommu.c | 5 -
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 6ef2df258673..36f43af0af53 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -368,6 +368,7 @@ struct dev_iommu {
struct iommu_fwspec *fwspec;
struct iommu_device *iommu_dev;
void*priv;
+ unsigned intpasid_bits;
};
int iommu_device_register(struct iommu_device *iommu,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 627a3ed5ee8f..8e262210b5ad 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2812,6 +2812,7 @@ static int arm_smmu_dev_enable_feature(struct
device *dev,
master->iopf_enabled = true;
return 0;
case IOMMU_DEV_FEAT_SVA:
+ dev->iommu->pasid_bits = master->ssid_bits;
return arm_smmu_master_enable_sva(master);
default:
return -EINVAL;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 6f7485c44a4b..c1b91bce1530 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4587,8 +4587,11 @@ static struct iommu_device
*intel_iommu_probe_device(struct device *dev)
if (pasid_supported(iommu)) {
int features = pci_pasid_features(pdev);
- if (features >= 0)
+ if (features >= 0) {
info->pasid_supported = features | 1;
+ dev->iommu->pasid_bits =
+ fls(pci_max_pasids(pdev)) - 1;
Original intel_svm_alloc_pasid() covers both PCI and non-PCI devices:
ioasid_t max_pasid = dev_is_pci(dev) ?
pci_max_pasids(to_pci_dev(dev)) : intel_pasid_max_id;
though I'm not sure whether non-PCI SVA has been supported indeed, this
patch implies a functional change here.
The info->pasid_supported is only set for PCI devices. So the status is
that non-PCI SVA hasn't been supported. No functional change here from
this point of view.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC 10/11] iommu: Make IOPF handling framework generic
> From: Lu Baolu > Sent: Sunday, March 20, 2022 2:40 PM > > The existing IOPF handling framework only handles the I/O page faults for > SVA. Ginven that we are able to link iommu domain with each I/O page fault, > we can now make the I/O page fault handling framework more general for > more types of page faults. "make ... generic" in subject line is kind of confusing. Reading this patch I think you really meant changing from per-device fault handling to per-domain fault handling. This is more accurate in concept since the fault is caused by the domain page table. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC 06/11] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
> From: Lu Baolu
> Sent: Sunday, March 20, 2022 2:40 PM
> +struct iommu_sva *
> +iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void
> *drvdata)
> +{
> + int ret = -EINVAL;
> + struct iommu_sva *handle;
> + struct iommu_domain *domain;
> +
> + handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> + if (!handle)
> + return ERR_PTR(-ENOMEM);
> +
> + ret = iommu_sva_alloc_pasid(mm, 1, (1U << dev->iommu->pasid_bits)
> - 1);
> + if (ret)
> + goto out;
> +
> + domain = iommu_sva_domain_alloc(dev);
> + if (!domain) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + domain->sva_cookie = mm;
one domain can be attached by multiple devices, so this should not be
a blind alloc.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC 04/11] iommu/vt-d: Add SVA domain support
> From: Lu Baolu
> Sent: Sunday, March 20, 2022 2:40 PM
>
> Add support for SVA domain allocation and provide an SVA-specific
> iommu_domain_ops.
>
> Signed-off-by: Lu Baolu
> ---
> include/linux/intel-iommu.h | 1 +
> drivers/iommu/intel/iommu.c | 12
> drivers/iommu/intel/svm.c | 34 ++
> 3 files changed, 47 insertions(+)
>
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index 2f9891cb3d00..c14283137fb5 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -744,6 +744,7 @@ void intel_svm_unbind(struct iommu_sva *handle);
> u32 intel_svm_get_pasid(struct iommu_sva *handle);
> int intel_svm_page_response(struct device *dev, struct iommu_fault_event
> *evt,
> struct iommu_page_response *msg);
> +extern const struct iommu_domain_ops intel_svm_domain_ops;
>
> struct intel_svm_dev {
> struct list_head list;
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index c1b91bce1530..d55dca3eacf8 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4318,6 +4318,18 @@ static struct iommu_domain
> *intel_iommu_domain_alloc(unsigned type)
> return domain;
> case IOMMU_DOMAIN_IDENTITY:
> return _domain->domain;
> +#ifdef CONFIG_INTEL_IOMMU_SVM
> + case IOMMU_DOMAIN_SVA:
> + dmar_domain = alloc_domain(type);
> + if (!dmar_domain) {
> + pr_err("Can't allocate sva domain\n");
> + return NULL;
> + }
> + domain = _domain->domain;
> + domain->ops = _svm_domain_ops;
> +
> + return domain;
> +#endif /* CONFIG_INTEL_IOMMU_SVM */
> default:
> return NULL;
> }
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index ee5ecde5b318..b9f4dd7057d1 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -932,3 +932,37 @@ int intel_svm_page_response(struct device *dev,
> mutex_unlock(_mutex);
> return ret;
> }
> +
> +static int intel_svm_attach_dev_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t pasid)
> +{
> + struct device_domain_info *info = dev_iommu_priv_get(dev);
> + struct mm_struct *mm = domain->sva_cookie;
> + struct intel_iommu *iommu = info->iommu;
> + struct iommu_sva *sva;
> +
> + mutex_lock(_mutex);
> + sva = intel_svm_bind_mm(iommu, dev, mm);
> + mutex_unlock(_mutex);
> +
I'm not sure whether this is the right implementation of this callback.
In last patch you said it will be used for multiple usages but here it
is fixed to mm binding. Also the pasid argument is not used at all
and instead it is retrieved from mm struct implicitly.
Basically SVA requires three steps:
1) alloc a SVA-type domain;
2) construct the domain to wrap mm;
3) attach the domain to a PASID;
If we aim .attach_dev_pasid to be generic it may suggest that 1) and 2)
should be done before .attach_dev_pasid then within this callback it
just deals with domain/pasid attach in a generic way.
> + return IS_ERR_OR_NULL(sva);
> +}
> +
> +static void intel_svm_detach_dev_pasid(struct iommu_domain *domain,
> +struct device *dev, ioasid_t pasid)
> +{
> + mutex_lock(_mutex);
> + intel_svm_unbind_mm(dev, pasid);
> + mutex_unlock(_mutex);
> +}
> +
> +static void intel_svm_domain_free(struct iommu_domain *domain)
> +{
> + kfree(domain);
> +}
> +
> +const struct iommu_domain_ops intel_svm_domain_ops = {
> + .attach_dev_pasid = intel_svm_attach_dev_pasid,
> + .detach_dev_pasid = intel_svm_detach_dev_pasid,
> + .free = intel_svm_domain_free,
> +};
> --
> 2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC 03/11] iommu: Add attach/detach_dev_pasid domain ops
> From: Lu Baolu
> Sent: Sunday, March 20, 2022 2:40 PM
>
> Attaching an IOMMU domain to a PASID of a device is a generic operation
> for modern IOMMU drivers which support PASID-granular DMA address
> translation. Currently visible usage scenarios include (but not limited):
>
> - SVA
> - kernel DMA with PASID
> - hardware-assist mediated device
>
> This adds a pair of common domain ops for this purpose and implements a
> couple of wrapper helpers for in-kernel usage.
>
> Signed-off-by: Lu Baolu
> ---
> include/linux/iommu.h | 22 ++
> drivers/iommu/iommu.c | 41
> +
> 2 files changed, 63 insertions(+)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 3e179b853380..e51845b9a146 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -268,6 +268,8 @@ struct iommu_ops {
> * struct iommu_domain_ops - domain specific operations
> * @attach_dev: attach an iommu domain to a device
> * @detach_dev: detach an iommu domain from a device
> + * @attach_dev_pasid: attach an iommu domain to a pasid of device
> + * @detach_dev_pasid: detach an iommu domain from a pasid of device
> * @map: map a physically contiguous memory region to an iommu domain
> * @map_pages: map a physically contiguous set of pages of the same size
> to
> * an iommu domain.
> @@ -285,6 +287,10 @@ struct iommu_ops {
> struct iommu_domain_ops {
> int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
> void (*detach_dev)(struct iommu_domain *domain, struct device
> *dev);
> + int (*attach_dev_pasid)(struct iommu_domain *domain,
> + struct device *dev, ioasid_t id);
> + void (*detach_dev_pasid)(struct iommu_domain *domain,
> + struct device *dev, ioasid_t id);
>
> int (*map)(struct iommu_domain *domain, unsigned long iova,
> phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
> @@ -678,6 +684,11 @@ int iommu_group_claim_dma_owner(struct
> iommu_group *group, void *owner);
> void iommu_group_release_dma_owner(struct iommu_group *group);
> bool iommu_group_dma_owner_claimed(struct iommu_group *group);
>
> +int iommu_attach_device_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t pasid);
> +void iommu_detach_device_pasid(struct iommu_domain *domain,
> +struct device *dev, ioasid_t pasid);
> +
> #else /* CONFIG_IOMMU_API */
>
> struct iommu_ops {};
> @@ -1046,6 +1057,17 @@ static inline bool
> iommu_group_dma_owner_claimed(struct iommu_group *group)
> {
> return false;
> }
> +
> +static inline int iommu_attach_device_pasid(struct iommu_domain
> *domain,
> + struct device *dev, ioasid_t pasid)
> +{
> + return -ENODEV;
> +}
> +
> +static inline void iommu_detach_device_pasid(struct iommu_domain
> *domain,
> + struct device *dev, ioasid_t pasid)
> +{
> +}
> #endif /* CONFIG_IOMMU_API */
>
> /**
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 0c42ece25854..78c71ee15f36 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3167,3 +3167,44 @@ bool iommu_group_dma_owner_claimed(struct
> iommu_group *group)
> return user;
> }
> EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
> +
> +int iommu_attach_device_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t pasid)
> +{
> + struct iommu_group *group;
> + int ret = -EINVAL;
> +
> + if (!domain->ops->attach_dev_pasid)
> + return -EINVAL;
> +
> + group = iommu_group_get(dev);
> + if (!group)
> + return -ENODEV;
> +
> + mutex_lock(>mutex);
> + if (iommu_group_device_count(group) != 1)
> + goto out_unlock;
Need move the reason of above limitation from iommu_sva_bind_device()
to here:
/*
* To keep things simple, SVA currently doesn't support IOMMU groups
* with more than one device. Existing SVA-capable systems are not
* affected by the problems that required IOMMU groups (lack of ACS
* isolation, device ID aliasing and other hardware issues).
*/
if (iommu_group_device_count(group) != 1)
goto out_unlock;
btw I didn't see any safeguard on above assumption in device hotplug path
to a group which already has SVA enabled...
> +
> + ret = domain->ops->attach_dev_pasid(domain, dev, pasid);
> +
> +out_unlock:
> + mutex_unlock(>mutex);
> + iommu_group_put(group);
> +
> + return ret;
> +}
> +
> +void iommu_detach_device_pasid(struct iommu_domain *domain,
> +struct device *dev, ioasid_t pasid)
> +{
> + struct iommu_group *group;
> +
> + group = iommu_group_get(dev);
> + if (WARN_ON(!group))
> + return;
> +
> +
RE: [PATCH RFC 02/11] iommu: Add iommu_domain type for SVA
> From: Lu Baolu
> Sent: Sunday, March 20, 2022 2:40 PM
>
> Add a new iommu domain type IOMMU_DOMAIN_SVA to represent an I/O
> page
> table which is shared from CPU host VA. Add a sva_cookie field in the
> iommu_domain structure to save the mm_struct which represent the CPU
> memory page table.
>
> Signed-off-by: Lu Baolu
> ---
> include/linux/iommu.h | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 36f43af0af53..3e179b853380 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -64,6 +64,9 @@ struct iommu_domain_geometry {
> #define __IOMMU_DOMAIN_PT(1U << 2) /* Domain is identity
> mapped */
> #define __IOMMU_DOMAIN_DMA_FQ(1U << 3) /* DMA-API uses
> flush queue*/
>
> +#define __IOMMU_DOMAIN_SHARED(1U << 4) /* Page table shared from
> CPU */
> +#define __IOMMU_DOMAIN_HOST_VA (1U << 5) /* Host CPU virtual
> address */
suppose the SHARED bit will be also used for KVM page table sharing and
HOST_VA bit is to differentiate mm sharing from the latter?
> +
> /*
> * This are the possible domain-types
> *
> @@ -86,6 +89,8 @@ struct iommu_domain_geometry {
> #define IOMMU_DOMAIN_DMA_FQ (__IOMMU_DOMAIN_PAGING |
> \
>__IOMMU_DOMAIN_DMA_API | \
>__IOMMU_DOMAIN_DMA_FQ)
> +#define IOMMU_DOMAIN_SVA (__IOMMU_DOMAIN_SHARED |
> \
> + __IOMMU_DOMAIN_HOST_VA)
>
> struct iommu_domain {
> unsigned type;
> @@ -95,6 +100,7 @@ struct iommu_domain {
> void *handler_token;
> struct iommu_domain_geometry geometry;
> struct iommu_dma_cookie *iova_cookie;
> + struct mm_struct *sva_cookie;
> };
>
> static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
> --
> 2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC 01/11] iommu: Add pasid_bits field in struct dev_iommu
> From: Lu Baolu
> Sent: Sunday, March 20, 2022 2:40 PM
>
> Use this field to save the pasid/ssid bits that a device is able to
> support with its IOMMU hardware. It is a generic attribute of a device
> and lifting it into the per-device dev_iommu struct makes it possible
> to allocate a PASID for device without calls into the IOMMU drivers.
> Any iommu driver which suports PASID related features should set this
> field before features are enabled on the devices.
>
> Signed-off-by: Lu Baolu
> ---
> include/linux/iommu.h | 1 +
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 +
> drivers/iommu/intel/iommu.c | 5 -
> 3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 6ef2df258673..36f43af0af53 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -368,6 +368,7 @@ struct dev_iommu {
> struct iommu_fwspec *fwspec;
> struct iommu_device *iommu_dev;
> void*priv;
> + unsigned intpasid_bits;
> };
>
> int iommu_device_register(struct iommu_device *iommu,
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 627a3ed5ee8f..8e262210b5ad 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2812,6 +2812,7 @@ static int arm_smmu_dev_enable_feature(struct
> device *dev,
> master->iopf_enabled = true;
> return 0;
> case IOMMU_DEV_FEAT_SVA:
> + dev->iommu->pasid_bits = master->ssid_bits;
> return arm_smmu_master_enable_sva(master);
> default:
> return -EINVAL;
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 6f7485c44a4b..c1b91bce1530 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4587,8 +4587,11 @@ static struct iommu_device
> *intel_iommu_probe_device(struct device *dev)
> if (pasid_supported(iommu)) {
> int features = pci_pasid_features(pdev);
>
> - if (features >= 0)
> + if (features >= 0) {
> info->pasid_supported = features | 1;
> + dev->iommu->pasid_bits =
> + fls(pci_max_pasids(pdev)) - 1;
Original intel_svm_alloc_pasid() covers both PCI and non-PCI devices:
ioasid_t max_pasid = dev_is_pci(dev) ?
pci_max_pasids(to_pci_dev(dev)) : intel_pasid_max_id;
though I'm not sure whether non-PCI SVA has been supported indeed, this
patch implies a functional change here.
> + }
> }
>
> if (info->ats_supported && ecap_prs(iommu->ecap)
> &&
> --
> 2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
