Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1733214 > unrolled thread

DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

Started byHarsh Jain <Harsh@chelsio.com>
First post2017-09-16 08:20 +0200
Last post2017-09-26 13:20 +0200
Articles 19 on this page of 39 — 8 participants

Back to article view | Back to linux.kernel

Contents

  DMA error when sg->offset value is greater than PAGE_SIZE in Intel  IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-16 08:20 +0200
    Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Herbert Xu <herbert@gondor.apana.org.au> - 2017-09-20 10:10 +0200
      Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Robin Murphy <robin.murphy@arm.com> - 2017-09-20 12:20 +0200
        Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-20 13:30 +0200
        Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-25 19:50 +0200
          Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU "Raj, Ashok" <ashok.raj@intel.com> - 2017-09-25 20:50 +0200
            Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-25 20:50 +0200
              Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-26 05:50 +0200
                Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-26 14:30 +0200
                  Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Robin Murphy <robin.murphy@arm.com> - 2017-09-26 16:30 +0200
                    Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Dan Williams <dan.j.williams@intel.com> - 2017-09-26 18:20 +0200
                      Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-27 18:40 +0200
                        Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Dan Williams <dan.j.williams@intel.com> - 2017-09-27 19:20 +0200
                          Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Christoph Hellwig <hch@infradead.org> - 2017-10-01 11:00 +0200
                        Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU "Raj, Ashok" <ashok.raj@intel.com> - 2017-09-27 19:50 +0200
                          Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-27 23:30 +0200
                            Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU "Raj, Ashok" <ashok.raj@intel.com> - 2017-09-28 00:10 +0200
                              Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-28 00:20 +0200
                                Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-28 07:10 +0200
                                Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Herbert Xu <herbert@gondor.apana.org.au> - 2017-09-28 12:40 +0200
                            Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-28 15:40 +0200
                              Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU "Raj, Ashok" <ashok.raj@intel.com> - 2017-09-28 18:10 +0200
                                Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-29 07:40 +0200
                      Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-27 19:40 +0200
                    Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU "Raj, Ashok" <ashok.raj@intel.com> - 2017-09-26 19:30 +0200
                      Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-26 23:00 +0200
                    Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU "Raj, Ashok" <ashok.raj@intel.com> - 2017-09-26 19:30 +0200
                      Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Robin Murphy <robin.murphy@arm.com> - 2017-09-26 20:20 +0200
                    Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-26 19:40 +0200
          Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Dan Williams <dan.j.williams@intel.com> - 2017-09-25 21:40 +0200
            Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-25 22:10 +0200
              Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Dan Williams <dan.j.williams@intel.com> - 2017-09-25 22:20 +0200
                Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU "Raj, Ashok" <ashok.raj@intel.com> - 2017-09-25 23:50 +0200
                  Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-26 01:50 +0200
                Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-26 15:10 +0200
      Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-20 13:40 +0200
      Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU David Woodhouse <dwmw2@infradead.org> - 2017-09-25 20:50 +0200
        Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Casey Leedom <leedom@chelsio.com> - 2017-09-25 22:20 +0200
        Re: DMA error when sg->offset value is greater than PAGE_SIZE in  Intel IOMMU Harsh Jain <Harsh@chelsio.com> - 2017-09-26 13:20 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1741563 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

FromHarsh Jain <Harsh@chelsio.com>
Date2017-09-28 15:40 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<uuHcK-17h-17@gated-at.bofh.it>
In reply to#1741031

On 28-09-2017 02:59, Casey Leedom wrote:
> Hey Raj,
>
>   Let us know if you need help in gathering more debugging information.  For
> the time being we've decided to ERRATA the use of the Intel I/O MMU with
> IPsec till we Root Cause the issue.  But this is still at the top of Harsh's
> bug list.
>  
>   With Robin's comments, I'm almost sure that the:
>
>     (iov_pfn + sg->offset) << VTD_PAGE_SHIFT)
>
> in your suggested patch is an issue.  iov_pfn is a Page Frame Number and
> sg->offset is a Byte Offset.  It feels like this should be:
>
>     size_t page_off = sg->offset & ~VTD_PAGE_MASK;
>     unsigned long pfn_off = sg->offset >> VTD_PAGE_MASK;
>     ...
>     sg->dma_address = ((dma_addr_t)
>                        (iov_pfn + pfn_off) << VTD_PAGE_SHIFT) + page_off;
>
> When Harsh tried your original patch, Harsh' test system wouldn't even boot.
Today I tried with "Intel_iommu=sp_off" boot option. Traffic runs without any error for more than 1 hrs. What magic this option did? :)
>
> Casey

[toc] | [prev] | [next] | [standalone]

#1741682 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

From"Raj, Ashok" <ashok.raj@intel.com>
Date2017-09-28 18:10 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<uuJxU-2Gx-23@gated-at.bofh.it>
In reply to#1741563
Thanks for trying that Harsh. 

sp_off turns of super page support. Which this mode, do you still see offsets greater than 4k?

On Thu, Sep 28, 2017 at 07:08:21PM +0530, Harsh Jain wrote:
> 
> 
> Today I tried with "Intel_iommu=sp_off" boot option. Traffic runs without any error for more than 1 hrs. What magic this option did? :)

Cheers,
Ashok

[toc] | [prev] | [next] | [standalone]

#1741927 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

FromHarsh Jain <Harsh@chelsio.com>
Date2017-09-29 07:40 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<uuWbL-24g-9@gated-at.bofh.it>
In reply to#1741682
On 28-09-2017 18:35, Raj, Ashok wrote:
> Thanks for trying that Harsh. 
>
> sp_off turns of super page support. Which this mode, do you still see offsets greater than 4k?
Yes, offset greater than 4k is still there. Refer below.
[56732.774872] offset 4110 len 76 dma addr 3a531200e dma len 76
[56732.804187] offset 4110 len 84 dma addr 3a63b200e dma len 84
[56732.805104] offset 4110 len 68 dma addr 3a531200e dma len 68
[56732.806870] offset 4110 len 56 dma addr 3a531200e dma len 56
[56732.808987] offset 4110 len 56 dma addr 3a531200e dma len 56
[56732.811215] offset 4110 len 56 dma addr 3a531200e dma len 56
[56732.813155] offset 4110 len 56 dma addr 3a531200e dma len 56
[56732.814823] offset 4110 len 56 dma addr 3a531200e dma len 56
[56732.816481] offset 4110 len 56 dma addr 3a531200e dma len 56
[56732.818159] offset 4110 len 56 dma addr 3a531200e dma len 56
[56732.819712] offset 4110 len 56 dma addr 3a531200e dma len 56
[56732.821629] offset 4110 len 56 dma addr 3a531200e dma len 56
[root@heptagon linux_t4_build]#
[root@heptagon linux_t4_build]#
[root@heptagon linux_t4_build]# cat /proc/cmdline
BOOT_IMAGE=/vmlinuz-4.9.51 root=UUID=ccbb7f18-b3f0-43df-89de-07521e9c02fe ro intel_iommu=sp_off crashkernel=auto rhgb quiet rhgb quiet console=ttyS0,115200, console=tty0 LANG=en_US.UTF-8

>
> On Thu, Sep 28, 2017 at 07:08:21PM +0530, Harsh Jain wrote:
>>
>> Today I tried with "Intel_iommu=sp_off" boot option. Traffic runs without any error for more than 1 hrs. What magic this option did? :)
> Cheers,
> Ashok

[toc] | [prev] | [next] | [standalone]

#1740914 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

FromCasey Leedom <leedom@chelsio.com>
Date2017-09-27 19:40 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<uuots-68f-5@gated-at.bofh.it>
In reply to#1740004
| From: Robin Murphy <robin.murphy@arm.com>
| Sent: Wednesday, September 27, 2017 10:18 AM
| 
| From my experience, in general terms each scatterlist segment
| represents some contiguous quantity of pages, of which sg->page is the
| first, while sg->length and sg->offset describe the specific bounds of
| that segment's data. ...

  Okay, thanks Robin.  That'll help me in my reviews of your and Ashok's
suggested changes to the Intel I/O MMU __domain_mapping() routine.

Casey

[toc] | [prev] | [next] | [standalone]

#1740046 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

From"Raj, Ashok" <ashok.raj@intel.com>
Date2017-09-26 19:30 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<uu1Qd-7HQ-1@gated-at.bofh.it>
In reply to#1739936
Oops..minor typo.. VTD_PAGE_SHIFT instead of VTD_PAGE_MASK


