From 19bb33c756edee5e3f0fb126895f6ec23e60dd08 Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Wed, 11 Jul 2018 08:36:02 +0100 Subject: drm/i915: Introduce i915_address_space.mutex Add a mutex into struct i915_address_space to be used while operating on the vma and their lists for a particular vm. As this may be called from the shrinker, we taint the mutex with fs_reclaim so that from the start lockdep warns us if we are caught holding the mutex across an allocation. (With such small steps we will eventually rid ourselves of struct_mutex recursion!) Signed-off-by: Chris Wilson Reviewed-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20180711073608.20286-2-chris@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_gem_gtt.c | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'drivers/gpu/drm/i915/i915_gem_gtt.c') diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 210baf3c8d11..502353b9bf84 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -531,6 +531,14 @@ static void vm_free_page(struct i915_address_space *vm, struct page *page) static void i915_address_space_init(struct i915_address_space *vm, struct drm_i915_private *dev_priv) { + /* + * The vm->mutex must be reclaim safe (for use in the shrinker). + * Do a dummy acquire now under fs_reclaim so that any allocation + * attempt holding the lock is immediately reported by lockdep. + */ + mutex_init(&vm->mutex); + i915_gem_shrinker_taints_mutex(&vm->mutex); + GEM_BUG_ON(!vm->total); drm_mm_init(&vm->mm, 0, vm->total); vm->mm.head_node.color = I915_COLOR_UNEVICTABLE; @@ -551,6 +559,8 @@ static void i915_address_space_fini(struct i915_address_space *vm) spin_unlock(&vm->free_pages.lock); drm_mm_takedown(&vm->mm); + + mutex_destroy(&vm->mutex); } static int __setup_page_dma(struct i915_address_space *vm, -- cgit v1.2.3 From 25dda4dabeeb12af5209b0183c788ef2a88dabbe Mon Sep 17 00:00:00 2001 From: Jon Bloomfield Date: Thu, 12 Jul 2018 19:53:10 +0100 Subject: drm/i915/gtt: Add read only pages to gen8_pte_encode We can set a bit inside the ppGTT PTE to indicate a page is read-only; writes from the GPU will be discarded. We can use this to protect pages and in particular support read-only userptr mappings (necessary for importing PROT_READ vma). Signed-off-by: Jon Bloomfield Signed-off-by: Chris Wilson Cc: Joonas Lahtinen Cc: Matthew Auld Reviewed-by: Joonas Lahtinen Reviewed-by: Matthew Auld Link: https://patchwork.freedesktop.org/patch/msgid/20180712185315.3288-1-chris@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_gem_gtt.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) (limited to 'drivers/gpu/drm/i915/i915_gem_gtt.c') diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 502353b9bf84..b74f1ed03bae 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -244,10 +244,13 @@ static void clear_pages(struct i915_vma *vma) } static gen8_pte_t gen8_pte_encode(dma_addr_t addr, - enum i915_cache_level level) + enum i915_cache_level level, + u32 flags) { - gen8_pte_t pte = _PAGE_PRESENT | _PAGE_RW; - pte |= addr; + gen8_pte_t pte = addr | _PAGE_PRESENT | _PAGE_RW; + + if (unlikely(flags & PTE_READ_ONLY)) + pte &= ~_PAGE_RW; switch (level) { case I915_CACHE_NONE: @@ -721,7 +724,7 @@ static void gen8_initialize_pt(struct i915_address_space *vm, struct i915_page_table *pt) { fill_px(vm, pt, - gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC)); + gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC, 0)); } static void gen6_initialize_pt(struct gen6_hw_ppgtt *ppgtt, @@ -869,7 +872,7 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm, unsigned int pte = gen8_pte_index(start); unsigned int pte_end = pte + num_entries; const gen8_pte_t scratch_pte = - gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC); + gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC, 0); gen8_pte_t *vaddr; GEM_BUG_ON(num_entries > pt->used_ptes); @@ -1044,7 +1047,7 @@ gen8_ppgtt_insert_pte_entries(struct i915_hw_ppgtt *ppgtt, enum i915_cache_level cache_level) { struct i915_page_directory *pd; - const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level); + const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, 0); gen8_pte_t *vaddr; bool ret; @@ -1112,7 +1115,7 @@ static void gen8_ppgtt_insert_huge_entries(struct i915_vma *vma, struct sgt_dma *iter, enum i915_cache_level cache_level) { - const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level); + const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, 0); u64 start = vma->node.start; dma_addr_t rem = iter->sg->length; @@ -1578,7 +1581,7 @@ static void gen8_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m) { struct i915_address_space *vm = &ppgtt->vm; const gen8_pte_t scratch_pte = - gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC); + gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC, 0); u64 start = 0, length = ppgtt->vm.total; if (use_4lvl(vm)) { @@ -2461,7 +2464,7 @@ static void gen8_ggtt_insert_page(struct i915_address_space *vm, gen8_pte_t __iomem *pte = (gen8_pte_t __iomem *)ggtt->gsm + (offset >> PAGE_SHIFT); - gen8_set_pte(pte, gen8_pte_encode(addr, level)); + gen8_set_pte(pte, gen8_pte_encode(addr, level, 0)); ggtt->invalidate(vm->i915); } @@ -2474,7 +2477,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm, struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm); struct sgt_iter sgt_iter; gen8_pte_t __iomem *gtt_entries; - const gen8_pte_t pte_encode = gen8_pte_encode(0, level); + const gen8_pte_t pte_encode = gen8_pte_encode(0, level, 0); dma_addr_t addr; gtt_entries = (gen8_pte_t __iomem *)ggtt->gsm; @@ -2542,7 +2545,7 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm, unsigned first_entry = start >> PAGE_SHIFT; unsigned num_entries = length >> PAGE_SHIFT; const gen8_pte_t scratch_pte = - gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC); + gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC, 0); gen8_pte_t __iomem *gtt_base = (gen8_pte_t __iomem *)ggtt->gsm + first_entry; const int max_entries = ggtt_total_entries(ggtt) - first_entry; -- cgit v1.2.3 From 250f8c8140ac0a5e5acb91891d6813f12778b224 Mon Sep 17 00:00:00 2001 From: Jon Bloomfield Date: Thu, 12 Jul 2018 19:53:11 +0100 Subject: drm/i915/gtt: Read-only pages for insert_entries on bdw+ Hook up the flags to allow read-only ppGTT mappings for gen8+ v2: Include a selftest to check that writes to a readonly PTE are dropped v3: Don't duplicate cpu_check() as we can just reuse it, and even worse don't wholesale copy the theory-of-operation comment from igt_ctx_exec without changing it to explain the intention behind the new test! v4: Joonas really likes magic mystery values Signed-off-by: Jon Bloomfield Signed-off-by: Chris Wilson Cc: Joonas Lahtinen Cc: Matthew Auld Reviewed-by: Joonas Lahtinen Reviewed-by: Matthew Auld Link: https://patchwork.freedesktop.org/patch/msgid/20180712185315.3288-2-chris@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_gem_gtt.c | 45 ++++++--- drivers/gpu/drm/i915/i915_gem_gtt.h | 7 +- drivers/gpu/drm/i915/intel_ringbuffer.c | 11 ++- drivers/gpu/drm/i915/selftests/i915_gem_context.c | 112 +++++++++++++++++++++- 4 files changed, 153 insertions(+), 22 deletions(-) (limited to 'drivers/gpu/drm/i915/i915_gem_gtt.c') diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index b74f1ed03bae..734b67f7853c 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -204,7 +204,7 @@ static int ppgtt_bind_vma(struct i915_vma *vma, return err; } - /* Currently applicable only to VLV */ + /* Applicable to VLV, and gen8+ */ pte_flags = 0; if (vma->obj->gt_ro) pte_flags |= PTE_READ_ONLY; @@ -1044,10 +1044,11 @@ gen8_ppgtt_insert_pte_entries(struct i915_hw_ppgtt *ppgtt, struct i915_page_directory_pointer *pdp, struct sgt_dma *iter, struct gen8_insert_pte *idx, - enum i915_cache_level cache_level) + enum i915_cache_level cache_level, + u32 flags) { struct i915_page_directory *pd; - const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, 0); + const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, flags); gen8_pte_t *vaddr; bool ret; @@ -1098,14 +1099,14 @@ gen8_ppgtt_insert_pte_entries(struct i915_hw_ppgtt *ppgtt, static void gen8_ppgtt_insert_3lvl(struct i915_address_space *vm, struct i915_vma *vma, enum i915_cache_level cache_level, - u32 unused) + u32 flags) { struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm); struct sgt_dma iter = sgt_dma(vma); struct gen8_insert_pte idx = gen8_insert_pte(vma->node.start); gen8_ppgtt_insert_pte_entries(ppgtt, &ppgtt->pdp, &iter, &idx, - cache_level); + cache_level, flags); vma->page_sizes.gtt = I915_GTT_PAGE_SIZE; } @@ -1113,9 +1114,10 @@ static void gen8_ppgtt_insert_3lvl(struct i915_address_space *vm, static void gen8_ppgtt_insert_huge_entries(struct i915_vma *vma, struct i915_page_directory_pointer **pdps, struct sgt_dma *iter, - enum i915_cache_level cache_level) + enum i915_cache_level cache_level, + u32 flags) { - const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, 0); + const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, flags); u64 start = vma->node.start; dma_addr_t rem = iter->sg->length; @@ -1231,19 +1233,21 @@ static void gen8_ppgtt_insert_huge_entries(struct i915_vma *vma, static void gen8_ppgtt_insert_4lvl(struct i915_address_space *vm, struct i915_vma *vma, enum i915_cache_level cache_level, - u32 unused) + u32 flags) { struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm); struct sgt_dma iter = sgt_dma(vma); struct i915_page_directory_pointer **pdps = ppgtt->pml4.pdps; if (vma->page_sizes.sg > I915_GTT_PAGE_SIZE) { - gen8_ppgtt_insert_huge_entries(vma, pdps, &iter, cache_level); + gen8_ppgtt_insert_huge_entries(vma, pdps, &iter, cache_level, + flags); } else { struct gen8_insert_pte idx = gen8_insert_pte(vma->node.start); while (gen8_ppgtt_insert_pte_entries(ppgtt, pdps[idx.pml4e++], - &iter, &idx, cache_level)) + &iter, &idx, cache_level, + flags)) GEM_BUG_ON(idx.pml4e >= GEN8_PML4ES_PER_PML4); vma->page_sizes.gtt = I915_GTT_PAGE_SIZE; @@ -1658,6 +1662,9 @@ static struct i915_hw_ppgtt *gen8_ppgtt_create(struct drm_i915_private *i915) 1ULL << 48 : 1ULL << 32; + /* From bdw, there is support for read-only pages in the PPGTT */ + ppgtt->vm.has_read_only = true; + i915_address_space_init(&ppgtt->vm, i915); /* There are only few exceptions for gen >=6. chv and bxt. @@ -2472,7 +2479,7 @@ static void gen8_ggtt_insert_page(struct i915_address_space *vm, static void gen8_ggtt_insert_entries(struct i915_address_space *vm, struct i915_vma *vma, enum i915_cache_level level, - u32 unused) + u32 flags) { struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm); struct sgt_iter sgt_iter; @@ -2480,6 +2487,9 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm, const gen8_pte_t pte_encode = gen8_pte_encode(0, level, 0); dma_addr_t addr; + /* The GTT does not support read-only mappings */ + GEM_BUG_ON(flags & PTE_READ_ONLY); + gtt_entries = (gen8_pte_t __iomem *)ggtt->gsm; gtt_entries += vma->node.start >> PAGE_SHIFT; for_each_sgt_dma(addr, sgt_iter, vma->pages) @@ -2606,13 +2616,14 @@ struct insert_entries { struct i915_address_space *vm; struct i915_vma *vma; enum i915_cache_level level; + u32 flags; }; static int bxt_vtd_ggtt_insert_entries__cb(void *_arg) { struct insert_entries *arg = _arg; - gen8_ggtt_insert_entries(arg->vm, arg->vma, arg->level, 0); + gen8_ggtt_insert_entries(arg->vm, arg->vma, arg->level, arg->flags); bxt_vtd_ggtt_wa(arg->vm); return 0; @@ -2621,9 +2632,9 @@ static int bxt_vtd_ggtt_insert_entries__cb(void *_arg) static void bxt_vtd_ggtt_insert_entries__BKL(struct i915_address_space *vm, struct i915_vma *vma, enum i915_cache_level level, - u32 unused) + u32 flags) { - struct insert_entries arg = { vm, vma, level }; + struct insert_entries arg = { vm, vma, level, flags }; stop_machine(bxt_vtd_ggtt_insert_entries__cb, &arg, NULL); } @@ -2714,7 +2725,7 @@ static int ggtt_bind_vma(struct i915_vma *vma, struct drm_i915_gem_object *obj = vma->obj; u32 pte_flags; - /* Currently applicable only to VLV */ + /* Applicable to VLV (gen8+ do not support RO in the GGTT) */ pte_flags = 0; if (obj->gt_ro) pte_flags |= PTE_READ_ONLY; @@ -3594,6 +3605,10 @@ int i915_ggtt_init_hw(struct drm_i915_private *dev_priv) */ mutex_lock(&dev_priv->drm.struct_mutex); i915_address_space_init(&ggtt->vm, dev_priv); + + /* Only VLV supports read-only GGTT mappings */ + ggtt->vm.has_read_only = IS_VALLEYVIEW(dev_priv); + if (!HAS_LLC(dev_priv) && !USES_PPGTT(dev_priv)) ggtt->vm.mm.color_adjust = i915_gtt_color_adjust; mutex_unlock(&dev_priv->drm.struct_mutex); diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 14e62651010b..2a116a91420b 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -331,7 +331,12 @@ struct i915_address_space { struct list_head unbound_list; struct pagestash free_pages; - bool pt_kmap_wc; + + /* Some systems require uncached updates of the page directories */ + bool pt_kmap_wc:1; + + /* Some systems support read-only mappings for GGTT and/or PPGTT */ + bool has_read_only:1; /* FIXME: Need a more generic return type */ gen6_pte_t (*pte_encode)(dma_addr_t addr, diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index fab83e3c1502..6de88add508a 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1085,6 +1085,7 @@ void intel_ring_unpin(struct intel_ring *ring) static struct i915_vma * intel_ring_create_vma(struct drm_i915_private *dev_priv, int size) { + struct i915_address_space *vm = &dev_priv->ggtt.vm; struct drm_i915_gem_object *obj; struct i915_vma *vma; @@ -1094,10 +1095,14 @@ intel_ring_create_vma(struct drm_i915_private *dev_priv, int size) if (IS_ERR(obj)) return ERR_CAST(obj); - /* mark ring buffers as read-only from GPU side by default */ - obj->gt_ro = 1; + /* + * Mark ring buffers as read-only from GPU side (so no stray overwrites) + * if supported by the platform's GGTT. + */ + if (vm->has_read_only) + obj->gt_ro = 1; - vma = i915_vma_instance(obj, &dev_priv->ggtt.vm, NULL); + vma = i915_vma_instance(obj, vm, NULL); if (IS_ERR(vma)) goto err; diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c index ab2590242033..2e2a3c44b8e9 100644 --- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c +++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c @@ -23,6 +23,7 @@ */ #include "../i915_selftest.h" +#include "i915_random.h" #include "igt_flush_test.h" #include "mock_drm.h" @@ -252,9 +253,9 @@ static int cpu_check(struct drm_i915_gem_object *obj, unsigned int max) } for (; m < DW_PER_PAGE; m++) { - if (map[m] != 0xdeadbeef) { + if (map[m] != STACK_MAGIC) { pr_err("Invalid value at page %d, offset %d: found %x expected %x\n", - n, m, map[m], 0xdeadbeef); + n, m, map[m], STACK_MAGIC); err = -EINVAL; goto out_unmap; } @@ -310,7 +311,7 @@ create_test_object(struct i915_gem_context *ctx, if (err) return ERR_PTR(err); - err = cpu_fill(obj, 0xdeadbeef); + err = cpu_fill(obj, STACK_MAGIC); if (err) { pr_err("Failed to fill object with cpu, err=%d\n", err); @@ -432,6 +433,110 @@ out_unlock: return err; } +static int igt_ctx_readonly(void *arg) +{ + struct drm_i915_private *i915 = arg; + struct drm_i915_gem_object *obj = NULL; + struct drm_file *file; + I915_RND_STATE(prng); + IGT_TIMEOUT(end_time); + LIST_HEAD(objects); + struct i915_gem_context *ctx; + struct i915_hw_ppgtt *ppgtt; + unsigned long ndwords, dw; + int err = -ENODEV; + + /* + * Create a few read-only objects (with the occasional writable object) + * and try to write into these object checking that the GPU discards + * any write to a read-only object. + */ + + file = mock_file(i915); + if (IS_ERR(file)) + return PTR_ERR(file); + + mutex_lock(&i915->drm.struct_mutex); + + ctx = i915_gem_create_context(i915, file->driver_priv); + if (IS_ERR(ctx)) { + err = PTR_ERR(ctx); + goto out_unlock; + } + + ppgtt = ctx->ppgtt ?: i915->mm.aliasing_ppgtt; + if (!ppgtt || !ppgtt->vm.has_read_only) { + err = 0; + goto out_unlock; + } + + ndwords = 0; + dw = 0; + while (!time_after(jiffies, end_time)) { + struct intel_engine_cs *engine; + unsigned int id; + + for_each_engine(engine, i915, id) { + if (!intel_engine_can_store_dword(engine)) + continue; + + if (!obj) { + obj = create_test_object(ctx, file, &objects); + if (IS_ERR(obj)) { + err = PTR_ERR(obj); + goto out_unlock; + } + + obj->gt_ro = prandom_u32_state(&prng); + } + + intel_runtime_pm_get(i915); + err = gpu_fill(obj, ctx, engine, dw); + intel_runtime_pm_put(i915); + if (err) { + pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) in ctx %u [full-ppgtt? %s], err=%d\n", + ndwords, dw, max_dwords(obj), + engine->name, ctx->hw_id, + yesno(!!ctx->ppgtt), err); + goto out_unlock; + } + + if (++dw == max_dwords(obj)) { + obj = NULL; + dw = 0; + } + ndwords++; + } + } + pr_info("Submitted %lu dwords (across %u engines)\n", + ndwords, INTEL_INFO(i915)->num_rings); + + dw = 0; + list_for_each_entry(obj, &objects, st_link) { + unsigned int rem = + min_t(unsigned int, ndwords - dw, max_dwords(obj)); + unsigned int num_writes; + + num_writes = rem; + if (obj->gt_ro) + num_writes = 0; + + err = cpu_check(obj, num_writes); + if (err) + break; + + dw += rem; + } + +out_unlock: + if (igt_flush_test(i915, I915_WAIT_LOCKED)) + err = -EIO; + mutex_unlock(&i915->drm.struct_mutex); + + mock_file_free(i915, file); + return err; +} + static __maybe_unused const char * __engine_name(struct drm_i915_private *i915, unsigned int engines) { @@ -608,6 +713,7 @@ int i915_gem_context_live_selftests(struct drm_i915_private *dev_priv) static const struct i915_subtest tests[] = { SUBTEST(igt_switch_to_kernel_context), SUBTEST(igt_ctx_exec), + SUBTEST(igt_ctx_readonly), }; bool fake_alias = false; int err; -- cgit v1.2.3 From c9e666880de5a1fed04dc412b046916d542b72dd Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Thu, 12 Jul 2018 19:53:12 +0100 Subject: drm/i915/gtt: Disable read-only support under GVT GVT is not propagating the PTE bits, and is always setting the read-write bit, thus breaking read-only support. Signed-off-by: Chris Wilson Cc: Zhenyu Wang Cc: Jon Bloomfield Cc: Joonas Lahtinen Cc: Matthew Auld Reviewed-by: Jon Bloomfield Link: https://patchwork.freedesktop.org/patch/msgid/20180712185315.3288-3-chris@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'drivers/gpu/drm/i915/i915_gem_gtt.c') diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 734b67f7853c..86a9618bab24 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1662,8 +1662,12 @@ static struct i915_hw_ppgtt *gen8_ppgtt_create(struct drm_i915_private *i915) 1ULL << 48 : 1ULL << 32; - /* From bdw, there is support for read-only pages in the PPGTT */ - ppgtt->vm.has_read_only = true; + /* + * From bdw, there is support for read-only pages in the PPGTT. + * + * XXX GVT is not honouring the lack of RW in the PTE bits. + */ + ppgtt->vm.has_read_only = !intel_vgpu_active(i915); i915_address_space_init(&ppgtt->vm, i915); -- cgit v1.2.3 From 3e977ac6179b39faa3c0eda5fce4f00663ae298d Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Thu, 12 Jul 2018 19:53:13 +0100 Subject: drm/i915: Prevent writing into a read-only object via a GGTT mmap If the user has created a read-only object, they should not be allowed to circumvent the write protection by using a GGTT mmapping. Deny it. Also most machines do not support read-only GGTT PTEs, so again we have to reject attempted writes. Fortunately, this is known a priori, so we can at least reject in the call to create the mmap (with a sanity check in the fault handler). v2: Check the vma->vm_flags during mmap() to allow readonly access. v3: Remove VM_MAYWRITE to curtail mprotect() Testcase: igt/gem_userptr_blits/readonly_mmap* Signed-off-by: Chris Wilson Cc: Jon Bloomfield Cc: Joonas Lahtinen Cc: Matthew Auld Cc: David Herrmann Reviewed-by: Matthew Auld #v1 Reviewed-by: Jon Bloomfield Reviewed-by: Joonas Lahtinen Link: https://patchwork.freedesktop.org/patch/msgid/20180712185315.3288-4-chris@chris-wilson.co.uk --- drivers/gpu/drm/drm_gem.c | 9 +++++++++ drivers/gpu/drm/i915/i915_gem.c | 4 ++++ drivers/gpu/drm/i915/i915_gem_gtt.c | 12 +++++++----- drivers/gpu/drm/i915/i915_gem_object.h | 13 ++++++++++++- drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +- drivers/gpu/drm/i915/selftests/i915_gem_context.c | 5 +++-- include/drm/drm_vma_manager.h | 1 + 7 files changed, 37 insertions(+), 9 deletions(-) (limited to 'drivers/gpu/drm/i915/i915_gem_gtt.c') diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 4a16d7b26c89..bf90625df3c5 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1036,6 +1036,15 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) return -EACCES; } + if (node->readonly) { + if (vma->vm_flags & VM_WRITE) { + drm_gem_object_put_unlocked(obj); + return -EINVAL; + } + + vma->vm_flags &= ~VM_MAYWRITE; + } + ret = drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT, vma); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index ed2be33ec58a..1910c66f48e2 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2012,6 +2012,10 @@ vm_fault_t i915_gem_fault(struct vm_fault *vmf) pgoff_t page_offset; int ret; + /* Sanity check that we allow writing into this object */ + if (i915_gem_object_is_readonly(obj) && write) + return VM_FAULT_SIGBUS; + /* We don't use vmf->pgoff since that has the fake offset */ page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 86a9618bab24..3d75f2bb5623 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -206,7 +206,7 @@ static int ppgtt_bind_vma(struct i915_vma *vma, /* Applicable to VLV, and gen8+ */ pte_flags = 0; - if (vma->obj->gt_ro) + if (i915_gem_object_is_readonly(vma->obj)) pte_flags |= PTE_READ_ONLY; vma->vm->insert_entries(vma->vm, vma, cache_level, pte_flags); @@ -2491,8 +2491,10 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm, const gen8_pte_t pte_encode = gen8_pte_encode(0, level, 0); dma_addr_t addr; - /* The GTT does not support read-only mappings */ - GEM_BUG_ON(flags & PTE_READ_ONLY); + /* + * Note that we ignore PTE_READ_ONLY here. The caller must be careful + * not to allow the user to override access to a read only page. + */ gtt_entries = (gen8_pte_t __iomem *)ggtt->gsm; gtt_entries += vma->node.start >> PAGE_SHIFT; @@ -2731,7 +2733,7 @@ static int ggtt_bind_vma(struct i915_vma *vma, /* Applicable to VLV (gen8+ do not support RO in the GGTT) */ pte_flags = 0; - if (obj->gt_ro) + if (i915_gem_object_is_readonly(obj)) pte_flags |= PTE_READ_ONLY; intel_runtime_pm_get(i915); @@ -2769,7 +2771,7 @@ static int aliasing_gtt_bind_vma(struct i915_vma *vma, /* Currently applicable only to VLV */ pte_flags = 0; - if (vma->obj->gt_ro) + if (i915_gem_object_is_readonly(vma->obj)) pte_flags |= PTE_READ_ONLY; if (flags & I915_VMA_LOCAL_BIND) { diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h index c3c6f2e588fb..56e9f00d2c4c 100644 --- a/drivers/gpu/drm/i915/i915_gem_object.h +++ b/drivers/gpu/drm/i915/i915_gem_object.h @@ -141,7 +141,6 @@ struct drm_i915_gem_object { * Is the object to be mapped as read-only to the GPU * Only honoured if hardware has relevant pte bit */ - unsigned long gt_ro:1; unsigned int cache_level:3; unsigned int cache_coherent:2; #define I915_BO_CACHE_COHERENT_FOR_READ BIT(0) @@ -358,6 +357,18 @@ static inline void i915_gem_object_unlock(struct drm_i915_gem_object *obj) reservation_object_unlock(obj->resv); } +static inline void +i915_gem_object_set_readonly(struct drm_i915_gem_object *obj) +{ + obj->base.vma_node.readonly = true; +} + +static inline bool +i915_gem_object_is_readonly(const struct drm_i915_gem_object *obj) +{ + return obj->base.vma_node.readonly; +} + static inline bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj) { diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 6de88add508a..69bd7f697f6d 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1100,7 +1100,7 @@ intel_ring_create_vma(struct drm_i915_private *dev_priv, int size) * if supported by the platform's GGTT. */ if (vm->has_read_only) - obj->gt_ro = 1; + i915_gem_object_set_readonly(obj); vma = i915_vma_instance(obj, vm, NULL); if (IS_ERR(vma)) diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c index 2e2a3c44b8e9..1c92560d35da 100644 --- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c +++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c @@ -487,7 +487,8 @@ static int igt_ctx_readonly(void *arg) goto out_unlock; } - obj->gt_ro = prandom_u32_state(&prng); + if (prandom_u32_state(&prng) & 1) + i915_gem_object_set_readonly(obj); } intel_runtime_pm_get(i915); @@ -518,7 +519,7 @@ static int igt_ctx_readonly(void *arg) unsigned int num_writes; num_writes = rem; - if (obj->gt_ro) + if (i915_gem_object_is_readonly(obj)) num_writes = 0; err = cpu_check(obj, num_writes); diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h index 8758df94e9a0..c7987daeaed0 100644 --- a/include/drm/drm_vma_manager.h +++ b/include/drm/drm_vma_manager.h @@ -41,6 +41,7 @@ struct drm_vma_offset_node { rwlock_t vm_lock; struct drm_mm_node vm_node; struct rb_root vm_files; + bool readonly:1; }; struct drm_vma_offset_manager { -- cgit v1.2.3