Revert "arm64: dma: Drop cache invalidation from arch_dma_prep_coherent()"

This reverts commit c44094eee32f32f175aadc0efcac449d99b1bbf7.

As reported by Amit [1], dropping cache invalidation from arch_dma_prep_coherent() triggers a crash on the Qualcomm SM8250 platform

(most probably on other Qcom platforms too). The reason is, Qcom qcom_q6v5_mss driver copies the firmware metadata and shares it with modem for validation. The modem has a secure block (XPU) that will trigger a whole system crash if the shared memory is accessed by the CPU while modem is poking at it.

To avoid this issue, the qcom_q6v5_mss driver allocates a chunk of memory with no kernel mapping, vmap's it, copies the firmware metadata and unvmap's it. Finally the address is then shared with modem for metadata validation [2].

Now because of the removal of cache invalidation from arch_dma_prep_coherent(), there will be cache lines associated with this memory even after sharing with modem. So when the CPU accesses it, the XPU violation gets triggered.

So let's revert this commit to get remoteproc's working (thereby avoiding full system crash) on Qcom platforms.

[1] https://lore.kernel.org/linux-arm-kernel/CAMi1Hd1VBCFhf7+EXWHQWcGy4k=tcyLa7RGiFdprtRnegSG0Mw@mail.gmail.com/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/remoteproc/qcom_q6v5_mss.c#n933

Signed-off-by Manivannan Sadhasivam <[email protected]>

---

Will, Catalin: Please share if you have any other suggestions to handle the resource sharing in the remoteproc driver that could avoid this revert.

 arch/arm64/mm/dma-mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/mm/dma-mapping.c +1 -1

index 3cb101e8cb29..7d7e9a046305 100644
@@ -36,7 +36,7 @@ void arch_dma_prep_coherent(struct page *page, size_t size)
 {
 	unsigned long start = (unsigned long)page_address(page);
 
-	dcache_clean_poc(start, start + size);
+	dcache_clean_inval_poc(start, start + size);
 }
 
 #ifdef CONFIG_IOMMU_DMA

Signature · 1 line

2.25.1

This reverts commit c44094eee32f32f175aadc0efcac449d99b1bbf7.

As reported by Amit [1], dropping cache invalidation from arch_dma_prep_coherent() triggers a crash on the Qualcomm SM8250 platform (most probably on other Qcom platforms too). The reason is, Qcom qcom_q6v5_mss driver copies the firmware metadata and shares it with modem for validation. The modem has a secure block (XPU) that will trigger a whole system crash if the shared memory is accessed by the CPU while modem is poking at it.

To avoid this issue, the qcom_q6v5_mss driver allocates a chunk of memory with no kernel mapping, vmap's it, copies the firmware metadata and unvmap's it. Finally the address is then shared with modem for metadata validation [2].

Now because of the removal of cache invalidation from arch_dma_prep_coherent(), there will be cache lines associated with this memory even after sharing with modem. So when the CPU accesses it, the XPU violation gets triggered.

Quoted 15 lines · Manivannan Sadhasivam

This reverts commit c44094eee32f32f175aadc0efcac449d99b1bbf7.

As reported by Amit [1], dropping cache invalidation from arch_dma_prep_coherent() triggers a crash on the Qualcomm SM8250 platform (most probably on other Qcom platforms too). The reason is, Qcom qcom_q6v5_mss driver copies the firmware metadata and shares it with modem for validation. The modem has a secure block (XPU) that will trigger a whole system crash if the shared memory is accessed by the CPU while modem is poking at it.

To avoid this issue, the qcom_q6v5_mss driver allocates a chunk of memory with no kernel mapping, vmap's it, copies the firmware metadata and unvmap's it. Finally the address is then shared with modem for metadata validation [2].

Quoted 4 lines

Now because of the removal of cache invalidation from arch_dma_prep_coherent(), there will be cache lines associated with this memory even after sharing with modem. So when the CPU accesses it, the XPU violation gets triggered.

This last past is a non-sequitur: the buffer is no longer mapped on the CPU side, so how would the CPU access it?

As I just replied to Amit, we need more information about what this "access" is and how it is being detected.

Will

Quoted 16 lines · Will Deacon

Quoted 15 lines · Manivannan Sadhasivam

This reverts commit c44094eee32f32f175aadc0efcac449d99b1bbf7.

As reported by Amit [1], dropping cache invalidation from arch_dma_prep_coherent() triggers a crash on the Qualcomm SM8250 platform (most probably on other Qcom platforms too). The reason is, Qcom qcom_q6v5_mss driver copies the firmware metadata and shares it with modem for validation. The modem has a secure block (XPU) that will trigger a whole system crash if the shared memory is accessed by the CPU while modem is poking at it.

To avoid this issue, the qcom_q6v5_mss driver allocates a chunk of memory with no kernel mapping, vmap's it, copies the firmware metadata and unvmap's it. Finally the address is then shared with modem for metadata validation [2].

Quoted 7 lines

Quoted 4 lines

Now because of the removal of cache invalidation from arch_dma_prep_coherent(), there will be cache lines associated with this memory even after sharing with modem. So when the CPU accesses it, the XPU violation gets triggered.

This last past is a non-sequitur: the buffer is no longer mapped on the CPU side, so how would the CPU access it?

Right, for the previous change to have made a difference the offending part of this buffer must be present in some cache somewhere *before* the DMA buffer allocation completes.

Clearly that driver is completely broken though. If the DMA allocation came from a no-map carveout vma_dma_alloc_from_dev_coherent() then the vmap() shenanigans wouldn't work, so if it backed by struct pages then the whole dance is still pointless because *a cacheable linear mapping exists*, and it's just relying on the reduced chance that anything's going to re-fetch the linear map address after those pages have been allocated, exactly as I called out previously[1].

Quoted 29 lines · Robin Murphy

Quoted 16 lines · Will Deacon

Quoted 15 lines · Manivannan Sadhasivam

This reverts commit c44094eee32f32f175aadc0efcac449d99b1bbf7.

As reported by Amit [1], dropping cache invalidation from arch_dma_prep_coherent() triggers a crash on the Qualcomm SM8250 platform (most probably on other Qcom platforms too). The reason is, Qcom qcom_q6v5_mss driver copies the firmware metadata and shares it with modem for validation. The modem has a secure block (XPU) that will trigger a whole system crash if the shared memory is accessed by the CPU while modem is poking at it.

To avoid this issue, the qcom_q6v5_mss driver allocates a chunk of memory with no kernel mapping, vmap's it, copies the firmware metadata and unvmap's it. Finally the address is then shared with modem for metadata validation [2].

Quoted 7 lines

Quoted 4 lines

Now because of the removal of cache invalidation from arch_dma_prep_coherent(), there will be cache lines associated with this memory even after sharing with modem. So when the CPU accesses it, the XPU violation gets triggered.

This last past is a non-sequitur: the buffer is no longer mapped on the CPU side, so how would the CPU access it?

Right, for the previous change to have made a difference the offending part of this buffer must be present in some cache somewhere *before* the DMA buffer allocation completes.

Quoted 7 lines

Clearly that driver is completely broken though. If the DMA allocation came from a no-map carveout vma_dma_alloc_from_dev_coherent() then the vmap() shenanigans wouldn't work, so if it backed by struct pages then the whole dance is still pointless because *a cacheable linear mapping exists*, and it's just relying on the reduced chance that anything's going to re-fetch the linear map address after those pages have been allocated, exactly as I called out previously[1].

So I guess a DMA pool that's not mapped in the linear map, together with memremap() instead of vmap(), would work around the issue. But the driver needs fixing, not the arch code.

Signature · 1 line

Catalin

Quoted 38 lines · Catalin Marinas

Quoted 36 lines · Robin Murphy

Quoted 16 lines · Will Deacon

Quoted 15 lines · Manivannan Sadhasivam

This reverts commit c44094eee32f32f175aadc0efcac449d99b1bbf7.

As reported by Amit [1], dropping cache invalidation from arch_dma_prep_coherent() triggers a crash on the Qualcomm SM8250 platform (most probably on other Qcom platforms too). The reason is, Qcom qcom_q6v5_mss driver copies the firmware metadata and shares it with modem for validation. The modem has a secure block (XPU) that will trigger a whole system crash if the shared memory is accessed by the CPU while modem is poking at it.

To avoid this issue, the qcom_q6v5_mss driver allocates a chunk of memory with no kernel mapping, vmap's it, copies the firmware metadata and unvmap's it. Finally the address is then shared with modem for metadata validation [2].

Quoted 7 lines

Quoted 4 lines

Now because of the removal of cache invalidation from arch_dma_prep_coherent(), there will be cache lines associated with this memory even after sharing with modem. So when the CPU accesses it, the XPU violation gets triggered.

This last past is a non-sequitur: the buffer is no longer mapped on the CPU side, so how would the CPU access it?

Right, for the previous change to have made a difference the offending part of this buffer must be present in some cache somewhere *before* the DMA buffer allocation completes.

Clearly that driver is completely broken though. If the DMA allocation came from a no-map carveout vma_dma_alloc_from_dev_coherent() then the vmap() shenanigans wouldn't work, so if it backed by struct pages then the whole dance is still pointless because *a cacheable linear mapping exists*, and it's just relying on the reduced chance that anything's going to re-fetch the linear map address after those pages have been allocated, exactly as I called out previously[1].

Quoted 4 lines

So I guess a DMA pool that's not mapped in the linear map, together with memremap() instead of vmap(), would work around the issue. But the driver needs fixing, not the arch code.

Okay, thanks for the hint. Can you share how to allocate the dma-pool that's not part of the kernel's linear map? I looked into it but couldn't find a way.

Quoted 44 lines · Manivannan Sadhasivam

Quoted 42 lines · Catalin Marinas

Quoted 29 lines · Robin Murphy

Quoted 16 lines · Will Deacon

Quoted 15 lines · Manivannan Sadhasivam

This reverts commit c44094eee32f32f175aadc0efcac449d99b1bbf7.

As reported by Amit [1], dropping cache invalidation from arch_dma_prep_coherent() triggers a crash on the Qualcomm SM8250 platform (most probably on other Qcom platforms too). The reason is, Qcom qcom_q6v5_mss driver copies the firmware metadata and shares it with modem for validation. The modem has a secure block (XPU) that will trigger a whole system crash if the shared memory is accessed by the CPU while modem is poking at it.

To avoid this issue, the qcom_q6v5_mss driver allocates a chunk of memory with no kernel mapping, vmap's it, copies the firmware metadata and unvmap's it. Finally the address is then shared with modem for metadata validation [2].

Quoted 7 lines

Quoted 4 lines

Now because of the removal of cache invalidation from arch_dma_prep_coherent(), there will be cache lines associated with this memory even after sharing with modem. So when the CPU accesses it, the XPU violation gets triggered.

This last past is a non-sequitur: the buffer is no longer mapped on the CPU side, so how would the CPU access it?

Right, for the previous change to have made a difference the offending part of this buffer must be present in some cache somewhere *before* the DMA buffer allocation completes.

Quoted 7 lines

Clearly that driver is completely broken though. If the DMA allocation came from a no-map carveout vma_dma_alloc_from_dev_coherent() then the vmap() shenanigans wouldn't work, so if it backed by struct pages then the whole dance is still pointless because *a cacheable linear mapping exists*, and it's just relying on the reduced chance that anything's going to re-fetch the linear map address after those pages have been allocated, exactly as I called out previously[1].

So I guess a DMA pool that's not mapped in the linear map, together with memremap() instead of vmap(), would work around the issue. But the driver needs fixing, not the arch code.

Quoted 2 lines

Okay, thanks for the hint. Can you share how to allocate the dma-pool that's not part of the kernel's linear map? I looked into it but couldn't find a way.

The no-map property should take care of this iirc

Will

Quoted 48 lines · Will Deacon

Quoted 46 lines · Manivannan Sadhasivam

Quoted 38 lines · Catalin Marinas

Quoted 29 lines · Robin Murphy

Quoted 16 lines · Will Deacon

Quoted 15 lines · Manivannan Sadhasivam

This reverts commit c44094eee32f32f175aadc0efcac449d99b1bbf7.

As reported by Amit [1], dropping cache invalidation from arch_dma_prep_coherent() triggers a crash on the Qualcomm SM8250 platform (most probably on other Qcom platforms too). The reason is, Qcom qcom_q6v5_mss driver copies the firmware metadata and shares it with modem for validation. The modem has a secure block (XPU) that will trigger a whole system crash if the shared memory is accessed by the CPU while modem is poking at it.

To avoid this issue, the qcom_q6v5_mss driver allocates a chunk of memory with no kernel mapping, vmap's it, copies the firmware metadata and unvmap's it. Finally the address is then shared with modem for metadata validation [2].

Quoted 7 lines

Quoted 4 lines

Now because of the removal of cache invalidation from arch_dma_prep_coherent(), there will be cache lines associated with this memory even after sharing with modem. So when the CPU accesses it, the XPU violation gets triggered.

This last past is a non-sequitur: the buffer is no longer mapped on the CPU side, so how would the CPU access it?

Right, for the previous change to have made a difference the offending part of this buffer must be present in some cache somewhere *before* the DMA buffer allocation completes.

Quoted 7 lines

Clearly that driver is completely broken though. If the DMA allocation came from a no-map carveout vma_dma_alloc_from_dev_coherent() then the vmap() shenanigans wouldn't work, so if it backed by struct pages then the whole dance is still pointless because *a cacheable linear mapping exists*, and it's just relying on the reduced chance that anything's going to re-fetch the linear map address after those pages have been allocated, exactly as I called out previously[1].

Quoted 4 lines

So I guess a DMA pool that's not mapped in the linear map, together with memremap() instead of vmap(), would work around the issue. But the driver needs fixing, not the arch code.

Okay, thanks for the hint. Can you share how to allocate the dma-pool that's not part of the kernel's linear map? I looked into it but couldn't find a way.

Quoted 2 lines

The no-map property should take care of this iirc

Yeah, we have been using it in other places of the same driver. But as per Sibi, we used dynamic allocation for metadata validation since there was no memory reserved statically for that.

But if we do not have a way to allocate a dynamic memory that is not part of kernel's linear map, then we may have to resort to using an existing reserved memory.

Thanks,
Mani

Quoted 1 line

Will

Quoted 16 lines · Manivannan Sadhasivam

Quoted 15 lines · Will Deacon

Quoted 14 lines · Manivannan Sadhasivam

Quoted 12 lines · Catalin Marinas

Quoted 7 lines · Robin Murphy

Clearly that driver is completely broken though. If the DMA allocation came from a no-map carveout vma_dma_alloc_from_dev_coherent() then the vmap() shenanigans wouldn't work, so if it backed by struct pages then the whole dance is still pointless because *a cacheable linear mapping exists*, and it's just relying on the reduced chance that anything's going to re-fetch the linear map address after those pages have been allocated, exactly as I called out previously[1].

So I guess a DMA pool that's not mapped in the linear map, together with memremap() instead of vmap(), would work around the issue. But the driver needs fixing, not the arch code.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Okay, thanks for the hint. Can you share how to allocate the dma-pool that's not part of the kernel's linear map? I looked into it but couldn't find a way.

The no-map property should take care of this iirc

Yeah, we have been using it in other places of the same driver. But as per Sibi, we used dynamic allocation for metadata validation since there was no memory reserved statically for that.

Unlike the other portions in the driver that required statically defined no-map carveouts, metadata just needed a contiguous memory for authentication. Re-using existing carveouts for this metadata region may not work due to modem FW limitations and declaring a new carveout for metadata will break the device tree bindings. That's the reason for using DMA_ATTR_NO_KERNEL_MAPPING for dma_alloc_attr and vmpa/vunmap with VM_FLUSH_RESET_PERMS before passing the memory onto modem. Are there other suggestions for achieving the same without breaking bindings?

Quoted 26 lines · Sibi Sankar

Quoted 16 lines · Manivannan Sadhasivam

Quoted 15 lines · Will Deacon

Quoted 14 lines · Manivannan Sadhasivam

Quoted 12 lines · Catalin Marinas

Quoted 7 lines · Robin Murphy

Clearly that driver is completely broken though. If the DMA allocation came from a no-map carveout vma_dma_alloc_from_dev_coherent() then the vmap() shenanigans wouldn't work, so if it backed by struct pages then the whole dance is still pointless because *a cacheable linear mapping exists*, and it's just relying on the reduced chance that anything's going to re-fetch the linear map address after those pages have been allocated, exactly as I called out previously[1].

So I guess a DMA pool that's not mapped in the linear map, together with memremap() instead of vmap(), would work around the issue. But the driver needs fixing, not the arch code.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Okay, thanks for the hint. Can you share how to allocate the dma-pool that's not part of the kernel's linear map? I looked into it but couldn't find a way.

The no-map property should take care of this iirc

Yeah, we have been using it in other places of the same driver. But as per Sibi, we used dynamic allocation for metadata validation since there was no memory reserved statically for that.

Quoted 8 lines

Unlike the other portions in the driver that required statically defined no-map carveouts, metadata just needed a contiguous memory for authentication. Re-using existing carveouts for this metadata region may not work due to modem FW limitations and declaring a new carveout for metadata will break the device tree bindings. That's the reason for using DMA_ATTR_NO_KERNEL_MAPPING for dma_alloc_attr and vmpa/vunmap with VM_FLUSH_RESET_PERMS before passing the memory onto modem. Are there other suggestions for achieving the same without breaking bindings?

Your DMA_ATTR_NO_KERNEL_MAPPING workaround doesn't work, it only makes the failure rate smaller. All this attribute does is avoiding creating a non-cacheable mapping but you still have the kernel linear mapping in place that may be speculatively accessed by the CPU. You were just lucky so far not to have hit the issue. So I'd rather see this fixed properly with a no-map carveout. Maybe you can reuse an existing carveout if the driver already needs some and avoid changing the DT. More complicated options include allocating memory and unmapping it from the linear map with set_memory_valid(), though that's not exported to modules and it also requires the linear map to be pages only, not block mappings.

Yet another option is to have the swiotlb buffer unmapped from the kernel linear map and use the bounce buffer for this. That's more involved (Robin has some patches, though for a different reason and they may not make it upstream).

Has any progress been made to fix this regression? It afaics is not a release critical issue, but well, it still would be nice to get this fixed before 6.1 is released.

Quoted 3 lines · Thorsten Leemhuis

Has any progress been made to fix this regression? It afaics is not a release critical issue, but well, it still would be nice to get this fixed before 6.1 is released.

The only (nearly) risk-free "fix" for 6.1 would be to revert the commit that exposed the driver bug. It doesn't fix the actual bug, it only makes it less likely to happen.

I like the original commit removing the cache invalidation as it shows drivers not behaving properly but, as a workaround, we could add a command line option to force back the old behaviour (defaulting to the new one) until the driver is fixed.

--
Catalin

Quoted 6 lines · Catalin Marinas

Quoted 3 lines · Thorsten Leemhuis

Has any progress been made to fix this regression? It afaics is not a release critical issue, but well, it still would be nice to get this fixed before 6.1 is released.

Quoted 6 lines

The only (nearly) risk-free "fix" for 6.1 would be to revert the commit that exposed the driver bug. It doesn't fix the actual bug, it only makes it less likely to happen.

I like the original commit removing the cache invalidation as it shows drivers not behaving properly

Quoted 3 lines

but, as a workaround, we could add a
command line option to force back the old behaviour (defaulting to the new one) until the driver is fixed.

We use DB845c extensively for mainline and android-mainline[1] testing with AOSP, and it is broken for weeks now. So be it a temporary workaround or a proper driver fix in place, we'd really appreciate a quick fix here.

[...]

Quoted 11 lines · Amit Pundir

Quoted 10 lines · Catalin Marinas

Quoted 3 lines · Thorsten Leemhuis

Has any progress been made to fix this regression? It afaics is not a release critical issue, but well, it still would be nice to get this fixed before 6.1 is released.

The only (nearly) risk-free "fix" for 6.1 would be to revert the commit that exposed the driver bug. It doesn't fix the actual bug, it only makes it less likely to happen.

Quoted 2 lines

Quoted 2 lines

I like the original commit removing the cache invalidation as it shows drivers not behaving properly

Yeah, I understand that, but I guess it's my job to ask at this point:
"is continuing to live with the old behavior for one or two more cycles" that much of a problem"?

Quoted 3 lines

Quoted 3 lines

but, as a workaround, we could add a
command line option to force back the old behaviour (defaulting to the new one) until the driver is fixed.

Well, sometimes that approach is fine to fix a regression, but I'm not sure this is one of those situations, as this...

Quoted 4 lines

We use DB845c extensively for mainline and android-mainline[1] testing with AOSP, and it is broken for weeks now. So be it a temporary workaround or a proper driver fix in place, we'd really appreciate a quick fix here.

...doesn't sound like we are not talking about some odd corner case here. But in the end that would be up to Linus to decide.

I'll point him to this thread once more in my weekly report anyway. Maybe I'll even suggest to revert this change, not sure yet.

Quoted 15 lines · Thorsten Leemhuis

Quoted 13 lines · Amit Pundir

Quoted 12 lines · Catalin Marinas

Quoted 3 lines · Thorsten Leemhuis

Has any progress been made to fix this regression? It afaics is not a release critical issue, but well, it still would be nice to get this fixed before 6.1 is released.

The only (nearly) risk-free "fix" for 6.1 would be to revert the commit that exposed the driver bug. It doesn't fix the actual bug, it only makes it less likely to happen.

I like the original commit removing the cache invalidation as it shows drivers not behaving properly

Quoted 3 lines

Yeah, I understand that, but I guess it's my job to ask at this point:
"is continuing to live with the old behavior for one or two more cycles" that much of a problem"?

That wouldn't be a problem. The problem is that I haven't see any efforts from the Qualcomm side to actually fix the drivers [...]

Quoted 20 lines · Will Deacon

Quoted 18 lines · Thorsten Leemhuis

Quoted 7 lines · Amit Pundir

Quoted 6 lines · Catalin Marinas

Quoted 3 lines · Thorsten Leemhuis

Has any progress been made to fix this regression? It afaics is not a release critical issue, but well, it still would be nice to get this fixed before 6.1 is released.

Quoted 6 lines

Quoted 6 lines

The only (nearly) risk-free "fix" for 6.1 would be to revert the commit that exposed the driver bug. It doesn't fix the actual bug, it only makes it less likely to happen.

I like the original commit removing the cache invalidation as it shows drivers not behaving properly

Yeah, I understand that, but I guess it's my job to ask at this point:
"is continuing to live with the old behavior for one or two more cycles" that much of a problem"?

Quoted 2 lines

That wouldn't be a problem. The problem is that I haven't see any efforts from the Qualcomm side to actually fix the drivers [...]

Thx for sharing the details. I can fully understand your pain. But well, in the end it looks to me like this commit it intentionally breaking something that used to work -- which to my understanding of the "no regression rule" is not okay, even if things only worked by chance and not flawless.

Quoted 24 lines · Thorsten Leemhuis

Quoted 22 lines · Will Deacon

Quoted 12 lines · Thorsten Leemhuis

Quoted 11 lines · Amit Pundir

Quoted 10 lines · Catalin Marinas

Quoted 3 lines · Thorsten Leemhuis

Has any progress been made to fix this regression? It afaics is not a release critical issue, but well, it still would be nice to get this fixed before 6.1 is released.

The only (nearly) risk-free "fix" for 6.1 would be to revert the commit that exposed the driver bug. It doesn't fix the actual bug, it only makes it less likely to happen.

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

I like the original commit removing the cache invalidation as it shows drivers not behaving properly

Yeah, I understand that, but I guess it's my job to ask at this point:
"is continuing to live with the old behavior for one or two more cycles" that much of a problem"?

That wouldn't be a problem. The problem is that I haven't see any efforts from the Qualcomm side to actually fix the drivers [...]

Quoted 5 lines

Thx for sharing the details. I can fully understand your pain. But well, in the end it looks to me like this commit it intentionally breaking something that used to work -- which to my understanding of the "no regression rule" is not okay, even if things only worked by chance and not flawless.

"no regressions" for userspace code, this is broken, out-of-tree driver code, right?

I do not think any in-kernel drivers have this issue today
from what I can tell, but if I am wrong here, please let me know.

We don't keep stable apis, or even functionality, for out-of-tree kernel code as that would be impossible for us to do for obvious reasons.

Quoted 31 lines · Greg KH

Quoted 29 lines · Thorsten Leemhuis

Quoted 16 lines · Will Deacon

Quoted 15 lines · Thorsten Leemhuis

Quoted 13 lines · Amit Pundir

Quoted 12 lines · Catalin Marinas

Quoted 3 lines · Thorsten Leemhuis

Has any progress been made to fix this regression? It afaics is not a release critical issue, but well, it still would be nice to get this fixed before 6.1 is released.

The only (nearly) risk-free "fix" for 6.1 would be to revert the commit that exposed the driver bug. It doesn't fix the actual bug, it only makes it less likely to happen.

I like the original commit removing the cache invalidation as it shows drivers not behaving properly

Quoted 6 lines

Quoted 3 lines

Yeah, I understand that, but I guess it's my job to ask at this point:
"is continuing to live with the old behavior for one or two more cycles" that much of a problem"?

That wouldn't be a problem. The problem is that I haven't see any efforts from the Qualcomm side to actually fix the drivers [...]

Thx for sharing the details. I can fully understand your pain. But well, in the end it looks to me like this commit it intentionally breaking something that used to work -- which to my understanding of the "no regression rule" is not okay, even if things only worked by chance and not flawless.

Quoted 2 lines

"no regressions" for userspace code, this is broken, out-of-tree driver code, right?

If so: apologies. But that's not the impression I got, as Amit wrote "I can reproduce this crash on vanilla v6.1-rc1 as well with no out-of-tree drivers." here:
https://lore.kernel.org/linux-arm-kernel/CAMi1Hd3H2k1J8hJ6e-Miy5+nVDNzv6qQ3nN-9929B0GbHJkXEg@mail.gmail.com/

Quoted 34 lines · Thorsten Leemhuis

Quoted 31 lines · Greg KH

Quoted 29 lines · Thorsten Leemhuis

Quoted 16 lines · Will Deacon

Quoted 15 lines · Thorsten Leemhuis

Quoted 13 lines · Amit Pundir

Quoted 12 lines · Catalin Marinas

Quoted 3 lines · Thorsten Leemhuis

Has any progress been made to fix this regression? It afaics is not a release critical issue, but well, it still would be nice to get this fixed before 6.1 is released.

The only (nearly) risk-free "fix" for 6.1 would be to revert the commit that exposed the driver bug. It doesn't fix the actual bug, it only makes it less likely to happen.

I like the original commit removing the cache invalidation as it shows drivers not behaving properly

Quoted 6 lines

Quoted 3 lines

Yeah, I understand that, but I guess it's my job to ask at this point:
"is continuing to live with the old behavior for one or two more cycles" that much of a problem"?

That wouldn't be a problem. The problem is that I haven't see any efforts from the Qualcomm side to actually fix the drivers [...]

Thx for sharing the details. I can fully understand your pain. But well, in the end it looks to me like this commit it intentionally breaking something that used to work -- which to my understanding of the "no regression rule" is not okay, even if things only worked by chance and not flawless.

Quoted 7 lines

Quoted 2 lines

"no regressions" for userspace code, this is broken, out-of-tree driver code, right?

If so: apologies. But that's not the impression I got, as Amit wrote "I can reproduce this crash on vanilla v6.1-rc1 as well with no out-of-tree drivers." here:
https://lore.kernel.org/linux-arm-kernel/CAMi1Hd3H2k1J8hJ6e-Miy5+nVDNzv6qQ3nN-9929B0GbHJkXEg@mail.gmail.com/

Ah, I missed that.

Ok, what in-tree drivers are having problems being buggy? I can't seem to figure that out from that report at all. Does anyone know?

thanks,

greg k-h

Quoted 41 lines · Greg KH

Quoted 32 lines · Thorsten Leemhuis

Quoted 31 lines · Greg KH

Quoted 29 lines · Thorsten Leemhuis

Quoted 16 lines · Will Deacon

Quoted 15 lines · Thorsten Leemhuis

Quoted 13 lines · Amit Pundir

Quoted 12 lines · Catalin Marinas

Quoted 3 lines · Thorsten Leemhuis

Has any progress been made to fix this regression? It afaics is not a release critical issue, but well, it still would be nice to get this fixed before 6.1 is released.

The only (nearly) risk-free "fix" for 6.1 would be to revert the commit that exposed the driver bug. It doesn't fix the actual bug, it only makes it less likely to happen.

I like the original commit removing the cache invalidation as it shows drivers not behaving properly

Quoted 6 lines

Quoted 3 lines

Yeah, I understand that, but I guess it's my job to ask at this point:
"is continuing to live with the old behavior for one or two more cycles" that much of a problem"?

That wouldn't be a problem. The problem is that I haven't see any efforts from the Qualcomm side to actually fix the drivers [...]

Thx for sharing the details. I can fully understand your pain. But well, in the end it looks to me like this commit it intentionally breaking something that used to work -- which to my understanding of the "no regression rule" is not okay, even if things only worked by chance and not flawless.

Quoted 7 lines

Quoted 2 lines

"no regressions" for userspace code, this is broken, out-of-tree driver code, right?

If so: apologies. But that's not the impression I got, as Amit wrote "I can reproduce this crash on vanilla v6.1-rc1 as well with no out-of-tree drivers." here:
https://lore.kernel.org/linux-arm-kernel/CAMi1Hd3H2k1J8hJ6e-Miy5+nVDNzv6qQ3nN-9929B0GbHJkXEg@mail.gmail.com/

Quoted 5 lines

Ah, I missed that.

Ok, what in-tree drivers are having problems being buggy? I can't seem to figure that out from that report at all. Does anyone know?

It is the Qualcomm Q6V5_MSS remoteproc driver:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/remoteproc/qcom_q6v5_mss.c

Qualcomm is working on the fix but the patches are not ready yet. So if we can get this patch reverted in the meantime, that would be helpful.

Quoted 51 lines · Manivannan Sadhasivam

Quoted 41 lines · Greg KH

Quoted 32 lines · Thorsten Leemhuis

Quoted 31 lines · Greg KH

Quoted 29 lines · Thorsten Leemhuis

Quoted 16 lines · Will Deacon

Quoted 15 lines · Thorsten Leemhuis

Quoted 13 lines · Amit Pundir

Quoted 12 lines · Catalin Marinas

Quoted 3 lines · Thorsten Leemhuis

Has any progress been made to fix this regression? It afaics is not a release critical issue, but well, it still would be nice to get this fixed before 6.1 is released.

The only (nearly) risk-free "fix" for 6.1 would be to revert the commit that exposed the driver bug. It doesn't fix the actual bug, it only makes it less likely to happen.

I like the original commit removing the cache invalidation as it shows drivers not behaving properly

Quoted 6 lines

Quoted 3 lines

Yeah, I understand that, but I guess it's my job to ask at this point:
"is continuing to live with the old behavior for one or two more cycles" that much of a problem"?

That wouldn't be a problem. The problem is that I haven't see any efforts from the Qualcomm side to actually fix the drivers [...]

Thx for sharing the details. I can fully understand your pain. But well, in the end it looks to me like this commit it intentionally breaking something that used to work -- which to my understanding of the "no regression rule" is not okay, even if things only worked by chance and not flawless.

Quoted 7 lines

Quoted 2 lines

"no regressions" for userspace code, this is broken, out-of-tree driver code, right?

If so: apologies. But that's not the impression I got, as Amit wrote "I can reproduce this crash on vanilla v6.1-rc1 as well with no out-of-tree drivers." here:
https://lore.kernel.org/linux-arm-kernel/CAMi1Hd3H2k1J8hJ6e-Miy5+nVDNzv6qQ3nN-9929B0GbHJkXEg@mail.gmail.com/

Quoted 5 lines

Ah, I missed that.

Ok, what in-tree drivers are having problems being buggy? I can't seem to figure that out from that report at all. Does anyone know?

It is the Qualcomm Q6V5_MSS remoteproc driver:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/remoteproc/qcom_q6v5_mss.c

Quoted 2 lines

Qualcomm is working on the fix but the patches are not ready yet. So if we can get this patch reverted in the meantime, that would be helpful.

It's good to hear that you're working to fix this, even if it's happening behind closed doors. Do you have a rough idea how soon you'll be able to post the remoteproc driver fixes? That would help us to figure out when to bring back the change if we were to revert it.

Cheers,

Will

Quoted 55 lines · Will Deacon

Quoted 53 lines · Manivannan Sadhasivam

Quoted 41 lines · Greg KH

Quoted 32 lines · Thorsten Leemhuis

Quoted 31 lines · Greg KH

Quoted 29 lines · Thorsten Leemhuis

Quoted 16 lines · Will Deacon

Quoted 15 lines · Thorsten Leemhuis

Quoted 13 lines · Amit Pundir

Quoted 12 lines · Catalin Marinas

Quoted 3 lines · Thorsten Leemhuis

Has any progress been made to fix this regression? It afaics is not a release critical issue, but well, it still would be nice to get this fixed before 6.1 is released.

The only (nearly) risk-free "fix" for 6.1 would be to revert the commit that exposed the driver bug. It doesn't fix the actual bug, it only makes it less likely to happen.

I like the original commit removing the cache invalidation as it shows drivers not behaving properly

Quoted 6 lines

Quoted 3 lines

Yeah, I understand that, but I guess it's my job to ask at this point:
"is continuing to live with the old behavior for one or two more cycles" that much of a problem"?

That wouldn't be a problem. The problem is that I haven't see any efforts from the Qualcomm side to actually fix the drivers [...]

Thx for sharing the details. I can fully understand your pain. But well, in the end it looks to me like this commit it intentionally breaking something that used to work -- which to my understanding of the "no regression rule" is not okay, even if things only worked by chance and not flawless.

Quoted 7 lines

Quoted 2 lines

"no regressions" for userspace code, this is broken, out-of-tree driver code, right?

If so: apologies. But that's not the impression I got, as Amit wrote "I can reproduce this crash on vanilla v6.1-rc1 as well with no out-of-tree drivers." here:
https://lore.kernel.org/linux-arm-kernel/CAMi1Hd3H2k1J8hJ6e-Miy5+nVDNzv6qQ3nN-9929B0GbHJkXEg@mail.gmail.com/

Quoted 5 lines

Ah, I missed that.

Ok, what in-tree drivers are having problems being buggy? I can't seem to figure that out from that report at all. Does anyone know?

It is the Qualcomm Q6V5_MSS remoteproc driver:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/remoteproc/qcom_q6v5_mss.c

Qualcomm is working on the fix but the patches are not ready yet. So if we can get this patch reverted in the meantime, that would be helpful.

Quoted 5 lines

It's good to hear that you're working to fix this, even if it's happening behind closed doors. Do you have a rough idea how soon you'll be able to post the remoteproc driver fixes? That would help us to figure out when to bring back the change if we were to revert it.

Sibi is the one working on the fix. I believe he should be able to post the patches within this week.

Quoted 12 lines · Thorsten Leemhuis

Quoted 11 lines · Amit Pundir

Quoted 10 lines · Catalin Marinas

Quoted 3 lines · Thorsten Leemhuis

Has any progress been made to fix this regression? It afaics is not a release critical issue, but well, it still would be nice to get this fixed before 6.1 is released.

The only (nearly) risk-free "fix" for 6.1 would be to revert the commit that exposed the driver bug. It doesn't fix the actual bug, it only makes it less likely to happen.

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

I like the original commit removing the cache invalidation as it shows drivers not behaving properly

Yeah, I understand that, but I guess it's my job to ask at this point:
"is continuing to live with the old behavior for one or two more cycles" that much of a problem"?

That wouldn't be a problem. The problem is that I haven't see any efforts from the Qualcomm side to actually fix the drivers so what makes you think the issue will be addressed in one or two more cycles? On the other hand, if there were patches out there trying to fix the drivers and we just needed to revert this change to buy them some time, then that would obviously be the right thing to do.

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

but, as a workaround, we could add a
command line option to force back the old behaviour (defaulting to the new one) until the driver is fixed.

Well, sometimes that approach is fine to fix a regression, but I'm not sure this is one of those situations, as this...

Quoted 7 lines

Quoted 4 lines

We use DB845c extensively for mainline and android-mainline[1] testing with AOSP, and it is broken for weeks now. So be it a temporary workaround or a proper driver fix in place, we'd really appreciate a quick fix here.

...doesn't sound like we are not talking about some odd corner case here. But in the end that would be up to Linus to decide.

The issue is that these drivers are abusing the DMA API to manage buffers which are being transferred to trustzone. Even with the revert, this is broken (the CPU can speculate from the kernel's cacheable linear mapping of memory), it just appears to be less likely with the CPUs on this SoC. So we end up in a situation where the kernel is flakey on these devices but with even less incentive for the drivers to be fixed.

As well as broken drivers, the patch has also identified broken device-tree files where DMA-coherent devices weher incorrectly being treated as non-coherent:

https://lore.kernel.org/linux-arm-kernel/[email protected]/

so I do think it's something that's worth having as the default behaviour.

Quoted 2 lines

I'll point him to this thread once more in my weekly report anyway. Maybe I'll even suggest to revert this change, not sure yet.

As I said above, I think the revert makes sense if the drivers are actually being fixed, but I'm not seeing any movement at all on that front.

Will

This reverts commit c44094eee32f32f175aadc0efcac449d99b1bbf7.

As reported by Amit [1], dropping cache invalidation from
arch_dma_prep_coherent() triggers a crash on the Qualcomm SM8250 platform
(most probably on other Qcom platforms too). The reason is, Qcom
qcom_q6v5_mss driver copies the firmware metadata and shares it with modem
for validation. The modem has a secure block (XPU) that will trigger a
whole system crash if the shared memory is accessed by the CPU while modem
is poking at it.
[...]
[1] https://lore.kernel.org/linux-arm-kernel/CAMi1Hd1VBCFhf7+EXWHQWcGy4k=tcyLa7RGiFdprtRnegSG0Mw@mail.gmail.com/

Quoted 12 lines · Manivannan Sadhasivam

This reverts commit c44094eee32f32f175aadc0efcac449d99b1bbf7.

As reported by Amit [1], dropping cache invalidation from
arch_dma_prep_coherent() triggers a crash on the Qualcomm SM8250 platform
(most probably on other Qcom platforms too). The reason is, Qcom
qcom_q6v5_mss driver copies the firmware metadata and shares it with modem
for validation. The modem has a secure block (XPU) that will trigger a
whole system crash if the shared memory is accessed by the CPU while modem
is poking at it.
[...]
[1] https://lore.kernel.org/linux-arm-kernel/CAMi1Hd1VBCFhf7+EXWHQWcGy4k=tcyLa7RGiFdprtRnegSG0Mw@mail.gmail.com/

I have Amit's report on the list of tracked regressions. I also noticed the proposed change "arm64: dts: qcom: sc8280xp: fix PCIe DMA coherency":
https://lore.kernel.org/all/[email protected]/

I have no expertise in this area, but it looked somewhat related to my eyes, so please allow me to quickly ask: is that related or even supposed to fix Amit's regression?

Ciao, Thorsten

This reverts commit c44094eee32f32f175aadc0efcac449d99b1bbf7.

As reported by Amit [1], dropping cache invalidation from arch_dma_prep_coherent() triggers a crash on the Qualcomm SM8250 platform (most probably on other Qcom platforms too).