On Tue, Sep 26, 2017 at 07:34:41AM -0700, Ashok Raj wrote:
> On Tue, Sep 26, 2017 at 03:22:47PM +0100, Robin Murphy wrote:
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index 6784a05dd6b2..d7f7def81613 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -2254,10 +2254,12 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
> >  		uint64_t tmp;
> >  
> >  		if (!sg_res) {
> > +			size_t off = sg->offset & ~PAGE_MASK;
> 
> Should this be VTD_PAGE_MASK?
> 
> > +
> >  			sg_res = aligned_nrpages(sg->offset, sg->length);
> > -			sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + sg->offset;
> > +			sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + off;
> >  			sg->dma_length = sg->length;
> > -			pteval = page_to_phys(sg_page(sg)) | prot;
> > +			pteval = (page_to_phys(sg_page(sg)) + sg->offset - off) | prot;
> 
> Something seems wrong here.. sg->offset can be > VTD_PAGE_SIZE, think 
> we should add sg->offset and then find the pteval?
> 
> attached below another cut at fixing the same problem.. if there is something
> obvious i missed, let me know.
> 
> again.. untested :-)
> 
> Cheers,
> Ashok
> 

> Sometimes offset can be greater than 4K. vt-d needs to account for that.
> 
> From: Ashok Raj <ashok.raj@intel.com>
> 
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> ---
>  drivers/iommu/intel-iommu.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 6784a05..d43b566 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2254,10 +2254,13 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
>  		uint64_t tmp;
>  
>  		if (!sg_res) {
> +			size_t off = sg->offset & ~VTD_PAGE_SHIFT;
>  			sg_res = aligned_nrpages(sg->offset, sg->length);
> -			sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + sg->offset;
> +			sg->dma_address = ((dma_addr_t)
> +				(iov_pfn + sg->offset) << VTD_PAGE_SHIFT) + off;
>  			sg->dma_length = sg->length;
> -			pteval = page_to_phys(sg_page(sg)) | prot;
> +			pteval = (page_to_phys(sg_page(sg)) +
> +				(sg->offset << VTD_PAGE_SHIFT)) | prot;
>  			phys_pfn = pteval >> VTD_PAGE_SHIFT;
>  		}
>  


Sometimes offset can be greater than 4K. vt-d needs to account for that.

From: Ashok Raj <ashok.raj@intel.com>

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 drivers/iommu/intel-iommu.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 6784a05..0333afe 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2254,10 +2254,13 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
 		uint64_t tmp;
 
 		if (!sg_res) {
+			size_t off = sg->offset & ~VTD_PAGE_MASK;
 			sg_res = aligned_nrpages(sg->offset, sg->length);
-			sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + sg->offset;
+			sg->dma_address = ((dma_addr_t)
+				(iov_pfn + sg->offset) << VTD_PAGE_SHIFT) + off;
 			sg->dma_length = sg->length;
-			pteval = page_to_phys(sg_page(sg)) | prot;
+			pteval = (page_to_phys(sg_page(sg)) +
+				(sg->offset << VTD_PAGE_SHIFT)) | prot;
 			phys_pfn = pteval >> VTD_PAGE_SHIFT;
 		}

[toc] | [prev] | [next] | [standalone]

#1740200 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

FromCasey Leedom <leedom@chelsio.com>
Date2017-09-26 23:00 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<uu57t-1fQ-53@gated-at.bofh.it>
In reply to#1740046
  So just to be 100% sure I understand the patch you're proposing, you got
the first use of VTD_PAGE_SHIFT wrong; it should have been VTD_PAGE_MASK?  I.e.

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 6784a05..d43b566 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2254,10 +2254,13 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
                uint64_t tmp;

                if (!sg_res) {
+                       size_t off = sg->offset & ~VTD_PAGE_MASK;
                        sg_res = aligned_nrpages(sg->offset, sg->length);
-                       sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + sg->offset;
+                       sg->dma_address = ((dma_addr_t)
+                               (iov_pfn + sg->offset) << VTD_PAGE_SHIFT) + off;
                        sg->dma_length = sg->length;
-                       pteval = page_to_phys(sg_page(sg)) | prot;
+                       pteval = (page_to_phys(sg_page(sg)) +
+                               (sg->offset << VTD_PAGE_SHIFT)) | prot;
                        phys_pfn = pteval >> VTD_PAGE_SHIFT;
                }

???  And I'm still confused about this portion:

-                       sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + sg->offset;
+                       sg->dma_address = ((dma_addr_t)
+                               (iov_pfn + sg->offset) << VTD_PAGE_SHIFT) + off;
 
