anv: Fix a relocation race condition

Previously, we would read the offset from the BO in anv_reloc_list_add
to generate the presumed offset and then again in the caller to compute
the 64-bit address to write into the buffer.  However, if the offset
somehow changed between these two points, the presumed offset would no
longer match the written offset.  This is unlikely to actually ever be a
problem in practice because the presumed offset gets recorded first and
so if the written address is wrong then the presumed offset is almost
certainly wrong and the relocation will trigger.  However, it's much
safer to simply have anv_reloc_list_add return the 64-bit address.

Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>

diff --git a/src/intel/vulkan/anv_batch_chain.c b/src/intel/vulkan/anv_batch_chain.c
index c5362aaebc2bab6aea9b727760c378c3748f8fe2..b64e36c952307710fdbaa28c2f01ad7a095a69cd 100644 (file)
--- a/src/intel/vulkan/anv_batch_chain.c
+++ b/src/intel/vulkan/anv_batch_chain.c
@@ -143,14 +143,21 @@ anv_reloc_list_grow(struct anv_reloc_list *list,
    return VK_SUCCESS;
 }
 
+#define READ_ONCE(x) (*(volatile __typeof__(x) *)&(x))
+
 VkResult
 anv_reloc_list_add(struct anv_reloc_list *list,
                    const VkAllocationCallbacks *alloc,
-                   uint32_t offset, struct anv_bo *target_bo, uint32_t delta)
+                   uint32_t offset, struct anv_bo *target_bo, uint32_t delta,
+                   uint64_t *address_u64_out)
 {
    struct drm_i915_gem_relocation_entry *entry;
    int index;
 
+   uint64_t target_bo_offset = READ_ONCE(target_bo->offset);
+   if (address_u64_out)
+      *address_u64_out = target_bo_offset + delta;
+
    if (target_bo->flags & EXEC_OBJECT_PINNED) {
       if (list->deps == NULL) {
          list->deps = _mesa_pointer_set_create(NULL);
@@ -172,7 +179,7 @@ anv_reloc_list_add(struct anv_reloc_list *list,
    entry->target_handle = target_bo->gem_handle;
    entry->delta = delta;
    entry->offset = offset;
-   entry->presumed_offset = target_bo->offset;
+   entry->presumed_offset = target_bo_offset;
    entry->read_domains = 0;
    entry->write_domain = 0;
    VG(VALGRIND_CHECK_MEM_IS_DEFINED(entry, sizeof(*entry)));
@@ -241,14 +248,16 @@ uint64_t
 anv_batch_emit_reloc(struct anv_batch *batch,
                      void *location, struct anv_bo *bo, uint32_t delta)
 {
+   uint64_t address_u64 = 0;
    VkResult result = anv_reloc_list_add(batch->relocs, batch->alloc,
-                                        location - batch->start, bo, delta);
+                                        location - batch->start, bo, delta,
+                                        &address_u64);
    if (result != VK_SUCCESS) {
       anv_batch_set_error(batch, result);
       return 0;
    }
 
-   return bo->offset + delta;
+   return address_u64;
 }
 
 void

diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index db00494e5a57094081a11aa0884047e32eb813e2..f3f718a4855fa035b54e4705fda675044876a44a 100644 (file)
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -1317,7 +1317,7 @@ void anv_reloc_list_finish(struct anv_reloc_list *list,
 VkResult anv_reloc_list_add(struct anv_reloc_list *list,
                             const VkAllocationCallbacks *alloc,
                             uint32_t offset, struct anv_bo *target_bo,
-                            uint32_t delta);
+                            uint32_t delta, uint64_t *address_u64_out);
 
 struct anv_batch_bo {
    /* Link in the anv_cmd_buffer.owned_batch_bos list */

diff --git a/src/intel/vulkan/genX_blorp_exec.c b/src/intel/vulkan/genX_blorp_exec.c
index 9c754f7318e76eb3583626920e04d21a85f91a14..7ac603abb2885ce185124db8c60630f64ddd21fc 100644 (file)
--- a/src/intel/vulkan/genX_blorp_exec.c
+++ b/src/intel/vulkan/genX_blorp_exec.c
@@ -57,17 +57,17 @@ blorp_surface_reloc(struct blorp_batch *batch, uint32_t ss_offset,
                     struct blorp_address address, uint32_t delta)
 {
    struct anv_cmd_buffer *cmd_buffer = batch->driver_batch;
+   uint64_t address_u64 = 0;
    VkResult result =
       anv_reloc_list_add(&cmd_buffer->surface_relocs, &cmd_buffer->pool->alloc,
-                         ss_offset, address.buffer, address.offset + delta);
+                         ss_offset, address.buffer, address.offset + delta,
+                         &address_u64);
    if (result != VK_SUCCESS)
       anv_batch_set_error(&cmd_buffer->batch, result);
 
    void *dest = anv_block_pool_map(
       &cmd_buffer->device->surface_state_pool.block_pool, ss_offset);
-   uint64_t val = ((struct anv_bo*)address.buffer)->offset + address.offset +
-      delta;
-   write_reloc(cmd_buffer->device, dest, val, false);
+   write_reloc(cmd_buffer->device, dest, address_u64, false);
 }
 
 static uint64_t

diff --git a/src/intel/vulkan/genX_cmd_buffer.c b/src/intel/vulkan/genX_cmd_buffer.c
index a4223bfd1efe9c493003f84a9f9c66f75d05b6c8..f7a4914739407cfe616a64fdc5101f6309655c05 100644 (file)
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -207,7 +207,7 @@ add_surface_reloc(struct anv_cmd_buffer *cmd_buffer,
    VkResult result =
       anv_reloc_list_add(&cmd_buffer->surface_relocs, &cmd_buffer->pool->alloc,
                          state.offset + isl_dev->ss.addr_offset,
-                         addr.bo, addr.offset);
+                         addr.bo, addr.offset, NULL);
    if (result != VK_SUCCESS)
       anv_batch_set_error(&cmd_buffer->batch, result);
 }
@@ -226,7 +226,9 @@ add_surface_state_relocs(struct anv_cmd_buffer *cmd_buffer,
          anv_reloc_list_add(&cmd_buffer->surface_relocs,
                             &cmd_buffer->pool->alloc,
                             state.state.offset + isl_dev->ss.aux_addr_offset,
-                            state.aux_address.bo, state.aux_address.offset);
+                            state.aux_address.bo,
+                            state.aux_address.offset,
+                            NULL);
       if (result != VK_SUCCESS)
          anv_batch_set_error(&cmd_buffer->batch, result);
    }
@@ -237,7 +239,9 @@ add_surface_state_relocs(struct anv_cmd_buffer *cmd_buffer,
                             &cmd_buffer->pool->alloc,
                             state.state.offset +
                             isl_dev->ss.clear_color_state_offset,
-                            state.clear_address.bo, state.clear_address.offset);
+                            state.clear_address.bo,
+                            state.clear_address.offset,
+                            NULL);
       if (result != VK_SUCCESS)
          anv_batch_set_error(&cmd_buffer->batch, result);
    }