Re: [PATCH v9 04/11] iommu: Add sva iommu_domain support
On 2022/6/29 09:54, Tian, Kevin wrote:
From: Baolu Lu
Sent: Tuesday, June 28, 2022 7:34 PM
On 2022/6/28 16:50, Tian, Kevin wrote:
From: Baolu Lu
Sent: Tuesday, June 28, 2022 1:41 PM
struct iommu_domain {
unsigned type;
const struct iommu_domain_ops *ops;
unsigned long pgsize_bitmap;/* Bitmap of page sizes in use */
- iommu_fault_handler_t handler;
- void *handler_token;
struct iommu_domain_geometry geometry;
struct iommu_dma_cookie *iova_cookie;
+ union {
+ struct {/* IOMMU_DOMAIN_DMA */
+ iommu_fault_handler_t handler;
+ void *handler_token;
+ };
why is it DMA domain specific? What about unmanaged
domain? Unrecoverable fault can happen on any type
including SVA. Hence I think above should be domain type
agnostic.
The report_iommu_fault() should be replaced by the new
iommu_report_device_fault(). Jean has already started this work.
https://lore.kernel.org/linux-iommu/Yo4Nw9QyllT1RZbd@myrica/
Currently this is only for DMA domains, hence Robin suggested to make it
exclude with the SVA domain things.
https://lore.kernel.org/linux-iommu/f3170016-4d7f-e78e-db48-
68305f683...@arm.com/
Then it's worthy a comment that those two fields are for
some legacy fault reporting stuff and DMA type only.
The iommu_fault and SVA fields are exclusive. The former is used for
unrecoverable DMA remapping faults, while the latter is only interested
in the recoverable page faults.
I will update the commit message with above explanation. Does this work
for you?
Not exactly. Your earlier explanation is about old vs. new API thus
leaving the existing fault handler with current only user is fine.
but this is not related to unrecoverable vs. recoverable. As I said
unrecoverable could happen on all domain types. Tying it to
DMA-only doesn't make sense and I think in the end the new
iommu_report_device_fault() will need support both. Is it not the
case?
You are right.
The report_iommu_fault() should be replaced by the new
iommu_report_device_fault(). Leave the existing fault handler with the
existing users and the newly added SVA members should exclude it.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v9 04/11] iommu: Add sva iommu_domain support
> From: Baolu Lu
> Sent: Tuesday, June 28, 2022 7:34 PM
>
> On 2022/6/28 16:50, Tian, Kevin wrote:
> >> From: Baolu Lu
> >> Sent: Tuesday, June 28, 2022 1:41 PM
> struct iommu_domain {
> unsigned type;
> const struct iommu_domain_ops *ops;
> unsigned long pgsize_bitmap;/* Bitmap of page sizes in use
> */
> -iommu_fault_handler_t handler;
> -void *handler_token;
> struct iommu_domain_geometry geometry;
> struct iommu_dma_cookie *iova_cookie;
> +union {
> +struct {/* IOMMU_DOMAIN_DMA */
> +iommu_fault_handler_t handler;
> +void *handler_token;
> +};
> >>> why is it DMA domain specific? What about unmanaged
> >>> domain? Unrecoverable fault can happen on any type
> >>> including SVA. Hence I think above should be domain type
> >>> agnostic.
> >> The report_iommu_fault() should be replaced by the new
> >> iommu_report_device_fault(). Jean has already started this work.
> >>
> >> https://lore.kernel.org/linux-iommu/Yo4Nw9QyllT1RZbd@myrica/
> >>
> >> Currently this is only for DMA domains, hence Robin suggested to make it
> >> exclude with the SVA domain things.
> >>
> >> https://lore.kernel.org/linux-iommu/f3170016-4d7f-e78e-db48-
> >> 68305f683...@arm.com/
> > Then it's worthy a comment that those two fields are for
> > some legacy fault reporting stuff and DMA type only.
>
> The iommu_fault and SVA fields are exclusive. The former is used for
> unrecoverable DMA remapping faults, while the latter is only interested
> in the recoverable page faults.
>
> I will update the commit message with above explanation. Does this work
> for you?
>
Not exactly. Your earlier explanation is about old vs. new API thus
leaving the existing fault handler with current only user is fine.
but this is not related to unrecoverable vs. recoverable. As I said
unrecoverable could happen on all domain types. Tying it to
DMA-only doesn't make sense and I think in the end the new
iommu_report_device_fault() will need support both. Is it not the
case?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 04/11] iommu: Add sva iommu_domain support
On 2022/6/28 16:50, Tian, Kevin wrote:
+
+ mutex_lock(&group->mutex);
+ curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain,
GFP_KERNEL);
+ if (curr)
+ goto out_unlock;
Need check xa_is_err(old).
Either
(1) old entry is a valid pointer, or
return -EBUSY in this case
(2) xa_is_err(curr)
return xa_err(cur)
are failure cases. Hence, "curr == NULL" is the only check we need. Did
I miss anything?
But now you always return -EBUSY for all kinds of xa errors.
Fair enough. Updated like below.
curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain,
GFP_KERNEL);
if (curr) {
ret = xa_err(curr) ? : -EBUSY;
goto out_unlock;
}
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 04/11] iommu: Add sva iommu_domain support
On 2022/6/28 16:50, Tian, Kevin wrote:
From: Baolu Lu
Sent: Tuesday, June 28, 2022 1:41 PM
struct iommu_domain {
unsigned type;
const struct iommu_domain_ops *ops;
unsigned long pgsize_bitmap;/* Bitmap of page sizes in use */
- iommu_fault_handler_t handler;
- void *handler_token;
struct iommu_domain_geometry geometry;
struct iommu_dma_cookie *iova_cookie;
+ union {
+ struct {/* IOMMU_DOMAIN_DMA */
+ iommu_fault_handler_t handler;
+ void *handler_token;
+ };
why is it DMA domain specific? What about unmanaged
domain? Unrecoverable fault can happen on any type
including SVA. Hence I think above should be domain type
agnostic.
The report_iommu_fault() should be replaced by the new
iommu_report_device_fault(). Jean has already started this work.
https://lore.kernel.org/linux-iommu/Yo4Nw9QyllT1RZbd@myrica/
Currently this is only for DMA domains, hence Robin suggested to make it
exclude with the SVA domain things.
https://lore.kernel.org/linux-iommu/f3170016-4d7f-e78e-db48-
68305f683...@arm.com/
Then it's worthy a comment that those two fields are for
some legacy fault reporting stuff and DMA type only.
The iommu_fault and SVA fields are exclusive. The former is used for
unrecoverable DMA remapping faults, while the latter is only interested
in the recoverable page faults.
I will update the commit message with above explanation. Does this work
for you?
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v9 04/11] iommu: Add sva iommu_domain support
> From: Baolu Lu
> Sent: Tuesday, June 28, 2022 1:41 PM
> >
> >> struct iommu_domain {
> >>unsigned type;
> >>const struct iommu_domain_ops *ops;
> >>unsigned long pgsize_bitmap;/* Bitmap of page sizes in use */
> >> - iommu_fault_handler_t handler;
> >> - void *handler_token;
> >>struct iommu_domain_geometry geometry;
> >>struct iommu_dma_cookie *iova_cookie;
> >> + union {
> >> + struct {/* IOMMU_DOMAIN_DMA */
> >> + iommu_fault_handler_t handler;
> >> + void *handler_token;
> >> + };
> >
> > why is it DMA domain specific? What about unmanaged
> > domain? Unrecoverable fault can happen on any type
> > including SVA. Hence I think above should be domain type
> > agnostic.
>
> The report_iommu_fault() should be replaced by the new
> iommu_report_device_fault(). Jean has already started this work.
>
> https://lore.kernel.org/linux-iommu/Yo4Nw9QyllT1RZbd@myrica/
>
> Currently this is only for DMA domains, hence Robin suggested to make it
> exclude with the SVA domain things.
>
> https://lore.kernel.org/linux-iommu/f3170016-4d7f-e78e-db48-
> 68305f683...@arm.com/
Then it's worthy a comment that those two fields are for
some legacy fault reporting stuff and DMA type only.
> >
> >> +
> >> + mutex_lock(&group->mutex);
> >> + curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain,
> >> GFP_KERNEL);
> >> + if (curr)
> >> + goto out_unlock;
> >
> > Need check xa_is_err(old).
>
> Either
>
> (1) old entry is a valid pointer, or
return -EBUSY in this case
> (2) xa_is_err(curr)
return xa_err(cur)
>
> are failure cases. Hence, "curr == NULL" is the only check we need. Did
> I miss anything?
>
But now you always return -EBUSY for all kinds of xa errors.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 04/11] iommu: Add sva iommu_domain support
Hi Kevin,
On 2022/6/27 16:29, Tian, Kevin wrote:
From: Lu Baolu
Sent: Tuesday, June 21, 2022 10:44 PM
The sva iommu_domain represents a hardware pagetable that the IOMMU
hardware could use for SVA translation. This adds some infrastructure
to support SVA domain in the iommu common layer. It includes:
- Extend the iommu_domain to support a new IOMMU_DOMAIN_SVA
domain
type. The IOMMU drivers that support SVA should provide the sva
domain specific iommu_domain_ops.
- Add a helper to allocate an SVA domain. The iommu_domain_free()
is still used to free an SVA domain.
- Add helpers to attach an SVA domain to a device and the reverse
operation.
Some buses, like PCI, route packets without considering the PASID value.
Thus a DMA target address with PASID might be treated as P2P if the
address falls into the MMIO BAR of other devices in the group. To make
things simple, the attach/detach interfaces only apply to devices
belonging to the singleton groups, and the singleton is immutable in
fabric i.e. not affected by hotplug.
The iommu_attach/detach_device_pasid() can be used for other purposes,
such as kernel DMA with pasid, mediation device, etc.
I'd split this into two patches. One for adding iommu_attach/
detach_device_pasid() and set/block_dev_pasid ops, and the
other for adding SVA.
Yes. Make sense.
struct iommu_domain {
unsigned type;
const struct iommu_domain_ops *ops;
unsigned long pgsize_bitmap;/* Bitmap of page sizes in use */
- iommu_fault_handler_t handler;
- void *handler_token;
struct iommu_domain_geometry geometry;
struct iommu_dma_cookie *iova_cookie;
+ union {
+ struct {/* IOMMU_DOMAIN_DMA */
+ iommu_fault_handler_t handler;
+ void *handler_token;
+ };
why is it DMA domain specific? What about unmanaged
domain? Unrecoverable fault can happen on any type
including SVA. Hence I think above should be domain type
agnostic.
The report_iommu_fault() should be replaced by the new
iommu_report_device_fault(). Jean has already started this work.
https://lore.kernel.org/linux-iommu/Yo4Nw9QyllT1RZbd@myrica/
Currently this is only for DMA domains, hence Robin suggested to make it
exclude with the SVA domain things.
https://lore.kernel.org/linux-iommu/f3170016-4d7f-e78e-db48-68305f683...@arm.com/
+ struct {/* IOMMU_DOMAIN_SVA */
+ struct mm_struct *mm;
+ };
+ };
};
+
+struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
+ struct mm_struct *mm)
+{
+ const struct iommu_ops *ops = dev_iommu_ops(dev);
+ struct iommu_domain *domain;
+
+ domain = ops->domain_alloc(IOMMU_DOMAIN_SVA);
+ if (!domain)
+ return NULL;
+
+ domain->type = IOMMU_DOMAIN_SVA;
It's a bit weird that the type has been specified when calling
ops->domain_alloc while it still leaves to the caller to set the
type. But this is not caused by this series. could be cleaned
up separately.
Yes. Robin has patches to refactor the domain allocation interface,
let's wait and see what it looks like finally.
+
+ mutex_lock(&group->mutex);
+ curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain,
GFP_KERNEL);
+ if (curr)
+ goto out_unlock;
Need check xa_is_err(old).
Either
(1) old entry is a valid pointer, or
(2) xa_is_err(curr)
are failure cases. Hence, "curr == NULL" is the only check we need. Did
I miss anything?
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v9 04/11] iommu: Add sva iommu_domain support
> From: Lu Baolu
> Sent: Tuesday, June 21, 2022 10:44 PM
>
> The sva iommu_domain represents a hardware pagetable that the IOMMU
> hardware could use for SVA translation. This adds some infrastructure
> to support SVA domain in the iommu common layer. It includes:
>
> - Extend the iommu_domain to support a new IOMMU_DOMAIN_SVA
> domain
> type. The IOMMU drivers that support SVA should provide the sva
> domain specific iommu_domain_ops.
> - Add a helper to allocate an SVA domain. The iommu_domain_free()
> is still used to free an SVA domain.
> - Add helpers to attach an SVA domain to a device and the reverse
> operation.
>
> Some buses, like PCI, route packets without considering the PASID value.
> Thus a DMA target address with PASID might be treated as P2P if the
> address falls into the MMIO BAR of other devices in the group. To make
> things simple, the attach/detach interfaces only apply to devices
> belonging to the singleton groups, and the singleton is immutable in
> fabric i.e. not affected by hotplug.
>
> The iommu_attach/detach_device_pasid() can be used for other purposes,
> such as kernel DMA with pasid, mediation device, etc.
I'd split this into two patches. One for adding iommu_attach/
detach_device_pasid() and set/block_dev_pasid ops, and the
other for adding SVA.
> struct iommu_domain {
> unsigned type;
> const struct iommu_domain_ops *ops;
> unsigned long pgsize_bitmap;/* Bitmap of page sizes in use */
> - iommu_fault_handler_t handler;
> - void *handler_token;
> struct iommu_domain_geometry geometry;
> struct iommu_dma_cookie *iova_cookie;
> + union {
> + struct {/* IOMMU_DOMAIN_DMA */
> + iommu_fault_handler_t handler;
> + void *handler_token;
> + };
why is it DMA domain specific? What about unmanaged
domain? Unrecoverable fault can happen on any type
including SVA. Hence I think above should be domain type
agnostic.
> + struct {/* IOMMU_DOMAIN_SVA */
> + struct mm_struct *mm;
> + };
> + };
> };
>
> +
> +struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
> + struct mm_struct *mm)
> +{
> + const struct iommu_ops *ops = dev_iommu_ops(dev);
> + struct iommu_domain *domain;
> +
> + domain = ops->domain_alloc(IOMMU_DOMAIN_SVA);
> + if (!domain)
> + return NULL;
> +
> + domain->type = IOMMU_DOMAIN_SVA;
It's a bit weird that the type has been specified when calling
ops->domain_alloc while it still leaves to the caller to set the
type. But this is not caused by this series. could be cleaned
up separately.
> +
> + mutex_lock(&group->mutex);
> + curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain,
> GFP_KERNEL);
> + if (curr)
> + goto out_unlock;
Need check xa_is_err(old).
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v9 04/11] iommu: Add sva iommu_domain support
The sva iommu_domain represents a hardware pagetable that the IOMMU
hardware could use for SVA translation. This adds some infrastructure
to support SVA domain in the iommu common layer. It includes:
- Extend the iommu_domain to support a new IOMMU_DOMAIN_SVA domain
type. The IOMMU drivers that support SVA should provide the sva
domain specific iommu_domain_ops.
- Add a helper to allocate an SVA domain. The iommu_domain_free()
is still used to free an SVA domain.
- Add helpers to attach an SVA domain to a device and the reverse
operation.
Some buses, like PCI, route packets without considering the PASID value.
Thus a DMA target address with PASID might be treated as P2P if the
address falls into the MMIO BAR of other devices in the group. To make
things simple, the attach/detach interfaces only apply to devices
belonging to the singleton groups, and the singleton is immutable in
fabric i.e. not affected by hotplug.
The iommu_attach/detach_device_pasid() can be used for other purposes,
such as kernel DMA with pasid, mediation device, etc.
Suggested-by: Jean-Philippe Brucker
Suggested-by: Jason Gunthorpe
Signed-off-by: Lu Baolu
Reviewed-by: Jean-Philippe Brucker
---
include/linux/iommu.h | 45 -
drivers/iommu/iommu.c | 93 +++
2 files changed, 136 insertions(+), 2 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 3fbad42c0bf8..b8b6b8c5e20e 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -64,6 +64,8 @@ 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_SVA (1U << 4) /* Shared process address space */
+
/*
* This are the possible domain-types
*
@@ -77,6 +79,8 @@ struct iommu_domain_geometry {
* certain optimizations for these domains
* IOMMU_DOMAIN_DMA_FQ - As above, but definitely using batched TLB
* invalidation.
+ * IOMMU_DOMAIN_SVA- DMA addresses are shared process address
+ * spaces represented by mm_struct's.
*/
#define IOMMU_DOMAIN_BLOCKED (0U)
#define IOMMU_DOMAIN_IDENTITY (__IOMMU_DOMAIN_PT)
@@ -86,15 +90,23 @@ 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_SVA)
struct iommu_domain {
unsigned type;
const struct iommu_domain_ops *ops;
unsigned long pgsize_bitmap;/* Bitmap of page sizes in use */
- iommu_fault_handler_t handler;
- void *handler_token;
struct iommu_domain_geometry geometry;
struct iommu_dma_cookie *iova_cookie;
+ union {
+ struct {/* IOMMU_DOMAIN_DMA */
+ iommu_fault_handler_t handler;
+ void *handler_token;
+ };
+ struct {/* IOMMU_DOMAIN_SVA */
+ struct mm_struct *mm;
+ };
+ };
};
static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
@@ -262,6 +274,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
+ * @set_dev_pasid: set an iommu domain to a pasid of device
+ * @block_dev_pasid: block pasid of device from using iommu domain
* @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.
@@ -282,6 +296,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 (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
+ioasid_t pasid);
+ void (*block_dev_pasid)(struct iommu_domain *domain, struct device *dev,
+ ioasid_t pasid);
int (*map)(struct iommu_domain *domain, unsigned long iova,
phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
@@ -679,6 +697,12 @@ 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);
+struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
+ struct mm_struct *mm);
+int iommu_attach_device_pasid(struct iommu_domain *domain, struct device *dev,
+ ioasid_t pasid);
+void iommu_detach_devic
