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
[PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu
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.
Signed-off-by: Tom Murphy
---
drivers/iommu/dma-iommu.c | 38 ++
1 file changed, 6 insertions(+), 32 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 185cd504ca5a..6697b4ad0df6 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -843,49 +843,23 @@ static int __finalise_sg(struct device *dev, struct
scatterlist *sg, int nents,
dma_addr_t dma_addr)
{
struct scatterlist *s, *cur = sg;
- unsigned long seg_mask = dma_get_seg_boundary(dev);
- unsigned int cur_len = 0, max_len = dma_get_max_seg_size(dev);
- int i, count = 0;
+ int i;
for_each_sg(sg, s, nents, i) {
/* Restore this segment's original unaligned fields first */
unsigned int s_iova_off = sg_dma_address(s);
unsigned int s_length = sg_dma_len(s);
unsigned int s_iova_len = s->length;
+ if (i > 0)
+ cur = sg_next(cur);
s->offset += s_iova_off;
s->length = s_length;
- sg_dma_address(s) = DMA_MAPPING_ERROR;
- sg_dma_len(s) = 0;
-
- /*
-* Now fill in the real DMA data. If...
-* - there is a valid output segment to append to
-* - and this segment starts on an IOVA page boundary
-* - but doesn't fall at a segment boundary
-* - and wouldn't make the resulting output segment too long
-*/
- if (cur_len && !s_iova_off && (dma_addr & seg_mask) &&
- (max_len - cur_len >= s_length)) {
- /* ...then concatenate it with the previous one */
- cur_len += s_length;
- } else {
- /* Otherwise start the next output segment */
- if (i > 0)
- cur = sg_next(cur);
- cur_len = s_length;
- count++;
-
- sg_dma_address(cur) = dma_addr + s_iova_off;
- }
-
- sg_dma_len(cur) = cur_len;
+ sg_dma_address(cur) = dma_addr + s_iova_off;
+ sg_dma_len(cur) = s_length;
dma_addr += s_iova_len;
-
- if (s_length + s_iova_off < s_iova_len)
- cur_len = 0;
}
- return count;
+ return nents;
}
/*
--
2.20.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
