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

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-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

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

Hi,

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.

Before reaching to chelsio crypto driver(driver/crypto/chelsio) following entities change the sg'

1) IN esp_output() "__skb_to_sgvec()" convert skb frags to scatter gather list. At that moment sg->offset was 4094.
2) From esp_output control reaches to "crypto_authenc_encrypt()". Here in "scatterwalk_ffwd()" sg->offset become 4110.
3) Same sg list received by chelsio crypto driver(chcr). When chcr try to do DMA mapping it starts giving DMA errors.

Following error observed. first two prints are added for debugging in chcr. Kernel version used to reproduce is 4.9.28 on x86_64.

Sep 15 12:40:52 heptagon kernel: process_cipher req src ffff8803cb41f0a8
Sep 15 12:40:52 heptagon kernel: ========= issue    hit offset:4110 ======= dma_addr f24b000e ==> DMA mapped address returned by dma_map_sg()

Sep 15 12:40:52 heptagon kernel: DMAR: DRHD: handling fault status reg 2
Sep 15 12:40:52 heptagon kernel: DMAR: [DMA Write] Request device [02:00.4] fault addr f24b0000 [fault reason 05] PTE Write access is not set

 By applying following hack in kernel. Things start working.

diff --git a/crypto/scatterwalk.c b/crypto/scatterwalk.c
index c16c94f8..1d75a3a 100644
--- a/crypto/scatterwalk.c
+++ b/crypto/scatterwalk.c
@@ -78,6 +78,8 @@ struct scatterlist *scatterwalk_ffwd(struct scatterlist dst[2]
                                     struct scatterlist *src,
                                     unsigned int len)
 {
+       unsigned int mod_page_offset;
+
        for (;;) {
                if (!len)
                        return src;
@@ -90,7 +92,9 @@ struct scatterlist *scatterwalk_ffwd(struct scatterlist dst[2]
        }

        sg_init_table(dst, 2);
-       sg_set_page(dst, sg_page(src), src->length - len, src->offset + len);
+        mod_page_offset = (src->offset + len) / PAGE_SIZE;
+       sg_set_page(dst, sg_page(src) + mod_page_offset, src->length - len,
+                   (src->offset + len) - (mod_page_offset * PAGE_SIZE));
        scatterwalk_crypto_chain(dst, sg_next(src), 0, 2);


1) We are not expecting issue in "scatterwalk_ffwd" because it is not the only place where kernel 
updates src->offset without checking page boundary. similar logic used in "__skb_to_sgvec".
 
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.

3) Since Without IOMMU every thing works fine. We are expecting IOMMU bugs.

Regards

Harsh Jain

[toc] | [next] | [standalone]

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

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.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

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

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;
 			phys_pfn = pteval >> VTD_PAGE_SHIFT;
 		}
 

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

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

On 20-09-2017 15:42, Robin Murphy wrote:
> 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;
>  			phys_pfn = pteval >> VTD_PAGE_SHIFT;
>  		}
Robin,
Still having following error with above change.

[  429.645492] DMAR: DRHD: handling fault status reg 2
[  429.650847] DMAR: [DMA Write] Request device [02:00.4] fault addr f2682000 [t


>  

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

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

| 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;
|                          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.

Casey

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

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

Hi Casey

Sorry, somehow didn't see this one come by.


On Mon, Sep 25, 2017 at 05:46:40PM +0000, Casey Leedom 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.

Not sure how the page->offset would end up being greater than page-size?

If you have additional traces, please send them by. 

Is this a new driver? wondering how we didn't run into this?


Cheers,
Ashok

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

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

| From: Raj, Ashok <ashok.raj@intel.com>
| Sent: Monday, September 25, 2017 8:54 AM
|
| Not sure how the page->offset would end up being greater than page-size?
|
| If you have additional traces, please send them by.
|
| Is this a new driver? wondering how we didn't run into this?

  According to Herbert Xu and one of our own engineers, it's actually legal
for Scatter/Gather Lists to have this.  This isn't my area of expertise
though so I'm just passing that on.

  I've asked our team to produce a detailed trace of the exact
Scatter/Gather Lists they're seeing and what ends up coming out of the DMA
Mappings, etc.  They're in India, so I expect that they'll have this for you
by tomorrow morning.

Casey

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

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

On 26-09-2017 00:16, Casey Leedom wrote:
> | From: Raj, Ashok <ashok.raj@intel.com>
> | Sent: Monday, September 25, 2017 8:54 AM
> |
> | Not sure how the page->offset would end up being greater than page-size?
Refer below
> |
> | If you have additional traces, please send them by.
> |
> | Is this a new driver? wondering how we didn't run into this?
>
>   According to Herbert Xu and one of our own engineers, it's actually legal
> for Scatter/Gather Lists to have this.  This isn't my area of expertise
> though so I'm just passing that on.
>
>   I've asked our team to produce a detailed trace of the exact
> Scatter/Gather Lists they're seeing and what ends up coming out of the DMA
> Mappings, etc.  They're in India, so I expect that they'll have this for you
> by tomorrow morning.
Below mentioned log was already there in 1st mail. Copied here for easy reference. Let me know if you need
additional traces.

1) IN esp_output() "__skb_to_sgvec()" convert skb frags to scatter gather list. 
At that moment sg->offset was 4094.
2) From esp_output control reaches to "crypto_authenc_encrypt()". Here in 
"scatterwalk_ffwd()" sg->offset become 4110.
3) Same sg list received by chelsio crypto driver(chcr). When chcr try to do 
DMA mapping it starts giving DMA errors.

Following error observed. first two prints are added for debugging in chcr. 
Kernel version used to reproduce is 4.9.28 on x86_64 with Page size 4K.

Sep 15 12:40:52 heptagon kernel: process_cipher req src ffff8803cb41f0a8
Sep 15 12:40:52 heptagon kernel: ========= issue    hit offset:4110 ======= 
dma_addr f24b000e ==> DMA mapped address returned by dma_map_sg()

Sep 15 12:40:52 heptagon kernel: DMAR: DRHD: handling fault status reg 2
Sep 15 12:40:52 heptagon kernel: DMAR: [DMA Write] Request device [02:00.4] 
fault addr f24b0000 [fault reason 05] PTE Write access is not set

>
> Casey

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

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

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

Find attached new set of log. After repeated tries it panics.


On 26-09-2017 09:16, Harsh Jain wrote:
> On 26-09-2017 00:16, Casey Leedom wrote:
>> | From: Raj, Ashok <ashok.raj@intel.com>
>> | Sent: Monday, September 25, 2017 8:54 AM
>> |
>> | Not sure how the page->offset would end up being greater than page-size?
> Refer below
>> |
>> | If you have additional traces, please send them by.
>> |
>> | Is this a new driver? wondering how we didn't run into this?
>>
>>   According to Herbert Xu and one of our own engineers, it's actually legal
>> for Scatter/Gather Lists to have this.  This isn't my area of expertise
>> though so I'm just passing that on.
>>
>>   I've asked our team to produce a detailed trace of the exact
>> Scatter/Gather Lists they're seeing and what ends up coming out of the DMA
>> Mappings, etc.  They're in India, so I expect that they'll have this for you
>> by tomorrow morning.
> Below mentioned log was already there in 1st mail. Copied here for easy reference. Let me know if you need
> additional traces.
>
> 1) IN esp_output() "__skb_to_sgvec()" convert skb frags to scatter gather list. 
> At that moment sg->offset was 4094.
> 2) From esp_output control reaches to "crypto_authenc_encrypt()". Here in 
> "scatterwalk_ffwd()" sg->offset become 4110.
> 3) Same sg list received by chelsio crypto driver(chcr). When chcr try to do 
> DMA mapping it starts giving DMA errors.
>
> Following error observed. first two prints are added for debugging in chcr. 
> Kernel version used to reproduce is 4.9.28 on x86_64 with Page size 4K.
>
> Sep 15 12:40:52 heptagon kernel: process_cipher req src ffff8803cb41f0a8
> Sep 15 12:40:52 heptagon kernel: ========= issue    hit offset:4110 ======= 
> dma_addr f24b000e ==> DMA mapped address returned by dma_map_sg()
>
> Sep 15 12:40:52 heptagon kernel: DMAR: DRHD: handling fault status reg 2
> Sep 15 12:40:52 heptagon kernel: DMAR: [DMA Write] Request device [02:00.4] 
> fault addr f24b0000 [fault reason 05] PTE Write access is not set
>
>> Casey

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

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

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 *domain, unsigned long iov_pfn,
 		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_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;
 			phys_pfn = pteval >> VTD_PAGE_SHIFT;
 		}
 
> 
> 
> On 26-09-2017 09:16, Harsh Jain wrote:
>> On 26-09-2017 00:16, Casey Leedom wrote:
>>> | From: Raj, Ashok <ashok.raj@intel.com>
>>> | Sent: Monday, September 25, 2017 8:54 AM
>>> |
>>> | Not sure how the page->offset would end up being greater than page-size?
>> Refer below
>>> |
>>> | If you have additional traces, please send them by.
>>> |
>>> | Is this a new driver? wondering how we didn't run into this?
>>>
>>>   According to Herbert Xu and one of our own engineers, it's actually legal
>>> for Scatter/Gather Lists to have this.  This isn't my area of expertise
>>> though so I'm just passing that on.
>>>
>>>   I've asked our team to produce a detailed trace of the exact
>>> Scatter/Gather Lists they're seeing and what ends up coming out of the DMA
>>> Mappings, etc.  They're in India, so I expect that they'll have this for you
>>> by tomorrow morning.
>> Below mentioned log was already there in 1st mail. Copied here for easy reference. Let me know if you need
>> additional traces.
>>
>> 1) IN esp_output() "__skb_to_sgvec()" convert skb frags to scatter gather list. 
>> At that moment sg->offset was 4094.
>> 2) From esp_output control reaches to "crypto_authenc_encrypt()". Here in 
>> "scatterwalk_ffwd()" sg->offset become 4110.
>> 3) Same sg list received by chelsio crypto driver(chcr). When chcr try to do 
>> DMA mapping it starts giving DMA errors.
>>
>> Following error observed. first two prints are added for debugging in chcr. 
>> Kernel version used to reproduce is 4.9.28 on x86_64 with Page size 4K.
>>
>> Sep 15 12:40:52 heptagon kernel: process_cipher req src ffff8803cb41f0a8
>> Sep 15 12:40:52 heptagon kernel: ========= issue    hit offset:4110 ======= 
>> dma_addr f24b000e ==> DMA mapped address returned by dma_map_sg()
>>
>> Sep 15 12:40:52 heptagon kernel: DMAR: DRHD: handling fault status reg 2
>> Sep 15 12:40:52 heptagon kernel: DMAR: [DMA Write] Request device [02:00.4] 
>> fault addr f24b0000 [fault reason 05] PTE Write access is not set
>>
>>> Casey
> 

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

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

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

| 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

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

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

| 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

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

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

[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

| 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

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]

Back to top | Article view | linux.kernel

csiph-web