[Intel-gfx] [PATCH 2/2] drm/i915/gt: Always try to reserve GGTT address 0x0

[Chris Wilson]

Quoting Matthew Auld (2021-01-25 11:28:16)
> On Sun, 24 Jan 2021 at 13:54, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> >
> > Since writing to address 0 is a very common mistake, let's try to avoid
> > putting anything sensitive there.
> >
> > References: https://gitlab.freedesktop.org/drm/intel/-/issues/2989
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_ggtt.c | 40 +++++++++++++++++++---------
> >  1 file changed, 28 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > index dac07d66f658..3a737d4fbc3c 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > @@ -535,16 +535,32 @@ static int init_ggtt(struct i915_ggtt *ggtt)
> >
> >         mutex_init(&ggtt->error_mutex);
> >         if (ggtt->mappable_end) {
> > -               /* Reserve a mappable slot for our lockless error capture */
> > -               ret = drm_mm_insert_node_in_range(&ggtt->vm.mm,
> > -                                                 &ggtt->error_capture,
> > -                                                 PAGE_SIZE, 0,
> > -                                                 I915_COLOR_UNEVICTABLE,
> > -                                                 0, ggtt->mappable_end,
> > -                                                 DRM_MM_INSERT_LOW);
> > -               if (ret)
> > -                       return ret;
> > +               /*
> > +                * Reserve a mappable slot for our lockless error capture.
> > +                *
> > +                * We strongly prefer taking address 0x0 in order to protect
> > +                * other critical buffers against accidental overwrites,
> > +                * as writing to address 0 is a very common mistake.
> > +                *
> > +                * Since 0 may already be in use by the system (e.g. the BIOS
> > +                * framebuffer), we let the reservation fail quietly and hope
> > +                * 0 remains reserved always.
> > +                */
> > +               ggtt->error_capture.size = I915_GTT_PAGE_SIZE;
> > +               ggtt->error_capture.color = I915_COLOR_UNEVICTABLE;
> > +               if (drm_mm_reserve_node(&ggtt->vm.mm, &ggtt->error_capture))
> > +                       drm_mm_insert_node_in_range(&ggtt->vm.mm,
> > +                                                   &ggtt->error_capture,
> > +                                                   ggtt->error_capture.size, 0,
> > +                                                   ggtt->error_capture.color,
> > +                                                   0, ggtt->mappable_end,
> > +                                                   DRM_MM_INSERT_LOW);
> 
> We don't want to check the in_range for an error? It should be
> impossible I guess?

For the purpose of error-capture, we will survive without. (I guess we
should include a note in the error capture as to which path was used to
retrieve the data.) Far better to risk error capture not being fully
functional than aborting the driver load. (Although what could go wrong
here... A driver bug?)

For the secondary purpose of preventing use of 0x0, if the reservation
fails, then we know something is at 0x0 and we have to trust that it
remains in place preventing our user.
-Chris