Add Nested Translation Support for SMMUv3

The nature of ITS virtualization on ARM is done via hypercalls, so kernel handles all IOVA mappings for the MSI doorbell in iommu_dma_prepare_msi() and iommu_dma_compose_msi_msg(). The current virtualization solution with a 2-stage nested translation setup is to do 1:1 IOVA mappings at stage-1 guest-level IO page table via a RMR region in guest-level IORT, aligning with an IOVA region that's predefined and mapped in the host kernel:

   [stage-2 host level]
   #define MSI_IOVA_BASE		0x8000000
   #define MSI_IOVA_LENGTH	0x100000
   ...
   iommu_get_msi_cookie():
	cookie->msi_iova = MSI_IOVA_BASE;
   ...
   iommu_dma_prepare_msi(its_pa):
	domain = iommu_get_domain_for_dev(dev);
	iommu_dma_get_msi_page(its_pa, domain):
		cookie = domain->iova_cookie;
		iova = iommu_dma_alloc_iova():
			return cookie->msi_iova - size;
		iommu_map(iova, its_pa, ...);

   [stage-1 guest level]
   // Define in IORT a RMR [MSI_IOVA_BASE, MSI_IOVA_LENGTH]
   ...
   iommu_create_device_direct_mappings():
	iommu_map(iova=MSI_IOVA_BASE, pa=MSI_IOVA_BASE, len=MSI_IOVA_LENGTH);

This solution calling iommu_get_domain_for_dev() needs the device to get attached to a host-level iommu_domain that has the msi_cookie.

On the other hand, IOMMUFD designs two iommu_domain objects to represent the two stages: a stage-1 domain (IOMMU_DOMAIN_NESTED type) and a stage-2 domain (IOMMU_DOMAIN_UNMANAGED type). In this design, the device will be attached to the stage-1 domain representing a guest-level IO page table, or a Context Descriptor Table in SMMU's term.

This is obviously a mismatch, as the iommu_get_domain_for_dev() does not return the correct domain pointer in iommu_dma_prepare_msi().

Add an iommu_get_unmanaged_domain helper to allow drivers to return the correct IOMMU_DOMAIN_UNMANAGED iommu_domain having the IOVA mappings for the msi_cookie. Keep it in the iommu-priv header for internal use only.

Suggested-by Jason Gunthorpe <[email protected]>

Signed-off-by Nicolin Chen <[email protected]>

---
  drivers/iommu/dma-iommu.c  |  5 +++--
  drivers/iommu/iommu-priv.h | 15 +++++++++++++++
  include/linux/iommu.h      |  2 ++
  3 files changed, 20 insertions(+), 2 deletions(-)

drivers/iommu/dma-iommu.c +1 -0

index 99b2646cb5c7..6b0409d0ff85 100644
@@ -31,6 +31,7 @@
  #include <linux/vmalloc.h>
 
  #include "dma-iommu.h"
+#include "iommu-priv.h"
 
  struct iommu_dma_msi_page {
  	struct list_head	list;

drivers/iommu/dma-iommu.c +1 -1

  	struct iommu_dma_msi_page *msi_page;
  	static DEFINE_MUTEX(msi_prepare_lock); /* see below */
 

@@ -1685,7 +1686,7 @@ int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
  void iommu_dma_compose_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
  {
  	struct device *dev = msi_desc_to_dev(desc);
-	const struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+	const struct iommu_domain *domain = iommu_get_unmanaged_domain(dev);
  	const struct iommu_dma_msi_page *msi_page;
 
  	msi_page = msi_desc_get_iommu_cookie(desc);

drivers/iommu/iommu-priv.h +15 -0

index a6e694f59f64..da8044da9ad8 100644
@@ -15,6 +15,21 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
  	return dev->iommu->iommu_dev->ops;
  }
 
+static inline struct iommu_domain *iommu_get_unmanaged_domain(struct device *dev)
+{
+	const struct iommu_ops *ops;
+
+	if (!dev->iommu || !dev->iommu->iommu_dev)
+		goto attached_domain;
+
+	ops = dev_iommu_ops(dev);
+	if (ops->get_unmanaged_domain)
+		return ops->get_unmanaged_domain(dev);
+
+attached_domain:
+	return iommu_get_domain_for_dev(dev);
+}
+
  int iommu_group_replace_domain(struct iommu_group *group,
  			       struct iommu_domain *new_domain);
 

include/linux/iommu.h +2 -0

index 080278c8154d..76c65cc4fc15 100644
@@ -275,6 +275,8 @@ struct iommu_ops {
  						  struct iommu_domain *parent,
  						  const void *user_data);
 
+	struct iommu_domain *(*get_unmanaged_domain)(struct device *dev);
+
  	struct iommu_device *(*probe_device)(struct device *dev);
  	void (*release_device)(struct device *dev);
  	void (*probe_finalize)(struct device *dev);

Please note I stress "valid" since I'm not buying arbitrarily made-up conceptual purity arguments. A nested domain cannot be the "one true domain" that is an opaque combination of S1+S2; the IOMMU API view has to be more like the device is attached to both the nested domain and the parent stage 2 domain somewhat in parallel.

Even when nesting is active, the S2 domain still exists as a domain in its own right, and still needs to be visible and operated on as such, for instance if memory is hotplugged in or out of the VM.

Quoted 5 lines · Robin Murphy

Please note I stress "valid" since I'm not buying arbitrarily made-up conceptual purity arguments. A nested domain cannot be the "one true domain" that is an opaque combination of S1+S2; the IOMMU API view has to be more like the device is attached to both the nested domain and the parent stage 2 domain somewhat in parallel.

I strongly disagree with this.

The design we have from the core perspective is an opaque domain that is a hidden combination of S1/S2 inside the driver. We do not want to change the basic design of the iommu core: there is only one domain attached to a device/group at a time.

This patch should be seen as a temporary hack to allow the ARM ITS stuff to hobble on a little longer. We already know that iommufd use cases are incompatible with the design and we need to fix it. The fixed solution must have iommufd install the ITS pages at domain allocation time and so it will not need these APIs at all. This tempoary code should not dictate the overall design of the iommu core.

If we belive exposing the S1/S2 relationships through the iommu core is necessary for another legitimate purpose I would like to hear about it. In my opinion using APIs to peek into these details is far more likely to be buggy and long term I prefer to block the ability to learn the S2 externally from iommufd completely.

Thus the overall design of the iommu core APIs is not being changed. The core API design follows this logic with and without nesting:

    iommu_attach_device(domin);
    WARN_ON(domain != iommu_get_domain_for_dev());

The hack in this patch gets its own special single-use APIs so we can purge them once they are not needed and do not confusingly contaminate the whole design. For this reason the ops call back should only be implemented by SMMUv3.

Quoted 3 lines

Even when nesting is active, the S2 domain still exists as a domain in its own right, and still needs to be visible and operated on as such, for instance if memory is hotplugged in or out of the VM.

It exists in iommufd and iommufd will operate it. This is not a problem.

iommufd is not using a dual attach model.

The S2 is provided to the S1's domain allocation function as creation data. The returned S1 domain opaquely embeds the S2. The embedded S2 cannot be changed once the S1 domain is allocated.

Attaching the S1 always causes the embedded S2 to be used too - they are not separable so we don't have APIs talking about about "attaching" the S2.

You are literally insisting on changing this core API, to work around intentionally breaking an existing behaviour which could easily be preserved (far less invasively), for the sake of preserving some other theoretical behaviour that IS NOT USEFUL.

The overall design of the iommu core APIs *is* being changed, because the core API design also follows this logic for any domain type:

	domain = iommu_get_domain_for_dev();
	phys = iommu_iova_to_phys(domain, iova);
	//phys meaningfully represents whether iova was valid

Forgive me for getting wound up, but I'm a pragmatist and my tolerance for ignoring reality is low.

Quoted 3 lines

Attaching the S1 always causes the embedded S2 to be used too - they are not separable so we don't have APIs talking about about "attaching" the S2.

Yes, that is one way of viewing things, but it's far from the only way.

typical lifecycle will involve starting the VM with S2 alone, then enabling nesting later - we can view that as allocating a nested domain based on S2, then "replacing" S2 with nested, but we could equally view it as just attaching the nested domain on top of the existing S2, like pushing to a stack (I do agree that trying to model it as two completely independent and orthogonal attachments would not be sensible). It's really just semantics of how we prefer to describe things, and whether the s2_domain pointer is passed up-front to domain_alloc or later to attach_dev.

The opaque nested domain looks clean in some ways, but on the other hand it also heavily obfuscates how translations may be shared between one nested domain and other nested and non-nested domains, such that changing mappings in one may affect others.

This is a major and potentially surprising
paradigm shift away from the current notion that all domains represent individual isolated address spaces, so abstracting it more openly within the core API internals would be clearer about what's really going on. And putting common concepts at common levels of abstraction makes things simpler and is why we have core API code at all.

ultimately be a good deal neater and more productive than adding yet more special-case ops that every driver is just going to implement identically.

And to even imagine the future possibility of things like S2 pagetable sharing with KVM, or unpinned S2 with ATS/PRI or SMMU stalls, by tunnelling more private nesting interfaces directly between IOMMUFD and individual drivers without some degree of common IOMMU API abstraction in between... no. Just... no.

Quoted 71 lines · Nicolin Chen

The nature of ITS virtualization on ARM is done via hypercalls, so kernel
handles all IOVA mappings for the MSI doorbell in iommu_dma_prepare_msi()
and iommu_dma_compose_msi_msg(). The current virtualization solution with
a 2-stage nested translation setup is to do 1:1 IOVA mappings at stage-1 guest-level IO page table via a RMR region in guest-level IORT, aligning with an IOVA region that's predefined and mapped in the host kernel:

   [stage-2 host level]
   #define MSI_IOVA_BASE        0x8000000
   #define MSI_IOVA_LENGTH    0x100000
   ...
   iommu_get_msi_cookie():
    cookie->msi_iova = MSI_IOVA_BASE;
   ...
   iommu_dma_prepare_msi(its_pa):
    domain = iommu_get_domain_for_dev(dev);
    iommu_dma_get_msi_page(its_pa, domain):
        cookie = domain->iova_cookie;
        iova = iommu_dma_alloc_iova():
            return cookie->msi_iova - size;
        iommu_map(iova, its_pa, ...);

   [stage-1 guest level]
   // Define in IORT a RMR [MSI_IOVA_BASE, MSI_IOVA_LENGTH]
   ...
   iommu_create_device_direct_mappings():
    iommu_map(iova=MSI_IOVA_BASE, pa=MSI_IOVA_BASE, len=MSI_IOVA_LENGTH);

This solution calling iommu_get_domain_for_dev() needs the device to get attached to a host-level iommu_domain that has the msi_cookie.

On the other hand, IOMMUFD designs two iommu_domain objects to represent the two stages: a stage-1 domain (IOMMU_DOMAIN_NESTED type) and a stage-2
domain (IOMMU_DOMAIN_UNMANAGED type). In this design, the device will be attached to the stage-1 domain representing a guest-level IO page table, or a Context Descriptor Table in SMMU's term.

This is obviously a mismatch, as the iommu_get_domain_for_dev() does not return the correct domain pointer in iommu_dma_prepare_msi().

Add an iommu_get_unmanaged_domain helper to allow drivers to return the correct IOMMU_DOMAIN_UNMANAGED iommu_domain having the IOVA mappings for the msi_cookie. Keep it in the iommu-priv header for internal use only.

Suggested-by Jason Gunthorpe <[email protected]>

Signed-off-by Nicolin Chen <[email protected]>

---
  drivers/iommu/dma-iommu.c  |  5 +++--
  drivers/iommu/iommu-priv.h | 15 +++++++++++++++
  include/linux/iommu.h      |  2 ++

  3 files changed, 20 insertions(+), 2 deletions(-)

drivers/iommu/dma-iommu.c +1 -0

index 99b2646cb5c7..6b0409d0ff85 100644
@@ -31,6 +31,7 @@
   #include <linux/vmalloc.h>
     #include "dma-iommu.h"
+#include "iommu-priv.h"
     struct iommu_dma_msi_page {
       struct list_head    list;

@@ -1652,7 +1653,7 @@ static struct iommu_dma_msi_page
 *iommu_dma_get_msi_page(struct device *dev,
   int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
   {

drivers/iommu/dma-iommu.c static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev, · line 1656

Quoted 3 lines

drivers/iommu/dma-iommu.c +1 -1

       struct device *dev = msi_desc_to_dev(desc);
-    struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+    struct iommu_domain *domain = iommu_get_unmanaged_domain(dev);

This still doesn't make sense - most of the time this will be expected to return the default DMA/identity domain if that's what the device is currently using. We can't know whether the current domain is managed or not until we look at it.

Just like every other caller of iommu_get_domain_for_dev(), what we want here is the current kernel-owned domain that we can inspect and maybe do standard IOMMU API things with. Why can't iommu_get_domain_for_dev() simply maintain that established usage model and return the kernel-owned s2_domain from a nested domain automatically? No IOMMU API user expects or needs it to return anything else (and IOMMUFD should certainly not be losing track of a nested domain within its own higher-level abstractions and needing to fall back on iommu_get_domain_for_dev()), so I really don't see a valid reason to overcomplicate things.

Please note I stress "valid" since I'm not buying arbitrarily made-up conceptual purity arguments. A nested domain cannot be the "one true domain" that is an opaque combination of S1+S2; the IOMMU API view has to be more like the device is attached to both the nested domain and the parent stage 2 domain somewhat in parallel. Even when nesting is active, the S2 domain still exists as a domain in its own right, and still needs to be visible and operated on as such, for instance if memory is hotplugged in or out of the VM.

TBH I'd also move the s2_domain pointer into the iommu_domain itself, since it's going to be a common feature for all nesting implementations, thus there seems little need to indirect lookups through the drivers at all.

Thanks,
Robin.

Quoted 51 lines

drivers/iommu/dma-iommu.c +0 -0

       struct iommu_dma_msi_page *msi_page;

      static DEFINE_MUTEX(msi_prepare_lock); /* see below */ @@ -1685,7 +1686,7 @@ int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
  void iommu_dma_compose_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
  {
      struct device *dev = msi_desc_to_dev(desc);
-    const struct iommu_domain *domain = iommu_get_domain_for_dev(dev); +    const struct iommu_domain *domain = iommu_get_unmanaged_domain(dev);
      const struct iommu_dma_msi_page *msi_page;
        msi_page = msi_desc_get_iommu_cookie(desc);

drivers/iommu/iommu-priv.h +0 -0

index a6e694f59f64..da8044da9ad8 100644
@@ -15,6 +15,21 @@ static inline const struct iommu_ops
 *dev_iommu_ops(struct device *dev)
       return dev->iommu->iommu_dev->ops;
   }

  +static inline struct iommu_domain
*iommu_get_unmanaged_domain(struct device *dev)

drivers/iommu/iommu-priv.h +14 -0

+{
+    const struct iommu_ops *ops;
+
+    if (!dev->iommu || !dev->iommu->iommu_dev)
+        goto attached_domain;
+
+    ops = dev_iommu_ops(dev);
+    if (ops->get_unmanaged_domain)
+        return ops->get_unmanaged_domain(dev);
+
+attached_domain:
+    return iommu_get_domain_for_dev(dev);
+}
+

  int iommu_group_replace_domain(struct iommu_group *group, struct iommu_domain *new_domain); diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 080278c8154d..76c65cc4fc15 100644

include/linux/iommu.h +1 -0

@@ -275,6 +275,8 @@ struct iommu_ops {
                             struct iommu_domain *parent,
                             const void *user_data);
   +    struct iommu_domain *(*get_unmanaged_domain)(struct device *dev);
+
       struct iommu_device *(*probe_device)(struct device *dev);
       void (*release_device)(struct device *dev);
       void (*probe_finalize)(struct device *dev);

I tend to agree with Robin here. This was first introduced by

[PATCH v7 21/22] iommu/dma: Add support for mapping MSIs <https://lore.kernel.org/all/2273af20d844bd618c6a90b57e639700328ebf7f.1473695704.git.robin.murphy@arm.com/#r>
https://lore.kernel.org/all/2273af20d844bd618c6a90b57e639700328ebf7f.1473695704.git.robin.murphy@arm.com/

even before the support un VFIO use case which came later on. So using the "unmanaged" terminology sounds improper to me, at least. Couldn't we use a parent/child terminology as used in the past in

Quoted 4 lines · Eric Auger

I tend to agree with Robin here. This was first introduced by

[PATCH v7 21/22] iommu/dma: Add support for mapping MSIs <https://lore.kernel.org/all/2273af20d844bd618c6a90b57e639700328ebf7f.1473695704.git.robin.murphy@arm.com/#r>
https://lore.kernel.org/all/2273af20d844bd618c6a90b57e639700328ebf7f.1473695704.git.robin.murphy@arm.com/

Presumably it had to use the iommu_get_domain_for_dev() instead of iommu_get_dma_domain() to support ARM 32 arm-iommu. Ie it is poking into the arm-iommu owned domain as well. VFIO just ended being the same flow

Quoted 3 lines

even before the support un VFIO use case which came later on. So using the "unmanaged" terminology sounds improper to me, at least. Couldn't we use a parent/child terminology as used in the past in

No objection to a better name...

Actually how about if we write it like this? Robin would you be happier? I think it much more clearly explains why this function is special within our single domain attachment model.

"get_unmanaged_msi_domain" seems like a much more narrowly specific to the purpose name.

int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)

{
	struct device *dev = msi_desc_to_dev(desc);
	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
	struct iommu_dma_msi_page *msi_page;
	static DEFINE_MUTEX(msi_prepare_lock); /* see below */

	desc->iommu_cookie = NULL;

	/*
	 * This probably shouldn't happen as the ARM32 systems should only have
	 * NULL if arm-iommu has been disconnected during setup/destruction.
	 * Assume it is an identity domain.
	 */
	if (!domain)
		return 0;

	/* Caller is expected to use msi_addr for the page */
	if (domain->type == IOMMU_DOMAIN_IDENTITY)
		return 0;

	/*
	 * The current domain is some driver opaque thing. We assume the
	 * driver/user knows what it is doing regarding ARM ITS MSI pages and we
	 * want to try to install the page into some kind of kernel owned
	 * unmanaged domain. Eg for nesting this will install the ITS page into
	 * the S2 domain and then we assume that the S1 domain has independently
	 * made it mapped at the same address.
	 */
	// FIXME wrap in a function
	if (domain->type != IOMMU_DOMAIN_UNMANAGED &&
	    domain->ops->get_unmanged_msi_domain)
		domain = domain->ops->get_unmanged_msi_domain(domain);

	if (!domain || domain->type != IOMMU_DOMAIN_UNMANAGED)
		return -EINVAL;

	// ???
	if (!domain->iova_cookie)
		return 0;

	/*
	 * In fact the whole prepare operation should already be serialised by
	 * irq_domain_mutex further up the callchain, but that's pretty subtle
	 * on its own, so consider this locking as failsafe documentation...
	 */
	mutex_lock(&msi_prepare_lock);
	msi_page = iommu_dma_get_msi_page(dev, msi_addr, domain);
	mutex_unlock(&msi_prepare_lock);

	msi_desc_set_iommu_cookie(desc, msi_page);

	if (!msi_page)
		return -ENOMEM;
	return 0;
}

Jason

Hi Robin,

How do you think about Jason's proposal below? I'd like to see us come to an agreement on an acceptable solution...

External email: Use caution opening links or attachments

Quoted 2 lines · Nicolin Chen

Hi Robin,

Quoted 2 lines

How do you think about Jason's proposal below? I'd like to see us come to an agreement on an acceptable solution...

I think it's so thoroughly broken that I suspect Cunningham's law might be at play, but fine, you win :) Hopefully it's sufficiently obvious how the other pieces would fit around the patch below. FWIW I'd still prefer a generic domain->s2_domain pointer rather than any op at all, but I'm happy enough with this compromise.

----->8-----
Subject: [PATCH] iommu/dma: Support MSIs through nested domains

Currently, iommu-dma is the only place outside of IOMMUFD and drivers which might need to be aware of the stage 2 domain encapsulated within a nested domain. This would be in the legacy-VFIO-style case where we're using host-managed MSIs with an identity mapping at stage 1, where it is the underlying stage 2 domain which owns an MSI cookie and holds the corresponding dynamic mappings. Hook up the new op to resolve what we need from a nested domain.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/dma-iommu.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

drivers/iommu/dma-iommu.c +16 -3

index 99b2646cb5c7..66b0d5fa49f8 100644
@@ -1642,6 +1642,20 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
       return NULL;
 }
 
+/*
+ * Nested domains may not have an MSI cookie or accept mappings, but they may
+ * be related to a domain which does, so we let them tell us what they need.
+ */
+static struct iommu_domain *iommu_dma_get_msi_mapping_domain(struct device *dev)
+{
+       struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+       if (domain && domain->type == IOMMU_DOMAIN_NESTED &&
+           domain->ops->get_msi_mapping_domain)
+               domain = domain->ops->get_msi_mapping_domain(domain);
+       return domain;
+}
+
 /**
  * iommu_dma_prepare_msi() - Map the MSI page in the IOMMU domain
  * @desc: MSI descriptor, will store the MSI page

@@ -1652,7 +1666,7 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
 int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
 {
       struct device *dev = msi_desc_to_dev(desc);
-       struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+       struct iommu_domain *domain = iommu_dma_get_msi_mapping_domain(dev);
       struct iommu_dma_msi_page *msi_page;
       static DEFINE_MUTEX(msi_prepare_lock); /* see below */
 

@@ -1685,7 +1699,7 @@ int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
 void iommu_dma_compose_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
 {
       struct device *dev = msi_desc_to_dev(desc);
-       const struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+       const struct iommu_domain *domain = iommu_dma_get_msi_mapping_domain(dev);
       const struct iommu_dma_msi_page *msi_page;
 
       msi_page = msi_desc_get_iommu_cookie(desc);
--

2.39.2.101.g768bb238c484.dirty

Quoted 4 lines · Nicolin Chen

Hi Robin,

How do you think about Jason's proposal below? I'd like to see us come to an agreement on an acceptable solution...

I think it's so thoroughly broken that I suspect Cunningham's law might be at play, but fine, you win :)

The nature of ITS virtualization on ARM is done via hypercalls, so kernel handles all IOVA mappings for the MSI doorbell in iommu_dma_prepare_msi() and iommu_dma_compose_msi_msg(). The current virtualization solution with a 2-stage nested translation setup is to do 1:1 IOVA mappings at stage-1

guest-level IO page table via a RMR region in guest-level IORT, aligning with an IOVA region that's predefined and mapped in the host kernel:

[stage-2 host level]
#define MSI_IOVA_BASE 0x8000000
#define MSI_IOVA_LENGTH 0x100000
...
iommu_get_msi_cookie():

	cookie->msi_iova = MSI_IOVA_BASE;
  ...
  iommu_dma_prepare_msi(its_pa):
	domain = iommu_get_domain_for_dev(dev);
	iommu_dma_get_msi_page(its_pa, domain):
		cookie = domain->iova_cookie;
		iova = iommu_dma_alloc_iova():
			return cookie->msi_iova - size;
		iommu_map(iova, its_pa, ...);

[stage-1 guest level]
// Define in IORT a RMR [MSI_IOVA_BASE, MSI_IOVA_LENGTH]
...
iommu_create_device_direct_mappings():

	iommu_map(iova=MSI_IOVA_BASE, pa=MSI_IOVA_BASE, len=MSI_IOVA_LENGTH);

This solution calling iommu_get_domain_for_dev() needs the device to get attached to a host-level iommu_domain that has the msi_cookie.

On the other hand, IOMMUFD designs two iommu_domain objects to represent the two stages: a stage-1 domain (IOMMU_DOMAIN_NESTED type) and a stage-2 domain (IOMMU_DOMAIN_UNMANAGED type). In this design, the device will be attached to the stage-1 domain representing a guest-level IO page table, or a Context Descriptor Table in SMMU's term.

This is obviously a mismatch, as the iommu_get_domain_for_dev() does not return the correct domain pointer in iommu_dma_prepare_msi().

Add an iommu_get_unmanaged_domain helper to allow drivers to return the correct IOMMU_DOMAIN_UNMANAGED iommu_domain having the IOVA mappings for the msi_cookie. Keep it in the iommu-priv header for internal use only.

Suggested-by Jason Gunthorpe <[email protected]>

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/dma-iommu.c  |  5 +++--
 drivers/iommu/iommu-priv.h | 15 +++++++++++++++
 include/linux/iommu.h      |  2 ++
 3 files changed, 20 insertions(+), 2 deletions(-)

drivers/iommu/dma-iommu.c +3 -2

index 99b2646cb5c7..6b0409d0ff85 100644
@@ -31,6 +31,7 @@
 #include <linux/vmalloc.h>
 
 #include "dma-iommu.h"
+#include "iommu-priv.h"
 
 struct iommu_dma_msi_page {
 	struct list_head	list;

@@ -1652,7 +1653,7 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
 int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
 {
 	struct device *dev = msi_desc_to_dev(desc);
-	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+	struct iommu_domain *domain = iommu_get_unmanaged_domain(dev);
 	struct iommu_dma_msi_page *msi_page;
 	static DEFINE_MUTEX(msi_prepare_lock); /* see below */
 

@@ -1685,7 +1686,7 @@ int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
 void iommu_dma_compose_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
 {
 	struct device *dev = msi_desc_to_dev(desc);
-	const struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+	const struct iommu_domain *domain = iommu_get_unmanaged_domain(dev);
 	const struct iommu_dma_msi_page *msi_page;
 
 	msi_page = msi_desc_get_iommu_cookie(desc);

drivers/iommu/iommu-priv.h +15 -0

index a6e694f59f64..da8044da9ad8 100644
@@ -15,6 +15,21 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
 	return dev->iommu->iommu_dev->ops;
 }
 
+static inline struct iommu_domain *iommu_get_unmanaged_domain(struct device *dev)
+{
+	const struct iommu_ops *ops;
+
+	if (!dev->iommu || !dev->iommu->iommu_dev)
+		goto attached_domain;
+
+	ops = dev_iommu_ops(dev);
+	if (ops->get_unmanaged_domain)
+		return ops->get_unmanaged_domain(dev);
+
+attached_domain:
+	return iommu_get_domain_for_dev(dev);
+}
+
 int iommu_group_replace_domain(struct iommu_group *group,
 			       struct iommu_domain *new_domain);
 

include/linux/iommu.h +2 -0

index 080278c8154d..76c65cc4fc15 100644
@@ -275,6 +275,8 @@ struct iommu_ops {
 						  struct iommu_domain *parent,
 						  const void *user_data);
 
+	struct iommu_domain *(*get_unmanaged_domain)(struct device *dev);
+
 	struct iommu_device *(*probe_device)(struct device *dev);
 	void (*release_device)(struct device *dev);
 	void (*probe_finalize)(struct device *dev);

Hi Nicolin,

Quoted 4 lines · Nicolin Chen

The nature of ITS virtualization on ARM is done via hypercalls, so kernel handles all IOVA mappings for the MSI doorbell in iommu_dma_prepare_msi() and iommu_dma_compose_msi_msg(). The current virtualization solution with a 2-stage nested translation setup is to do 1:1 IOVA mappings at stage-1

Note that if we still intend to use that trick there is a known issue at kernel side that needs to be fixed.

ARM DEN 0049E.b IORT specification mandates that when RMRs are present, the OS must preserve PCIe configuration performed by the boot FW.

-----Original Message-----
From: Jason Gunthorpe [mailto:[email protected]]
Sent: 10 March 2023 15:33
To: Eric Auger <[email protected]>
Cc: Nicolin Chen <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Shameerali Kolothum Thodi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 01/14] iommu: Add iommu_get_unmanaged_domain helper

Quoted 6 lines · Eric Auger

Hi Nicolin,

Quoted 3 lines · Nicolin Chen

The nature of ITS virtualization on ARM is done via hypercalls, so kernel handles all IOVA mappings for the MSI doorbell in iommu_dma_prepare_msi() and iommu_dma_compose_msi_msg(). The

current

Quoted 5 lines

Quoted 2 lines

virtualization solution with a 2-stage nested translation setup is to do 1:1 IOVA mappings at stage-1

Note that if we still intend to use that trick there is a known issue at kernel side that needs to be fixed.

Quoted 3 lines

ARM DEN 0049E.b IORT specification mandates that when RMRs are present, the OS must preserve PCIe configuration performed by the boot FW.

This limitation doesn't seem necessary for this MSI stuff?

What is it for?

Quoted 32 lines

-----Original Message-----
From: Jason Gunthorpe [mailto:[email protected]]
Sent: 10 March 2023 15:33
To: Eric Auger <[email protected]>
Cc: Nicolin Chen <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Shameerali Kolothum Thodi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 01/14] iommu: Add iommu_get_unmanaged_domain helper

Quoted 6 lines · Eric Auger

Hi Nicolin,

Quoted 3 lines · Nicolin Chen

The nature of ITS virtualization on ARM is done via hypercalls, so kernel handles all IOVA mappings for the MSI doorbell in iommu_dma_prepare_msi() and iommu_dma_compose_msi_msg(). The

current

Quoted 8 lines

Quoted 2 lines

virtualization solution with a 2-stage nested translation setup is to do 1:1 IOVA mappings at stage-1

Note that if we still intend to use that trick there is a known issue at kernel side that needs to be fixed.

ARM DEN 0049E.b IORT specification mandates that when RMRs are present, the OS must preserve PCIe configuration performed by the boot FW.

Quoted 3 lines

This limitation doesn't seem necessary for this MSI stuff?

What is it for?

That is to make sure the Stream Ids specified in RMR are still valid and is not being reassigned by OS. The kernel checks for this(iort_rmr_has_dev()),
https://lore.kernel.org/linux-arm-kernel/[email protected]/

-----Original Message-----
From: Jason Gunthorpe [mailto:[email protected]]
Sent: 10 March 2023 15:57
To: Shameerali Kolothum Thodi <[email protected]>
Cc: Eric Auger <[email protected]>; Nicolin Chen <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 01/14] iommu: Add iommu_get_unmanaged_domain helper

