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

[Koenig, Christian]

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 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)

If that is possible then this would indeed be a solution for the problem.

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