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 :)

Based-on: <20200507155813.16169-1-philmd@redhat.com>
"Move Xen accelerator code under accel/xen/"
https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg01722.html

[*] https://www.mail-archive.com/qemu-block@nongnu.org/msg66242.html

Philippe Mathieu-Daudé (10):
  exec: Rename qemu_ram_writeback() as qemu_ram_msync()
  exec/ramblock: Add missing 'qemu/rcu.h' include
  exec: Move tb_invalidate_phys_range() to 'exec/exec-all.h'
  exec/memory-internal: Check CONFIG_SOFTMMU instead of CONFIG_USER_ONLY
  exec: Move qemu_minrampagesize/qemu_maxrampagesize to 'qemu-common.h'
  exec: Move ramblock_recv_bitmap_offset() to migration/ram.c
  exec: Move all RAMBlock functions to 'exec/ramblock.h'
  hw/block: Let the NVMe emulated device be target-agnostic
  exec: Update coding style to make checkpatch.pl happy
  exec: Move cpu_physical_memory_* functions to 'exec/memory-internal.h'

 include/exec/cpu-common.h      |  24 --
 include/exec/exec-all.h        |  14 +-
 include/exec/memory-internal.h | 307 +++++++++++++++++++++++-
 include/exec/ram_addr.h        | 415 +--------------------------------
 include/exec/ramblock.h        | 135 +++++++++++
 include/qemu-common.h          |  10 +
 migration/migration.h          |   1 +
 accel/tcg/cputlb.c             |   1 -
 accel/tcg/translate-all.c      |   2 -
 exec.c                         |   2 +-
 hw/block/nvme.c                |   5 +-
 hw/ppc/spapr.c                 |   1 -
 hw/ppc/spapr_caps.c            |   2 +-
 hw/ppc/spapr_pci.c             |   1 -
 hw/s390x/s390-stattrib-kvm.c   |   1 -
 hw/s390x/s390-stattrib.c       |   1 -
 hw/s390x/s390-virtio-ccw.c     |   2 +-
 hw/vfio/spapr.c                |   2 +-
 hw/virtio/vhost-user.c         |   1 +
 hw/virtio/vhost.c              |   1 +
 hw/virtio/virtio-balloon.c     |   1 +
 memory.c                       |   4 +-
 migration/migration.c          |   1 +
 migration/postcopy-ram.c       |   1 +
 migration/ram.c                |   8 +
 migration/savevm.c             |   1 +
 stubs/ram-block.c              |   2 +-
 target/ppc/kvm.c               |   1 -
 target/s390x/kvm.c             |   1 -
 util/vfio-helpers.c            |   2 +-
 hw/block/Makefile.objs         |   2 +-
 31 files changed, 485 insertions(+), 467 deletions(-)

-- 
2.21.3

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

Paolo Bonzini <pbonzini@redhat.com> 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.

Patchew URL: https://patchew.org/QEMU/20200507173958.25894-1-philmd@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 ===

  TEST    check-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....
  TEST    check-unit: tests/check-block-qdict
  TEST    check-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

real    14m12.181s
user    0m9.451s


The full log is available at
http://patchew.org/logs/20200507173958.25894-1-philmd@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com