Quoted 13 lines · Shameerali Kolothum Thodi

Quoted 13 lines

-----Original Message-----
From: Jason Gunthorpe [mailto:[email protected]]
Sent: 10 March 2023 15:33
To: Eric Auger <[email protected]>
Cc: Nicolin Chen <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Shameerali Kolothum Thodi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 01/14] iommu: Add

iommu_get_unmanaged_domain

Quoted 9 lines

Quoted 9 lines

helper

Quoted 6 lines · Eric Auger

Hi Nicolin,

Quoted 3 lines · Nicolin Chen

The nature of ITS virtualization on ARM is done via hypercalls, so kernel handles all IOVA mappings for the MSI doorbell in iommu_dma_prepare_msi() and iommu_dma_compose_msi_msg().

The

Quoted 10 lines

Quoted 10 lines

current

Quoted 8 lines

Quoted 2 lines

virtualization solution with a 2-stage nested translation setup is to do 1:1 IOVA mappings at stage-1

Note that if we still intend to use that trick there is a known issue at kernel side that needs to be fixed.

ARM DEN 0049E.b IORT specification mandates that when RMRs are present, the OS must preserve PCIe configuration performed by the boot FW.

Quoted 8 lines

Quoted 3 lines

This limitation doesn't seem necessary for this MSI stuff?

What is it for?

That is to make sure the Stream Ids specified in RMR are still valid and is not being reassigned by OS. The kernel checks for this(iort_rmr_has_dev()),

https://lore.kernel.org/linux-arm-kernel/20220420164836.1181-5-shameer

Quoted 1 line

[email protected]/

So "boot configration" is more like "don't change the RIDs"? Ie don't enable SRIOV?

Quoted 5 lines

https://lore.kernel.org/linux-arm-kernel/20220420164836.1181-5-shameer

Quoted 1 line

[email protected]/

So "boot configration" is more like "don't change the RIDs"? Ie don't enable SRIOV?

Yes. Don't think it will work with SR-IOV if you can't guarantee the RMR specified SID.

-----Original Message-----
From: Jason Gunthorpe [mailto:[email protected]]
Sent: 10 March 2023 16:21
To: Shameerali Kolothum Thodi <[email protected]>
Cc: Eric Auger <[email protected]>; Nicolin Chen <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 01/14] iommu: Add iommu_get_unmanaged_domain helper

Quoted 1 line · Shameerali Kolothum Thodi

Quoted 1 line

https://lore.kernel.org/linux-arm-kernel/20220420164836.1181-5-shameer

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

[email protected]/

So "boot configration" is more like "don't change the RIDs"? Ie don't

enable

Quoted 2 lines

Quoted 1 line

SRIOV?

Quoted 1 line

Yes. Don't think it will work with SR-IOV if you can't guarantee the RMR

specified

Quoted 1 line

SID.

So I think we are probably good them because vSR-IOV is already not supported by qemu, so it impossible for a VM to change the PCI configuration in a way that would alter the RID to SID mapping?

Quoted 28 lines

-----Original Message-----
From: Jason Gunthorpe [mailto:[email protected]]
Sent: 10 March 2023 16:21
To: Shameerali Kolothum Thodi <[email protected]>
Cc: Eric Auger <[email protected]>; Nicolin Chen <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 01/14] iommu: Add iommu_get_unmanaged_domain helper

Quoted 1 line · Shameerali Kolothum Thodi

Quoted 1 line

https://lore.kernel.org/linux-arm-kernel/20220420164836.1181-5-shameer

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

[email protected]/

So "boot configration" is more like "don't change the RIDs"? Ie don't

enable

Quoted 3 lines

Quoted 1 line

SRIOV?

Yes. Don't think it will work with SR-IOV if you can't guarantee the RMR

specified

Quoted 1 line

SID.

Quoted 4 lines

So I think we are probably good them because vSR-IOV is already not supported by qemu, so it impossible for a VM to change the PCI configuration in a way that would alter the RID to SID mapping?

Provided we fix the issue mentioned by Eric. This was discussed here previously,

https://lore.kernel.org/linux-arm-kernel/[email protected]/

Quoted 27 lines · Shameerali Kolothum Thodi

Quoted 27 lines

-----Original Message-----
From: Jason Gunthorpe [mailto:[email protected]]
Sent: 10 March 2023 16:21
To: Shameerali Kolothum Thodi <[email protected]>
Cc: Eric Auger <[email protected]>; Nicolin Chen <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 01/14] iommu: Add iommu_get_unmanaged_domain helper

On Fri, Mar 10, 2023 at 04:07:38PM +0000, Shameerali Kolothum Thodi wrote:
https://lore.kernel.org/linux-arm-kernel/20220420164836.1181-5-shameer

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

[email protected]/

So "boot configration" is more like "don't change the RIDs"? Ie don't

enable

Quoted 2 lines

Quoted 1 line

SRIOV?

Yes. Don't think it will work with SR-IOV if you can't guarantee the RMR

specified

Quoted 1 line

SID.

So I think we are probably good them because vSR-IOV is already not supported by qemu, so it impossible for a VM to change the PCI configuration in a way that would alter the RID to SID mapping?

Quoted 3 lines

Provided we fix the issue mentioned by Eric. This was discussed here previously,

https://lore.kernel.org/linux-arm-kernel/[email protected]/

Ah, I see so that we don't renumber the buses during PCI discovery..

It seems like Eric's issue is overly broad if we just want to block RID reassignment that doesn't impact MMIO layout.

But, still, why do we care about this?

The vIOMMU should virtualize the vSIDs right? So why does qemu give a vSID list to the guest anyhow? Shouldn't the guest use an algorithmic calculation from the vRID so that qemu can reverse it to the correct vPCI device and thus the correct vfio_device and then dev id in the iommu_domain?

Jason

Quoted 2 lines

It seems like Eric's issue is overly broad if we just want to block RID reassignment that doesn't impact MMIO layout.

IORT spec says

"
If reserved memory regions are present, the OS must preserve PCIe configuration performed by the boot
firmware. This preservation is required to ensure functional continuity of the endpoints that are using the reserved
memory regions. Therefore, RMR nodes must be supported by the inclusion of the PCI Firmware defined _DSM
for ignoring PCI boot configuration, Function 5, in the ACPI device object of the PCIe host bridge in ACPI
namespace. The _DSM method should return a value of 0 to indicate that the OS must honour the PCI
configuration that the firmware has done at boot time. See [PCIFW] for more details on this _DSM method.
"

Quoted 7 lines

But, still, why do we care about this?

The vIOMMU should virtualize the vSIDs right? So why does qemu give a vSID list to the guest anyhow? Shouldn't the guest use an algorithmic calculation from the vRID so that qemu can reverse it to the correct vPCI device and thus the correct vfio_device and then dev id in the iommu_domain?

I don't understand how this changes the above picture?

Quoted 4 lines · Eric Auger

Quoted 2 lines

It seems like Eric's issue is overly broad if we just want to block RID reassignment that doesn't impact MMIO layout.

IORT spec says

Quoted 14 lines

"
If reserved memory regions are present, the OS must preserve PCIe configuration performed by the boot
firmware. This preservation is required to ensure functional continuity of the endpoints that are using the reserved
memory regions. Therefore, RMR nodes must be supported by the inclusion of the PCI Firmware defined _DSM
for ignoring PCI boot configuration, Function 5, in the ACPI device object of the PCIe host bridge in ACPI
namespace. The _DSM method should return a value of 0 to indicate that the OS must honour the PCI
configuration that the firmware has done at boot time. See [PCIFW] for more details on this _DSM method.
"

I would say this spec language is overly broad. If the FW knows the reserved memory regions it creates are not sensitive to PCI layout then it should not be forced to set this flag.

Quoted 9 lines

Quoted 8 lines

But, still, why do we care about this?

The vIOMMU should virtualize the vSIDs right? So why does qemu give a vSID list to the guest anyhow? Shouldn't the guest use an algorithmic calculation from the vRID so that qemu can reverse it to the correct vPCI device and thus the correct vfio_device and then dev id in the iommu_domain?

I don't understand how this changes the above picture?

We are forced to use RMR because of the hacky GIC ITS stuff.

ITS placement is not sensitive to PCI layout.

ITS is not sensitive to bus numbers/etc.

vSID to dev_id should also be taken care of by QEMU even if bus numbers change and doesn't need to be fixed.

So let's have a reason why we need to do all this weird stuff beyond the spec says so.

If there is no actual functional issue we should not restrict the guest and provide RMR without the DSM method. Someone should go and update the spec if this offends them :)

Jason

Quoted 3 lines

I would say this spec language is overly broad. If the FW knows the reserved memory regions it creates are not sensitive to PCI layout then it should not be forced to set this flag.

But do we have any guarantee the bus numbers can't change. I thought the guest was allowed to re-number at will? While further thinking at it, all RID ID mappings should be affected by this concern, I mean not only RID 2 RMRs? What do I miss?

Quoted 1 line

We are forced to use RMR because of the hacky GIC ITS stuff.

well we are not obliged to use RMRs. My first revisions did not use it and created a non direct S1 mapping. This is just a commodity that simplifies the integration and was nicely suggested by jean.

External email: Use caution opening links or attachments

Quoted 26 lines

-----Original Message-----
From: Jason Gunthorpe [mailto:[email protected]]
Sent: 10 March 2023 16:21
To: Shameerali Kolothum Thodi <[email protected]>
Cc: Eric Auger <[email protected]>; Nicolin Chen <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 01/14] iommu: Add iommu_get_unmanaged_domain helper

Quoted 1 line · Shameerali Kolothum Thodi

Quoted 1 line

https://lore.kernel.org/linux-arm-kernel/20220420164836.1181-5-shameer

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

[email protected]/

So "boot configration" is more like "don't change the RIDs"? Ie don't

enable

Quoted 3 lines

Quoted 1 line

SRIOV?

Yes. Don't think it will work with SR-IOV if you can't guarantee the RMR

specified

Quoted 1 line

SID.

Quoted 4 lines

So I think we are probably good them because vSR-IOV is already not supported by qemu, so it impossible for a VM to change the PCI configuration in a way that would alter the RID to SID mapping?

Provided we fix the issue mentioned by Eric. This was discussed here previously,

https://lore.kernel.org/linux-arm-kernel/[email protected]/

-----Original Message-----
From: Nicolin Chen [mailto:[email protected]]
Sent: 16 March 2023 19:51
To: Shameerali Kolothum Thodi <[email protected]>; Eric Auger <[email protected]>
Cc: Jason Gunthorpe <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 01/14] iommu: Add iommu_get_unmanaged_domain helper

Hi Shameer/Eric,

Quoted 7 lines · Shameerali Kolothum Thodi

External email: Use caution opening links or attachments

Quoted 4 lines

-----Original Message-----
From: Jason Gunthorpe [mailto:[email protected]]
Sent: 10 March 2023 16:21
To: Shameerali Kolothum Thodi

<[email protected]>

Quoted 6 lines

Quoted 6 lines

Cc: Eric Auger <[email protected]>; Nicolin Chen <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 01/14] iommu: Add

iommu_get_unmanaged_domain

Quoted 6 lines

Quoted 6 lines

helper

Quoted 1 line · Shameerali Kolothum Thodi

Quoted 1 line

https://lore.kernel.org/linux-arm-kernel/20220420164836.1181-5-shame

Quoted 21 lines

Quoted 8 lines

er

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

[email protected]/

So "boot configration" is more like "don't change the RIDs"? Ie don't

enable

Quoted 2 lines

Quoted 1 line

SRIOV?

Quoted 9 lines

Quoted 2 lines

Yes. Don't think it will work with SR-IOV if you can't guarantee the RMR

specified

Quoted 1 line

SID.

So I think we are probably good them because vSR-IOV is already not supported by qemu, so it impossible for a VM to change the PCI configuration in a way that would alter the RID to SID mapping?

Provided we fix the issue mentioned by Eric. This was discussed here previously,

Quoted 2 lines

https://lore.kernel.org/linux-arm-kernel/bb3688c7-8f42-039e-e22f-65290
[email protected]/

Have we fixed the issue? I saw Lorenzo replying in that thread:
https://lore.kernel.org/linux-arm-kernel/Yi8nV8H4Jjlpadmk@lpieralisi/

Or, what's remaining here regarding this topic? Is it a blocker?

Quoted 55 lines

-----Original Message-----
From: Nicolin Chen [mailto:[email protected]]
Sent: 16 March 2023 19:51
To: Shameerali Kolothum Thodi <[email protected]>; Eric Auger <[email protected]>
Cc: Jason Gunthorpe <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 01/14] iommu: Add iommu_get_unmanaged_domain helper

Hi Shameer/Eric,

Quoted 7 lines · Shameerali Kolothum Thodi

External email: Use caution opening links or attachments

Quoted 4 lines

-----Original Message-----
From: Jason Gunthorpe [mailto:[email protected]]
Sent: 10 March 2023 16:21
To: Shameerali Kolothum Thodi

<[email protected]>

Quoted 6 lines

Quoted 6 lines

Cc: Eric Auger <[email protected]>; Nicolin Chen <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 01/14] iommu: Add

iommu_get_unmanaged_domain

Quoted 4 lines

Quoted 4 lines

helper

On Fri, Mar 10, 2023 at 04:07:38PM +0000, Shameerali Kolothum Thodi wrote:

https://lore.kernel.org/linux-arm-kernel/20220420164836.1181-5-shame

Quoted 17 lines

Quoted 5 lines

er

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

[email protected]/

So "boot configration" is more like "don't change the RIDs"? Ie don't

enable

Quoted 9 lines

Quoted 3 lines

Quoted 1 line

SRIOV?

Yes. Don't think it will work with SR-IOV if you can't guarantee the RMR

specified

Quoted 1 line

SID.

So I think we are probably good them because vSR-IOV is already not supported by qemu, so it impossible for a VM to change the PCI configuration in a way that would alter the RID to SID mapping?

Provided we fix the issue mentioned by Eric. This was discussed here previously,

Quoted 6 lines

Quoted 2 lines

https://lore.kernel.org/linux-arm-kernel/bb3688c7-8f42-039e-e22f-65290
[email protected]/

Have we fixed the issue? I saw Lorenzo replying in that thread:
https://lore.kernel.org/linux-arm-kernel/Yi8nV8H4Jjlpadmk@lpieralisi/

Or, what's remaining here regarding this topic? Is it a blocker?

[+Lorenzo]

Not sure it is fixed yet. Also, assuming we take RMR path, do we plan to support DT base Guests at all?

Thanks,
Shameer

Add the following data structures for corresponding ioctls:
iommu_hwpt_arm_smmuv3 => IOMMUFD_CMD_HWPT_ALLOC
iommu_hwpt_invalidate_arm_smmuv3 => IOMMUFD_CMD_HWPT_INVALIDATE

Also, add IOMMU_HW_INFO_TYPE_ARM_SMMUV3 and IOMMU_PGTBL_TYPE_ARM_SMMUV3_S1 to the header and corresponding type/size arrays.

Signed-off-by Nicolin Chen <[email protected]>

include/uapi/linux/iommufd.h +2 -0

+/**
+ * struct iommu_hwpt_arm_smmuv3 - ARM SMMUv3 specific page table data

+#define IOMMU_SMMUV3_FLAG_VMID (1 << 1) /* vmid override */

(quoted diff) +17 -0

+	__u64 flags;
+	__u32 s2vmid;
+	__u32 __reserved;
+	__u64 s1ctxptr;
+	__u64 s1cdmax;
+	__u64 s1fmt;
+	__u64 s1dss;
+};
+
 
 
+/**
+ * struct iommu_hwpt_invalidate_arm_smmuv3 - ARM SMMUv3 cahce invalidation info
+ * @flags: boolean attributes of cache invalidation command
+ * @opcode: opcode of cache invalidation command
+ * @ssid: SubStream ID
+ * @granule_size: page/block size of the mapping in bytes
+ * @range: IOVA range to invalidate
+ */

Although we can keep the alloc and hardware info separate for each IOMMU architecture, we should try to come up with common invalidation methods.

Ideally I'd like something like this for vhost-iommu:

* slow path through userspace for complex requests like attach-table and probe, where the VMM can decode arch-specific information and translate them to iommufd and vhost-iommu ioctls to update the configuration.

* fast path within the kernel for performance-critical requests like invalidate, page request and response. It would be absurd for the vhost-iommu driver to translate generic invalidation requests from the guest into arch-specific commands with special opcodes, when the next step is calling the IOMMU driver which does that for free.

During previous discussions we came up with generic invalidations that could fit both Arm and x86 [1][2]. The only difference was the ASID (called archid/id in those proposals) which VT-d didn't need. Could we try to build on that?

Quoted 2 lines · Jean-Philippe Brucker

Although we can keep the alloc and hardware info separate for each IOMMU architecture, we should try to come up with common invalidation methods.

The invalidation language is tightly linked to the actual cache block and cache tag in the IOMMU HW design.

Generality will loose or
obfuscate the necessary specificity that is required for creating real vIOMMUs.

Further, invalidation is a fast path, it is crazy to take a vIOMMU of a real HW receving a native invalidation request, mangle it to some obfuscated kernel version and then de-mangle it again in the kernel driver. IMHO ideally qemu will simply point the invalidation at the WQE in the SW vIOMMU command queue and invoke the ioctl. (Nicolin, we should check more into this)

The purpose of these interfaces is to support high performance full functionality vIOMMUs of the real HW, we should not loose sight of that goal.

We are actually planning to go futher and expose direct invalidation work queues complete with HW doorbells to userspace. This obviously must be in native HW format.

Nicolin, I think we should tweak the uAPI here so that the invalidation opaque data has a format tagged on its own, instead of re-using the HWPT tag. Ie you can have a ARM SMMUv3 invalidate type tag and also a virtio-viommu invalidate type tag.

This will allow Jean to put the invalidation decoding in the iommu drivers if we think that is the right direction. virtio can standardize it as a "HW format".

Quoted 11 lines

Ideally I'd like something like this for vhost-iommu:

* slow path through userspace for complex requests like attach-table and probe, where the VMM can decode arch-specific information and translate them to iommufd and vhost-iommu ioctls to update the configuration.

* fast path within the kernel for performance-critical requests like invalidate, page request and response. It would be absurd for the vhost-iommu driver to translate generic invalidation requests from the guest into arch-specific commands with special opcodes, when the next step is calling the IOMMU driver which does that for free.

Someone has to do the conversion. If you don't think virito should do it then I'd be OK to add a type tag for virtio format invalidation and put it in the IOMMU driver.

But given virtio overall already has to know *alot* about how the HW it is wrapping works I don't think it is necessarily absurd for virtio to do the conversion. I'd like to evaluate this in patches in context with how much other unique HW code ends up in kernel-side vhost-iommu.

However, I don't know the rational for virtio-viommu, it seems like a strange direction to me.

All the iommu drivers have native command
queues. ARM and AMD are both supporting native command queues directly in the guest, complete with a direct guest MMIO doorbell ring.

If someone wants to optimize this I'd think the way to do it is to use virtio like techniques to put SW command queue processing in the kernel iommu driver and continue to use the HW native interface in the VM.

What benifit comes from replacing the HW native interface with virtio? Especially on ARM where the native interface is pretty clean?

Quoted 4 lines

During previous discussions we came up with generic invalidations that could fit both Arm and x86 [1][2]. The only difference was the ASID (called archid/id in those proposals) which VT-d didn't need. Could we try to build on that?

IMHO this was just unioning all the different invalidation types together. It makes sense for something like virtio but it is illogical/obfuscated as a user/kernel interface since it still requires a userspace HW driver to understand what subset of the invalidations are used on the actual HW.

Quoted 5 lines · Jason Gunthorpe

Quoted 2 lines · Jean-Philippe Brucker

Although we can keep the alloc and hardware info separate for each IOMMU architecture, we should try to come up with common invalidation methods.

Quoted 2 lines

The invalidation language is tightly linked to the actual cache block and cache tag in the IOMMU HW design.

Concretely though, what are the incompatibilities between the HW designs? They all need to remove a range of TLB entries, using some address space tag. But if there is an actual difference I do need to know.

Quoted 7 lines

The purpose of these interfaces is to support high performance full functionality vIOMMUs of the real HW, we should not loose sight of that goal.

We are actually planning to go futher and expose direct invalidation work queues complete with HW doorbells to userspace. This obviously must be in native HW format.

Doesn't seem relevant since direct access to command queue wouldn't use this struct.

Quoted 3 lines

Someone has to do the conversion. If you don't think virito should do it then I'd be OK to add a type tag for virtio format invalidation and put it in the IOMMU driver.

Implementing two invalidation formats in each IOMMU driver does not seem practical.

Quoted 2 lines

However, I don't know the rational for virtio-viommu, it seems like a strange direction to me.

A couple of reasons are relevant here: non-QEMU VMMs don't want to emulate all vendor IOMMUs, new architectures get vIOMMU mostly for free,

Quoted 3 lines

All the iommu drivers have native command
queues. ARM and AMD are both supporting native command queues directly in the guest, complete with a direct guest MMIO doorbell ring.

Arm SMMUv3 mandates a single global command queue (SMMUv2 uses registers). An SMMUv3 can optionally implement multiple command queues, though I don't know if they can be safely assigned to guests.

For a lot of SMMUv3 implementations that have a single queue and for other architectures, we can do better than hardware emulation.

Quoted 4 lines

If someone wants to optimize this I'd think the way to do it is to use virtio like techniques to put SW command queue processing in the kernel iommu driver and continue to use the HW native interface in the VM.

I didn't get which kernel this is.

Quoted 5 lines

IMHO this was just unioning all the different invalidation types together. It makes sense for something like virtio but it is illogical/obfuscated as a user/kernel interface since it still requires a userspace HW driver to understand what subset of the invalidations are used on the actual HW.

As above, decoding arch-specific structures into generic ones is what an emulated IOMMU does,

and it doesn't make a performance difference in which
format it forwards that to the kernel. The host IOMMU driver checks the guest request and copies them into the command queue. Whether that request comes in the form of a structure binary-compatible with Arm SMMUvX.Y, or some generic structure, does not make a difference.

Quoted 3 lines

Concretely though, what are the incompatibilities between the HW designs? They all need to remove a range of TLB entries, using some address space tag. But if there is an actual difference I do need to know.

For instance the address space tags and the cache entires they match to are wildly different.

ARM uses a fine grained ASID and Intel uses a shared ASID called a DID and incorporates the PASID into the cache tag.

AMD uses something called a DID that covers a different set of stuff than the Intel DID, and it doesn't seem to work for nesting. AMD uses PASID as the primary nested cache tag.

This is because SMMUv3 has no option to keep the PASID table in the hypervisor so you are sadly forced to expose both the native ASID and native PASID caches to the virtio protocol.

Given that the VM virtio driver has to have SMMUv3 specific code to handle the CD table it must get, I don't see the problem with also having SMMUv3 specific code in the hypervisor virtio driver to handle invalidating based on the CD table.

Quoted 2 lines

A couple of reasons are relevant here: non-QEMU VMMs don't want to emulate all vendor IOMMUs, new architectures get vIOMMU mostly for free,

So your argument is you can implement a simple map/unmap API riding on the common IOMMU API and this is portable?

Seems sensible, but that falls apart pretty quickly when we talk about nesting.. I don't think we can avoid VMM components to set this up, so it stops being portable. At that point I'm back to asking why not use the real HW model?

Quoted 9 lines

Quoted 4 lines

All the iommu drivers have native command
queues. ARM and AMD are both supporting native command queues directly in the guest, complete with a direct guest MMIO doorbell ring.

Arm SMMUv3 mandates a single global command queue (SMMUv2 uses registers). An SMMUv3 can optionally implement multiple command queues, though I don't know if they can be safely assigned to guests.

It is not standardized by ARM, but it can (and has) been done.

Quoted 2 lines

For a lot of SMMUv3 implementations that have a single queue and for other architectures, we can do better than hardware emulation.

How is using a SW emulated virtio formatted queue better than using a SW emulated SMMUv3 ECMDQ?

Quoted 2 lines

As above, decoding arch-specific structures into generic ones is what an emulated IOMMU does,

No, it is what virtio wants to do. We are deliberately trying not to do that for real accelerated HW vIOMMU emulators.

Quoted 6 lines

and it doesn't make a performance difference in which
format it forwards that to the kernel. The host IOMMU driver checks the guest request and copies them into the command queue. Whether that request comes in the form of a structure binary-compatible with Arm SMMUvX.Y, or some generic structure, does not make a difference.

It is not the structure layouts that matter!

It is the semantic meaning of each request, on each unique piece of hardware. We actually want to leak the subtle semantic differences to userspace.

Quoted 2 lines

For a lot of SMMUv3 implementations that have a single queue and for other architectures, we can do better than hardware emulation.

How is using a SW emulated virtio formatted queue better than using a SW emulated SMMUv3 ECMDQ?

Quoted 3 lines · Jason Gunthorpe

Quoted 2 lines

For a lot of SMMUv3 implementations that have a single queue and for other architectures, we can do better than hardware emulation.

Quoted 2 lines

How is using a SW emulated virtio formatted queue better than using a SW emulated SMMUv3 ECMDQ?

Since it's not been said, the really big thing is that virtio explicitly informs the host whenever the guest maps something. Emulating SMMUv3 means the host has to chase all the pagetable pointers in guest memory and trap writes such that it has visibility of invalid->valid transitions and can update the physical shadow pagetable correspondingly.

Quoted 7 lines · Robin Murphy

Quoted 5 lines · Jason Gunthorpe

Quoted 2 lines

For a lot of SMMUv3 implementations that have a single queue and for other architectures, we can do better than hardware emulation.

How is using a SW emulated virtio formatted queue better than using a SW emulated SMMUv3 ECMDQ?

Quoted 5 lines

Since it's not been said, the really big thing is that virtio explicitly informs the host whenever the guest maps something. Emulating SMMUv3 means the host has to chase all the pagetable pointers in guest memory and trap writes such that it has visibility of invalid->valid transitions and can update the physical shadow pagetable correspondingly.

Sorry, I mean in the context of future virtio-iommu that is providing nested translation.

eg why would anyone want to use virtio to provide SMMUv3 based HW accelerated nesting?

Jean suggested that the invalidation flow for virtio-iommu could be faster because it is in kernel, but I'm saying that we could also make the SMMUv3 invalidation in-kernel with the same basic technique. (and actively wondering if we should put more focus on that)

I understand the appeal of the virtio scheme with its current map/unmap interface.

I could also see some appeal of a simple virtio-iommu SVA that could point map a CPU page table as an option. The guest already has to know how to manage these anyhow so it is nicely general.

If iommufd could provide a general cross-driver API to set exactly that scenario up then VMM code could also be general. That seems prettty interesting.

But if the plan is to expose more detailed stuff like the CD or GCR3 PASID tables as something the guest has to manipulate and then a bunch of special invalidation to support that, and VMM code to back it, then I'm questioning the whole point. We lost the generality.

Just use the normal HW accelerated SMMUv3 nesting model instead.

If virtio-iommu SVA is really important for ARM then I'd suggest SMMUv3 should gain a new HW capability to allowed the CD table to be in hypervisor memory so it works consistently for virtio-iommu SVA.

about the nitty-gritty of all the IOMMU-specific moving parts around it. For guests that want to get into more advanced stuff like managing their own PASID tables, pushing them towards "native" nesting probably does make more sense.

From: Jason Gunthorpe <[email protected]>
Sent: Saturday, March 11, 2023 12:03 AM

Quoted 3 lines · Robin Murphy

about the nitty-gritty of all the IOMMU-specific moving parts around it. For guests that want to get into more advanced stuff like managing their own PASID tables, pushing them towards "native" nesting probably does make

more

Quoted 1 line

sense.

IMHO with the simplified virtio model I would say the guest should not have its own PASID table.

hyper trap to install a PASID and let the hypervisor driver handle this abstractly. If abstractly is the whole point and benifit then virtio should lean into that.

This also means virtio protocol doesn't do PASID invalidation. It invalidates an ASID and the hypervisor takes care of whatever it is connected to. Very simple and general for the VM.

Adding a S1 iommu_domain op for invalidate address range is perfectly fine and the virtio kernel hypervisor driver can call it generically.

The primary reason to have guest-owned PASID tables is CC stuff, which definitely won't be part of virtio-iommu.

From: Robin Murphy <[email protected]>
Sent: Friday, March 10, 2023 11:57 PM

Quoted 4 lines

If iommufd could provide a general cross-driver API to set exactly that scenario up then VMM code could also be general. That seems prettty interesting.

Indeed, I've always assumed the niche for virtio would be that kind of in-between use-case using nesting to accelerate simple translation, where we plug a guest-owned pagetable into a host-owned context. That way the guest retains the simple virtio interface and only needs to understand a pagetable format (or as you say, simply share a CPU pagetable) without having to care about the nitty-gritty of all the IOMMU-specific moving parts around it. For guests that want to get into more advanced stuff like managing their own PASID tables, pushing them towards "native" nesting probably does make more sense.

Nicolin, I think we should tweak the uAPI here so that the invalidation opaque data has a format tagged on its own, instead of re-using the HWPT tag. Ie you can have a ARM SMMUv3 invalidate type tag and also a virtio-viommu invalidate type tag.

Quoted 4 lines · Jason Gunthorpe

Nicolin, I think we should tweak the uAPI here so that the invalidation opaque data has a format tagged on its own, instead of re-using the HWPT tag. Ie you can have a ARM SMMUv3 invalidate type tag and also a virtio-viommu invalidate type tag.

The invalidation tage is shared with the hwpt allocation. Does it mean that virtio-iommu won't have it's own allocation tag?

Quoted 7 lines · Nicolin Chen

Quoted 4 lines · Jason Gunthorpe

Nicolin, I think we should tweak the uAPI here so that the invalidation opaque data has a format tagged on its own, instead of re-using the HWPT tag. Ie you can have a ARM SMMUv3 invalidate type tag and also a virtio-viommu invalidate type tag.

Quoted 2 lines

The invalidation tage is shared with the hwpt allocation. Does it mean that virtio-iommu won't have it's own allocation tag?

I'm not entirely sure what you mean by allocation tag.

Note that none of this is set in stone. It copies the Linux API we originally discussed, but we were waiting for progress on that front before committing to anything. Now we'll probably align to the new API where possible, leaving out what doesn't work for virtio-iommu.

Quoted 4 lines · Jason Gunthorpe

Nicolin, I think we should tweak the uAPI here so that the invalidation opaque data has a format tagged on its own, instead of re-using the HWPT tag. Ie you can have a ARM SMMUv3 invalidate type tag and also a virtio-viommu invalidate type tag.

The invalidation tage is shared with the hwpt allocation. Does it mean that virtio-iommu won't have it's own allocation tag?

Quoted 7 lines · Nicolin Chen

Quoted 4 lines · Jason Gunthorpe

Nicolin, I think we should tweak the uAPI here so that the invalidation opaque data has a format tagged on its own, instead of re-using the HWPT tag. Ie you can have a ARM SMMUv3 invalidate type tag and also a virtio-viommu invalidate type tag.

Quoted 2 lines

The invalidation tage is shared with the hwpt allocation. Does it mean that virtio-iommu won't have it's own allocation tag?

We probably shouldn't assume it will

-----Original Message-----
From: Jean-Philippe Brucker [mailto:[email protected]]
Sent: 09 March 2023 13:42
To: Nicolin Chen <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Shameerali Kolothum Thodi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 02/14] iommufd: Add nesting related data structures for ARM SMMUv3

Hi Nicolin,

Quoted 2 lines · Nicolin Chen

Add the following data structures for corresponding ioctls:
iommu_hwpt_arm_smmuv3 =>

IOMMUFD_CMD_HWPT_ALLOC

Quoted 1 line

iommu_hwpt_invalidate_arm_smmuv3 =>

IOMMUFD_CMD_HWPT_INVALIDATE

Quoted 2 lines

Also, add IOMMU_HW_INFO_TYPE_ARM_SMMUV3 and

IOMMU_PGTBL_TYPE_ARM_SMMUV3_S1

Quoted 6 lines

to the header and corresponding type/size arrays.

Signed-off-by Nicolin Chen <[email protected]>

+/**
+ * struct iommu_hwpt_arm_smmuv3 - ARM SMMUv3 specific page table

data

include/uapi/linux/iommufd.h struct iommu_hwpt_intel_vtd { · line 428

Quoted 10 lines

+ *
+ * @flags: page table entry attributes
+ * @s2vmid: Virtual machine identifier
+ * @s1ctxptr: Stage-1 context descriptor pointer
+ * @s1cdmax: Number of CDs pointed to by s1ContextPtr
+ * @s1fmt: Stage-1 Format
+ * @s1dss: Default substream
+ */

include/uapi/linux/iommufd.h +2 -0

+struct iommu_hwpt_arm_smmuv3 {
+#define IOMMU_SMMUV3_FLAG_S2	(1 << 0) /* if unset, stage-1 */

I don't understand the purpose of this flag, since the structure only provides stage-1 configuration fields

Quoted 1 line

+#define IOMMU_SMMUV3_FLAG_VMID (1 << 1) /* vmid override */

Doesn't this break isolation? The VMID space is global for the SMMU, so the guest could access cached mappings of another device

On platforms that supports BTM [1], we may need the VMID allocated by KVM. But again getting that from user pace doesn't look safe. I have attempted to revise the earlier RFC to pin and use the KVM VMID from SMMUv3 here[2].

" In the new design we can require from the start that creating a nesting IOMMU container through /dev/iommu *must* come with a KVM context, that way we're sure to reuse the existing VMID. "

-----Original Message-----
From: Jason Gunthorpe [mailto:[email protected]]
Sent: 09 March 2023 15:40
To: Shameerali Kolothum Thodi <[email protected]>
Cc: Jean-Philippe Brucker <[email protected]>; Nicolin Chen <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 02/14] iommufd: Add nesting related data structures for ARM SMMUv3

Quoted 1 line · Shameerali Kolothum Thodi

On platforms that supports BTM [1], we may need the VMID allocated by

KVM.

Quoted 1 line

But again getting that from user pace doesn't look safe. I have attempted

to revise

Quoted 1 line

the earlier RFC to pin and use the KVM VMID from SMMUv3 here[2].

Gurk

Quoted 1 line

" In the new design we can require from the start that creating a nesting

IOMMU

Quoted 2 lines

container through /dev/iommu *must* come with a KVM context, that way we're sure to reuse the existing VMID. "

I've been dreading this but yes I execpt we will eventually need to connect kvm and iommufd together. The iommu driver can receive a kvm pointer as part of the alloc domain operation to do any setups like this.

If there is no KVM it should either fail to setup the domain or setup a domain disconnected from KVM.

If IOMMU HW and KVM HW are using the same ID number space then arguably the two kernel drivers need to use a shared ID allocator in the arch, regardless of what iommufd/etc does. Using KVM should not be mandatory for iommufd.

For ARM cases where there is no shared VMID space with KVM, the ARM VMID should be somehow assigned to the iommfd_ctx itself and the alloc domain op should receive it from there.

Nicolin, that seems to be missing in this series? I'm not entirely sure how to elegantly code it :\

Jason

Quoted 3 lines

For ARM cases where there is no shared VMID space with KVM, the ARM VMID should be somehow assigned to the iommfd_ctx itself and the alloc domain op should receive it from there.

Is there any use of VMID outside SMMUv3? I was thinking if nested domain alloc doesn't provide the KVM instance, then SMMUv3 can use its internal VMID.

-----Original Message-----
From: Jason Gunthorpe [mailto:[email protected]]
Sent: 09 March 2023 16:00
To: Shameerali Kolothum Thodi <[email protected]>
Cc: Jean-Philippe Brucker <[email protected]>; Nicolin Chen <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 02/14] iommufd: Add nesting related data structures for ARM SMMUv3

Quoted 5 lines · Shameerali Kolothum Thodi

Quoted 3 lines

For ARM cases where there is no shared VMID space with KVM, the ARM VMID should be somehow assigned to the iommfd_ctx itself and the alloc domain op should receive it from there.

Is there any use of VMID outside SMMUv3? I was thinking if nested domain

alloc

Quoted 1 line

doesn't provide the KVM instance, then SMMUv3 can use its internal VMID.

When we talk about exposing an SMMUv3 IOMMU CMDQ directly to userspace then
VMID is the security token that protects it.

So in that environment every domain under the same iommufd should share the same VMID so that the CMDQ's also share the same VMID.

I expect this to be a common sort of requirement as we will see userspace command queues in the other HW as well.

So, I suppose the answer for now is that ARM SMMUv3 should just allocate one VMID per iommu_domain and there should be no VMID in the uapi at all.

Moving all iommu_domains to share the same VMID is a future patch.

Though.. I have no idea how vVMID is handled in the SMMUv3 architecture. I suppose the guest IOMMU HW caps are set in a way that it knows it does not have VMID?

External email: Use caution opening links or attachments

Quoted 37 lines

-----Original Message-----
From: Jason Gunthorpe [mailto:[email protected]]
Sent: 09 March 2023 16:00
To: Shameerali Kolothum Thodi <[email protected]>
Cc: Jean-Philippe Brucker <[email protected]>; Nicolin Chen <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 02/14] iommufd: Add nesting related data structures for ARM SMMUv3

Quoted 5 lines · Shameerali Kolothum Thodi

Quoted 3 lines

For ARM cases where there is no shared VMID space with KVM, the ARM VMID should be somehow assigned to the iommfd_ctx itself and the alloc domain op should receive it from there.

Is there any use of VMID outside SMMUv3? I was thinking if nested domain

