[PATCH] drm/ttm: Make sure the mapped tt pages are decrypted when needed

[PATCH] drm/ttm: Make sure the mapped tt pages are decrypted when needed

[Zack Rusin]

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.

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      |  7 +++++++
 include/drm/ttm/ttm_tt.h          |  9 ++++++++-
 3 files changed, 26 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..02dcb728e29c 100644
--- a/drivers/gpu/drm/ttm/ttm_tt.c
+++ b/drivers/gpu/drm/ttm/ttm_tt.c
@@ -81,6 +81,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))
+		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;
-- 
2.39.2

Re: [PATCH] drm/ttm: Make sure the mapped tt pages are decrypted when needed

[kernel test robot]

Hi Zack,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.6-rc3 next-20230926]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Zack-Rusin/drm-ttm-Make-sure-the-mapped-tt-pages-are-decrypted-when-needed/20230926-120619
base:   linus/master
patch link:    https://lore.kernel.org/r/20230926040359.3040017-1-zack%40kde.org
patch subject: [PATCH] drm/ttm: Make sure the mapped tt pages are decrypted when needed
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20230926/202309261923.XeaDU2Wg-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230926/202309261923.XeaDU2Wg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309261923.XeaDU2Wg-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpu/drm/ttm/ttm_tt.c: In function 'ttm_tt_create':
>> drivers/gpu/drm/ttm/ttm_tt.c:89:41: error: implicit declaration of function 'cc_platform_has' [-Werror=implicit-function-declaration]
      89 |         if (bdev->pool.use_dma_alloc && cc_platform_has(CC_ATTR_MEM_ENCRYPT))
         |                                         ^~~~~~~~~~~~~~~
>> drivers/gpu/drm/ttm/ttm_tt.c:89:57: error: 'CC_ATTR_MEM_ENCRYPT' undeclared (first use in this function)
      89 |         if (bdev->pool.use_dma_alloc && cc_platform_has(CC_ATTR_MEM_ENCRYPT))
         |                                                         ^~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/ttm/ttm_tt.c:89:57: note: each undeclared identifier is reported only once for each function it appears in
   cc1: some warnings being treated as errors


vim +/cc_platform_has +89 drivers/gpu/drm/ttm/ttm_tt.c

    56	
    57	/*
    58	 * Allocates a ttm structure for the given BO.
    59	 */
    60	int ttm_tt_create(struct ttm_buffer_object *bo, bool zero_alloc)
    61	{
    62		struct ttm_device *bdev = bo->bdev;
    63		uint32_t page_flags = 0;
    64	
    65		dma_resv_assert_held(bo->base.resv);
    66	
    67		if (bo->ttm)
    68			return 0;
    69	
    70		switch (bo->type) {
    71		case ttm_bo_type_device:
    72			if (zero_alloc)
    73				page_flags |= TTM_TT_FLAG_ZERO_ALLOC;
    74			break;
    75		case ttm_bo_type_kernel:
    76			break;
    77		case ttm_bo_type_sg:
    78			page_flags |= TTM_TT_FLAG_EXTERNAL;
    79			break;
    80		default:
    81			pr_err("Illegal buffer object type\n");
    82			return -EINVAL;
    83		}
    84		/*
    85		 * When using dma_alloc_coherent with memory encryption the
    86		 * mapped TT pages need to be decrypted or otherwise the drivers
    87		 * will end up sending encrypted mem to the gpu.
    88		 */
  > 89		if (bdev->pool.use_dma_alloc && cc_platform_has(CC_ATTR_MEM_ENCRYPT))
    90			page_flags |= TTM_TT_FLAG_DECRYPTED;
    91	
    92		bo->ttm = bdev->funcs->ttm_tt_create(bo, page_flags);
    93		if (unlikely(bo->ttm == NULL))
    94			return -ENOMEM;
    95	
    96		WARN_ON(bo->ttm->page_flags & TTM_TT_FLAG_EXTERNAL_MAPPABLE &&
    97			!(bo->ttm->page_flags & TTM_TT_FLAG_EXTERNAL));
    98	
    99		return 0;
   100	}
   101	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2] drm/ttm: Make sure the mapped tt pages are decrypted when needed

[Zack Rusin]

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))
+		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;
-- 
2.39.2

Re: [PATCH v2] drm/ttm: Make sure the mapped tt pages are decrypted when needed

[Thomas Hellström]

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;

Re: [PATCH v2] drm/ttm: Make sure the mapped tt pages are decrypted when needed

[Thomas Hellström]

On Mon, 2023-10-02 at 10:16 +0200, Thomas Hellström wrote:
> 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.

FWIW, I think that if TTM_TT_FLAG_DECRYPTED is set, it should be
possible to add a debug WARN_ON_ONCE() if the first PTE of the dma
page's kernel virtual address does not use a decrypted pgprot_t. One
way of accessing the PTEs in a platform-generic fashion is
apply_to_page_range().

/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;

Re: [PATCH v2] drm/ttm: Make sure the mapped tt pages are decrypted when needed

[Zack Rusin]

On Mon, 2023-10-02 at 16:27 +0200, Thomas Hellström wrote:
> !! External Email
> 
> On Mon, 2023-10-02 at 10:16 +0200, Thomas Hellström wrote:
> > 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.
> 
> FWIW, I think that if TTM_TT_FLAG_DECRYPTED is set, it should be
> possible to add a debug WARN_ON_ONCE() if the first PTE of the dma
> page's kernel virtual address does not use a decrypted pgprot_t. One
> way of accessing the PTEs in a platform-generic fashion is
> apply_to_page_range().

Good point. 

Another, trivial solution to that problem of possible regression would simply be
introducing:

#define TTM_DEVICE_USE_DMA_ALLOC          BIT(0)
#define TTM_DEVICE_USE_GFP_DMA32          BIT(1)
#define TTM_DEVICE_USE_DECRYPTED_SYS_MEM  BIT(2)

and changing ttm_device_init from:

int ttm_device_init(struct ttm_device *bdev, const struct ttm_device_funcs *funcs,
		    struct device *dev, struct address_space *mapping,
		    struct drm_vma_offset_manager *vma_manager,
		    bool use_dma_alloc, bool use_dma32);
to:
int ttm_device_init(struct ttm_device *bdev, const struct ttm_device_funcs *funcs,
		    struct device *dev, struct address_space *mapping,
		    struct drm_vma_offset_manager *vma_manager,
		    u32 use_flags);

The driver should have a lot clearer picture of whether
TTM_DEVICE_USE_DECRYPTED_SYS_MEM should be used. That change requires porting the
drivers to the new ttm_device_init (which is trivial) but guarantees no regressions
simply by virture of having vmwgfx use TTM_DEVICE_USE_DECRYPTED_SYS_MEM only (at
least initially, I imagine at least qxl would need it as well).

Christian, any thoughts?

z

Re: [PATCH v2] drm/ttm: Make sure the mapped tt pages are decrypted when needed

[Thomas Hellström]

On 10/3/23 06:13, Zack Rusin wrote:
> On Mon, 2023-10-02 at 16:27 +0200, Thomas Hellström wrote:
>> !! External Email
>>
>> On Mon, 2023-10-02 at 10:16 +0200, Thomas Hellström wrote:
>>> 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.
>> FWIW, I think that if TTM_TT_FLAG_DECRYPTED is set, it should be
>> possible to add a debug WARN_ON_ONCE() if the first PTE of the dma
>> page's kernel virtual address does not use a decrypted pgprot_t. One
>> way of accessing the PTEs in a platform-generic fashion is
>> apply_to_page_range().
> Good point.
>
> Another, trivial solution to that problem of possible regression would simply be
> introducing:
>
> #define TTM_DEVICE_USE_DMA_ALLOC          BIT(0)
> #define TTM_DEVICE_USE_GFP_DMA32          BIT(1)
> #define TTM_DEVICE_USE_DECRYPTED_SYS_MEM  BIT(2)
>
> and changing ttm_device_init from:
>
> int ttm_device_init(struct ttm_device *bdev, const struct ttm_device_funcs *funcs,
> 		    struct device *dev, struct address_space *mapping,
> 		    struct drm_vma_offset_manager *vma_manager,
> 		    bool use_dma_alloc, bool use_dma32);
> to:
> int ttm_device_init(struct ttm_device *bdev, const struct ttm_device_funcs *funcs,
> 		    struct device *dev, struct address_space *mapping,
> 		    struct drm_vma_offset_manager *vma_manager,
> 		    u32 use_flags);
>
> The driver should have a lot clearer picture of whether
> TTM_DEVICE_USE_DECRYPTED_SYS_MEM should be used. That change requires porting the
> drivers to the new ttm_device_init (which is trivial) but guarantees no regressions
> simply by virture of having vmwgfx use TTM_DEVICE_USE_DECRYPTED_SYS_MEM only (at
> least initially, I imagine at least qxl would need it as well).

