> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@xxxxxxx>
> Sent: Thursday, September 12, 2019 7:49 PM
> To: Huang, Ray <Ray.Huang@xxxxxxx>
> Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Tuikov, Luben
> <Luben.Tuikov@xxxxxxx>; Liu, Aaron <Aaron.Liu@xxxxxxx>
> Subject: Re: [PATCH 14/14] drm/amdgpu: set TMZ bits in PTEs for secure bo
> (v2)
>
> Am 12.09.19 um 12:27 schrieb Huang, Ray:
> > On Wed, Sep 11, 2019 at 08:13:19PM +0800, Koenig, Christian wrote:
> >> Am 11.09.19 um 13:50 schrieb Huang, Ray:
> >>> From: Alex Deucher <alexander.deucher@xxxxxxx>
> >>>
> >>> If one bo is secure (created with AMDGPU_GEM_CREATE_ENCRYPTED),
> the
> >>> TMZ bits of PTEs that belongs that bo should be set. Then psp is
> >>> able to protect the pages of this bo to avoid the access from an "untrust"
> domain such as CPU.
> >>>
> >>> v1: design and draft the skeletion of tmz bits setting on PTEs
> >>> (Alex)
> >>> v2: return failure once create secure bo on no-tmz platform (Ray)
> >>>
> >>> Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
> >>> Reviewed-by: Huang Rui <ray.huang@xxxxxxx>
> >>> Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
> >>> ---
> >>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 12 +++++++++++-
> >>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 10 ++++++++++
> >>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 5 +++++
> >>> 3 files changed, 26 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >>> index 22eab74..5332104 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> >>> @@ -222,7 +222,8 @@ int amdgpu_gem_create_ioctl(struct drm_device
> *dev, void *data,
> >>> AMDGPU_GEM_CREATE_CPU_GTT_USWC |
> >>> AMDGPU_GEM_CREATE_VRAM_CLEARED |
> >>> AMDGPU_GEM_CREATE_VM_ALWAYS_VALID |
> >>> - AMDGPU_GEM_CREATE_EXPLICIT_SYNC))
> >>> + AMDGPU_GEM_CREATE_EXPLICIT_SYNC |
> >>> + AMDGPU_GEM_CREATE_ENCRYPTED))
> >>>
> >>> return -EINVAL;
> >>>
> >>> @@ -230,6 +231,11 @@ int amdgpu_gem_create_ioctl(struct
> drm_device *dev, void *data,
> >>> if (args->in.domains & ~AMDGPU_GEM_DOMAIN_MASK)
> >>> return -EINVAL;
> >>>
> >>> + if (!adev->tmz.enabled && (flags &
> AMDGPU_GEM_CREATE_ENCRYPTED)) {
> >>> + DRM_ERROR("Cannot allocate secure buffer while tmz is
> disabled\n");
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> /* create a gem object to contain this object in */
> >>> if (args->in.domains & (AMDGPU_GEM_DOMAIN_GDS |
> >>> AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA))
> { @@ -251,6
> >>> +257,10 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev,
> void *data,
> >>> resv = vm->root.base.bo->tbo.resv;
> >>> }
> >>>
> >>> + if (flags & AMDGPU_GEM_CREATE_ENCRYPTED) {
> >>> + /* XXX: pad out alignment to meet TMZ requirements */
> >>> + }
> >>> +
> >>> r = amdgpu_gem_object_create(adev, size, args->in.alignment,
> >>> (u32)(0xffffffff & args->in.domains),
> >>> flags, ttm_bo_type_device, resv, &gobj);
> diff --git
> >>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> >>> index 5a3c177..286e2e2 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> >>> @@ -224,6 +224,16 @@ static inline bool
> amdgpu_bo_explicit_sync(struct amdgpu_bo *bo)
> >>> return bo->flags & AMDGPU_GEM_CREATE_EXPLICIT_SYNC;
> >>> }
> >>>
> >>> +/**
> >>> + * amdgpu_bo_encrypted - return whether the bo is encrypted */
> >>> +static inline bool amdgpu_bo_encrypted(struct amdgpu_bo *bo) {
> >>> + struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> >>> +
> >>> + return adev->tmz.enabled && (bo->flags &
> >>> +AMDGPU_GEM_CREATE_ENCRYPTED);
> >> Checking the adev->tmz.enabled flags should be dropped here.
> >>
> > That's fine. BO should be validated while it is created.
> >
> > But if the BO is created by vmid 0, is this checking needed?
> >
> >>> +}
> >>> +
> >>> bool amdgpu_bo_is_amdgpu_bo(struct ttm_buffer_object *bo);
> >>> void amdgpu_bo_placement_from_domain(struct amdgpu_bo *abo,
> u32
> >>> domain);
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> >>> index 3663655..8f00bb2 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> >>> @@ -1434,6 +1434,8 @@ bool amdgpu_ttm_tt_is_readonly(struct
> ttm_tt *ttm)
> >>> uint64_t amdgpu_ttm_tt_pde_flags(struct ttm_tt *ttm, struct
> ttm_mem_reg *mem)
> >>> {
> >>> uint64_t flags = 0;
> >>> + struct ttm_buffer_object *tbo = ttm_mem_reg_to_bo(mem);
> >>> + struct amdgpu_bo *abo = ttm_to_amdgpu_bo(tbo);
> >> That's a clear NAK. The function is not necessarily called with
> >> &bo->mem, which is also the reason why this function doesn't gets the
> >> BO as parameter.
> >>
> > Do you mean we can revise the below functions to use bo as the
> > parameter instead?
> >
> > uint64_t amdgpu_ttm_tt_pde_flags(struct ttm_tt *ttm, struct amdgpu_bo
> > *bo) uint64_t amdgpu_ttm_tt_pte_flags(struct ttm_tt *ttm, struct
> > amdgpu_bo *bo)
>
> If that is possible then this would indeed be a solution for the problem.
>
Sorry to late response, I was revising the unit test for secure memory few days ago.
Most of cases can be updated to amdgpu_ttm_tt_pte_flags with amdgpu_bo as the parameter except one case in
amdgpu_ttm_backend_bind(). It will be called by ttm_tt_bind() under amdgpu_move_vram_ram(). But ttm_mem_reg is new assigned.
How about using modify the bind callback in ttm_backend_func:
int (*bind) (struct ttm_buffer_object *bo, struct ttm_mem_reg *bo_mem);
Then we can update the following functions as:
uint64_t amdgpu_ttm_tt_pte_flags(struct amdgpu_device *adev, struct ttm_buffer_object *tbo, struct ttm_mem_reg *mem);
uint64_t amdgpu_ttm_tt_pde_flags(struct ttm_buffer_object *tbo, struct ttm_mem_reg *mem);
It looks better than before.
Thanks,
Ray
> Christian.
>
> >
> > Thanks,
> > Ray
> >
> >> Christian.
> >>
> >>>
> >>> if (mem && mem->mem_type != TTM_PL_SYSTEM)
> >>> flags |= AMDGPU_PTE_VALID;
> >>> @@ -1444,6 +1446,9 @@ uint64_t amdgpu_ttm_tt_pde_flags(struct
> ttm_tt *ttm, struct ttm_mem_reg *mem)
> >>> if (ttm->caching_state == tt_cached)
> >>> flags |= AMDGPU_PTE_SNOOPED;
> >>> }
> >>> + if (amdgpu_bo_encrypted(abo)) {
> >>> + flags |= AMDGPU_PTE_TMZ;
> >>> + }
> >>>
> >>> return flags;
> >>> }
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