alloc

Quoted 1 line

doesn't provide the KVM instance, then SMMUv3 can use its internal VMID.

When we talk about exposing an SMMUv3 IOMMU CMDQ directly to userspace then
VMID is the security token that protects it.

So in that environment every domain under the same iommufd should share the same VMID so that the CMDQ's also share the same VMID.

I expect this to be a common sort of requirement as we will see userspace command queues in the other HW as well.

So, I suppose the answer for now is that ARM SMMUv3 should just allocate one VMID per iommu_domain and there should be no VMID in the uapi at all.

Quoted 5 lines

Moving all iommu_domains to share the same VMID is a future patch.

Though.. I have no idea how vVMID is handled in the SMMUv3 architecture. I suppose the guest IOMMU HW caps are set in a way that it knows it does not have VMID?

I think, Guest only sets up the SMMUv3 S1 stage and it doesn't use VMID.

Quoted 46 lines · Shameerali Kolothum Thodi

External email: Use caution opening links or attachments

Quoted 42 lines

-----Original Message-----
From: Jason Gunthorpe [mailto:[email protected]]
Sent: 09 March 2023 16:00
To: Shameerali Kolothum Thodi <[email protected]>
Cc: Jean-Philippe Brucker <[email protected]>; Nicolin Chen <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 02/14] iommufd: Add nesting related data structures for ARM SMMUv3

Quoted 5 lines · Shameerali Kolothum Thodi

Quoted 3 lines

For ARM cases where there is no shared VMID space with KVM, the ARM VMID should be somehow assigned to the iommfd_ctx itself and the alloc domain op should receive it from there.

Is there any use of VMID outside SMMUv3? I was thinking if nested domain

alloc

Quoted 1 line

doesn't provide the KVM instance, then SMMUv3 can use its internal VMID.

When we talk about exposing an SMMUv3 IOMMU CMDQ directly to userspace then
VMID is the security token that protects it.

So in that environment every domain under the same iommufd should share the same VMID so that the CMDQ's also share the same VMID.

I expect this to be a common sort of requirement as we will see userspace command queues in the other HW as well.

So, I suppose the answer for now is that ARM SMMUv3 should just allocate one VMID per iommu_domain and there should be no VMID in the uapi at all.

Moving all iommu_domains to share the same VMID is a future patch.

Though.. I have no idea how vVMID is handled in the SMMUv3 architecture. I suppose the guest IOMMU HW caps are set in a way that it knows it does not have VMID?

Quoted 1 line

I think, Guest only sets up the SMMUv3 S1 stage and it doesn't use VMID.

Yea, a vmid is only allocated in an S2 domain allocation. So, a guest allocating only S1 domains always sets VMID=0. Yet, I think that the hypervisor or some where in host kernel should replace the VMID=0 with a unified VMID.

Quoted 4 lines

Yea, a vmid is only allocated in an S2 domain allocation. So, a guest allocating only S1 domains always sets VMID=0. Yet, I think that the hypervisor or some where in host kernel should replace the VMID=0 with a unified VMID.

Ah, I just recall a conversation with Jason that a VM should only have one S2 domain. In that case, the VMID is already unified?

Quoted 3 lines · Shameerali Kolothum Thodi

On platforms that supports BTM [1], we may need the VMID allocated by KVM. But again getting that from user pace doesn't look safe. I have attempted to revise the earlier RFC to pin and use the KVM VMID from SMMUv3 here[2].

Gurk

Quoted 3 lines

" In the new design we can require from the start that creating a nesting IOMMU container through /dev/iommu *must* come with a KVM context, that way we're sure to reuse the existing VMID. "

I've been dreading this but yes I execpt we will eventually need to connect kvm and iommufd together. The iommu driver can receive a kvm pointer as part of the alloc domain operation to do any setups like this.

If there is no KVM it should either fail to setup the domain or setup a domain disconnected from KVM.

If IOMMU HW and KVM HW are using the same ID number space then arguably the two kernel drivers need to use a shared ID allocator in the arch, regardless of what iommufd/etc does. Using KVM should not be mandatory for iommufd.

For ARM cases where there is no shared VMID space with KVM, the ARM VMID should be somehow assigned to the iommfd_ctx itself and the alloc domain op should receive it from there.

Nicolin, that seems to be missing in this series? I'm not entirely sure how to elegantly code it :\

