linux-kernel - Re: [RFC PATCH] drm: disable WC optimization for cache coherent devices on non-x86

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 21 Jan 2019 10:11:04 +0000
From:   "Koenig, Christian" <Christian.Koenig@....com>
To:     Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
CC:     "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "amd-gfx@...ts.freedesktop.org" <amd-gfx@...ts.freedesktop.org>,
        "Deucher, Alexander" <Alexander.Deucher@....com>,
        "Zhou, David(ChunMing)" <David1.Zhou@....com>,
        "Huang, Ray" <Ray.Huang@....com>,
        "Zhang, Jerry" <Jerry.Zhang@....com>,
        "Daenzer, Michel" <Michel.Daenzer@....com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <maxime.ripard@...tlin.com>,
        Sean Paul <sean@...rly.run>,
        Michael Ellerman <mpe@...erman.id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Will Deacon <will.deacon@....com>
Subject: Re: [RFC PATCH] drm: disable WC optimization for cache coherent
 devices on non-x86

Am 21.01.19 um 11:06 schrieb Ard Biesheuvel:
> Currently, the DRM code assumes that PCI devices are always cache
> coherent for DMA, and that this can be selectively overridden for
> some buffers using non-cached mappings on the CPU side and PCIe
> NoSnoop transactions on the bus side.
>
> Whether the NoSnoop part is implemented correctly is highly platform
> specific. Whether it /matters/ if NoSnoop is implemented correctly or
> not is architecture specific: on x86, such transactions are coherent
> with the CPU whether the NoSnoop attribute is honored or not. On other
> architectures, it depends on whether such transactions may allocate in
> caches that are non-coherent with the CPU's uncached mappings.
>
> Bottom line is that we should not rely on this optimization to work
> correctly for cache coherent devices in the general case. On the
> other hand, disabling this optimization for non-coherent devices
> is likely to cause breakage as well, since the driver will assume
> cache coherent PCIe if this optimization is turned off.
>
> So rename drm_arch_can_wc_memory() to drm_device_can_wc_memory(), and
> pass the drm_device pointer into it so we can base the return value
> on whether the device is cache coherent or not if not running on
> X86.
>
> Cc: Christian Koenig <christian.koenig@....com>
> Cc: Alex Deucher <alexander.deucher@....com>
> Cc: David Zhou <David1.Zhou@....com>
> Cc: Huang Rui <ray.huang@....com>
> Cc: Junwei Zhang <Jerry.Zhang@....com>
> Cc: Michel Daenzer <michel.daenzer@....com>
> Cc: David Airlie <airlied@...ux.ie>
> Cc: Daniel Vetter <daniel@...ll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
> Cc: Maxime Ripard <maxime.ripard@...tlin.com>
> Cc: Sean Paul <sean@...rly.run>
> Cc: Michael Ellerman <mpe@...erman.id.au>
> Cc: Benjamin Herrenschmidt <benh@...nel.crashing.org>
> Cc: Will Deacon <will.deacon@....com>
> Reported-by: Carsten Haitzler <Carsten.Haitzler@....com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...aro.org>

Looks sane to me, but I can't fully judge if the check is actually 
correct or not.

So Acked-by: Christian König <christian.koenig@....com>

> ---
> This is a followup to '[RFC PATCH] drm/ttm: force cached mappings for system
> RAM on ARM'
>
> https://lore.kernel.org/linux-arm-kernel/20190110072841.3283-1-ard.biesheuvel@linaro.org/
>
> Without t
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  2 +-
>   drivers/gpu/drm/radeon/radeon_object.c     |  2 +-
>   include/drm/drm_cache.h                    | 19 +++++++++++--------
>   3 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 728e15e5d68a..777fa251838f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -480,7 +480,7 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>   	/* For architectures that don't support WC memory,
>   	 * mask out the WC flag from the BO
>   	 */
> -	if (!drm_arch_can_wc_memory())
> +	if (!drm_device_can_wc_memory(adev->ddev))
>   		bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>   #endif
>   
> diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
> index 833e909706a9..610889bf6ab5 100644
> --- a/drivers/gpu/drm/radeon/radeon_object.c
> +++ b/drivers/gpu/drm/radeon/radeon_object.c
> @@ -249,7 +249,7 @@ int radeon_bo_create(struct radeon_device *rdev,
>   	/* For architectures that don't support WC memory,
>   	 * mask out the WC flag from the BO
>   	 */
> -	if (!drm_arch_can_wc_memory())
> +	if (!drm_device_can_wc_memory(rdev->ddev))
>   		bo->flags &= ~RADEON_GEM_GTT_WC;
>   #endif
>   
> diff --git a/include/drm/drm_cache.h b/include/drm/drm_cache.h
> index bfe1639df02d..ced63b1207a3 100644
> --- a/include/drm/drm_cache.h
> +++ b/include/drm/drm_cache.h
> @@ -33,6 +33,8 @@
>   #ifndef _DRM_CACHE_H_
>   #define _DRM_CACHE_H_
>   
> +#include <drm/drm_device.h>
> +#include <linux/dma-noncoherent.h>
>   #include <linux/scatterlist.h>
>   
>   void drm_clflush_pages(struct page *pages[], unsigned long num_pages);
> @@ -41,15 +43,16 @@ void drm_clflush_virt_range(void *addr, unsigned long length);
>   u64 drm_get_max_iomem(void);
>   
>   
> -static inline bool drm_arch_can_wc_memory(void)
> +static inline bool drm_device_can_wc_memory(struct drm_device *ddev)
>   {
> -#if defined(CONFIG_PPC) && !defined(CONFIG_NOT_COHERENT_CACHE)
> -	return false;
> -#elif defined(CONFIG_MIPS) && defined(CONFIG_CPU_LOONGSON3)
> -	return false;
> -#else
> -	return true;
> -#endif
> +	if (IS_ENABLED(CONFIG_PPC))
> +		return IS_ENABLED(CONFIG_NOT_COHERENT_CACHE);
> +	else if (IS_ENABLED(CONFIG_MIPS))
> +		return !IS_ENABLED(CONFIG_CPU_LOONGSON3);
> +	else if (IS_ENABLED(CONFIG_X86))
> +		return true;
> +
> +	return !dev_is_dma_coherent(ddev->dev);
>   }
>   
>   #endif