nv2a: Fix CPU surface access callback race and use-after-free

hw/xbox/nv2a/pgraph/gl/surface.c

@@ -413,16 +413,48 @@ bool pgraph_gl_check_surface_to_texture_compatibility(
     return false;
 }
 
-static void wait_for_surface_download(SurfaceBinding *e)
+static bool check_surface_overlaps_range(const SurfaceBinding *surface,
+                                         hwaddr range_start, hwaddr range_len)
 {
-    NV2AState *d = g_nv2a;
-    PGRAPHState *pg = &d->pgraph;
-    PGRAPHGLState *r = pg->gl_renderer_state;
+    hwaddr surface_end = surface->vram_addr + surface->size;
+    hwaddr range_end = range_start + range_len;
+    return !(surface->vram_addr >= range_end || range_start >= surface_end);
+}
 
-    if (qatomic_read(&e->draw_dirty)) {
+static void surface_access_callback(void *opaque, MemoryRegion *mr, hwaddr addr,
+                                    hwaddr len, bool write)
+{
+    NV2AState *d = (NV2AState *)opaque;
+    qemu_mutex_lock(&d->pgraph.lock);
+
+    PGRAPHGLState *r = d->pgraph.gl_renderer_state;
+    bool wait_for_downloads = false;
+
+    SurfaceBinding *surface;
+    QTAILQ_FOREACH(surface, &r->surfaces, entry) {
+        if (!check_surface_overlaps_range(surface, addr, len)) {
+            continue;
+        }
+
+        hwaddr offset = addr - surface->vram_addr;
+
+        if (surface->draw_dirty) {
+            trace_nv2a_pgraph_surface_cpu_access(surface->vram_addr, offset);
+            surface->download_pending = true;
+            wait_for_downloads = true;
+        }
+
+        if (write && !surface->upload_pending) {
+            trace_nv2a_pgraph_surface_cpu_access(surface->vram_addr, offset);
+            surface->upload_pending = true;
+        }
+    }
+
+    qemu_mutex_unlock(&d->pgraph.lock);
+
+    if (wait_for_downloads) {
         qemu_mutex_lock(&d->pfifo.lock);
         qemu_event_reset(&r->downloads_complete);
-        qatomic_set(&e->download_pending, true);
         qatomic_set(&r->downloads_pending, true);
         pfifo_kick(d);
         qemu_mutex_unlock(&d->pfifo.lock);
@@ -430,22 +462,44 @@ static void wait_for_surface_download(SurfaceBinding *e)
     }
 }
 
-static void surface_access_callback(void *opaque, MemoryRegion *mr, hwaddr addr,
-                                    hwaddr len, bool write)
+static void register_cpu_access_callback(NV2AState *d, SurfaceBinding *surface)
 {
-    SurfaceBinding *e = opaque;
-    assert(addr >= e->vram_addr);
-    hwaddr offset = addr - e->vram_addr;
-    assert(offset < e->size);
-
-    if (qatomic_read(&e->draw_dirty)) {
-        trace_nv2a_pgraph_surface_cpu_access(e->vram_addr, offset);
-        wait_for_surface_download(e);
+    if (tcg_enabled()) {
+        surface->access_cb = mem_access_callback_insert(
+            qemu_get_cpu(0), d->vram, surface->vram_addr, surface->size,
+            &surface_access_callback, d);
     }
+}
 
-    if (write && !qatomic_read(&e->upload_pending)) {
-        trace_nv2a_pgraph_surface_cpu_access(e->vram_addr, offset);
-        qatomic_set(&e->upload_pending, true);
+static void unregister_cpu_access_callback(NV2AState *d,
+                                           SurfaceBinding const *surface)
+{
+    if (tcg_enabled()) {
+        mem_access_callback_remove_by_ref(qemu_get_cpu(0), surface->access_cb);
+    }
+}
+
+static bool check_surfaces_overlap(const SurfaceBinding *surface,
+                                   const SurfaceBinding *other_surface)
+{
+    return check_surface_overlaps_range(surface, other_surface->vram_addr,
+                                        other_surface->size);
+}
+
+static void invalidate_overlapping_surfaces(NV2AState *d, SurfaceBinding *surface)
+{
+    PGRAPHState *pg = &d->pgraph;
+    PGRAPHGLState *r = pg->gl_renderer_state;
+
+    SurfaceBinding *other_surface, *next_surface;
+    QTAILQ_FOREACH_SAFE(other_surface, &r->surfaces, entry, next_surface) {
+        if (check_surfaces_overlap(surface, other_surface)) {
+            trace_nv2a_pgraph_surface_evict_overlapping(
+                other_surface->vram_addr, other_surface->width, other_surface->height,
+                other_surface->pitch);
+            pgraph_gl_surface_download_if_dirty(d, other_surface);
+            pgraph_gl_surface_invalidate(d, other_surface);
+        }
     }
 }
 
@@ -457,35 +511,13 @@ static SurfaceBinding *surface_put(NV2AState *d, hwaddr addr,
 
     assert(pgraph_gl_surface_get(d, addr) == NULL);
 
-    SurfaceBinding *surface, *next;
-    uintptr_t e_end = surface_in->vram_addr + surface_in->size - 1;
-    QTAILQ_FOREACH_SAFE(surface, &r->surfaces, entry, next) {
-        uintptr_t s_end = surface->vram_addr + surface->size - 1;
-        bool overlapping = !(surface->vram_addr > e_end
-                             || surface_in->vram_addr > s_end);
-        if (overlapping) {
-            trace_nv2a_pgraph_surface_evict_overlapping(
-                surface->vram_addr, surface->width, surface->height,
-                surface->pitch);
-            pgraph_gl_surface_download_if_dirty(d, surface);
-            pgraph_gl_surface_invalidate(d, surface);
-        }
-    }
+    invalidate_overlapping_surfaces(d, surface_in);
 
     SurfaceBinding *surface_out = g_malloc(sizeof(SurfaceBinding));
     assert(surface_out != NULL);
     *surface_out = *surface_in;
 
-    if (tcg_enabled()) {
-        qemu_mutex_unlock(&d->pgraph.lock);
-        bql_lock();
-        mem_access_callback_insert(qemu_get_cpu(0),
-            d->vram, surface_out->vram_addr, surface_out->size,
-            &surface_out->access_cb, &surface_access_callback,
-            surface_out);
-        bql_unlock();
-        qemu_mutex_lock(&d->pgraph.lock);
-    }
+    register_cpu_access_callback(d, surface_out);
 
     QTAILQ_INSERT_TAIL(&r->surfaces, surface_out, entry);
 
@@ -539,13 +571,7 @@ void pgraph_gl_surface_invalidate(NV2AState *d, SurfaceBinding *surface)
         pgraph_gl_unbind_surface(d, false);
     }
 
-    if (tcg_enabled()) {
-        qemu_mutex_unlock(&d->pgraph.lock);
-        bql_lock();
-        mem_access_callback_remove_by_ref(qemu_get_cpu(0), surface->access_cb);
-        bql_unlock();
-        qemu_mutex_lock(&d->pgraph.lock);
-    }
+    unregister_cpu_access_callback(d, surface);
 
     glDeleteTextures(1, &surface->gl_buffer);
 

hw/xbox/nv2a/pgraph/pgraph.c

@@ -225,6 +225,8 @@ void pgraph_init(NV2AState *d)
     qemu_event_init(&pg->sync_complete, false);
     qemu_event_init(&pg->flush_complete, false);
     qemu_cond_init(&pg->framebuffer_released);
+    qemu_event_init(&pg->renderer_switch_complete, false);
+    pg->renderer_switch_phase = PGRAPH_RENDERER_SWITCH_PHASE_IDLE;
 
     pg->frame_time = 0;
     pg->draw_time = 0;
@@ -3145,12 +3147,31 @@ void pgraph_write_zpass_pixel_cnt_report(NV2AState *d, uint32_t parameter,
     NV2A_DPRINTF("Report result %d @%" HWADDR_PRIx, result, offset);
 }
 
+static void do_wait_for_renderer_switch(CPUState *cpu, run_on_cpu_data data)
+{
+    NV2AState *d = (NV2AState *)data.host_ptr;
+
+    qemu_mutex_lock(&d->pfifo.lock);
+    d->pgraph.renderer_switch_phase = PGRAPH_RENDERER_SWITCH_PHASE_CPU_WAITING;
+    pfifo_kick(d);
+    qemu_mutex_unlock(&d->pfifo.lock);
+    qemu_event_wait(&d->pgraph.renderer_switch_complete);
+}
+
 void pgraph_process_pending(NV2AState *d)
 {
     PGRAPHState *pg = &d->pgraph;
     pg->renderer->ops.process_pending(d);
 
-    if (g_config.display.renderer != pg->renderer->type) {
+    if (g_config.display.renderer != pg->renderer->type &&
+        pg->renderer_switch_phase == PGRAPH_RENDERER_SWITCH_PHASE_IDLE) {
+        pg->renderer_switch_phase = PGRAPH_RENDERER_SWITCH_PHASE_STARTED;
+        qemu_event_reset(&pg->renderer_switch_complete);
+        async_safe_run_on_cpu(qemu_get_cpu(0), do_wait_for_renderer_switch,
+                              RUN_ON_CPU_HOST_PTR(d));
+    }
+
+    if (pg->renderer_switch_phase == PGRAPH_RENDERER_SWITCH_PHASE_CPU_WAITING) {
         qemu_mutex_lock(&d->pgraph.renderer_lock);
         qemu_mutex_unlock(&d->pfifo.lock);
         qemu_mutex_lock(&d->pgraph.lock);
@@ -3162,14 +3183,13 @@ void pgraph_process_pending(NV2AState *d)
             qemu_mutex_lock(&d->pfifo.lock);
             qemu_mutex_unlock(&d->pgraph.lock);
 
-            if (pg->renderer->ops.process_pending) {
-                pg->renderer->ops.process_pending(d);
-            }
+            pg->renderer->ops.process_pending(d);
 
             qemu_mutex_unlock(&d->pfifo.lock);
             qemu_mutex_lock(&d->pgraph.lock);
             while (pg->framebuffer_in_use) {
-                qemu_cond_wait(&d->pgraph.framebuffer_released, &d->pgraph.renderer_lock);
+                qemu_cond_wait(&d->pgraph.framebuffer_released,
+                               &d->pgraph.renderer_lock);
             }
 
             if (pg->renderer->ops.finalize) {
@@ -3182,6 +3202,9 @@ void pgraph_process_pending(NV2AState *d)
         qemu_mutex_unlock(&d->pgraph.renderer_lock);
         qemu_mutex_unlock(&d->pgraph.lock);
         qemu_mutex_lock(&d->pfifo.lock);
+
+        pg->renderer_switch_phase = PGRAPH_RENDERER_SWITCH_PHASE_IDLE;
+        qemu_event_set(&pg->renderer_switch_complete);
     }
 }
 

hw/xbox/nv2a/pgraph/pgraph.h

@@ -244,6 +244,13 @@ typedef struct PGRAPHState {
     bool framebuffer_in_use;
     QemuCond framebuffer_released;
 
+    enum {
+        PGRAPH_RENDERER_SWITCH_PHASE_IDLE,
+        PGRAPH_RENDERER_SWITCH_PHASE_STARTED,
+        PGRAPH_RENDERER_SWITCH_PHASE_CPU_WAITING,
+    } renderer_switch_phase;
+    QemuEvent renderer_switch_complete;
+
     unsigned int surface_scale_factor;
     uint8_t *scale_buf;
 

hw/xbox/nv2a/pgraph/vk/surface.c

@@ -122,17 +122,22 @@ static void memcpy_image(void *dst, void const *src, int dst_stride,
     }
 }
 
-void pgraph_vk_download_surfaces_in_range_if_dirty(PGRAPHState *pg, hwaddr start, hwaddr size)
+static bool check_surface_overlaps_range(const SurfaceBinding *surface,
+                                         hwaddr range_start, hwaddr range_len)
+{
+    hwaddr surface_end = surface->vram_addr + surface->size;
+    hwaddr range_end = range_start + range_len;
+    return !(surface->vram_addr >= range_end || range_start >= surface_end);
+}
+
+void pgraph_vk_download_surfaces_in_range_if_dirty(PGRAPHState *pg,
+                                                   hwaddr start, hwaddr size)
 {
     PGRAPHVkState *r = pg->vk_renderer_state;
     SurfaceBinding *surface;
 
-    hwaddr end = start + size - 1;
-
     QTAILQ_FOREACH(surface, &r->surfaces, entry) {
-        hwaddr surf_end = surface->vram_addr + surface->size - 1;
-        bool overlapping = !(surface->vram_addr >= end || start >= surf_end);
-        if (overlapping) {
+        if (check_surface_overlaps_range(surface, start, size)) {
             pgraph_vk_surface_download_if_dirty(
                 container_of(pg, NV2AState, pgraph), surface);
         }
@@ -527,33 +532,50 @@ void pgraph_vk_download_dirty_surfaces(NV2AState *d)
 static void surface_access_callback(void *opaque, MemoryRegion *mr, hwaddr addr,
                                     hwaddr len, bool write)
 {
-    SurfaceBinding *e = opaque;
-    assert(addr >= e->vram_addr);
-    hwaddr offset = addr - e->vram_addr;
-    assert(offset < e->size);
+    NV2AState *d = (NV2AState *)opaque;
+    qemu_mutex_lock(&d->pgraph.lock);
 
-    if (qatomic_read(&e->draw_dirty)) {
-        trace_nv2a_pgraph_surface_cpu_access(e->vram_addr, offset);
-        pgraph_vk_wait_for_surface_download(e);
+    PGRAPHVkState *r = d->pgraph.vk_renderer_state;
+    bool wait_for_downloads = false;
+
+    SurfaceBinding *surface;
+    QTAILQ_FOREACH(surface, &r->surfaces, entry) {
+        if (!check_surface_overlaps_range(surface, addr, len)) {
+            continue;
+        }
+
+        hwaddr offset = addr - surface->vram_addr;
+
+        if (surface->draw_dirty) {
+            trace_nv2a_pgraph_surface_cpu_access(surface->vram_addr, offset);
+            surface->download_pending = true;
+            wait_for_downloads = true;
+        }
+
+        if (write && !surface->upload_pending) {
+            trace_nv2a_pgraph_surface_cpu_access(surface->vram_addr, offset);
+            surface->upload_pending = true;
+        }
     }
 
-    if (write && !qatomic_read(&e->upload_pending)) {
-        trace_nv2a_pgraph_surface_cpu_access(e->vram_addr, offset);
-        qatomic_set(&e->upload_pending, true);
+    qemu_mutex_unlock(&d->pgraph.lock);
+
+    if (wait_for_downloads) {
+        qemu_mutex_lock(&d->pfifo.lock);
+        qemu_event_reset(&r->downloads_complete);
+        qatomic_set(&r->downloads_pending, true);
+        pfifo_kick(d);
+        qemu_mutex_unlock(&d->pfifo.lock);
+        qemu_event_wait(&r->downloads_complete);
     }
 }
 
 static void register_cpu_access_callback(NV2AState *d, SurfaceBinding *surface)
 {
     if (tcg_enabled()) {
-        qemu_mutex_unlock(&d->pgraph.lock);
-        bql_lock();
-        mem_access_callback_insert(qemu_get_cpu(0),
-            d->vram, surface->vram_addr, surface->size,
-            &surface->access_cb, &surface_access_callback,
-            surface);
-        bql_unlock();
-        qemu_mutex_lock(&d->pgraph.lock);
+        surface->access_cb = mem_access_callback_insert(
+            qemu_get_cpu(0), d->vram, surface->vram_addr, surface->size,
+            &surface_access_callback, d);
     }
 }
 
@@ -561,11 +583,7 @@ static void unregister_cpu_access_callback(NV2AState *d,
                                            SurfaceBinding const *surface)
 {
     if (tcg_enabled()) {
-        qemu_mutex_unlock(&d->pgraph.lock);
-        bql_lock();
         mem_access_callback_remove_by_ref(qemu_get_cpu(0), surface->access_cb);
-        bql_unlock();
-        qemu_mutex_lock(&d->pgraph.lock);
     }
 }
 
@@ -627,24 +645,26 @@ static void invalidate_surface(NV2AState *d, SurfaceBinding *surface)
     QTAILQ_INSERT_HEAD(&r->invalid_surfaces, surface, entry);
 }
 
+static bool check_surfaces_overlap(const SurfaceBinding *surface,
+                                   const SurfaceBinding *other_surface)
+{
+    return check_surface_overlaps_range(surface, other_surface->vram_addr,
+                                        other_surface->size);
+}
+
 static void invalidate_overlapping_surfaces(NV2AState *d,
                                             SurfaceBinding const *surface)
 {
     PGRAPHVkState *r = d->pgraph.vk_renderer_state;
 
-    uintptr_t e_end = surface->vram_addr + surface->size - 1;
-
-    SurfaceBinding *s, *next;
-    QTAILQ_FOREACH_SAFE(s, &r->surfaces, entry, next) {
-        uintptr_t s_end = s->vram_addr + s->size - 1;
-        bool overlapping =
-            !(s->vram_addr > e_end || surface->vram_addr > s_end);
-        if (overlapping) {
+    SurfaceBinding *other_surface, *next_surface;
+    QTAILQ_FOREACH_SAFE (other_surface, &r->surfaces, entry, next_surface) {
+        if (check_surfaces_overlap(surface, other_surface)) {
             trace_nv2a_pgraph_surface_evict_overlapping(
-                s->vram_addr, s->width, s->height,
-                s->pitch);
-            pgraph_vk_surface_download_if_dirty(d, s);
-            invalidate_surface(d, s);
+                other_surface->vram_addr, other_surface->width,
+                other_surface->height, other_surface->pitch);
+            pgraph_vk_surface_download_if_dirty(d, other_surface);
+            invalidate_surface(d, other_surface);
         }
     }
 }

include/hw/core/cpu.h

@@ -1173,10 +1173,10 @@ void cpu_watchpoint_remove_all(CPUState *cpu, int mask);
  * to a surface will be slower.
  */
 
-int mem_access_callback_insert(CPUState *cpu, MemoryRegion *mr,
-                               hwaddr offset, hwaddr len,
-                               MemAccessCallback **cb,
-                               MemAccessCallbackFunc func, void *opaque);
+MemAccessCallback *mem_access_callback_insert(CPUState *cpu, MemoryRegion *mr,
+                                              hwaddr offset, hwaddr len,
+                                              MemAccessCallbackFunc func,
+                                              void *opaque);
 void mem_access_callback_remove_by_ref(CPUState *cpu, MemAccessCallback *cb);
 int mem_access_callback_address_matches(CPUState *cpu, hwaddr addr, hwaddr len);
 void mem_check_access_callback_ramaddr(CPUState *cpu,

system/physmem.c

@@ -19,6 +19,7 @@
 
 #include "qemu/osdep.h"
 #include "exec/page-vary.h"
+#include "hw/core/cpu.h"
 #include "qapi/error.h"
 
 #include "qemu/cutils.h"
@@ -836,33 +837,48 @@ int mem_access_callback_address_matches(CPUState *cpu, hwaddr addr, hwaddr len)
     return ret;
 }
 
-int mem_access_callback_insert(CPUState *cpu, MemoryRegion *mr, hwaddr offset,
-                               hwaddr len, MemAccessCallback **cb,
-                               MemAccessCallbackFunc func, void *opaque)
+static void do_mem_access_callback_insert(CPUState *cpu, run_on_cpu_data data)
+
+{
+    MemAccessCallback *cb = (MemAccessCallback *)data.host_ptr;
+    QTAILQ_INSERT_TAIL(&cpu->mem_access_callbacks, cb, entry);
+}
+
+MemAccessCallback *mem_access_callback_insert(CPUState *cpu, MemoryRegion *mr,
+                                              hwaddr offset, hwaddr len,
+                                              MemAccessCallbackFunc func,
+                                              void *opaque)
 {
     assert(len > 0);
 
-    MemAccessCallback *cb_ = g_malloc(sizeof(*cb_));
-    cb_->mr = mr;
-    cb_->addr = memory_region_get_ram_addr(mr) + offset;
-    cb_->len = len;
-    cb_->func = func;
-    cb_->opaque = opaque;
-    QTAILQ_INSERT_TAIL(&cpu->mem_access_callbacks, cb_, entry);
-    if (cb) {
-        *cb = cb_;
-    }
+    MemAccessCallback *cb = g_malloc(sizeof(*cb));
+    cb->mr = mr;
+    cb->addr = memory_region_get_ram_addr(mr) + offset;
+    cb->len = len;
+    cb->func = func;
+    cb->opaque = opaque;
+
+    async_safe_run_on_cpu(cpu, do_mem_access_callback_insert,
+                          RUN_ON_CPU_HOST_PTR(cb));
 
     // FIXME: flush only applicable pages
     tlb_flush_all_cpus_synced(cpu);
 
-    return 0;
+    return cb;
+}
+
+static void do_mem_access_callback_remove_by_ref(CPUState *cpu,
+                                                 run_on_cpu_data data)
+{
+    MemAccessCallback *cb = (MemAccessCallback *)data.host_ptr;
+    QTAILQ_REMOVE(&cpu->mem_access_callbacks, cb, entry);
+    g_free(cb);
 }
 
 void mem_access_callback_remove_by_ref(CPUState *cpu, MemAccessCallback *cb)
 {
-    QTAILQ_REMOVE(&cpu->mem_access_callbacks, cb, entry);
-    g_free(cb);
+    async_safe_run_on_cpu(cpu, do_mem_access_callback_remove_by_ref,
+                          RUN_ON_CPU_HOST_PTR(cb));
 
     // FIXME: flush only applicable pages
     tlb_flush_all_cpus_synced(cpu);