Re: [Qemu-devel] [PATCH 07/18] arch_init: export RAM_SAVE_xxx flags

[Paolo Bonzini]

Il 22/08/2013 22:14, Michael R. Hines ha scritto:
> On 08/21/2013 06:49 AM, Paolo Bonzini wrote:
>> Il 21/08/2013 09:18, Lei Li ha scritto:
>>> Export RAM_SAVE_xxx flags for localhost migration.
>>>
>>> Signed-off-by: Lei Li <address@hidden>
>>> ---
>>>   arch_init.c                   |   12 ------------
>>>   include/migration/migration.h |   14 ++++++++++++++
>>>   2 files changed, 14 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch_init.c b/arch_init.c
>>> index 68a7ab7..1ea7c29 100644
>>> --- a/arch_init.c
>>> +++ b/arch_init.c
>>> @@ -108,18 +108,6 @@ static bool mig_throttle_on;
>>>   static int dirty_rate_high_cnt;
>>>   static void check_guest_throttling(void);
>>>   -/***********************************************************/
>>> -/* ram save/restore */
>>> -
>>> -#define RAM_SAVE_FLAG_FULL     0x01 /* Obsolete, not used anymore */
>>> -#define RAM_SAVE_FLAG_COMPRESS 0x02
>>> -#define RAM_SAVE_FLAG_MEM_SIZE 0x04
>>> -#define RAM_SAVE_FLAG_PAGE     0x08
>>> -#define RAM_SAVE_FLAG_EOS      0x10
>>> -#define RAM_SAVE_FLAG_CONTINUE 0x20
>>> -#define RAM_SAVE_FLAG_XBZRLE   0x40
>>> -/* 0x80 is reserved in migration.h start with 0x100 next */
>>> -
>>>     static struct defconfig_file {
>>>       const char *filename;
>>> diff --git a/include/migration/migration.h
>>> b/include/migration/migration.h
>>> index 6a24e65..5336117 100644
>>> --- a/include/migration/migration.h
>>> +++ b/include/migration/migration.h
>>> @@ -158,12 +158,26 @@ void ram_control_before_iterate(QEMUFile *f,
>>> uint64_t flags);
>>>   void ram_control_after_iterate(QEMUFile *f, uint64_t flags);
>>>   void ram_control_load_hook(QEMUFile *f, uint64_t flags);
>>>   +
>>> +/***********************************************************/
>>> +/* ram save/restore */
>>> +
>>> +#define RAM_SAVE_FLAG_FULL     0x01 /* Obsolete, not used anymore */
>>> +#define RAM_SAVE_FLAG_COMPRESS 0x02
>>> +#define RAM_SAVE_FLAG_MEM_SIZE 0x04
>>> +#define RAM_SAVE_FLAG_PAGE     0x08
>>> +#define RAM_SAVE_FLAG_EOS      0x10
>>> +#define RAM_SAVE_FLAG_CONTINUE 0x20
>>> +#define RAM_SAVE_FLAG_XBZRLE   0x40
>>> +
>>>   /* Whenever this is found in the data stream, the flags
>>>    * will be passed to ram_control_load_hook in the incoming-migration
>>>    * side. This lets before_ram_iterate/after_ram_iterate add
>>>    * transport-specific sections to the RAM migration data.
>>>    */
>>>   #define RAM_SAVE_FLAG_HOOK     0x80
>>> +/* Start with 0x100 next */
>>> +
>>>     #define RAM_SAVE_CONTROL_NOT_SUPP -1000
>>>   #define RAM_SAVE_CONTROL_DELAYED  -2000
>>>
>> This also looks like an encapsulation violation.
>>
>> Localhost migration is not very different in concept from RDMA (except
>> that it runs with the VM stopped, but that's just because you do
>> MADV_DONTNEED---it's not specific to the migration transport), and it
>> manages to do everything without touching arch_init.c and migration.c.
> 
> I think maybe we need a private header file here. Multiple parties want
> to add potentially add to this flag list without modifying arch_init.c.

There is a mechanism for private flags, which is the page-load hook we
introduced for RDMA.  The existing RAM_SAVE_FLAG_* should not be needed
outside arch_init.c.

MIG_STATE_* might be needed elsewhere, but the right way to export it is
this one: change MigrationInfo's status field from a string to a QAPI
enum, and remove MIG_STATE_* altogether in favor of the QAPI enum.

Paolo