From linux-kernel Mon Oct 02 08:16:40 2023 From: =?UTF-8?Q?Thomas_Hellstr=c3=b6m?= Date: Mon, 02 Oct 2023 08:16:40 +0000 To: linux-kernel Subject: Re: [PATCH v2] drm/ttm: Make sure the mapped tt pages are decrypted when needed Message-Id: <2b5648aa-f83d-d8f7-b0fd-39c859f32f33 () linux ! intel ! com> X-MARC-Message: https://marc.info/?l=linux-kernel&m=169623444117771 Hi, Zack On 9/26/23 19:51, Zack Rusin wrote: > From: 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 > > Signed-off-by: Zack Rusin > Fixes: 3bf3710e3718 ("drm/ttm: Add a generic TTM memcpy move for page-based iomem") > Cc: Christian König > Cc: Thomas Hellström > Cc: Huang Rui > Cc: dri-devel@lists.freedesktop.org > Cc: linux-kernel@vger.kernel.org > Cc: # 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 > #include > #include > #include > @@ -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 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;