diff mbox series

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

Message ID 1568202584-14471-15-git-send-email-ray.huang@amd.com (mailing list archive)
State New, archived
Headers show
Series drm/amdgpu: introduce secure buffer object support (trusted memory zone) | expand

Commit Message

Huang Rui Sept. 11, 2019, 11:50 a.m. UTC 
From: Alex Deucher <alexander.deucher@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@amd.com>
Reviewed-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Huang Rui <ray.huang@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(-)

Comments

Christian König Sept. 11, 2019, 12:13 p.m. UTC | #1 
Am 11.09.19 um 13:50 schrieb Huang, Ray:
> From: Alex Deucher <alexander.deucher@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@amd.com>
> Reviewed-by: Huang Rui <ray.huang@amd.com>
> Signed-off-by: Huang Rui <ray.huang@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.

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

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;
>   }
Huang Rui Sept. 12, 2019, 10:27 a.m. UTC | #2 
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@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@amd.com>
> > Reviewed-by: Huang Rui <ray.huang@amd.com>
> > Signed-off-by: Huang Rui <ray.huang@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;
> >   }
>
Christian König Sept. 12, 2019, 11:48 a.m. UTC | #3 
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@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@amd.com>
>>> Reviewed-by: Huang Rui <ray.huang@amd.com>
>>> Signed-off-by: Huang Rui <ray.huang@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;
>>>    }
Huang Rui Sept. 24, 2019, 11:48 a.m. UTC | #4 
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Thursday, September 12, 2019 7:49 PM
> To: Huang, Ray <Ray.Huang@amd.com>
> Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
> Deucher, Alexander <Alexander.Deucher@amd.com>; Tuikov, Luben
> <Luben.Tuikov@amd.com>; Liu, Aaron <Aaron.Liu@amd.com>
> 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@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@amd.com>
> >>> Reviewed-by: Huang Rui <ray.huang@amd.com>
> >>> Signed-off-by: Huang Rui <ray.huang@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.
> 

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

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.

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;
> >>>    }
Christian König Sept. 24, 2019, 11:55 a.m. UTC | #5 
Am 24.09.19 um 13:48 schrieb Huang, Ray:
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Thursday, September 12, 2019 7:49 PM
>> To: Huang, Ray <Ray.Huang@amd.com>
>> Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
>> Deucher, Alexander <Alexander.Deucher@amd.com>; Tuikov, Luben
>> <Luben.Tuikov@amd.com>; Liu, Aaron <Aaron.Liu@amd.com>
>> 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@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@amd.com>
>>>>> Reviewed-by: Huang Rui <ray.huang@amd.com>
>>>>> Signed-off-by: Huang Rui <ray.huang@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.
>>
> 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;
>>>>>     }
diff mbox series

Patch

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