Re: [Intel-gfx] [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu

Re: [Intel-gfx] [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu

[Christoph Hellwig]

On Thu, Sep 03, 2020 at 09:18:37PM +0100, Tom Murphy wrote:
> Disable combining sg segments in the dma-iommu api.
> Combining the sg segments exposes a bug in the intel i915 driver which
> causes visual artifacts and the screen to freeze. This is most likely
> because of how the i915 handles the returned list. It probably doesn't
> respect the returned value specifying the number of elements in the list
> and instead depends on the previous behaviour of the intel iommu driver
> which would return the same number of elements in the output list as in
> the input list.

So what is the state of addressing this properly in i915?  IF we can't
get it done ASAP I wonder if we need a runtime quirk to disable
merging instead of blocking this conversion..
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu

[Tom Murphy]

On Mon, 7 Sep 2020 at 08:00, Christoph Hellwig <hch@infradead.org> wrote:
>
> On Thu, Sep 03, 2020 at 09:18:37PM +0100, Tom Murphy wrote:
> > Disable combining sg segments in the dma-iommu api.
> > Combining the sg segments exposes a bug in the intel i915 driver which
> > causes visual artifacts and the screen to freeze. This is most likely
> > because of how the i915 handles the returned list. It probably doesn't
> > respect the returned value specifying the number of elements in the list
> > and instead depends on the previous behaviour of the intel iommu driver
> > which would return the same number of elements in the output list as in
> > the input list.
>
> So what is the state of addressing this properly in i915?  IF we can't

I think this is the latest on addressing this issue:
https://patchwork.kernel.org/cover/11306999/

tl;dr: some people seem to be looking at it but I'm not sure if it's
being actively worked on

> get it done ASAP I wonder if we need a runtime quirk to disable
> merging instead of blocking this conversion..

Yeah we talked about passing an attr to map_sg to disable merging at
the following microconfernce:
https://linuxplumbersconf.org/event/7/contributions/846/
As far as I can remember everyone seemed happy with that solution. I
won't be working on this though as I don't have any more time to
dedicate to this. It seems Lu Baolu will take over this.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu

[Christoph Hellwig]

On Mon, Sep 07, 2020 at 09:18:50PM +0100, Tom Murphy wrote:
> Yeah we talked about passing an attr to map_sg to disable merging at
> the following microconfernce:
> https://linuxplumbersconf.org/event/7/contributions/846/
> As far as I can remember everyone seemed happy with that solution. I
> won't be working on this though as I don't have any more time to
> dedicate to this. It seems Lu Baolu will take over this.

I'm absolutely again passing a flag.  Tha just invites further
abuse.  We need a PCI ID based quirk or something else that can't
be as easily abused.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu

[Christoph Hellwig]

On Tue, Sep 08, 2020 at 06:36:19AM +0100, Christoph Hellwig wrote:
> On Mon, Sep 07, 2020 at 09:18:50PM +0100, Tom Murphy wrote:
> > Yeah we talked about passing an attr to map_sg to disable merging at
> > the following microconfernce:
> > https://linuxplumbersconf.org/event/7/contributions/846/
> > As far as I can remember everyone seemed happy with that solution. I
> > won't be working on this though as I don't have any more time to
> > dedicate to this. It seems Lu Baolu will take over this.
> 
> I'm absolutely again passing a flag.  Tha just invites further
> abuse.  We need a PCI ID based quirk or something else that can't
> be as easily abused.

Also, I looked at i915 and there are just three dma_map_sg callers.
The two dmabuf related ones are fixed by Marek in his series, leaving
just the one in i915_gem_gtt_prepare_pages, which does indeed look
very fishy.  But if that one is so hard to fix it can just be replaced
by an open coded for_each_sg loop that contains manual dma_map_page
calls.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu

[Lu Baolu]

Hi Christoph,

On 9/8/20 1:55 PM, Christoph Hellwig wrote:
> On Tue, Sep 08, 2020 at 06:36:19AM +0100, Christoph Hellwig wrote:
>> On Mon, Sep 07, 2020 at 09:18:50PM +0100, Tom Murphy wrote:
>>> Yeah we talked about passing an attr to map_sg to disable merging at
>>> the following microconfernce:
>>> https://linuxplumbersconf.org/event/7/contributions/846/
>>> As far as I can remember everyone seemed happy with that solution. I
>>> won't be working on this though as I don't have any more time to
>>> dedicate to this. It seems Lu Baolu will take over this.
>>
>> I'm absolutely again passing a flag.  Tha just invites further
>> abuse.  We need a PCI ID based quirk or something else that can't
>> be as easily abused.
> 
> Also, I looked at i915 and there are just three dma_map_sg callers.
> The two dmabuf related ones are fixed by Marek in his series, leaving

Do you mind telling where can I find Marek's series?

Best regards,
baolu

> just the one in i915_gem_gtt_prepare_pages, which does indeed look
> very fishy.  But if that one is so hard to fix it can just be replaced
> by an open coded for_each_sg loop that contains manual dma_map_page
> calls.
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu

[Christoph Hellwig]

On Tue, Sep 08, 2020 at 02:04:53PM +0800, Lu Baolu wrote:
> Do you mind telling where can I find Marek's series?

[PATCH v10 00/30] DRM: fix struct sg_table nents vs. orig_nents misuse

on various lists including the iommu one.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu

[Lu Baolu]

On 2020/9/8 14:23, Christoph Hellwig wrote:
> On Tue, Sep 08, 2020 at 02:04:53PM +0800, Lu Baolu wrote:
>> Do you mind telling where can I find Marek's series?
> 
> [PATCH v10 00/30] DRM: fix struct sg_table nents vs. orig_nents misuse
> 
> on various lists including the iommu one.
> 

Get it. Thank you!

Best regards,
baolu
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu

[Lu Baolu]

Hi Christoph,

On 9/8/20 2:23 PM, Christoph Hellwig wrote:
> On Tue, Sep 08, 2020 at 02:04:53PM +0800, Lu Baolu wrote:
>> Do you mind telling where can I find Marek's series?
> 
> [PATCH v10 00/30] DRM: fix struct sg_table nents vs. orig_nents misuse
> 
> on various lists including the iommu one.
> 

It seems that more work is needed in i915 driver. I will added below
quirk as you suggested.

--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -851,6 +851,31 @@ static int __finalise_sg(struct device *dev, struct 
scatterlist *sg, int nents,
         unsigned int cur_len = 0, max_len = dma_get_max_seg_size(dev);
         int i, count = 0;

+       /*
+        * The Intel graphic device driver is used to assume that the 
returned
+        * sg list is not combound. This blocks the efforts of 
converting the
+        * Intel IOMMU driver to dma-iommu api's. Add this quirk to make the
+        * device driver work and should be removed once it's fixed in i915
+        * driver.
+        */
+       if (dev_is_pci(dev) &&
+           to_pci_dev(dev)->vendor == PCI_VENDOR_ID_INTEL &&
+           (to_pci_dev(dev)->class >> 16) == PCI_BASE_CLASS_DISPLAY) {
+               for_each_sg(sg, s, nents, i) {
+                       unsigned int s_iova_off = sg_dma_address(s);
+                       unsigned int s_length = sg_dma_len(s);
+                       unsigned int s_iova_len = s->length;
+
+                       s->offset += s_iova_off;
+                       s->length = s_length;
+                       sg_dma_address(s) = dma_addr + s_iova_off;
+                       sg_dma_len(s) = s_length;
+                       dma_addr += s_iova_len;
+               }
+
+               return nents;
+       }
+

Best regards,
baolu
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu

[Christoph Hellwig]

On Wed, Sep 09, 2020 at 09:43:09AM +0800, Lu Baolu wrote:
> +       /*
> +        * The Intel graphic device driver is used to assume that the
> returned
> +        * sg list is not combound. This blocks the efforts of converting
> the

This adds pointless overly long lines.

> +        * Intel IOMMU driver to dma-iommu api's. Add this quirk to make the
> +        * device driver work and should be removed once it's fixed in i915
> +        * driver.
> +        */
> +       if (dev_is_pci(dev) &&
> +           to_pci_dev(dev)->vendor == PCI_VENDOR_ID_INTEL &&
> +           (to_pci_dev(dev)->class >> 16) == PCI_BASE_CLASS_DISPLAY) {
> +               for_each_sg(sg, s, nents, i) {
> +                       unsigned int s_iova_off = sg_dma_address(s);
> +                       unsigned int s_length = sg_dma_len(s);
> +                       unsigned int s_iova_len = s->length;
> +
> +                       s->offset += s_iova_off;
> +                       s->length = s_length;
> +                       sg_dma_address(s) = dma_addr + s_iova_off;
> +                       sg_dma_len(s) = s_length;
> +                       dma_addr += s_iova_len;
> +               }
> +
> +               return nents;
> +       }

This wants an IS_ENABLED() check.  And probably a pr_once reminding
of the workaround.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu

[Lu Baolu]

On 2020/9/9 15:06, Christoph Hellwig wrote:
> On Wed, Sep 09, 2020 at 09:43:09AM +0800, Lu Baolu wrote:
>> +       /*
>> +        * The Intel graphic device driver is used to assume that the
>> returned
>> +        * sg list is not combound. This blocks the efforts of converting
>> the
> 
> This adds pointless overly long lines.
> 
>> +        * Intel IOMMU driver to dma-iommu api's. Add this quirk to make the
>> +        * device driver work and should be removed once it's fixed in i915
>> +        * driver.
>> +        */
>> +       if (dev_is_pci(dev) &&
>> +           to_pci_dev(dev)->vendor == PCI_VENDOR_ID_INTEL &&
>> +           (to_pci_dev(dev)->class >> 16) == PCI_BASE_CLASS_DISPLAY) {
>> +               for_each_sg(sg, s, nents, i) {
>> +                       unsigned int s_iova_off = sg_dma_address(s);
>> +                       unsigned int s_length = sg_dma_len(s);
>> +                       unsigned int s_iova_len = s->length;
>> +
>> +                       s->offset += s_iova_off;
>> +                       s->length = s_length;
>> +                       sg_dma_address(s) = dma_addr + s_iova_off;
>> +                       sg_dma_len(s) = s_length;
>> +                       dma_addr += s_iova_len;
>> +               }
>> +
>> +               return nents;
>> +       }
> 
> This wants an IS_ENABLED() check.  And probably a pr_once reminding
> of the workaround.
> 

Will fix in the next version.

Best regards,
baolu
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx