Re: [PATCH 00/10] exec: Shear 'exec/ram_addr.h' and make NVMe device target-agnostic

[Juan Quintela]

Paolo Bonzini <address@hidden> 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.