Isn't iov_pfn a Physical Page Number and we're adding a Byte Offset
to that?  I would have though that it would be more like:

    size_t page_off = sg->offset & ~VTD_PAGE_MASK;
    unsigned long pfn_off = sg->offset >> VTD_PAGE_MASK;
    ...
    sg->dma_address = ((dma_addr_t)
                       (iov_pfn + pfn_off) << VTD_PAGE_SHIFT) + page_off;

  I want to be sure that Harsh has a concrete patch to work with when he
wakes up.

  How about it Robin, Dan, David, Herbert, what do you guys think of Raj's
proposed patch?

Casey

[toc] | [prev] | [next] | [standalone]

#1740048 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

From"Raj, Ashok" <ashok.raj@intel.com>
Date2017-09-26 19:30 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<uu1Qd-7HQ-3@gated-at.bofh.it>
In reply to#1739936

[Multipart message — attachments visible in raw view] — view raw

On Tue, Sep 26, 2017 at 03:22:47PM +0100, Robin Murphy wrote:
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 6784a05dd6b2..d7f7def81613 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2254,10 +2254,12 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
>  		uint64_t tmp;
>  
>  		if (!sg_res) {
> +			size_t off = sg->offset & ~PAGE_MASK;

Should this be VTD_PAGE_MASK?

> +
>  			sg_res = aligned_nrpages(sg->offset, sg->length);
> -			sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + sg->offset;
> +			sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + off;
>  			sg->dma_length = sg->length;
> -			pteval = page_to_phys(sg_page(sg)) | prot;
> +			pteval = (page_to_phys(sg_page(sg)) + sg->offset - off) | prot;

Something seems wrong here.. sg->offset can be > VTD_PAGE_SIZE, think 
we should add sg->offset and then find the pteval?

attached below another cut at fixing the same problem.. if there is something
obvious i missed, let me know.

again.. untested :-)

Cheers,
Ashok

[toc] | [prev] | [next] | [standalone]

#1740092 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

FromRobin Murphy <robin.murphy@arm.com>
Date2017-09-26 20:20 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<uu2CB-8fI-1@gated-at.bofh.it>
In reply to#1740048
On 26/09/17 15:34, Raj, Ashok wrote:
> On Tue, Sep 26, 2017 at 03:22:47PM +0100, Robin Murphy wrote:
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index 6784a05dd6b2..d7f7def81613 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -2254,10 +2254,12 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
>>  		uint64_t tmp;
>>  
>>  		if (!sg_res) {
>> +			size_t off = sg->offset & ~PAGE_MASK;
> 
> Should this be VTD_PAGE_MASK?

PAGE_MASK (and the corresponding pteval arithmetic) was intentional
here; given the way aligned_nrpages() works, the IOVA space allocated in
intel_map_sg() (and thus iov_pfn) is already rounded to full MM pages,
and it seemed like the original intent was to map the whole lot - this
change is just to make that happen correctly.

Whether it's actually reasonable to decouple the IOMMU and CPU page
sizes entirely (as we do in dma-iommu, for example), and not do the
MM-page-alignment thing at all, is another matter that I'm happy to
leave in your hands :)

Robin.

>> +
>>  			sg_res = aligned_nrpages(sg->offset, sg->length);
>> -			sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + sg->offset;
>> +			sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + off;
>>  			sg->dma_length = sg->length;
>> -			pteval = page_to_phys(sg_page(sg)) | prot;
>> +			pteval = (page_to_phys(sg_page(sg)) + sg->offset - off) | prot;
> 
> Something seems wrong here.. sg->offset can be > VTD_PAGE_SIZE, think 
> we should add sg->offset and then find the pteval?
> 
> attached below another cut at fixing the same problem.. if there is something
> obvious i missed, let me know.
> 
> again.. untested :-)
> 
> Cheers,
> Ashok
>

[toc] | [prev] | [next] | [standalone]

#1740051 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

FromCasey Leedom <leedom@chelsio.com>
Date2017-09-26 19:40 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<uu1ZU-7KQ-5@gated-at.bofh.it>
In reply to#1739936
| From: Robin Murphy <robin.murphy@arm.com>
| Sent: Tuesday, September 26, 2017 7:22 AM
|
| On 26/09/17 13:21, Harsh Jain wrote:
| > Find attached new set of log. After repeated tries it panics.
|
| Thanks, that makes things a bit clearer - looks like fixing the physical
| address/pteval calculation to not be off by a page in one direction wasn't
| helping much because the returned DMA address is actually also off by a
| page in the other direction, and thus overflowing past the allocated IOVA
| into whoever else's mapping happened to be there; complete carnage ensues.
|
| After another look through the intel_map_sg() path, here's my second (still
| completely untested) guess at a possible fix.
|
| Robin.
|
| ----->8-----
| diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
| index 6784a05dd6b2..d7f7def81613 100644
| --- a/drivers/iommu/intel-iommu.c
| +++ b/drivers/iommu/intel-iommu.c
| @@ -2254,10 +2254,12 @@ static int __domain_mapping(struct dmar_domain *doma!
|                  uint64_t tmp;
|
|                  if (!sg_res) {
| +                       size_t off = sg->offset & ~PAGE_MASK;
| +
|                          sg_res = aligned_nrpages(sg->offset, sg->length);
| -                       sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_S!
| +                       sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_S!
|                          sg->dma_length = sg->length;
| -                       pteval = page_to_phys(sg_page(sg)) | prot;
| +                       pteval = (page_to_phys(sg_page(sg)) + sg->offset - o!
|                          phys_pfn = pteval >> VTD_PAGE_SHIFT;
|                  }

Robin,

  Harsh was able to do an initial test of you proposed patch above and his
test setup survived for 2 minutes with iperf traffic before his PEER went
belly up.  Since his PEER isn't on a Remote Power IPMI, we'll have to wait
till tomorrow for further tests -- Harsh does have to sleep some time ...
:-) But the 2 minutes his test machine did survive were Very Promising!
 
  And I see that Raj has also sent a couple of proposed fixes now.
It's just as well that Harsh is off to bed now so we can hash things out
while he sleeps ...

Casey

[toc] | [prev] | [next] | [standalone]

#1739242 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

FromDan Williams <dan.j.williams@intel.com>
Date2017-09-25 21:40 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<utHot-2k6-17@gated-at.bofh.it>
In reply to#1739194
On Mon, Sep 25, 2017 at 10:46 AM, Casey Leedom <leedom@chelsio.com> wrote:
> | From: Robin Murphy <robin.murphy@arm.com>
> | Sent: Wednesday, September 20, 2017 3:12 AM
> |
> | On 20/09/17 09:01, Herbert Xu wrote:
> | >
> | > Harsh Jain <Harsh@chelsio.com> wrote:
> | >>
> | >> While debugging DMA mapping error in chelsio crypto driver we
> | >> observed that when scatter/gather list received by driver has
> | >> some entry with page->offset > 4096 (PAGE_SIZE). It starts
> | >> giving DMA error.  Without IOMMU it works fine.
> | >
> | > This is not a bug.  The network stack can and will feed us such
> | > SG lists.
> | >
> | >> 2) It cannot be driver's responsibilty to update received sg
> | >> entries to adjust offset and page because we are not the only
> | >> one who directly uses received sg list.
> | >
> | > No the driver must deal with this.  Having said that, if we can
> | > improve our driver helper interface to make this easier then we
> | > should do that too.  What we certainly shouldn't do is to take a
> | > whack-a-mole approach like this patch does.
> |
> | AFAICS this is entirely on intel-iommu - from a brief look it appears
> | that all the IOVA calculations would handle the offset correctly, but
> | then __domain_mapping() blindly uses sg_page() for the physical address,
> | so if offset is larger than a page it would end up with the DMA mapping
> | covering the wrong part of the buffer.
> |
> | Does the diff below help?
> |
> | Robin.
> |
> | ----->8-----
> | diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> | index b3914fce8254..2ed43d928135 100644
> | --- a/drivers/iommu/intel-iommu.c
> | +++ b/drivers/iommu/intel-iommu.c
> | @@ -2253,7 +2253,7 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
> |                          sg_res = aligned_nrpages(sg->offset, sg->length);
> |                          sg->dma_address = ((dma_addr_t)iov_pfn << VTD_PAGE_SHIFT) + sg->offset;
> |                          sg->dma_length = sg->length;
> | -                       pteval = page_to_phys(sg_page(sg)) | prot;
> | +                       pteval = (sg_phys(sg) & PAGE_MASK) | prot;

This breaks on platforms where sizeof(phys_addr_t) > sizeof(unsigned
long). I.e. it's not always safe to assume that PAGE_MASK is the
correct width.

> |                          phys_pfn = pteval >> VTD_PAGE_SHIFT;
> |                  }
>
>   Adding some likely people to the Cc list so they can comment on this.
> Dan Williams submitted that specific piece of code in kernel.org:3e6110fd54
> ... but there are lots of similar bits in that function.  Hopefully one of
> the Intel I/O MMU Gurus will have a better idea of what may be going wrong
> here.  In the mean time I've asked our team to gather far more detailed
> debug traces showing the exact Scatter/Gather Lists we're getting, what they
> get translated to in the DMA Mappings, and what DMA Addresses were seeing in
> error.

IIUC it looks like this has been broken ever since commit e1605495c716
"intel-iommu: Introduce domain_sg_mapping() to speed up
intel_map_sg()". I.e. it looks like the calculation for pte_val should
be:

    pteval = (page_to_phys(sg_page(sg)) + sg->offset) | prot;

[toc] | [prev] | [next] | [standalone]

#1739257 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

FromCasey Leedom <leedom@chelsio.com>
Date2017-09-25 22:10 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<utHRx-2MV-15@gated-at.bofh.it>
In reply to#1739242
| From: Dan Williams <dan.j.williams@intel.com>
| Sent: Monday, September 25, 2017 12:31 PM
| ...
| IIUC it looks like this has been broken ever since commit e1605495c716
| "intel-iommu: Introduce domain_sg_mapping() to speed up
| intel_map_sg()". I.e. it looks like the calculation for pte_val should
| be:
| 
|     pteval = (page_to_phys(sg_page(sg)) + sg->offset) | prot;

Hhmmm, shouldn't that be:

    pteval = (page_to_phys(sg_page(sg)) + (sg->offset>>PAGE_SHIFT)) | prot;

???

Casey

[toc] | [prev] | [next] | [standalone]

#1739262 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

FromDan Williams <dan.j.williams@intel.com>
Date2017-09-25 22:20 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<utI1c-2RB-15@gated-at.bofh.it>
In reply to#1739257
On Mon, Sep 25, 2017 at 1:05 PM, Casey Leedom <leedom@chelsio.com> wrote:
> | From: Dan Williams <dan.j.williams@intel.com>
> | Sent: Monday, September 25, 2017 12:31 PM
> | ...
> | IIUC it looks like this has been broken ever since commit e1605495c716
> | "intel-iommu: Introduce domain_sg_mapping() to speed up
> | intel_map_sg()". I.e. it looks like the calculation for pte_val should
> | be:
> |
> |     pteval = (page_to_phys(sg_page(sg)) + sg->offset) | prot;
>
> Hhmmm, shouldn't that be:
>
>     pteval = (page_to_phys(sg_page(sg)) + (sg->offset>>PAGE_SHIFT)) | prot;

Yes, I think you're right. We do want to mask off the page-unaligned
portion of sg->offset.

[toc] | [prev] | [next] | [standalone]

#1739292 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

From"Raj, Ashok" <ashok.raj@intel.com>
Date2017-09-25 23:50 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<utJqi-3Hi-21@gated-at.bofh.it>
In reply to#1739262
Hi

On Mon, Sep 25, 2017 at 01:11:04PM -0700, Dan Williams wrote:
> On Mon, Sep 25, 2017 at 1:05 PM, Casey Leedom <leedom@chelsio.com> wrote:
> > | From: Dan Williams <dan.j.williams@intel.com>
> > | Sent: Monday, September 25, 2017 12:31 PM
> > | ...
> > | IIUC it looks like this has been broken ever since commit e1605495c716
> > | "intel-iommu: Introduce domain_sg_mapping() to speed up
> > | intel_map_sg()". I.e. it looks like the calculation for pte_val should
> > | be:
> > |
> > |     pteval = (page_to_phys(sg_page(sg)) + sg->offset) | prot;
> >
> > Hhmmm, shouldn't that be:
> >
> >     pteval = (page_to_phys(sg_page(sg)) + (sg->offset>>PAGE_SHIFT)) | prot;
> 
> Yes, I think you're right. We do want to mask off the page-unaligned
> portion of sg->offset.

Shoulnd't we normalize the entire sg_page(sg) + sg_offset.

if when you only mask the page-unaligned portion i suspect you might be
pointing to a different region?

something like (sg_page(sg) + (sg->offset << VTD_PAGE_SHIFT)) 

then add the unaligned part.. sg->offset>>VTD_PAGE_SHIFT

Is this happening because you are using a 2M page? not sure what triggers
this or causes the driver to get passed in larger than 4K offset, or 
running 32bit kernel?

if its legal to get passed in such odd values, we should fix IOMMU driver to 
handle it properly, otherwise we should atleast fail those requests.

Cheers,
Ashok

[toc] | [prev] | [next] | [standalone]

#1739369 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

FromCasey Leedom <leedom@chelsio.com>
Date2017-09-26 01:50 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<utLiq-4Yh-19@gated-at.bofh.it>
In reply to#1739292
| From: Raj, Ashok <ashok.raj@intel.com>
| Sent: Monday, September 25, 2017 12:03 PM
|   
| On Mon, Sep 25, 2017 at 01:11:04PM -0700, Dan Williams wrote:
| > On Mon, Sep 25, 2017 at 1:05 PM, Casey Leedom <leedom@chelsio.com> wrote:
| > > | From: Dan Williams <dan.j.williams@intel.com>
| > > | Sent: Monday, September 25, 2017 12:31 PM
| > > | ...
| > > | IIUC it looks like this has been broken ever since commit e1605495c716
| > > | "intel-iommu: Introduce domain_sg_mapping() to speed up
| > > | intel_map_sg()". I.e. it looks like the calculation for pte_val should
| > > | be:
| > > |
| > > |     pteval = (page_to_phys(sg_page(sg)) + sg->offset) | prot;
| > >
| > > Hhmmm, shouldn't that be:
| > >
| > >     pteval = (page_to_phys(sg_page(sg)) + (sg->offset>>PAGE_SHIFT)) | prot;
| >
| > Yes, I think you're right. We do want to mask off the page-unaligned
| > portion of sg->offset.
|
| Shoulnd't we normalize the entire sg_page(sg) + sg_offset.
|
| if when you only mask the page-unaligned portion i suspect you might be
| pointing to a different region?
|
| something like (sg_page(sg) + (sg->offset << VTD_PAGE_SHIFT))
|
| then add the unaligned part.. sg->offset>>VTD_PAGE_SHIFT
|
| Is this happening because you are using a 2M page? not sure what triggers
| this or causes the driver to get passed in larger than 4K offset, or
| running 32bit kernel?
|
| if its legal to get passed in such odd values, we should fix IOMMU driver to
| handle it properly, otherwise we should atleast fail those requests.

  (woof) This is all above me.  I've spent a chunk of time fruitlessly
trying to find documentation which says that scatterlist's are allowed to
have offset/length values which extend outside the sg_page(sg).  So someone
much more familiar with this stuff is going to need to say what's allowed.

  As I said, I've asked Harsh to provide us with a detailed trace of exactly
what he's seeing and what the Scatter/Gather Lists are getting translated
into.  That information may make it easier to understand if/how
__domain_mapping() is screwing up ...

Casey

[toc] | [prev] | [next] | [standalone]

#1739870 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

FromHarsh Jain <Harsh@chelsio.com>
Date2017-09-26 15:10 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<utXMB-5dg-5@gated-at.bofh.it>
In reply to#1739262

On 26-09-2017 01:41, Dan Williams wrote:
> On Mon, Sep 25, 2017 at 1:05 PM, Casey Leedom <leedom@chelsio.com> wrote:
>> | From: Dan Williams <dan.j.williams@intel.com>
>> | Sent: Monday, September 25, 2017 12:31 PM
>> | ...
>> | IIUC it looks like this has been broken ever since commit e1605495c716
>> | "intel-iommu: Introduce domain_sg_mapping() to speed up
>> | intel_map_sg()". I.e. it looks like the calculation for pte_val should
>> | be:
>> |
>> |     pteval = (page_to_phys(sg_page(sg)) + sg->offset) | prot;
>>
>> Hhmmm, shouldn't that be:
>>
>>     pteval = (page_to_phys(sg_page(sg)) + (sg->offset>>PAGE_SHIFT)) | prot;
> Yes, I think you're right. We do want to mask off the page-unaligned
> portion of sg->offset.
Tried changing above line in "__domain_mapping" but didn't help.

[toc] | [prev] | [next] | [standalone]

#1735732 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

FromHarsh Jain <Harsh@chelsio.com>
Date2017-09-20 13:40 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<urLwe-2qd-13@gated-at.bofh.it>
In reply to#1735611

