Re: [Qemu-devel] [PATCH v2 15/29] Include migration/vmstate.h less

[Markus Armbruster]

Philippe Mathieu-Daudé <address@hidden> writes:

> On 8/6/19 5:14 PM, Markus Armbruster wrote:
>> In my "build everything" tree, changing migration/vmstate.h triggers a
>> recompile of some 2700 out of 6600 objects (not counting tests and
>> objects that don't depend on qemu/osdep.h).
>> 
>> hw/hw.h supposedly includes it for convenience.  Several other headers
>> include it just to get VMStateDescription.  The previous commit made
>> that unnecessary.
>> 
>> Include migration/vmstate.h only where it's still needed.  Touching it
>> now recompiles only some 1600 objects.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> Reviewed-by: Alistair Francis <address@hidden>
>> ---
[...]
>>  target/alpha/machine.c             | 1 +
>>  target/arm/machine.c               | 1 +
>>  target/cris/machine.c              | 1 +
>>  target/hppa/machine.c              | 1 +
>>  target/i386/machine.c              | 1 +
>>  target/lm32/machine.c              | 1 +
>>  target/mips/machine.c              | 1 +
>>  target/moxie/machine.c             | 1 +
>>  target/openrisc/machine.c          | 1 +
>>  target/ppc/machine.c               | 1 +
>>  target/s390x/machine.c             | 1 +
>>  target/sparc/machine.c             | 1 +
>
> target/*/machine.c are odd files... Find a common pattern is not obvious.
>
> $ git grep 'cc..vmsd ='
> target/arm/cpu.c:2604:    cc->vmsd = &vmstate_arm_cpu;
> target/i386/cpu.c:5953:    cc->vmsd = &vmstate_x86_cpu;
> target/lm32/cpu.c:235:    cc->vmsd = &vmstate_lm32_cpu;
> target/mips/cpu.c:203:    cc->vmsd = &vmstate_mips_cpu;
> target/moxie/cpu.c:116:    cc->vmsd = &vmstate_moxie_cpu;
> target/ppc/translate_init.inc.c:10597:    cc->vmsd = &vmstate_ppc_cpu;
> target/riscv/cpu.c:492:    cc->vmsd = &vmstate_riscv_cpu;
> target/s390x/cpu.c:491:    cc->vmsd = &vmstate_s390_cpu;
> target/sparc/cpu.c:883:    cc->vmsd = &vmstate_sparc_cpu;
>
> Various machine.c are not justified and could go into cpu.c.
> (Not this patch problem).
>
> However I wonder if "migration/vmstate.h" shouldn't be included in
> include/migration/cpu.h instead.

Hmm...

    $ git-grep migration/cpu
    target/alpha/machine.c:#include "migration/cpu.h"
    target/arm/machine.c:#include "migration/cpu.h"
    target/cris/machine.c:#include "migration/cpu.h"
    target/hppa/machine.c:#include "migration/cpu.h"
    target/i386/machine.c:#include "migration/cpu.h"
    target/lm32/machine.c:#include "migration/cpu.h"
    target/mips/machine.c:#include "migration/cpu.h"
    target/moxie/machine.c:#include "migration/cpu.h"
    target/openrisc/machine.c:#include "migration/cpu.h"
    target/ppc/machine.c:#include "migration/cpu.h"
    target/sparc/machine.c:#include "migration/cpu.h"

Can do.

The odd one out: target/s390x/machine.c does not include
migration/cpu.h.

>>  util/fifo8.c                       | 1 +
>>  435 files changed, 436 insertions(+), 8 deletions(-)
> [...]
>> diff --git a/target/moxie/machine.c b/target/moxie/machine.c
>> index 322a724e6f..0d6012603e 100644
>> --- a/target/moxie/machine.c
>> +++ b/target/moxie/machine.c
>> @@ -4,6 +4,7 @@
>>  #include "hw/boards.h"
>>  #include "machine.h"
>>  #include "migration/cpu.h"
>> +#include "migration/vmstate.h"
>
> Hmm this one seems to belong to target/moxie/machine.h... which happens
> to be also odd.

Yes, other targets declare vmstate_FOO_cpu in cpu.h.

>>  const VMStateDescription vmstate_moxie_cpu = {
>>      .name = "cpu",
[...]