| Message ID | 20200226155304.60219-9-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migrate/ram: Fix resizing RAM blocks while migrating | expand |
* David Hildenbrand (david@redhat.com) 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" <dgilbert@redhat.com> > Cc: Juan Quintela <quintela@redhat.com> > Cc: Peter Xu <peterx@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > 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. Other than that, Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > + break; > + } > > - host = host_from_ram_block_offset(block, addr); > - if (!host) { > + if (!offset_in_ramblock(block, addr)) { > error_report("Illegal RAM offset " RAM_ADDR_FMT, addr); > ret = -EINVAL; > break; > @@ -3163,21 +3179,18 @@ static int ram_load_postcopy(QEMUFile *f) > * of a host page in one chunk. > */ > page_buffer = postcopy_host_page + > - ((uintptr_t)host & (block->page_size - 1)); > + host_page_offset_from_ram_block_offset(block, addr); > /* If all TP are zero then we can optimise the place */ > if (target_pages == 1) { > all_zero = true; > - this_host = (void *)QEMU_ALIGN_DOWN((uintptr_t)host, > - block->page_size); > - } else { > + host_page = host_page_from_ram_block_offset(block, addr); > + } else if (host_page != host_page_from_ram_block_offset(block, > + addr)) { > /* not the 1st TP within the HP */ > - if (QEMU_ALIGN_DOWN((uintptr_t)host, block->page_size) != > - (uintptr_t)this_host) { > - error_report("Non-same host page %p/%p", > - host, this_host); > - ret = -EINVAL; > - break; > - } > + error_report("Non-same host page %p/%p", host_page, > + host_page_from_ram_block_offset(block, addr)); > + ret = -EINVAL; > + break; > } > > /* > @@ -3257,16 +3270,11 @@ static int ram_load_postcopy(QEMUFile *f) > } > > if (!ret && place_needed) { > - /* This gets called at the last target page in the host page */ > - void *place_dest = (void *)QEMU_ALIGN_DOWN((uintptr_t)host, > - block->page_size); > - > if (all_zero) { > - ret = postcopy_place_page_zero(mis, place_dest, > - block); > + ret = postcopy_place_page_zero(mis, host_page, block); > } else { > - ret = postcopy_place_page(mis, place_dest, > - place_source, block); > + ret = postcopy_place_page(mis, host_page, place_source, > + block); > } > } > } > -- > 2.24.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 06.03.20 17:05, Dr. David Alan Gilbert wrote: > * David Hildenbrand (david@redhat.com) 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" <dgilbert@redhat.com> >> Cc: Juan Quintela <quintela@redhat.com> >> Cc: Peter Xu <peterx@redhat.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> 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 <dgilbert@redhat.com> Thanks!
On 06.03.20 17:20, David Hildenbrand wrote: > On 06.03.20 17:05, Dr. David Alan Gilbert wrote: >> * David Hildenbrand (david@redhat.com) 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" <dgilbert@redhat.com> >>> Cc: Juan Quintela <quintela@redhat.com> >>> Cc: Peter Xu <peterx@redhat.com> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >>> 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! My memory kicks in: This was dropped on purpose. ram_block_from_stream() will print proper errors already. Cheers!
* David Hildenbrand (david@redhat.com) wrote: > On 06.03.20 17:20, David Hildenbrand wrote: > > On 06.03.20 17:05, Dr. David Alan Gilbert wrote: > >> * David Hildenbrand (david@redhat.com) 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" <dgilbert@redhat.com> > >>> Cc: Juan Quintela <quintela@redhat.com> > >>> Cc: Peter Xu <peterx@redhat.com> > >>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>> --- > >>> 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! > > My memory kicks in: This was dropped on purpose. ram_block_from_stream() > will print proper errors already. OK! Dave > Cheers! > > > -- > Thanks, > > David / dhildenb -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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; + break; + } - host = host_from_ram_block_offset(block, addr); - if (!host) { + if (!offset_in_ramblock(block, addr)) { error_report("Illegal RAM offset " RAM_ADDR_FMT, addr); ret = -EINVAL; break; @@ -3163,21 +3179,18 @@ static int ram_load_postcopy(QEMUFile *f) * of a host page in one chunk. */ page_buffer = postcopy_host_page + - ((uintptr_t)host & (block->page_size - 1)); + host_page_offset_from_ram_block_offset(block, addr); /* If all TP are zero then we can optimise the place */ if (target_pages == 1) { all_zero = true; - this_host = (void *)QEMU_ALIGN_DOWN((uintptr_t)host, - block->page_size); - } else { + host_page = host_page_from_ram_block_offset(block, addr); + } else if (host_page != host_page_from_ram_block_offset(block, + addr)) { /* not the 1st TP within the HP */ - if (QEMU_ALIGN_DOWN((uintptr_t)host, block->page_size) != - (uintptr_t)this_host) { - error_report("Non-same host page %p/%p", - host, this_host); - ret = -EINVAL; - break; - } + error_report("Non-same host page %p/%p", host_page, + host_page_from_ram_block_offset(block, addr)); + ret = -EINVAL; + break; } /* @@ -3257,16 +3270,11 @@ static int ram_load_postcopy(QEMUFile *f) } if (!ret && place_needed) { - /* This gets called at the last target page in the host page */ - void *place_dest = (void *)QEMU_ALIGN_DOWN((uintptr_t)host, - block->page_size); - if (all_zero) { - ret = postcopy_place_page_zero(mis, place_dest, - block); + ret = postcopy_place_page_zero(mis, host_page, block); } else { - ret = postcopy_place_page(mis, place_dest, - place_source, block); + ret = postcopy_place_page(mis, host_page, place_source, + block); } } }
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" <dgilbert@redhat.com> Cc: Juan Quintela <quintela@redhat.com> Cc: Peter Xu <peterx@redhat.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- migration/ram.c | 54 ++++++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 23 deletions(-)