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