I've been thinking along those lines as well. But the current direction 
appears to be to hide all of the encryption interaction in the dma 
layer, so when / once we do it right, those driver overrides will 
probably cause even more grief.

/Thomas


>
> Christian, any thoughts?
>
> z

[PATCH v3] drm/ttm: Make sure the mapped tt pages are decrypted when needed

[Zack Rusin]

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
v3: Use CC_ATTR_GUEST_MEM_ENCRYPT instead of CC_ATTR_MEM_ENCRYPT to
limit the scope to guests and log when memory decryption is enabled.

Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
Fixes: 3bf3710e3718 ("drm/ttm: Add a generic TTM memcpy move for page-based iomem")
Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
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      | 12 ++++++++++++
 include/drm/ttm/ttm_tt.h          |  9 ++++++++-
 3 files changed, 31 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 d978dc539a9b..578a7c37f00b 100644
--- a/drivers/gpu/drm/ttm/ttm_tt.c
+++ b/drivers/gpu/drm/ttm/ttm_tt.c
@@ -31,11 +31,13 @@
 
 #define pr_fmt(fmt) "[TTM] " fmt
 
+#include <linux/cc_platform.h>
 #include <linux/sched.h>
 #include <linux/shmem_fs.h>
 #include <linux/file.h>
 #include <linux/module.h>
 #include <drm/drm_cache.h>
+#include <drm/drm_device.h>
 #include <drm/drm_util.h>
 #include <drm/ttm/ttm_bo.h>
 #include <drm/ttm/ttm_tt.h>
@@ -61,6 +63,7 @@ static atomic_long_t ttm_dma32_pages_allocated;
 int ttm_tt_create(struct ttm_buffer_object *bo, bool zero_alloc)
 {
 	struct ttm_device *bdev = bo->bdev;
+	struct drm_device *ddev = bo->base.dev;
 	uint32_t page_flags = 0;
 
 	dma_resv_assert_held(bo->base.resv);
@@ -82,6 +85,15 @@ 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_GUEST_MEM_ENCRYPT)) {
+		page_flags |= TTM_TT_FLAG_DECRYPTED;
+		drm_info(ddev, "TT memory decryption enabled.");
+	}
 
 	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;
-- 
2.40.1

Re: [PATCH v3] drm/ttm: Make sure the mapped tt pages are decrypted when needed

[Zack Rusin]

On Fri, Jan 5, 2024 at 8:51 AM Zack Rusin <zack.rusin@broadcom.com> wrote:
>
> 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
> v3: Use CC_ATTR_GUEST_MEM_ENCRYPT instead of CC_ATTR_MEM_ENCRYPT to
> limit the scope to guests and log when memory decryption is enabled.

Sorry, this also got a bit lost during the s/VMware/Broadcom/
transition. It seems to be pretty safe in general now. I wasn't able
to find a really clean way of adding a warn_once when pte's don't
match as suggested by Thomas, but I did add a quick log to at least
point out in the logs that we've enabled memory decryption in tt.

z

Re: [PATCH v3] drm/ttm: Make sure the mapped tt pages are decrypted when needed

[Zack Rusin]

On Fri, Jan 5, 2024 at 8:51 AM Zack Rusin <zack.rusin@broadcom.com> wrote:
>
> 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
> v3: Use CC_ATTR_GUEST_MEM_ENCRYPT instead of CC_ATTR_MEM_ENCRYPT to
> limit the scope to guests and log when memory decryption is enabled.

Hi, Christian.

Gentle ping on that one. This is probably the cleanest we can get this
code. Can we land this or is there anything else you'd like to see?

z

Re: [PATCH v3] drm/ttm: Make sure the mapped tt pages are decrypted when needed

[Christian König]

Am 26.01.24 um 06:10 schrieb Zack Rusin:
> On Fri, Jan 5, 2024 at 8:51 AM Zack Rusin <zack.rusin@broadcom.com> wrote:
>> 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
>> v3: Use CC_ATTR_GUEST_MEM_ENCRYPT instead of CC_ATTR_MEM_ENCRYPT to
>> limit the scope to guests and log when memory decryption is enabled.
> Hi, Christian.
>
> Gentle ping on that one. This is probably the cleanest we can get this
> code. Can we land this or is there anything else you'd like to see?

Sorry for the delay.

I'm not too familiar with the technical background, so I can't 100% 
judge if this is correct or not.

But if it works for you I think we should give it a try, feel free to 
add my Acked-by and push upstream through whatever branch you like.

Regards,
Christian.

>
> z