Re: [PATCH v2 2/7] iommu/dma-iommu: Split iommu_dma_map_msi_msg() in two parts

2019-04-30 Thread Auger Eric 
Hu Julien,

On 4/29/19 4:44 PM, Julien Grall wrote:
> On RT, iommu_dma_map_msi_msg() may be called from non-preemptible
> context. This will lead to a splat with CONFIG_DEBUG_ATOMIC_SLEEP as
> the function is using spin_lock (they can sleep on RT).
> 
> iommu_dma_map_msi_msg() is used to map the MSI page in the IOMMU PT
> and update the MSI message with the IOVA.
> 
> Only the part to lookup for the MSI page requires to be called in
> preemptible context. As the MSI page cannot change over the lifecycle
> of the MSI interrupt, the lookup can be cached and re-used later on.
> 
> iomma_dma_map_msi_msg() is now split in two functions:
> - iommu_dma_prepare_msi(): This function will prepare the mapping
> in the IOMMU and store the cookie in the structure msi_desc. This
> function should be called in preemptible context.
> - iommu_dma_compose_msi_msg(): This function will update the MSI
> message with the IOVA when the device is behind an IOMMU.
> 
> Signed-off-by: Julien Grall 
Besides other's comments,

Reviewed-by: Eric Auger 

Thanks

Eric
> 
> ---
> Changes in v2:
> - Rework the commit message to use imperative mood
> - Use the MSI accessor to get/set the iommu cookie
> - Don't use ternary on return
> - Select CONFIG_IRQ_MSI_IOMMU
> - Pass an msi_desc rather than the irq number
> ---
>  drivers/iommu/Kconfig |  1 +
>  drivers/iommu/dma-iommu.c | 47 
> ++-
>  include/linux/dma-iommu.h | 23 +++
>  3 files changed, 62 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 6f07f3b21816..eb1c8cd243f9 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -94,6 +94,7 @@ config IOMMU_DMA
>   bool
>   select IOMMU_API
>   select IOMMU_IOVA
> + select IRQ_MSI_IOMMU
>   select NEED_SG_DMA_LENGTH
>  
>  config FSL_PAMU
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 77aabe637a60..2309f59cefa4 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -888,17 +888,18 @@ static struct iommu_dma_msi_page 
> *iommu_dma_get_msi_page(struct device *dev,
>   return NULL;
>  }
>  
> -void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
> +int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
>  {
> - struct device *dev = msi_desc_to_dev(irq_get_msi_desc(irq));
> + struct device *dev = msi_desc_to_dev(desc);
>   struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>   struct iommu_dma_cookie *cookie;
>   struct iommu_dma_msi_page *msi_page;
> - phys_addr_t msi_addr = (u64)msg->address_hi << 32 | msg->address_lo;
>   unsigned long flags;
>  
> - if (!domain || !domain->iova_cookie)
> - return;
> + if (!domain || !domain->iova_cookie) {
> + desc->iommu_cookie = NULL;
> + return 0;
> + }
>  
>   cookie = domain->iova_cookie;
>  
> @@ -911,7 +912,37 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>   msi_page = iommu_dma_get_msi_page(dev, msi_addr, domain);
>   spin_unlock_irqrestore(>msi_lock, flags);
>  
> - if (WARN_ON(!msi_page)) {
> + msi_desc_set_iommu_cookie(desc, msi_page);
> +
> + if (!msi_page)
> + return -ENOMEM;
> + else
> + return 0;
> +}
> +
> +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_dma_msi_page *msi_page;
> +
> + msi_page = msi_desc_get_iommu_cookie(desc);
> +
> + if (!domain || !domain->iova_cookie || WARN_ON(!msi_page))
> + return;
> +
> + msg->address_hi = upper_32_bits(msi_page->iova);
> + msg->address_lo &= cookie_msi_granule(domain->iova_cookie) - 1;
> + msg->address_lo += lower_32_bits(msi_page->iova);
> +}
> +
> +void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
> +{
> + struct msi_desc *desc = irq_get_msi_desc(irq);
> + phys_addr_t msi_addr = (u64)msg->address_hi << 32 | msg->address_lo;
> +
> + if (WARN_ON(iommu_dma_prepare_msi(desc, msi_addr))) {
>   /*
>* We're called from a void callback, so the best we can do is
>* 'fail' by filling the message with obviously bogus values.
> @@ -922,8 +953,6 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>   msg->address_lo = ~0U;
>   msg->data = ~0U;
>   } else {
> - msg->address_hi = upper_32_bits(msi_page->iova);
> - msg->address_lo &= cookie_msi_granule(cookie) - 1;
> - msg->address_lo += lower_32_bits(msi_page->iova);
> + iommu_dma_compose_msi_msg(desc, msg);
>   }
>  }
> diff --git a/include/linux/dma-iommu.h

Re: [PATCH v2 2/7] iommu/dma-iommu: Split iommu_dma_map_msi_msg() in two parts

2019-04-29 Thread Marc Zyngier 
On 29/04/2019 15:44, Julien Grall wrote:
> On RT, iommu_dma_map_msi_msg() may be called from non-preemptible
> context. This will lead to a splat with CONFIG_DEBUG_ATOMIC_SLEEP as
> the function is using spin_lock (they can sleep on RT).
> 
> iommu_dma_map_msi_msg() is used to map the MSI page in the IOMMU PT
> and update the MSI message with the IOVA.
> 
> Only the part to lookup for the MSI page requires to be called in
> preemptible context. As the MSI page cannot change over the lifecycle
> of the MSI interrupt, the lookup can be cached and re-used later on.
> 
> iomma_dma_map_msi_msg() is now split in two functions:
> - iommu_dma_prepare_msi(): This function will prepare the mapping
> in the IOMMU and store the cookie in the structure msi_desc. This
> function should be called in preemptible context.
> - iommu_dma_compose_msi_msg(): This function will update the MSI
> message with the IOVA when the device is behind an IOMMU.
> 
> Signed-off-by: Julien Grall 
> 
> ---
> Changes in v2:
> - Rework the commit message to use imperative mood
> - Use the MSI accessor to get/set the iommu cookie
> - Don't use ternary on return
> - Select CONFIG_IRQ_MSI_IOMMU
> - Pass an msi_desc rather than the irq number
> ---
>  drivers/iommu/Kconfig |  1 +
>  drivers/iommu/dma-iommu.c | 47 
> ++-
>  include/linux/dma-iommu.h | 23 +++
>  3 files changed, 62 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 6f07f3b21816..eb1c8cd243f9 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -94,6 +94,7 @@ config IOMMU_DMA
>   bool
>   select IOMMU_API
>   select IOMMU_IOVA
> + select IRQ_MSI_IOMMU
>   select NEED_SG_DMA_LENGTH
>  
>  config FSL_PAMU
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 77aabe637a60..2309f59cefa4 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -888,17 +888,18 @@ static struct iommu_dma_msi_page 
> *iommu_dma_get_msi_page(struct device *dev,
>   return NULL;
>  }
>  
> -void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
> +int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
>  {
> - struct device *dev = msi_desc_to_dev(irq_get_msi_desc(irq));
> + struct device *dev = msi_desc_to_dev(desc);
>   struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>   struct iommu_dma_cookie *cookie;
>   struct iommu_dma_msi_page *msi_page;
> - phys_addr_t msi_addr = (u64)msg->address_hi << 32 | msg->address_lo;
>   unsigned long flags;
>  
> - if (!domain || !domain->iova_cookie)
> - return;
> + if (!domain || !domain->iova_cookie) {
> + desc->iommu_cookie = NULL;

nit: This could be

msi_desc_set_iommu_cookie(desc, NULL);

now that you have introduced the relevant accessors.

> + return 0;
> + }
>  
>   cookie = domain->iova_cookie;
>  
> @@ -911,7 +912,37 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>   msi_page = iommu_dma_get_msi_page(dev, msi_addr, domain);
>   spin_unlock_irqrestore(>msi_lock, flags);
>  
> - if (WARN_ON(!msi_page)) {
> + msi_desc_set_iommu_cookie(desc, msi_page);
> +
> + if (!msi_page)
> + return -ENOMEM;
> + else
> + return 0;
> +}
> +
> +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_dma_msi_page *msi_page;
> +
> + msi_page = msi_desc_get_iommu_cookie(desc);
> +
> + if (!domain || !domain->iova_cookie || WARN_ON(!msi_page))
> + return;
> +
> + msg->address_hi = upper_32_bits(msi_page->iova);
> + msg->address_lo &= cookie_msi_granule(domain->iova_cookie) - 1;
> + msg->address_lo += lower_32_bits(msi_page->iova);
> +}
> +
> +void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
> +{
> + struct msi_desc *desc = irq_get_msi_desc(irq);
> + phys_addr_t msi_addr = (u64)msg->address_hi << 32 | msg->address_lo;
> +
> + if (WARN_ON(iommu_dma_prepare_msi(desc, msi_addr))) {
>   /*
>* We're called from a void callback, so the best we can do is
>* 'fail' by filling the message with obviously bogus values.
> @@ -922,8 +953,6 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>   msg->address_lo = ~0U;
>   msg->data = ~0U;
>   } else {
> - msg->address_hi = upper_32_bits(msi_page->iova);
> - msg->address_lo &= cookie_msi_granule(cookie) - 1;
> - msg->address_lo += lower_32_bits(msi_page->iova);
> + iommu_dma_compose_msi_msg(desc, msg);
>   }
>  }
>

Re: [PATCH v2 2/7] iommu/dma-iommu: Split iommu_dma_map_msi_msg() in two parts

2019-04-29 Thread Robin Murphy 

On 29/04/2019 15:44, Julien Grall wrote:

On RT, iommu_dma_map_msi_msg() may be called from non-preemptible
context. This will lead to a splat with CONFIG_DEBUG_ATOMIC_SLEEP as
the function is using spin_lock (they can sleep on RT).

iommu_dma_map_msi_msg() is used to map the MSI page in the IOMMU PT
and update the MSI message with the IOVA.

Only the part to lookup for the MSI page requires to be called in
preemptible context. As the MSI page cannot change over the lifecycle
of the MSI interrupt, the lookup can be cached and re-used later on.

iomma_dma_map_msi_msg() is now split in two functions:
 - iommu_dma_prepare_msi(): This function will prepare the mapping
 in the IOMMU and store the cookie in the structure msi_desc. This
 function should be called in preemptible context.
 - iommu_dma_compose_msi_msg(): This function will update the MSI
 message with the IOVA when the device is behind an IOMMU.

Signed-off-by: Julien Grall 

---
 Changes in v2:
 - Rework the commit message to use imperative mood
 - Use the MSI accessor to get/set the iommu cookie
 - Don't use ternary on return
 - Select CONFIG_IRQ_MSI_IOMMU
 - Pass an msi_desc rather than the irq number
---
  drivers/iommu/Kconfig |  1 +
  drivers/iommu/dma-iommu.c | 47 ++-
  include/linux/dma-iommu.h | 23 +++
  3 files changed, 62 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 6f07f3b21816..eb1c8cd243f9 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -94,6 +94,7 @@ config IOMMU_DMA
bool
select IOMMU_API
select IOMMU_IOVA
+   select IRQ_MSI_IOMMU
select NEED_SG_DMA_LENGTH
  
  config FSL_PAMU

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 77aabe637a60..2309f59cefa4 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -888,17 +888,18 @@ static struct iommu_dma_msi_page 
*iommu_dma_get_msi_page(struct device *dev,
return NULL;
  }
  
-void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)

+int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
  {
-   struct device *dev = msi_desc_to_dev(irq_get_msi_desc(irq));
+   struct device *dev = msi_desc_to_dev(desc);
struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
struct iommu_dma_cookie *cookie;
struct iommu_dma_msi_page *msi_page;
-   phys_addr_t msi_addr = (u64)msg->address_hi << 32 | msg->address_lo;
unsigned long flags;
  
-	if (!domain || !domain->iova_cookie)

-   return;
+   if (!domain || !domain->iova_cookie) {
+   desc->iommu_cookie = NULL;
+   return 0;
+   }
  
  	cookie = domain->iova_cookie;
  
@@ -911,7 +912,37 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)

msi_page = iommu_dma_get_msi_page(dev, msi_addr, domain);
spin_unlock_irqrestore(>msi_lock, flags);
  
-	if (WARN_ON(!msi_page)) {

+   msi_desc_set_iommu_cookie(desc, msi_page);
+
+   if (!msi_page)
+   return -ENOMEM;
+   else


Nit: the "else" isn't really necessary.


+   return 0;
+}
+
+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_dma_msi_page *msi_page;
+
+   msi_page = msi_desc_get_iommu_cookie(desc);
+
+   if (!domain || !domain->iova_cookie || WARN_ON(!msi_page))
+   return;
+
+   msg->address_hi = upper_32_bits(msi_page->iova);
+   msg->address_lo &= cookie_msi_granule(domain->iova_cookie) - 1;
+   msg->address_lo += lower_32_bits(msi_page->iova);
+}
+
+void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
+{
+   struct msi_desc *desc = irq_get_msi_desc(irq);
+   phys_addr_t msi_addr = (u64)msg->address_hi << 32 | msg->address_lo;
+
+   if (WARN_ON(iommu_dma_prepare_msi(desc, msi_addr))) {
/*
 * We're called from a void callback, so the best we can do is
 * 'fail' by filling the message with obviously bogus values.
@@ -922,8 +953,6 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
msg->address_lo = ~0U;
msg->data = ~0U;
} else {
-   msg->address_hi = upper_32_bits(msi_page->iova);
-   msg->address_lo &= cookie_msi_granule(cookie) - 1;
-   msg->address_lo += lower_32_bits(msi_page->iova);
+   iommu_dma_compose_msi_msg(desc, msg);
}
  }
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index e760dc5d1fa8..3fc48fbd6f63 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -71,12 +71,24 @@ void

[PATCH v2 2/7] iommu/dma-iommu: Split iommu_dma_map_msi_msg() in two parts

2019-04-29 Thread Julien Grall 
On RT, iommu_dma_map_msi_msg() may be called from non-preemptible
context. This will lead to a splat with CONFIG_DEBUG_ATOMIC_SLEEP as
the function is using spin_lock (they can sleep on RT).

iommu_dma_map_msi_msg() is used to map the MSI page in the IOMMU PT
and update the MSI message with the IOVA.

Only the part to lookup for the MSI page requires to be called in
preemptible context. As the MSI page cannot change over the lifecycle
of the MSI interrupt, the lookup can be cached and re-used later on.

iomma_dma_map_msi_msg() is now split in two functions:
- iommu_dma_prepare_msi(): This function will prepare the mapping
in the IOMMU and store the cookie in the structure msi_desc. This
function should be called in preemptible context.
- iommu_dma_compose_msi_msg(): This function will update the MSI
message with the IOVA when the device is behind an IOMMU.

Signed-off-by: Julien Grall 

---
Changes in v2:
- Rework the commit message to use imperative mood
- Use the MSI accessor to get/set the iommu cookie
- Don't use ternary on return
- Select CONFIG_IRQ_MSI_IOMMU
- Pass an msi_desc rather than the irq number
---
 drivers/iommu/Kconfig |  1 +
 drivers/iommu/dma-iommu.c | 47 ++-
 include/linux/dma-iommu.h | 23 +++
 3 files changed, 62 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 6f07f3b21816..eb1c8cd243f9 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -94,6 +94,7 @@ config IOMMU_DMA
bool
select IOMMU_API
select IOMMU_IOVA
+   select IRQ_MSI_IOMMU
select NEED_SG_DMA_LENGTH
 
 config FSL_PAMU
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 77aabe637a60..2309f59cefa4 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -888,17 +888,18 @@ static struct iommu_dma_msi_page 
*iommu_dma_get_msi_page(struct device *dev,
return NULL;
 }
 
-void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
+int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
 {
-   struct device *dev = msi_desc_to_dev(irq_get_msi_desc(irq));
+   struct device *dev = msi_desc_to_dev(desc);
struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
struct iommu_dma_cookie *cookie;
struct iommu_dma_msi_page *msi_page;
-   phys_addr_t msi_addr = (u64)msg->address_hi << 32 | msg->address_lo;
unsigned long flags;
 
-   if (!domain || !domain->iova_cookie)
-   return;
+   if (!domain || !domain->iova_cookie) {
+   desc->iommu_cookie = NULL;
+   return 0;
+   }
 
cookie = domain->iova_cookie;
 
@@ -911,7 +912,37 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
msi_page = iommu_dma_get_msi_page(dev, msi_addr, domain);
spin_unlock_irqrestore(>msi_lock, flags);
 
-   if (WARN_ON(!msi_page)) {
+   msi_desc_set_iommu_cookie(desc, msi_page);
+
+   if (!msi_page)
+   return -ENOMEM;
+   else
+   return 0;
+}
+
+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_dma_msi_page *msi_page;
+
+   msi_page = msi_desc_get_iommu_cookie(desc);
+
+   if (!domain || !domain->iova_cookie || WARN_ON(!msi_page))
+   return;
+
+   msg->address_hi = upper_32_bits(msi_page->iova);
+   msg->address_lo &= cookie_msi_granule(domain->iova_cookie) - 1;
+   msg->address_lo += lower_32_bits(msi_page->iova);
+}
+
+void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
+{
+   struct msi_desc *desc = irq_get_msi_desc(irq);
+   phys_addr_t msi_addr = (u64)msg->address_hi << 32 | msg->address_lo;
+
+   if (WARN_ON(iommu_dma_prepare_msi(desc, msi_addr))) {
/*
 * We're called from a void callback, so the best we can do is
 * 'fail' by filling the message with obviously bogus values.
@@ -922,8 +953,6 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
msg->address_lo = ~0U;
msg->data = ~0U;
} else {
-   msg->address_hi = upper_32_bits(msi_page->iova);
-   msg->address_lo &= cookie_msi_granule(cookie) - 1;
-   msg->address_lo += lower_32_bits(msi_page->iova);
+   iommu_dma_compose_msi_msg(desc, msg);
}
 }
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index e760dc5d1fa8..3fc48fbd6f63 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -71,12 +71,24 @@ void iommu_dma_unmap_resource(struct device *dev, 
dma_addr_t handle,
size_t size, enum dma_data_direction dir,