On 20-09-2017 13:31, Herbert Xu wrote:
> Harsh Jain <Harsh@chelsio.com> wrote:
>> While debugging DMA mapping error in chelsio crypto driver we observed that when scatter/gather list received by driver has some entry with page->offset > 4096 (PAGE_SIZE). It starts giving DMA error.  Without IOMMU it works fine.
> This is not a bug.  The network stack can and will feed us such
> SG lists.
>
>> 2) It cannot be driver's responsibilty to update received sg entries to adjust offset and page 
>> because we are not the only one who directly uses received sg list.
> No the driver must deal with this.  Having said that, if we can
> improve our driver helper interface to make this easier then we
> should do that too.  What we certainly shouldn't do is to take a
> whack-a-mole approach like this patch does.
Agreed,I added that patch for understanding purpose only. Today I referred other crypto driver for DMA related code. Most of them are using dma_map_sg except QAT. In QAT, They are first updating the Page address using offset then mapping each page in for loop with dma_map_single(0. I will try the same in chelsio driver will see the behavior.
>
> Cheers,

[toc] | [prev] | [next] | [standalone]

#1739218 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

FromDavid Woodhouse <dwmw2@infradead.org>
Date2017-09-25 20:50 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<utGC6-1Kz-21@gated-at.bofh.it>
In reply to#1735611

[Multipart message — attachments visible in raw view] — view raw

On Wed, 2017-09-20 at 16:01 +0800, Herbert Xu wrote:
> Harsh Jain <Harsh@chelsio.com> wrote:
> > 
> > While debugging DMA mapping error in chelsio crypto driver we
> observed that when scatter/gather list received by driver has some
> entry with page->offset > 4096 (PAGE_SIZE). It starts giving DMA
> error.  Without IOMMU it works fine.
> 
> This is not a bug.  The network stack can and will feed us such
> SG lists.

Hm? Under what circumstances is the offset permitted to be >
PAGE_SIZE? 

[toc] | [prev] | [next] | [standalone]

#1739259 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

FromCasey Leedom <leedom@chelsio.com>
Date2017-09-25 22:20 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<utI1b-2RB-3@gated-at.bofh.it>
In reply to#1739218
| From: David Woodhouse <dwmw2@infradead.org>
| Sent: Monday, September 25, 2017 11:45 AM
|
| On Wed, 2017-09-20 at 16:01 +0800, Herbert Xu wrote:
| > Harsh Jain <Harsh@chelsio.com> wrote:
| > >
| > > While debugging DMA mapping error in chelsio crypto driver we
| > observed that when scatter/gather list received by driver has some
| > entry with page->offset > 4096 (PAGE_SIZE). It starts giving DMA
| > error.  Without IOMMU it works fine.
| >
| > This is not a bug.  The network stack can and will feed us such
| > SG lists.
|
| Hm? Under what circumstances is the offset permitted to be >
| PAGE_SIZE?

  As I noted earlier, this is an area of the kernel with which I'm not super
familiar.  Both Herbert Xu and our local VM Expert have said that having
Scatter/Gather Lists with Offsets greater than Page Size is not a bug ...

  I'm mostly trying to help out keeping focus on this because Harsh is in
India (presumable enjoying a good night's sleep while we look at this.
Hopefully we'll have a present of a bug fix for him when he wakes up ... :-)

Casey

[toc] | [prev] | [next] | [standalone]

#1739785 — Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

FromHarsh Jain <Harsh@chelsio.com>
Date2017-09-26 13:20 +0200
SubjectRe: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU
Message-ID<utW4a-42r-15@gated-at.bofh.it>
In reply to#1739218

On 26-09-2017 00:15, David Woodhouse wrote:
> On Wed, 2017-09-20 at 16:01 +0800, Herbert Xu wrote:
>> Harsh Jain <Harsh@chelsio.com> wrote:
>>>  
>>> While debugging DMA mapping error in chelsio crypto driver we
>> observed that when scatter/gather list received by driver has some
>> entry with page->offset > 4096 (PAGE_SIZE). It starts giving DMA
>> error.  Without IOMMU it works fine.
>>
>> This is not a bug.  The network stack can and will feed us such
>> SG lists.
> Hm? Under what circumstances is the offset permitted to be >
> PAGE_SIZE? 
Its random, Kernel API's don't check offset value after arithmetic operations like in "__skb_to_sgvec()", "scatterwalk_ffwd()".

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web