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

[Tuikov, Luben]

On 2019-09-26 3:12 a.m., Koenig, Christian wrote:
> Am 25.09.19 um 16:54 schrieb Huang, Ray:
>>> -----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;
>>>>    	}
>>>>
>>>> +	/**
>>>> +	 * 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?
> 
> You should not add a new parameter, but rather set the member in 
> amdgpu_cs_parser_init() after amdgpu_job_alloc().

I'd also rather have it right after amdgpu_job_alloc().

> Or maybe even better add that into amdgpu_cs_ib_fill(), cause here is 
> where we fill in most of the job description.

I didn't see much interaction between cs and job, it mostly does ib fill.
Only line mentioning job is line circa 946:
	if (parser->job->uf_addr && ring->funcs->no_user_fence)
as well as the fact that cs is not defined in this function.

I think it is cleaner in amdgpu_cs_parses_init() as per your suggestion
above, since cs is locally defined and accessible. The attached sequenced
patch shows this.

Regards,
Luben
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 1d92719a47cc243983e003985d1182e41cc00d4e.patch
Type: text/x-patch
Size: 2475 bytes
Desc: 1d92719a47cc243983e003985d1182e41cc00d4e.patch
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20190926/28842aaa/attachment.bin>