[PATCH v3 10/11] drm/amdgpu: job is secure iff CS is secure (v4)

[Tuikov, Luben]

On 2019-09-25 10:54, Huang, Ray wrote:
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig at amd.com>
>> Sent: Wednesday, September 25, 2019 10:47 PM
>> To: Huang, Ray <Ray.Huang at amd.com>; amd-gfx at lists.freedesktop.org; dri-
>> devel at lists.freedesktop.org; Deucher, Alexander
>> <Alexander.Deucher at amd.com>
>> Cc: Tuikov, Luben <Luben.Tuikov at amd.com>; Liu, Aaron
>> <Aaron.Liu at amd.com>
>> Subject: Re: [PATCH v3 10/11] drm/amdgpu: job is secure iff CS is secure (v4)
>>
>> Am 25.09.19 um 16:38 schrieb Huang, Ray:
>>> Mark a job as secure, if and only if the command submission flag has
>>> the secure flag set.
>>>
>>> v2: fix the null job pointer while in vmid 0 submission.
>>> v3: Context --> Command submission.
>>> v4: filling cs parser with cs->in.flags
>>>
>>> Signed-off-by: Huang Rui <ray.huang at amd.com>
>>> Co-developed-by: Luben Tuikov <luben.tuikov at amd.com>
>>> Signed-off-by: Luben Tuikov <luben.tuikov at amd.com>
>>> Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  3 +++
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 11 ++++++++++-
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  |  4 ++--
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.h |  2 ++
>>>   4 files changed, 17 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index 697e8e5..fd60695 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -485,6 +485,9 @@ struct amdgpu_cs_parser {
>>>   	uint64_t			bytes_moved;
>>>   	uint64_t			bytes_moved_vis;
>>>
>>> +	/* secure cs */
>>> +	bool				is_secure;
>>> +
>>>   	/* user fence */
>>>   	struct amdgpu_bo_list_entry	uf_entry;
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index 51f3db0..9038dc1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -133,6 +133,14 @@ static int amdgpu_cs_parser_init(struct
>> amdgpu_cs_parser *p, union drm_amdgpu_cs
>>>   		goto free_chunk;
>>>   	}
>>>
>>> +	/**

NAK to double-stars flag-pole top here--this is NOT a function comment.
Since you do have an empty line immediately BEFORE this comment,
then you do not need the flag-pole top "/*" to add yet another
semi-empty line. Just start your comment normally:

/* The command submission ...
 *
...
 */
p->is_secure = ...

Regards,
Luben

>>> +	 * The command submission (cs) is a union, so an assignment to
>>> +	 * 'out' is destructive to the cs (at least the first 8
>>> +	 * bytes). For this reason, inquire about the flags before the
>>> +	 * assignment to 'out'.
>>> +	 */
>>> +	p->is_secure = cs->in.flags & AMDGPU_CS_FLAGS_SECURE;
>>> +
>>>   	/* get chunks */
>>>   	chunk_array_user = u64_to_user_ptr(cs->in.chunks);
>>>   	if (copy_from_user(chunk_array, chunk_array_user, @@ -1252,8
>>> +1260,9 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>>>   		p->ctx->preamble_presented = true;
>>>   	}
>>>
>>> -	cs->out.handle = seq;
>>> +	job->secure = p->is_secure;
>>>   	job->uf_sequence = seq;
>>> +	cs->out.handle = seq;
>>
>> At least it is no longer accessing cs->in, but that still looks like the wrong place
>> to initialize the job.
>>
>> Why can't we fill that in directly after amdgpu_job_alloc() ?
> 
> There is not input member that is secure related in amdgpu_job_alloc() except add an one:
>  
> amdgpu_job_alloc(adev, num_ibs, job, vm, secure)
> 
> It looks too much, isn't it?
> 
> Thanks,
> Ray
> 
>>
>> Regards,
>> Christian.
>>
>>>
>>>   	amdgpu_job_free_resources(job);
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>> index e1dc229..cb9b650 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>> @@ -210,7 +210,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring,
>> unsigned num_ibs,
>>>   	if (job && ring->funcs->emit_cntxcntl) {
>>>   		status |= job->preamble_status;
>>>   		status |= job->preemption_status;
>>> -		amdgpu_ring_emit_cntxcntl(ring, status, false);
>>> +		amdgpu_ring_emit_cntxcntl(ring, status, job->secure);
>>>   	}
>>>
>>>   	for (i = 0; i < num_ibs; ++i) {
>>> @@ -229,7 +229,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring,
>> unsigned num_ibs,
>>>   	}
>>>
>>>   	if (ring->funcs->emit_tmz)
>>> -		amdgpu_ring_emit_tmz(ring, false, false);
>>> +		amdgpu_ring_emit_tmz(ring, false, job ? job->secure : false);
>>>
>>>   #ifdef CONFIG_X86_64
>>>   	if (!(adev->flags & AMD_IS_APU))
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
>>> index dc7ee93..aa0e375 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
>>> @@ -63,6 +63,8 @@ struct amdgpu_job {
>>>   	uint64_t		uf_addr;
>>>   	uint64_t		uf_sequence;
>>>
>>> +	/* the job is due to a secure command submission */
>>> +	bool			secure;
>>>   };
>>>
>>>   int amdgpu_job_alloc(struct amdgpu_device *adev, unsigned num_ibs,
>