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@xxxxxxx>
>>> Sent: Wednesday, September 25, 2019 10:47 PM
>>> To: Huang, Ray <Ray.Huang@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-
>>> devel@xxxxxxxxxxxxxxxxxxxxx; Deucher, Alexander
>>> <Alexander.Deucher@xxxxxxx>
>>> Cc: Tuikov, Luben <Luben.Tuikov@xxxxxxx>; Liu, Aaron
>>> <Aaron.Liu@xxxxxxx>
>>> 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@xxxxxxx>
>>>> Co-developed-by: Luben Tuikov <luben.tuikov@xxxxxxx>
>>>> Signed-off-by: Luben Tuikov <luben.tuikov@xxxxxxx>
>>>> Reviewed-by: Alex Deucher <alexander.deucher@xxxxxxx>
>>>> ---
>>>> 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
commit 1d92719a47cc243983e003985d1182e41cc00d4e
Author: Huang Rui <ray.huang@xxxxxxx>
Date: Thu Aug 8 20:05:15 2019 +0800
drm/amdgpu: job is secure iff CS is secure (v3)
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.
Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
Signed-off-by: Luben Tuikov <luben.tuikov@xxxxxxx>
Reviewed-by: Alex Deucher <alexander.deucher@xxxxxxx>
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 49b767b7238f..c18a153b3d2a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -231,6 +231,8 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, union drm_amdgpu_cs
if (ret)
goto free_all_kdata;
+ p->job->secure = cs->in.flags & AMDGPU_CS_FLAGS_SECURE;
+
if (p->ctx->vram_lost_counter != p->job->vram_lost_counter) {
ret = -ECANCELED;
goto free_all_kdata;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
index 9a6dbf3170a0..6e0f97afb030 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
@@ -213,7 +213,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) {
@@ -232,7 +232,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 dc7ee9358dcd..aa0e375062f2 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,
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
