[PATCH 14/14] drm/amdgpu: set TMZ bits in PTEs for secure bo (v2)

[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 at amd.com>
> >
> > 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 at amd.com>
> > Reviewed-by: Huang Rui <ray.huang at amd.com>
> > Signed-off-by: Huang Rui <ray.huang at amd.com>
> > ---
> >   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)

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