iommu/dma: Setup iova_domain granule for IOMMU_DMA_MSI cookies
Subsystems (2)
PATCH 1/1 iommu/dma: Setup iova_domain granule for IOMMU_DMA_MSI cookies
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().
--- drivers/iommu/dma-iommu.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
drivers/iommu/dma-iommu.c +10 -0
@@ -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);
Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. on behalf of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Robin Murphy
maintainerHi Nate,
Quoted 9 lines · Nate Watterson
Currently, the __iommu_dma_{map/free} functions call iova_{offset/align} making them unsuitable for use with
iommu_domainshaving anIOMMU_DMA_MSIcookie since the cookie'siova_domainmember, 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, theiova_domaingranule forIOMMU_DMA_MSIcookies is initialized to the value returned bycookie_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-----
drivers/iommu/dma-iommu.c +0 -0
@@ -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_tdma_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.
Quoted 28 lines
Fixes a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")Signed-off-by Nate Watterson <[email protected]>--- drivers/iommu/dma-iommu.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/iommu/dma-iommu.c +10
-0@@ -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);
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);
+ }
Quoted 95 lines · Robin Murphy
Hi Nate,
Quoted 9 lines · Nate Watterson
Currently, the __iommu_dma_{map/free} functions call iova_{offset/align} making them unsuitable for use with
iommu_domainshaving anIOMMU_DMA_MSIcookie since the cookie'siova_domainmember, 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, theiova_domaingranule forIOMMU_DMA_MSIcookies is initialized to the value returned bycookie_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-----
drivers/iommu/dma-iommu.c +0
-1@@ -396,13 +396,13 @@ static void iommu_dma_free_iova(structiommu_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; elsedrivers/iommu/dma-iommu.c +2
-1-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,drivers/iommu/dma-iommu.c +6
-3@@ -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.
Quoted 28 lines
Fixes a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")Signed-off-by Nate Watterson <[email protected]>--- drivers/iommu/dma-iommu.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/iommu/dma-iommu.c +10
-0@@ -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);
Signature · 3 lines
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.
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
Quoted 158 lines
-----Original Message-----
From: [email protected] [mailto:iommu- [email protected]] On Behalf Of Shanker Donthineni
Sent: Thursday, April 13, 2017 8:00 PM
To: Robin Murphy <[email protected]>; Nate Watterson <[email protected]>; Joerg Roedel <[email protected]>; [email protected]; [email protected]
Subject: Re: [PATCH] iommu/dma: Setupiova_domaingranule forIOMMU_DMA_MSIcookiesHi 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.
(quoted diff) +2
-2@@ -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_tdma_addr, @@ -538,11 +538,14 @@ staticdma_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;(quoted diff) +5
-3- 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); + }Quoted 6 lines · Robin Murphy
Hi Nate,
Quoted 3 lines · Nate Watterson
Currently, the __iommu_dma_{map/free} functions call iova_{offset/align} making them unsuitable for use with
iommu_domainshaving anIOMMU_DMA_MSIcookie since the cookie'siova_domainmember, iovad, is uninitialized.
Quoted 2 lines
Quoted 2 lines
Now that
iommu_dma_get_msi_page()calls__iommu_dma_map()regardless
Quoted 2 lines
Quoted 2 lines
of cookie type, failures are being seen when mapping MSI target addresses for devices attached to UNMANAGED domains. To work
around
Quoted 23 lines
Quoted 2 lines
this issue, the
iova_domaingranule forIOMMU_DMA_MSIcookies is initialized to the value returned bycookie_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-----
drivers/iommu/dma-iommu.c +0
-1@@ -396,13 +396,13 @@ static void iommu_dma_free_iova(structiommu_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; elsedrivers/iommu/dma-iommu.c
Quoted 6 lines
drivers/iommu/dma-iommu.c +2
-1-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_tQuoted 6 lines
dma_addr, @@ -617,11 +617,14 @@ staticdma_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;drivers/iommu/dma-iommu.c +0
-2-struct iova_domain *iovad = &cookie->iovad; -size_t iova_off =iova_offset (iovad, phys);drivers/iommu/dma-iommu.c
Quoted 10 lines
drivers/iommu/dma-iommu.c +6
-1+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);
Quoted 15 lines
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.
Quoted 6 lines
Fixes a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")Signed-off-by Nate Watterson <[email protected]>--- drivers/iommu/dma-iommu.c | 10 ++++++++++ 1 file changed, 10 insertions(+)Quoted 5 lines
drivers/iommu/dma-iommu.c int iommu_get_msi_cookie(struct
Quoted 5 lines
drivers/iommu/dma-iommu.c +0
-0@@ -127,6 +127,16 @@ int iommu_get_msi_cookie(struct
iommu_domainQuoted 7 lines
drivers/iommu/dma-iommu.c int iommu_get_msi_cookie(struct · line 133
Quoted 7 lines
drivers/iommu/dma-iommu.c +3
-0*domain, dma_addr_t base) cookie->msi_iova = base; domain->iova_cookie = cookie; + +/* +* Setup granule for compatibility with __iommu_dma_{alloc/free} and
Quoted 11 lines
Quoted 11 lines
drivers/iommu/dma-iommu.c +7
-0+* 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.
Quoted 4 lines
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Hi Robin,
Quoted 13 lines · Robin Murphy
Hi Nate,
Quoted 9 lines · Nate Watterson
Currently, the __iommu_dma_{map/free} functions call iova_{offset/align} making them unsuitable for use with
iommu_domainshaving anIOMMU_DMA_MSIcookie since the cookie'siova_domainmember, 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, theiova_domaingranule forIOMMU_DMA_MSIcookies is initialized to the value returned bycookie_msi_granule().
Quoted 4 lines
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.
Quoted 44 lines
----->8-----
drivers/iommu/dma-iommu.c +0
-1@@ -396,13 +396,13 @@ static void iommu_dma_free_iova(structiommu_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; elsedrivers/iommu/dma-iommu.c +2
-1-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,drivers/iommu/dma-iommu.c +6
-3@@ -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;
Quoted 4 lines
-----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?
Quoted 32 lines
Robin.
Quoted 28 lines
Fixes a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")Signed-off-by Nate Watterson <[email protected]>--- drivers/iommu/dma-iommu.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/iommu/dma-iommu.c +10
-0@@ -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);
Signature · 2 lines
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.
Hi,
Quoted 85 lines · Nate Watterson
Hi Robin,
Quoted 14 lines · Robin Murphy
Hi Nate,
Quoted 10 lines · Nate Watterson
Currently, the __iommu_dma_{map/free} functions call iova_{offset/align} making them unsuitable for use with
iommu_domainshaving anIOMMU_DMA_MSI
cookie since the cookie'siova_domainmember, 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, theiova_domaingranule forIOMMU_DMA_MSIcookies is initialized to the value returned bycookie_msi_granule().Quoted 4 lines
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_domainwith MSI cookies.Quoted 45 lines
----->8-----
drivers/iommu/dma-iommu.c +0
-1@@ -396,13 +396,13 @@ static void iommu_dma_free_iova(structiommu_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; elsedrivers/iommu/dma-iommu.c +2
-1-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,drivers/iommu/dma-iommu.c +6
-3@@ -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;Quoted 4 lines
-----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_iommucode by addingBUG_ON(!iovad->granule)to iova_{mask/align/offset/...} and running a variety of tests without issue.
Quoted 1 line
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
Quoted 34 lines
Quoted 34 lines
Robin.
Quoted 29 lines
Fixes a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")Signed-off-by Nate Watterson <[email protected]>--- drivers/iommu/dma-iommu.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/iommu/dma-iommu.c +3
-0@@ -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
drivers/iommu/dma-iommu.c +7
-0+* 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);
Quoted 88 lines · Auger Eric
Hi,
Quoted 85 lines · Nate Watterson
Hi Robin,
Quoted 14 lines · Robin Murphy
Hi Nate,
Quoted 10 lines · Nate Watterson
Currently, the __iommu_dma_{map/free} functions call iova_{offset/align} making them unsuitable for use with
iommu_domainshaving anIOMMU_DMA_MSI
cookie since the cookie'siova_domainmember, 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, theiova_domaingranule forIOMMU_DMA_MSIcookies is initialized to the value returned bycookie_msi_granule().Quoted 4 lines
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_domainwith MSI cookies.Quoted 45 lines
----->8-----
drivers/iommu/dma-iommu.c +0
-1@@ -396,13 +396,13 @@ static void iommu_dma_free_iova(structiommu_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; elsedrivers/iommu/dma-iommu.c +2
-1-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,drivers/iommu/dma-iommu.c +6
-3@@ -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;Quoted 4 lines
-----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_iommucode by addingBUG_ON(!iovad->granule)to iova_{mask/align/offset/...} and running a variety of tests without issue.
Quoted 7 lines
Quoted 1 line
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
Quoted 39 lines
Thanks
Eric
Quoted 34 lines
Quoted 34 lines
Robin.
Quoted 29 lines
Fixes a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")Signed-off-by Nate Watterson <[email protected]>--- drivers/iommu/dma-iommu.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/iommu/dma-iommu.c +3
-0@@ -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
drivers/iommu/dma-iommu.c +7
-0+* 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);
Signature · 2 lines
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.
Hi Nate,
Quoted 96 lines · Nate Watterson
Quoted 93 lines · Auger Eric
Hi,
Quoted 89 lines · Nate Watterson
Hi Robin,
Quoted 15 lines · Robin Murphy
Hi Nate,
Quoted 11 lines · Nate Watterson
Currently, the __iommu_dma_{map/free} functions call iova_{offset/align}
making them unsuitable for use withiommu_domainshaving anIOMMU_DMA_MSI
cookie since the cookie'siova_domainmember, 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, theiova_domaingranule forIOMMU_DMA_MSIcookies is initialized to the value returned bycookie_msi_granule().Quoted 5 lines
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_domainwith MSI cookies.Quoted 46 lines
----->8-----
drivers/iommu/dma-iommu.c +0
-1@@ -396,13 +396,13 @@ static void iommu_dma_free_iova(structiommu_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; elsedrivers/iommu/dma-iommu.c +2
-1-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_tdma_addr,drivers/iommu/dma-iommu.c +6
-3@@ -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;Quoted 4 lines
-----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_iommucode by addingBUG_ON(!iovad->granule)to iova_{mask/align/offset/...} and running a variety of tests without issue.Are you going to post the patch?
Quoted 7 lines
Quoted 5 lines
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_domainmembers
thanks!
Best Regards
Eric
Quoted 41 lines
Quoted 40 lines
Thanks
Eric
Quoted 34 lines
Quoted 34 lines
Robin.
Quoted 29 lines
Fixes a44e6657585b ("iommu/dma: Clean up MSI IOVA allocation")Signed-off-by Nate Watterson <[email protected]>--- drivers/iommu/dma-iommu.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/iommu/dma-iommu.c +3
-0@@ -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
drivers/iommu/dma-iommu.c +7
-0+* 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);