Re: [PATCH v6 08/16] libs/guest: fill directly iov structure

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/06/2026 2:04 pm, Frediano Ziglio wrote:
> Instead of storing page pointers into an array and lately adding to
> iov vector add the pages directly to iov to avoid "guest_data"
> array.
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>
> ---
>  tools/libs/guest/xg_sr_common.h |  1 -
>  tools/libs/guest/xg_sr_save.c   | 64 ++++++++++++---------------------
>  2 files changed, 23 insertions(+), 42 deletions(-)
>
> diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
> index 95b0564e5c..b2c441b644 100644
> --- a/tools/libs/guest/xg_sr_common.h
> +++ b/tools/libs/guest/xg_sr_common.h
> @@ -214,7 +214,6 @@ struct xc_sr_context_save_buffers
>      xen_pfn_t batch_pfns[MAX_BATCH_SIZE];
>      xen_pfn_t mfns[MAX_BATCH_SIZE];
>      xen_pfn_t types[MAX_BATCH_SIZE];
> -    void *guest_data[MAX_BATCH_SIZE];
>      void *local_pages[MAX_BATCH_SIZE];
>      struct iovec iov[MAX_BATCH_SIZE + 2]; /* Headers + data. */
>      uint64_t rec_pfns[MAX_BATCH_SIZE];
> diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
> index 4988d8040b..8a22267fdf 100644
> --- a/tools/libs/guest/xg_sr_save.c
> +++ b/tools/libs/guest/xg_sr_save.c
> @@ -88,7 +88,6 @@ static int write_batch(struct xc_sr_context *ctx)
>      xc_interface *xch = ctx->xch;
>      xen_pfn_t *mfns, *types;
>      void *guest_mapping = NULL;
> -    void **guest_data;
>      void **local_pages;
>      int *errors, rc = -1;
>      unsigned int i, p, nr_pages = 0, nr_pages_mapped = 0;
> @@ -113,9 +112,6 @@ static int write_batch(struct xc_sr_context *ctx)
>      types = ctx->save.buffers->types;
>      /* Errors from attempting to map the gfns. */
>      errors = ctx->save.buffers->errors;
> -    /* Pointers to page data to send.  Mapped gfns or local allocations. */
> -    guest_data = ctx->save.buffers->guest_data;
> -    memset(guest_data, 0, sizeof(*guest_data) * nr_pfns);
>      /* Pointers to locally allocated pages.  Need freeing. */
>      local_pages = ctx->save.buffers->local_pages;
>      memset(local_pages, 0, sizeof(*local_pages) * nr_pfns);
> @@ -158,6 +154,19 @@ static int write_batch(struct xc_sr_context *ctx)
>          mfns[nr_pages++] = mfns[i];
>      }
>  
> +    hdrs.rec.length = sizeof(hdrs.page_data);
> +    hdrs.rec.length += nr_pfns * sizeof(*rec_pfns);
> +
> +    hdrs.page_data.count = nr_pfns;
> +
> +    iov[0].iov_base = &hdrs;
> +    iov[0].iov_len = sizeof(hdrs);
> +
> +    iov[1].iov_base = rec_pfns;
> +    iov[1].iov_len = nr_pfns * sizeof(*rec_pfns);
> +
> +    iovcnt = 2;
> +
>      if ( nr_pages > 0 )
>      {
>          guest_mapping = xenforeignmemory_map(
> @@ -199,61 +208,34 @@ static int write_batch(struct xc_sr_context *ctx)
>                  else
>                      goto err;
>              }
> +            else if ( iov[iovcnt - 1].iov_base + iov[iovcnt - 1].iov_len !=
> +                      page )
> +            {
> +                iov[iovcnt].iov_base = page;
> +                iov[iovcnt].iov_len = PAGE_SIZE;
> +                iovcnt++;
> +            }
>              else
> -                guest_data[i] = page;
> +            {
> +                iov[iovcnt - 1].iov_len += PAGE_SIZE;
> +            }
>  
>              rc = -1;
>              ++p;
>          }
>      }
>  
> -    hdrs.rec.length = sizeof(hdrs.page_data);
> -    hdrs.rec.length += nr_pfns * sizeof(*rec_pfns);
>      hdrs.rec.length += nr_pages * PAGE_SIZE;
>  
> -    hdrs.page_data.count = nr_pfns;
> -
>      for ( i = 0; i < nr_pfns; ++i )
>          rec_pfns[i] = ((uint64_t)(types[i]) << 32) | ctx->save.batch_pfns[i];
>  
> -    iov[0].iov_base = &hdrs;
> -    iov[0].iov_len = sizeof(hdrs);
> -
> -    iov[1].iov_base = rec_pfns;
> -    iov[1].iov_len = nr_pfns * sizeof(*rec_pfns);
> -
> -    iovcnt = 2;
> -
> -    if ( nr_pages )
> -    {
> -        for ( i = 0; i < nr_pfns; ++i )
> -        {
> -            if ( !guest_data[i] )
> -                continue;
> -
> -            if ( iov[iovcnt - 1].iov_base + iov[iovcnt - 1].iov_len !=
> -                 guest_data[i] )
> -            {
> -                iov[iovcnt].iov_base = guest_data[i];
> -                iov[iovcnt].iov_len = PAGE_SIZE;
> -                iovcnt++;
> -            }
> -            else
> -            {
> -                iov[iovcnt - 1].iov_len += PAGE_SIZE;
> -            }
> -            --nr_pages;
> -        }
> -    }
> -
>      if ( writev_exact(ctx->fd, iov, iovcnt) )
>      {
>          PERROR("Failed to write page data to stream");
>          goto err;
>      }
>  
> -    /* Sanity check we have sent all the pages we expected to. */
> -    assert(nr_pages == 0);
>      rc = ctx->save.nr_batch_pfns = 0;
>  
>   err:

Looking at this patch, I think it wants merging with patch 3 and
bringing ahead of patch 5.

You're undoing/redoing work in both of those patches, where I think it
would be simpler to drop guest_data rather than convert it then drop.

Moving hdrs.rec.* can be done in patch 1 (which will probably simplify
it's diff too).

~Andrew