Re: [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu
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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu
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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu
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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu
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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu
On Mon, 7 Sep 2020 at 08:00, Christoph Hellwig 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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu
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.. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu