On 14/10/2019 16:55, Steven Price wrote:
On 14/10/2019 16:46, Robin Murphy wrote:
On 14/10/2019 16:16, Steven Price wrote:
Pages shared with the GPU are (often) not cache coherent with the CPU so cache maintenance is required to flush the CPU's caches. This was already done when mapping pages on fault, but wasn't previously done when mapping a freshly allocated page.
Fix this by moving the call to dma_map_sg() into mmu_map_sg() ensuring that it is always called when pages are mapped onto the GPU. Since mmu_map_sg() can now fail the code also now has to handle an error return.
Not performing this cache maintenance can cause errors in the GPU output (CPU caches are later flushed and overwrite the GPU output). In theory it also allows the GPU (and by extension user space) to observe the memory contents prior to sanitization.
For the non-heap case, aren't the pages already supposed to be mapped by drm_gem_shmem_get_pages_sgt()?
Hmm, well yes - it looks like it *should* work - but I was definitely seeing cache artefacts until I did this change... let me do some more testing. It's possible that this is actually only affecting buffers imported from another driver. Perhaps it's drm_gem_shmem_prime_import_sg_table() that needs fixing.
Yes this does appear to only affect imported buffers from what I can tell. Looking through the code there is something suspicious in drm_gem_map_dma_buf(). The following "fixes" the problem. But I'm not sure the reasoning behind DMA_ATTR_SKIP_CPU_SYNC being specified - presumably someone though it was a good idea! I'm not sure which driver's responsibility it is to ensure the caches are flushed.
---8<---- diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 0a2316e0e812..1f7353abd255 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -625,7 +625,7 @@ struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach, sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir, - DMA_ATTR_SKIP_CPU_SYNC)) { + 0)) { sg_free_table(sgt); kfree(sgt); sgt = ERR_PTR(-ENOMEM);