Re: [PATCH 00/10] exec: Shear 'exec/ram_addr.h' and make NVMe device target-agnostic
Paolo Bonzini wrote: > On 07/05/20 19:39, Philippe Mathieu-Daudé wrote: >> Stefan suggested to make qemu_ram_writeback() target agnostic, >> Paolo to add memory_region_msync(), and Peter to remove >> "exec/ram_addr.h" [*]. >> >> I let a single function in this file, >> cpu_physical_memory_sync_dirty_bitmap(), to let the maintainer >> have the pleasure to remove this header definitively himself :) > > I don't think this is a good idea. :) > > "exec/ram_addr.h" is a good place for functions that work on ram-addr_t > and/or RAMBlock data. There should very few of these, since these are > mostly an internal concept that should only be used for live migration. > You could: > > - figure out which files actually need to include exec/ram_addr.h. > There's already very few of them. ram_addr.h looks really "not dangerous", I think that I preffer the memory-internal.h or whatever name that implies that you should think twice before using that file. My main problem with that include are: - cpu_physical_memory_set_dirty_lebitmap() - cpu_physical_memory_sync_dirty_bitmap() Both are long, both are complex, and if one changes them, it is very probably that you end breaking some random architecture in TCG (being there, done that). As you said, that functions are used only in a couple of places. I haven't meassured the impact of moving it to a .c file, but I would preffer it if performance don't suffer. > > - move the large functions to a new .c file, ramblock.c. Figure out > which can be static, move the declarations for the others to ramblock.h Ok, it appears that we kind of agree. > - kill ram_addr.h and include ramblock.h instead. I created ramblock.h initially (/me checks) because if you just need to walk a ramblock (that is clearly target independent code), you needed to become target dependent, due to the definitions that are there inside. I don't care one way or another, just that we don't create the old dependency. > Not coincidentially, qemu_ram_writeback() takes a RAMBlock*, and it ends > up in ramblock.h. Thanks, Juan.
Re: [PATCH 00/10] exec: Shear 'exec/ram_addr.h' and make NVMe device target-agnostic
Patchew URL: https://patchew.org/QEMU/20200507173958.25894-1-phi...@redhat.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === TESTcheck-qtest-aarch64: tests/qtest/arm-cpu-features Broken pipe /tmp/qemu-test/src/tests/qtest/libqtest.c:175: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped) ERROR - too few tests run (expected 5, got 0) make: *** [check-qtest-aarch64] Error 1 make: *** Waiting for unfinished jobs TESTcheck-unit: tests/check-block-qdict TESTcheck-unit: tests/test-char --- raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=aa05160ac8ef4a8aabb028db62e978dc', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-sj3y_tsg/src/docker-src.2020-05-08-02.05.43.23802:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2. filter=--filter=label=com.qemu.instance.uuid=aa05160ac8ef4a8aabb028db62e978dc make[1]: *** [docker-run] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-sj3y_tsg/src' make: *** [docker-run-test-quick@centos7] Error 2 real14m12.181s user0m9.451s The full log is available at http://patchew.org/logs/20200507173958.25894-1-phi...@redhat.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH 00/10] exec: Shear 'exec/ram_addr.h' and make NVMe device target-agnostic
On 07/05/20 19:39, Philippe Mathieu-Daudé wrote: > Stefan suggested to make qemu_ram_writeback() target agnostic, > Paolo to add memory_region_msync(), and Peter to remove > "exec/ram_addr.h" [*]. > > I let a single function in this file, > cpu_physical_memory_sync_dirty_bitmap(), to let the maintainer > have the pleasure to remove this header definitively himself :) I don't think this is a good idea. :) "exec/ram_addr.h" is a good place for functions that work on ram-addr_t and/or RAMBlock data. There should very few of these, since these are mostly an internal concept that should only be used for live migration. You could: - figure out which files actually need to include exec/ram_addr.h. There's already very few of them. - move the large functions to a new .c file, ramblock.c. Figure out which can be static, move the declarations for the others to ramblock.h - kill ram_addr.h and include ramblock.h instead. Not coincidentially, qemu_ram_writeback() takes a RAMBlock*, and it ends up in ramblock.h. Also, this is orthogonal to adding the wrapper memory_region_msync. Thanks, Paolo
