| Message ID | 20220727164346.282407-1-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/ttm: don't leak the ccs state | expand |
> -----Original Message----- > From: Auld, Matthew <matthew.auld@intel.com> > Sent: Wednesday, July 27, 2022 10:14 PM > To: intel-gfx@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org; Thomas Hellström <thomas.hellstrom@linux.intel.com>; C, > Ramalingam <ramalingam.c@intel.com> > Subject: [PATCH] drm/i915/ttm: don't leak the ccs state > > The kernel only manages the ccs state with lmem-only objects, however the kernel should still take > care not to leak the CCS state from the previous user. > > Fixes: 48760ffe923a ("drm/i915/gt: Clear compress metadata for Flat-ccs objects") > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Cc: Ramalingam C <ramalingam.c@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_migrate.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c b/drivers/gpu/drm/i915/gt/intel_migrate.c > index a69b244f14d0..9a0814422ba4 100644 > --- a/drivers/gpu/drm/i915/gt/intel_migrate.c > +++ b/drivers/gpu/drm/i915/gt/intel_migrate.c > @@ -708,7 +708,7 @@ intel_context_migrate_copy(struct intel_context *ce, > u8 src_access, dst_access; > struct i915_request *rq; > int src_sz, dst_sz; > - bool ccs_is_src; > + bool ccs_is_src, overwrite_ccs; > int err; > > GEM_BUG_ON(ce->vm != ce->engine->gt->migrate.context->vm); > @@ -749,6 +749,8 @@ intel_context_migrate_copy(struct intel_context *ce, > get_ccs_sg_sgt(&it_ccs, bytes_to_cpy); > } > > + overwrite_ccs = HAS_FLAT_CCS(i915) && !ccs_bytes_to_cpy && > +dst_is_lmem; > + > src_offset = 0; > dst_offset = CHUNK_SZ; > if (HAS_64K_PAGES(ce->engine->i915)) { @@ -852,6 +854,25 @@ > intel_context_migrate_copy(struct intel_context *ce, > if (err) > goto out_rq; > ccs_bytes_to_cpy -= ccs_sz; > + } else if (overwrite_ccs) { > + err = rq->engine->emit_flush(rq, EMIT_INVALIDATE); > + if (err) > + goto out_rq; > + > + /* > + * While we can't always restore/manage the CCS state, > + * we still need to ensure we don't leak the CCS state > + * from the previous user, so make sure we overwrite it > + * with something. > + */ > + err = emit_copy_ccs(rq, dst_offset, INDIRECT_ACCESS, > + dst_offset, DIRECT_ACCESS, len); > + if (err) > + goto out_rq; > + > + err = rq->engine->emit_flush(rq, EMIT_INVALIDATE); > + if (err) > + goto out_rq; The change is looking good to the purpose. But shouldn't this be the part of lmem allocation itself? Ram. > } > > /* Arbitration is re-enabled between requests. */ > -- > 2.37.1
On 28/07/2022 00:16, C, Ramalingam wrote: >> -----Original Message----- >> From: Auld, Matthew <matthew.auld@intel.com> >> Sent: Wednesday, July 27, 2022 10:14 PM >> To: intel-gfx@lists.freedesktop.org >> Cc: dri-devel@lists.freedesktop.org; Thomas Hellström <thomas.hellstrom@linux.intel.com>; C, >> Ramalingam <ramalingam.c@intel.com> >> Subject: [PATCH] drm/i915/ttm: don't leak the ccs state >> >> The kernel only manages the ccs state with lmem-only objects, however the kernel should still take >> care not to leak the CCS state from the previous user. >> >> Fixes: 48760ffe923a ("drm/i915/gt: Clear compress metadata for Flat-ccs objects") >> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> Cc: Ramalingam C <ramalingam.c@intel.com> >> --- >> drivers/gpu/drm/i915/gt/intel_migrate.c | 23 ++++++++++++++++++++++- >> 1 file changed, 22 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c b/drivers/gpu/drm/i915/gt/intel_migrate.c >> index a69b244f14d0..9a0814422ba4 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_migrate.c >> +++ b/drivers/gpu/drm/i915/gt/intel_migrate.c >> @@ -708,7 +708,7 @@ intel_context_migrate_copy(struct intel_context *ce, >> u8 src_access, dst_access; >> struct i915_request *rq; >> int src_sz, dst_sz; >> - bool ccs_is_src; >> + bool ccs_is_src, overwrite_ccs; >> int err; >> >> GEM_BUG_ON(ce->vm != ce->engine->gt->migrate.context->vm); >> @@ -749,6 +749,8 @@ intel_context_migrate_copy(struct intel_context *ce, >> get_ccs_sg_sgt(&it_ccs, bytes_to_cpy); >> } >> >> + overwrite_ccs = HAS_FLAT_CCS(i915) && !ccs_bytes_to_cpy && >> +dst_is_lmem; >> + >> src_offset = 0; >> dst_offset = CHUNK_SZ; >> if (HAS_64K_PAGES(ce->engine->i915)) { @@ -852,6 +854,25 @@ >> intel_context_migrate_copy(struct intel_context *ce, >> if (err) >> goto out_rq; >> ccs_bytes_to_cpy -= ccs_sz; >> + } else if (overwrite_ccs) { >> + err = rq->engine->emit_flush(rq, EMIT_INVALIDATE); >> + if (err) >> + goto out_rq; >> + >> + /* >> + * While we can't always restore/manage the CCS state, >> + * we still need to ensure we don't leak the CCS state >> + * from the previous user, so make sure we overwrite it >> + * with something. >> + */ >> + err = emit_copy_ccs(rq, dst_offset, INDIRECT_ACCESS, >> + dst_offset, DIRECT_ACCESS, len); >> + if (err) >> + goto out_rq; >> + >> + err = rq->engine->emit_flush(rq, EMIT_INVALIDATE); >> + if (err) >> + goto out_rq; > The change is looking good to the purpose. But shouldn't this be the part of lmem allocation itself? Hmm, what do you mean by the lmem allocation itself? The scenarios I had in mind here were: - { lmem, smem } buffer, object is allocated in smem (like with initial mmap) and then moved to lmem (smem -> lmem). - { lmem, smem } buffer, object is allocated in lmem, but then evicted to smem. Object then moved back to lmem (smem -> lmem). - { lmem, smem} buffer with CPU_ACCESS flag on small-bar system, object is allocated in non-mappable lmem, and them moved to the mappable part of lmem on fault (lmem -> lmem). In all the above cases the CCS state is left uninitialised, AFAICT. > > Ram. >> } >> >> /* Arbitration is re-enabled between requests. */ >> -- >> 2.37.1 >
> -----Original Message----- > From: Auld, Matthew <matthew.auld@intel.com> > Sent: Thursday, July 28, 2022 1:38 PM > To: C, Ramalingam <ramalingam.c@intel.com>; intel-gfx@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org; Thomas Hellström <thomas.hellstrom@linux.intel.com> > Subject: Re: [PATCH] drm/i915/ttm: don't leak the ccs state > > On 28/07/2022 00:16, C, Ramalingam wrote: > >> -----Original Message----- > >> From: Auld, Matthew <matthew.auld@intel.com> > >> Sent: Wednesday, July 27, 2022 10:14 PM > >> To: intel-gfx@lists.freedesktop.org > >> Cc: dri-devel@lists.freedesktop.org; Thomas Hellström > >> <thomas.hellstrom@linux.intel.com>; C, Ramalingam > >> <ramalingam.c@intel.com> > >> Subject: [PATCH] drm/i915/ttm: don't leak the ccs state > >> > >> The kernel only manages the ccs state with lmem-only objects, however > >> the kernel should still take care not to leak the CCS state from the previous user. > >> > >> Fixes: 48760ffe923a ("drm/i915/gt: Clear compress metadata for > >> Flat-ccs objects") > >> Signed-off-by: Matthew Auld <matthew.auld@intel.com> > >> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > >> Cc: Ramalingam C <ramalingam.c@intel.com> > >> --- > >> drivers/gpu/drm/i915/gt/intel_migrate.c | 23 ++++++++++++++++++++++- > >> 1 file changed, 22 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c > >> b/drivers/gpu/drm/i915/gt/intel_migrate.c > >> index a69b244f14d0..9a0814422ba4 100644 > >> --- a/drivers/gpu/drm/i915/gt/intel_migrate.c > >> +++ b/drivers/gpu/drm/i915/gt/intel_migrate.c > >> @@ -708,7 +708,7 @@ intel_context_migrate_copy(struct intel_context *ce, > >> u8 src_access, dst_access; > >> struct i915_request *rq; > >> int src_sz, dst_sz; > >> - bool ccs_is_src; > >> + bool ccs_is_src, overwrite_ccs; > >> int err; > >> > >> GEM_BUG_ON(ce->vm != ce->engine->gt->migrate.context->vm); > >> @@ -749,6 +749,8 @@ intel_context_migrate_copy(struct intel_context *ce, > >> get_ccs_sg_sgt(&it_ccs, bytes_to_cpy); > >> } > >> > >> + overwrite_ccs = HAS_FLAT_CCS(i915) && !ccs_bytes_to_cpy && > >> +dst_is_lmem; > >> + > >> src_offset = 0; > >> dst_offset = CHUNK_SZ; > >> if (HAS_64K_PAGES(ce->engine->i915)) { @@ -852,6 +854,25 @@ > >> intel_context_migrate_copy(struct intel_context *ce, > >> if (err) > >> goto out_rq; > >> ccs_bytes_to_cpy -= ccs_sz; > >> + } else if (overwrite_ccs) { > >> + err = rq->engine->emit_flush(rq, EMIT_INVALIDATE); > >> + if (err) > >> + goto out_rq; > >> + > >> + /* > >> + * While we can't always restore/manage the CCS state, > >> + * we still need to ensure we don't leak the CCS state > >> + * from the previous user, so make sure we overwrite it > >> + * with something. > >> + */ > >> + err = emit_copy_ccs(rq, dst_offset, INDIRECT_ACCESS, > >> + dst_offset, DIRECT_ACCESS, len); > >> + if (err) > >> + goto out_rq; > >> + > >> + err = rq->engine->emit_flush(rq, EMIT_INVALIDATE); > >> + if (err) > >> + goto out_rq; > > The change is looking good to the purpose. But shouldn't this be the part of lmem allocation itself? > > Hmm, what do you mean by the lmem allocation itself? The scenarios I had in mind here were: > > - { lmem, smem } buffer, object is allocated in smem (like with initial > mmap) and then moved to lmem (smem -> lmem). > > - { lmem, smem } buffer, object is allocated in lmem, but then evicted to smem. Object then moved > back to lmem (smem -> lmem). > > - { lmem, smem} buffer with CPU_ACCESS flag on small-bar system, object is allocated in non- > mappable lmem, and them moved to the mappable part of lmem on fault (lmem -> lmem). > > In all the above cases the CCS state is left uninitialised, AFAICT. As we discussed offline, this will clear the ccs state(old) of the dst lmem in case of migration from smem to lmem of smem+lmem obj. this seems to be right place to fix the info leak of previous ccs state. Reviewed-by: Ramalingam C <ramalingam.c@intel.com> > > > > > Ram. > >> } > >> > >> /* Arbitration is re-enabled between requests. */ > >> -- > >> 2.37.1 > >
diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c b/drivers/gpu/drm/i915/gt/intel_migrate.c index a69b244f14d0..9a0814422ba4 100644 --- a/drivers/gpu/drm/i915/gt/intel_migrate.c +++ b/drivers/gpu/drm/i915/gt/intel_migrate.c @@ -708,7 +708,7 @@ intel_context_migrate_copy(struct intel_context *ce, u8 src_access, dst_access; struct i915_request *rq; int src_sz, dst_sz; - bool ccs_is_src; + bool ccs_is_src, overwrite_ccs; int err; GEM_BUG_ON(ce->vm != ce->engine->gt->migrate.context->vm); @@ -749,6 +749,8 @@ intel_context_migrate_copy(struct intel_context *ce, get_ccs_sg_sgt(&it_ccs, bytes_to_cpy); } + overwrite_ccs = HAS_FLAT_CCS(i915) && !ccs_bytes_to_cpy && dst_is_lmem; + src_offset = 0; dst_offset = CHUNK_SZ; if (HAS_64K_PAGES(ce->engine->i915)) { @@ -852,6 +854,25 @@ intel_context_migrate_copy(struct intel_context *ce, if (err) goto out_rq; ccs_bytes_to_cpy -= ccs_sz; + } else if (overwrite_ccs) { + err = rq->engine->emit_flush(rq, EMIT_INVALIDATE); + if (err) + goto out_rq; + + /* + * While we can't always restore/manage the CCS state, + * we still need to ensure we don't leak the CCS state + * from the previous user, so make sure we overwrite it + * with something. + */ + err = emit_copy_ccs(rq, dst_offset, INDIRECT_ACCESS, + dst_offset, DIRECT_ACCESS, len); + if (err) + goto out_rq; + + err = rq->engine->emit_flush(rq, EMIT_INVALIDATE); + if (err) + goto out_rq; } /* Arbitration is re-enabled between requests. */
The kernel only manages the ccs state with lmem-only objects, however the kernel should still take care not to leak the CCS state from the previous user. Fixes: 48760ffe923a ("drm/i915/gt: Clear compress metadata for Flat-ccs objects") Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> Cc: Ramalingam C <ramalingam.c@intel.com> --- drivers/gpu/drm/i915/gt/intel_migrate.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)