<div dir="ltr">I can't help but think that this could be a bit simpler and involve throwing fewer pointers around.<br><div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Dec 8, 2017 at 2:54 AM, <span dir="ltr"><<a href="mailto:kevin.rogovin@intel.com" target="_blank">kevin.rogovin@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Kevin Rogovin <<a href="mailto:kevin.rogovin@intel.com">kevin.rogovin@intel.com</a>><br>
<br>
Signed-off-by: Kevin Rogovin <<a href="mailto:kevin.rogovin@intel.com">kevin.rogovin@intel.com</a>><br>
---<br>
src/mesa/drivers/dri/i965/brw_<wbr>bufmgr.c | 68 ++++++++++++++++++++++++++++++<wbr>+++-<br>
src/mesa/drivers/dri/i965/brw_<wbr>bufmgr.h | 12 ++++++<br>
2 files changed, 79 insertions(+), 1 deletion(-)<br>
<br>
diff --git a/src/mesa/drivers/dri/i965/<wbr>brw_bufmgr.c b/src/mesa/drivers/dri/i965/<wbr>brw_bufmgr.c<br>
index 52b5bf9..7167165 100644<br>
--- a/src/mesa/drivers/dri/i965/<wbr>brw_bufmgr.c<br>
+++ b/src/mesa/drivers/dri/i965/<wbr>brw_bufmgr.c<br>
@@ -367,7 +367,14 @@ retry:<br>
bo->size = bo_size;<br>
bo->idle = true;<br>
<br>
- struct drm_i915_gem_create create = { .size = bo_size };<br>
+ bo->padding.size = 0;<br>
+ bo->padding.value = NULL;<br>
+ bo->padding.tmp = NULL;<br>
+ if (unlikely(INTEL_DEBUG & DEBUG_OUT_OF_BOUND_CHK)) {<br>
+ bo->padding.size = getpagesize();<br></blockquote><div><br></div><div>This is 4096. I think we could just have a single uint32_t padding field which is either 0 or 4096 (More on that later).<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+<br>
+ struct drm_i915_gem_create create = { .size = bo_size + bo->padding.size };<br>
<br>
/* All new BOs we get from the kernel are zeroed, so we don't need to<br>
* worry about that here.<br>
@@ -378,6 +385,31 @@ retry:<br>
goto err;<br>
}<br>
<br>
+ if (unlikely(bo->padding.size > 0)) {<br>
+ struct drm_i915_gem_pwrite pwrite;<br>
+<br>
+ bo->padding.value = calloc(bo->padding.size, 1);<br>
+ bo->padding.tmp = calloc(bo->padding.size, 1);<br>
+ if (!bo->padding.value || !bo->padding.tmp) {<br>
+ goto err_free;<br>
+ }<br>
+<br>
+ for (uint32_t i = 0; i < bo->padding.size; ++i) {<br>
+ bo->padding.value[i] = rand() & 0xFF;<br></blockquote><div><br></div><div>Does using rand() really help us? Why not just come up with some hash-like thing which generates consistent pseudo-random data? How about something like "value.[i] = i * 853 + 193" (some random primes)? That would mean that we can generate the data and check it without having to store it in a per-bo temporary. If you want it to be magic per-bo, you could also seed it somehow with the bo handle (just add handle * 607).</div><div><br></div><div>If we always allocate 4096B of padding, then you don't need to heap allocate it and can just put it on the stack for the purpose of interacting with pread/pwrite. It's a bit big but still perfectly reasonable. If a day came when we wanted to make the padding size adjustable, it would still probably be reasonable to make the heap allocations temporary so we have less random stuff in the BO we have to cleanup.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+<br>
+ pwrite.handle = create.handle;<br>
+ pwrite.pad = 0;<br>
+ pwrite.offset = bo_size;<br>
+ pwrite.size = bo->padding.size;<br>
+ pwrite.data_ptr = (__u64) (uintptr_t) bo->padding.value;<br>
+ ret = drmIoctl(bufmgr->fd, DRM_IOCTL_I915_GEM_PWRITE, &pwrite);<br></blockquote><div><br></div><div>There's a part of me that wants to kill pread/write. However, I think you may have come up with the only good use of it I've ever seen. :-)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ if (ret != 0) {<br>
+ goto err_free;<br>
+ }<br>
+ }<br>
+<br>
bo->gem_handle = create.handle;<br>
<br>
bo->bufmgr = bufmgr;<br>
@@ -424,6 +456,26 @@ err:<br>
return NULL;<br>
}<br>
<br>
+bool<br>
+brw_bo_padding_is_good(struct brw_bo *bo)<br>
+{<br>
+ if (bo->padding.size > 0) {<br>
+ struct drm_i915_gem_pread pread;<br>
+ int ret;<br>
+<br>
+ assert(bo->padding.tmp && bo->padding.value);<br>
+ pread.handle = bo->gem_handle;<br>
+ pread.pad = 0;<br>
+ pread.offset = bo->size;<br>
+ pread.size = bo->padding.size;<br>
+ pread.data_ptr = (__u64) (uintptr_t) bo->padding.tmp;<br>
+ ret = drmIoctl(bo->bufmgr->fd, DRM_IOCTL_I915_GEM_PREAD, &pread);<br>
+ assert(ret == 0);<br>
+ return memcmp(bo->padding.tmp, bo->padding.value, bo->padding.size) == 0;<br>
+ }<br>
+ return true;<br>
+}<br>
+<br>
struct brw_bo *<br>
brw_bo_alloc(struct brw_bufmgr *bufmgr,<br>
const char *name, uint64_t size, uint64_t alignment)<br>
@@ -598,6 +650,17 @@ bo_free(struct brw_bo *bo)<br>
DBG("DRM_IOCTL_GEM_CLOSE %d failed (%s): %s\n",<br>
bo->gem_handle, bo->name, strerror(errno));<br>
}<br>
+<br>
+ if (unlikely(INTEL_DEBUG & DEBUG_OUT_OF_BOUND_CHK)) {<br>
+ if (bo->padding.value) {<br>
+ free(bo->padding.value);<br></blockquote><div><br></div><div>free will happily ignore NULL pointers. The check is redundant.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+<br>
+ if (bo->padding.tmp) {<br>
+ free(bo->padding.tmp);<br>
+ }<br></blockquote><div><br></div><div>If we still keep these heap allocations, deleting them should be keyed off of bo->padding.size or nothing at all.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+<br>
free(bo);<br>
}<br>
<br>
@@ -1156,6 +1219,9 @@ brw_bo_gem_create_from_prime(<wbr>struct brw_bufmgr *bufmgr, int prime_fd)<br>
bo->name = "prime";<br>
bo->reusable = false;<br>
bo->external = true;<br>
+ bo->padding.size = 0;<br>
+ bo->padding.value = NULL;<br>
+ bo->padding.tmp = NULL;<br>
<br>
struct drm_i915_gem_get_tiling get_tiling = { .handle = bo->gem_handle };<br>
if (drmIoctl(bufmgr->fd, DRM_IOCTL_I915_GEM_GET_TILING, &get_tiling))<br>
diff --git a/src/mesa/drivers/dri/i965/<wbr>brw_bufmgr.h b/src/mesa/drivers/dri/i965/<wbr>brw_bufmgr.h<br>
index 0ae541c..4fff866 100644<br>
--- a/src/mesa/drivers/dri/i965/<wbr>brw_bufmgr.h<br>
+++ b/src/mesa/drivers/dri/i965/<wbr>brw_bufmgr.h<br>
@@ -165,6 +165,16 @@ struct brw_bo {<br>
* Boolean of whether this buffer is cache coherent<br>
*/<br>
bool cache_coherent;<br>
+<br>
+ /**<br>
+ * pointer and size to memory holding values for padding for the<br>
+ * purpose of checking out-of-bound writes.<br>
+ */<br>
+ struct {<br>
+ uint32_t size;<br>
+ uint8_t *value;<br>
+ uint8_t *tmp;<br>
+ } padding;<br>
};<br>
<br>
#define BO_ALLOC_BUSY (1<<0)<br>
@@ -342,6 +352,8 @@ uint32_t brw_bo_export_gem_handle(<wbr>struct brw_bo *bo);<br>
int brw_reg_read(struct brw_bufmgr *bufmgr, uint32_t offset,<br>
uint64_t *result);<br>
<br>
+bool brw_bo_padding_is_good(struct brw_bo *bo);<br>
+<br>
/** @{ */<br>
<br>
#if defined(__cplusplus)<br>
<span class="HOEnZb"><font color="#888888">--<br>
2.7.4<br>
<br>
______________________________<wbr>_________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
</font></span></blockquote></div><br></div></div></div>