Re: [PATCH v3 08/13] migration/ram: Simplify host page handling in ram_load_postcopy()

[David Hildenbrand]

On 06.03.20 17:05, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (address@hidden) wrote:
>> Add two new helper functions. This will in come handy once we want to
>> handle ram block resizes while postcopy is active.
>>
>> Cc: "Dr. David Alan Gilbert" <address@hidden>
>> Cc: Juan Quintela <address@hidden>
>> Cc: Peter Xu <address@hidden>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  migration/ram.c | 54 ++++++++++++++++++++++++++++---------------------
>>  1 file changed, 31 insertions(+), 23 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index d5a4d69e1c..f815f4e532 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2734,6 +2734,20 @@ static inline void 
>> *host_from_ram_block_offset(RAMBlock *block,
>>      return block->host + offset;
>>  }
>>  
>> +static void *host_page_from_ram_block_offset(RAMBlock *block,
>> +                                             ram_addr_t offset)
>> +{
>> +    /* Note: Explicitly no check against offset_in_ramblock(). */
>> +    return (void *)QEMU_ALIGN_DOWN((uintptr_t)block->host + offset,
>> +                                   block->page_size);
>> +}
>> +
>> +static ram_addr_t host_page_offset_from_ram_block_offset(RAMBlock *block,
>> +                                                         ram_addr_t offset)
>> +{
>> +    return ((uintptr_t)block->host + offset) & (block->page_size - 1);
>> +}
>> +
>>  static inline void *colo_cache_from_block_offset(RAMBlock *block,
>>                                                   ram_addr_t offset)
>>  {
>> @@ -3111,13 +3125,12 @@ static int ram_load_postcopy(QEMUFile *f)
>>      MigrationIncomingState *mis = migration_incoming_get_current();
>>      /* Temporary page that is later 'placed' */
>>      void *postcopy_host_page = mis->postcopy_tmp_page;
>> -    void *this_host = NULL;
>> +    void *host_page = NULL;
>>      bool all_zero = false;
>>      int target_pages = 0;
>>  
>>      while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
>>          ram_addr_t addr;
>> -        void *host = NULL;
>>          void *page_buffer = NULL;
>>          void *place_source = NULL;
>>          RAMBlock *block = NULL;
>> @@ -3143,9 +3156,12 @@ static int ram_load_postcopy(QEMUFile *f)
>>          if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
>>                       RAM_SAVE_FLAG_COMPRESS_PAGE)) {
>>              block = ram_block_from_stream(f, flags);
>> +            if (!block) {
>> +                ret = -EINVAL;
> 
> Could we have an error_report there, at the moment it would trigger
> the one below.

Makes sense, I'll add one!

> 
> Other than that,
> 
> 
> Reviewed-by: Dr. David Alan Gilbert <address@hidden>

Thanks!

-- 
Thanks,

David / dhildenb