include/uapi/linux/iommufd.h struct iommu_hwpt_intel_vtd { · line 428

Quoted 2 lines

include/uapi/linux/iommufd.h +2 -0

+struct iommu_hwpt_arm_smmuv3 {
+#define IOMMU_SMMUV3_FLAG_S2 (1 << 0) /* if unset, stage-1 */

I don't understand the purpose of this flag, since the structure only provides stage-1 configuration fields

Quoted 1 line

+#define IOMMU_SMMUV3_FLAG_VMID (1 << 1) /* vmid override */

Doesn't this break isolation? The VMID space is global for the SMMU, so the guest could access cached mappings of another device

Hi Nicolin,

Quoted 10 lines · Nicolin Chen

Add the following data structures for corresponding ioctls:
iommu_hwpt_arm_smmuv3 => IOMMUFD_CMD_HWPT_ALLOC
iommu_hwpt_invalidate_arm_smmuv3 => IOMMUFD_CMD_HWPT_INVALIDATE

Also, add IOMMU_HW_INFO_TYPE_ARM_SMMUV3 and IOMMU_PGTBL_TYPE_ARM_SMMUV3_S1 to the header and corresponding type/size arrays.

Signed-off-by Nicolin Chen <[email protected]>

include/uapi/linux/iommufd.h +2 -0

+/**
+ * struct iommu_hwpt_arm_smmuv3 - ARM SMMUv3 specific page table data

include/uapi/linux/iommufd.h struct iommu_hwpt_intel_vtd { · line 428

Quoted 10 lines

include/uapi/linux/iommufd.h +10 -0

+ *
+ * @flags: page table entry attributes
+ * @s2vmid: Virtual machine identifier
+ * @s1ctxptr: Stage-1 context descriptor pointer
+ * @s1cdmax: Number of CDs pointed to by s1ContextPtr
+ * @s1fmt: Stage-1 Format
+ * @s1dss: Default substream
+ */
+struct iommu_hwpt_arm_smmuv3 {
+#define IOMMU_SMMUV3_FLAG_S2	(1 << 0) /* if unset, stage-1 */

I don't understand the purpose of this flag, since the structure only provides stage-1 configuration fields

Quoted 1 line

+#define IOMMU_SMMUV3_FLAG_VMID (1 << 1) /* vmid override */

Doesn't this break isolation? The VMID space is global for the SMMU, so the guest could access cached mappings of another device

Quoted 18 lines

(quoted diff) +17 -0

+	__u64 flags;
+	__u32 s2vmid;
+	__u32 __reserved;
+	__u64 s1ctxptr;
+	__u64 s1cdmax;
+	__u64 s1fmt;
+	__u64 s1dss;
+};
+
 
+/**
+ * struct iommu_hwpt_invalidate_arm_smmuv3 - ARM SMMUv3 cahce invalidation info
+ * @flags: boolean attributes of cache invalidation command
+ * @opcode: opcode of cache invalidation command
+ * @ssid: SubStream ID
+ * @granule_size: page/block size of the mapping in bytes
+ * @range: IOVA range to invalidate
+ */

Review

Quoted 10 lines

(quoted diff) +10 -0

+struct iommu_hwpt_invalidate_arm_smmuv3 {
+#define IOMMU_SMMUV3_CMDQ_TLBI_VA_LEAF	(1 << 0)
+	__u64 flags;
+	__u8 opcode;
+	__u8 padding[3];
+	__u32 asid;
+	__u32 ssid;
+	__u32 granule_size;
+	struct iommu_iova_range range;
+};

Although we can keep the alloc and hardware info separate for each IOMMU architecture, we should try to come up with common invalidation methods.

It matters because things like vSVA, or just efficient dynamic mappings, will require optimal invalidation latency. A paravirtual interface like vhost-iommu can help with that, as the host kernel will handle guest invalidations directly instead of doing a round-trip to host userspace (and we'll likely want the same path for PRI.)

Supporting HW page tables for a common PV IOMMU does require some architecture-specific knowledge, but invalidation messages contain roughly the same information on all architectures. The PV IOMMU won't include command opcodes for each possible architecture if one generic command does the same job.

Ideally I'd like something like this for vhost-iommu:

* slow path through userspace for complex requests like attach-table and probe, where the VMM can decode arch-specific information and translate them to iommufd and vhost-iommu ioctls to update the configuration.

* fast path within the kernel for performance-critical requests like invalidate, page request and response. It would be absurd for the vhost-iommu driver to translate generic invalidation requests from the guest into arch-specific commands with special opcodes, when the next step is calling the IOMMU driver which does that for free.

During previous discussions we came up with generic invalidations that could fit both Arm and x86 [1][2]. The only difference was the ASID (called archid/id in those proposals) which VT-d didn't need. Could we try to build on that?

[1] https://elixir.bootlin.com/linux/v5.17/source/include/uapi/linux/iommu.h#L161
[2] https://lists.oasis-open.org/archives/virtio-dev/202102/msg00014.html

Thanks,
Jean

I do agree with Jean. We spent a lot of efforts all together to define this generic invalidation API and if there is compelling reason that prevents from using it, we should try to reuse it.

From: Jason Gunthorpe <[email protected]>
Sent: Friday, March 10, 2023 8:52 PM

Quoted 3 lines · Eric Auger

I do agree with Jean. We spent a lot of efforts all together to define this generic invalidation API and if there is compelling reason that prevents from using it, we should try to reuse it.

That's the compelling reason in a nutshell right there.

Alot of time was invested to create something that might be general. We still don't know if it is well defined and general. Even more time is going to be required on it before it could go forward. In future more time will be needed for every future HW to try and fit into it. We don't even know if it will scale to future HW. Nobody has even checked what today's POWER and S390 HW need.

vs, this stuff was made in a few days. We know it is correct as a uAPI since it mirrors the HW and we know it is scalable to different HW schemes if they come up.

So I don't see a good reason to take a risk on a "general" uAPI. If we make this wrong it could seriously damage the main goal of iommufd - to build accelerated vIOMMU models.

The IOMMU_RESV_SW_MSI is a kernel-managed domain thing. So, it should be only setup on a kernel-managed domain only. If the attaching domain is a user-managed domain, redirect the hwpt to hwpt->parent to do it correctly.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/iommufd/device.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/iommu/iommufd/device.c +0 -0

index f95b558f5e95..a3e7d2889164 100644

drivers/iommu/iommufd/device.c +0 -0

 	if (sw_msi_start != PHYS_ADDR_MAX && !hwpt->msi_cookie) {
 		rc = iommu_get_msi_cookie(hwpt->domain, sw_msi_start);
 		if (rc)

External email: Use caution opening links or attachments

Hi Nicolin,

Quoted 9 lines · Nicolin Chen

The IOMMU_RESV_SW_MSI is a kernel-managed domain thing. So, it should be only setup on a kernel-managed domain only. If the attaching domain is a user-managed domain, redirect the hwpt to hwpt->parent to do it correctly.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/iommufd/device.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/iommu/iommufd/device.c static int iommufd_group_setup_msi(struct iommufd_group *igroup, · line 353

Quoted 10 lines

drivers/iommu/iommufd/device.c +1 -1

index f95b558f5e95..a3e7d2889164 100644
@@ -350,7 +350,8 @@ static int iommufd_group_setup_msi(struct iommufd_group *igroup,
       * call iommu_get_msi_cookie() on its behalf. This is necessary to setup
       * the MSI window so iommu_dma_prepare_msi() can install pages into our
       * domain after request_irq(). If it is not done interrupts will not
-      * work on this domain.
+      * work on this domain. And the msi_cookie should be always set into the

s/And the/The/

Quoted 4 lines

drivers/iommu/iommufd/device.c +1 -0

+      * kernel-managed (parent) domain.
       *
       * FIXME: This is conceptually broken for iommufd since we want to allow
       * userspace to change the domains, eg switch from an identity IOAS to a

drivers/iommu/iommufd/device.c static int iommufd_group_setup_msi(struct iommufd_group *igroup, · line 363

Quoted 6 lines

drivers/iommu/iommufd/device.c +2 -0

@@ -358,6 +359,8 @@ static int iommufd_group_setup_msi(struct iommufd_group *igroup,
       * matches what the IRQ layer actually expects in a newly created
       * domain.
       */
+     if (hwpt->parent)
+             hwpt = hwpt->parent;

I guess there is a garantee the parent hwpt is necessarily a kernel-managed domain?

Is it that part of the spec that enforces it?

IOMMU_HWPT_ALLOC doc says:
" * A user-managed HWPT will be created from a given parent HWPT via @pt_id, in
* which the parent HWPT must be allocated previously via the same ioctl
from a
* given IOAS.
"
Maybe precise that in the commit msg?

This is used to forward the host IDR values to the user space, so the hypervisor and the guest VM can learn about the underlying hardware's capabilities.

Also, set the driver_type to IOMMU_HW_INFO_TYPE_ARM_SMMUV3 to pass the corresponding type sanity in the core.

Signed-off-by Nicolin Chen <[email protected]>

---
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 +++++++++++++++++++++
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  2 ++
  include/uapi/linux/iommufd.h                | 14 ++++++++++++
  3 files changed, 41 insertions(+)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +7 -0

index f2425b0f0cd6..c1aac695ae0d 100644
@@ -2005,6 +2005,29 @@ static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
  	}
  }
 
+static void *arm_smmu_hw_info(struct device *dev, u32 *length)
+{
+	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+	struct iommu_hw_info_smmuv3 *info;
+	void *base_idr;
+	int i;
+

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +8 -0

+
+	*length = sizeof(*info);
+
+	return info;
+}
+
  static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
  {
  	struct arm_smmu_domain *smmu_domain;

@@ -2845,6 +2868,7 @@ static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 
  static struct iommu_ops arm_smmu_ops = {
  	.capable		= arm_smmu_capable,
+	.hw_info		= arm_smmu_hw_info,
  	.domain_alloc		= arm_smmu_domain_alloc,
  	.probe_device		= arm_smmu_probe_device,
  	.release_device		= arm_smmu_release_device,

@@ -2857,6 +2881,7 @@ static struct iommu_ops arm_smmu_ops = {
  	.page_response		= arm_smmu_page_response,
  	.def_domain_type	= arm_smmu_def_domain_type,
  	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
+	.driver_type		= IOMMU_HW_INFO_TYPE_ARM_SMMUV3,
  	.owner			= THIS_MODULE,
  	.default_domain_ops = &(const struct iommu_domain_ops) {
  		.attach_dev		= arm_smmu_attach_dev,

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +2 -0

index 8d772ea8a583..ba2b4562f4b2 100644
@@ -14,6 +14,8 @@
  #include <linux/mmzone.h>
  #include <linux/sizes.h>
 
+#include <uapi/linux/iommufd.h>
+
  /* MMIO registers */
  #define ARM_SMMU_IDR0			0x0
  #define IDR0_ST_LVL			GENMASK(28, 27)

include/uapi/linux/iommufd.h +14 -0

index 0d5551b1b2be..c7a37915b49c 100644
@@ -519,6 +519,20 @@ struct iommu_hw_info_vtd {
  	__aligned_u64 ecap_reg;
  };
 
+/**
+ * struct iommu_hw_info_smmuv3 - ARM SMMUv3 device info
+ *
+ * @flags: Must be set to 0
+ * @__reserved: Must be 0
+ * @idr: Implemented features for the SMMU Non-secure programming interface.
+ *       Please refer to the chapters from 6.3.1 to 6.3.6 in the SMMUv3 Spec.
+ */
+struct iommu_hw_info_smmuv3 {
+	__u32 flags;
+	__u32 __reserved;
+	__u32 idr[6];
+};
+
  /**
   * struct iommu_hw_info - ioctl(IOMMU_DEVICE_GET_HW_INFO)
   * @size: sizeof(struct iommu_hw_info)

External email: Use caution opening links or attachments

Quoted 29 lines · Nicolin Chen

This is used to forward the host IDR values to the user space, so the hypervisor and the guest VM can learn about the underlying hardware's capabilities.

Also, set the driver_type to IOMMU_HW_INFO_TYPE_ARM_SMMUV3 to pass the corresponding type sanity in the core.

Signed-off-by Nicolin Chen <[email protected]>

---
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 +++++++++++++++++++++
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  2 ++
  include/uapi/linux/iommufd.h                | 14 ++++++++++++
  3 files changed, 41 insertions(+)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +7 -0

index f2425b0f0cd6..c1aac695ae0d 100644
@@ -2005,6 +2005,29 @@ static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
      }
  }
 
+static void *arm_smmu_hw_info(struct device *dev, u32 *length)
+{
+     struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+     struct iommu_hw_info_smmuv3 *info;
+     void *base_idr;
+     int i;
+

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap) · line 2024

Quoted 10 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +10 -0

+     if (!master || !master->smmu)
+             return ERR_PTR(-ENODEV);
+
+     info = kzalloc(sizeof(*info), GFP_KERNEL);
+     if (!info)
+             return ERR_PTR(-ENOMEM);
+
+     base_idr = master->smmu->base + ARM_SMMU_IDR0;
+     for (i = 0; i <= 5; i++)
+             info->idr[i] = readl_relaxed(base_idr + 0x4 * i);

You need to take firmware overrides etc. into account here. In particular, features like BTM may need to be hidden to work around errata either in the system integration or the SMMU itself. It isn't reasonable to expect every VMM to be aware of every erratum and workaround, and there may even be workarounds where we need to go out of our way to prevent guests from trying to use certain features in order to maintain correctness at S2.

In general this should probably follow the same principle as KVM, where we only expose sanitised feature registers representing the functionality the host understands. Code written today is almost guaranteed to be running on hardware released in 2030, at least *somewhere*.

Hi Robin,

Thanks for the inputs.

Quoted 44 lines · Robin Murphy

External email: Use caution opening links or attachments

Quoted 29 lines · Nicolin Chen

This is used to forward the host IDR values to the user space, so the hypervisor and the guest VM can learn about the underlying hardware's capabilities.

Also, set the driver_type to IOMMU_HW_INFO_TYPE_ARM_SMMUV3 to pass the corresponding type sanity in the core.

Signed-off-by Nicolin Chen <[email protected]>

---
   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 +++++++++++++++++++++
   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  2 ++
   include/uapi/linux/iommufd.h                | 14 ++++++++++++
   3 files changed, 41 insertions(+)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +7 -0

index f2425b0f0cd6..c1aac695ae0d 100644
@@ -2005,6 +2005,29 @@ static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
       }
   }
 
+static void *arm_smmu_hw_info(struct device *dev, u32 *length)
+{
+     struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+     struct iommu_hw_info_smmuv3 *info;
+     void *base_idr;
+     int i;
+

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap) · line 2024

Quoted 10 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +10 -0

+     if (!master || !master->smmu)
+             return ERR_PTR(-ENODEV);
+
+     info = kzalloc(sizeof(*info), GFP_KERNEL);
+     if (!info)
+             return ERR_PTR(-ENOMEM);
+
+     base_idr = master->smmu->base + ARM_SMMU_IDR0;
+     for (i = 0; i <= 5; i++)
+             info->idr[i] = readl_relaxed(base_idr + 0x4 * i);

Quoted 7 lines

You need to take firmware overrides etc. into account here. In particular, features like BTM may need to be hidden to work around errata either in the system integration or the SMMU itself. It isn't reasonable to expect every VMM to be aware of every erratum and workaround, and there may even be workarounds where we need to go out of our way to prevent guests from trying to use certain features in order to maintain correctness at S2.

We can add a bit of overrides after this for errata, perhaps?

I have some trouble with finding the errata docs. Would it be possible for you to direct me to it with a link maybe?

Quoted 4 lines

In general this should probably follow the same principle as KVM, where we only expose sanitised feature registers representing the functionality the host understands. Code written today is almost guaranteed to be running on hardware released in 2030, at least *somewhere*.

Yes.

Thanks
Nicolin

External email: Use caution opening links or attachments

Quoted 59 lines · Nicolin Chen

Hi Robin,

Thanks for the inputs.

Quoted 44 lines · Robin Murphy

External email: Use caution opening links or attachments

Quoted 29 lines · Nicolin Chen

This is used to forward the host IDR values to the user space, so the hypervisor and the guest VM can learn about the underlying hardware's capabilities.

Also, set the driver_type to IOMMU_HW_INFO_TYPE_ARM_SMMUV3 to pass the corresponding type sanity in the core.

Signed-off-by Nicolin Chen <[email protected]>

---
   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 +++++++++++++++++++++
   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  2 ++
   include/uapi/linux/iommufd.h                | 14 ++++++++++++
   3 files changed, 41 insertions(+)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +7 -0

index f2425b0f0cd6..c1aac695ae0d 100644
@@ -2005,6 +2005,29 @@ static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
       }
   }
 
+static void *arm_smmu_hw_info(struct device *dev, u32 *length)
+{
+     struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+     struct iommu_hw_info_smmuv3 *info;
+     void *base_idr;
+     int i;
+

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap) · line 2024

Quoted 10 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +10 -0

+     if (!master || !master->smmu)
+             return ERR_PTR(-ENODEV);
+
+     info = kzalloc(sizeof(*info), GFP_KERNEL);
+     if (!info)
+             return ERR_PTR(-ENOMEM);
+
+     base_idr = master->smmu->base + ARM_SMMU_IDR0;
+     for (i = 0; i <= 5; i++)
+             info->idr[i] = readl_relaxed(base_idr + 0x4 * i);

Quoted 7 lines

You need to take firmware overrides etc. into account here. In particular, features like BTM may need to be hidden to work around errata either in the system integration or the SMMU itself. It isn't reasonable to expect every VMM to be aware of every erratum and workaround, and there may even be workarounds where we need to go out of our way to prevent guests from trying to use certain features in order to maintain correctness at S2.

We can add a bit of overrides after this for errata, perhaps?

Quoted 2 lines

I have some trouble with finding the errata docs. Would it be possible for you to direct me to it with a link maybe?

The key Arm term is "Software Developer Errata Notice", or just SDEN. Here's the ones for MMU-600 and MMU-700:

https://developer.arm.com/documentation/SDEN-946810/latest/

https://developer.arm.com/documentation/SDEN-1786925/latest/

Note that until now it has been extremely fortunate that in pretty much every case Linux either hasn't supported the affected feature at all, or has happened to avoid meeting the conditions. Once we do introduce nesting support that all goes out the window (and I'll have to think more when reviewing new errata in future...)

I've been putting off revisiting all the existing errata to figure out what we'd need to do until new nesting patches appeared, so I'll try to get to that soon now. I think in many cases it's likely to be best to just disallowing nesting entirely on affected implementations.

Quoted 69 lines · Robin Murphy

External email: Use caution opening links or attachments

Quoted 57 lines · Nicolin Chen

Hi Robin,

Thanks for the inputs.

Quoted 44 lines · Robin Murphy

External email: Use caution opening links or attachments

Quoted 29 lines · Nicolin Chen

This is used to forward the host IDR values to the user space, so the hypervisor and the guest VM can learn about the underlying hardware's capabilities.

Also, set the driver_type to IOMMU_HW_INFO_TYPE_ARM_SMMUV3 to pass the corresponding type sanity in the core.

Signed-off-by Nicolin Chen <[email protected]>

---
    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 +++++++++++++++++++++
    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  2 ++
    include/uapi/linux/iommufd.h                | 14 ++++++++++++
    3 files changed, 41 insertions(+)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +7 -0

index f2425b0f0cd6..c1aac695ae0d 100644
@@ -2005,6 +2005,29 @@ static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
        }
    }
 
+static void *arm_smmu_hw_info(struct device *dev, u32 *length)
+{
+     struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+     struct iommu_hw_info_smmuv3 *info;
+     void *base_idr;
+     int i;
+

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap) · line 2024

Quoted 10 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +10 -0

+     if (!master || !master->smmu)
+             return ERR_PTR(-ENODEV);
+
+     info = kzalloc(sizeof(*info), GFP_KERNEL);
+     if (!info)
+             return ERR_PTR(-ENOMEM);
+
+     base_idr = master->smmu->base + ARM_SMMU_IDR0;
+     for (i = 0; i <= 5; i++)
+             info->idr[i] = readl_relaxed(base_idr + 0x4 * i);

Quoted 7 lines

You need to take firmware overrides etc. into account here. In particular, features like BTM may need to be hidden to work around errata either in the system integration or the SMMU itself. It isn't reasonable to expect every VMM to be aware of every erratum and workaround, and there may even be workarounds where we need to go out of our way to prevent guests from trying to use certain features in order to maintain correctness at S2.

Quoted 4 lines

We can add a bit of overrides after this for errata, perhaps?

I have some trouble with finding the errata docs. Would it be possible for you to direct me to it with a link maybe?

The key Arm term is "Software Developer Errata Notice", or just SDEN. Here's the ones for MMU-600 and MMU-700:

Quoted 1 line

https://developer.arm.com/documentation/SDEN-946810/latest/

This page shows "Arm CoreLink MMU-600 System Memory Management Unit Software Developer Errata Notice" but the downloaded file is "Arm CoreLink CI-700 Coherent Interconnect" errata notice. And I don't quite understand what it's about.

Quoted 1 line

https://developer.arm.com/documentation/SDEN-1786925/latest/

Yea, this one I got an "MMU-700 System Memory Management Unit" SMMU errata file that I can read and understand.

Quoted 10 lines

Note that until now it has been extremely fortunate that in pretty much every case Linux either hasn't supported the affected feature at all, or has happened to avoid meeting the conditions. Once we do introduce nesting support that all goes out the window (and I'll have to think more when reviewing new errata in future...)

I've been putting off revisiting all the existing errata to figure out what we'd need to do until new nesting patches appeared, so I'll try to get to that soon now. I think in many cases it's likely to be best to just disallowing nesting entirely on affected implementations.

Do we have already a list of "affected implementations"? Or, we would need to make such a list now? In a latter case, can these affected implementations be detected from their IRD0-5 registers, so that we can simply do something in hw_info()?

Quoted 11 lines

Quoted 10 lines

Note that until now it has been extremely fortunate that in pretty much every case Linux either hasn't supported the affected feature at all, or has happened to avoid meeting the conditions. Once we do introduce nesting support that all goes out the window (and I'll have to think more when reviewing new errata in future...)

I've been putting off revisiting all the existing errata to figure out what we'd need to do until new nesting patches appeared, so I'll try to get to that soon now. I think in many cases it's likely to be best to just disallowing nesting entirely on affected implementations.

Quoted 4 lines

Do we have already a list of "affected implementations"? Or, we would need to make such a list now? In a latter case, can these affected implementations be detected from their IRD0-5 registers, so that we can simply do something in hw_info()?

Somewhere I have a patch that adds all the IIDR stuff needed for this, but I never sent it upstream since the erratum itself was an early MMU-600 one which in practice doesn't matter. I'll dig that out and update it with what I have in mind.

Quoted 16 lines · Robin Murphy

Quoted 11 lines

Quoted 10 lines

Note that until now it has been extremely fortunate that in pretty much every case Linux either hasn't supported the affected feature at all, or has happened to avoid meeting the conditions. Once we do introduce nesting support that all goes out the window (and I'll have to think more when reviewing new errata in future...)

I've been putting off revisiting all the existing errata to figure out what we'd need to do until new nesting patches appeared, so I'll try to get to that soon now. I think in many cases it's likely to be best to just disallowing nesting entirely on affected implementations.

Quoted 4 lines

Do we have already a list of "affected implementations"? Or, we would need to make such a list now? In a latter case, can these affected implementations be detected from their IRD0-5 registers, so that we can simply do something in hw_info()?

Quoted 4 lines

Somewhere I have a patch that adds all the IIDR stuff needed for this, but I never sent it upstream since the erratum itself was an early MMU-600 one which in practice doesn't matter. I'll dig that out and update it with what I have in mind.

Nice!

Perhaps we should merge that first, or include in this series if you don't mind, so that we would be less worried about any affected platform when releasing the new Linux version having this nesting feature.

IOMMUFD designs two iommu_domain pointers to represent two stages. The S1

iommu_domain (IOMMU_DOMAIN_NESTED type) represents the Context Descriptor table in the user space. The S2 iommu_domain (IOMMU_DOMAIN_UNMANAGED type) represents the translation table in the kernel, owned by a hypervisor.

So there comes to no use case of the ARM_SMMU_DOMAIN_NESTED. Drop it, and use the type IOMMU_DOMAIN_NESTED instead.

Also drop the unused arm_smmu_enable_nesting(). One following patche will configure the correct smmu_domain->stage.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 ------------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 -

 2 files changed, 19 deletions(-)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -18

index c1aac695ae0d..c5616145e2a3 100644
@@ -1279,7 +1279,6 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
 			s1_cfg = &smmu_domain->s1_cfg;
 			break;
 		case ARM_SMMU_DOMAIN_S2:
-		case ARM_SMMU_DOMAIN_NESTED:
 			s2_cfg = &smmu_domain->s2_cfg;
 			break;
 		default:

@@ -2220,7 +2219,6 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain,
 		fmt = ARM_64_LPAE_S1;
 		finalise_stage_fn = arm_smmu_domain_finalise_s1;
 		break;
-	case ARM_SMMU_DOMAIN_NESTED:
 	case ARM_SMMU_DOMAIN_S2:
 		ias = smmu->ias;
 		oas = smmu->oas;

@@ -2747,21 +2745,6 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
 	return group;
 }
 
-static int arm_smmu_enable_nesting(struct iommu_domain *domain)
-{
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
-	int ret = 0;
-
-	mutex_lock(&smmu_domain->init_mutex);
-	if (smmu_domain->smmu)
-		ret = -EPERM;
-	else
-		smmu_domain->stage = ARM_SMMU_DOMAIN_NESTED;
-	mutex_unlock(&smmu_domain->init_mutex);
-
-	return ret;
-}
-
 static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
 {
 	return iommu_fwspec_add_ids(dev, args->args, 1);

@@ -2890,7 +2873,6 @@ static struct iommu_ops arm_smmu_ops = {
 		.flush_iotlb_all	= arm_smmu_flush_iotlb_all,
 		.iotlb_sync		= arm_smmu_iotlb_sync,
 		.iova_to_phys		= arm_smmu_iova_to_phys,
-		.enable_nesting		= arm_smmu_enable_nesting,
 		.free			= arm_smmu_domain_free,
 	}
 };

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +0 -1

index ba2b4562f4b2..233bfc377267 100644
@@ -704,7 +704,6 @@ struct arm_smmu_master {
 enum arm_smmu_domain_stage {
 	ARM_SMMU_DOMAIN_S1 = 0,
 	ARM_SMMU_DOMAIN_S2,
-	ARM_SMMU_DOMAIN_NESTED,
 	ARM_SMMU_DOMAIN_BYPASS,
 };
 

Quoted 7 lines

Also drop the unused arm_smmu_enable_nesting(). One following patche will configure the correct smmu_domain->stage.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 ------------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 -

If you go this way you may also remove it from arm/arm-smmu/arm-smmu.c. Then if I am not wrong no other driver does implement enable_nesting cb. Shouldn't we also remove it and fellow iommu_enable_nesting()?

Quoted 7 lines · Eric Auger

Quoted 7 lines

Also drop the unused arm_smmu_enable_nesting(). One following patche will configure the correct smmu_domain->stage.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 ------------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 -

Quoted 3 lines

If you go this way you may also remove it from arm/arm-smmu/arm-smmu.c. Then if I am not wrong no other driver does implement enable_nesting cb. Shouldn't we also remove it and fellow iommu_enable_nesting()?

Yes, lets just put this patch in the series please:

https://lore.kernel.org/kvm/[email protected]/

External email: Use caution opening links or attachments

Hi Nicolin,

Quoted 1 line · Nicolin Chen

IOMMUFD designs two iommu_domain pointers to represent two stages. The S1

s/designs/uses?

Quoted 6 lines

iommu_domain (IOMMU_DOMAIN_NESTED type) represents the Context Descriptor table in the user space. The S2 iommu_domain (IOMMU_DOMAIN_UNMANAGED type) represents the translation table in the kernel, owned by a hypervisor.

So there comes to no use case of the ARM_SMMU_DOMAIN_NESTED. Drop it, and use the type IOMMU_DOMAIN_NESTED instead.

last sentence may be rephrased as this patch does not use IOMMU_DOMAIN_NESTED anywhere:
Generic IOMMU_DOMAIN_NESTED type will be used in nested SMMU implementation instead.

Quoted 4 lines

Also drop the unused arm_smmu_enable_nesting(). One following patche will configure the correct smmu_domain->stage.

Quoted 4 lines

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 ------------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 -

If you go this way you may also remove it from arm/arm-smmu/arm-smmu.c. Then if I am not wrong no other driver does implement enable_nesting cb. Shouldn't we also remove it and fellow iommu_enable_nesting()?

From: Eric Auger <[email protected]>

Despite the spec does not seem to mention this, on some implementations, when the STE configuration switches from an S1+S2 cfg to an S1 only one, a C_BAD_STE error would happen if dst[3] (S2TTB) is not reset.

Explicitly reset those two higher 64b fields, to prevent that.

Note that this is not a bug at this moment, since a 2-stage translation setup is not yet enabled, until the following patches add its support.

Reported-by Shameer Kolothum <[email protected]>

Signed-off-by Eric Auger <[email protected]>

Signed-off-by Nicolin Chen <[email protected]>

---
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 +++
  1 file changed, 3 insertions(+)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3 -0

index c5616145e2a3..29e36448d23b 100644
@@ -1361,6 +1361,9 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
  		dst[3] = cpu_to_le64(s2_cfg->vttbr & STRTAB_STE_3_S2TTB_MASK);
 
  		val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_S2_TRANS);
+	}  else {
+		dst[2] = 0;
+		dst[3] = 0;
  	}
 
  	if (master->ats_enabled)

-----Original Message-----
From: Robin Murphy [mailto:[email protected]]
Sent: 09 March 2023 13:13
To: Nicolin Chen <[email protected]>; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Shameerali Kolothum Thodi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 06/14] iommu/arm-smmu-v3: Unset corresponding STE fields when s2_cfg is NULL

Quoted 5 lines · Nicolin Chen

From: Eric Auger <[email protected]>

Despite the spec does not seem to mention this, on some implementations, when the STE configuration switches from an S1+S2 cfg to an S1 only one, a C_BAD_STE error would happen if dst[3] (S2TTB) is not reset.

Can you provide more details, since it's not clear whether this is a hardware erratum workaround or a bodge around the driver itself doing something wrong like not doing a proper break-before-make transition of the STE. The architecture explicitly states that all the STE.S2* fields except S2VMID and potentially S2S are ignored when Stage 2 is bypassed.

External email: Use caution opening links or attachments

Quoted 19 lines

-----Original Message-----
From: Robin Murphy [mailto:[email protected]]
Sent: 09 March 2023 13:13
To: Nicolin Chen <[email protected]>; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Shameerali Kolothum Thodi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v1 06/14] iommu/arm-smmu-v3: Unset corresponding STE fields when s2_cfg is NULL

Quoted 5 lines · Nicolin Chen

From: Eric Auger <[email protected]>

Despite the spec does not seem to mention this, on some implementations, when the STE configuration switches from an S1+S2 cfg to an S1 only one, a C_BAD_STE error would happen if dst[3] (S2TTB) is not reset.

Quoted 5 lines

Can you provide more details, since it's not clear whether this is a hardware erratum workaround or a bodge around the driver itself doing something wrong like not doing a proper break-before-make transition of the STE. The architecture explicitly states that all the STE.S2* fields except S2VMID and potentially S2S are ignored when Stage 2 is bypassed.

Took a while to locate the email thread where this was discussed,
https://patchwork.kernel.org/cover/11449895/#23244457

This was observed on a HiSilicon implementation where, if the SMMUv3 is configured with both Stage 1 and Stage 2 (nested) mode once, then it is not possible to configure it back for Stage 1 mode for the same device(stream id).

IIRC, the SMMUv3 implementation on these boards expects to set the S2TTB field in STE to zero when using S1, otherwise it reports C_BAD_STE error. :(

You are right that the specification doesn't demand this and I am not sure there are any other Hardware that requires this.

Could we please have this with a comment added in the code?

In a nested translation setup, the device is attached to a stage-1 domain that represents the guest-level Context Descriptor table. A Stream Table Entry for a 2-stage translation needs both the stage-1 Context Descriptor table info and the stage-2 Translation table information, i.e. a pair of s1_cfg and s2_cfg.

Add an "s2" pointer in struct arm_smmu_domain, so a nested stage-1 domain can simply navigate its stage-2 domain for the s2_cfg pointer. Also, add a to_s2_cfg() helper for this purpose, and use it at proper places.

Signed-off-by Nicolin Chen <[email protected]>

---
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 +++++++++++++++++++--
  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 +
  2 files changed, 24 insertions(+), 2 deletions(-)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +18 -0

index 21d819979865..fee5977feef3 100644
@@ -100,6 +100,24 @@ static void parse_driver_options(struct arm_smmu_device *smmu)
  	} while (arm_smmu_options[++i].opt);
  }
 
+static struct arm_smmu_s2_cfg *to_s2_cfg(struct arm_smmu_domain *smmu_domain)
+{
+	if (!smmu_domain)
+		return NULL;
+
+	switch (smmu_domain->stage) {
+	case ARM_SMMU_DOMAIN_S1:
+		if (smmu_domain->s2)
+			return &smmu_domain->s2->s2_cfg;
+		return NULL;
+	case ARM_SMMU_DOMAIN_S2:
+		return &smmu_domain->s2_cfg;
+	case ARM_SMMU_DOMAIN_BYPASS:
+	default:
+		return NULL;
+	}
+}
+
  /* Low-level queue manipulation functions */
  static bool queue_has_space(struct arm_smmu_ll_queue *q, u32 n)
  {

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4 -2

  			break;
  		case ARM_SMMU_DOMAIN_S2:
  			s2_cfg = &smmu_domain->s2_cfg;

@@ -1846,6 +1865,7 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
  static void arm_smmu_tlb_inv_context(void *cookie)
  {
  	struct arm_smmu_domain *smmu_domain = cookie;
+	struct arm_smmu_s2_cfg *s2_cfg = to_s2_cfg(smmu_domain);
  	struct arm_smmu_device *smmu = smmu_domain->smmu;
  	struct arm_smmu_cmdq_ent cmd;
 

@@ -1860,7 +1880,7 @@ static void arm_smmu_tlb_inv_context(void *cookie)
  		arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg.cd.asid);
  	} else {
  		cmd.opcode	= CMDQ_OP_TLBI_S12_VMALL;
-		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
+		cmd.tlbi.vmid	= s2_cfg->vmid;
  		arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
  	}
  	arm_smmu_atc_inv_domain(smmu_domain, 0, 0, 0);

@@ -1931,6 +1951,7 @@ static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
  					  size_t granule, bool leaf,
  					  struct arm_smmu_domain *smmu_domain)
  {
+	struct arm_smmu_s2_cfg *s2_cfg = to_s2_cfg(smmu_domain);
  	struct arm_smmu_cmdq_ent cmd = {
  		.tlbi = {
  			.leaf	= leaf,

@@ -1943,7 +1964,7 @@ static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
  		cmd.tlbi.asid	= smmu_domain->s1_cfg.cd.asid;
  	} else {
  		cmd.opcode	= CMDQ_OP_TLBI_S2_IPA;
-		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
+		cmd.tlbi.vmid	= s2_cfg->vmid;
  	}
  	__arm_smmu_tlb_inv_range(&cmd, iova, size, granule, smmu_domain);
 

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -0

index 1a93eeb993ea..6cf516852721 100644
@@ -709,6 +709,7 @@ enum arm_smmu_domain_stage {
  };
 
  struct arm_smmu_domain {
+	struct arm_smmu_domain		*s2;
  	struct arm_smmu_device		*smmu;
  	struct mutex			init_mutex; /* Protects smmu pointer */
 

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid, · line 1298

Quoted 5 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1 -0

@@ -1277,6 +1295,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
              switch (smmu_domain->stage) {
              case ARM_SMMU_DOMAIN_S1:
                      s1_cfg = &smmu_domain->s1_cfg;
+                     s2_cfg = to_s2_cfg(smmu_domain);

TBH I'd say you only need a 2-line change here. All the other cases below are when the stage is guaranteed to be ARM_SMMU_DOMAIN_S2 (once ARM_SMMU_DOMAIN_NESTED is gone), so pretending it might be otherwise seems unnecessarily confusing.