Am 24.09.19 um 13:48 schrieb Huang, Ray:
>> -----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);
That won't work correctly.
Binding and unbinding the ttm_mem_reg from the GART is separate to the
BO. E.g. the BO can long be destroyed or it could be a ghost BO.
>
> 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.
The whole approach of adding the TMZ flag to amdgpu_ttm_tt_pte_flags()
is completely broken by design. Rather add adding the flag to
amdgpu_vm_bo_update() instead.
Regards,
Christian.
>
> 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
