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

Submitted by Huang, Ray on Sept. 25, 2019, 2:38 p.m.

Details

Message ID 1569422279-6609-1-git-send-email-ray.huang@amd.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Huang, Ray Sept. 25, 2019, 2:38 p.m.
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@amd.com>
Co-developed-by: Luben Tuikov <luben.tuikov@amd.com>
Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@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(-)

Patch hide | download patch | download mbox

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

Comments

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@amd.com>

> Co-developed-by: Luben Tuikov <luben.tuikov@amd.com>

> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>

> Reviewed-by: Alex Deucher <alexander.deucher@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() ?

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,
> -----Original Message-----

> From: Koenig, Christian <Christian.Koenig@amd.com>

> Sent: Wednesday, September 25, 2019 10:47 PM

> To: Huang, Ray <Ray.Huang@amd.com>; amd-gfx@lists.freedesktop.org; dri-

> devel@lists.freedesktop.org; Deucher, Alexander

> <Alexander.Deucher@amd.com>

> Cc: Tuikov, Luben <Luben.Tuikov@amd.com>; Liu, Aaron

> <Aaron.Liu@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@amd.com>

> > Co-developed-by: Luben Tuikov <luben.tuikov@amd.com>

> > Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>

> > Reviewed-by: Alex Deucher <alexander.deucher@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?

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,
On 2019-09-25 10:54, Huang, Ray wrote:
>> -----Original Message-----

>> From: Koenig, Christian <Christian.Koenig@amd.com>

>> Sent: Wednesday, September 25, 2019 10:47 PM

>> To: Huang, Ray <Ray.Huang@amd.com>; amd-gfx@lists.freedesktop.org; dri-

>> devel@lists.freedesktop.org; Deucher, Alexander

>> <Alexander.Deucher@amd.com>

>> Cc: Tuikov, Luben <Luben.Tuikov@amd.com>; Liu, Aaron

>> <Aaron.Liu@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@amd.com>

>>> Co-developed-by: Luben Tuikov <luben.tuikov@amd.com>

>>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>

>>> Reviewed-by: Alex Deucher <alexander.deucher@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,

>
Am 25.09.19 um 16:54 schrieb Huang, Ray:
>> -----Original Message-----

>> From: Koenig, Christian <Christian.Koenig@amd.com>

>> Sent: Wednesday, September 25, 2019 10:47 PM

>> To: Huang, Ray <Ray.Huang@amd.com>; amd-gfx@lists.freedesktop.org; dri-

>> devel@lists.freedesktop.org; Deucher, Alexander

>> <Alexander.Deucher@amd.com>

>> Cc: Tuikov, Luben <Luben.Tuikov@amd.com>; Liu, Aaron

>> <Aaron.Liu@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@amd.com>

>>> Co-developed-by: Luben Tuikov <luben.tuikov@amd.com>

>>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>

>>> Reviewed-by: Alex Deucher <alexander.deucher@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().

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

Regards,
Christian.

>

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