* Re: [PATCH] iommu/dma: Setup iova_domain granule for IOMMU_DMA_MSI cookies
@ 2017-04-13 11:21 ` Robin Murphy
0 siblings, 0 replies; 16+ messages in thread
From: Robin Murphy @ 2017-04-13 11:21 UTC (permalink / raw)
To: Nate Watterson, Joerg Roedel,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: shankerd-sgV2jX0FEOL9JmXXK+q4OQ
Hi Nate,
On 13/04/17 09:55, Nate Watterson wrote:
> Currently, the __iommu_dma_{map/free} functions call iova_{offset/align}
> making them unsuitable for use with iommu_domains having an IOMMU_DMA_MSI
> cookie since the cookie's iova_domain member, iovad, is uninitialized.
>
> Now that iommu_dma_get_msi_page() calls __iommu_dma_map() regardless
> of cookie type, failures are being seen when mapping MSI target
> addresses for devices attached to UNMANAGED domains. To work around
> this issue, the iova_domain granule for IOMMU_DMA_MSI cookies is
> initialized to the value returned by cookie_msi_granule().
Oh bum. Thanks for the report.
However, I really don't like bodging around it with deliberate undefined
behaviour. Fixing things properly doesn't seem too hard:
----->8-----
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 8348f366ddd1..62618e77bedc 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -396,13 +396,13 @@ static void iommu_dma_free_iova(struct
iommu_dma_cookie *cookie,
dma_addr_t iova, size_t size)
{
struct iova_domain *iovad = &cookie->iovad;
- unsigned long shift = iova_shift(iovad);
/* The MSI case is only ever cleaning up its most recent
allocation */
if (cookie->type == IOMMU_DMA_MSI_COOKIE)
cookie->msi_iova -= size;
else
- free_iova_fast(iovad, iova >> shift, size >> shift);
+ free_iova_fast(iovad, iova_pfn(iovad, iova),
+ size >> iova_shift(iovad));
}
static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t
dma_addr,
@@ -617,11 +617,14 @@ static dma_addr_t __iommu_dma_map(struct device
*dev, phys_addr_t phys,
{
struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
struct iommu_dma_cookie *cookie = domain->iova_cookie;
- struct iova_domain *iovad = &cookie->iovad;
- size_t iova_off = iova_offset(iovad, phys);
+ size_t iova_off = 0;
dma_addr_t iova;
- size = iova_align(iovad, size + iova_off);
+ if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
+ iova_off = iova_offset(&cookie->iovad, phys);
+ size = iova_align(&cookie->iovad, size + iova_off);
+ }
+
iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
if (!iova)
return DMA_ERROR_CODE;
-----8<-----
Untested, and you'll probably want to double-check it anyway given that
the original oversight was mine in the first place ;)
Robin.
> Fixes: a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")
> Signed-off-by: Nate Watterson <nwatters-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
> drivers/iommu/dma-iommu.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 8348f366..d7b0816 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -127,6 +127,16 @@ int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base)
>
> cookie->msi_iova = base;
> domain->iova_cookie = cookie;
> +
> + /*
> + * Setup granule for compatibility with __iommu_dma_{alloc/free} and
> + * add a compile time check to ensure that writing granule won't
> + * clobber msi_iova.
> + */
> + cookie->iovad.granule = cookie_msi_granule(cookie);
> + BUILD_BUG_ON(offsetof(struct iova_domain, granule) <
> + sizeof(cookie->msi_iova));
> +
> return 0;
> }
> EXPORT_SYMBOL(iommu_get_msi_cookie);
>
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH] iommu/dma: Setup iova_domain granule for IOMMU_DMA_MSI cookies
@ 2017-04-13 14:29 ` Shanker Donthineni
0 siblings, 0 replies; 16+ messages in thread
From: Shanker Donthineni @ 2017-04-13 14:29 UTC (permalink / raw)
To: Robin Murphy, Nate Watterson, Joerg Roedel, iommu, linux-kernel
Hi Robin,
I tested your changes and the device pass-through feature works fine on QDF2400 server platform. Maybe Nate comments on the patch contents but it fixes the problem.
@@ -317,13 +317,13 @@ static void iommu_dma_free_iova(struct iommu_dma_cookie *cookie,
dma_addr_t iova, size_t size)
{
struct iova_domain *iovad = &cookie->iovad;
- unsigned long shift = iova_shift(iovad);
/* The MSI case is only ever cleaning up its most recent allocation */
if (cookie->type == IOMMU_DMA_MSI_COOKIE)
cookie->msi_iova -= size;
else
- free_iova_fast(iovad, iova >> shift, size >> shift);
+ free_iova_fast(iovad, iova_pfn(iovad, iova),
+ size >> iova_shift(iovad));
}
static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t dma_addr,
@@ -538,11 +538,14 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
{
struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
struct iommu_dma_cookie *cookie = domain->iova_cookie;
- struct iova_domain *iovad = &cookie->iovad;
- size_t iova_off = iova_offset(iovad, phys);
+ size_t iova_off = 0;
dma_addr_t iova;
- size = iova_align(iovad, size + iova_off);
+ if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
+ iova_off = iova_offset(&cookie->iovad, phys);
+ size = iova_align(&cookie->iovad, size + iova_off);
+ }
On 04/13/2017 06:21 AM, Robin Murphy wrote:
> Hi Nate,
>
> On 13/04/17 09:55, Nate Watterson wrote:
>> Currently, the __iommu_dma_{map/free} functions call iova_{offset/align}
>> making them unsuitable for use with iommu_domains having an IOMMU_DMA_MSI
>> cookie since the cookie's iova_domain member, iovad, is uninitialized.
>>
>> Now that iommu_dma_get_msi_page() calls __iommu_dma_map() regardless
>> of cookie type, failures are being seen when mapping MSI target
>> addresses for devices attached to UNMANAGED domains. To work around
>> this issue, the iova_domain granule for IOMMU_DMA_MSI cookies is
>> initialized to the value returned by cookie_msi_granule().
> Oh bum. Thanks for the report.
>
> However, I really don't like bodging around it with deliberate undefined
> behaviour. Fixing things properly doesn't seem too hard:
>
> ----->8-----
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 8348f366ddd1..62618e77bedc 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -396,13 +396,13 @@ static void iommu_dma_free_iova(struct
> iommu_dma_cookie *cookie,
> dma_addr_t iova, size_t size)
> {
> struct iova_domain *iovad = &cookie->iovad;
> - unsigned long shift = iova_shift(iovad);
>
> /* The MSI case is only ever cleaning up its most recent
> allocation */
> if (cookie->type == IOMMU_DMA_MSI_COOKIE)
> cookie->msi_iova -= size;
> else
> - free_iova_fast(iovad, iova >> shift, size >> shift);
> + free_iova_fast(iovad, iova_pfn(iovad, iova),
> + size >> iova_shift(iovad));
> }
>
> static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t
> dma_addr,
> @@ -617,11 +617,14 @@ static dma_addr_t __iommu_dma_map(struct device
> *dev, phys_addr_t phys,
> {
> struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> struct iommu_dma_cookie *cookie = domain->iova_cookie;
> - struct iova_domain *iovad = &cookie->iovad;
> - size_t iova_off = iova_offset(iovad, phys);
> + size_t iova_off = 0;
> dma_addr_t iova;
>
> - size = iova_align(iovad, size + iova_off);
> + if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
> + iova_off = iova_offset(&cookie->iovad, phys);
> + size = iova_align(&cookie->iovad, size + iova_off);
> + }
> +
> iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
> if (!iova)
> return DMA_ERROR_CODE;
> -----8<-----
>
> Untested, and you'll probably want to double-check it anyway given that
> the original oversight was mine in the first place ;)
>
> Robin.
>
>> Fixes: a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")
>> Signed-off-by: Nate Watterson <nwatters@codeaurora.org>
>> ---
>> drivers/iommu/dma-iommu.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 8348f366..d7b0816 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -127,6 +127,16 @@ int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base)
>>
>> cookie->msi_iova = base;
>> domain->iova_cookie = cookie;
>> +
>> + /*
>> + * Setup granule for compatibility with __iommu_dma_{alloc/free} and
>> + * add a compile time check to ensure that writing granule won't
>> + * clobber msi_iova.
>> + */
>> + cookie->iovad.granule = cookie_msi_granule(cookie);
>> + BUILD_BUG_ON(offsetof(struct iova_domain, granule) <
>> + sizeof(cookie->msi_iova));
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL(iommu_get_msi_cookie);
>>
--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] iommu/dma: Setup iova_domain granule for IOMMU_DMA_MSI cookies
@ 2017-04-13 14:29 ` Shanker Donthineni
0 siblings, 0 replies; 16+ messages in thread
From: Shanker Donthineni @ 2017-04-13 14:29 UTC (permalink / raw)
To: Robin Murphy, Nate Watterson, Joerg Roedel,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Hi Robin,
I tested your changes and the device pass-through feature works fine on QDF2400 server platform. Maybe Nate comments on the patch contents but it fixes the problem.
@@ -317,13 +317,13 @@ static void iommu_dma_free_iova(struct iommu_dma_cookie *cookie,
dma_addr_t iova, size_t size)
{
struct iova_domain *iovad = &cookie->iovad;
- unsigned long shift = iova_shift(iovad);
/* The MSI case is only ever cleaning up its most recent allocation */
if (cookie->type == IOMMU_DMA_MSI_COOKIE)
cookie->msi_iova -= size;
else
- free_iova_fast(iovad, iova >> shift, size >> shift);
+ free_iova_fast(iovad, iova_pfn(iovad, iova),
+ size >> iova_shift(iovad));
}
static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t dma_addr,
@@ -538,11 +538,14 @@ static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
{
struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
struct iommu_dma_cookie *cookie = domain->iova_cookie;
- struct iova_domain *iovad = &cookie->iovad;
- size_t iova_off = iova_offset(iovad, phys);
+ size_t iova_off = 0;
dma_addr_t iova;
- size = iova_align(iovad, size + iova_off);
+ if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
+ iova_off = iova_offset(&cookie->iovad, phys);
+ size = iova_align(&cookie->iovad, size + iova_off);
+ }
On 04/13/2017 06:21 AM, Robin Murphy wrote:
> Hi Nate,
>
> On 13/04/17 09:55, Nate Watterson wrote:
>> Currently, the __iommu_dma_{map/free} functions call iova_{offset/align}
>> making them unsuitable for use with iommu_domains having an IOMMU_DMA_MSI
>> cookie since the cookie's iova_domain member, iovad, is uninitialized.
>>
>> Now that iommu_dma_get_msi_page() calls __iommu_dma_map() regardless
>> of cookie type, failures are being seen when mapping MSI target
>> addresses for devices attached to UNMANAGED domains. To work around
>> this issue, the iova_domain granule for IOMMU_DMA_MSI cookies is
>> initialized to the value returned by cookie_msi_granule().
> Oh bum. Thanks for the report.
>
> However, I really don't like bodging around it with deliberate undefined
> behaviour. Fixing things properly doesn't seem too hard:
>
> ----->8-----
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 8348f366ddd1..62618e77bedc 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -396,13 +396,13 @@ static void iommu_dma_free_iova(struct
> iommu_dma_cookie *cookie,
> dma_addr_t iova, size_t size)
> {
> struct iova_domain *iovad = &cookie->iovad;
> - unsigned long shift = iova_shift(iovad);
>
> /* The MSI case is only ever cleaning up its most recent
> allocation */
> if (cookie->type == IOMMU_DMA_MSI_COOKIE)
> cookie->msi_iova -= size;
> else
> - free_iova_fast(iovad, iova >> shift, size >> shift);
> + free_iova_fast(iovad, iova_pfn(iovad, iova),
> + size >> iova_shift(iovad));
> }
>
> static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t
> dma_addr,
> @@ -617,11 +617,14 @@ static dma_addr_t __iommu_dma_map(struct device
> *dev, phys_addr_t phys,
> {
> struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> struct iommu_dma_cookie *cookie = domain->iova_cookie;
> - struct iova_domain *iovad = &cookie->iovad;
> - size_t iova_off = iova_offset(iovad, phys);
> + size_t iova_off = 0;
> dma_addr_t iova;
>
> - size = iova_align(iovad, size + iova_off);
> + if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
> + iova_off = iova_offset(&cookie->iovad, phys);
> + size = iova_align(&cookie->iovad, size + iova_off);
> + }
> +
> iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
> if (!iova)
> return DMA_ERROR_CODE;
> -----8<-----
>
> Untested, and you'll probably want to double-check it anyway given that
> the original oversight was mine in the first place ;)
>
> Robin.
>
>> Fixes: a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")
>> Signed-off-by: Nate Watterson <nwatters-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> ---
>> drivers/iommu/dma-iommu.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 8348f366..d7b0816 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -127,6 +127,16 @@ int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base)
>>
>> cookie->msi_iova = base;
>> domain->iova_cookie = cookie;
>> +
>> + /*
>> + * Setup granule for compatibility with __iommu_dma_{alloc/free} and
>> + * add a compile time check to ensure that writing granule won't
>> + * clobber msi_iova.
>> + */
>> + cookie->iovad.granule = cookie_msi_granule(cookie);
>> + BUILD_BUG_ON(offsetof(struct iova_domain, granule) <
>> + sizeof(cookie->msi_iova));
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL(iommu_get_msi_cookie);
>>
--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 16+ messages in thread* RE: [PATCH] iommu/dma: Setup iova_domain granule for IOMMU_DMA_MSI cookies
@ 2017-05-04 13:32 ` Bharat Bhushan
0 siblings, 0 replies; 16+ messages in thread
From: Bharat Bhushan @ 2017-05-04 13:32 UTC (permalink / raw)
To: shankerd, Robin Murphy, Nate Watterson, Joerg Roedel, iommu,
linux-kernel
Hi Robin,
I faced same issue on our platform and debugged to get the root cause of the issue. Also fixed in somewhat similar way, cleared iova_off and not change size for if cookie->type is IOMMU_DMA_MSI_COOKIE. Anyways that was not as good as below changes.
While just now I saw this was already discussed and solved. Should have looked earlier to save time, but the debugging was some learning experience 😊
I have tested your below change and it works fine on NXP platform.
Thanks
-Bharat
> -----Original Message-----
> From: iommu-bounces@lists.linux-foundation.org [mailto:iommu-
> bounces@lists.linux-foundation.org] On Behalf Of Shanker Donthineni
> Sent: Thursday, April 13, 2017 8:00 PM
> To: Robin Murphy <robin.murphy@arm.com>; Nate Watterson
> <nwatters@codeaurora.org>; Joerg Roedel <joro@8bytes.org>;
> iommu@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] iommu/dma: Setup iova_domain granule for
> IOMMU_DMA_MSI cookies
>
> Hi Robin,
>
> I tested your changes and the device pass-through feature works fine on
> QDF2400 server platform. Maybe Nate comments on the patch contents but
> it fixes the problem.
>
>
> @@ -317,13 +317,13 @@ static void iommu_dma_free_iova(struct
> iommu_dma_cookie *cookie,
> dma_addr_t iova, size_t size) {
> struct iova_domain *iovad = &cookie->iovad;
> - unsigned long shift = iova_shift(iovad);
>
> /* The MSI case is only ever cleaning up its most recent allocation */
> if (cookie->type == IOMMU_DMA_MSI_COOKIE)
> cookie->msi_iova -= size;
> else
> - free_iova_fast(iovad, iova >> shift, size >> shift);
> + free_iova_fast(iovad, iova_pfn(iovad, iova),
> + size >> iova_shift(iovad));
> }
>
> static void __iommu_dma_unmap(struct iommu_domain *domain,
> dma_addr_t dma_addr, @@ -538,11 +538,14 @@ static dma_addr_t
> __iommu_dma_map(struct device *dev, phys_addr_t phys, {
> struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> struct iommu_dma_cookie *cookie = domain->iova_cookie;
> - struct iova_domain *iovad = &cookie->iovad;
> - size_t iova_off = iova_offset(iovad, phys);
> + size_t iova_off = 0;
> dma_addr_t iova;
>
> - size = iova_align(iovad, size + iova_off);
> + if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
> + iova_off = iova_offset(&cookie->iovad, phys);
> + size = iova_align(&cookie->iovad, size + iova_off);
> + }
>
>
> On 04/13/2017 06:21 AM, Robin Murphy wrote:
> > Hi Nate,
> >
> > On 13/04/17 09:55, Nate Watterson wrote:
> >> Currently, the __iommu_dma_{map/free} functions call
> >> iova_{offset/align} making them unsuitable for use with iommu_domains
> >> having an IOMMU_DMA_MSI cookie since the cookie's iova_domain
> member, iovad, is uninitialized.
> >>
> >> Now that iommu_dma_get_msi_page() calls __iommu_dma_map()
> regardless
> >> of cookie type, failures are being seen when mapping MSI target
> >> addresses for devices attached to UNMANAGED domains. To work
> around
> >> this issue, the iova_domain granule for IOMMU_DMA_MSI cookies is
> >> initialized to the value returned by cookie_msi_granule().
> > Oh bum. Thanks for the report.
> >
> > However, I really don't like bodging around it with deliberate
> > undefined behaviour. Fixing things properly doesn't seem too hard:
> >
> > ----->8-----
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 8348f366ddd1..62618e77bedc 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -396,13 +396,13 @@ static void iommu_dma_free_iova(struct
> > iommu_dma_cookie *cookie,
> > dma_addr_t iova, size_t size) {
> > struct iova_domain *iovad = &cookie->iovad;
> > - unsigned long shift = iova_shift(iovad);
> >
> > /* The MSI case is only ever cleaning up its most recent
> > allocation */
> > if (cookie->type == IOMMU_DMA_MSI_COOKIE)
> > cookie->msi_iova -= size;
> > else
> > - free_iova_fast(iovad, iova >> shift, size >> shift);
> > + free_iova_fast(iovad, iova_pfn(iovad, iova),
> > + size >> iova_shift(iovad));
> > }
> >
> > static void __iommu_dma_unmap(struct iommu_domain *domain,
> dma_addr_t
> > dma_addr, @@ -617,11 +617,14 @@ static dma_addr_t
> > __iommu_dma_map(struct device *dev, phys_addr_t phys, {
> > struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> > struct iommu_dma_cookie *cookie = domain->iova_cookie;
> > - struct iova_domain *iovad = &cookie->iovad;
> > - size_t iova_off = iova_offset(iovad, phys);
> > + size_t iova_off = 0;
> > dma_addr_t iova;
> >
> > - size = iova_align(iovad, size + iova_off);
> > + if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
> > + iova_off = iova_offset(&cookie->iovad, phys);
> > + size = iova_align(&cookie->iovad, size + iova_off);
> > + }
> > +
> > iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev),
> dev);
> > if (!iova)
> > return DMA_ERROR_CODE;
> > -----8<-----
> >
> > Untested, and you'll probably want to double-check it anyway given
> > that the original oversight was mine in the first place ;)
> >
> > Robin.
> >
> >> Fixes: a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")
> >> Signed-off-by: Nate Watterson <nwatters@codeaurora.org>
> >> ---
> >> drivers/iommu/dma-iommu.c | 10 ++++++++++
> >> 1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> >> index 8348f366..d7b0816 100644
> >> --- a/drivers/iommu/dma-iommu.c
> >> +++ b/drivers/iommu/dma-iommu.c
> >> @@ -127,6 +127,16 @@ int iommu_get_msi_cookie(struct
> iommu_domain
> >> *domain, dma_addr_t base)
> >>
> >> cookie->msi_iova = base;
> >> domain->iova_cookie = cookie;
> >> +
> >> + /*
> >> + * Setup granule for compatibility with __iommu_dma_{alloc/free}
> and
> >> + * add a compile time check to ensure that writing granule won't
> >> + * clobber msi_iova.
> >> + */
> >> + cookie->iovad.granule = cookie_msi_granule(cookie);
> >> + BUILD_BUG_ON(offsetof(struct iova_domain, granule) <
> >> + sizeof(cookie->msi_iova));
> >> +
> >> return 0;
> >> }
> >> EXPORT_SYMBOL(iommu_get_msi_cookie);
> >>
>
> --
> Shanker Donthineni
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
> Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
> Foundation Collaborative Project.
>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 16+ messages in thread* RE: [PATCH] iommu/dma: Setup iova_domain granule for IOMMU_DMA_MSI cookies
@ 2017-05-04 13:32 ` Bharat Bhushan
0 siblings, 0 replies; 16+ messages in thread
From: Bharat Bhushan @ 2017-05-04 13:32 UTC (permalink / raw)
To: shankerd-sgV2jX0FEOL9JmXXK+q4OQ, Robin Murphy, Nate Watterson,
Joerg Roedel, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Hi Robin,
I faced same issue on our platform and debugged to get the root cause of the issue. Also fixed in somewhat similar way, cleared iova_off and not change size for if cookie->type is IOMMU_DMA_MSI_COOKIE. Anyways that was not as good as below changes.
While just now I saw this was already discussed and solved. Should have looked earlier to save time, but the debugging was some learning experience 😊
I have tested your below change and it works fine on NXP platform.
Thanks
-Bharat
> -----Original Message-----
> From: iommu-bounces@lists.linux-foundation.org [mailto:iommu-
> bounces@lists.linux-foundation.org] On Behalf Of Shanker Donthineni
> Sent: Thursday, April 13, 2017 8:00 PM
> To: Robin Murphy <robin.murphy@arm.com>; Nate Watterson
> <nwatters@codeaurora.org>; Joerg Roedel <joro@8bytes.org>;
> iommu@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] iommu/dma: Setup iova_domain granule for
> IOMMU_DMA_MSI cookies
>
> Hi Robin,
>
> I tested your changes and the device pass-through feature works fine on
> QDF2400 server platform. Maybe Nate comments on the patch contents but
> it fixes the problem.
>
>
> @@ -317,13 +317,13 @@ static void iommu_dma_free_iova(struct
> iommu_dma_cookie *cookie,
> dma_addr_t iova, size_t size) {
> struct iova_domain *iovad = &cookie->iovad;
> - unsigned long shift = iova_shift(iovad);
>
> /* The MSI case is only ever cleaning up its most recent allocation */
> if (cookie->type == IOMMU_DMA_MSI_COOKIE)
> cookie->msi_iova -= size;
> else
> - free_iova_fast(iovad, iova >> shift, size >> shift);
> + free_iova_fast(iovad, iova_pfn(iovad, iova),
> + size >> iova_shift(iovad));
> }
>
> static void __iommu_dma_unmap(struct iommu_domain *domain,
> dma_addr_t dma_addr, @@ -538,11 +538,14 @@ static dma_addr_t
> __iommu_dma_map(struct device *dev, phys_addr_t phys, {
> struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> struct iommu_dma_cookie *cookie = domain->iova_cookie;
> - struct iova_domain *iovad = &cookie->iovad;
> - size_t iova_off = iova_offset(iovad, phys);
> + size_t iova_off = 0;
> dma_addr_t iova;
>
> - size = iova_align(iovad, size + iova_off);
> + if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
> + iova_off = iova_offset(&cookie->iovad, phys);
> + size = iova_align(&cookie->iovad, size + iova_off);
> + }
>
>
> On 04/13/2017 06:21 AM, Robin Murphy wrote:
> > Hi Nate,
> >
> > On 13/04/17 09:55, Nate Watterson wrote:
> >> Currently, the __iommu_dma_{map/free} functions call
> >> iova_{offset/align} making them unsuitable for use with iommu_domains
> >> having an IOMMU_DMA_MSI cookie since the cookie's iova_domain
> member, iovad, is uninitialized.
> >>
> >> Now that iommu_dma_get_msi_page() calls __iommu_dma_map()
> regardless
> >> of cookie type, failures are being seen when mapping MSI target
> >> addresses for devices attached to UNMANAGED domains. To work
> around
> >> this issue, the iova_domain granule for IOMMU_DMA_MSI cookies is
> >> initialized to the value returned by cookie_msi_granule().
> > Oh bum. Thanks for the report.
> >
> > However, I really don't like bodging around it with deliberate
> > undefined behaviour. Fixing things properly doesn't seem too hard:
> >
> > ----->8-----
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 8348f366ddd1..62618e77bedc 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -396,13 +396,13 @@ static void iommu_dma_free_iova(struct
> > iommu_dma_cookie *cookie,
> > dma_addr_t iova, size_t size) {
> > struct iova_domain *iovad = &cookie->iovad;
> > - unsigned long shift = iova_shift(iovad);
> >
> > /* The MSI case is only ever cleaning up its most recent
> > allocation */
> > if (cookie->type == IOMMU_DMA_MSI_COOKIE)
> > cookie->msi_iova -= size;
> > else
> > - free_iova_fast(iovad, iova >> shift, size >> shift);
> > + free_iova_fast(iovad, iova_pfn(iovad, iova),
> > + size >> iova_shift(iovad));
> > }
> >
> > static void __iommu_dma_unmap(struct iommu_domain *domain,
> dma_addr_t
> > dma_addr, @@ -617,11 +617,14 @@ static dma_addr_t
> > __iommu_dma_map(struct device *dev, phys_addr_t phys, {
> > struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> > struct iommu_dma_cookie *cookie = domain->iova_cookie;
> > - struct iova_domain *iovad = &cookie->iovad;
> > - size_t iova_off = iova_offset(iovad, phys);
> > + size_t iova_off = 0;
> > dma_addr_t iova;
> >
> > - size = iova_align(iovad, size + iova_off);
> > + if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
> > + iova_off = iova_offset(&cookie->iovad, phys);
> > + size = iova_align(&cookie->iovad, size + iova_off);
> > + }
> > +
> > iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev),
> dev);
> > if (!iova)
> > return DMA_ERROR_CODE;
> > -----8<-----
> >
> > Untested, and you'll probably want to double-check it anyway given
> > that the original oversight was mine in the first place ;)
> >
> > Robin.
> >
> >> Fixes: a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")
> >> Signed-off-by: Nate Watterson <nwatters@codeaurora.org>
> >> ---
> >> drivers/iommu/dma-iommu.c | 10 ++++++++++
> >> 1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> >> index 8348f366..d7b0816 100644
> >> --- a/drivers/iommu/dma-iommu.c
> >> +++ b/drivers/iommu/dma-iommu.c
> >> @@ -127,6 +127,16 @@ int iommu_get_msi_cookie(struct
> iommu_domain
> >> *domain, dma_addr_t base)
> >>
> >> cookie->msi_iova = base;
> >> domain->iova_cookie = cookie;
> >> +
> >> + /*
> >> + * Setup granule for compatibility with __iommu_dma_{alloc/free}
> and
> >> + * add a compile time check to ensure that writing granule won't
> >> + * clobber msi_iova.
> >> + */
> >> + cookie->iovad.granule = cookie_msi_granule(cookie);
> >> + BUILD_BUG_ON(offsetof(struct iova_domain, granule) <
> >> + sizeof(cookie->msi_iova));
> >> +
> >> return 0;
> >> }
> >> EXPORT_SYMBOL(iommu_get_msi_cookie);
> >>
>
> --
> Shanker Donthineni
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
> Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
> Foundation Collaborative Project.
>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] iommu/dma: Setup iova_domain granule for IOMMU_DMA_MSI cookies
@ 2017-04-13 19:38 ` Nate Watterson
0 siblings, 0 replies; 16+ messages in thread
From: Nate Watterson @ 2017-04-13 19:38 UTC (permalink / raw)
To: Robin Murphy, Joerg Roedel, iommu, linux-kernel; +Cc: shankerd
Hi Robin,
On 4/13/2017 7:21 AM, Robin Murphy wrote:
> Hi Nate,
>
> On 13/04/17 09:55, Nate Watterson wrote:
>> Currently, the __iommu_dma_{map/free} functions call iova_{offset/align}
>> making them unsuitable for use with iommu_domains having an IOMMU_DMA_MSI
>> cookie since the cookie's iova_domain member, iovad, is uninitialized.
>>
>> Now that iommu_dma_get_msi_page() calls __iommu_dma_map() regardless
>> of cookie type, failures are being seen when mapping MSI target
>> addresses for devices attached to UNMANAGED domains. To work around
>> this issue, the iova_domain granule for IOMMU_DMA_MSI cookies is
>> initialized to the value returned by cookie_msi_granule().
>
> Oh bum. Thanks for the report.
>
> However, I really don't like bodging around it with deliberate undefined
> behaviour. Fixing things properly doesn't seem too hard:
I was not especially please with my solution, but I wanted to avoid
potentially missing any other spots in the code where granule was
used uninitialized. The compile time check made me feel a little
less dirty about innappropriately using the iova_domain with MSI
cookies.
>
> ----->8-----
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 8348f366ddd1..62618e77bedc 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -396,13 +396,13 @@ static void iommu_dma_free_iova(struct
> iommu_dma_cookie *cookie,
> dma_addr_t iova, size_t size)
> {
> struct iova_domain *iovad = &cookie->iovad;
> - unsigned long shift = iova_shift(iovad);
>
> /* The MSI case is only ever cleaning up its most recent
> allocation */
> if (cookie->type == IOMMU_DMA_MSI_COOKIE)
> cookie->msi_iova -= size;
> else
> - free_iova_fast(iovad, iova >> shift, size >> shift);
> + free_iova_fast(iovad, iova_pfn(iovad, iova),
> + size >> iova_shift(iovad));
> }
>
> static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t
> dma_addr,
> @@ -617,11 +617,14 @@ static dma_addr_t __iommu_dma_map(struct device
> *dev, phys_addr_t phys,
> {
> struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> struct iommu_dma_cookie *cookie = domain->iova_cookie;
> - struct iova_domain *iovad = &cookie->iovad;
> - size_t iova_off = iova_offset(iovad, phys);
> + size_t iova_off = 0;
> dma_addr_t iova;
>
> - size = iova_align(iovad, size + iova_off);
> + if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
> + iova_off = iova_offset(&cookie->iovad, phys);
> + size = iova_align(&cookie->iovad, size + iova_off);
> + }
> +
> iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
> if (!iova)
> return DMA_ERROR_CODE;
> -----8<-----
>
> Untested, and you'll probably want to double-check it anyway given that
> the original oversight was mine in the first place ;)
This looks good to me. As Shanker has already mentioned, it does fix the
faults we were previously seeing with direct device assignment. I also
verified that there aren't any other obvious cases of a granule == 0
being used in the dma_iommu code by adding BUG_ON(!iovad->granule) to
iova_{mask/align/offset/...} and running a variety of tests without
issue.
Are you going to post the patch?
>
> Robin.
>
>> Fixes: a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")
>> Signed-off-by: Nate Watterson <nwatters@codeaurora.org>
>> ---
>> drivers/iommu/dma-iommu.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 8348f366..d7b0816 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -127,6 +127,16 @@ int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base)
>>
>> cookie->msi_iova = base;
>> domain->iova_cookie = cookie;
>> +
>> + /*
>> + * Setup granule for compatibility with __iommu_dma_{alloc/free} and
>> + * add a compile time check to ensure that writing granule won't
>> + * clobber msi_iova.
>> + */
>> + cookie->iovad.granule = cookie_msi_granule(cookie);
>> + BUILD_BUG_ON(offsetof(struct iova_domain, granule) <
>> + sizeof(cookie->msi_iova));
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL(iommu_get_msi_cookie);
>>
>
--
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] iommu/dma: Setup iova_domain granule for IOMMU_DMA_MSI cookies
@ 2017-04-13 19:38 ` Nate Watterson
0 siblings, 0 replies; 16+ messages in thread
From: Nate Watterson @ 2017-04-13 19:38 UTC (permalink / raw)
To: Robin Murphy, Joerg Roedel,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: shankerd-sgV2jX0FEOL9JmXXK+q4OQ
Hi Robin,
On 4/13/2017 7:21 AM, Robin Murphy wrote:
> Hi Nate,
>
> On 13/04/17 09:55, Nate Watterson wrote:
>> Currently, the __iommu_dma_{map/free} functions call iova_{offset/align}
>> making them unsuitable for use with iommu_domains having an IOMMU_DMA_MSI
>> cookie since the cookie's iova_domain member, iovad, is uninitialized.
>>
>> Now that iommu_dma_get_msi_page() calls __iommu_dma_map() regardless
>> of cookie type, failures are being seen when mapping MSI target
>> addresses for devices attached to UNMANAGED domains. To work around
>> this issue, the iova_domain granule for IOMMU_DMA_MSI cookies is
>> initialized to the value returned by cookie_msi_granule().
>
> Oh bum. Thanks for the report.
>
> However, I really don't like bodging around it with deliberate undefined
> behaviour. Fixing things properly doesn't seem too hard:
I was not especially please with my solution, but I wanted to avoid
potentially missing any other spots in the code where granule was
used uninitialized. The compile time check made me feel a little
less dirty about innappropriately using the iova_domain with MSI
cookies.
>
> ----->8-----
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 8348f366ddd1..62618e77bedc 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -396,13 +396,13 @@ static void iommu_dma_free_iova(struct
> iommu_dma_cookie *cookie,
> dma_addr_t iova, size_t size)
> {
> struct iova_domain *iovad = &cookie->iovad;
> - unsigned long shift = iova_shift(iovad);
>
> /* The MSI case is only ever cleaning up its most recent
> allocation */
> if (cookie->type == IOMMU_DMA_MSI_COOKIE)
> cookie->msi_iova -= size;
> else
> - free_iova_fast(iovad, iova >> shift, size >> shift);
> + free_iova_fast(iovad, iova_pfn(iovad, iova),
> + size >> iova_shift(iovad));
> }
>
> static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t
> dma_addr,
> @@ -617,11 +617,14 @@ static dma_addr_t __iommu_dma_map(struct device
> *dev, phys_addr_t phys,
> {
> struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> struct iommu_dma_cookie *cookie = domain->iova_cookie;
> - struct iova_domain *iovad = &cookie->iovad;
> - size_t iova_off = iova_offset(iovad, phys);
> + size_t iova_off = 0;
> dma_addr_t iova;
>
> - size = iova_align(iovad, size + iova_off);
> + if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
> + iova_off = iova_offset(&cookie->iovad, phys);
> + size = iova_align(&cookie->iovad, size + iova_off);
> + }
> +
> iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev), dev);
> if (!iova)
> return DMA_ERROR_CODE;
> -----8<-----
>
> Untested, and you'll probably want to double-check it anyway given that
> the original oversight was mine in the first place ;)
This looks good to me. As Shanker has already mentioned, it does fix the
faults we were previously seeing with direct device assignment. I also
verified that there aren't any other obvious cases of a granule == 0
being used in the dma_iommu code by adding BUG_ON(!iovad->granule) to
iova_{mask/align/offset/...} and running a variety of tests without
issue.
Are you going to post the patch?
>
> Robin.
>
>> Fixes: a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")
>> Signed-off-by: Nate Watterson <nwatters-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> ---
>> drivers/iommu/dma-iommu.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 8348f366..d7b0816 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -127,6 +127,16 @@ int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base)
>>
>> cookie->msi_iova = base;
>> domain->iova_cookie = cookie;
>> +
>> + /*
>> + * Setup granule for compatibility with __iommu_dma_{alloc/free} and
>> + * add a compile time check to ensure that writing granule won't
>> + * clobber msi_iova.
>> + */
>> + cookie->iovad.granule = cookie_msi_granule(cookie);
>> + BUILD_BUG_ON(offsetof(struct iova_domain, granule) <
>> + sizeof(cookie->msi_iova));
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL(iommu_get_msi_cookie);
>>
>
--
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] iommu/dma: Setup iova_domain granule for IOMMU_DMA_MSI cookies
@ 2017-05-16 19:55 ` Auger Eric
0 siblings, 0 replies; 16+ messages in thread
From: Auger Eric @ 2017-05-16 19:55 UTC (permalink / raw)
To: Nate Watterson, Robin Murphy, Joerg Roedel, iommu, linux-kernel; +Cc: shankerd
Hi,
On 13/04/2017 21:38, Nate Watterson wrote:
> Hi Robin,
>
> On 4/13/2017 7:21 AM, Robin Murphy wrote:
>> Hi Nate,
>>
>> On 13/04/17 09:55, Nate Watterson wrote:
>>> Currently, the __iommu_dma_{map/free} functions call iova_{offset/align}
>>> making them unsuitable for use with iommu_domains having an
>>> IOMMU_DMA_MSI
>>> cookie since the cookie's iova_domain member, iovad, is uninitialized.
>>>
>>> Now that iommu_dma_get_msi_page() calls __iommu_dma_map() regardless
>>> of cookie type, failures are being seen when mapping MSI target
>>> addresses for devices attached to UNMANAGED domains. To work around
>>> this issue, the iova_domain granule for IOMMU_DMA_MSI cookies is
>>> initialized to the value returned by cookie_msi_granule().
>>
>> Oh bum. Thanks for the report.
>>
>> However, I really don't like bodging around it with deliberate undefined
>> behaviour. Fixing things properly doesn't seem too hard:
>
> I was not especially please with my solution, but I wanted to avoid
> potentially missing any other spots in the code where granule was
> used uninitialized. The compile time check made me feel a little
> less dirty about innappropriately using the iova_domain with MSI
> cookies.
>
>>
>> ----->8-----
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 8348f366ddd1..62618e77bedc 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -396,13 +396,13 @@ static void iommu_dma_free_iova(struct
>> iommu_dma_cookie *cookie,
>> dma_addr_t iova, size_t size)
>> {
>> struct iova_domain *iovad = &cookie->iovad;
>> - unsigned long shift = iova_shift(iovad);
>>
>> /* The MSI case is only ever cleaning up its most recent
>> allocation */
>> if (cookie->type == IOMMU_DMA_MSI_COOKIE)
>> cookie->msi_iova -= size;
>> else
>> - free_iova_fast(iovad, iova >> shift, size >> shift);
>> + free_iova_fast(iovad, iova_pfn(iovad, iova),
>> + size >> iova_shift(iovad));
>> }
>>
>> static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t
>> dma_addr,
>> @@ -617,11 +617,14 @@ static dma_addr_t __iommu_dma_map(struct device
>> *dev, phys_addr_t phys,
>> {
>> struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>> struct iommu_dma_cookie *cookie = domain->iova_cookie;
>> - struct iova_domain *iovad = &cookie->iovad;
>> - size_t iova_off = iova_offset(iovad, phys);
>> + size_t iova_off = 0;
>> dma_addr_t iova;
>>
>> - size = iova_align(iovad, size + iova_off);
>> + if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
>> + iova_off = iova_offset(&cookie->iovad, phys);
>> + size = iova_align(&cookie->iovad, size + iova_off);
>> + }
>> +
>> iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev),
>> dev);
>> if (!iova)
>> return DMA_ERROR_CODE;
>> -----8<-----
>>
>> Untested, and you'll probably want to double-check it anyway given that
>> the original oversight was mine in the first place ;)
>
> This looks good to me. As Shanker has already mentioned, it does fix the
> faults we were previously seeing with direct device assignment. I also
> verified that there aren't any other obvious cases of a granule == 0
> being used in the dma_iommu code by adding BUG_ON(!iovad->granule) to
> iova_{mask/align/offset/...} and running a variety of tests without
> issue.
>
> Are you going to post the patch?
I also noticed PCIe passthrough/ARM is broken for me with 4.12-r1. I
tested Robin's patch as well, on Cavium ThunderX, and this fixes the
faults I have seen.
Has anyone sent a formal patch?
Thanks
Eric
>
>>
>> Robin.
>>
>>> Fixes: a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")
>>> Signed-off-by: Nate Watterson <nwatters@codeaurora.org>
>>> ---
>>> drivers/iommu/dma-iommu.c | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>> index 8348f366..d7b0816 100644
>>> --- a/drivers/iommu/dma-iommu.c
>>> +++ b/drivers/iommu/dma-iommu.c
>>> @@ -127,6 +127,16 @@ int iommu_get_msi_cookie(struct iommu_domain
>>> *domain, dma_addr_t base)
>>> cookie->msi_iova = base;
>>> domain->iova_cookie = cookie;
>>> +
>>> + /*
>>> + * Setup granule for compatibility with __iommu_dma_{alloc/free}
>>> and
>>> + * add a compile time check to ensure that writing granule won't
>>> + * clobber msi_iova.
>>> + */
>>> + cookie->iovad.granule = cookie_msi_granule(cookie);
>>> + BUILD_BUG_ON(offsetof(struct iova_domain, granule) <
>>> + sizeof(cookie->msi_iova));
>>> +
>>> return 0;
>>> }
>>> EXPORT_SYMBOL(iommu_get_msi_cookie);
>>>
>>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] iommu/dma: Setup iova_domain granule for IOMMU_DMA_MSI cookies
@ 2017-05-16 19:55 ` Auger Eric
0 siblings, 0 replies; 16+ messages in thread
From: Auger Eric @ 2017-05-16 19:55 UTC (permalink / raw)
To: Nate Watterson, Robin Murphy, Joerg Roedel,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: shankerd-sgV2jX0FEOL9JmXXK+q4OQ
Hi,
On 13/04/2017 21:38, Nate Watterson wrote:
> Hi Robin,
>
> On 4/13/2017 7:21 AM, Robin Murphy wrote:
>> Hi Nate,
>>
>> On 13/04/17 09:55, Nate Watterson wrote:
>>> Currently, the __iommu_dma_{map/free} functions call iova_{offset/align}
>>> making them unsuitable for use with iommu_domains having an
>>> IOMMU_DMA_MSI
>>> cookie since the cookie's iova_domain member, iovad, is uninitialized.
>>>
>>> Now that iommu_dma_get_msi_page() calls __iommu_dma_map() regardless
>>> of cookie type, failures are being seen when mapping MSI target
>>> addresses for devices attached to UNMANAGED domains. To work around
>>> this issue, the iova_domain granule for IOMMU_DMA_MSI cookies is
>>> initialized to the value returned by cookie_msi_granule().
>>
>> Oh bum. Thanks for the report.
>>
>> However, I really don't like bodging around it with deliberate undefined
>> behaviour. Fixing things properly doesn't seem too hard:
>
> I was not especially please with my solution, but I wanted to avoid
> potentially missing any other spots in the code where granule was
> used uninitialized. The compile time check made me feel a little
> less dirty about innappropriately using the iova_domain with MSI
> cookies.
>
>>
>> ----->8-----
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 8348f366ddd1..62618e77bedc 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -396,13 +396,13 @@ static void iommu_dma_free_iova(struct
>> iommu_dma_cookie *cookie,
>> dma_addr_t iova, size_t size)
>> {
>> struct iova_domain *iovad = &cookie->iovad;
>> - unsigned long shift = iova_shift(iovad);
>>
>> /* The MSI case is only ever cleaning up its most recent
>> allocation */
>> if (cookie->type == IOMMU_DMA_MSI_COOKIE)
>> cookie->msi_iova -= size;
>> else
>> - free_iova_fast(iovad, iova >> shift, size >> shift);
>> + free_iova_fast(iovad, iova_pfn(iovad, iova),
>> + size >> iova_shift(iovad));
>> }
>>
>> static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t
>> dma_addr,
>> @@ -617,11 +617,14 @@ static dma_addr_t __iommu_dma_map(struct device
>> *dev, phys_addr_t phys,
>> {
>> struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>> struct iommu_dma_cookie *cookie = domain->iova_cookie;
>> - struct iova_domain *iovad = &cookie->iovad;
>> - size_t iova_off = iova_offset(iovad, phys);
>> + size_t iova_off = 0;
>> dma_addr_t iova;
>>
>> - size = iova_align(iovad, size + iova_off);
>> + if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
>> + iova_off = iova_offset(&cookie->iovad, phys);
>> + size = iova_align(&cookie->iovad, size + iova_off);
>> + }
>> +
>> iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev),
>> dev);
>> if (!iova)
>> return DMA_ERROR_CODE;
>> -----8<-----
>>
>> Untested, and you'll probably want to double-check it anyway given that
>> the original oversight was mine in the first place ;)
>
> This looks good to me. As Shanker has already mentioned, it does fix the
> faults we were previously seeing with direct device assignment. I also
> verified that there aren't any other obvious cases of a granule == 0
> being used in the dma_iommu code by adding BUG_ON(!iovad->granule) to
> iova_{mask/align/offset/...} and running a variety of tests without
> issue.
>
> Are you going to post the patch?
I also noticed PCIe passthrough/ARM is broken for me with 4.12-r1. I
tested Robin's patch as well, on Cavium ThunderX, and this fixes the
faults I have seen.
Has anyone sent a formal patch?
Thanks
Eric
>
>>
>> Robin.
>>
>>> Fixes: a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")
>>> Signed-off-by: Nate Watterson <nwatters-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>> ---
>>> drivers/iommu/dma-iommu.c | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>> index 8348f366..d7b0816 100644
>>> --- a/drivers/iommu/dma-iommu.c
>>> +++ b/drivers/iommu/dma-iommu.c
>>> @@ -127,6 +127,16 @@ int iommu_get_msi_cookie(struct iommu_domain
>>> *domain, dma_addr_t base)
>>> cookie->msi_iova = base;
>>> domain->iova_cookie = cookie;
>>> +
>>> + /*
>>> + * Setup granule for compatibility with __iommu_dma_{alloc/free}
>>> and
>>> + * add a compile time check to ensure that writing granule won't
>>> + * clobber msi_iova.
>>> + */
>>> + cookie->iovad.granule = cookie_msi_granule(cookie);
>>> + BUILD_BUG_ON(offsetof(struct iova_domain, granule) <
>>> + sizeof(cookie->msi_iova));
>>> +
>>> return 0;
>>> }
>>> EXPORT_SYMBOL(iommu_get_msi_cookie);
>>>
>>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] iommu/dma: Setup iova_domain granule for IOMMU_DMA_MSI cookies
@ 2017-05-16 20:07 ` Nate Watterson
0 siblings, 0 replies; 16+ messages in thread
From: Nate Watterson @ 2017-05-16 20:07 UTC (permalink / raw)
To: Auger Eric, Robin Murphy, Joerg Roedel, iommu, linux-kernel; +Cc: shankerd
On 5/16/2017 3:55 PM, Auger Eric wrote:
> Hi,
>
> On 13/04/2017 21:38, Nate Watterson wrote:
>> Hi Robin,
>>
>> On 4/13/2017 7:21 AM, Robin Murphy wrote:
>>> Hi Nate,
>>>
>>> On 13/04/17 09:55, Nate Watterson wrote:
>>>> Currently, the __iommu_dma_{map/free} functions call iova_{offset/align}
>>>> making them unsuitable for use with iommu_domains having an
>>>> IOMMU_DMA_MSI
>>>> cookie since the cookie's iova_domain member, iovad, is uninitialized.
>>>>
>>>> Now that iommu_dma_get_msi_page() calls __iommu_dma_map() regardless
>>>> of cookie type, failures are being seen when mapping MSI target
>>>> addresses for devices attached to UNMANAGED domains. To work around
>>>> this issue, the iova_domain granule for IOMMU_DMA_MSI cookies is
>>>> initialized to the value returned by cookie_msi_granule().
>>>
>>> Oh bum. Thanks for the report.
>>>
>>> However, I really don't like bodging around it with deliberate undefined
>>> behaviour. Fixing things properly doesn't seem too hard:
>>
>> I was not especially please with my solution, but I wanted to avoid
>> potentially missing any other spots in the code where granule was
>> used uninitialized. The compile time check made me feel a little
>> less dirty about innappropriately using the iova_domain with MSI
>> cookies.
>>
>>>
>>> ----->8-----
>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>> index 8348f366ddd1..62618e77bedc 100644
>>> --- a/drivers/iommu/dma-iommu.c
>>> +++ b/drivers/iommu/dma-iommu.c
>>> @@ -396,13 +396,13 @@ static void iommu_dma_free_iova(struct
>>> iommu_dma_cookie *cookie,
>>> dma_addr_t iova, size_t size)
>>> {
>>> struct iova_domain *iovad = &cookie->iovad;
>>> - unsigned long shift = iova_shift(iovad);
>>>
>>> /* The MSI case is only ever cleaning up its most recent
>>> allocation */
>>> if (cookie->type == IOMMU_DMA_MSI_COOKIE)
>>> cookie->msi_iova -= size;
>>> else
>>> - free_iova_fast(iovad, iova >> shift, size >> shift);
>>> + free_iova_fast(iovad, iova_pfn(iovad, iova),
>>> + size >> iova_shift(iovad));
>>> }
>>>
>>> static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t
>>> dma_addr,
>>> @@ -617,11 +617,14 @@ static dma_addr_t __iommu_dma_map(struct device
>>> *dev, phys_addr_t phys,
>>> {
>>> struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>>> struct iommu_dma_cookie *cookie = domain->iova_cookie;
>>> - struct iova_domain *iovad = &cookie->iovad;
>>> - size_t iova_off = iova_offset(iovad, phys);
>>> + size_t iova_off = 0;
>>> dma_addr_t iova;
>>>
>>> - size = iova_align(iovad, size + iova_off);
>>> + if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
>>> + iova_off = iova_offset(&cookie->iovad, phys);
>>> + size = iova_align(&cookie->iovad, size + iova_off);
>>> + }
>>> +
>>> iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev),
>>> dev);
>>> if (!iova)
>>> return DMA_ERROR_CODE;
>>> -----8<-----
>>>
>>> Untested, and you'll probably want to double-check it anyway given that
>>> the original oversight was mine in the first place ;)
>>
>> This looks good to me. As Shanker has already mentioned, it does fix the
>> faults we were previously seeing with direct device assignment. I also
>> verified that there aren't any other obvious cases of a granule == 0
>> being used in the dma_iommu code by adding BUG_ON(!iovad->granule) to
>> iova_{mask/align/offset/...} and running a variety of tests without
>> issue.
>>
>> Are you going to post the patch?
>
> I also noticed PCIe passthrough/ARM is broken for me with 4.12-r1. I
> tested Robin's patch as well, on Cavium ThunderX, and this fixes the
> faults I have seen.
>
> Has anyone sent a formal patch?
iommu/dma: Don't touch invalid iova_domain members
>
> Thanks
>
> Eric
>
>>
>>>
>>> Robin.
>>>
>>>> Fixes: a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")
>>>> Signed-off-by: Nate Watterson <nwatters@codeaurora.org>
>>>> ---
>>>> drivers/iommu/dma-iommu.c | 10 ++++++++++
>>>> 1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>>> index 8348f366..d7b0816 100644
>>>> --- a/drivers/iommu/dma-iommu.c
>>>> +++ b/drivers/iommu/dma-iommu.c
>>>> @@ -127,6 +127,16 @@ int iommu_get_msi_cookie(struct iommu_domain
>>>> *domain, dma_addr_t base)
>>>> cookie->msi_iova = base;
>>>> domain->iova_cookie = cookie;
>>>> +
>>>> + /*
>>>> + * Setup granule for compatibility with __iommu_dma_{alloc/free}
>>>> and
>>>> + * add a compile time check to ensure that writing granule won't
>>>> + * clobber msi_iova.
>>>> + */
>>>> + cookie->iovad.granule = cookie_msi_granule(cookie);
>>>> + BUILD_BUG_ON(offsetof(struct iova_domain, granule) <
>>>> + sizeof(cookie->msi_iova));
>>>> +
>>>> return 0;
>>>> }
>>>> EXPORT_SYMBOL(iommu_get_msi_cookie);
>>>>
>>>
--
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] iommu/dma: Setup iova_domain granule for IOMMU_DMA_MSI cookies
@ 2017-05-16 20:07 ` Nate Watterson
0 siblings, 0 replies; 16+ messages in thread
From: Nate Watterson @ 2017-05-16 20:07 UTC (permalink / raw)
To: Auger Eric, Robin Murphy, Joerg Roedel,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: shankerd-sgV2jX0FEOL9JmXXK+q4OQ
On 5/16/2017 3:55 PM, Auger Eric wrote:
> Hi,
>
> On 13/04/2017 21:38, Nate Watterson wrote:
>> Hi Robin,
>>
>> On 4/13/2017 7:21 AM, Robin Murphy wrote:
>>> Hi Nate,
>>>
>>> On 13/04/17 09:55, Nate Watterson wrote:
>>>> Currently, the __iommu_dma_{map/free} functions call iova_{offset/align}
>>>> making them unsuitable for use with iommu_domains having an
>>>> IOMMU_DMA_MSI
>>>> cookie since the cookie's iova_domain member, iovad, is uninitialized.
>>>>
>>>> Now that iommu_dma_get_msi_page() calls __iommu_dma_map() regardless
>>>> of cookie type, failures are being seen when mapping MSI target
>>>> addresses for devices attached to UNMANAGED domains. To work around
>>>> this issue, the iova_domain granule for IOMMU_DMA_MSI cookies is
>>>> initialized to the value returned by cookie_msi_granule().
>>>
>>> Oh bum. Thanks for the report.
>>>
>>> However, I really don't like bodging around it with deliberate undefined
>>> behaviour. Fixing things properly doesn't seem too hard:
>>
>> I was not especially please with my solution, but I wanted to avoid
>> potentially missing any other spots in the code where granule was
>> used uninitialized. The compile time check made me feel a little
>> less dirty about innappropriately using the iova_domain with MSI
>> cookies.
>>
>>>
>>> ----->8-----
>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>> index 8348f366ddd1..62618e77bedc 100644
>>> --- a/drivers/iommu/dma-iommu.c
>>> +++ b/drivers/iommu/dma-iommu.c
>>> @@ -396,13 +396,13 @@ static void iommu_dma_free_iova(struct
>>> iommu_dma_cookie *cookie,
>>> dma_addr_t iova, size_t size)
>>> {
>>> struct iova_domain *iovad = &cookie->iovad;
>>> - unsigned long shift = iova_shift(iovad);
>>>
>>> /* The MSI case is only ever cleaning up its most recent
>>> allocation */
>>> if (cookie->type == IOMMU_DMA_MSI_COOKIE)
>>> cookie->msi_iova -= size;
>>> else
>>> - free_iova_fast(iovad, iova >> shift, size >> shift);
>>> + free_iova_fast(iovad, iova_pfn(iovad, iova),
>>> + size >> iova_shift(iovad));
>>> }
>>>
>>> static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t
>>> dma_addr,
>>> @@ -617,11 +617,14 @@ static dma_addr_t __iommu_dma_map(struct device
>>> *dev, phys_addr_t phys,
>>> {
>>> struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>>> struct iommu_dma_cookie *cookie = domain->iova_cookie;
>>> - struct iova_domain *iovad = &cookie->iovad;
>>> - size_t iova_off = iova_offset(iovad, phys);
>>> + size_t iova_off = 0;
>>> dma_addr_t iova;
>>>
>>> - size = iova_align(iovad, size + iova_off);
>>> + if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
>>> + iova_off = iova_offset(&cookie->iovad, phys);
>>> + size = iova_align(&cookie->iovad, size + iova_off);
>>> + }
>>> +
>>> iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev),
>>> dev);
>>> if (!iova)
>>> return DMA_ERROR_CODE;
>>> -----8<-----
>>>
>>> Untested, and you'll probably want to double-check it anyway given that
>>> the original oversight was mine in the first place ;)
>>
>> This looks good to me. As Shanker has already mentioned, it does fix the
>> faults we were previously seeing with direct device assignment. I also
>> verified that there aren't any other obvious cases of a granule == 0
>> being used in the dma_iommu code by adding BUG_ON(!iovad->granule) to
>> iova_{mask/align/offset/...} and running a variety of tests without
>> issue.
>>
>> Are you going to post the patch?
>
> I also noticed PCIe passthrough/ARM is broken for me with 4.12-r1. I
> tested Robin's patch as well, on Cavium ThunderX, and this fixes the
> faults I have seen.
>
> Has anyone sent a formal patch?
iommu/dma: Don't touch invalid iova_domain members
>
> Thanks
>
> Eric
>
>>
>>>
>>> Robin.
>>>
>>>> Fixes: a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")
>>>> Signed-off-by: Nate Watterson <nwatters-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>>> ---
>>>> drivers/iommu/dma-iommu.c | 10 ++++++++++
>>>> 1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>>> index 8348f366..d7b0816 100644
>>>> --- a/drivers/iommu/dma-iommu.c
>>>> +++ b/drivers/iommu/dma-iommu.c
>>>> @@ -127,6 +127,16 @@ int iommu_get_msi_cookie(struct iommu_domain
>>>> *domain, dma_addr_t base)
>>>> cookie->msi_iova = base;
>>>> domain->iova_cookie = cookie;
>>>> +
>>>> + /*
>>>> + * Setup granule for compatibility with __iommu_dma_{alloc/free}
>>>> and
>>>> + * add a compile time check to ensure that writing granule won't
>>>> + * clobber msi_iova.
>>>> + */
>>>> + cookie->iovad.granule = cookie_msi_granule(cookie);
>>>> + BUILD_BUG_ON(offsetof(struct iova_domain, granule) <
>>>> + sizeof(cookie->msi_iova));
>>>> +
>>>> return 0;
>>>> }
>>>> EXPORT_SYMBOL(iommu_get_msi_cookie);
>>>>
>>>
--
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] iommu/dma: Setup iova_domain granule for IOMMU_DMA_MSI cookies
@ 2017-05-16 20:20 ` Auger Eric
0 siblings, 0 replies; 16+ messages in thread
From: Auger Eric @ 2017-05-16 20:20 UTC (permalink / raw)
To: Nate Watterson, Robin Murphy, Joerg Roedel, iommu, linux-kernel; +Cc: shankerd
Hi Nate,
On 16/05/2017 22:07, Nate Watterson wrote:
>
>
> On 5/16/2017 3:55 PM, Auger Eric wrote:
>> Hi,
>>
>> On 13/04/2017 21:38, Nate Watterson wrote:
>>> Hi Robin,
>>>
>>> On 4/13/2017 7:21 AM, Robin Murphy wrote:
>>>> Hi Nate,
>>>>
>>>> On 13/04/17 09:55, Nate Watterson wrote:
>>>>> Currently, the __iommu_dma_{map/free} functions call
>>>>> iova_{offset/align}
>>>>> making them unsuitable for use with iommu_domains having an
>>>>> IOMMU_DMA_MSI
>>>>> cookie since the cookie's iova_domain member, iovad, is uninitialized.
>>>>>
>>>>> Now that iommu_dma_get_msi_page() calls __iommu_dma_map() regardless
>>>>> of cookie type, failures are being seen when mapping MSI target
>>>>> addresses for devices attached to UNMANAGED domains. To work around
>>>>> this issue, the iova_domain granule for IOMMU_DMA_MSI cookies is
>>>>> initialized to the value returned by cookie_msi_granule().
>>>>
>>>> Oh bum. Thanks for the report.
>>>>
>>>> However, I really don't like bodging around it with deliberate
>>>> undefined
>>>> behaviour. Fixing things properly doesn't seem too hard:
>>>
>>> I was not especially please with my solution, but I wanted to avoid
>>> potentially missing any other spots in the code where granule was
>>> used uninitialized. The compile time check made me feel a little
>>> less dirty about innappropriately using the iova_domain with MSI
>>> cookies.
>>>
>>>>
>>>> ----->8-----
>>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>>> index 8348f366ddd1..62618e77bedc 100644
>>>> --- a/drivers/iommu/dma-iommu.c
>>>> +++ b/drivers/iommu/dma-iommu.c
>>>> @@ -396,13 +396,13 @@ static void iommu_dma_free_iova(struct
>>>> iommu_dma_cookie *cookie,
>>>> dma_addr_t iova, size_t size)
>>>> {
>>>> struct iova_domain *iovad = &cookie->iovad;
>>>> - unsigned long shift = iova_shift(iovad);
>>>>
>>>> /* The MSI case is only ever cleaning up its most recent
>>>> allocation */
>>>> if (cookie->type == IOMMU_DMA_MSI_COOKIE)
>>>> cookie->msi_iova -= size;
>>>> else
>>>> - free_iova_fast(iovad, iova >> shift, size >> shift);
>>>> + free_iova_fast(iovad, iova_pfn(iovad, iova),
>>>> + size >> iova_shift(iovad));
>>>> }
>>>>
>>>> static void __iommu_dma_unmap(struct iommu_domain *domain,
>>>> dma_addr_t
>>>> dma_addr,
>>>> @@ -617,11 +617,14 @@ static dma_addr_t __iommu_dma_map(struct device
>>>> *dev, phys_addr_t phys,
>>>> {
>>>> struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>>>> struct iommu_dma_cookie *cookie = domain->iova_cookie;
>>>> - struct iova_domain *iovad = &cookie->iovad;
>>>> - size_t iova_off = iova_offset(iovad, phys);
>>>> + size_t iova_off = 0;
>>>> dma_addr_t iova;
>>>>
>>>> - size = iova_align(iovad, size + iova_off);
>>>> + if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
>>>> + iova_off = iova_offset(&cookie->iovad, phys);
>>>> + size = iova_align(&cookie->iovad, size + iova_off);
>>>> + }
>>>> +
>>>> iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev),
>>>> dev);
>>>> if (!iova)
>>>> return DMA_ERROR_CODE;
>>>> -----8<-----
>>>>
>>>> Untested, and you'll probably want to double-check it anyway given that
>>>> the original oversight was mine in the first place ;)
>>>
>>> This looks good to me. As Shanker has already mentioned, it does fix the
>>> faults we were previously seeing with direct device assignment. I also
>>> verified that there aren't any other obvious cases of a granule == 0
>>> being used in the dma_iommu code by adding BUG_ON(!iovad->granule) to
>>> iova_{mask/align/offset/...} and running a variety of tests without
>>> issue.
>>>
>>> Are you going to post the patch?
>>
>> I also noticed PCIe passthrough/ARM is broken for me with 4.12-r1. I
>> tested Robin's patch as well, on Cavium ThunderX, and this fixes the
>> faults I have seen.
>>
>> Has anyone sent a formal patch?
>
> iommu/dma: Don't touch invalid iova_domain members
thanks!
Best Regards
Eric
>
>>
>> Thanks
>>
>> Eric
>>
>>>
>>>>
>>>> Robin.
>>>>
>>>>> Fixes: a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")
>>>>> Signed-off-by: Nate Watterson <nwatters@codeaurora.org>
>>>>> ---
>>>>> drivers/iommu/dma-iommu.c | 10 ++++++++++
>>>>> 1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>>>> index 8348f366..d7b0816 100644
>>>>> --- a/drivers/iommu/dma-iommu.c
>>>>> +++ b/drivers/iommu/dma-iommu.c
>>>>> @@ -127,6 +127,16 @@ int iommu_get_msi_cookie(struct iommu_domain
>>>>> *domain, dma_addr_t base)
>>>>> cookie->msi_iova = base;
>>>>> domain->iova_cookie = cookie;
>>>>> +
>>>>> + /*
>>>>> + * Setup granule for compatibility with __iommu_dma_{alloc/free}
>>>>> and
>>>>> + * add a compile time check to ensure that writing granule won't
>>>>> + * clobber msi_iova.
>>>>> + */
>>>>> + cookie->iovad.granule = cookie_msi_granule(cookie);
>>>>> + BUILD_BUG_ON(offsetof(struct iova_domain, granule) <
>>>>> + sizeof(cookie->msi_iova));
>>>>> +
>>>>> return 0;
>>>>> }
>>>>> EXPORT_SYMBOL(iommu_get_msi_cookie);
>>>>>
>>>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] iommu/dma: Setup iova_domain granule for IOMMU_DMA_MSI cookies
@ 2017-05-16 20:20 ` Auger Eric
0 siblings, 0 replies; 16+ messages in thread
From: Auger Eric @ 2017-05-16 20:20 UTC (permalink / raw)
To: Nate Watterson, Robin Murphy, Joerg Roedel,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: shankerd-sgV2jX0FEOL9JmXXK+q4OQ
Hi Nate,
On 16/05/2017 22:07, Nate Watterson wrote:
>
>
> On 5/16/2017 3:55 PM, Auger Eric wrote:
>> Hi,
>>
>> On 13/04/2017 21:38, Nate Watterson wrote:
>>> Hi Robin,
>>>
>>> On 4/13/2017 7:21 AM, Robin Murphy wrote:
>>>> Hi Nate,
>>>>
>>>> On 13/04/17 09:55, Nate Watterson wrote:
>>>>> Currently, the __iommu_dma_{map/free} functions call
>>>>> iova_{offset/align}
>>>>> making them unsuitable for use with iommu_domains having an
>>>>> IOMMU_DMA_MSI
>>>>> cookie since the cookie's iova_domain member, iovad, is uninitialized.
>>>>>
>>>>> Now that iommu_dma_get_msi_page() calls __iommu_dma_map() regardless
>>>>> of cookie type, failures are being seen when mapping MSI target
>>>>> addresses for devices attached to UNMANAGED domains. To work around
>>>>> this issue, the iova_domain granule for IOMMU_DMA_MSI cookies is
>>>>> initialized to the value returned by cookie_msi_granule().
>>>>
>>>> Oh bum. Thanks for the report.
>>>>
>>>> However, I really don't like bodging around it with deliberate
>>>> undefined
>>>> behaviour. Fixing things properly doesn't seem too hard:
>>>
>>> I was not especially please with my solution, but I wanted to avoid
>>> potentially missing any other spots in the code where granule was
>>> used uninitialized. The compile time check made me feel a little
>>> less dirty about innappropriately using the iova_domain with MSI
>>> cookies.
>>>
>>>>
>>>> ----->8-----
>>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>>> index 8348f366ddd1..62618e77bedc 100644
>>>> --- a/drivers/iommu/dma-iommu.c
>>>> +++ b/drivers/iommu/dma-iommu.c
>>>> @@ -396,13 +396,13 @@ static void iommu_dma_free_iova(struct
>>>> iommu_dma_cookie *cookie,
>>>> dma_addr_t iova, size_t size)
>>>> {
>>>> struct iova_domain *iovad = &cookie->iovad;
>>>> - unsigned long shift = iova_shift(iovad);
>>>>
>>>> /* The MSI case is only ever cleaning up its most recent
>>>> allocation */
>>>> if (cookie->type == IOMMU_DMA_MSI_COOKIE)
>>>> cookie->msi_iova -= size;
>>>> else
>>>> - free_iova_fast(iovad, iova >> shift, size >> shift);
>>>> + free_iova_fast(iovad, iova_pfn(iovad, iova),
>>>> + size >> iova_shift(iovad));
>>>> }
>>>>
>>>> static void __iommu_dma_unmap(struct iommu_domain *domain,
>>>> dma_addr_t
>>>> dma_addr,
>>>> @@ -617,11 +617,14 @@ static dma_addr_t __iommu_dma_map(struct device
>>>> *dev, phys_addr_t phys,
>>>> {
>>>> struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>>>> struct iommu_dma_cookie *cookie = domain->iova_cookie;
>>>> - struct iova_domain *iovad = &cookie->iovad;
>>>> - size_t iova_off = iova_offset(iovad, phys);
>>>> + size_t iova_off = 0;
>>>> dma_addr_t iova;
>>>>
>>>> - size = iova_align(iovad, size + iova_off);
>>>> + if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
>>>> + iova_off = iova_offset(&cookie->iovad, phys);
>>>> + size = iova_align(&cookie->iovad, size + iova_off);
>>>> + }
>>>> +
>>>> iova = iommu_dma_alloc_iova(domain, size, dma_get_mask(dev),
>>>> dev);
>>>> if (!iova)
>>>> return DMA_ERROR_CODE;
>>>> -----8<-----
>>>>
>>>> Untested, and you'll probably want to double-check it anyway given that
>>>> the original oversight was mine in the first place ;)
>>>
>>> This looks good to me. As Shanker has already mentioned, it does fix the
>>> faults we were previously seeing with direct device assignment. I also
>>> verified that there aren't any other obvious cases of a granule == 0
>>> being used in the dma_iommu code by adding BUG_ON(!iovad->granule) to
>>> iova_{mask/align/offset/...} and running a variety of tests without
>>> issue.
>>>
>>> Are you going to post the patch?
>>
>> I also noticed PCIe passthrough/ARM is broken for me with 4.12-r1. I
>> tested Robin's patch as well, on Cavium ThunderX, and this fixes the
>> faults I have seen.
>>
>> Has anyone sent a formal patch?
>
> iommu/dma: Don't touch invalid iova_domain members
thanks!
Best Regards
Eric
>
>>
>> Thanks
>>
>> Eric
>>
>>>
>>>>
>>>> Robin.
>>>>
>>>>> Fixes: a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")
>>>>> Signed-off-by: Nate Watterson <nwatters-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>>>> ---
>>>>> drivers/iommu/dma-iommu.c | 10 ++++++++++
>>>>> 1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>>>> index 8348f366..d7b0816 100644
>>>>> --- a/drivers/iommu/dma-iommu.c
>>>>> +++ b/drivers/iommu/dma-iommu.c
>>>>> @@ -127,6 +127,16 @@ int iommu_get_msi_cookie(struct iommu_domain
>>>>> *domain, dma_addr_t base)
>>>>> cookie->msi_iova = base;
>>>>> domain->iova_cookie = cookie;
>>>>> +
>>>>> + /*
>>>>> + * Setup granule for compatibility with __iommu_dma_{alloc/free}
>>>>> and
>>>>> + * add a compile time check to ensure that writing granule won't
>>>>> + * clobber msi_iova.
>>>>> + */
>>>>> + cookie->iovad.granule = cookie_msi_granule(cookie);
>>>>> + BUILD_BUG_ON(offsetof(struct iova_domain, granule) <
>>>>> + sizeof(cookie->msi_iova));
>>>>> +
>>>>> return 0;
>>>>> }
>>>>> EXPORT_SYMBOL(iommu_get_msi_cookie);
>>>>>
>>>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread