| Message ID | 20210124135406.28756-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: Remove guard page insertion around unevictable nodes | expand |
On Sun, 24 Jan 2021 at 13:54, Chris Wilson <chris@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@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? Reviewed-by: Matthew Auld <matthew.auld@intel.com> > } > + if (drm_mm_node_allocated(&ggtt->error_capture)) > + drm_dbg(&ggtt->vm.i915->drm, > + "Reserved GGTT:[%llx, %llx] for use by error capture\n", > + ggtt->error_capture.start, > + ggtt->error_capture.start + ggtt->error_capture.size); > > /* > * The upper portion of the GuC address space has a sizeable hole > @@ -557,9 +573,9 @@ static int init_ggtt(struct i915_ggtt *ggtt) > > /* Clear any non-preallocated blocks */ > drm_mm_for_each_hole(entry, &ggtt->vm.mm, hole_start, hole_end) { > - drm_dbg_kms(&ggtt->vm.i915->drm, > - "clearing unused GTT space: [%lx, %lx]\n", > - hole_start, hole_end); > + drm_dbg(&ggtt->vm.i915->drm, > + "clearing unused GTT space: [%lx, %lx]\n", > + hole_start, hole_end); > ggtt->vm.clear_range(&ggtt->vm, hole_start, > hole_end - hole_start); > } > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Matthew Auld (2021-01-25 11:28:16) > On Sun, 24 Jan 2021 at 13:54, Chris Wilson <chris@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@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
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); } + if (drm_mm_node_allocated(&ggtt->error_capture)) + drm_dbg(&ggtt->vm.i915->drm, + "Reserved GGTT:[%llx, %llx] for use by error capture\n", + ggtt->error_capture.start, + ggtt->error_capture.start + ggtt->error_capture.size); /* * The upper portion of the GuC address space has a sizeable hole @@ -557,9 +573,9 @@ static int init_ggtt(struct i915_ggtt *ggtt) /* Clear any non-preallocated blocks */ drm_mm_for_each_hole(entry, &ggtt->vm.mm, hole_start, hole_end) { - drm_dbg_kms(&ggtt->vm.i915->drm, - "clearing unused GTT space: [%lx, %lx]\n", - hole_start, hole_end); + drm_dbg(&ggtt->vm.i915->drm, + "clearing unused GTT space: [%lx, %lx]\n", + hole_start, hole_end); ggtt->vm.clear_range(&ggtt->vm, hole_start, hole_end - hole_start); }
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@chris-wilson.co.uk> --- drivers/gpu/drm/i915/gt/intel_ggtt.c | 40 +++++++++++++++++++--------- 1 file changed, 28 insertions(+), 12 deletions(-)