Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <2b5648aa-f83d-d8f7-b0fd-39c859f32f33@linux.intel.com>
Date:   Mon, 2 Oct 2023 10:16:40 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v2] drm/ttm: Make sure the mapped tt pages are decrypted
 when needed
To:     Zack Rusin <zackr@vmware.com>, dri-devel@lists.freedesktop.org
Cc:     =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        Huang Rui <ray.huang@amd.com>, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org
References: <20230926040359.3040017-1-zack@kde.org>
 <20230926175113.679880-1-zack@kde.org>
Content-Language: en-US
From:   =?UTF-8?Q?Thomas_Hellstr=c3=b6m?= 
        <thomas.hellstrom@linux.intel.com>
In-Reply-To: <20230926175113.679880-1-zack@kde.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi, Zack

On 9/26/23 19:51, Zack Rusin wrote:
> From: Zack Rusin <zackr@vmware.com>
>
> Some drivers require the mapped tt pages to be decrypted. In an ideal
> world this would have been handled by the dma layer, but the TTM page
> fault handling would have to be rewritten to able to do that.
>
> A side-effect of the TTM page fault handling is using a dma allocation
> per order (via ttm_pool_alloc_page) which makes it impossible to just
> trivially use dma_mmap_attrs. As a result ttm has to be very careful
> about trying to make its pgprot for the mapped tt pages match what
> the dma layer thinks it is. At the ttm layer it's possible to
> deduce the requirement to have tt pages decrypted by checking
> whether coherent dma allocations have been requested and the system
> is running with confidential computing technologies.
>
> This approach isn't ideal but keeping TTM matching DMAs expectations
> for the page properties is in general fragile, unfortunately proper
> fix would require a rewrite of TTM's page fault handling.
>
> Fixes vmwgfx with SEV enabled.
>
> v2: Explicitly include cc_platform.h
>
> Signed-off-by: Zack Rusin <zackr@vmware.com>
> Fixes: 3bf3710e3718 ("drm/ttm: Add a generic TTM memcpy move for page-based iomem")
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Huang Rui <ray.huang@amd.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Cc: <stable@vger.kernel.org> # v5.14+
> ---
>   drivers/gpu/drm/ttm/ttm_bo_util.c | 13 +++++++++++--
>   drivers/gpu/drm/ttm/ttm_tt.c      |  8 ++++++++
>   include/drm/ttm/ttm_tt.h          |  9 ++++++++-
>   3 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index fd9fd3d15101..0b3f4267130c 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -294,7 +294,13 @@ pgprot_t ttm_io_prot(struct ttm_buffer_object *bo, struct ttm_resource *res,
>   	enum ttm_caching caching;
>   
>   	man = ttm_manager_type(bo->bdev, res->mem_type);
> -	caching = man->use_tt ? bo->ttm->caching : res->bus.caching;
> +	if (man->use_tt) {
> +		caching = bo->ttm->caching;
> +		if (bo->ttm->page_flags & TTM_TT_FLAG_DECRYPTED)
> +			tmp = pgprot_decrypted(tmp);
> +	} else  {
> +		caching = res->bus.caching;
> +	}
>   
>   	return ttm_prot_from_caching(caching, tmp);
>   }
> @@ -337,6 +343,8 @@ static int ttm_bo_kmap_ttm(struct ttm_buffer_object *bo,
>   		.no_wait_gpu = false
>   	};
>   	struct ttm_tt *ttm = bo->ttm;
> +	struct ttm_resource_manager *man =
> +			ttm_manager_type(bo->bdev, bo->resource->mem_type);
>   	pgprot_t prot;
>   	int ret;
>   
> @@ -346,7 +354,8 @@ static int ttm_bo_kmap_ttm(struct ttm_buffer_object *bo,
>   	if (ret)
>   		return ret;
>   
> -	if (num_pages == 1 && ttm->caching == ttm_cached) {
> +	if (num_pages == 1 && ttm->caching == ttm_cached &&
> +	    !(man->use_tt && (ttm->page_flags & TTM_TT_FLAG_DECRYPTED))) {
>   		/*
>   		 * We're mapping a single page, and the desired
>   		 * page protection is consistent with the bo.
> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
> index e0a77671edd6..e4966e2c988d 100644
> --- a/drivers/gpu/drm/ttm/ttm_tt.c
> +++ b/drivers/gpu/drm/ttm/ttm_tt.c
> @@ -31,6 +31,7 @@
>   
>   #define pr_fmt(fmt) "[TTM] " fmt
>   
> +#include <linux/cc_platform.h>
>   #include <linux/sched.h>
>   #include <linux/shmem_fs.h>
>   #include <linux/file.h>
> @@ -81,6 +82,13 @@ int ttm_tt_create(struct ttm_buffer_object *bo, bool zero_alloc)
>   		pr_err("Illegal buffer object type\n");
>   		return -EINVAL;
>   	}
> +	/*
> +	 * When using dma_alloc_coherent with memory encryption the
> +	 * mapped TT pages need to be decrypted or otherwise the drivers
> +	 * will end up sending encrypted mem to the gpu.
> +	 */
> +	if (bdev->pool.use_dma_alloc && cc_platform_has(CC_ATTR_MEM_ENCRYPT))

You need to use CC_ATTR_GUEST_MEM_ENCRYPT here rather than 
CC_ATTR_MEM_ENCRYPT to avoid touching and breaking the SME case and only 
fix the SEV / SEV-ES case. I'd also hold off the stable inclusion until 
it's completely verified that this doesn't break anything because if it 
does, I suspect all hell will break loose.

With that said, for the functionality

Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

But I think this needs a wider Ack at the ttm / drm level for the 
approach taken.

/Thomas.

> +		page_flags |= TTM_TT_FLAG_DECRYPTED;
>   
>   	bo->ttm = bdev->funcs->ttm_tt_create(bo, page_flags);
>   	if (unlikely(bo->ttm == NULL))
> diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h
> index a4eff85b1f44..2b9d856ff388 100644
> --- a/include/drm/ttm/ttm_tt.h
> +++ b/include/drm/ttm/ttm_tt.h
> @@ -79,6 +79,12 @@ struct ttm_tt {
>   	 *   page_flags = TTM_TT_FLAG_EXTERNAL |
>   	 *		  TTM_TT_FLAG_EXTERNAL_MAPPABLE;
>   	 *
> +	 * TTM_TT_FLAG_DECRYPTED: The mapped ttm pages should be marked as
> +	 * not encrypted. The framework will try to match what the dma layer
> +	 * is doing, but note that it is a little fragile because ttm page
> +	 * fault handling abuses the DMA api a bit and dma_map_attrs can't be
> +	 * used to assure pgprot always matches.
> +	 *
>   	 * TTM_TT_FLAG_PRIV_POPULATED: TTM internal only. DO NOT USE. This is
>   	 * set by TTM after ttm_tt_populate() has successfully returned, and is
>   	 * then unset when TTM calls ttm_tt_unpopulate().
> @@ -87,8 +93,9 @@ struct ttm_tt {
>   #define TTM_TT_FLAG_ZERO_ALLOC		BIT(1)
>   #define TTM_TT_FLAG_EXTERNAL		BIT(2)
>   #define TTM_TT_FLAG_EXTERNAL_MAPPABLE	BIT(3)
> +#define TTM_TT_FLAG_DECRYPTED		BIT(4)
>   
> -#define TTM_TT_FLAG_PRIV_POPULATED	BIT(4)
> +#define TTM_TT_FLAG_PRIV_POPULATED	BIT(5)
>   	uint32_t page_flags;
>   	/** @num_pages: Number of pages in the page array. */
>   	uint32_t num_pages;