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

[Casey Leedom]

| From: Raj, Ashok <ashok.raj@xxxxxxxxx>
| 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@xxxxxxxxxxx> wrote:
| > > | From: Dan Williams <dan.j.williams@xxxxxxxxx>
| > > | 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