In the last version of the SHMEM MAP/UNMAP [1] there was a comment [2] from Stefan about the lifecycle of the memory regions.
After some discussion, David Hildenbrand proposed to detect RAM regions and handle refcounting differently to clear the initial concern. This RFC patch is meant for gathering feedback from others (i.e., Paolo Bonzini and Peter Xu). [1] https://patchwork.ozlabs.org/project/qemu-devel/list/?series=460121 [2] https://patchwork.ozlabs.org/comment/3528600/ --- This patch enhances memory_region_ref() and memory_region_unref() to handle RAM and MMIO memory regions differently, improving reference counting semantics. RAM regions now reference/unreference the memory region object itself, while MMIO continue to reference/unreference the owner device as before. An additional qtest has been added to stress the memory lifecycle. All tests pass as these changes keep backward compatibility (prior behaviour is kept for MMIO regions). Signed-off-by: David Hildenbrand <da...@redhat.com > Signed-off-by: Albert Esteve <aest...@redhat.com> --- system/memory.c | 22 +++++++++++++---- tests/qtest/ivshmem-test.c | 50 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/system/memory.c b/system/memory.c index 5646547940..48ab6e5592 100644 --- a/system/memory.c +++ b/system/memory.c @@ -1826,6 +1826,14 @@ Object *memory_region_owner(MemoryRegion *mr) void memory_region_ref(MemoryRegion *mr) { + /* Regions without an owner are considered static. */ + if (!mr || !mr->owner) { + return; + } + if (mr->ram) { + object_ref(OBJECT(mr)); + return; + } /* MMIO callbacks most likely will access data that belongs * to the owner, hence the need to ref/unref the owner whenever * the memory region is in use. @@ -1836,16 +1844,20 @@ void memory_region_ref(MemoryRegion *mr) * Memory regions without an owner are supposed to never go away; * we do not ref/unref them because it slows down DMA sensibly. */ - if (mr && mr->owner) { - object_ref(mr->owner); - } + object_ref(mr->owner); } void memory_region_unref(MemoryRegion *mr) { - if (mr && mr->owner) { - object_unref(mr->owner); + /* Regions without an owner are considered static. */ + if (!mr || !mr->owner) { + return; + } + if (mr->ram) { + object_unref(OBJECT(mr)); + return; } + object_unref(mr->owner); } uint64_t memory_region_size(MemoryRegion *mr) diff --git a/tests/qtest/ivshmem-test.c b/tests/qtest/ivshmem-test.c index fb45fdeb07..44f712e9ae 100644 --- a/tests/qtest/ivshmem-test.c +++ b/tests/qtest/ivshmem-test.c @@ -194,6 +194,55 @@ static void test_ivshmem_single(void) cleanup_vm(s); } +static void test_memory_region_lifecycle(void) +{ + /* Device creation triggers memory region mapping (calls ref) */ + IVState state1, state2; + uint32_t test_data, read_data; + int i; + + setup_vm(&state1); + + /* Basic verification that device works */ + test_data = 0x12345678; + write_mem(&state1, 0, &test_data, sizeof(test_data)); + read_mem(&state1, 0, &read_data, sizeof(read_data)); + g_assert_cmpuint(read_data, ==, test_data); + + /* Multiple devices stress test memory region ref counting */ + setup_vm(&state2); + + /* Verify both devices work independently */ + test_data = 0xDEADBEEF; + write_mem(&state2, 4, &test_data, sizeof(test_data)); + read_mem(&state2, 4, &read_data, sizeof(read_data)); + g_assert_cmpuint(read_data, ==, test_data); + + /* Device destruction triggers memory region unmapping (calls unref) */ + cleanup_vm(&state1); + + /* Verify remaining device still works after first device cleanup */ + read_mem(&state2, 4, &read_data, sizeof(read_data)); + g_assert_cmpuint(read_data, ==, test_data); + + /* Final cleanup */ + cleanup_vm(&state2); + + /* Quick lifecycle stress test - multiple create/destroy cycles */ + for (i = 0; i < 5; i++) { + IVState temp_state; + setup_vm(&temp_state); + + /* Quick test to ensure device works */ + test_data = 0x1000 + i; + write_mem(&temp_state, 0, &test_data, sizeof(test_data)); + read_mem(&temp_state, 0, &read_data, sizeof(read_data)); + g_assert_cmpuint(read_data, ==, test_data); + + cleanup_vm(&temp_state); + } +} + static void test_ivshmem_pair(void) { IVState state1, state2, *s1, *s2; @@ -503,6 +552,7 @@ int main(int argc, char **argv) tmpserver = g_strconcat(tmpdir, "/server", NULL); qtest_add_func("/ivshmem/single", test_ivshmem_single); + qtest_add_func("/ivshmem/memory-lifecycle", test_memory_region_lifecycle); qtest_add_func("/ivshmem/hotplug", test_ivshmem_hotplug); qtest_add_func("/ivshmem/memdev", test_ivshmem_memdev); if (g_test_slow()) { -- 2.49.0
