After using number of target page received to track one host page, we
could have the capability to handle random order target page arrival in
one host page.

This is a preparation for enabling compress during postcopy.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index b5759793a9..da0596411c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4015,7 +4015,6 @@ 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 *last_host = NULL;
     bool all_zero = false;
     int target_pages = 0;
 
@@ -4062,24 +4061,15 @@ static int ram_load_postcopy(QEMUFile *f)
              * that's moved into place later.
              * The migration protocol uses,  possibly smaller, target-pages
              * however the source ensures it always sends all the components
-             * of a host page in order.
+             * of a host page in one chunk.
              */
             page_buffer = postcopy_host_page +
                           ((uintptr_t)host & (block->page_size - 1));
             /* If all TP are zero then we can optimise the place */
             if (target_pages == 1) {
                 all_zero = true;
-            } else {
-                /* not the 1st TP within the HP */
-                if (host != (last_host + TARGET_PAGE_SIZE)) {
-                    error_report("Non-sequential target page %p/%p",
-                                  host, last_host);
-                    ret = -EINVAL;
-                    break;
-                }
             }
 
-
             /*
              * If it's the last part of a host page then we place the host
              * page
@@ -4090,7 +4080,6 @@ static int ram_load_postcopy(QEMUFile *f)
             }
             place_source = postcopy_host_page;
         }
-        last_host = host;
 
         switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
         case RAM_SAVE_FLAG_ZERO:
@@ -4143,7 +4132,8 @@ 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 = host + TARGET_PAGE_SIZE - block->page_size;
+            void *place_dest = (void *)QEMU_ALIGN_DOWN((unsigned long)host,
+                                                       block->page_size);
 
             if (all_zero) {
                 ret = postcopy_place_page_zero(mis, place_dest,
-- 
2.17.1

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> After using number of target page received to track one host page, we
> could have the capability to handle random order target page arrival in
> one host page.
> 
> This is a preparation for enabling compress during postcopy.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  migration/ram.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index b5759793a9..da0596411c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4015,7 +4015,6 @@ 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 *last_host = NULL;
>      bool all_zero = false;
>      int target_pages = 0;
>  
> @@ -4062,24 +4061,15 @@ static int ram_load_postcopy(QEMUFile *f)
>               * that's moved into place later.
>               * The migration protocol uses,  possibly smaller, target-pages
>               * however the source ensures it always sends all the components
> -             * of a host page in order.
> +             * of a host page in one chunk.
>               */
>              page_buffer = postcopy_host_page +
>                            ((uintptr_t)host & (block->page_size - 1));
>              /* If all TP are zero then we can optimise the place */
>              if (target_pages == 1) {
>                  all_zero = true;
> -            } else {
> -                /* not the 1st TP within the HP */
> -                if (host != (last_host + TARGET_PAGE_SIZE)) {
> -                    error_report("Non-sequential target page %p/%p",
> -                                  host, last_host);
> -                    ret = -EINVAL;
> -                    break;
> -                }

I think this is losing more protection than needed.
I think you can still protect against a page from a different host-page
arriving until we've placed the current host-page.
So something like:

    if (((uintptr_t)host & ~(block->page_size - 1)) !=
        last_host)

and then set last_host to the start of the host page.

Then you'll check if that flush is really working.

Dave

>              }
>  
> -
>              /*
>               * If it's the last part of a host page then we place the host
>               * page
> @@ -4090,7 +4080,6 @@ static int ram_load_postcopy(QEMUFile *f)
>              }
>              place_source = postcopy_host_page;
>          }
> -        last_host = host;
>  
>          switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
>          case RAM_SAVE_FLAG_ZERO:
> @@ -4143,7 +4132,8 @@ 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 = host + TARGET_PAGE_SIZE - block->page_size;
> +            void *place_dest = (void *)QEMU_ALIGN_DOWN((unsigned long)host,
> +                                                       block->page_size);
>  
>              if (all_zero) {
>                  ret = postcopy_place_page_zero(mis, place_dest,
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On Wed, Nov 06, 2019 at 08:08:28PM +0000, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> After using number of target page received to track one host page, we
>> could have the capability to handle random order target page arrival in
>> one host page.
>> 
>> This is a preparation for enabling compress during postcopy.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  migration/ram.c | 16 +++-------------
>>  1 file changed, 3 insertions(+), 13 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index b5759793a9..da0596411c 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -4015,7 +4015,6 @@ 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 *last_host = NULL;
>>      bool all_zero = false;
>>      int target_pages = 0;
>>  
>> @@ -4062,24 +4061,15 @@ static int ram_load_postcopy(QEMUFile *f)
>>               * that's moved into place later.
>>               * The migration protocol uses,  possibly smaller, target-pages
>>               * however the source ensures it always sends all the components
>> -             * of a host page in order.
>> +             * of a host page in one chunk.
>>               */
>>              page_buffer = postcopy_host_page +
>>                            ((uintptr_t)host & (block->page_size - 1));
>>              /* If all TP are zero then we can optimise the place */
>>              if (target_pages == 1) {
>>                  all_zero = true;
>> -            } else {
>> -                /* not the 1st TP within the HP */
>> -                if (host != (last_host + TARGET_PAGE_SIZE)) {
>> -                    error_report("Non-sequential target page %p/%p",
>> -                                  host, last_host);
>> -                    ret = -EINVAL;
>> -                    break;
>> -                }
>
>I think this is losing more protection than needed.
>I think you can still protect against a page from a different host-page
>arriving until we've placed the current host-page.
>So something like:
>
>    if (((uintptr_t)host & ~(block->page_size - 1)) !=
>        last_host)
>

OK, looks reasonable.

>and then set last_host to the start of the host page.
>

I think it is not necessary to update the last_host on each target page. We
can just set it at the first target page.

>Then you'll check if that flush is really working.
>
>Dave
>
>>              }
>>  
>> -
>>              /*
>>               * If it's the last part of a host page then we place the host
>>               * page
>> @@ -4090,7 +4080,6 @@ static int ram_load_postcopy(QEMUFile *f)
>>              }
>>              place_source = postcopy_host_page;
>>          }
>> -        last_host = host;
>>  
>>          switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
>>          case RAM_SAVE_FLAG_ZERO:
>> @@ -4143,7 +4132,8 @@ 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 = host + TARGET_PAGE_SIZE - block->page_size;
>> +            void *place_dest = (void *)QEMU_ALIGN_DOWN((unsigned long)host,
>> +                                                       block->page_size);
>>  
>>              if (all_zero) {
>>                  ret = postcopy_place_page_zero(mis, place_dest,
>> -- 
>> 2.17.1
>> 
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> On Wed, Nov 06, 2019 at 08:08:28PM +0000, Dr. David Alan Gilbert wrote:
> >* Wei Yang (richardw.yang@linux.intel.com) wrote:
> >> After using number of target page received to track one host page, we
> >> could have the capability to handle random order target page arrival in
> >> one host page.
> >> 
> >> This is a preparation for enabling compress during postcopy.
> >> 
> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> >> ---
> >>  migration/ram.c | 16 +++-------------
> >>  1 file changed, 3 insertions(+), 13 deletions(-)
> >> 
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index b5759793a9..da0596411c 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -4015,7 +4015,6 @@ 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 *last_host = NULL;
> >>      bool all_zero = false;
> >>      int target_pages = 0;
> >>  
> >> @@ -4062,24 +4061,15 @@ static int ram_load_postcopy(QEMUFile *f)
> >>               * that's moved into place later.
> >>               * The migration protocol uses,  possibly smaller, target-pages
> >>               * however the source ensures it always sends all the components
> >> -             * of a host page in order.
> >> +             * of a host page in one chunk.
> >>               */
> >>              page_buffer = postcopy_host_page +
> >>                            ((uintptr_t)host & (block->page_size - 1));
> >>              /* If all TP are zero then we can optimise the place */
> >>              if (target_pages == 1) {
> >>                  all_zero = true;
> >> -            } else {
> >> -                /* not the 1st TP within the HP */
> >> -                if (host != (last_host + TARGET_PAGE_SIZE)) {
> >> -                    error_report("Non-sequential target page %p/%p",
> >> -                                  host, last_host);
> >> -                    ret = -EINVAL;
> >> -                    break;
> >> -                }
> >
> >I think this is losing more protection than needed.
> >I think you can still protect against a page from a different host-page
> >arriving until we've placed the current host-page.
> >So something like:
> >
> >    if (((uintptr_t)host & ~(block->page_size - 1)) !=
> >        last_host)
> >
> 
> OK, looks reasonable.
> 
> >and then set last_host to the start of the host page.
> >
> 
> I think it is not necessary to update the last_host on each target page. We
> can just set it at the first target page.

Yes, that would be fine.

Dave

> >Then you'll check if that flush is really working.
> >
> >Dave
> >
> >>              }
> >>  
> >> -
> >>              /*
> >>               * If it's the last part of a host page then we place the host
> >>               * page
> >> @@ -4090,7 +4080,6 @@ static int ram_load_postcopy(QEMUFile *f)
> >>              }
> >>              place_source = postcopy_host_page;
> >>          }
> >> -        last_host = host;
> >>  
> >>          switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
> >>          case RAM_SAVE_FLAG_ZERO:
> >> @@ -4143,7 +4132,8 @@ 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 = host + TARGET_PAGE_SIZE - block->page_size;
> >> +            void *place_dest = (void *)QEMU_ALIGN_DOWN((unsigned long)host,
> >> +                                                       block->page_size);
> >>  
> >>              if (all_zero) {
> >>                  ret = postcopy_place_page_zero(mis, place_dest,
> >> -- 
> >> 2.17.1
> >> 
> >--
> >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> -- 
> Wei Yang
> Help you, Help me
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK