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