[PATCH v6 00/16] xenguest optimisations

[PATCH v6 00/16] xenguest optimisations

[Frediano Ziglio]

Reduce some number of parts passed to writev.
Avoid possible allocation sending data with writev.
Reduce number of allocations sending memory state.

Implement and use new Xen and Linux kernel ABI to copy foreign memory.
This new ABI allows to replace theexpensive  map/copy/unmap sequence
with a single call.

Changes since v1:
- add commit to cache up to 4 pages in hypercall;
- add other 2 commits reducing chunks passed to write/writev.

Changes since v2:
- update patches commit prefixes;
- add other 2 optisations.

Changes since v3:
- address some comments;
- add patches for foreign copy optimisation.

Changes since v4:
- added Reviewed-by;
- improved commit messages;
- other minor fixes, see individual commits.

Changes since v5:
- avoids potential buffer underflow if nr_pages is 0 calling cache_alloc;
- do not overwrite errno if xenforeignmemory_map fails;
- lot of changes to "implement new foreign copy hypercall", see specific
  commit.

Edwin Török (3):
  libs/guest: allocate various migration arrays just once
  libs/call: cache up to 4 pages in hypercall bounce buffers
  PoC: libs/guest: use foreign copy during migration

Frediano Ziglio (12):
  libs/guest: Reduce number of parts in write_split_record
  libs/guest: Reduce number of I/O vectors in write_batch
  libs/guest: Reduce number of I/O vectors in write_batch
  libs/guest: Use a single write_exact in write_headers
  libs/guest: avoids using 2 indexes
  libs/guest: fill directly iov structure
  libs/ctrl: Allows writev_exact to change iov array
  libs/guest: add xg_foreignmemory_copy_{from,to}
  xen: implement new foreign copy hypercall
  privcmd: Add definition for new Linux privcmd to access new Xen
    hypercall
  libs/guest: use new hypercall if available
  libs/guest: finalize PoC

 tools/include/xen-sys/Linux/privcmd.h |  10 ++
 tools/libs/call/buffer.c              |  31 ++--
 tools/libs/call/core.c                |   3 +-
 tools/libs/call/private.h             |   8 +-
 tools/libs/ctrl/xc_private.c          |  26 +--
 tools/libs/ctrl/xc_private.h          |   2 +-
 tools/libs/guest/xg_sr_common.c       |  92 ++++++++++-
 tools/libs/guest/xg_sr_common.h       |  21 +++
 tools/libs/guest/xg_sr_restore.c      | 100 ++++++------
 tools/libs/guest/xg_sr_save.c         | 224 +++++++++++---------------
 xen/arch/x86/traps-setup.c            |   2 +-
 xen/common/memory.c                   | 145 +++++++++++++++++
 xen/include/public/memory.h           |  44 ++++-
 13 files changed, 489 insertions(+), 219 deletions(-)

-- 
2.43.0

[PATCH v6 01/16] libs/guest: Reduce number of parts in write_split_record

[Frediano Ziglio]

Small optimization.
There's no much sense to split the header in 2 pieces, it will
just take more time and space to reassemble them in the final
buffer.
This also avoids truncating combined_length to 32 bit in case of
64 bit machines potentially avoiding following record_length check
(it could still be truncated writing it in xc_sr_rhdr structure
but the following check will catch it).
The function become more coherent with following read_record
function.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
--
Changes since v2:
- change prefix in subject.

Changes since v3:
- clarify commit message.

Changes since v4:
- added Reviewed-by;
- improved commit message.
---
 tools/libs/guest/xg_sr_common.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/libs/guest/xg_sr_common.c b/tools/libs/guest/xg_sr_common.c
index c7b3c6f3bc..9b2782b5cf 100644
--- a/tools/libs/guest/xg_sr_common.c
+++ b/tools/libs/guest/xg_sr_common.c
@@ -59,11 +59,11 @@ int write_split_record(struct xc_sr_context *ctx, struct xc_sr_record *rec,
     static const char zeroes[REC_ALIGN] = {};
 
     xc_interface *xch = ctx->xch;
-    typeof(rec->length) combined_length = rec->length + sz;
+    size_t combined_length = rec->length + sz;
     size_t record_length = ROUNDUP(combined_length, REC_ALIGN);
+    struct xc_sr_rhdr rhdr = { rec->type, combined_length };
     struct iovec parts[] = {
-        { &rec->type,       sizeof(rec->type) },
-        { &combined_length, sizeof(combined_length) },
+        { &rhdr,            sizeof(rhdr) },
         { rec->data,        rec->length },
         { buf,              sz },
         { (void *)zeroes,   record_length - combined_length },
-- 
2.43.0

[PATCH v6 02/16] libs/guest: Reduce number of I/O vectors in write_batch

[Frediano Ziglio]

Small optimization.
Reduce number of pieces passed to writev.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
--
Changes since v2:
- change prefix in subject.

Changes since v4:
- added Reviewed-by.
---
 tools/libs/guest/xg_sr_save.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
index fdbceab52e..68ce1aeb98 100644
--- a/tools/libs/guest/xg_sr_save.c
+++ b/tools/libs/guest/xg_sr_save.c
@@ -97,9 +97,11 @@ static int write_batch(struct xc_sr_context *ctx)
     void *page, *orig_page;
     uint64_t *rec_pfns = NULL;
     struct iovec *iov = NULL; int iovcnt = 0;
-    struct xc_sr_rec_page_data_header hdr = { 0 };
-    struct xc_sr_record rec = {
-        .type = REC_TYPE_PAGE_DATA,
+    struct {
+        struct xc_sr_rhdr rec;
+        struct xc_sr_rec_page_data_header page_data;
+    } hdrs = {
+        { .type = REC_TYPE_PAGE_DATA },
     };
 
     assert(nr_pfns != 0);
@@ -115,7 +117,7 @@ static int write_batch(struct xc_sr_context *ctx)
     /* Pointers to locally allocated pages.  Need freeing. */
     local_pages = calloc(nr_pfns, sizeof(*local_pages));
     /* iovec[] for writev(). */
-    iov = malloc((nr_pfns + 4) * sizeof(*iov));
+    iov = malloc((nr_pfns + 2) * sizeof(*iov));
 
     if ( !mfns || !types || !errors || !guest_data || !local_pages || !iov )
     {
@@ -216,28 +218,22 @@ static int write_batch(struct xc_sr_context *ctx)
         goto err;
     }
 
-    hdr.count = nr_pfns;
+    hdrs.rec.length = sizeof(hdrs.page_data);
+    hdrs.rec.length += nr_pfns * sizeof(*rec_pfns);
+    hdrs.rec.length += nr_pages * PAGE_SIZE;
 
-    rec.length = sizeof(hdr);
-    rec.length += nr_pfns * sizeof(*rec_pfns);
-    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 = &rec.type;
-    iov[0].iov_len = sizeof(rec.type);
+    iov[0].iov_base = &hdrs;
+    iov[0].iov_len = sizeof(hdrs);
 
-    iov[1].iov_base = &rec.length;
-    iov[1].iov_len = sizeof(rec.length);
+    iov[1].iov_base = rec_pfns;
+    iov[1].iov_len = nr_pfns * sizeof(*rec_pfns);
 
-    iov[2].iov_base = &hdr;
-    iov[2].iov_len = sizeof(hdr);
-
-    iov[3].iov_base = rec_pfns;
-    iov[3].iov_len = nr_pfns * sizeof(*rec_pfns);
-
-    iovcnt = 4;
+    iovcnt = 2;
 
     if ( nr_pages )
     {
-- 
2.43.0

[PATCH v6 03/16] libs/guest: Reduce number of I/O vectors in write_batch

[Frediano Ziglio]

Each page was sent using a different iovec item. This potentially exceed
Linux maximum (1024).
Coalesce adjacent IO vector elements to attempt to reduce the number of
overall IO vectors for each operation.
Also some implementation (MiniOS) emulate writev with multiple write calls.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
--
Changes since v2:
- change prefix in subject.

Changes since v4:
- added Reviewed-by;
- improved commit message;
- minor style fix.
---
 tools/libs/guest/xg_sr_save.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
index 68ce1aeb98..eba33f861a 100644
--- a/tools/libs/guest/xg_sr_save.c
+++ b/tools/libs/guest/xg_sr_save.c
@@ -239,13 +239,21 @@ static int write_batch(struct xc_sr_context *ctx)
     {
         for ( i = 0; i < nr_pfns; ++i )
         {
-            if ( guest_data[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++;
-                --nr_pages;
             }
+            else
+            {
+                iov[iovcnt - 1].iov_len += PAGE_SIZE;
+            }
+            --nr_pages;
         }
     }
 
-- 
2.43.0

[PATCH v6 04/16] libs/guest: Use a single write_exact in write_headers

[Frediano Ziglio]

Reduce number of syscalls by coalescing the image and the domain headers
into a single I/O vector array.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
--
Changes since v2:
- change prefix in subject.

Changes since v4:
- added Reviewed-by;
- improved commit message.
---
 tools/libs/guest/xg_sr_save.c | 37 +++++++++++++++++------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
index eba33f861a..8c31f9f86c 100644
--- a/tools/libs/guest/xg_sr_save.c
+++ b/tools/libs/guest/xg_sr_save.c
@@ -10,17 +10,22 @@ static int write_headers(struct xc_sr_context *ctx, uint16_t guest_type)
 {
     xc_interface *xch = ctx->xch;
     int32_t xen_version = xc_version(xch, XENVER_version, NULL);
-    struct xc_sr_ihdr ihdr = {
-        .marker  = IHDR_MARKER,
-        .id      = htonl(IHDR_ID),
-        .version = htonl(3),
-        .options = htons(IHDR_OPT_LITTLE_ENDIAN),
-    };
-    struct xc_sr_dhdr dhdr = {
-        .type       = guest_type,
-        .page_shift = XC_PAGE_SHIFT,
-        .xen_major  = (xen_version >> 16) & 0xffff,
-        .xen_minor  = (xen_version)       & 0xffff,
+    struct {
+        struct xc_sr_ihdr ihdr;
+        struct xc_sr_dhdr dhdr;
+    } hdrs = {
+        {
+            .marker  = IHDR_MARKER,
+            .id      = htonl(IHDR_ID),
+            .version = htonl(3),
+            .options = htons(IHDR_OPT_LITTLE_ENDIAN),
+        },
+        {
+            .type       = guest_type,
+            .page_shift = XC_PAGE_SHIFT,
+            .xen_major  = (xen_version >> 16) & 0xffff,
+            .xen_minor  = (xen_version)       & 0xffff,
+        },
     };
 
     if ( xen_version < 0 )
@@ -29,15 +34,9 @@ static int write_headers(struct xc_sr_context *ctx, uint16_t guest_type)
         return -1;
     }
 
-    if ( write_exact(ctx->fd, &ihdr, sizeof(ihdr)) )
-    {
-        PERROR("Unable to write Image Header to stream");
-        return -1;
-    }
-
-    if ( write_exact(ctx->fd, &dhdr, sizeof(dhdr)) )
+    if ( write_exact(ctx->fd, &hdrs, sizeof(hdrs)) )
     {
-        PERROR("Unable to write Domain Header to stream");
+        PERROR("Unable to write Headers to stream");
         return -1;
     }
 
-- 
2.43.0

[PATCH v6 05/16] libs/guest: allocate various migration arrays just once

[Frediano Ziglio]

From: Edwin Török <edwin.torok@citrix.com>

Allocate these array just once at the start of migration,
using the maximum batch size, and free them at the end.

Signed-off-by: Edwin Török <edwin.torok@citrix.com>
Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
--
Changes since v2:
- change prefix in subject.

Changes since v3:
- fix comment style

Changes since v4:
- change order of fields in structure.
---
 tools/libs/guest/xg_sr_common.h | 13 +++++++
 tools/libs/guest/xg_sr_save.c   | 66 +++++++++++++--------------------
 2 files changed, 39 insertions(+), 40 deletions(-)

diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
index f1573aefcb..95b0564e5c 100644
--- a/tools/libs/guest/xg_sr_common.h
+++ b/tools/libs/guest/xg_sr_common.h
@@ -209,6 +209,18 @@ static inline int update_blob(struct xc_sr_blob *blob,
     return 0;
 }
 
+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];
+    int errors[MAX_BATCH_SIZE];
+};
+
 struct xc_sr_context
 {
     xc_interface *xch;
@@ -244,6 +256,7 @@ struct xc_sr_context
             unsigned long *deferred_pages;
             unsigned long nr_deferred_pages;
             xc_hypercall_buffer_t dirty_bitmap_hbuf;
+            struct xc_sr_context_save_buffers *buffers;
         } save;
 
         struct /* Restore data. */
diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
index 8c31f9f86c..4988d8040b 100644
--- a/tools/libs/guest/xg_sr_save.c
+++ b/tools/libs/guest/xg_sr_save.c
@@ -86,16 +86,16 @@ static int write_checkpoint_record(struct xc_sr_context *ctx)
 static int write_batch(struct xc_sr_context *ctx)
 {
     xc_interface *xch = ctx->xch;
-    xen_pfn_t *mfns = NULL, *types = NULL;
+    xen_pfn_t *mfns, *types;
     void *guest_mapping = NULL;
-    void **guest_data = NULL;
-    void **local_pages = NULL;
-    int *errors = NULL, rc = -1;
+    void **guest_data;
+    void **local_pages;
+    int *errors, rc = -1;
     unsigned int i, p, nr_pages = 0, nr_pages_mapped = 0;
     unsigned int nr_pfns = ctx->save.nr_batch_pfns;
     void *page, *orig_page;
-    uint64_t *rec_pfns = NULL;
-    struct iovec *iov = NULL; int iovcnt = 0;
+    uint64_t *rec_pfns;
+    struct iovec *iov; int iovcnt = 0;
     struct {
         struct xc_sr_rhdr rec;
         struct xc_sr_rec_page_data_header page_data;
@@ -104,26 +104,24 @@ static int write_batch(struct xc_sr_context *ctx)
     };
 
     assert(nr_pfns != 0);
+    assert(nr_pfns <= MAX_BATCH_SIZE);
+    assert(ctx->save.buffers);
 
     /* Mfns of the batch pfns. */
-    mfns = malloc(nr_pfns * sizeof(*mfns));
+    mfns = ctx->save.buffers->mfns;
     /* Types of the batch pfns. */
-    types = malloc(nr_pfns * sizeof(*types));
+    types = ctx->save.buffers->types;
     /* Errors from attempting to map the gfns. */
-    errors = malloc(nr_pfns * sizeof(*errors));
+    errors = ctx->save.buffers->errors;
     /* Pointers to page data to send.  Mapped gfns or local allocations. */
-    guest_data = calloc(nr_pfns, sizeof(*guest_data));
+    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 = calloc(nr_pfns, sizeof(*local_pages));
+    local_pages = ctx->save.buffers->local_pages;
+    memset(local_pages, 0, sizeof(*local_pages) * nr_pfns);
     /* iovec[] for writev(). */
-    iov = malloc((nr_pfns + 2) * sizeof(*iov));
-
-    if ( !mfns || !types || !errors || !guest_data || !local_pages || !iov )
-    {
-        ERROR("Unable to allocate arrays for a batch of %u pages",
-              nr_pfns);
-        goto err;
-    }
+    iov = ctx->save.buffers->iov;
+    rec_pfns = ctx->save.buffers->rec_pfns;
 
     for ( i = 0; i < nr_pfns; ++i )
     {
@@ -209,14 +207,6 @@ static int write_batch(struct xc_sr_context *ctx)
         }
     }
 
-    rec_pfns = malloc(nr_pfns * sizeof(*rec_pfns));
-    if ( !rec_pfns )
-    {
-        ERROR("Unable to allocate %zu bytes of memory for page data pfn list",
-              nr_pfns * sizeof(*rec_pfns));
-        goto err;
-    }
-
     hdrs.rec.length = sizeof(hdrs.page_data);
     hdrs.rec.length += nr_pfns * sizeof(*rec_pfns);
     hdrs.rec.length += nr_pages * PAGE_SIZE;
@@ -267,17 +257,13 @@ static int write_batch(struct xc_sr_context *ctx)
     rc = ctx->save.nr_batch_pfns = 0;
 
  err:
-    free(rec_pfns);
     if ( guest_mapping )
         xenforeignmemory_unmap(xch->fmem, guest_mapping, nr_pages_mapped);
     for ( i = 0; local_pages && i < nr_pfns; ++i )
+    {
         free(local_pages[i]);
-    free(iov);
-    free(local_pages);
-    free(guest_data);
-    free(errors);
-    free(types);
-    free(mfns);
+        local_pages[i] = NULL;
+    }
 
     return rc;
 }
@@ -806,18 +792,18 @@ static int setup(struct xc_sr_context *ctx)
 
     dirty_bitmap = xc_hypercall_buffer_alloc_pages(
         xch, dirty_bitmap, NRPAGES(bitmap_size(ctx->save.p2m_size)));
-    ctx->save.batch_pfns = malloc(MAX_BATCH_SIZE *
-                                  sizeof(*ctx->save.batch_pfns));
     ctx->save.deferred_pages = bitmap_alloc(ctx->save.p2m_size);
+    ctx->save.buffers = calloc(1, sizeof(*ctx->save.buffers));
 
-    if ( !ctx->save.batch_pfns || !dirty_bitmap || !ctx->save.deferred_pages )
+    if ( !dirty_bitmap || !ctx->save.deferred_pages || !ctx->save.buffers)
     {
-        ERROR("Unable to allocate memory for dirty bitmaps, batch pfns and"
-              " deferred pages");
+        ERROR("Unable to allocate memory for dirty bitmaps, deferred pages"
+              " and various batch buffers");
         rc = -1;
         errno = ENOMEM;
         goto err;
     }
+    ctx->save.batch_pfns = ctx->save.buffers->batch_pfns;
 
     rc = 0;
 
@@ -841,7 +827,7 @@ static void cleanup(struct xc_sr_context *ctx)
     xc_hypercall_buffer_free_pages(xch, dirty_bitmap,
                                    NRPAGES(bitmap_size(ctx->save.p2m_size)));
     free(ctx->save.deferred_pages);
-    free(ctx->save.batch_pfns);
+    free(ctx->save.buffers);
 }
 
 /*
-- 
2.43.0

[PATCH v6 06/16] libs/call: cache up to 4 pages in hypercall bounce buffers

[Frediano Ziglio]

From: Edwin Török <edwin.torok@citrix.com>

During migration there are a lot of mmap/munmap calls,
because `xc_get_pfn_type_batch` exceeds the default hypercall bounce
buffer cache size, and needs to allocate every time it is called.

`munmap` is slow, especially in a PV Dom0 (takes an emulation fault),
so is best avoided.

Eventually it'd be good if the memory pool from  xmalloc_tlsf.c
was reused here, but for now make it handle the commonly encountered
sizes (so far up to 4 pages).

Signed-off-by: Edwin Török <edwin.torok@citrix.com>
Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
--
Changes since v2:
- change prefix in subject.

Changes since v4:
- fix off-by-one bug.

Changes since v5:
- avoids potential buffer underflow if nr_pages is 0 calling cache_alloc.
---
 tools/libs/call/buffer.c  | 31 ++++++++++++++++++++-----------
 tools/libs/call/core.c    |  3 ++-
 tools/libs/call/private.h |  8 +++++---
 3 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/tools/libs/call/buffer.c b/tools/libs/call/buffer.c
index 155e4f9d43..2f0515c273 100644
--- a/tools/libs/call/buffer.c
+++ b/tools/libs/call/buffer.c
@@ -49,6 +49,9 @@ static void *cache_alloc(xencall_handle *xcall, size_t nr_pages)
 {
     void *p = NULL;
 
+    if ( nr_pages == 0 )
+        return NULL;
+
     cache_lock(xcall);
 
     xcall->buffer_total_allocations++;
@@ -56,13 +59,13 @@ static void *cache_alloc(xencall_handle *xcall, size_t nr_pages)
     if ( xcall->buffer_current_allocations > xcall->buffer_maximum_allocations )
         xcall->buffer_maximum_allocations = xcall->buffer_current_allocations;
 
-    if ( nr_pages > 1 )
+    if ( nr_pages > ARRAY_SIZE(xcall->buffer_cache) )
     {
         xcall->buffer_cache_toobig++;
     }
-    else if ( xcall->buffer_cache_nr > 0 )
+    else if ( xcall->buffer_cache_nr[nr_pages-1] > 0 )
     {
-        p = xcall->buffer_cache[--xcall->buffer_cache_nr];
+        p = xcall->buffer_cache[nr_pages-1][--xcall->buffer_cache_nr[nr_pages-1]];
         xcall->buffer_cache_hits++;
     }
     else
@@ -84,10 +87,10 @@ static int cache_free(xencall_handle *xcall, void *p, size_t nr_pages)
     xcall->buffer_total_releases++;
     xcall->buffer_current_allocations--;
 
-    if ( nr_pages == 1 &&
-         xcall->buffer_cache_nr < BUFFER_CACHE_SIZE )
+    if ( nr_pages && nr_pages <= ARRAY_SIZE(xcall->buffer_cache) &&
+         xcall->buffer_cache_nr[nr_pages-1] < BUFFER_CACHE_SIZE )
     {
-        xcall->buffer_cache[xcall->buffer_cache_nr++] = p;
+        xcall->buffer_cache[nr_pages-1][xcall->buffer_cache_nr[nr_pages-1]++] = p;
         rc = 1;
     }
 
@@ -108,17 +111,23 @@ void buffer_release_cache(xencall_handle *xcall)
     DBGPRINTF("current allocations:%d maximum allocations:%d",
               xcall->buffer_current_allocations,
               xcall->buffer_maximum_allocations);
-    DBGPRINTF("cache current size:%d",
-              xcall->buffer_cache_nr);
+    for ( unsigned i = 0; i < ARRAY_SIZE(xcall->buffer_cache_nr); ++i )
+    {
+        DBGPRINTF("cache current size[%u pages]:%d", i+1,
+                xcall->buffer_cache_nr[i]);
+    }
     DBGPRINTF("cache hits:%d misses:%d toobig:%d",
               xcall->buffer_cache_hits,
               xcall->buffer_cache_misses,
               xcall->buffer_cache_toobig);
 
-    while ( xcall->buffer_cache_nr > 0 )
+    for ( unsigned i = 0; i < ARRAY_SIZE(xcall->buffer_cache_nr); ++i )
     {
-        p = xcall->buffer_cache[--xcall->buffer_cache_nr];
-        osdep_free_pages(xcall, p, 1);
+        while ( xcall->buffer_cache_nr[i] > 0 )
+        {
+            p = xcall->buffer_cache[i][--xcall->buffer_cache_nr[i]];
+            osdep_free_pages(xcall, p, i + 1);
+        }
     }
 
     cache_unlock(xcall);
diff --git a/tools/libs/call/core.c b/tools/libs/call/core.c
index 02c4f8e1ae..dd8877c1a0 100644
--- a/tools/libs/call/core.c
+++ b/tools/libs/call/core.c
@@ -14,6 +14,7 @@
  */
 
 #include <stdlib.h>
+#include <string.h>
 
 #include "private.h"
 
@@ -44,7 +45,7 @@ xencall_handle *xencall_open(xentoollog_logger *logger, unsigned open_flags)
     xentoolcore__register_active_handle(&xcall->tc_ah);
 
     xcall->flags = open_flags;
-    xcall->buffer_cache_nr = 0;
+    memset(xcall->buffer_cache_nr, 0, sizeof(xcall->buffer_cache_nr));
 
     xcall->buffer_total_allocations = 0;
     xcall->buffer_total_releases = 0;
diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
index 9c3aa432ef..8e6a208975 100644
--- a/tools/libs/call/private.h
+++ b/tools/libs/call/private.h
@@ -31,13 +31,15 @@ struct xencall_handle {
     Xentoolcore__Active_Handle tc_ah;
 
     /*
-     * A simple cache of unused, single page, hypercall buffers
+     * A simple cache of unused, small, hypercall buffers
+     * buffer_cache[i]'s size is (i+1) pages
      *
      * Protected by a global lock.
      */
 #define BUFFER_CACHE_SIZE 4
-    int buffer_cache_nr;
-    void *buffer_cache[BUFFER_CACHE_SIZE];
+#define BUFFER_CACHE_NRPAGES 4
+    int buffer_cache_nr[BUFFER_CACHE_NRPAGES];
+    void *buffer_cache[BUFFER_CACHE_NRPAGES][BUFFER_CACHE_SIZE];
 
     /*
      * Hypercall buffer statistics. All protected by the global
-- 
2.43.0

[PATCH v6 07/16] libs/guest: avoids using 2 indexes

[Frediano Ziglio]

Simplify code, after the first scan of the various arrays we don't need to
keep original types and PFNs but only the ones having data.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--
Changes since v4:
- added Reviewed-by.
---
 tools/libs/guest/xg_sr_restore.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/tools/libs/guest/xg_sr_restore.c b/tools/libs/guest/xg_sr_restore.c
index e148fc594a..fb46142d87 100644
--- a/tools/libs/guest/xg_sr_restore.c
+++ b/tools/libs/guest/xg_sr_restore.c
@@ -260,9 +260,7 @@ static int process_page_data(struct xc_sr_context *ctx, unsigned int count,
     int *map_errs = malloc(count * sizeof(*map_errs));
     int rc;
     void *mapping = NULL, *guest_page = NULL;
-    unsigned int i, /* i indexes the pfns from the record. */
-        j,          /* j indexes the subset of pfns we decide to map. */
-        nr_pages = 0;
+    unsigned nr_pages;
 
     if ( !mfns || !map_errs )
     {
@@ -279,12 +277,18 @@ static int process_page_data(struct xc_sr_context *ctx, unsigned int count,
         goto err;
     }
 
-    for ( i = 0; i < count; ++i )
+    nr_pages = 0;
+    for ( unsigned i = 0; i < count; ++i )
     {
         ctx->restore.ops.set_page_type(ctx, pfns[i], types[i]);
 
-        if ( page_type_has_stream_data(types[i]) )
-            mfns[nr_pages++] = ctx->restore.ops.pfn_to_gfn(ctx, pfns[i]);
+        if ( !page_type_has_stream_data(types[i]) )
+            continue;
+
+        mfns[nr_pages] = ctx->restore.ops.pfn_to_gfn(ctx, pfns[i]);
+        pfns[nr_pages] = pfns[i];
+        types[nr_pages] = types[i];
+        nr_pages++;
     }
 
     /* Nothing to do? */
@@ -302,16 +306,13 @@ static int process_page_data(struct xc_sr_context *ctx, unsigned int count,
         goto err;
     }
 
-    for ( i = 0, j = 0; i < count; ++i )
+    for ( unsigned i = 0; i < nr_pages; ++i )
     {
-        if ( !page_type_has_stream_data(types[i]) )
-            continue;
-
-        if ( map_errs[j] )
+        if ( map_errs[i] )
         {
             rc = -1;
             ERROR("Mapping pfn %#"PRIpfn" (mfn %#"PRIpfn", type %#"PRIx32") failed with %d",
-                  pfns[i], mfns[j], types[i], map_errs[j]);
+                  pfns[i], mfns[i], types[i], map_errs[i]);
             goto err;
         }
 
@@ -337,7 +338,6 @@ static int process_page_data(struct xc_sr_context *ctx, unsigned int count,
             memcpy(guest_page, page_data, PAGE_SIZE);
         }
 
-        ++j;
         guest_page += PAGE_SIZE;
         page_data += PAGE_SIZE;
     }
-- 
2.43.0

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

[Frediano Ziglio]

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@citrix.com>
---
 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:
-- 
2.43.0

[PATCH v6 09/16] libs/ctrl: Allows writev_exact to change iov array

[Frediano Ziglio]

Avoid having to allocate and copy the array if a partial write
happens.
The implementation in tools/libs/store/xs.c already use this
signature and method.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
--
Changes since v2:
- change prefix in subject.
---
 tools/libs/ctrl/xc_private.c | 26 +++++---------------------
 tools/libs/ctrl/xc_private.h |  2 +-
 2 files changed, 6 insertions(+), 22 deletions(-)

diff --git a/tools/libs/ctrl/xc_private.c b/tools/libs/ctrl/xc_private.c
index bb0f81d6f3..946fc307aa 100644
--- a/tools/libs/ctrl/xc_private.c
+++ b/tools/libs/ctrl/xc_private.c
@@ -635,7 +635,7 @@ int write_exact(int fd, const void *data, size_t size)
 /*
  * MiniOS's libc doesn't know about writev(). Implement it as multiple write()s.
  */
-int writev_exact(int fd, const struct iovec *iov, int iovcnt)
+int writev_exact(int fd, struct iovec *iov, int iovcnt)
 {
     int rc, i;
 
@@ -649,9 +649,8 @@ int writev_exact(int fd, const struct iovec *iov, int iovcnt)
     return 0;
 }
 #else
-int writev_exact(int fd, const struct iovec *iov, int iovcnt)
+int writev_exact(int fd, struct iovec *iov, int iovcnt)
 {
-    struct iovec *local_iov = NULL;
     int rc = 0, iov_idx = 0, saved_errno = 0;
     ssize_t len;
 
@@ -686,23 +685,9 @@ int writev_exact(int fd, const struct iovec *iov, int iovcnt)
                 len -= iov[iov_idx++].iov_len;
             else
             {
-                /* Partial write of iov[iov_idx]. Copy iov so we can adjust
-                 * element iov_idx and resubmit the rest. */
-                if ( !local_iov )
-                {
-                    local_iov = malloc(iovcnt * sizeof(*iov));
-                    if ( !local_iov )
-                    {
-                        saved_errno = ENOMEM;
-                        rc = -1;
-                        goto out;
-                    }
-
-                    iov = memcpy(local_iov, iov, iovcnt * sizeof(*iov));
-                }
-
-                local_iov[iov_idx].iov_base += len;
-                local_iov[iov_idx].iov_len  -= len;
+                /* Partial write of iov[iov_idx]. */
+                iov[iov_idx].iov_base += len;
+                iov[iov_idx].iov_len  -= len;
                 break;
             }
         }
@@ -711,7 +696,6 @@ int writev_exact(int fd, const struct iovec *iov, int iovcnt)
     saved_errno = 0;
 
  out:
-    free(local_iov);
     errno = saved_errno;
     return rc;
 }
diff --git a/tools/libs/ctrl/xc_private.h b/tools/libs/ctrl/xc_private.h
index b5892ae8dc..3af996e900 100644
--- a/tools/libs/ctrl/xc_private.h
+++ b/tools/libs/ctrl/xc_private.h
@@ -383,7 +383,7 @@ int xc_flush_mmu_updates(xc_interface *xch, struct xc_mmu *mmu);
 /* Return 0 on success; -1 on error setting errno. */
 int read_exact(int fd, void *data, size_t size); /* EOF => -1, errno=0 */
 int write_exact(int fd, const void *data, size_t size);
-int writev_exact(int fd, const struct iovec *iov, int iovcnt);
+int writev_exact(int fd, struct iovec *iov, int iovcnt);
 
 int xc_ffs8(uint8_t x);
 int xc_ffs16(uint16_t x);
-- 
2.43.0

[PATCH v6 10/16] libs/guest: add xg_foreignmemory_copy_{from,to}

[Frediano Ziglio]

This change prepare code to use a new "foreign copy" hypercall.
The new hypercall will copy memory from/to a foreign domain.
The new hypercall can be emulated with a sequence of:
- map foreign memory;
- copy memory;
- unmap foreign memory.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
--
Changes since v5:
- Do not overwrite errno if xenforeignmemory_map fails.
---
 tools/libs/guest/xg_sr_common.c | 57 +++++++++++++++++++++++++++++++++
 tools/libs/guest/xg_sr_common.h |  8 +++++
 2 files changed, 65 insertions(+)

diff --git a/tools/libs/guest/xg_sr_common.c b/tools/libs/guest/xg_sr_common.c
index 9b2782b5cf..90da21c35f 100644
--- a/tools/libs/guest/xg_sr_common.c
+++ b/tools/libs/guest/xg_sr_common.c
@@ -156,6 +156,63 @@ static void __attribute__((unused)) build_assertions(void)
     BUILD_BUG_ON(sizeof(struct xc_sr_rec_hvm_params)        != 8);
 }
 
+enum {
+    foreigncopy_from,
+    foreigncopy_to
+};
+
+static int xg_foreignmemory_copy(xc_interface *xch, domid_t domid,
+                                 int dir, size_t nr_pages, void *buffer,
+                                 const xen_pfn_t foreign_pfns[nr_pages])
+{
+    if ( nr_pages == 0 )
+        return 0;
+
+    if ( !buffer || !foreign_pfns )
+    {
+        errno = EINVAL;
+        return -1;
+    }
+
+    int err[nr_pages];
+    const int prot = (dir == foreigncopy_from) ? PROT_READ : PROT_READ|PROT_WRITE;
+
+    void *p = xenforeignmemory_map(xch->fmem, domid, prot, nr_pages, foreign_pfns, err);
+    if ( !p )
+        return -1;
+
+    for ( size_t n = 0; n < nr_pages; ++n )
+        if ( err[n] )
+        {
+            xenforeignmemory_unmap(xch->fmem, p, nr_pages);
+            errno = -err[n];
+            return -1;
+        }
+
+    if ( dir == foreigncopy_from )
+        memcpy(buffer, p, nr_pages * XC_PAGE_SIZE);
+    else
+        memcpy(p, buffer, nr_pages * XC_PAGE_SIZE);
+
+    return xenforeignmemory_unmap(xch->fmem, p, nr_pages);
+}
+
+int xg_foreignmemory_copy_from(xc_interface *xch, domid_t dom,
+                               size_t nr_pages, void *dest,
+                               const xen_pfn_t source[nr_pages])
+{
+    return xg_foreignmemory_copy(xch, dom, foreigncopy_from,
+                                 nr_pages, dest, source);
+}
+
+int xg_foreignmemory_copy_to(xc_interface *xch, domid_t dom,
+                             size_t nr_pages, const xen_pfn_t dest[nr_pages],
+                             const void *source)
+{
+    return xg_foreignmemory_copy(xch, dom, foreigncopy_to,
+                                 nr_pages, (void *) source, dest);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
index b2c441b644..e37f805240 100644
--- a/tools/libs/guest/xg_sr_common.h
+++ b/tools/libs/guest/xg_sr_common.h
@@ -553,6 +553,14 @@ static inline bool page_type_has_stream_data(uint32_t type)
     }
 }
 
+int xg_foreignmemory_copy_from(xc_interface *xch, domid_t dom,
+                               size_t nr_pages, void *dest,
+                               const xen_pfn_t source[nr_pages]);
+
+int xg_foreignmemory_copy_to(xc_interface *xch, domid_t dom,
+                             size_t nr_pages, const xen_pfn_t dest[nr_pages],
+                             const void *source);
+
 #endif
 /*
  * Local variables:
-- 
2.43.0

[PATCH v6 11/16] PoC: libs/guest: use foreign copy during migration

[Frediano Ziglio]

From: Edwin Török <edwin.torok@citrix.com>

ministat confirms the improvement:

```
x baseline
+ foreigncopy
    N           Min           Max        Median           Avg        Stddev
x  20     1.1306997     1.1447931     1.1356569     1.1365742   0.003242175
+  20     0.4311504    0.44180303    0.43616705    0.43600089  0.0031094689
Difference at 95.0% confidence
	-0.700573 +/- 0.00203311
	-61.639% +/- 0.133355%
	(Student's t, pooled s = 0.00317652)
```

The tests pass too, which means that it has correctly migrated all guest
memory.

Frediano: This PoC was adapted to be included in a final series.

Signed-off-by: Edwin Török <edwin.torok@citrix.com>
Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
---
 tools/libs/guest/xg_sr_common.h  |  1 +
 tools/libs/guest/xg_sr_restore.c | 43 +++--------------
 tools/libs/guest/xg_sr_save.c    | 82 +++++++++-----------------------
 3 files changed, 31 insertions(+), 95 deletions(-)

diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
index e37f805240..d8d8a0f9f7 100644
--- a/tools/libs/guest/xg_sr_common.h
+++ b/tools/libs/guest/xg_sr_common.h
@@ -256,6 +256,7 @@ struct xc_sr_context
             unsigned long nr_deferred_pages;
             xc_hypercall_buffer_t dirty_bitmap_hbuf;
             struct xc_sr_context_save_buffers *buffers;
+            void *dest_buf;
         } save;
 
         struct /* Restore data. */
diff --git a/tools/libs/guest/xg_sr_restore.c b/tools/libs/guest/xg_sr_restore.c
index fb46142d87..ff27560ff7 100644
--- a/tools/libs/guest/xg_sr_restore.c
+++ b/tools/libs/guest/xg_sr_restore.c
@@ -259,8 +259,8 @@ static int process_page_data(struct xc_sr_context *ctx, unsigned int count,
     xen_pfn_t *mfns = malloc(count * sizeof(*mfns));
     int *map_errs = malloc(count * sizeof(*map_errs));
     int rc;
-    void *mapping = NULL, *guest_page = NULL;
     unsigned nr_pages;
+    void *const source = page_data;
 
     if ( !mfns || !map_errs )
     {
@@ -295,27 +295,8 @@ static int process_page_data(struct xc_sr_context *ctx, unsigned int count,
     if ( nr_pages == 0 )
         goto done;
 
-    mapping = guest_page = xenforeignmemory_map(
-        xch->fmem, ctx->domid, PROT_READ | PROT_WRITE,
-        nr_pages, mfns, map_errs);
-    if ( !mapping )
-    {
-        rc = -1;
-        PERROR("Unable to map %u mfns for %u pages of data",
-               nr_pages, count);
-        goto err;
-    }
-
     for ( unsigned i = 0; i < nr_pages; ++i )
     {
-        if ( map_errs[i] )
-        {
-            rc = -1;
-            ERROR("Mapping pfn %#"PRIpfn" (mfn %#"PRIpfn", type %#"PRIx32") failed with %d",
-                  pfns[i], mfns[i], types[i], map_errs[i]);
-            goto err;
-        }
-
         /* Undo page normalisation done by the saver. */
         rc = ctx->restore.ops.localise_page(ctx, types[i], page_data);
         if ( rc )
@@ -325,29 +306,19 @@ static int process_page_data(struct xc_sr_context *ctx, unsigned int count,
             goto err;
         }
 
-        if ( ctx->restore.verify )
-        {
-            /* Verify mode - compare incoming data to what we already have. */
-            if ( memcmp(guest_page, page_data, PAGE_SIZE) )
-                ERROR("verify pfn %#"PRIpfn" failed (type %#"PRIx32")",
-                      pfns[i], types[i] >> XEN_DOMCTL_PFINFO_LTAB_SHIFT);
-        }
-        else
-        {
-            /* Regular mode - copy incoming data into place. */
-            memcpy(guest_page, page_data, PAGE_SIZE);
-        }
-
-        guest_page += PAGE_SIZE;
         page_data += PAGE_SIZE;
     }
+    if ( !ctx->restore.verify )
+    {
+        rc = xg_foreignmemory_copy_to(xch, ctx->domid, nr_pages, mfns, source);
+        if ( rc < 0 )
+            goto err;
+    }
 
  done:
     rc = 0;
 
  err:
-    if ( mapping )
-        xenforeignmemory_unmap(xch->fmem, mapping, nr_pages);
 
     free(map_errs);
     free(mfns);
diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
index 8a22267fdf..7a48f6b0a3 100644
--- a/tools/libs/guest/xg_sr_save.c
+++ b/tools/libs/guest/xg_sr_save.c
@@ -87,12 +87,10 @@ static int write_batch(struct xc_sr_context *ctx)
 {
     xc_interface *xch = ctx->xch;
     xen_pfn_t *mfns, *types;
-    void *guest_mapping = NULL;
     void **local_pages;
     int *errors, rc = -1;
-    unsigned int i, p, nr_pages = 0, nr_pages_mapped = 0;
+    unsigned int i, nr_pages = 0;
     unsigned int nr_pfns = ctx->save.nr_batch_pfns;
-    void *page, *orig_page;
     uint64_t *rec_pfns;
     struct iovec *iov; int iovcnt = 0;
     struct {
@@ -167,62 +165,18 @@ static int write_batch(struct xc_sr_context *ctx)
 
     iovcnt = 2;
 
-    if ( nr_pages > 0 )
+    rc = xg_foreignmemory_copy_from(xch, ctx->domid, nr_pages, ctx->save.dest_buf, mfns);
+    if ( rc < 0 )
     {
-        guest_mapping = xenforeignmemory_map(
-            xch->fmem, ctx->domid, PROT_READ, nr_pages, mfns, errors);
-        if ( !guest_mapping )
-        {
-            PERROR("Failed to map guest pages");
-            goto err;
-        }
-        nr_pages_mapped = nr_pages;
-
-        for ( i = 0, p = 0; i < nr_pfns; ++i )
-        {
-            if ( !page_type_has_stream_data(types[i]) )
-                continue;
-
-            if ( errors[p] )
-            {
-                ERROR("Mapping of pfn %#"PRIpfn" (mfn %#"PRIpfn") failed %d",
-                      ctx->save.batch_pfns[i], mfns[p], errors[p]);
-                goto err;
-            }
-
-            orig_page = page = guest_mapping + (p * PAGE_SIZE);
-            rc = ctx->save.ops.normalise_page(ctx, types[i], &page);
-
-            if ( orig_page != page )
-                local_pages[i] = page;
-
-            if ( rc )
-            {
-                if ( rc == -1 && errno == EAGAIN )
-                {
-                    set_bit(ctx->save.batch_pfns[i], ctx->save.deferred_pages);
-                    ++ctx->save.nr_deferred_pages;
-                    types[i] = XEN_DOMCTL_PFINFO_XTAB;
-                    --nr_pages;
-                }
-                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
-            {
-                iov[iovcnt - 1].iov_len += PAGE_SIZE;
-            }
+        ERROR("xg_foreignmemory_copy_from failed");
+        goto err;
+    }
 
-            rc = -1;
-            ++p;
-        }
+    if ( nr_pages )
+    {
+        iov[iovcnt].iov_base = ctx->save.dest_buf;
+        iov[iovcnt].iov_len = nr_pages << XC_PAGE_SHIFT;
+        iovcnt++;
     }
 
     hdrs.rec.length += nr_pages * PAGE_SIZE;
@@ -239,8 +193,6 @@ static int write_batch(struct xc_sr_context *ctx)
     rc = ctx->save.nr_batch_pfns = 0;
 
  err:
-    if ( guest_mapping )
-        xenforeignmemory_unmap(xch->fmem, guest_mapping, nr_pages_mapped);
     for ( i = 0; local_pages && i < nr_pfns; ++i )
     {
         free(local_pages[i]);
@@ -765,6 +717,7 @@ static int setup(struct xc_sr_context *ctx)
 {
     xc_interface *xch = ctx->xch;
     int rc;
+    const unsigned dest_buf_len = MAX_BATCH_SIZE * XC_PAGE_SIZE;
     DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap,
                                     &ctx->save.dirty_bitmap_hbuf);
 
@@ -776,6 +729,16 @@ static int setup(struct xc_sr_context *ctx)
         xch, dirty_bitmap, NRPAGES(bitmap_size(ctx->save.p2m_size)));
     ctx->save.deferred_pages = bitmap_alloc(ctx->save.p2m_size);
     ctx->save.buffers = calloc(1, sizeof(*ctx->save.buffers));
+    ctx->save.dest_buf = NULL;
+
+    rc = posix_memalign(&ctx->save.dest_buf, XC_PAGE_SIZE, dest_buf_len);
+    if ( rc )
+    {
+        ERROR("Unable to allocate %u bytes of buffer", dest_buf_len);
+        errno = rc;
+        rc = -1;
+        goto err;
+    }
 
     if ( !dirty_bitmap || !ctx->save.deferred_pages || !ctx->save.buffers)
     {
@@ -810,6 +773,7 @@ static void cleanup(struct xc_sr_context *ctx)
                                    NRPAGES(bitmap_size(ctx->save.p2m_size)));
     free(ctx->save.deferred_pages);
     free(ctx->save.buffers);
+    free(ctx->save.dest_buf);
 }
 
 /*
-- 
2.43.0

[PATCH v6 12/16] xen: implement new foreign copy hypercall

[Frediano Ziglio]

Add a sub hypercall to __HYPERVISOR_memory_op to allow to read/write
memory from/to a foreign domain.

Extending MMUEXT_COPY_PAGE seems better on first sight but considering
that MMUEXT is meant for PV only and trying to change that sub-op this
solution is better.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
--
Changes since v4:
- Fix typo in comment.

Changes since v5:
- update xen_foreigncopy structure comments;
- move check for no frames after checking the domain;
- use mnemonic instead of 1U;
- fix page type checks;
- do not overwrite error copying back structure;
- latch MFN value;
- improved commit message.
---
 xen/common/memory.c         | 145 ++++++++++++++++++++++++++++++++++++
 xen/include/public/memory.h |  44 ++++++++++-
 2 files changed, 188 insertions(+), 1 deletion(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 3672bda025..98726766bf 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1545,6 +1545,139 @@ static int acquire_resource(
     return rc;
 }
 
+/*
+ * The "noinline" qualifier avoids the compiler to create a large function
+ * consuming quite a lot of stack.
+ */
+static int noinline mem_foreigncopy(
+    XEN_GUEST_HANDLE_PARAM(xen_foreigncopy_t) arg)
+{
+    struct domain *d, *const currd = current->domain;
+    xen_foreigncopy_t copy;
+    int rc, direction;
+
+    if ( copy_from_guest(&copy, arg, 1) )
+        return -EFAULT;
+
+    if ( copy.flags & ~XENMEM_foreigncopy_direction )
+        return -EINVAL;
+
+    direction = copy.flags & XENMEM_foreigncopy_direction;
+
+    rc = rcu_lock_remote_domain_by_id(copy.domid, &d);
+    if ( rc )
+        return rc;
+
+    if ( copy.nr_frames == 0 )
+    {
+        rcu_unlock_domain(d);
+        return 0;
+    }
+
+    /*
+     * Check we are allowed to map and access these foreign pages.
+     */
+    rc = xsm_map_gmfn_foreign(XSM_TARGET, currd, d);
+    if ( rc )
+        goto out;
+
+    do {
+        /*
+         * Arbitrary size.  Not too much stack space, and a reasonable stride
+         * for continuation checks.
+         */
+        xen_pfn_t gfn_list[32];
+        unsigned int todo = MIN(ARRAY_SIZE(gfn_list), copy.nr_frames);
+
+        rc = -EFAULT;
+        if ( copy_from_guest(gfn_list, copy.frame_list, todo) )
+            goto out;
+
+        for ( unsigned int i = 0; i < todo; i++ )
+        {
+            struct page_info *foreign_page;
+            mfn_t foreign_mfn;
+            void *foreign;
+            p2m_type_t p2mt;
+            const unsigned long valid_mask =
+#ifdef CONFIG_X86
+                p2m_to_mask(p2m_ram_rw) | p2m_to_mask(p2m_ram_logdirty);
+#else
+                p2m_to_mask(p2m_ram_rw);
+#endif
+
+            foreign_page = get_page_from_gfn(d, gfn_list[i], &p2mt, P2M_ALLOC);
+
+            if ( unlikely(!(p2m_to_mask(p2mt) & valid_mask)) && foreign_page )
+            {
+                put_page(foreign_page);
+                foreign_page = NULL;
+            }
+            if ( unlikely(!foreign_page) )
+            {
+                gdprintk(XENLOG_WARNING,
+                         "Error accessing foreign gfn %" PRI_gfn "\n",
+                         gfn_list[i]);
+                rc = -EINVAL;
+                copy.nr_frames -= i;
+                guest_handle_add_offset(copy.frame_list, i);
+                goto out;
+            }
+
+            foreign_mfn = page_to_mfn(foreign_page);
+
+            /* A page is dirtied when it's being copied to. */
+            if ( direction == XENMEM_foreigncopy_to )
+                paging_mark_dirty(d, foreign_mfn);
+
+            foreign = map_domain_page(foreign_mfn);
+            if ( direction == XENMEM_foreigncopy_from )
+                rc = copy_to_guest(copy.buffer, foreign, PAGE_SIZE);
+            else
+                rc = copy_from_guest(foreign, copy.buffer, PAGE_SIZE);
+            unmap_domain_page(foreign);
+            put_page(foreign_page);
+
+            if ( unlikely(rc) )
+            {
+                gdprintk(XENLOG_WARNING,
+                         "Error %d copying gfn %" PRI_gfn "\n",
+                         -rc, gfn_list[i]);
+                copy.nr_frames -= i;
+                guest_handle_add_offset(copy.frame_list, i);
+                goto out;
+            }
+
+            guest_handle_add_offset(copy.buffer, PAGE_SIZE);
+        }
+
+        copy.nr_frames -= todo;
+        guest_handle_add_offset(copy.frame_list, todo);
+
+        if ( copy.nr_frames && hypercall_preempt_check() )
+        {
+            rc = hypercall_create_continuation(
+                __HYPERVISOR_memory_op, "lh", XENMEM_foreigncopy, arg);
+            goto out;
+        }
+    } while ( copy.nr_frames );
+
+    rc = 0;
+
+ out:
+    rcu_unlock_domain(d);
+
+    /*
+     * Update in all cases, it allows the caller to know how many
+     * frames were successfully copied and the continuation to
+     * continue correctly.
+     */
+    if ( __copy_to_guest(arg, &copy, 1) && rc >= 0 )
+        rc = -EFAULT;
+
+    return rc;
+}
+
 long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     struct domain *d, *curr_d = current->domain;
@@ -2012,6 +2145,18 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
             start_extent);
         break;
 
+    case XENMEM_foreigncopy:
+        /*
+         * Instead of using "start_extent" we update the structure back,
+         * we update it back in anyway to tell caller were the copy
+         * stopped.
+         */
+        if ( unlikely(start_extent) )
+            return -EINVAL;
+
+        rc = mem_foreigncopy(guest_handle_cast(arg, xen_foreigncopy_t));
+        break;
+
     default:
         rc = arch_memory_op(cmd, arg);
         break;
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index bd9fc37b52..dbf86fd595 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -740,7 +740,49 @@ struct xen_vnuma_topology_info {
 typedef struct xen_vnuma_topology_info xen_vnuma_topology_info_t;
 DEFINE_XEN_GUEST_HANDLE(xen_vnuma_topology_info_t);
 
-/* Next available subop number is 29 */
+/*
+ * Copy memory from/to a given domain.
+ * As this call requires target access and guest with target access won't be
+ * compat guests supported for compat guests this is not implemented.
+ */
+#define XENMEM_foreigncopy 29
+struct xen_foreigncopy {
+    /* IN - The domain whose memory is to be copied. */
+    domid_t domid;
+
+    /* IN - Flags. */
+#define XENMEM_foreigncopy_from 0
+#define XENMEM_foreigncopy_to 1
+#define XENMEM_foreigncopy_direction 1
+    uint16_t flags;
+
+    /*
+     * IN/OUT
+     *
+     * As an IN parameter number of frames of the domain to be copied.
+     * On output on error updated number of frames left.
+     */
+    uint32_t nr_frames;
+
+    /*
+     * IN/OUT
+     *
+     * Frames to be copied.
+     * On output on error updated to point to first frame unhandled.
+     */
+    XEN_GUEST_HANDLE(xen_pfn_t) frame_list;
+
+    /*
+     * IN/OUT
+     *
+     * Userspace buffer to read/write from.
+     */
+    XEN_GUEST_HANDLE(uint8) buffer;
+};
+typedef struct xen_foreigncopy xen_foreigncopy_t;
+DEFINE_XEN_GUEST_HANDLE(xen_foreigncopy_t);
+
+/* Next available subop number is 30 */
 
 #endif /* __XEN_PUBLIC_MEMORY_H__ */
 
-- 
2.43.0

[PATCH v6 13/16] privcmd: Add definition for new Linux privcmd to access new Xen hypercall

[Frediano Ziglio]

Userspace should use new ioctl to access new hypercall.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
--
Changes since v4:
- update comment.
---
 tools/include/xen-sys/Linux/privcmd.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/include/xen-sys/Linux/privcmd.h b/tools/include/xen-sys/Linux/privcmd.h
index 607dfa2287..7a3c41308b 100644
--- a/tools/include/xen-sys/Linux/privcmd.h
+++ b/tools/include/xen-sys/Linux/privcmd.h
@@ -100,6 +100,14 @@ typedef struct privcmd_pcidev_get_gsi {
 	__u32 gsi;
 } privcmd_pcidev_get_gsi_t;
 
+typedef struct privcmd_foreigncopy {
+	domid_t dom;          /* Foreign domain. */
+	__u16 dir;            /* Direction,  0 from, 1 to. */
+	__u32 num;            /* Number of pages to copy. */
+	const xen_pfn_t __user *pfns; /* Array of pfns. */
+	void __user *buffer;  /* Buffer to copy to/from. */
+} privcmd_foreigncopy_t;
+
 /*
  * @cmd: IOCTL_PRIVCMD_HYPERCALL
  * @arg: &privcmd_hypercall_t
@@ -121,6 +129,8 @@ typedef struct privcmd_pcidev_get_gsi {
 	_IOC(_IOC_NONE, 'P', 7, sizeof(privcmd_mmap_resource_t))
 #define IOCTL_PRIVCMD_PCIDEV_GET_GSI			\
 	_IOC(_IOC_NONE, 'P', 10, sizeof(privcmd_pcidev_get_gsi_t))
+#define IOCTL_PRIVCMD_FOREIGNCOPY				\
+	_IOWR('P', 11, privcmd_foreigncopy_t)
 #define IOCTL_PRIVCMD_UNIMPLEMENTED				\
 	_IOC(_IOC_NONE, 'P', 0xFF, 0)
 
-- 
2.43.0

[PATCH v6 14/16] libs/guest: use new hypercall if available

[Frediano Ziglio]

Use new hypercall if available, otherwise fall back to map+copy+unmap
sequence.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
--
Changes since v4:
- use int8_t instead of char for signed type.
---
 tools/libs/guest/xg_sr_common.c | 47 ++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 9 deletions(-)

diff --git a/tools/libs/guest/xg_sr_common.c b/tools/libs/guest/xg_sr_common.c
index 90da21c35f..c2e7d78e33 100644
--- a/tools/libs/guest/xg_sr_common.c
+++ b/tools/libs/guest/xg_sr_common.c
@@ -156,11 +156,6 @@ static void __attribute__((unused)) build_assertions(void)
     BUILD_BUG_ON(sizeof(struct xc_sr_rec_hvm_params)        != 8);
 }
 
-enum {
-    foreigncopy_from,
-    foreigncopy_to
-};
-
 static int xg_foreignmemory_copy(xc_interface *xch, domid_t domid,
                                  int dir, size_t nr_pages, void *buffer,
                                  const xen_pfn_t foreign_pfns[nr_pages])
@@ -174,8 +169,42 @@ static int xg_foreignmemory_copy(xc_interface *xch, domid_t domid,
         return -1;
     }
 
+    /*
+     * If foreign copy is supported, -1 not initialized, 0 not supported,
+     * 1 supported.
+     */
+    static int8_t foreign_copy_supported = -1;
+
+    if ( foreign_copy_supported )
+    {
+        int rc;
+        privcmd_foreigncopy_t copy = {
+            .dom = domid,
+            .dir = dir,
+            .num = nr_pages,
+            .buffer = buffer,
+        };
+        DECLARE_HYPERCALL_BOUNCE_IN(foreign_pfns, nr_pages * sizeof(xen_pfn_t));
+
+        if ( xc_hypercall_bounce_pre(xch, foreign_pfns) )
+            return -1;
+
+        copy.pfns = foreign_pfns;
+
+        rc = ioctl(xencall_fd(xch->xcall), IOCTL_PRIVCMD_FOREIGNCOPY, &copy);
+        if ( foreign_copy_supported < 0 )
+            foreign_copy_supported =
+                (!rc || (errno != ENOTTY && errno != ENOSYS));
+
+        xc_hypercall_bounce_post(xch, foreign_pfns);
+
+        if ( foreign_copy_supported )
+            return rc;
+    }
+
+    /* Fallback, emulate. */
     int err[nr_pages];
-    const int prot = (dir == foreigncopy_from) ? PROT_READ : PROT_READ|PROT_WRITE;
+    const int prot = (dir == XENMEM_foreigncopy_from) ? PROT_READ : PROT_READ|PROT_WRITE;
 
     void *p = xenforeignmemory_map(xch->fmem, domid, prot, nr_pages, foreign_pfns, err);
     if ( !p )
@@ -189,7 +218,7 @@ static int xg_foreignmemory_copy(xc_interface *xch, domid_t domid,
             return -1;
         }
 
-    if ( dir == foreigncopy_from )
+    if ( dir == XENMEM_foreigncopy_from )
         memcpy(buffer, p, nr_pages * XC_PAGE_SIZE);
     else
         memcpy(p, buffer, nr_pages * XC_PAGE_SIZE);
@@ -201,7 +230,7 @@ int xg_foreignmemory_copy_from(xc_interface *xch, domid_t dom,
                                size_t nr_pages, void *dest,
                                const xen_pfn_t source[nr_pages])
 {
-    return xg_foreignmemory_copy(xch, dom, foreigncopy_from,
+    return xg_foreignmemory_copy(xch, dom, XENMEM_foreigncopy_from,
                                  nr_pages, dest, source);
 }
 
@@ -209,7 +238,7 @@ int xg_foreignmemory_copy_to(xc_interface *xch, domid_t dom,
                              size_t nr_pages, const xen_pfn_t dest[nr_pages],
                              const void *source)
 {
-    return xg_foreignmemory_copy(xch, dom, foreigncopy_to,
+    return xg_foreignmemory_copy(xch, dom, XENMEM_foreigncopy_to,
                                  nr_pages, (void *) source, dest);
 }
 
-- 
2.43.0

[PATCH v6 15/16] libs/guest: finalize PoC

[Frediano Ziglio]

Remove now unused map_errs array.
Test and restore verification code.
Report correctly errors from writev_exact.
Allocate verification buffer using hypercall buffer to avoid errors
using hypercall.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
---
 tools/libs/guest/xg_sr_common.h  |  4 +-
 tools/libs/guest/xg_sr_restore.c | 45 +++++++++++++++--
 tools/libs/guest/xg_sr_save.c    | 83 +++++++++++++++++++++-----------
 3 files changed, 98 insertions(+), 34 deletions(-)

diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
index d8d8a0f9f7..cd562f028a 100644
--- a/tools/libs/guest/xg_sr_common.h
+++ b/tools/libs/guest/xg_sr_common.h
@@ -217,7 +217,6 @@ struct xc_sr_context_save_buffers
     void *local_pages[MAX_BATCH_SIZE];
     struct iovec iov[MAX_BATCH_SIZE + 2]; /* Headers + data. */
     uint64_t rec_pfns[MAX_BATCH_SIZE];
-    int errors[MAX_BATCH_SIZE];
 };
 
 struct xc_sr_context
@@ -255,8 +254,8 @@ struct xc_sr_context
             unsigned long *deferred_pages;
             unsigned long nr_deferred_pages;
             xc_hypercall_buffer_t dirty_bitmap_hbuf;
+            xc_hypercall_buffer_t dest_buf;
             struct xc_sr_context_save_buffers *buffers;
-            void *dest_buf;
         } save;
 
         struct /* Restore data. */
@@ -267,6 +266,7 @@ struct xc_sr_context
             int send_back_fd;
             unsigned long p2m_size;
             xc_hypercall_buffer_t dirty_bitmap_hbuf;
+            xc_hypercall_buffer_t verify_buf;
 
             /* From Image Header. */
             uint32_t format_version;
diff --git a/tools/libs/guest/xg_sr_restore.c b/tools/libs/guest/xg_sr_restore.c
index ff27560ff7..b2df36c6f6 100644
--- a/tools/libs/guest/xg_sr_restore.c
+++ b/tools/libs/guest/xg_sr_restore.c
@@ -257,16 +257,15 @@ static int process_page_data(struct xc_sr_context *ctx, unsigned int count,
 {
     xc_interface *xch = ctx->xch;
     xen_pfn_t *mfns = malloc(count * sizeof(*mfns));
-    int *map_errs = malloc(count * sizeof(*map_errs));
     int rc;
     unsigned nr_pages;
     void *const source = page_data;
 
-    if ( !mfns || !map_errs )
+    if ( !mfns )
     {
         rc = -1;
         ERROR("Failed to allocate %zu bytes to process page data",
-              count * (sizeof(*mfns) + sizeof(*map_errs)));
+              count * sizeof(*mfns));
         goto err;
     }
 
@@ -314,13 +313,33 @@ static int process_page_data(struct xc_sr_context *ctx, unsigned int count,
         if ( rc < 0 )
             goto err;
     }
+    else
+    {
+        DECLARE_HYPERCALL_BUFFER_SHADOW(uint8_t, verify_buf,
+                                        &ctx->restore.verify_buf);
+
+        rc = xg_foreignmemory_copy_from(xch, ctx->domid, nr_pages, verify_buf, mfns);
+        if ( rc < 0 )
+            goto err;
+
+        void *guest_page = verify_buf;
+        page_data = source;
+        for ( unsigned i = 0; i < nr_pages; ++i )
+        {
+            /* Verify mode - compare incoming data to what we already have. */
+            if ( memcmp(guest_page, page_data, PAGE_SIZE) )
+                ERROR("verify pfn %#"PRIpfn" failed (type %#"PRIx32")",
+                      pfns[i], types[i] >> XEN_DOMCTL_PFINFO_LTAB_SHIFT);
+
+            guest_page += PAGE_SIZE;
+            page_data += PAGE_SIZE;
+        }
+    }
 
  done:
     rc = 0;
 
  err:
-
-    free(map_errs);
     free(mfns);
 
     return rc;
@@ -710,6 +729,18 @@ static int setup(struct xc_sr_context *ctx)
     int rc;
     DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap,
                                     &ctx->restore.dirty_bitmap_hbuf);
+    DECLARE_HYPERCALL_BUFFER_SHADOW(uint8_t, verify_buf,
+                                    &ctx->restore.verify_buf);
+
+    verify_buf = xc_hypercall_buffer_alloc_pages(
+        xch, verify_buf, MAX_BATCH_SIZE);
+
+    if ( !verify_buf )
+    {
+        ERROR("Unable to allocate memory for test buffer");
+        rc = -1;
+        goto err;
+    }
 
     if ( ctx->stream_type == XC_STREAM_COLO )
     {
@@ -758,6 +789,8 @@ static void cleanup(struct xc_sr_context *ctx)
     unsigned int i;
     DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap,
                                     &ctx->restore.dirty_bitmap_hbuf);
+    DECLARE_HYPERCALL_BUFFER_SHADOW(uint8_t, verify_buf,
+                                    &ctx->restore.verify_buf);
 
     for ( i = 0; i < ctx->restore.buffered_rec_num; i++ )
         free(ctx->restore.buffered_records[i].data);
@@ -766,6 +799,8 @@ static void cleanup(struct xc_sr_context *ctx)
         xc_hypercall_buffer_free_pages(
             xch, dirty_bitmap, NRPAGES(bitmap_size(ctx->restore.p2m_size)));
 
+    xc_hypercall_buffer_free_pages(xch, verify_buf, MAX_BATCH_SIZE);
+
     free(ctx->restore.buffered_records);
     free(ctx->restore.populated_pfns);
 
diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
index 7a48f6b0a3..f6ada3152d 100644
--- a/tools/libs/guest/xg_sr_save.c
+++ b/tools/libs/guest/xg_sr_save.c
@@ -88,7 +88,7 @@ static int write_batch(struct xc_sr_context *ctx)
     xc_interface *xch = ctx->xch;
     xen_pfn_t *mfns, *types;
     void **local_pages;
-    int *errors, rc = -1;
+    int rc = -1;
     unsigned int i, nr_pages = 0;
     unsigned int nr_pfns = ctx->save.nr_batch_pfns;
     uint64_t *rec_pfns;
@@ -108,8 +108,6 @@ static int write_batch(struct xc_sr_context *ctx)
     mfns = ctx->save.buffers->mfns;
     /* Types of the batch pfns. */
     types = ctx->save.buffers->types;
-    /* Errors from attempting to map the gfns. */
-    errors = ctx->save.buffers->errors;
     /* Pointers to locally allocated pages.  Need freeing. */
     local_pages = ctx->save.buffers->local_pages;
     memset(local_pages, 0, sizeof(*local_pages) * nr_pfns);
@@ -165,18 +163,54 @@ static int write_batch(struct xc_sr_context *ctx)
 
     iovcnt = 2;
 
-    rc = xg_foreignmemory_copy_from(xch, ctx->domid, nr_pages, ctx->save.dest_buf, mfns);
-    if ( rc < 0 )
-    {
-        ERROR("xg_foreignmemory_copy_from failed");
-        goto err;
-    }
-
     if ( nr_pages )
     {
-        iov[iovcnt].iov_base = ctx->save.dest_buf;
-        iov[iovcnt].iov_len = nr_pages << XC_PAGE_SHIFT;
-        iovcnt++;
+        int p;
+        void *page, *orig_page;
+
+        DECLARE_HYPERCALL_BUFFER_SHADOW(uint8_t, dest_buf,
+                                        &ctx->save.dest_buf);
+
+        rc = xg_foreignmemory_copy_from(xch, ctx->domid, nr_pages, dest_buf, mfns);
+        if ( rc < 0 )
+        {
+            ERROR("xg_foreignmemory_copy_from failed");
+            goto err;
+        }
+
+        for ( i = 0, p = 0; i < nr_pfns; ++i )
+        {
+            if ( !page_type_has_stream_data(types[i]) )
+                continue;
+
+            orig_page = page = dest_buf + (p * PAGE_SIZE);
+            rc = ctx->save.ops.normalise_page(ctx, types[i], &page);
+
+            if ( orig_page != page )
+                local_pages[i] = page;
+
+            if ( rc )
+            {
+                if ( rc != -1 || errno != EAGAIN )
+                    goto err;
+
+                set_bit(ctx->save.batch_pfns[i], ctx->save.deferred_pages);
+                ++ctx->save.nr_deferred_pages;
+                types[i] = XEN_DOMCTL_PFINFO_XTAB;
+                --nr_pages;
+            }
+            else if ( iov[iovcnt-1].iov_base + iov[iovcnt-1].iov_len == page )
+            {
+                iov[iovcnt-1].iov_len += PAGE_SIZE;
+            }
+            else
+            {
+                iov[iovcnt].iov_base = page;
+                iov[iovcnt].iov_len = PAGE_SIZE;
+                iovcnt++;
+            }
+            ++p;
+        }
     }
 
     hdrs.rec.length += nr_pages * PAGE_SIZE;
@@ -187,6 +221,7 @@ static int write_batch(struct xc_sr_context *ctx)
     if ( writev_exact(ctx->fd, iov, iovcnt) )
     {
         PERROR("Failed to write page data to stream");
+        rc = -1;
         goto err;
     }
 
@@ -717,30 +752,23 @@ static int setup(struct xc_sr_context *ctx)
 {
     xc_interface *xch = ctx->xch;
     int rc;
-    const unsigned dest_buf_len = MAX_BATCH_SIZE * XC_PAGE_SIZE;
     DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap,
                                     &ctx->save.dirty_bitmap_hbuf);
+    DECLARE_HYPERCALL_BUFFER_SHADOW(uint8_t, dest_buf,
+                                    &ctx->save.dest_buf);
 
     rc = ctx->save.ops.setup(ctx);
     if ( rc )
         goto err;
 
+    dest_buf = xc_hypercall_buffer_alloc_pages(
+        xch, dest_buf, MAX_BATCH_SIZE);
     dirty_bitmap = xc_hypercall_buffer_alloc_pages(
         xch, dirty_bitmap, NRPAGES(bitmap_size(ctx->save.p2m_size)));
     ctx->save.deferred_pages = bitmap_alloc(ctx->save.p2m_size);
     ctx->save.buffers = calloc(1, sizeof(*ctx->save.buffers));
-    ctx->save.dest_buf = NULL;
-
-    rc = posix_memalign(&ctx->save.dest_buf, XC_PAGE_SIZE, dest_buf_len);
-    if ( rc )
-    {
-        ERROR("Unable to allocate %u bytes of buffer", dest_buf_len);
-        errno = rc;
-        rc = -1;
-        goto err;
-    }
 
-    if ( !dirty_bitmap || !ctx->save.deferred_pages || !ctx->save.buffers)
+    if ( !dirty_bitmap || !ctx->save.deferred_pages || !ctx->save.buffers || !dest_buf )
     {
         ERROR("Unable to allocate memory for dirty bitmaps, deferred pages"
               " and various batch buffers");
@@ -761,7 +789,8 @@ static void cleanup(struct xc_sr_context *ctx)
     xc_interface *xch = ctx->xch;
     DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap,
                                     &ctx->save.dirty_bitmap_hbuf);
-
+    DECLARE_HYPERCALL_BUFFER_SHADOW(uint8_t, dest_buf,
+                                    &ctx->save.dest_buf);
 
     xc_shadow_control(xch, ctx->domid, XEN_DOMCTL_SHADOW_OP_OFF,
                       NULL, 0);
@@ -771,9 +800,9 @@ static void cleanup(struct xc_sr_context *ctx)
 
     xc_hypercall_buffer_free_pages(xch, dirty_bitmap,
                                    NRPAGES(bitmap_size(ctx->save.p2m_size)));
+    xc_hypercall_buffer_free_pages(xch, dest_buf, MAX_BATCH_SIZE);
     free(ctx->save.deferred_pages);
     free(ctx->save.buffers);
-    free(ctx->save.dest_buf);
 }
 
 /*
-- 
2.43.0

[PATCH Linux v6 16/16] xen/privcmd: Add new ABI to allow copying foreign memory

[Frediano Ziglio]

This new ABI allows to copy foreign domain memory to/from a buffer.
This avoids having to map/copy/unmap foreign memory which is
expensive.
This operation is done particularly when migrating VMs.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
--
Changes since v4:
- fix wrong assign;
- use set_xen_guest_handle to set handle;
- wrap slow hypercall with xen_preemptible_hcall_{begin,end};
- use _IOWR for ioctl code to be more specific;
- use __copy_to_user if buffer already checked.

---
 arch/x86/include/asm/xen/interface.h |  1 +
 drivers/xen/privcmd.c                | 49 ++++++++++++++++++++++++++++
 include/uapi/xen/privcmd.h           | 10 ++++++
 include/xen/interface/memory.h       | 37 +++++++++++++++++++++
 4 files changed, 97 insertions(+)

diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h
index a078a2b0f032..bac3c3bc60fd 100644
--- a/arch/x86/include/asm/xen/interface.h
+++ b/arch/x86/include/asm/xen/interface.h
@@ -91,6 +91,7 @@ DEFINE_GUEST_HANDLE(int);
 DEFINE_GUEST_HANDLE(void);
 DEFINE_GUEST_HANDLE(uint64_t);
 DEFINE_GUEST_HANDLE(uint32_t);
+DEFINE_GUEST_HANDLE(uint8_t);
 DEFINE_GUEST_HANDLE(xen_pfn_t);
 DEFINE_GUEST_HANDLE(xen_ulong_t);
 #endif
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 725a49a0eee7..67bf085d91e1 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -1522,6 +1522,51 @@ static inline void privcmd_ioeventfd_exit(void)
 }
 #endif /* CONFIG_XEN_PRIVCMD_EVENTFD */
 
+static long privcmd_ioctl_foreigncopy(
+	struct file *file, void __user *udata)
+{
+	const struct privcmd_data *const data = file->private_data;
+	long ret;
+	struct privcmd_foreigncopy copy;
+	struct xen_foreigncopy xcopy;
+
+	if (copy_from_user(&copy, udata, sizeof(copy)))
+		return -EFAULT;
+	if (copy.dir & ~1u)
+		return -EINVAL;
+	if (copy.num >= U32_MAX >> PAGE_SHIFT)
+		return -EINVAL;
+	if (!access_ok(copy.pfns, copy.num * sizeof(*copy.pfns)))
+		return -EFAULT;
+	if (!access_ok(copy.buffer, copy.num << PAGE_SHIFT))
+		return -EFAULT;
+
+	/* If restriction is in place, check the domid matches */
+	if (data->domid != DOMID_INVALID && data->domid != copy.dom)
+		return -EPERM;
+
+	xcopy.domid = copy.dom;
+	xcopy.flags = copy.dir;
+	xcopy.nr_frames = copy.num;
+	set_xen_guest_handle(xcopy.frame_list,  (__force xen_pfn_t *)copy.pfns);
+	set_xen_guest_handle(xcopy.buffer, (__force uint8_t *)copy.buffer);
+
+	xen_preemptible_hcall_begin();
+	ret = HYPERVISOR_memory_op(XENMEM_foreigncopy, &xcopy);
+	xen_preemptible_hcall_end();
+
+	/* copy values back in case of error */
+	if (ret) {
+		copy.num = xcopy.nr_frames;
+		copy.pfns = xcopy.frame_list;
+		copy.buffer = xcopy.buffer;
+		if (__copy_to_user(udata, &copy, sizeof(copy)))
+			ret = -EFAULT;
+	}
+
+	return ret;
+}
+
 static long privcmd_ioctl(struct file *file,
 			  unsigned int cmd, unsigned long data)
 {
@@ -1569,6 +1614,10 @@ static long privcmd_ioctl(struct file *file,
 		ret = privcmd_ioctl_pcidev_get_gsi(file, udata);
 		break;
 
+	case IOCTL_PRIVCMD_FOREIGNCOPY:
+		ret = privcmd_ioctl_foreigncopy(file, udata);
+		break;
+
 	default:
 		break;
 	}
diff --git a/include/uapi/xen/privcmd.h b/include/uapi/xen/privcmd.h
index 8e2c8fd44764..993b501e35bf 100644
--- a/include/uapi/xen/privcmd.h
+++ b/include/uapi/xen/privcmd.h
@@ -131,6 +131,14 @@ struct privcmd_pcidev_get_gsi {
 	__u32 gsi;
 };
 
+struct privcmd_foreigncopy {
+	domid_t dom;		/* foreign domain */
+	__u16 dir;		/* direction,  0 from, 1 to */
+	__u32 num;		/* number of pages to copy */
+	const xen_pfn_t __user *pfns;	/* array of pfns */
+	void __user *buffer;	/* buffer to copy to/from */
+};
+
 /*
  * @cmd: IOCTL_PRIVCMD_HYPERCALL
  * @arg: &privcmd_hypercall_t
@@ -164,5 +172,7 @@ struct privcmd_pcidev_get_gsi {
 	_IOW('P', 9, struct privcmd_ioeventfd)
 #define IOCTL_PRIVCMD_PCIDEV_GET_GSI				\
 	_IOC(_IOC_NONE, 'P', 10, sizeof(struct privcmd_pcidev_get_gsi))
+#define IOCTL_PRIVCMD_FOREIGNCOPY				\
+	_IOWR('P', 11, struct privcmd_foreigncopy)
 
 #endif /* __LINUX_PUBLIC_PRIVCMD_H__ */
diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
index 1a371a825c55..5981402fccde 100644
--- a/include/xen/interface/memory.h
+++ b/include/xen/interface/memory.h
@@ -325,4 +325,41 @@ struct xen_mem_acquire_resource {
 };
 DEFINE_GUEST_HANDLE_STRUCT(xen_mem_acquire_resource);
 
+/*
+ * Copy memory from/to a given domain.
+ */
+#define XENMEM_foreigncopy 29
+struct xen_foreigncopy {
+    /* IN - The domain whose resource is to be copied */
+    domid_t domid;
+
+    /* IN - Flags */
+#define XENMEM_foreigncopy_from 0
+#define XENMEM_foreigncopy_to 1
+#define XENMEM_foreigncopy_direction 1
+    uint16_t flags;
+
+    /*
+     * IN
+     *
+     * As an IN parameter number of frames of the domain to be copied.
+     */
+    uint32_t nr_frames;
+
+    /*
+     * IN
+     *
+     * Frames to be copied.
+     */
+    GUEST_HANDLE(xen_pfn_t) frame_list;
+
+    /*
+     * IN/OUT
+     *
+     * Userspace buffer to read/write from.
+     */
+    GUEST_HANDLE(uint8_t) buffer;
+};
+DEFINE_GUEST_HANDLE_STRUCT(xen_foreigncopy);
+
 #endif /* __XEN_PUBLIC_MEMORY_H__ */
-- 
2.54.0

Re: [PATCH v6 12/16] xen: implement new foreign copy hypercall

[Jan Beulich]

On 19.06.2026 15:04, Frediano Ziglio wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1545,6 +1545,139 @@ static int acquire_resource(
>      return rc;
>  }
>  
> +/*
> + * The "noinline" qualifier avoids the compiler to create a large function
> + * consuming quite a lot of stack.
> + */
> +static int noinline mem_foreigncopy(
> +    XEN_GUEST_HANDLE_PARAM(xen_foreigncopy_t) arg)
> +{
> +    struct domain *d, *const currd = current->domain;
> +    xen_foreigncopy_t copy;
> +    int rc, direction;
> +
> +    if ( copy_from_guest(&copy, arg, 1) )
> +        return -EFAULT;
> +
> +    if ( copy.flags & ~XENMEM_foreigncopy_direction )
> +        return -EINVAL;
> +
> +    direction = copy.flags & XENMEM_foreigncopy_direction;
> +
> +    rc = rcu_lock_remote_domain_by_id(copy.domid, &d);

Iirc I did ask before why this isn't ..._by_any_id().

> +    if ( rc )
> +        return rc;
> +
> +    if ( copy.nr_frames == 0 )
> +    {
> +        rcu_unlock_domain(d);
> +        return 0;
> +    }

Any reason this cannot also be "goto out"? The more that now that you have
moved this past the domid validity check, imo it should further move to ...

> +    /*
> +     * Check we are allowed to map and access these foreign pages.
> +     */
> +    rc = xsm_map_gmfn_foreign(XSM_TARGET, currd, d);
> +    if ( rc )
> +        goto out;

... below here. Perhaps simply as

    if ( rc || !copy.nr_frames )
        goto out;

> +    do {
> +        /*
> +         * Arbitrary size.  Not too much stack space, and a reasonable stride
> +         * for continuation checks.
> +         */
> +        xen_pfn_t gfn_list[32];
> +        unsigned int todo = MIN(ARRAY_SIZE(gfn_list), copy.nr_frames);
> +
> +        rc = -EFAULT;
> +        if ( copy_from_guest(gfn_list, copy.frame_list, todo) )
> +            goto out;
> +
> +        for ( unsigned int i = 0; i < todo; i++ )
> +        {
> +            struct page_info *foreign_page;
> +            mfn_t foreign_mfn;
> +            void *foreign;
> +            p2m_type_t p2mt;
> +            const unsigned long valid_mask =
> +#ifdef CONFIG_X86
> +                p2m_to_mask(p2m_ram_rw) | p2m_to_mask(p2m_ram_logdirty);
> +#else
> +                p2m_to_mask(p2m_ram_rw);
> +#endif

The set of permitted types didn't change, yet a justification for the resulting
limitation also didn't appear.

> +            foreign_page = get_page_from_gfn(d, gfn_list[i], &p2mt, P2M_ALLOC);
> +
> +            if ( unlikely(!(p2m_to_mask(p2mt) & valid_mask)) && foreign_page )
> +            {
> +                put_page(foreign_page);
> +                foreign_page = NULL;
> +            }
> +            if ( unlikely(!foreign_page) )
> +            {
> +                gdprintk(XENLOG_WARNING,
> +                         "Error accessing foreign gfn %" PRI_gfn "\n",
> +                         gfn_list[i]);
> +                rc = -EINVAL;
> +                copy.nr_frames -= i;
> +                guest_handle_add_offset(copy.frame_list, i);
> +                goto out;
> +            }
> +
> +            foreign_mfn = page_to_mfn(foreign_page);
> +
> +            /* A page is dirtied when it's being copied to. */
> +            if ( direction == XENMEM_foreigncopy_to )
> +                paging_mark_dirty(d, foreign_mfn);
> +
> +            foreign = map_domain_page(foreign_mfn);
> +            if ( direction == XENMEM_foreigncopy_from )
> +                rc = copy_to_guest(copy.buffer, foreign, PAGE_SIZE);
> +            else
> +                rc = copy_from_guest(foreign, copy.buffer, PAGE_SIZE);

You cannot validly write to the page without holding a PGT_writable ref.
Else you might overwrite a page table or a descriptor table in a PV guest.

Once again - can you please make sure you have addressed earlier review
comments, before sending a new version? I did point this out before.

> +            unmap_domain_page(foreign);
> +            put_page(foreign_page);
> +
> +            if ( unlikely(rc) )
> +            {
> +                gdprintk(XENLOG_WARNING,
> +                         "Error %d copying gfn %" PRI_gfn "\n",
> +                         -rc, gfn_list[i]);

Why "-rc"? (See other log messages including error codes.)

> +                copy.nr_frames -= i;
> +                guest_handle_add_offset(copy.frame_list, i);
> +                goto out;
> +            }
> +
> +            guest_handle_add_offset(copy.buffer, PAGE_SIZE);
> +        }
> +
> +        copy.nr_frames -= todo;
> +        guest_handle_add_offset(copy.frame_list, todo);

Don't you need to also update copy.buffer?

> @@ -2012,6 +2145,18 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>              start_extent);
>          break;
>  
> +    case XENMEM_foreigncopy:
> +        /*
> +         * Instead of using "start_extent" we update the structure back,
> +         * we update it back in anyway to tell caller were the copy
> +         * stopped.
> +         */
> +        if ( unlikely(start_extent) )
> +            return -EINVAL;

As before - please be precise with comments like this. We update it back also
when encoding a continuation. Perhaps instead "..., to indicate the point of
failure to the caller as well as to encode continuations without being
constrained by MEMOP_EXTENT_SHIFT".

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -740,7 +740,49 @@ struct xen_vnuma_topology_info {
>  typedef struct xen_vnuma_topology_info xen_vnuma_topology_info_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_vnuma_topology_info_t);
>  
> -/* Next available subop number is 29 */
> +/*
> + * Copy memory from/to a given domain.
> + * As this call requires target access and guest with target access won't be
> + * compat guests supported for compat guests this is not implemented.

As before - I question this. You simply can't know. (I'm also struggling with
wording / grammar.)

> + */
> +#define XENMEM_foreigncopy 29
> +struct xen_foreigncopy {
> +    /* IN - The domain whose memory is to be copied. */
> +    domid_t domid;
> +
> +    /* IN - Flags. */
> +#define XENMEM_foreigncopy_from 0
> +#define XENMEM_foreigncopy_to 1
> +#define XENMEM_foreigncopy_direction 1
> +    uint16_t flags;
> +
> +    /*
> +     * IN/OUT
> +     *
> +     * As an IN parameter number of frames of the domain to be copied.
> +     * On output on error updated number of frames left.
> +     */
> +    uint32_t nr_frames;
> +
> +    /*
> +     * IN/OUT
> +     *
> +     * Frames to be copied.
> +     * On output on error updated to point to first frame unhandled.

Is "on error" really correct / meaningful? The field can be updated at
any intermediate point, when a continuation is scheduled. Perhaps:

     * On output:
     *  - on error updated to point to first frame which couldn't be handled,
     *  - on success undefined.

Along these lines for nr_frames then as well (if needed at all, seeing
that it could as well be undefined in both cases, as the information is
redundant with the frame_list update).

> +     */
> +    XEN_GUEST_HANDLE(xen_pfn_t) frame_list;
> +
> +    /*
> +     * IN/OUT
> +     *
> +     * Userspace buffer to read/write from.

s/Userspace/Guest/ ?

Also still no mention of when / how this field is updated.

> +     */
> +    XEN_GUEST_HANDLE(uint8) buffer;
> +};

What was (again) left unaddressed is the question towards using GFNs on both
sides of the copy. This would eliminate the need for the flags field, taken
by a 2nd domid_t one then.

Jan

Re: [PATCH v6 12/16] xen: implement new foreign copy hypercall

[Jan Beulich]

Daniel,

On 19.06.2026 15:04, Frediano Ziglio wrote:
> @@ -1545,6 +1545,139 @@ static int acquire_resource(
>      return rc;
>  }
>  
> +/*
> + * The "noinline" qualifier avoids the compiler to create a large function
> + * consuming quite a lot of stack.
> + */
> +static int noinline mem_foreigncopy(
> +    XEN_GUEST_HANDLE_PARAM(xen_foreigncopy_t) arg)
> +{
> +    struct domain *d, *const currd = current->domain;
> +    xen_foreigncopy_t copy;
> +    int rc, direction;
> +
> +    if ( copy_from_guest(&copy, arg, 1) )
> +        return -EFAULT;
> +
> +    if ( copy.flags & ~XENMEM_foreigncopy_direction )
> +        return -EINVAL;
> +
> +    direction = copy.flags & XENMEM_foreigncopy_direction;
> +
> +    rc = rcu_lock_remote_domain_by_id(copy.domid, &d);
> +    if ( rc )
> +        return rc;
> +
> +    if ( copy.nr_frames == 0 )
> +    {
> +        rcu_unlock_domain(d);
> +        return 0;
> +    }
> +
> +    /*
> +     * Check we are allowed to map and access these foreign pages.
> +     */
> +    rc = xsm_map_gmfn_foreign(XSM_TARGET, currd, d);
> +    if ( rc )
> +        goto out;

can you please clarify whether such a re-use of an existing predicate is
acceptable?

Jan

Re: [PATCH v6 12/16] xen: implement new foreign copy hypercall

[Frediano Ziglio]

On Mon, 22 Jun 2026 at 11:34, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 19.06.2026 15:04, Frediano Ziglio wrote:
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -1545,6 +1545,139 @@ static int acquire_resource(
> >      return rc;
> >  }
> >
> > +/*
> > + * The "noinline" qualifier avoids the compiler to create a large function
> > + * consuming quite a lot of stack.
> > + */
> > +static int noinline mem_foreigncopy(
> > +    XEN_GUEST_HANDLE_PARAM(xen_foreigncopy_t) arg)
> > +{
> > +    struct domain *d, *const currd = current->domain;
> > +    xen_foreigncopy_t copy;
> > +    int rc, direction;
> > +
> > +    if ( copy_from_guest(&copy, arg, 1) )
> > +        return -EFAULT;
> > +
> > +    if ( copy.flags & ~XENMEM_foreigncopy_direction )
> > +        return -EINVAL;
> > +
> > +    direction = copy.flags & XENMEM_foreigncopy_direction;
> > +
> > +    rc = rcu_lock_remote_domain_by_id(copy.domid, &d);
>
> Iirc I did ask before why this isn't ..._by_any_id().
>

I probably was confused by the question about MMUEXT and the 2 domains.
There are different similar hypercalls (like the mentioned MMUEXT but
also hypercalls to map foreign domain memory) that have this check
(not the same domain). Any domain has, obviously, access to its own
memory, so it should not have to use hypercall to access its own
memory. If it does it looks like a mistake causing performance issues
or an attempt to circumvent security; in either case you would like to
avoid it.

> > +    if ( rc )
> > +        return rc;
> > +
> > +    if ( copy.nr_frames == 0 )
> > +    {
> > +        rcu_unlock_domain(d);
> > +        return 0;
> > +    }
>
> Any reason this cannot also be "goto out"? The more that now that you have
> moved this past the domid validity check, imo it should further move to ...
>

The only reason was style and to avoid a memory copy, but it's not a
hot case so I'll change to "goto out" (no strong about it).

> > +    /*
> > +     * Check we are allowed to map and access these foreign pages.
> > +     */
> > +    rc = xsm_map_gmfn_foreign(XSM_TARGET, currd, d);
> > +    if ( rc )
> > +        goto out;
>
> ... below here. Perhaps simply as
>
>     if ( rc || !copy.nr_frames )
>         goto out;
>

I think this would be confusing with the above "Check we are allowed
to map and access these foreign pages" comment.
Are you okay with just the change above to "goto out" ?
Also moving here would potentially change the result and do a useless check.

> > +    do {
> > +        /*
> > +         * Arbitrary size.  Not too much stack space, and a reasonable stride
> > +         * for continuation checks.
> > +         */
> > +        xen_pfn_t gfn_list[32];
> > +        unsigned int todo = MIN(ARRAY_SIZE(gfn_list), copy.nr_frames);
> > +
> > +        rc = -EFAULT;
> > +        if ( copy_from_guest(gfn_list, copy.frame_list, todo) )
> > +            goto out;
> > +
> > +        for ( unsigned int i = 0; i < todo; i++ )
> > +        {
> > +            struct page_info *foreign_page;
> > +            mfn_t foreign_mfn;
> > +            void *foreign;
> > +            p2m_type_t p2mt;
> > +            const unsigned long valid_mask =
> > +#ifdef CONFIG_X86
> > +                p2m_to_mask(p2m_ram_rw) | p2m_to_mask(p2m_ram_logdirty);
> > +#else
> > +                p2m_to_mask(p2m_ram_rw);
> > +#endif
>
> The set of permitted types didn't change, yet a justification for the resulting
> limitation also didn't appear.
>

Yes, that's missing, indeed.
Should the set of types be different for reading and writing? For
instance do not allow writing to read-only memory?
Given that it looks like different architectures have different
meanings and definitions for these constants, should it not be better
to define some new constants for this specific usage? For instance
P2M_READ_TYPES and P2M_WRITE_TYPES?

> > +            foreign_page = get_page_from_gfn(d, gfn_list[i], &p2mt, P2M_ALLOC);
> > +
> > +            if ( unlikely(!(p2m_to_mask(p2mt) & valid_mask)) && foreign_page )
> > +            {
> > +                put_page(foreign_page);
> > +                foreign_page = NULL;
> > +            }
> > +            if ( unlikely(!foreign_page) )
> > +            {
> > +                gdprintk(XENLOG_WARNING,
> > +                         "Error accessing foreign gfn %" PRI_gfn "\n",
> > +                         gfn_list[i]);
> > +                rc = -EINVAL;
> > +                copy.nr_frames -= i;
> > +                guest_handle_add_offset(copy.frame_list, i);
> > +                goto out;
> > +            }
> > +
> > +            foreign_mfn = page_to_mfn(foreign_page);
> > +
> > +            /* A page is dirtied when it's being copied to. */
> > +            if ( direction == XENMEM_foreigncopy_to )
> > +                paging_mark_dirty(d, foreign_mfn);
> > +
> > +            foreign = map_domain_page(foreign_mfn);
> > +            if ( direction == XENMEM_foreigncopy_from )
> > +                rc = copy_to_guest(copy.buffer, foreign, PAGE_SIZE);
> > +            else
> > +                rc = copy_from_guest(foreign, copy.buffer, PAGE_SIZE);
>
> You cannot validly write to the page without holding a PGT_writable ref.
> Else you might overwrite a page table or a descriptor table in a PV guest.
>

Given that this code was "inspired" by other hypercalls I'll also
check the other code.

> Once again - can you please make sure you have addressed earlier review
> comments, before sending a new version? I did point this out before.
>

Apparently not.

> > +            unmap_domain_page(foreign);
> > +            put_page(foreign_page);
> > +
> > +            if ( unlikely(rc) )
> > +            {
> > +                gdprintk(XENLOG_WARNING,
> > +                         "Error %d copying gfn %" PRI_gfn "\n",
> > +                         -rc, gfn_list[i]);
>
> Why "-rc"? (See other log messages including error codes.)
>

Because the errors are positive but for ABI we return them as negative.
But I suppose if for other messages we use the negated value this
should be just "rc".
I'll change.

> > +                copy.nr_frames -= i;
> > +                guest_handle_add_offset(copy.frame_list, i);
> > +                goto out;
> > +            }
> > +
> > +            guest_handle_add_offset(copy.buffer, PAGE_SIZE);
> > +        }
> > +
> > +        copy.nr_frames -= todo;
> > +        guest_handle_add_offset(copy.frame_list, todo);
>
> Don't you need to also update copy.buffer?
>

It's updated some lines above inside the loop.

> > @@ -2012,6 +2145,18 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >              start_extent);
> >          break;
> >
> > +    case XENMEM_foreigncopy:
> > +        /*
> > +         * Instead of using "start_extent" we update the structure back,
> > +         * we update it back in anyway to tell caller were the copy
> > +         * stopped.
> > +         */
> > +        if ( unlikely(start_extent) )
> > +            return -EINVAL;
>
> As before - please be precise with comments like this. We update it back also
> when encoding a continuation. Perhaps instead "..., to indicate the point of
> failure to the caller as well as to encode continuations without being
> constrained by MEMOP_EXTENT_SHIFT".
>

What about (trying to include your suggestion, to be fixed for line length):

        /*
         * Instead of using "start_extent" for the continuation, we
update the structure back,
         * we update the xen_foreigncopy structure back, so we are not
constrained
         * by MEMOP_EXTENT_SHIFT.
         * We copy it back also to tell the caller where the copy stopped.
         */

> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -740,7 +740,49 @@ struct xen_vnuma_topology_info {
> >  typedef struct xen_vnuma_topology_info xen_vnuma_topology_info_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_vnuma_topology_info_t);
> >
> > -/* Next available subop number is 29 */
> > +/*
> > + * Copy memory from/to a given domain.
> > + * As this call requires target access and guest with target access won't be
> > + * compat guests supported for compat guests this is not implemented.
>
> As before - I question this. You simply can't know. (I'm also struggling with
> wording / grammar.)
>

I was trying to code the compatibility layer. Is there a way to have
64 bit PFN even for compatibility guests instead of having to limit
and convert PFN numbers?

> > + */
> > +#define XENMEM_foreigncopy 29
> > +struct xen_foreigncopy {
> > +    /* IN - The domain whose memory is to be copied. */
> > +    domid_t domid;
> > +
> > +    /* IN - Flags. */
> > +#define XENMEM_foreigncopy_from 0
> > +#define XENMEM_foreigncopy_to 1
> > +#define XENMEM_foreigncopy_direction 1
> > +    uint16_t flags;
> > +
> > +    /*
> > +     * IN/OUT
> > +     *
> > +     * As an IN parameter number of frames of the domain to be copied.
> > +     * On output on error updated number of frames left.
> > +     */
> > +    uint32_t nr_frames;
> > +
> > +    /*
> > +     * IN/OUT
> > +     *
> > +     * Frames to be copied.
> > +     * On output on error updated to point to first frame unhandled.
>
> Is "on error" really correct / meaningful? The field can be updated at
> any intermediate point, when a continuation is scheduled. Perhaps:
>
>      * On output:
>      *  - on error updated to point to first frame which couldn't be handled,
>      *  - on success undefined.
>
> Along these lines for nr_frames then as well (if needed at all, seeing
> that it could as well be undefined in both cases, as the information is
> redundant with the frame_list update).
>
> > +     */
> > +    XEN_GUEST_HANDLE(xen_pfn_t) frame_list;
> > +
> > +    /*
> > +     * IN/OUT
> > +     *
> > +     * Userspace buffer to read/write from.
>
> s/Userspace/Guest/ ?
>
> Also still no mention of when / how this field is updated.
>

What about:

/*
 * Copy memory from/to a given domain.
 */
#define XENMEM_foreigncopy 29
struct xen_foreigncopy {
    /* IN - The domain whose memory is to be copied. */
    domid_t domid;

    /* IN - Flags. */
#define XENMEM_foreigncopy_from 0
#define XENMEM_foreigncopy_to 1
#define XENMEM_foreigncopy_direction 1
    uint16_t flags;

    /*
     * IN/OUT
     *
     * As an IN parameter number of frames of the domain to be copied.
     * On output updated number of frames left (0 if success).
     */
    uint32_t nr_frames;

    /*
     * IN/OUT
     *
     * Frames to be copied.
     * On output updated to point to the first frame unhandled.
     */
    XEN_GUEST_HANDLE(xen_pfn_t) frame_list;

    /*
     * IN/OUT
     *
     * Guest buffer to read/write from.
     * On output updated to point to the first frame unhandled.
     */
    XEN_GUEST_HANDLE(uint8) buffer;
};
typedef struct xen_foreigncopy xen_foreigncopy_t;
DEFINE_XEN_GUEST_HANDLE(xen_foreigncopy_t);

> > +     */
> > +    XEN_GUEST_HANDLE(uint8) buffer;
> > +};
>
> What was (again) left unaddressed is the question towards using GFNs on both
> sides of the copy. This would eliminate the need for the flags field, taken
> by a 2nd domid_t one then.
>

This was addressed in
https://lists.xenproject.org/archives/html/xen-devel/2026-06/msg00567.html
and in minor way by
https://lists.xenproject.org/archives/html/xen-devel/2026-06/msg00847.html.
It was considered but more complicated and worse from a performance perspective.

> Jan

Frediano

Re: [PATCH v6 12/16] xen: implement new foreign copy hypercall

[Jan Beulich]

On 23.06.2026 12:55, Frediano Ziglio wrote:
> On Mon, 22 Jun 2026 at 11:34, Jan Beulich <jbeulich@suse.com> wrote:
>> On 19.06.2026 15:04, Frediano Ziglio wrote:
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -1545,6 +1545,139 @@ static int acquire_resource(
>>>      return rc;
>>>  }
>>>
>>> +/*
>>> + * The "noinline" qualifier avoids the compiler to create a large function
>>> + * consuming quite a lot of stack.
>>> + */
>>> +static int noinline mem_foreigncopy(
>>> +    XEN_GUEST_HANDLE_PARAM(xen_foreigncopy_t) arg)
>>> +{
>>> +    struct domain *d, *const currd = current->domain;
>>> +    xen_foreigncopy_t copy;
>>> +    int rc, direction;
>>> +
>>> +    if ( copy_from_guest(&copy, arg, 1) )
>>> +        return -EFAULT;
>>> +
>>> +    if ( copy.flags & ~XENMEM_foreigncopy_direction )
>>> +        return -EINVAL;
>>> +
>>> +    direction = copy.flags & XENMEM_foreigncopy_direction;
>>> +
>>> +    rc = rcu_lock_remote_domain_by_id(copy.domid, &d);
>>
>> Iirc I did ask before why this isn't ..._by_any_id().
> 
> I probably was confused by the question about MMUEXT and the 2 domains.
> There are different similar hypercalls (like the mentioned MMUEXT but
> also hypercalls to map foreign domain memory) that have this check
> (not the same domain). Any domain has, obviously, access to its own
> memory, so it should not have to use hypercall to access its own
> memory. If it does it looks like a mistake causing performance issues
> or an attempt to circumvent security; in either case you would like to
> avoid it.

No. Self-grants are possible as well, for example, and for a good reason.
Allowing normally-remote operations on oneself helps with testing, for
example. It may also help avoid needing to special-case "self" in code
which needs to cover both cases.

>>> +    if ( rc )
>>> +        return rc;
>>> +
>>> +    if ( copy.nr_frames == 0 )
>>> +    {
>>> +        rcu_unlock_domain(d);
>>> +        return 0;
>>> +    }
>>
>> Any reason this cannot also be "goto out"? The more that now that you have
>> moved this past the domid validity check, imo it should further move to ...
> 
> The only reason was style and to avoid a memory copy, but it's not a
> hot case so I'll change to "goto out" (no strong about it).
> 
>>> +    /*
>>> +     * Check we are allowed to map and access these foreign pages.
>>> +     */
>>> +    rc = xsm_map_gmfn_foreign(XSM_TARGET, currd, d);
>>> +    if ( rc )
>>> +        goto out;
>>
>> ... below here. Perhaps simply as
>>
>>     if ( rc || !copy.nr_frames )
>>         goto out;
>>
> 
> I think this would be confusing with the above "Check we are allowed
> to map and access these foreign pages" comment.
> Are you okay with just the change above to "goto out" ?

I do want the order adjusted as indicated. I won't insist on (but I would
prefer) folding both if()-s.

> Also moving here would potentially change the result and do a useless check.

Affecting the result is the goal of the re-ordering.

>>> +    do {
>>> +        /*
>>> +         * Arbitrary size.  Not too much stack space, and a reasonable stride
>>> +         * for continuation checks.
>>> +         */
>>> +        xen_pfn_t gfn_list[32];
>>> +        unsigned int todo = MIN(ARRAY_SIZE(gfn_list), copy.nr_frames);
>>> +
>>> +        rc = -EFAULT;
>>> +        if ( copy_from_guest(gfn_list, copy.frame_list, todo) )
>>> +            goto out;
>>> +
>>> +        for ( unsigned int i = 0; i < todo; i++ )
>>> +        {
>>> +            struct page_info *foreign_page;
>>> +            mfn_t foreign_mfn;
>>> +            void *foreign;
>>> +            p2m_type_t p2mt;
>>> +            const unsigned long valid_mask =
>>> +#ifdef CONFIG_X86
>>> +                p2m_to_mask(p2m_ram_rw) | p2m_to_mask(p2m_ram_logdirty);
>>> +#else
>>> +                p2m_to_mask(p2m_ram_rw);
>>> +#endif
>>
>> The set of permitted types didn't change, yet a justification for the resulting
>> limitation also didn't appear.
>>
> 
> Yes, that's missing, indeed.
> Should the set of types be different for reading and writing? For
> instance do not allow writing to read-only memory?

Of course.

> Given that it looks like different architectures have different
> meanings and definitions for these constants, should it not be better
> to define some new constants for this specific usage? For instance
> P2M_READ_TYPES and P2M_WRITE_TYPES?

Perhaps, yes. The suggested names look overly generic to me, though.

>>> +            foreign_page = get_page_from_gfn(d, gfn_list[i], &p2mt, P2M_ALLOC);
>>> +
>>> +            if ( unlikely(!(p2m_to_mask(p2mt) & valid_mask)) && foreign_page )
>>> +            {
>>> +                put_page(foreign_page);
>>> +                foreign_page = NULL;
>>> +            }
>>> +            if ( unlikely(!foreign_page) )
>>> +            {
>>> +                gdprintk(XENLOG_WARNING,
>>> +                         "Error accessing foreign gfn %" PRI_gfn "\n",
>>> +                         gfn_list[i]);
>>> +                rc = -EINVAL;
>>> +                copy.nr_frames -= i;
>>> +                guest_handle_add_offset(copy.frame_list, i);
>>> +                goto out;
>>> +            }
>>> +
>>> +            foreign_mfn = page_to_mfn(foreign_page);
>>> +
>>> +            /* A page is dirtied when it's being copied to. */
>>> +            if ( direction == XENMEM_foreigncopy_to )
>>> +                paging_mark_dirty(d, foreign_mfn);
>>> +
>>> +            foreign = map_domain_page(foreign_mfn);
>>> +            if ( direction == XENMEM_foreigncopy_from )
>>> +                rc = copy_to_guest(copy.buffer, foreign, PAGE_SIZE);
>>> +            else
>>> +                rc = copy_from_guest(foreign, copy.buffer, PAGE_SIZE);
>>
>> You cannot validly write to the page without holding a PGT_writable ref.
>> Else you might overwrite a page table or a descriptor table in a PV guest.
>>
> 
> Given that this code was "inspired" by other hypercalls I'll also
> check the other code.
> 
>> Once again - can you please make sure you have addressed earlier review
>> comments, before sending a new version? I did point this out before.
> 
> Apparently not.

https://lists.xen.org/archives/html/xen-devel/2026-06/msg00850.html

>>> +                copy.nr_frames -= i;
>>> +                guest_handle_add_offset(copy.frame_list, i);
>>> +                goto out;
>>> +            }
>>> +
>>> +            guest_handle_add_offset(copy.buffer, PAGE_SIZE);
>>> +        }
>>> +
>>> +        copy.nr_frames -= todo;
>>> +        guest_handle_add_offset(copy.frame_list, todo);
>>
>> Don't you need to also update copy.buffer?
> 
> It's updated some lines above inside the loop.

Oh, sorry. Yet then - not doing all updates together is, as you can see,
potentially confusing.

>>> @@ -2012,6 +2145,18 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>              start_extent);
>>>          break;
>>>
>>> +    case XENMEM_foreigncopy:
>>> +        /*
>>> +         * Instead of using "start_extent" we update the structure back,
>>> +         * we update it back in anyway to tell caller were the copy
>>> +         * stopped.
>>> +         */
>>> +        if ( unlikely(start_extent) )
>>> +            return -EINVAL;
>>
>> As before - please be precise with comments like this. We update it back also
>> when encoding a continuation. Perhaps instead "..., to indicate the point of
>> failure to the caller as well as to encode continuations without being
>> constrained by MEMOP_EXTENT_SHIFT".
>>
> 
> What about (trying to include your suggestion, to be fixed for line length):
> 
>         /*
>          * Instead of using "start_extent" for the continuation, we
> update the structure back,
>          * we update the xen_foreigncopy structure back, so we are not
> constrained
>          * by MEMOP_EXTENT_SHIFT.
>          * We copy it back also to tell the caller where the copy stopped.
>          */

One of the things I take issue with (because it's hard to read that way,
at least for me) is the repeated use of "update ... back", effectively
saying the same things twice. The last sentence also wants disambiguating
towards the "stopped" possibly being a non-error situation as well.

>>> --- a/xen/include/public/memory.h
>>> +++ b/xen/include/public/memory.h
>>> @@ -740,7 +740,49 @@ struct xen_vnuma_topology_info {
>>>  typedef struct xen_vnuma_topology_info xen_vnuma_topology_info_t;
>>>  DEFINE_XEN_GUEST_HANDLE(xen_vnuma_topology_info_t);
>>>
>>> -/* Next available subop number is 29 */
>>> +/*
>>> + * Copy memory from/to a given domain.
>>> + * As this call requires target access and guest with target access won't be
>>> + * compat guests supported for compat guests this is not implemented.
>>
>> As before - I question this. You simply can't know. (I'm also struggling with
>> wording / grammar.)
> 
> I was trying to code the compatibility layer. Is there a way to have
> 64 bit PFN even for compatibility guests instead of having to limit
> and convert PFN numbers?

compat_pfn_t is a typedef of unsigned int (since a 32-bit guest seeing
"typedef unsigned long xen_pfn_t;" results in xen_pfn_t being a 32-bit
quantity for it), so 32-bit guests can only supply 32-bit frame numbers.
There's also no value in trying to be clever and using uint64_t instead
for the frame_list handle, as 32-bit guests won't ever own pages with
MFNs wider than 32 bits.

>>> + */
>>> +#define XENMEM_foreigncopy 29
>>> +struct xen_foreigncopy {
>>> +    /* IN - The domain whose memory is to be copied. */
>>> +    domid_t domid;
>>> +
>>> +    /* IN - Flags. */
>>> +#define XENMEM_foreigncopy_from 0
>>> +#define XENMEM_foreigncopy_to 1
>>> +#define XENMEM_foreigncopy_direction 1
>>> +    uint16_t flags;
>>> +
>>> +    /*
>>> +     * IN/OUT
>>> +     *
>>> +     * As an IN parameter number of frames of the domain to be copied.
>>> +     * On output on error updated number of frames left.
>>> +     */
>>> +    uint32_t nr_frames;
>>> +
>>> +    /*
>>> +     * IN/OUT
>>> +     *
>>> +     * Frames to be copied.
>>> +     * On output on error updated to point to first frame unhandled.
>>
>> Is "on error" really correct / meaningful? The field can be updated at
>> any intermediate point, when a continuation is scheduled. Perhaps:
>>
>>      * On output:
>>      *  - on error updated to point to first frame which couldn't be handled,
>>      *  - on success undefined.
>>
>> Along these lines for nr_frames then as well (if needed at all, seeing
>> that it could as well be undefined in both cases, as the information is
>> redundant with the frame_list update).
>>
>>> +     */
>>> +    XEN_GUEST_HANDLE(xen_pfn_t) frame_list;
>>> +
>>> +    /*
>>> +     * IN/OUT
>>> +     *
>>> +     * Userspace buffer to read/write from.
>>
>> s/Userspace/Guest/ ?
>>
>> Also still no mention of when / how this field is updated.
>>
> 
> What about:
> 
> /*
>  * Copy memory from/to a given domain.
>  */
> #define XENMEM_foreigncopy 29
> struct xen_foreigncopy {
>     /* IN - The domain whose memory is to be copied. */
>     domid_t domid;
> 
>     /* IN - Flags. */
> #define XENMEM_foreigncopy_from 0
> #define XENMEM_foreigncopy_to 1
> #define XENMEM_foreigncopy_direction 1
>     uint16_t flags;
> 
>     /*
>      * IN/OUT
>      *
>      * As an IN parameter number of frames of the domain to be copied.
>      * On output updated number of frames left (0 if success).
>      */
>     uint32_t nr_frames;
> 
>     /*
>      * IN/OUT
>      *
>      * Frames to be copied.
>      * On output updated to point to the first frame unhandled.

There may be no such frame, so at the very least add "..., if any"?

>      */
>     XEN_GUEST_HANDLE(xen_pfn_t) frame_list;
> 
>     /*
>      * IN/OUT
>      *
>      * Guest buffer to read/write from.
>      * On output updated to point to the first frame unhandled.

There's no frame here, as long as you don't switch to using two frame
lists (for source and destination).

>      */
>     XEN_GUEST_HANDLE(uint8) buffer;
> };
> typedef struct xen_foreigncopy xen_foreigncopy_t;
> DEFINE_XEN_GUEST_HANDLE(xen_foreigncopy_t);
> 
>>> +     */
>>> +    XEN_GUEST_HANDLE(uint8) buffer;
>>> +};
>>
>> What was (again) left unaddressed is the question towards using GFNs on both
>> sides of the copy. This would eliminate the need for the flags field, taken
>> by a 2nd domid_t one then.
>>
> 
> This was addressed in
> https://lists.xenproject.org/archives/html/xen-devel/2026-06/msg00567.html

Well, yes, but not in a satisfactory way. Back channels tell me that you
actually got the same feedback already on internal review. Which makes it
all the more puzzling that you insist on doing it differently. Multiple
maintainers asking for the same thing may be an indication of something.

> and in minor way by
> https://lists.xenproject.org/archives/html/xen-devel/2026-06/msg00847.html.
> It was considered but more complicated and worse from a performance perspective.

Okay, performance-wise worse would of course be relevant. But that would
need supporting by numbers (for both PV and PVH Dom0, as the latter
incurs extra overhead for virtual-address-based hypercall buffer operands).

Jan

Re: [PATCH v6 12/16] xen: implement new foreign copy hypercall

[Daniel P. Smith]

On 6/19/26 9:04 AM, Frediano Ziglio wrote:
> Add a sub hypercall to __HYPERVISOR_memory_op to allow to read/write
> memory from/to a foreign domain.
> 
> Extending MMUEXT_COPY_PAGE seems better on first sight but considering
> that MMUEXT is meant for PV only and trying to change that sub-op this
> solution is better.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
> --
> Changes since v4:
> - Fix typo in comment.
> 
> Changes since v5:
> - update xen_foreigncopy structure comments;
> - move check for no frames after checking the domain;
> - use mnemonic instead of 1U;
> - fix page type checks;
> - do not overwrite error copying back structure;
> - latch MFN value;
> - improved commit message.
> ---
>   xen/common/memory.c         | 145 ++++++++++++++++++++++++++++++++++++
>   xen/include/public/memory.h |  44 ++++++++++-
>   2 files changed, 188 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 3672bda025..98726766bf 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1545,6 +1545,139 @@ static int acquire_resource(
>       return rc;
>   }
>   
> +/*
> + * The "noinline" qualifier avoids the compiler to create a large function
> + * consuming quite a lot of stack.
> + */
> +static int noinline mem_foreigncopy(
> +    XEN_GUEST_HANDLE_PARAM(xen_foreigncopy_t) arg)
> +{
> +    struct domain *d, *const currd = current->domain;
> +    xen_foreigncopy_t copy;
> +    int rc, direction;
> +
> +    if ( copy_from_guest(&copy, arg, 1) )
> +        return -EFAULT;
> +
> +    if ( copy.flags & ~XENMEM_foreigncopy_direction )
> +        return -EINVAL;
> +
> +    direction = copy.flags & XENMEM_foreigncopy_direction;
> +
> +    rc = rcu_lock_remote_domain_by_id(copy.domid, &d);
> +    if ( rc )
> +        return rc;
> +
> +    if ( copy.nr_frames == 0 )
> +    {
> +        rcu_unlock_domain(d);
> +        return 0;
> +    }
> +
> +    /*
> +     * Check we are allowed to map and access these foreign pages.
> +     */
> +    rc = xsm_map_gmfn_foreign(XSM_TARGET, currd, d);

While on the result is the same, this is a different action. I didn't 
immediately answer because I am split on what the new hook should be. In 
particular if it should be only one like map_gmfn, if it should instead 
also take a direction (in/out), of should we have two hooks (xxx_read, 
xxx_write). Myself, I am leaning toward the last option but I am open to 
hearing other's opinions. If you need any assistance with writing the 
hooks, feel free to reach out.

v/r,
dps

Re: [PATCH v6 12/16] xen: implement new foreign copy hypercall

[Frediano Ziglio]

On Tue, 23 Jun 2026 at 14:21, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 23.06.2026 12:55, Frediano Ziglio wrote:
> > On Mon, 22 Jun 2026 at 11:34, Jan Beulich <jbeulich@suse.com> wrote:
> >> On 19.06.2026 15:04, Frediano Ziglio wrote:
> >>> --- a/xen/common/memory.c
> >>> +++ b/xen/common/memory.c
> >>> @@ -1545,6 +1545,139 @@ static int acquire_resource(
> >>>      return rc;
> >>>  }
> >>>
> >>> +/*
> >>> + * The "noinline" qualifier avoids the compiler to create a large function
> >>> + * consuming quite a lot of stack.
> >>> + */
> >>> +static int noinline mem_foreigncopy(
> >>> +    XEN_GUEST_HANDLE_PARAM(xen_foreigncopy_t) arg)
> >>> +{
> >>> +    struct domain *d, *const currd = current->domain;
> >>> +    xen_foreigncopy_t copy;
> >>> +    int rc, direction;
> >>> +
> >>> +    if ( copy_from_guest(&copy, arg, 1) )
> >>> +        return -EFAULT;
> >>> +
> >>> +    if ( copy.flags & ~XENMEM_foreigncopy_direction )
> >>> +        return -EINVAL;
> >>> +
> >>> +    direction = copy.flags & XENMEM_foreigncopy_direction;
> >>> +
> >>> +    rc = rcu_lock_remote_domain_by_id(copy.domid, &d);
> >>
> >> Iirc I did ask before why this isn't ..._by_any_id().
> >
> > I probably was confused by the question about MMUEXT and the 2 domains.
> > There are different similar hypercalls (like the mentioned MMUEXT but
> > also hypercalls to map foreign domain memory) that have this check
> > (not the same domain). Any domain has, obviously, access to its own
> > memory, so it should not have to use hypercall to access its own
> > memory. If it does it looks like a mistake causing performance issues
> > or an attempt to circumvent security; in either case you would like to
> > avoid it.
>
> No. Self-grants are possible as well, for example, and for a good reason.
> Allowing normally-remote operations on oneself helps with testing, for
> example. It may also help avoid needing to special-case "self" in code
> which needs to cover both cases.
>

But this is not a grant, it's a copy.

> >>> +    if ( rc )
> >>> +        return rc;
> >>> +
> >>> +    if ( copy.nr_frames == 0 )
> >>> +    {
> >>> +        rcu_unlock_domain(d);
> >>> +        return 0;
> >>> +    }
> >>
> >> Any reason this cannot also be "goto out"? The more that now that you have
> >> moved this past the domid validity check, imo it should further move to ...
> >
> > The only reason was style and to avoid a memory copy, but it's not a
> > hot case so I'll change to "goto out" (no strong about it).
> >
> >>> +    /*
> >>> +     * Check we are allowed to map and access these foreign pages.
> >>> +     */
> >>> +    rc = xsm_map_gmfn_foreign(XSM_TARGET, currd, d);
> >>> +    if ( rc )
> >>> +        goto out;
> >>
> >> ... below here. Perhaps simply as
> >>
> >>     if ( rc || !copy.nr_frames )
> >>         goto out;
> >>
> >
> > I think this would be confusing with the above "Check we are allowed
> > to map and access these foreign pages" comment.
> > Are you okay with just the change above to "goto out" ?
>
> I do want the order adjusted as indicated. I won't insist on (but I would
> prefer) folding both if()-s.
>

What about

    /*
     * Check we are allowed to map and access these foreign pages.
     */
    rc = xsm_map_gmfn_foreign(XSM_TARGET, currd, d);
    if ( rc )
        goto out;

    while ( copy.nr_frames )
    {
        /*
         * Arbitrary size.  Not too much stack space, and a reasonable stride
         * for continuation checks.
         */


> > Also moving here would potentially change the result and do a useless check.
>
> Affecting the result is the goal of the re-ordering.
>

Changed.

> >>> +    do {
> >>> +        /*
> >>> +         * Arbitrary size.  Not too much stack space, and a reasonable stride
> >>> +         * for continuation checks.
> >>> +         */
> >>> +        xen_pfn_t gfn_list[32];
> >>> +        unsigned int todo = MIN(ARRAY_SIZE(gfn_list), copy.nr_frames);
> >>> +
> >>> +        rc = -EFAULT;
> >>> +        if ( copy_from_guest(gfn_list, copy.frame_list, todo) )
> >>> +            goto out;
> >>> +
> >>> +        for ( unsigned int i = 0; i < todo; i++ )
> >>> +        {
> >>> +            struct page_info *foreign_page;
> >>> +            mfn_t foreign_mfn;
> >>> +            void *foreign;
> >>> +            p2m_type_t p2mt;
> >>> +            const unsigned long valid_mask =
> >>> +#ifdef CONFIG_X86
> >>> +                p2m_to_mask(p2m_ram_rw) | p2m_to_mask(p2m_ram_logdirty);
> >>> +#else
> >>> +                p2m_to_mask(p2m_ram_rw);
> >>> +#endif
> >>
> >> The set of permitted types didn't change, yet a justification for the resulting
> >> limitation also didn't appear.
> >>
> >
> > Yes, that's missing, indeed.
> > Should the set of types be different for reading and writing? For
> > instance do not allow writing to read-only memory?
>
> Of course.
>
> > Given that it looks like different architectures have different
> > meanings and definitions for these constants, should it not be better
> > to define some new constants for this specific usage? For instance
> > P2M_READ_TYPES and P2M_WRITE_TYPES?
>
> Perhaps, yes. The suggested names look overly generic to me, though.
>

I suppose P2M_READABLE_TYPES and P2M_WRITABLE_TYPES are more correct
but still too generic.
P2M_EXPORTABLE_TYPES and P2M_IMPORTABLE_TYPES ?

> >>> +            foreign_page = get_page_from_gfn(d, gfn_list[i], &p2mt, P2M_ALLOC);
> >>> +
> >>> +            if ( unlikely(!(p2m_to_mask(p2mt) & valid_mask)) && foreign_page )
> >>> +            {
> >>> +                put_page(foreign_page);
> >>> +                foreign_page = NULL;
> >>> +            }
> >>> +            if ( unlikely(!foreign_page) )
> >>> +            {
> >>> +                gdprintk(XENLOG_WARNING,
> >>> +                         "Error accessing foreign gfn %" PRI_gfn "\n",
> >>> +                         gfn_list[i]);
> >>> +                rc = -EINVAL;
> >>> +                copy.nr_frames -= i;
> >>> +                guest_handle_add_offset(copy.frame_list, i);
> >>> +                goto out;
> >>> +            }
> >>> +
> >>> +            foreign_mfn = page_to_mfn(foreign_page);
> >>> +
> >>> +            /* A page is dirtied when it's being copied to. */
> >>> +            if ( direction == XENMEM_foreigncopy_to )
> >>> +                paging_mark_dirty(d, foreign_mfn);
> >>> +
> >>> +            foreign = map_domain_page(foreign_mfn);
> >>> +            if ( direction == XENMEM_foreigncopy_from )
> >>> +                rc = copy_to_guest(copy.buffer, foreign, PAGE_SIZE);
> >>> +            else
> >>> +                rc = copy_from_guest(foreign, copy.buffer, PAGE_SIZE);
> >>
> >> You cannot validly write to the page without holding a PGT_writable ref.
> >> Else you might overwrite a page table or a descriptor table in a PV guest.
> >>
> >
> > Given that this code was "inspired" by other hypercalls I'll also
> > check the other code.
> >
> >> Once again - can you please make sure you have addressed earlier review
> >> comments, before sending a new version? I did point this out before.
> >
> > Apparently not.
>
> https://lists.xen.org/archives/html/xen-devel/2026-06/msg00850.html
>

The "apparently not" is the reply to "can you please make sure you
have addressed earlier review comments, before sending a new version".

> >>> +                copy.nr_frames -= i;
> >>> +                guest_handle_add_offset(copy.frame_list, i);
> >>> +                goto out;
> >>> +            }
> >>> +
> >>> +            guest_handle_add_offset(copy.buffer, PAGE_SIZE);
> >>> +        }
> >>> +
> >>> +        copy.nr_frames -= todo;
> >>> +        guest_handle_add_offset(copy.frame_list, todo);
> >>
> >> Don't you need to also update copy.buffer?
> >
> > It's updated some lines above inside the loop.
>
> Oh, sorry. Yet then - not doing all updates together is, as you can see,
> potentially confusing.
>
> >>> @@ -2012,6 +2145,18 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>              start_extent);
> >>>          break;
> >>>
> >>> +    case XENMEM_foreigncopy:
> >>> +        /*
> >>> +         * Instead of using "start_extent" we update the structure back,
> >>> +         * we update it back in anyway to tell caller were the copy
> >>> +         * stopped.
> >>> +         */
> >>> +        if ( unlikely(start_extent) )
> >>> +            return -EINVAL;
> >>
> >> As before - please be precise with comments like this. We update it back also
> >> when encoding a continuation. Perhaps instead "..., to indicate the point of
> >> failure to the caller as well as to encode continuations without being
> >> constrained by MEMOP_EXTENT_SHIFT".
> >>
> >
> > What about (trying to include your suggestion, to be fixed for line length):
> >
> >         /*
> >          * Instead of using "start_extent" for the continuation, we
> > update the structure back,
> >          * we update the xen_foreigncopy structure back, so we are not
> > constrained
> >          * by MEMOP_EXTENT_SHIFT.
> >          * We copy it back also to tell the caller where the copy stopped.
> >          */
>
> One of the things I take issue with (because it's hard to read that way,
> at least for me) is the repeated use of "update ... back", effectively
> saying the same things twice. The last sentence also wants disambiguating
> towards the "stopped" possibly being a non-error situation as well.
>

Changed to

        /*
         * Instead of using "start_extent" for the continuation, we update
         * the xen_foreigncopy structure back, so we are not constrained by
         * MEMOP_EXTENT_SHIFT.
         * We copy it back also to tell the caller where the copy stopped
         * (either for error or because all frames were copied).
         */

> >>> --- a/xen/include/public/memory.h
> >>> +++ b/xen/include/public/memory.h
> >>> @@ -740,7 +740,49 @@ struct xen_vnuma_topology_info {
> >>>  typedef struct xen_vnuma_topology_info xen_vnuma_topology_info_t;
> >>>  DEFINE_XEN_GUEST_HANDLE(xen_vnuma_topology_info_t);
> >>>
> >>> -/* Next available subop number is 29 */
> >>> +/*
> >>> + * Copy memory from/to a given domain.
> >>> + * As this call requires target access and guest with target access won't be
> >>> + * compat guests supported for compat guests this is not implemented.
> >>
> >> As before - I question this. You simply can't know. (I'm also struggling with
> >> wording / grammar.)
> >
> > I was trying to code the compatibility layer. Is there a way to have
> > 64 bit PFN even for compatibility guests instead of having to limit
> > and convert PFN numbers?
>
> compat_pfn_t is a typedef of unsigned int (since a 32-bit guest seeing
> "typedef unsigned long xen_pfn_t;" results in xen_pfn_t being a 32-bit
> quantity for it), so 32-bit guests can only supply 32-bit frame numbers.
> There's also no value in trying to be clever and using uint64_t instead
> for the frame_list handle, as 32-bit guests won't ever own pages with
> MFNs wider than 32 bits.
>

They don't need to own such pages. But probably they would also need
other hypercalls to support larger frame numbers. To be honest you
just point out a reason to not support this for compatible guests.

> >>> + */
> >>> +#define XENMEM_foreigncopy 29
> >>> +struct xen_foreigncopy {
> >>> +    /* IN - The domain whose memory is to be copied. */
> >>> +    domid_t domid;
> >>> +
> >>> +    /* IN - Flags. */
> >>> +#define XENMEM_foreigncopy_from 0
> >>> +#define XENMEM_foreigncopy_to 1
> >>> +#define XENMEM_foreigncopy_direction 1
> >>> +    uint16_t flags;
> >>> +
> >>> +    /*
> >>> +     * IN/OUT
> >>> +     *
> >>> +     * As an IN parameter number of frames of the domain to be copied.
> >>> +     * On output on error updated number of frames left.
> >>> +     */
> >>> +    uint32_t nr_frames;
> >>> +
> >>> +    /*
> >>> +     * IN/OUT
> >>> +     *
> >>> +     * Frames to be copied.
> >>> +     * On output on error updated to point to first frame unhandled.
> >>
> >> Is "on error" really correct / meaningful? The field can be updated at
> >> any intermediate point, when a continuation is scheduled. Perhaps:
> >>
> >>      * On output:
> >>      *  - on error updated to point to first frame which couldn't be handled,
> >>      *  - on success undefined.
> >>
> >> Along these lines for nr_frames then as well (if needed at all, seeing
> >> that it could as well be undefined in both cases, as the information is
> >> redundant with the frame_list update).
> >>
> >>> +     */
> >>> +    XEN_GUEST_HANDLE(xen_pfn_t) frame_list;
> >>> +
> >>> +    /*
> >>> +     * IN/OUT
> >>> +     *
> >>> +     * Userspace buffer to read/write from.
> >>
> >> s/Userspace/Guest/ ?
> >>
> >> Also still no mention of when / how this field is updated.
> >>
> >
> > What about:
> >
> > /*
> >  * Copy memory from/to a given domain.
> >  */
> > #define XENMEM_foreigncopy 29
> > struct xen_foreigncopy {
> >     /* IN - The domain whose memory is to be copied. */
> >     domid_t domid;
> >
> >     /* IN - Flags. */
> > #define XENMEM_foreigncopy_from 0
> > #define XENMEM_foreigncopy_to 1
> > #define XENMEM_foreigncopy_direction 1
> >     uint16_t flags;
> >
> >     /*
> >      * IN/OUT
> >      *
> >      * As an IN parameter number of frames of the domain to be copied.
> >      * On output updated number of frames left (0 if success).
> >      */
> >     uint32_t nr_frames;
> >
> >     /*
> >      * IN/OUT
> >      *
> >      * Frames to be copied.
> >      * On output updated to point to the first frame unhandled.
>
> There may be no such frame, so at the very least add "..., if any"?
>

Added.

> >      */
> >     XEN_GUEST_HANDLE(xen_pfn_t) frame_list;
> >
> >     /*
> >      * IN/OUT
> >      *
> >      * Guest buffer to read/write from.
> >      * On output updated to point to the first frame unhandled.
>
> There's no frame here, as long as you don't switch to using two frame
> lists (for source and destination).
>

Changed to

    /*
     * IN/OUT
     *
     * Guest buffer to read/write from.
     * On output updated to point to the first page pointer unhandled.
     */

> >      */
> >     XEN_GUEST_HANDLE(uint8) buffer;
> > };
> > typedef struct xen_foreigncopy xen_foreigncopy_t;
> > DEFINE_XEN_GUEST_HANDLE(xen_foreigncopy_t);
> >
> >>> +     */
> >>> +    XEN_GUEST_HANDLE(uint8) buffer;
> >>> +};
> >>
> >> What was (again) left unaddressed is the question towards using GFNs on both
> >> sides of the copy. This would eliminate the need for the flags field, taken
> >> by a 2nd domid_t one then.
> >>
> >
> > This was addressed in
> > https://lists.xenproject.org/archives/html/xen-devel/2026-06/msg00567.html
>
> Well, yes, but not in a satisfactory way. Back channels tell me that you
> actually got the same feedback already on internal review. Which makes it
> all the more puzzling that you insist on doing it differently. Multiple
> maintainers asking for the same thing may be an indication of something.
>

Not needing to have backchannel feedback, I already wrote that a
similar approach was tried and made the code more complicated.
Both maintainers didn't comment on my replies so I assume they were
fine with it.
And you are failing to provide positive feedback.
I asked (that one internally) for examples of guest buffers provided
as frame numbers but I got no answer (or better the answer was more
"currently there are not").
Also note that the location of xen_foreigncopy_t structure is also
provided using a guest pointer.
I remember there were some discussions about ABI changes (2/3 years
ago) to address this and other issues but I cannot see much progress.
That's why I say this is out of scope.

> > and in minor way by
> > https://lists.xenproject.org/archives/html/xen-devel/2026-06/msg00847.html.
> > It was considered but more complicated and worse from a performance perspective.
>
> Okay, performance-wise worse would of course be relevant. But that would
> need supporting by numbers (for both PV and PVH Dom0, as the latter
> incurs extra overhead for virtual-address-based hypercall buffer operands).
>

I'm more concerned about the PV case than PVH to be honest.

I was having an idea about solving the pointer/frames issue but, as I
said, it's out of scope here.

> Jan

Frediano

Re: [PATCH v6 12/16] xen: implement new foreign copy hypercall

[Jan Beulich]

On 23.06.2026 23:18, Frediano Ziglio wrote:
> On Tue, 23 Jun 2026 at 14:21, Jan Beulich <jbeulich@suse.com> wrote:
>> On 23.06.2026 12:55, Frediano Ziglio wrote:
>>> On Mon, 22 Jun 2026 at 11:34, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 19.06.2026 15:04, Frediano Ziglio wrote:
>>>>> --- a/xen/common/memory.c
>>>>> +++ b/xen/common/memory.c
>>>>> @@ -1545,6 +1545,139 @@ static int acquire_resource(
>>>>>      return rc;
>>>>>  }
>>>>>
>>>>> +/*
>>>>> + * The "noinline" qualifier avoids the compiler to create a large function
>>>>> + * consuming quite a lot of stack.
>>>>> + */
>>>>> +static int noinline mem_foreigncopy(
>>>>> +    XEN_GUEST_HANDLE_PARAM(xen_foreigncopy_t) arg)
>>>>> +{
>>>>> +    struct domain *d, *const currd = current->domain;
>>>>> +    xen_foreigncopy_t copy;
>>>>> +    int rc, direction;
>>>>> +
>>>>> +    if ( copy_from_guest(&copy, arg, 1) )
>>>>> +        return -EFAULT;
>>>>> +
>>>>> +    if ( copy.flags & ~XENMEM_foreigncopy_direction )
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    direction = copy.flags & XENMEM_foreigncopy_direction;
>>>>> +
>>>>> +    rc = rcu_lock_remote_domain_by_id(copy.domid, &d);
>>>>
>>>> Iirc I did ask before why this isn't ..._by_any_id().
>>>
>>> I probably was confused by the question about MMUEXT and the 2 domains.
>>> There are different similar hypercalls (like the mentioned MMUEXT but
>>> also hypercalls to map foreign domain memory) that have this check
>>> (not the same domain). Any domain has, obviously, access to its own
>>> memory, so it should not have to use hypercall to access its own
>>> memory. If it does it looks like a mistake causing performance issues
>>> or an attempt to circumvent security; in either case you would like to
>>> avoid it.
>>
>> No. Self-grants are possible as well, for example, and for a good reason.
>> Allowing normally-remote operations on oneself helps with testing, for
>> example. It may also help avoid needing to special-case "self" in code
>> which needs to cover both cases.
> 
> But this is not a grant, it's a copy.

Sure, but the underlying principle is what matters. Plus you don't prevent
self-copy by using ..._by_id(), you only preclude the use of DOMID_SELF.

>>>>> +    if ( rc )
>>>>> +        return rc;
>>>>> +
>>>>> +    if ( copy.nr_frames == 0 )
>>>>> +    {
>>>>> +        rcu_unlock_domain(d);
>>>>> +        return 0;
>>>>> +    }
>>>>
>>>> Any reason this cannot also be "goto out"? The more that now that you have
>>>> moved this past the domid validity check, imo it should further move to ...
>>>
>>> The only reason was style and to avoid a memory copy, but it's not a
>>> hot case so I'll change to "goto out" (no strong about it).
>>>
>>>>> +    /*
>>>>> +     * Check we are allowed to map and access these foreign pages.
>>>>> +     */
>>>>> +    rc = xsm_map_gmfn_foreign(XSM_TARGET, currd, d);
>>>>> +    if ( rc )
>>>>> +        goto out;
>>>>
>>>> ... below here. Perhaps simply as
>>>>
>>>>     if ( rc || !copy.nr_frames )
>>>>         goto out;
>>>>
>>>
>>> I think this would be confusing with the above "Check we are allowed
>>> to map and access these foreign pages" comment.
>>> Are you okay with just the change above to "goto out" ?
>>
>> I do want the order adjusted as indicated. I won't insist on (but I would
>> prefer) folding both if()-s.
>>
> 
> What about
> 
>     /*
>      * Check we are allowed to map and access these foreign pages.
>      */
>     rc = xsm_map_gmfn_foreign(XSM_TARGET, currd, d);
>     if ( rc )
>         goto out;
> 
>     while ( copy.nr_frames )
>     {
>         /*
>          * Arbitrary size.  Not too much stack space, and a reasonable stride
>          * for continuation checks.
>          */

That's fine.

>>>>> +    do {
>>>>> +        /*
>>>>> +         * Arbitrary size.  Not too much stack space, and a reasonable stride
>>>>> +         * for continuation checks.
>>>>> +         */
>>>>> +        xen_pfn_t gfn_list[32];
>>>>> +        unsigned int todo = MIN(ARRAY_SIZE(gfn_list), copy.nr_frames);
>>>>> +
>>>>> +        rc = -EFAULT;
>>>>> +        if ( copy_from_guest(gfn_list, copy.frame_list, todo) )
>>>>> +            goto out;
>>>>> +
>>>>> +        for ( unsigned int i = 0; i < todo; i++ )
>>>>> +        {
>>>>> +            struct page_info *foreign_page;
>>>>> +            mfn_t foreign_mfn;
>>>>> +            void *foreign;
>>>>> +            p2m_type_t p2mt;
>>>>> +            const unsigned long valid_mask =
>>>>> +#ifdef CONFIG_X86
>>>>> +                p2m_to_mask(p2m_ram_rw) | p2m_to_mask(p2m_ram_logdirty);
>>>>> +#else
>>>>> +                p2m_to_mask(p2m_ram_rw);
>>>>> +#endif
>>>>
>>>> The set of permitted types didn't change, yet a justification for the resulting
>>>> limitation also didn't appear.
>>>>
>>>
>>> Yes, that's missing, indeed.
>>> Should the set of types be different for reading and writing? For
>>> instance do not allow writing to read-only memory?
>>
>> Of course.
>>
>>> Given that it looks like different architectures have different
>>> meanings and definitions for these constants, should it not be better
>>> to define some new constants for this specific usage? For instance
>>> P2M_READ_TYPES and P2M_WRITE_TYPES?
>>
>> Perhaps, yes. The suggested names look overly generic to me, though.
> 
> I suppose P2M_READABLE_TYPES and P2M_WRITABLE_TYPES are more correct
> but still too generic.
> P2M_EXPORTABLE_TYPES and P2M_IMPORTABLE_TYPES ?

First: Do you foresee uses of those constants anywhere else? If not (I
don't), tie the names to this particular operation. That'll make them
entirely non-generic.

>>>>> @@ -2012,6 +2145,18 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>              start_extent);
>>>>>          break;
>>>>>
>>>>> +    case XENMEM_foreigncopy:
>>>>> +        /*
>>>>> +         * Instead of using "start_extent" we update the structure back,
>>>>> +         * we update it back in anyway to tell caller were the copy
>>>>> +         * stopped.
>>>>> +         */
>>>>> +        if ( unlikely(start_extent) )
>>>>> +            return -EINVAL;
>>>>
>>>> As before - please be precise with comments like this. We update it back also
>>>> when encoding a continuation. Perhaps instead "..., to indicate the point of
>>>> failure to the caller as well as to encode continuations without being
>>>> constrained by MEMOP_EXTENT_SHIFT".
>>>>
>>>
>>> What about (trying to include your suggestion, to be fixed for line length):
>>>
>>>         /*
>>>          * Instead of using "start_extent" for the continuation, we
>>> update the structure back,
>>>          * we update the xen_foreigncopy structure back, so we are not
>>> constrained
>>>          * by MEMOP_EXTENT_SHIFT.
>>>          * We copy it back also to tell the caller where the copy stopped.
>>>          */
>>
>> One of the things I take issue with (because it's hard to read that way,
>> at least for me) is the repeated use of "update ... back", effectively
>> saying the same things twice. The last sentence also wants disambiguating
>> towards the "stopped" possibly being a non-error situation as well.
>>
> 
> Changed to
> 
>         /*
>          * Instead of using "start_extent" for the continuation, we update
>          * the xen_foreigncopy structure back, so we are not constrained by
>          * MEMOP_EXTENT_SHIFT.
>          * We copy it back also to tell the caller where the copy stopped
>          * (either for error or because all frames were copied).
>          */

Thanks.

>>>>> +    XEN_GUEST_HANDLE(uint8) buffer;
>>>>> +};
>>>>
>>>> What was (again) left unaddressed is the question towards using GFNs on both
>>>> sides of the copy. This would eliminate the need for the flags field, taken
>>>> by a 2nd domid_t one then.
>>>>
>>>
>>> This was addressed in
>>> https://lists.xenproject.org/archives/html/xen-devel/2026-06/msg00567.html
>>
>> Well, yes, but not in a satisfactory way. Back channels tell me that you
>> actually got the same feedback already on internal review. Which makes it
>> all the more puzzling that you insist on doing it differently. Multiple
>> maintainers asking for the same thing may be an indication of something.
> 
> Not needing to have backchannel feedback, I already wrote that a
> similar approach was tried and made the code more complicated.

Even if indeed so: Yet at the same time more flexible.

> Both maintainers didn't comment on my replies so I assume they were
> fine with it.
> And you are failing to provide positive feedback.
> I asked (that one internally) for examples of guest buffers provided
> as frame numbers but I got no answer (or better the answer was more
> "currently there are not").
> Also note that the location of xen_foreigncopy_t structure is also
> provided using a guest pointer.
> I remember there were some discussions about ABI changes (2/3 years
> ago) to address this and other issues but I cannot see much progress.

And it's that (very slowly progressing effort) which made me ask. The
fewer virtual addresses we bake into new sub-ops, the better for that
effort. And no, that doesn't go as far as completely eliminating
handles (presently representing virtual addresses) - that needs to be
part of the new ABI.

To preempt the argument towards "fewer virtual addresses" not really
being true when changing from handle-to-uint8 to handle-to-pfn: The
former won't be able to express a buffer mapped contiguously in VA
space, but discontiguous in PA space. The latter will, simply be
avoiding buffer VAs in the first place (the array of frame numbers
can e.g. be placed in a dedicated hypercall argument area known to be
physically contiguous).

> That's why I say this is out of scope.

There's nothing scope related here. We're discussing how to shape the
new sub-op interface.

>>> and in minor way by
>>> https://lists.xenproject.org/archives/html/xen-devel/2026-06/msg00847.html.
>>> It was considered but more complicated and worse from a performance perspective.
>>
>> Okay, performance-wise worse would of course be relevant. But that would
>> need supporting by numbers (for both PV and PVH Dom0, as the latter
>> incurs extra overhead for virtual-address-based hypercall buffer operands).
> 
> I'm more concerned about the PV case than PVH to be honest.

For your (immediate) internal purposes that may be fine, but PVH Dom0
more likely being the future, for upstream both need considering
equally.

Jan

Re: [PATCH v6 12/16] xen: implement new foreign copy hypercall

[Frediano Ziglio]

On Wed, 24 Jun 2026 at 07:44, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 23.06.2026 23:18, Frediano Ziglio wrote:
> > On Tue, 23 Jun 2026 at 14:21, Jan Beulich <jbeulich@suse.com> wrote:
> >> On 23.06.2026 12:55, Frediano Ziglio wrote:
> >>> On Mon, 22 Jun 2026 at 11:34, Jan Beulich <jbeulich@suse.com> wrote:
> >>>> On 19.06.2026 15:04, Frediano Ziglio wrote:
> >>>>> --- a/xen/common/memory.c
> >>>>> +++ b/xen/common/memory.c
> >>>>> @@ -1545,6 +1545,139 @@ static int acquire_resource(
> >>>>>      return rc;
> >>>>>  }
> >>>>>
> >>>>> +/*
> >>>>> + * The "noinline" qualifier avoids the compiler to create a large function
> >>>>> + * consuming quite a lot of stack.
> >>>>> + */
> >>>>> +static int noinline mem_foreigncopy(
> >>>>> +    XEN_GUEST_HANDLE_PARAM(xen_foreigncopy_t) arg)
> >>>>> +{
> >>>>> +    struct domain *d, *const currd = current->domain;
> >>>>> +    xen_foreigncopy_t copy;
> >>>>> +    int rc, direction;
> >>>>> +
> >>>>> +    if ( copy_from_guest(&copy, arg, 1) )
> >>>>> +        return -EFAULT;
> >>>>> +
> >>>>> +    if ( copy.flags & ~XENMEM_foreigncopy_direction )
> >>>>> +        return -EINVAL;
> >>>>> +
> >>>>> +    direction = copy.flags & XENMEM_foreigncopy_direction;
> >>>>> +
> >>>>> +    rc = rcu_lock_remote_domain_by_id(copy.domid, &d);
> >>>>
> >>>> Iirc I did ask before why this isn't ..._by_any_id().
> >>>
> >>> I probably was confused by the question about MMUEXT and the 2 domains.
> >>> There are different similar hypercalls (like the mentioned MMUEXT but
> >>> also hypercalls to map foreign domain memory) that have this check
> >>> (not the same domain). Any domain has, obviously, access to its own
> >>> memory, so it should not have to use hypercall to access its own
> >>> memory. If it does it looks like a mistake causing performance issues
> >>> or an attempt to circumvent security; in either case you would like to
> >>> avoid it.
> >>
> >> No. Self-grants are possible as well, for example, and for a good reason.
> >> Allowing normally-remote operations on oneself helps with testing, for
> >> example. It may also help avoid needing to special-case "self" in code
> >> which needs to cover both cases.
> >
> > But this is not a grant, it's a copy.
>
> Sure, but the underlying principle is what matters. Plus you don't prevent
> self-copy by using ..._by_id(), you only preclude the use of DOMID_SELF.
>

Sure about this? The current implementation is

int rcu_lock_remote_domain_by_id(domid_t dom, struct domain **d)
{
    if ( (*d = rcu_lock_domain_by_id(dom)) == NULL )
        return -ESRCH;

    if ( *d == current->domain )
    {
        rcu_unlock_domain(*d);
        return -EPERM;
    }

    return 0;
}

rcu_lock_domain_by_id returns NULL if the domain is DOMID_SELF so it
would be a -ESRCH, if it's the current domain it would be excluded by
*d == current->domain returning -EPERM.

> >>>>> +    if ( rc )
> >>>>> +        return rc;
> >>>>> +
> >>>>> +    if ( copy.nr_frames == 0 )
> >>>>> +    {
> >>>>> +        rcu_unlock_domain(d);
> >>>>> +        return 0;
> >>>>> +    }
> >>>>
> >>>> Any reason this cannot also be "goto out"? The more that now that you have
> >>>> moved this past the domid validity check, imo it should further move to ...
> >>>
> >>> The only reason was style and to avoid a memory copy, but it's not a
> >>> hot case so I'll change to "goto out" (no strong about it).
> >>>
> >>>>> +    /*
> >>>>> +     * Check we are allowed to map and access these foreign pages.
> >>>>> +     */
> >>>>> +    rc = xsm_map_gmfn_foreign(XSM_TARGET, currd, d);
> >>>>> +    if ( rc )
> >>>>> +        goto out;
> >>>>
> >>>> ... below here. Perhaps simply as
> >>>>
> >>>>     if ( rc || !copy.nr_frames )
> >>>>         goto out;
> >>>>
> >>>
> >>> I think this would be confusing with the above "Check we are allowed
> >>> to map and access these foreign pages" comment.
> >>> Are you okay with just the change above to "goto out" ?
> >>
> >> I do want the order adjusted as indicated. I won't insist on (but I would
> >> prefer) folding both if()-s.
> >>
> >
> > What about
> >
> >     /*
> >      * Check we are allowed to map and access these foreign pages.
> >      */
> >     rc = xsm_map_gmfn_foreign(XSM_TARGET, currd, d);
> >     if ( rc )
> >         goto out;
> >
> >     while ( copy.nr_frames )
> >     {
> >         /*
> >          * Arbitrary size.  Not too much stack space, and a reasonable stride
> >          * for continuation checks.
> >          */
>
> That's fine.
>

Changed

> >>>>> +    do {
> >>>>> +        /*
> >>>>> +         * Arbitrary size.  Not too much stack space, and a reasonable stride
> >>>>> +         * for continuation checks.
> >>>>> +         */
> >>>>> +        xen_pfn_t gfn_list[32];
> >>>>> +        unsigned int todo = MIN(ARRAY_SIZE(gfn_list), copy.nr_frames);
> >>>>> +
> >>>>> +        rc = -EFAULT;
> >>>>> +        if ( copy_from_guest(gfn_list, copy.frame_list, todo) )
> >>>>> +            goto out;
> >>>>> +
> >>>>> +        for ( unsigned int i = 0; i < todo; i++ )
> >>>>> +        {
> >>>>> +            struct page_info *foreign_page;
> >>>>> +            mfn_t foreign_mfn;
> >>>>> +            void *foreign;
> >>>>> +            p2m_type_t p2mt;
> >>>>> +            const unsigned long valid_mask =
> >>>>> +#ifdef CONFIG_X86
> >>>>> +                p2m_to_mask(p2m_ram_rw) | p2m_to_mask(p2m_ram_logdirty);
> >>>>> +#else
> >>>>> +                p2m_to_mask(p2m_ram_rw);
> >>>>> +#endif
> >>>>
> >>>> The set of permitted types didn't change, yet a justification for the resulting
> >>>> limitation also didn't appear.
> >>>>
> >>>
> >>> Yes, that's missing, indeed.
> >>> Should the set of types be different for reading and writing? For
> >>> instance do not allow writing to read-only memory?
> >>
> >> Of course.
> >>
> >>> Given that it looks like different architectures have different
> >>> meanings and definitions for these constants, should it not be better
> >>> to define some new constants for this specific usage? For instance
> >>> P2M_READ_TYPES and P2M_WRITE_TYPES?
> >>
> >> Perhaps, yes. The suggested names look overly generic to me, though.
> >
> > I suppose P2M_READABLE_TYPES and P2M_WRITABLE_TYPES are more correct
> > but still too generic.
> > P2M_EXPORTABLE_TYPES and P2M_IMPORTABLE_TYPES ?
>
> First: Do you foresee uses of those constants anywhere else? If not (I
> don't), tie the names to this particular operation. That'll make them
> entirely non-generic.
>
> >>>>> @@ -2012,6 +2145,18 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>>>              start_extent);
> >>>>>          break;
> >>>>>
> >>>>> +    case XENMEM_foreigncopy:
> >>>>> +        /*
> >>>>> +         * Instead of using "start_extent" we update the structure back,
> >>>>> +         * we update it back in anyway to tell caller were the copy
> >>>>> +         * stopped.
> >>>>> +         */
> >>>>> +        if ( unlikely(start_extent) )
> >>>>> +            return -EINVAL;
> >>>>
> >>>> As before - please be precise with comments like this. We update it back also
> >>>> when encoding a continuation. Perhaps instead "..., to indicate the point of
> >>>> failure to the caller as well as to encode continuations without being
> >>>> constrained by MEMOP_EXTENT_SHIFT".
> >>>>
> >>>
> >>> What about (trying to include your suggestion, to be fixed for line length):
> >>>
> >>>         /*
> >>>          * Instead of using "start_extent" for the continuation, we
> >>> update the structure back,
> >>>          * we update the xen_foreigncopy structure back, so we are not
> >>> constrained
> >>>          * by MEMOP_EXTENT_SHIFT.
> >>>          * We copy it back also to tell the caller where the copy stopped.
> >>>          */
> >>
> >> One of the things I take issue with (because it's hard to read that way,
> >> at least for me) is the repeated use of "update ... back", effectively
> >> saying the same things twice. The last sentence also wants disambiguating
> >> towards the "stopped" possibly being a non-error situation as well.
> >>
> >
> > Changed to
> >
> >         /*
> >          * Instead of using "start_extent" for the continuation, we update
> >          * the xen_foreigncopy structure back, so we are not constrained by
> >          * MEMOP_EXTENT_SHIFT.
> >          * We copy it back also to tell the caller where the copy stopped
> >          * (either for error or because all frames were copied).
> >          */
>
> Thanks.
>
> >>>>> +    XEN_GUEST_HANDLE(uint8) buffer;
> >>>>> +};
> >>>>
> >>>> What was (again) left unaddressed is the question towards using GFNs on both
> >>>> sides of the copy. This would eliminate the need for the flags field, taken
> >>>> by a 2nd domid_t one then.
> >>>>
> >>>
> >>> This was addressed in
> >>> https://lists.xenproject.org/archives/html/xen-devel/2026-06/msg00567.html
> >>
> >> Well, yes, but not in a satisfactory way. Back channels tell me that you
> >> actually got the same feedback already on internal review. Which makes it
> >> all the more puzzling that you insist on doing it differently. Multiple
> >> maintainers asking for the same thing may be an indication of something.
> >
> > Not needing to have backchannel feedback, I already wrote that a
> > similar approach was tried and made the code more complicated.
>
> Even if indeed so: Yet at the same time more flexible.
>
> > Both maintainers didn't comment on my replies so I assume they were
> > fine with it.
> > And you are failing to provide positive feedback.
> > I asked (that one internally) for examples of guest buffers provided
> > as frame numbers but I got no answer (or better the answer was more
> > "currently there are not").
> > Also note that the location of xen_foreigncopy_t structure is also
> > provided using a guest pointer.
> > I remember there were some discussions about ABI changes (2/3 years
> > ago) to address this and other issues but I cannot see much progress.
>
> And it's that (very slowly progressing effort) which made me ask. The
> fewer virtual addresses we bake into new sub-ops, the better for that
> effort. And no, that doesn't go as far as completely eliminating
> handles (presently representing virtual addresses) - that needs to be
> part of the new ABI.
>

In other words, you want me to code something temporary that you
already know that needs to be changed.

> To preempt the argument towards "fewer virtual addresses" not really
> being true when changing from handle-to-uint8 to handle-to-pfn: The
> former won't be able to express a buffer mapped contiguously in VA
> space, but discontiguous in PA space. The latter will, simply be
> avoiding buffer VAs in the first place (the array of frame numbers
> can e.g. be placed in a dedicated hypercall argument area known to be
> physically contiguous).
>

If it's mapped continuously in VA and you pass the VA I don't
understand the problem. From the way I see it's more the latter that's
the problem.

> > That's why I say this is out of scope.
>
> There's nothing scope related here. We're discussing how to shape the
> new sub-op interface.
>
> >>> and in minor way by
> >>> https://lists.xenproject.org/archives/html/xen-devel/2026-06/msg00847.html.
> >>> It was considered but more complicated and worse from a performance perspective.
> >>
> >> Okay, performance-wise worse would of course be relevant. But that would
> >> need supporting by numbers (for both PV and PVH Dom0, as the latter
> >> incurs extra overhead for virtual-address-based hypercall buffer operands).
> >
> > I'm more concerned about the PV case than PVH to be honest.
>
> For your (immediate) internal purposes that may be fine, but PVH Dom0
> more likely being the future, for upstream both need considering
> equally.
>

From an internal discussion:

--------------------
About HVM and VAs/PFNs, I was thinking. We pass a single PFN for a
page containing either
- a single list of PFNs fitting into the page (plus number of entries)
for small hypercalls (most of them)
- a page containing a list of PFNs pointing to arrays of PFNs for
large hypercalls (like kexec load and few others)
Now the GUEST_HANDLEs are treated differently, instead of VAs they
contain offset into array above plus page offset.

Okay, let's do an example. I want to call a DOMCTL which have a handle
to a small array, so I need to pass the domctl structure and the
array, I suppose I need 2 PFNs (unless domct or the array span
multiple pages) so kernel would need to build and pass an array of 2
PFNs, let's say 0xabcabc01 and 0xabcabc02. I build a page with
1- 2 (number of PFNs)
2- 0xabcabc01
3- 0xabcabc02
I pass the PFN of the above page (how it's a detail, probably an
additional register), number of hypercall and... a GUEST_HANDLE. Here
the guest handle would be something like 0x0000ppp where ppp is the
page offset inside 0xabcabc01 page. Why 0xabcabc01 ? Because the upper
part of the guest handle is 0 (so the first entry in the PFNs array
above). Inside the domctl structure the guest handle to the array will
be something like 0x0001ppp where the 0x0001 means 0xabcabc02 while
ppp here is the page offset into 0xabcabc02.
What if the array is bigger than a single page? Let's say it spans
into 3 pages, you would have something like
1- 4 (number of PFNs)
2- 0xabcabc01
3- 0xabcabc02
4- 0xabcabc03
5- 0xabcabc04
(okay, in all example pages are contiguous but not important)
The guest handle for the array would still be 0x0001ppp but the code
will continue on array entry 2 (0xabcabc03) and 3 (0xabcabc04)
--------------------
The distinction for the above could be a flag added to the hypercall
number or automatic on type of VM (PV/HVM) but for compatibility I
would go for the first. But these are details.
This would work without much API changes and minor hypervisor changes
(mainly copy_from_guest and similar macros).

Okay, the above is a bit OT here but the point is that the change you
are asking me won't help with this in the future, basically you are
asking me an implementation based on an implementation that is not
currently even on paper.

> Jan

Frediano

Re: [PATCH v6 12/16] xen: implement new foreign copy hypercall

[Jan Beulich]

On 26.06.2026 16:14, Frediano Ziglio wrote:
> On Wed, 24 Jun 2026 at 07:44, Jan Beulich <jbeulich@suse.com> wrote:
>> On 23.06.2026 23:18, Frediano Ziglio wrote:
>>> On Tue, 23 Jun 2026 at 14:21, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 23.06.2026 12:55, Frediano Ziglio wrote:
>>>>> On Mon, 22 Jun 2026 at 11:34, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>> On 19.06.2026 15:04, Frediano Ziglio wrote:
>>>>>>> --- a/xen/common/memory.c
>>>>>>> +++ b/xen/common/memory.c
>>>>>>> @@ -1545,6 +1545,139 @@ static int acquire_resource(
>>>>>>>      return rc;
>>>>>>>  }
>>>>>>>
>>>>>>> +/*
>>>>>>> + * The "noinline" qualifier avoids the compiler to create a large function
>>>>>>> + * consuming quite a lot of stack.
>>>>>>> + */
>>>>>>> +static int noinline mem_foreigncopy(
>>>>>>> +    XEN_GUEST_HANDLE_PARAM(xen_foreigncopy_t) arg)
>>>>>>> +{
>>>>>>> +    struct domain *d, *const currd = current->domain;
>>>>>>> +    xen_foreigncopy_t copy;
>>>>>>> +    int rc, direction;
>>>>>>> +
>>>>>>> +    if ( copy_from_guest(&copy, arg, 1) )
>>>>>>> +        return -EFAULT;
>>>>>>> +
>>>>>>> +    if ( copy.flags & ~XENMEM_foreigncopy_direction )
>>>>>>> +        return -EINVAL;
>>>>>>> +
>>>>>>> +    direction = copy.flags & XENMEM_foreigncopy_direction;
>>>>>>> +
>>>>>>> +    rc = rcu_lock_remote_domain_by_id(copy.domid, &d);
>>>>>>
>>>>>> Iirc I did ask before why this isn't ..._by_any_id().
>>>>>
>>>>> I probably was confused by the question about MMUEXT and the 2 domains.
>>>>> There are different similar hypercalls (like the mentioned MMUEXT but
>>>>> also hypercalls to map foreign domain memory) that have this check
>>>>> (not the same domain). Any domain has, obviously, access to its own
>>>>> memory, so it should not have to use hypercall to access its own
>>>>> memory. If it does it looks like a mistake causing performance issues
>>>>> or an attempt to circumvent security; in either case you would like to
>>>>> avoid it.
>>>>
>>>> No. Self-grants are possible as well, for example, and for a good reason.
>>>> Allowing normally-remote operations on oneself helps with testing, for
>>>> example. It may also help avoid needing to special-case "self" in code
>>>> which needs to cover both cases.
>>>
>>> But this is not a grant, it's a copy.
>>
>> Sure, but the underlying principle is what matters. Plus you don't prevent
>> self-copy by using ..._by_id(), you only preclude the use of DOMID_SELF.
> 
> Sure about this?

No, I'm sorry: I (repeatedly) managed to ignore the "remote" in the function
called. That said, my request stands: No arbitrary restrictions please. If
you can properly justify a restriction, that's a different thing.

>>>>>>> +    XEN_GUEST_HANDLE(uint8) buffer;
>>>>>>> +};
>>>>>>
>>>>>> What was (again) left unaddressed is the question towards using GFNs on both
>>>>>> sides of the copy. This would eliminate the need for the flags field, taken
>>>>>> by a 2nd domid_t one then.
>>>>>>
>>>>>
>>>>> This was addressed in
>>>>> https://lists.xenproject.org/archives/html/xen-devel/2026-06/msg00567.html
>>>>
>>>> Well, yes, but not in a satisfactory way. Back channels tell me that you
>>>> actually got the same feedback already on internal review. Which makes it
>>>> all the more puzzling that you insist on doing it differently. Multiple
>>>> maintainers asking for the same thing may be an indication of something.
>>>
>>> Not needing to have backchannel feedback, I already wrote that a
>>> similar approach was tried and made the code more complicated.
>>
>> Even if indeed so: Yet at the same time more flexible.
>>
>>> Both maintainers didn't comment on my replies so I assume they were
>>> fine with it.
>>> And you are failing to provide positive feedback.
>>> I asked (that one internally) for examples of guest buffers provided
>>> as frame numbers but I got no answer (or better the answer was more
>>> "currently there are not").
>>> Also note that the location of xen_foreigncopy_t structure is also
>>> provided using a guest pointer.
>>> I remember there were some discussions about ABI changes (2/3 years
>>> ago) to address this and other issues but I cannot see much progress.
>>
>> And it's that (very slowly progressing effort) which made me ask. The
>> fewer virtual addresses we bake into new sub-ops, the better for that
>> effort. And no, that doesn't go as far as completely eliminating
>> handles (presently representing virtual addresses) - that needs to be
>> part of the new ABI.
> 
> In other words, you want me to code something temporary that you
> already know that needs to be changed.

What do you mean by "temporary"? We will need to live with the present
ABI for the foreseeable future. The new ABI's requirements haven't even
been spelled out yet. Patches to allow use of physical addresses in
place of virtual ones were actually turned down on the grounds of there
not having been a write-down of all requirements.

>> To preempt the argument towards "fewer virtual addresses" not really
>> being true when changing from handle-to-uint8 to handle-to-pfn: The
>> former won't be able to express a buffer mapped contiguously in VA
>> space, but discontiguous in PA space. The latter will, simply be
>> avoiding buffer VAs in the first place (the array of frame numbers
>> can e.g. be placed in a dedicated hypercall argument area known to be
>> physically contiguous).
> 
> If it's mapped continuously in VA and you pass the VA I don't
> understand the problem. From the way I see it's more the latter that's
> the problem.

I'm talking of the future, where VAs wouldn't be used anymore. The
buffer you use couldn't be described by a single PA, unless the caller
took specific measures up front.

Jan

Re: [PATCH v6 01/16] libs/guest: Reduce number of parts in write_split_record

[Andrew Cooper]

On 19/06/2026 2:04 pm, Frediano Ziglio wrote:
> Small optimization.
> There's no much sense to split the header in 2 pieces, it will
> just take more time and space to reassemble them in the final
> buffer.
> This also avoids truncating combined_length to 32 bit in case of
> 64 bit machines potentially avoiding following record_length check
> (it could still be truncated writing it in xc_sr_rhdr structure
> but the following check will catch it).
> The function become more coherent with following read_record
> function.
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> --

You need to use 3 dashes here.

~Andrew

Re: [PATCH v6 02/16] libs/guest: Reduce number of I/O vectors in write_batch

[Andrew Cooper]

On 19/06/2026 2:04 pm, Frediano Ziglio wrote:
> diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
> index fdbceab52e..68ce1aeb98 100644
> --- a/tools/libs/guest/xg_sr_save.c
> +++ b/tools/libs/guest/xg_sr_save.c
> @@ -97,9 +97,11 @@ static int write_batch(struct xc_sr_context *ctx)
>      void *page, *orig_page;
>      uint64_t *rec_pfns = NULL;
>      struct iovec *iov = NULL; int iovcnt = 0;
> -    struct xc_sr_rec_page_data_header hdr = { 0 };
> -    struct xc_sr_record rec = {
> -        .type = REC_TYPE_PAGE_DATA,
> +    struct {
> +        struct xc_sr_rhdr rec;
> +        struct xc_sr_rec_page_data_header page_data;
> +    } hdrs = {
> +        { .type = REC_TYPE_PAGE_DATA },

.rec = { .type = ... },

Otherwise this is fragile to reordering.

~Andrew

Re: [PATCH v6 03/16] libs/guest: Reduce number of I/O vectors in write_batch

[Andrew Cooper]

This has the same exact subject as the prior patch.

Either it wants merging, as they're both in the same function, or the
subject wants to be different.  Even a "Further ..." prefix would help.

On 19/06/2026 2:04 pm, Frediano Ziglio wrote:
> Each page was sent using a different iovec item. This potentially exceed
> Linux maximum (1024).

Linux cannot have a maximum of 1024 because this has been working fine
for a decade using 1028 in the common case.

> Coalesce adjacent IO vector elements to attempt to reduce the number of
> overall IO vectors for each operation.
> Also some implementation (MiniOS) emulate writev with multiple write calls.
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Re: [PATCH v6 04/16] libs/guest: Use a single write_exact in write_headers

[Andrew Cooper]

On 19/06/2026 2:04 pm, Frediano Ziglio wrote:
> diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
> index eba33f861a..8c31f9f86c 100644
> --- a/tools/libs/guest/xg_sr_save.c
> +++ b/tools/libs/guest/xg_sr_save.c
> @@ -10,17 +10,22 @@ static int write_headers(struct xc_sr_context *ctx, uint16_t guest_type)
>  {
>      xc_interface *xch = ctx->xch;
>      int32_t xen_version = xc_version(xch, XENVER_version, NULL);
> -    struct xc_sr_ihdr ihdr = {
> -        .marker  = IHDR_MARKER,
> -        .id      = htonl(IHDR_ID),
> -        .version = htonl(3),
> -        .options = htons(IHDR_OPT_LITTLE_ENDIAN),
> -    };
> -    struct xc_sr_dhdr dhdr = {
> -        .type       = guest_type,
> -        .page_shift = XC_PAGE_SHIFT,
> -        .xen_major  = (xen_version >> 16) & 0xffff,
> -        .xen_minor  = (xen_version)       & 0xffff,
> +    struct {
> +        struct xc_sr_ihdr ihdr;
> +        struct xc_sr_dhdr dhdr;
> +    } hdrs = {
> +        {

.ihdr = {

> +            .marker  = IHDR_MARKER,
> +            .id      = htonl(IHDR_ID),
> +            .version = htonl(3),
> +            .options = htons(IHDR_OPT_LITTLE_ENDIAN),
> +        },
> +        {

.dhdr = {

~Andrew

Re: [PATCH v6 09/16] libs/ctrl: Allows writev_exact to change iov array

[Andrew Cooper]

On 19/06/2026 2:04 pm, Frediano Ziglio wrote:
> diff --git a/tools/libs/ctrl/xc_private.h b/tools/libs/ctrl/xc_private.h
> index b5892ae8dc..3af996e900 100644
> --- a/tools/libs/ctrl/xc_private.h
> +++ b/tools/libs/ctrl/xc_private.h
> @@ -383,7 +383,7 @@ int xc_flush_mmu_updates(xc_interface *xch, struct xc_mmu *mmu);
>  /* Return 0 on success; -1 on error setting errno. */
>  int read_exact(int fd, void *data, size_t size); /* EOF => -1, errno=0 */
>  int write_exact(int fd, const void *data, size_t size);
> -int writev_exact(int fd, const struct iovec *iov, int iovcnt);
> +int writev_exact(int fd, struct iovec *iov, int iovcnt);

No callers care, but this is written with a const pointer to match writev().

If we really do want to take this change, then it needs to come with a
comment saying /* May edit iov to cope with partial writes. */

~Andrew

Re: [PATCH v6 05/16] libs/guest: allocate various migration arrays just once

[Andrew Cooper]

On 19/06/2026 2:04 pm, Frediano Ziglio wrote:
> From: Edwin Török <edwin.torok@citrix.com>
>
> Allocate these array just once at the start of migration,
> using the maximum batch size, and free them at the end.
>
> Signed-off-by: Edwin Török <edwin.torok@citrix.com>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>

The reason why these are allocated and freed on every iteration is so
they show up as uninitialised to valgrind or ASAN.

Maybe that's overly cautious, and maybe we can relax it, but it's also
not as if these allocations/frees are anywhere but in the noise on this
path.

> --
> Changes since v2:
> - change prefix in subject.
>
> Changes since v3:
> - fix comment style
>
> Changes since v4:
> - change order of fields in structure.
> ---
>  tools/libs/guest/xg_sr_common.h | 13 +++++++
>  tools/libs/guest/xg_sr_save.c   | 66 +++++++++++++--------------------
>  2 files changed, 39 insertions(+), 40 deletions(-)
>
> diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
> index f1573aefcb..95b0564e5c 100644
> --- a/tools/libs/guest/xg_sr_common.h
> +++ b/tools/libs/guest/xg_sr_common.h
> @@ -209,6 +209,18 @@ static inline int update_blob(struct xc_sr_blob *blob,
>      return 0;
>  }
>  
> +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];
> +    int errors[MAX_BATCH_SIZE];
> +};
> +
>  struct xc_sr_context
>  {
>      xc_interface *xch;
> @@ -244,6 +256,7 @@ struct xc_sr_context
>              unsigned long *deferred_pages;
>              unsigned long nr_deferred_pages;
>              xc_hypercall_buffer_t dirty_bitmap_hbuf;
> +            struct xc_sr_context_save_buffers *buffers;

Please move the higher hunk down here, as:

    struct xc_sr_context_safe_buffers {
        ...
    } *buffers;


This helps keep related content together.

(I'm half tempted to say they don't even need a second memory
allocation, but right now xc_sr_context is 538 bytes, and this buffer
object is nearly 16k which we don't really want to be adding as overhead
to the restore side.)

>          } save;
>  
>          struct /* Restore data. */
> diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
> index 8c31f9f86c..4988d8040b 100644
> --- a/tools/libs/guest/xg_sr_save.c
> +++ b/tools/libs/guest/xg_sr_save.c
> @@ -86,16 +86,16 @@ static int write_checkpoint_record(struct xc_sr_context *ctx)
>  static int write_batch(struct xc_sr_context *ctx)
>  {
>      xc_interface *xch = ctx->xch;
> -    xen_pfn_t *mfns = NULL, *types = NULL;
> +    xen_pfn_t *mfns, *types;
>      void *guest_mapping = NULL;
> -    void **guest_data = NULL;
> -    void **local_pages = NULL;
> -    int *errors = NULL, rc = -1;
> +    void **guest_data;
> +    void **local_pages;
> +    int *errors, rc = -1;
>      unsigned int i, p, nr_pages = 0, nr_pages_mapped = 0;
>      unsigned int nr_pfns = ctx->save.nr_batch_pfns;
>      void *page, *orig_page;
> -    uint64_t *rec_pfns = NULL;
> -    struct iovec *iov = NULL; int iovcnt = 0;
> +    uint64_t *rec_pfns;
> +    struct iovec *iov; int iovcnt = 0;
>      struct {
>          struct xc_sr_rhdr rec;
>          struct xc_sr_rec_page_data_header page_data;
> @@ -104,26 +104,24 @@ static int write_batch(struct xc_sr_context *ctx)
>      };
>  
>      assert(nr_pfns != 0);
> +    assert(nr_pfns <= MAX_BATCH_SIZE);
> +    assert(ctx->save.buffers);
>  
>      /* Mfns of the batch pfns. */
> -    mfns = malloc(nr_pfns * sizeof(*mfns));
> +    mfns = ctx->save.buffers->mfns;
>      /* Types of the batch pfns. */
> -    types = malloc(nr_pfns * sizeof(*types));
> +    types = ctx->save.buffers->types;
>      /* Errors from attempting to map the gfns. */
> -    errors = malloc(nr_pfns * sizeof(*errors));
> +    errors = ctx->save.buffers->errors;
>      /* Pointers to page data to send.  Mapped gfns or local allocations. */
> -    guest_data = calloc(nr_pfns, sizeof(*guest_data));
> +    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 = calloc(nr_pfns, sizeof(*local_pages));
> +    local_pages = ctx->save.buffers->local_pages;
> +    memset(local_pages, 0, sizeof(*local_pages) * nr_pfns);
>      /* iovec[] for writev(). */
> -    iov = malloc((nr_pfns + 2) * sizeof(*iov));
> -
> -    if ( !mfns || !types || !errors || !guest_data || !local_pages || !iov )
> -    {
> -        ERROR("Unable to allocate arrays for a batch of %u pages",
> -              nr_pfns);
> -        goto err;
> -    }
> +    iov = ctx->save.buffers->iov;
> +    rec_pfns = ctx->save.buffers->rec_pfns;

These two hunks are rather messy.  You don't actually need the first
hunk at all; the pointers can all start initialised to NULL.

Alternatively, if you want to avoid the redundant assignments, then
split the variable block in half and list the second half as /*
shorthand names for the buffers */ or somesuch.  This will need to come
ahead of the asserts().

But if you're going to try cleaning this up, please do it in a separate
patch.

>  
>      for ( i = 0; i < nr_pfns; ++i )
>      {
> @@ -209,14 +207,6 @@ static int write_batch(struct xc_sr_context *ctx)
>          }
>      }
>  
> -    rec_pfns = malloc(nr_pfns * sizeof(*rec_pfns));
> -    if ( !rec_pfns )
> -    {
> -        ERROR("Unable to allocate %zu bytes of memory for page data pfn list",
> -              nr_pfns * sizeof(*rec_pfns));
> -        goto err;
> -    }
> -
>      hdrs.rec.length = sizeof(hdrs.page_data);
>      hdrs.rec.length += nr_pfns * sizeof(*rec_pfns);
>      hdrs.rec.length += nr_pages * PAGE_SIZE;
> @@ -267,17 +257,13 @@ static int write_batch(struct xc_sr_context *ctx)
>      rc = ctx->save.nr_batch_pfns = 0;
>  
>   err:
> -    free(rec_pfns);
>      if ( guest_mapping )
>          xenforeignmemory_unmap(xch->fmem, guest_mapping, nr_pages_mapped);
>      for ( i = 0; local_pages && i < nr_pfns; ++i )
> +    {
>          free(local_pages[i]);
> -    free(iov);
> -    free(local_pages);
> -    free(guest_data);
> -    free(errors);
> -    free(types);
> -    free(mfns);
> +        local_pages[i] = NULL;
> +    }

Given this NULL-ing, the memset earlier shouldn't be needed.

Along with a memset() over guest_mapping, that gets rid of all the early
memset()'s I think.

>  
>      return rc;
>  }
> @@ -806,18 +792,18 @@ static int setup(struct xc_sr_context *ctx)
>  
>      dirty_bitmap = xc_hypercall_buffer_alloc_pages(
>          xch, dirty_bitmap, NRPAGES(bitmap_size(ctx->save.p2m_size)));
> -    ctx->save.batch_pfns = malloc(MAX_BATCH_SIZE *
> -                                  sizeof(*ctx->save.batch_pfns));
>      ctx->save.deferred_pages = bitmap_alloc(ctx->save.p2m_size);
> +    ctx->save.buffers = calloc(1, sizeof(*ctx->save.buffers));
>  
> -    if ( !ctx->save.batch_pfns || !dirty_bitmap || !ctx->save.deferred_pages )
> +    if ( !dirty_bitmap || !ctx->save.deferred_pages || !ctx->save.buffers)
>      {
> -        ERROR("Unable to allocate memory for dirty bitmaps, batch pfns and"
> -              " deferred pages");
> +        ERROR("Unable to allocate memory for dirty bitmaps, deferred pages"
> +              " and various batch buffers");
>          rc = -1;
>          errno = ENOMEM;
>          goto err;
>      }
> +    ctx->save.batch_pfns = ctx->save.buffers->batch_pfns;

This is wonky.  As far as I can tell, you've included batch_pfns in the
buffers struct, but left it's old pointer in place, meaning it becomes
dangling when the allocation is freed.

This wants splitting into two patches.  First introduce the buffers
struct with batch_pfns moved only, and sort out the allocation here. 
Then in the subsequent patch, move the contents of write_batch() into
the buffers struct.

~Andrew

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

[Andrew Cooper]

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@citrix.com>
> ---
>  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

[PATCH v6 1.9/16] libs/guest: Allocate rec_pfns earlier in write_batch()

[Andrew Cooper]

For reasons which escape me, rec_pfns are allocated separately to the rest of
the batch allocations.

Allocate them all together.  This will allow for future simplifications to be
performed in an incremental mannor.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Frediano Ziglio <frediano.ziglio@citrix.com>
---
 tools/libs/guest/xg_sr_save.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
index fdbceab52e46..69fe991a8113 100644
--- a/tools/libs/guest/xg_sr_save.c
+++ b/tools/libs/guest/xg_sr_save.c
@@ -116,8 +116,10 @@ static int write_batch(struct xc_sr_context *ctx)
     local_pages = calloc(nr_pfns, sizeof(*local_pages));
     /* iovec[] for writev(). */
     iov = malloc((nr_pfns + 4) * sizeof(*iov));
+    /* page_data record PFNs list */
+    rec_pfns = malloc(nr_pfns * sizeof(*rec_pfns));
 
-    if ( !mfns || !types || !errors || !guest_data || !local_pages || !iov )
+    if ( !mfns || !types || !errors || !guest_data || !local_pages || !iov || !rec_pfns )
     {
         ERROR("Unable to allocate arrays for a batch of %u pages",
               nr_pfns);
@@ -208,14 +210,6 @@ static int write_batch(struct xc_sr_context *ctx)
         }
     }
 
-    rec_pfns = malloc(nr_pfns * sizeof(*rec_pfns));
-    if ( !rec_pfns )
-    {
-        ERROR("Unable to allocate %zu bytes of memory for page data pfn list",
-              nr_pfns * sizeof(*rec_pfns));
-        goto err;
-    }
-
     hdr.count = nr_pfns;
 
     rec.length = sizeof(hdr);
@@ -264,11 +258,11 @@ static int write_batch(struct xc_sr_context *ctx)
     rc = ctx->save.nr_batch_pfns = 0;
 
  err:
-    free(rec_pfns);
     if ( guest_mapping )
         xenforeignmemory_unmap(xch->fmem, guest_mapping, nr_pages_mapped);
     for ( i = 0; local_pages && i < nr_pfns; ++i )
         free(local_pages[i]);
+    free(rec_pfns);
     free(iov);
     free(local_pages);
     free(guest_data);
-- 
2.39.5

[PATCH v6.1 02/16] libs/guest: Reduce number of iovecs in write_batch()

[Andrew Cooper]

From: Frediano Ziglio <freddy77@gmail.com>

Construct all of the headers together in one block, rather than a field at a
time.  Initialise as many of the fields as possible at declaration time.

Start filling in iov[] earlier, to allow for future simplifications.

No practical change.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Frediano Ziglio <frediano.ziglio@citrix.com>
---
 tools/libs/guest/xg_sr_save.c | 45 +++++++++++++++++------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
index 69fe991a8113..7736f4a055e0 100644
--- a/tools/libs/guest/xg_sr_save.c
+++ b/tools/libs/guest/xg_sr_save.c
@@ -97,9 +97,18 @@ static int write_batch(struct xc_sr_context *ctx)
     void *page, *orig_page;
     uint64_t *rec_pfns = NULL;
     struct iovec *iov = NULL; int iovcnt = 0;
-    struct xc_sr_rec_page_data_header hdr = { 0 };
-    struct xc_sr_record rec = {
-        .type = REC_TYPE_PAGE_DATA,
+    struct {
+        struct xc_sr_rhdr rec;
+        struct xc_sr_rec_page_data_header page_data;
+    } hdrs = {
+        .rec = {
+            .type = REC_TYPE_PAGE_DATA,
+            .length = offsetof(struct xc_sr_rec_page_data_header,
+                               pfn[nr_pfns]), /* + the pages to send */
+        },
+        .page_data = {
+            .count = nr_pfns,
+        },
     };
 
     assert(nr_pfns != 0);
@@ -115,7 +124,7 @@ static int write_batch(struct xc_sr_context *ctx)
     /* Pointers to locally allocated pages.  Need freeing. */
     local_pages = calloc(nr_pfns, sizeof(*local_pages));
     /* iovec[] for writev(). */
-    iov = malloc((nr_pfns + 4) * sizeof(*iov));
+    iov = malloc((nr_pfns + 2) * sizeof(*iov));
     /* page_data record PFNs list */
     rec_pfns = malloc(nr_pfns * sizeof(*rec_pfns));
 
@@ -126,6 +135,14 @@ static int write_batch(struct xc_sr_context *ctx)
         goto err;
     }
 
+    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;
+
     for ( i = 0; i < nr_pfns; ++i )
     {
         types[i] = mfns[i] = ctx->save.ops.pfn_to_gfn(ctx,
@@ -210,29 +227,11 @@ static int write_batch(struct xc_sr_context *ctx)
         }
     }
 
-    hdr.count = nr_pfns;
-
-    rec.length = sizeof(hdr);
-    rec.length += nr_pfns * sizeof(*rec_pfns);
-    rec.length += nr_pages * PAGE_SIZE;
+    hdrs.rec.length += nr_pages * PAGE_SIZE;
 
     for ( i = 0; i < nr_pfns; ++i )
         rec_pfns[i] = ((uint64_t)(types[i]) << 32) | ctx->save.batch_pfns[i];
 
-    iov[0].iov_base = &rec.type;
-    iov[0].iov_len = sizeof(rec.type);
-
-    iov[1].iov_base = &rec.length;
-    iov[1].iov_len = sizeof(rec.length);
-
-    iov[2].iov_base = &hdr;
-    iov[2].iov_len = sizeof(hdr);
-
-    iov[3].iov_base = rec_pfns;
-    iov[3].iov_len = nr_pfns * sizeof(*rec_pfns);
-
-    iovcnt = 4;
-
     if ( nr_pages )
     {
         for ( i = 0; i < nr_pfns; ++i )
-- 
2.39.5

Re: [PATCH v6 02/16] libs/guest: Reduce number of I/O vectors in write_batch

[Frediano Ziglio]

On Tue, 30 Jun 2026 at 17:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 19/06/2026 2:04 pm, Frediano Ziglio wrote:
> > diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
> > index fdbceab52e..68ce1aeb98 100644
> > --- a/tools/libs/guest/xg_sr_save.c
> > +++ b/tools/libs/guest/xg_sr_save.c
> > @@ -97,9 +97,11 @@ static int write_batch(struct xc_sr_context *ctx)
> >      void *page, *orig_page;
> >      uint64_t *rec_pfns = NULL;
> >      struct iovec *iov = NULL; int iovcnt = 0;
> > -    struct xc_sr_rec_page_data_header hdr = { 0 };
> > -    struct xc_sr_record rec = {
> > -        .type = REC_TYPE_PAGE_DATA,
> > +    struct {
> > +        struct xc_sr_rhdr rec;
> > +        struct xc_sr_rec_page_data_header page_data;
> > +    } hdrs = {
> > +        { .type = REC_TYPE_PAGE_DATA },
>
> .rec = { .type = ... },
>
> Otherwise this is fragile to reordering.
>

Changed.
Here we are implementing a network protocol, reordering should not be
done, unless you want to break the code.

> ~Andrew

Frediano

Re: [PATCH v6 03/16] libs/guest: Reduce number of I/O vectors in write_batch

[Frediano Ziglio]

On Tue, 30 Jun 2026 at 17:47, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
>
> This has the same exact subject as the prior patch.
>
> Either it wants merging, as they're both in the same function, or the
> subject wants to be different.  Even a "Further ..." prefix would help.
>
> On 19/06/2026 2:04 pm, Frediano Ziglio wrote:
> > Each page was sent using a different iovec item. This potentially exceed
> > Linux maximum (1024).
>
> Linux cannot have a maximum of 1024 because this has been working fine
> for a decade using 1028 in the common case.
>

But the code does not call writev or similars directly, so there's no
limit besides the sky.
The result with 1028 is simply that you do 2 system calls instead of one.

> > Coalesce adjacent IO vector elements to attempt to reduce the number of
> > overall IO vectors for each operation.
> > Also some implementation (MiniOS) emulate writev with multiple write calls.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>

Frediano

Re: [PATCH v6 06/16] libs/call: cache up to 4 pages in hypercall bounce buffers

[Anthony PERARD]

[-- Attachment #1: Type: text/plain, Size: 6194 bytes --]

On Fri, Jun 19, 2026 at 02:04:51PM +0100, Frediano Ziglio wrote:
> From: Edwin Török <edwin.torok@citrix.com>
> 
> During migration there are a lot of mmap/munmap calls,
> because `xc_get_pfn_type_batch` exceeds the default hypercall bounce
> buffer cache size, and needs to allocate every time it is called.

I think xc_get_pfn_type_batch() would allocate a buffer of 2 page top,
in write_batch(), right ?

> 
> `munmap` is slow, especially in a PV Dom0 (takes an emulation fault),
> so is best avoided.
> 
> Eventually it'd be good if the memory pool from  xmalloc_tlsf.c
> was reused here, but for now make it handle the commonly encountered
> sizes (so far up to 4 pages).
> 
> Signed-off-by: Edwin Török <edwin.torok@citrix.com>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
> --
> Changes since v2:
> - change prefix in subject.
> 
> Changes since v4:
> - fix off-by-one bug.
> 
> Changes since v5:
> - avoids potential buffer underflow if nr_pages is 0 calling cache_alloc.
> ---
>  tools/libs/call/buffer.c  | 31 ++++++++++++++++++++-----------
>  tools/libs/call/core.c    |  3 ++-
>  tools/libs/call/private.h |  8 +++++---
>  3 files changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/libs/call/buffer.c b/tools/libs/call/buffer.c
> index 155e4f9d43..2f0515c273 100644
> --- a/tools/libs/call/buffer.c
> +++ b/tools/libs/call/buffer.c
> @@ -49,6 +49,9 @@ static void *cache_alloc(xencall_handle *xcall, size_t nr_pages)
>  {
>      void *p = NULL;
>  
> +    if ( nr_pages == 0 )
> +        return NULL;

By doing that check here, we don't update the stat anymore. And it's
getting out-of-sync with the updates done in cache_free().

Before, we where returning a cache entry for that, and cache_hit++. I
think it's ok to return cache_miss++ instead.

>      cache_lock(xcall);
>  
>      xcall->buffer_total_allocations++;
> @@ -56,13 +59,13 @@ static void *cache_alloc(xencall_handle *xcall, size_t nr_pages)
>      if ( xcall->buffer_current_allocations > xcall->buffer_maximum_allocations )
>          xcall->buffer_maximum_allocations = xcall->buffer_current_allocations;
>  
> -    if ( nr_pages > 1 )
> +    if ( nr_pages > ARRAY_SIZE(xcall->buffer_cache) )
>      {
>          xcall->buffer_cache_toobig++;
>      }
> -    else if ( xcall->buffer_cache_nr > 0 )
> +    else if ( xcall->buffer_cache_nr[nr_pages-1] > 0 )
>      {
> -        p = xcall->buffer_cache[--xcall->buffer_cache_nr];
> +        p = xcall->buffer_cache[nr_pages-1][--xcall->buffer_cache_nr[nr_pages-1]];
>          xcall->buffer_cache_hits++;
>      }
>      else
> @@ -84,10 +87,10 @@ static int cache_free(xencall_handle *xcall, void *p, size_t nr_pages)
>      xcall->buffer_total_releases++;
>      xcall->buffer_current_allocations--;
>  
> -    if ( nr_pages == 1 &&
> -         xcall->buffer_cache_nr < BUFFER_CACHE_SIZE )
> +    if ( nr_pages && nr_pages <= ARRAY_SIZE(xcall->buffer_cache) &&
> +         xcall->buffer_cache_nr[nr_pages-1] < BUFFER_CACHE_SIZE )
>      {
> -        xcall->buffer_cache[xcall->buffer_cache_nr++] = p;
> +        xcall->buffer_cache[nr_pages-1][xcall->buffer_cache_nr[nr_pages-1]++] = p;
>          rc = 1;
>      }
>  
> @@ -108,17 +111,23 @@ void buffer_release_cache(xencall_handle *xcall)
>      DBGPRINTF("current allocations:%d maximum allocations:%d",
>                xcall->buffer_current_allocations,
>                xcall->buffer_maximum_allocations);
> -    DBGPRINTF("cache current size:%d",
> -              xcall->buffer_cache_nr);
> +    for ( unsigned i = 0; i < ARRAY_SIZE(xcall->buffer_cache_nr); ++i )
> +    {
> +        DBGPRINTF("cache current size[%u pages]:%d", i+1,
> +                xcall->buffer_cache_nr[i]);
> +    }
>      DBGPRINTF("cache hits:%d misses:%d toobig:%d",
>                xcall->buffer_cache_hits,
>                xcall->buffer_cache_misses,
>                xcall->buffer_cache_toobig);
>  
> -    while ( xcall->buffer_cache_nr > 0 )
> +    for ( unsigned i = 0; i < ARRAY_SIZE(xcall->buffer_cache_nr); ++i )
>      {
> -        p = xcall->buffer_cache[--xcall->buffer_cache_nr];
> -        osdep_free_pages(xcall, p, 1);
> +        while ( xcall->buffer_cache_nr[i] > 0 )
> +        {
> +            p = xcall->buffer_cache[i][--xcall->buffer_cache_nr[i]];
> +            osdep_free_pages(xcall, p, i + 1);
> +        }
>      }
>  
>      cache_unlock(xcall);
> diff --git a/tools/libs/call/core.c b/tools/libs/call/core.c
> index 02c4f8e1ae..dd8877c1a0 100644
> --- a/tools/libs/call/core.c
> +++ b/tools/libs/call/core.c
> @@ -14,6 +14,7 @@
>   */
>  
>  #include <stdlib.h>
> +#include <string.h>
>  
>  #include "private.h"
>  
> @@ -44,7 +45,7 @@ xencall_handle *xencall_open(xentoollog_logger *logger, unsigned open_flags)
>      xentoolcore__register_active_handle(&xcall->tc_ah);
>  
>      xcall->flags = open_flags;
> -    xcall->buffer_cache_nr = 0;
> +    memset(xcall->buffer_cache_nr, 0, sizeof(xcall->buffer_cache_nr));
>  
>      xcall->buffer_total_allocations = 0;
>      xcall->buffer_total_releases = 0;
> diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
> index 9c3aa432ef..8e6a208975 100644
> --- a/tools/libs/call/private.h
> +++ b/tools/libs/call/private.h
> @@ -31,13 +31,15 @@ struct xencall_handle {
>      Xentoolcore__Active_Handle tc_ah;
>  
>      /*
> -     * A simple cache of unused, single page, hypercall buffers
> +     * A simple cache of unused, small, hypercall buffers
> +     * buffer_cache[i]'s size is (i+1) pages
>       *
>       * Protected by a global lock.
>       */
>  #define BUFFER_CACHE_SIZE 4
> -    int buffer_cache_nr;
> -    void *buffer_cache[BUFFER_CACHE_SIZE];
> +#define BUFFER_CACHE_NRPAGES 4
> +    int buffer_cache_nr[BUFFER_CACHE_NRPAGES];
> +    void *buffer_cache[BUFFER_CACHE_NRPAGES][BUFFER_CACHE_SIZE];
>  
>      /*
>       * Hypercall buffer statistics. All protected by the global
> -- 
> 2.43.0
> 
> 


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v6 06/16] libs/call: cache up to 4 pages in hypercall bounce buffers

[Anthony PERARD]

[-- Attachment #1: Type: text/plain, Size: 1883 bytes --]

On Tue, Jul 07, 2026 at 03:51:40PM +0200, Anthony PERARD wrote:
> On Fri, Jun 19, 2026 at 02:04:51PM +0100, Frediano Ziglio wrote:
> > From: Edwin Török <edwin.torok@citrix.com>
> > 
> > During migration there are a lot of mmap/munmap calls,
> > because `xc_get_pfn_type_batch` exceeds the default hypercall bounce
> > buffer cache size, and needs to allocate every time it is called.
> 
> I think xc_get_pfn_type_batch() would allocate a buffer of 2 page top,
> in write_batch(), right ?

(because nr_pfns <= MAX_BATCH_SIZE, and we allocate
nr_pfns*sizeof(unsigned  long)

> > `munmap` is slow, especially in a PV Dom0 (takes an emulation fault),
> > so is best avoided.
> > 
> > Eventually it'd be good if the memory pool from  xmalloc_tlsf.c
> > was reused here, but for now make it handle the commonly encountered
> > sizes (so far up to 4 pages).

So do you know what would allocate 4 pages?

In anycase, I guess it's ok to keep an allocation of 160kb
for a short while.

> > diff --git a/tools/libs/call/buffer.c b/tools/libs/call/buffer.c
> > index 155e4f9d43..2f0515c273 100644
> > --- a/tools/libs/call/buffer.c
> > +++ b/tools/libs/call/buffer.c
> > @@ -49,6 +49,9 @@ static void *cache_alloc(xencall_handle *xcall, size_t nr_pages)
> >  {
> >      void *p = NULL;
> >  
> > +    if ( nr_pages == 0 )
> > +        return NULL;
> 
> By doing that check here, we don't update the stat anymore. And it's
> getting out-of-sync with the updates done in cache_free().
> 
> Before, we where returning a cache entry for that, and cache_hit++. I
> think it's ok to return cache_miss++ instead.
> 

The rest of the patch looks fine to me, and I guess is ok.

(and I send the previous mail a bit too soon)

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v6 06/16] libs/call: cache up to 4 pages in hypercall bounce buffers

[Frediano Ziglio]

On Tue, 7 Jul 2026 at 14:51, Anthony PERARD <anthony.perard@vates.tech> wrote:
>
> On Fri, Jun 19, 2026 at 02:04:51PM +0100, Frediano Ziglio wrote:
> > From: Edwin Török <edwin.torok@citrix.com>
> >
> > During migration there are a lot of mmap/munmap calls,
> > because `xc_get_pfn_type_batch` exceeds the default hypercall bounce
> > buffer cache size, and needs to allocate every time it is called.
>
> I think xc_get_pfn_type_batch() would allocate a buffer of 2 page top,
> in write_batch(), right ?
>

Yes. That however does not contradict the sense of the sentence (or
even the commit message).

> >
> > `munmap` is slow, especially in a PV Dom0 (takes an emulation fault),
> > so is best avoided.
> >
> > Eventually it'd be good if the memory pool from  xmalloc_tlsf.c
> > was reused here, but for now make it handle the commonly encountered
> > sizes (so far up to 4 pages).
> >

If a program uses 3/4 pages it will use the additional cache, if not
there's no much difference.

> > Signed-off-by: Edwin Török <edwin.torok@citrix.com>
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
> > --
> > Changes since v2:
> > - change prefix in subject.
> >
> > Changes since v4:
> > - fix off-by-one bug.
> >
> > Changes since v5:
> > - avoids potential buffer underflow if nr_pages is 0 calling cache_alloc.
> > ---
> >  tools/libs/call/buffer.c  | 31 ++++++++++++++++++++-----------
> >  tools/libs/call/core.c    |  3 ++-
> >  tools/libs/call/private.h |  8 +++++---
> >  3 files changed, 27 insertions(+), 15 deletions(-)
> >
> > diff --git a/tools/libs/call/buffer.c b/tools/libs/call/buffer.c
> > index 155e4f9d43..2f0515c273 100644
> > --- a/tools/libs/call/buffer.c
> > +++ b/tools/libs/call/buffer.c
> > @@ -49,6 +49,9 @@ static void *cache_alloc(xencall_handle *xcall, size_t nr_pages)
> >  {
> >      void *p = NULL;
> >
> > +    if ( nr_pages == 0 )
> > +        return NULL;
>
> By doing that check here, we don't update the stat anymore. And it's
> getting out-of-sync with the updates done in cache_free().
>
> Before, we where returning a cache entry for that, and cache_hit++. I
> think it's ok to return cache_miss++ instead.
>

Well... requesting 0 pages is weird by definition, even malloc(0) is
not well defined.
In theory in this case returning NULL would cause cache_free to not be
called as filtered by xencall_free_buffer_pages.

I think the most symmetric think would be adding a similar test in
cache_free, like

static int cache_free(xencall_handle *xcall, void *p, size_t nr_pages)
{
    int rc = 0;

    if ( nr_pages == 0 )
        return 1;

    cache_lock(xcall);


(the return 1 is needed to prevent the attempt to munmap the pointer
which does not make sense).

> >      cache_lock(xcall);
> >
> >      xcall->buffer_total_allocations++;
> > @@ -56,13 +59,13 @@ static void *cache_alloc(xencall_handle *xcall, size_t nr_pages)
> >      if ( xcall->buffer_current_allocations > xcall->buffer_maximum_allocations )
> >          xcall->buffer_maximum_allocations = xcall->buffer_current_allocations;
> >
> > -    if ( nr_pages > 1 )
> > +    if ( nr_pages > ARRAY_SIZE(xcall->buffer_cache) )
> >      {
> >          xcall->buffer_cache_toobig++;
> >      }
> > -    else if ( xcall->buffer_cache_nr > 0 )
> > +    else if ( xcall->buffer_cache_nr[nr_pages-1] > 0 )
> >      {
> > -        p = xcall->buffer_cache[--xcall->buffer_cache_nr];
> > +        p = xcall->buffer_cache[nr_pages-1][--xcall->buffer_cache_nr[nr_pages-1]];
> >          xcall->buffer_cache_hits++;
> >      }
> >      else
> > @@ -84,10 +87,10 @@ static int cache_free(xencall_handle *xcall, void *p, size_t nr_pages)
> >      xcall->buffer_total_releases++;
> >      xcall->buffer_current_allocations--;
> >
> > -    if ( nr_pages == 1 &&
> > -         xcall->buffer_cache_nr < BUFFER_CACHE_SIZE )
> > +    if ( nr_pages && nr_pages <= ARRAY_SIZE(xcall->buffer_cache) &&
> > +         xcall->buffer_cache_nr[nr_pages-1] < BUFFER_CACHE_SIZE )
> >      {
> > -        xcall->buffer_cache[xcall->buffer_cache_nr++] = p;
> > +        xcall->buffer_cache[nr_pages-1][xcall->buffer_cache_nr[nr_pages-1]++] = p;
> >          rc = 1;
> >      }
> >
> > @@ -108,17 +111,23 @@ void buffer_release_cache(xencall_handle *xcall)
> >      DBGPRINTF("current allocations:%d maximum allocations:%d",
> >                xcall->buffer_current_allocations,
> >                xcall->buffer_maximum_allocations);
> > -    DBGPRINTF("cache current size:%d",
> > -              xcall->buffer_cache_nr);
> > +    for ( unsigned i = 0; i < ARRAY_SIZE(xcall->buffer_cache_nr); ++i )
> > +    {
> > +        DBGPRINTF("cache current size[%u pages]:%d", i+1,
> > +                xcall->buffer_cache_nr[i]);
> > +    }
> >      DBGPRINTF("cache hits:%d misses:%d toobig:%d",
> >                xcall->buffer_cache_hits,
> >                xcall->buffer_cache_misses,
> >                xcall->buffer_cache_toobig);
> >
> > -    while ( xcall->buffer_cache_nr > 0 )
> > +    for ( unsigned i = 0; i < ARRAY_SIZE(xcall->buffer_cache_nr); ++i )
> >      {
> > -        p = xcall->buffer_cache[--xcall->buffer_cache_nr];
> > -        osdep_free_pages(xcall, p, 1);
> > +        while ( xcall->buffer_cache_nr[i] > 0 )
> > +        {
> > +            p = xcall->buffer_cache[i][--xcall->buffer_cache_nr[i]];
> > +            osdep_free_pages(xcall, p, i + 1);
> > +        }
> >      }
> >
> >      cache_unlock(xcall);
> > diff --git a/tools/libs/call/core.c b/tools/libs/call/core.c
> > index 02c4f8e1ae..dd8877c1a0 100644
> > --- a/tools/libs/call/core.c
> > +++ b/tools/libs/call/core.c
> > @@ -14,6 +14,7 @@
> >   */
> >
> >  #include <stdlib.h>
> > +#include <string.h>
> >
> >  #include "private.h"
> >
> > @@ -44,7 +45,7 @@ xencall_handle *xencall_open(xentoollog_logger *logger, unsigned open_flags)
> >      xentoolcore__register_active_handle(&xcall->tc_ah);
> >
> >      xcall->flags = open_flags;
> > -    xcall->buffer_cache_nr = 0;
> > +    memset(xcall->buffer_cache_nr, 0, sizeof(xcall->buffer_cache_nr));
> >
> >      xcall->buffer_total_allocations = 0;
> >      xcall->buffer_total_releases = 0;
> > diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
> > index 9c3aa432ef..8e6a208975 100644
> > --- a/tools/libs/call/private.h
> > +++ b/tools/libs/call/private.h
> > @@ -31,13 +31,15 @@ struct xencall_handle {
> >      Xentoolcore__Active_Handle tc_ah;
> >
> >      /*
> > -     * A simple cache of unused, single page, hypercall buffers
> > +     * A simple cache of unused, small, hypercall buffers
> > +     * buffer_cache[i]'s size is (i+1) pages
> >       *
> >       * Protected by a global lock.
> >       */
> >  #define BUFFER_CACHE_SIZE 4
> > -    int buffer_cache_nr;
> > -    void *buffer_cache[BUFFER_CACHE_SIZE];
> > +#define BUFFER_CACHE_NRPAGES 4
> > +    int buffer_cache_nr[BUFFER_CACHE_NRPAGES];
> > +    void *buffer_cache[BUFFER_CACHE_NRPAGES][BUFFER_CACHE_SIZE];
> >
> >      /*
> >       * Hypercall buffer statistics. All protected by the global

Frediano

Re: [PATCH v6 01/16] libs/guest: Reduce number of parts in write_split_record

[Anthony PERARD]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

On Fri, Jun 19, 2026 at 02:04:46PM +0100, Frediano Ziglio wrote:
> Small optimization.
> There's no much sense to split the header in 2 pieces, it will
> just take more time and space to reassemble them in the final
> buffer.
> This also avoids truncating combined_length to 32 bit in case of
> 64 bit machines potentially avoiding following record_length check
> (it could still be truncated writing it in xc_sr_rhdr structure
> but the following check will catch it).
> The function become more coherent with following read_record
> function.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v6 1.9/16] libs/guest: Allocate rec_pfns earlier in write_batch()

[Anthony PERARD]

[-- Attachment #1: Type: text/plain, Size: 576 bytes --]

On Wed, Jul 01, 2026 at 02:52:30PM +0100, Andrew Cooper wrote:
> For reasons which escape me, rec_pfns are allocated separately to the rest of
> the batch allocations.
> 
> Allocate them all together.  This will allow for future simplifications to be
> performed in an incremental mannor.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v6.1 02/16] libs/guest: Reduce number of iovecs in write_batch()

[Anthony PERARD]

[-- Attachment #1: Type: text/plain, Size: 689 bytes --]

On Wed, Jul 01, 2026 at 02:57:47PM +0100, Andrew Cooper wrote:
> From: Frediano Ziglio <freddy77@gmail.com>
> 
> Construct all of the headers together in one block, rather than a field at a
> time.  Initialise as many of the fields as possible at declaration time.
> 
> Start filling in iov[] earlier, to allow for future simplifications.
> 
> No practical change.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v6 03/16] libs/guest: Reduce number of I/O vectors in write_batch

[Anthony PERARD]

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

On Thu, Jul 02, 2026 at 01:33:05PM +0100, Frediano Ziglio wrote:
> On Tue, 30 Jun 2026 at 17:47, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >
> >
> > This has the same exact subject as the prior patch.
> >
> > Either it wants merging, as they're both in the same function, or the
> > subject wants to be different.  Even a "Further ..." prefix would help.
> >
> > On 19/06/2026 2:04 pm, Frediano Ziglio wrote:
> > > Each page was sent using a different iovec item. This potentially exceed
> > > Linux maximum (1024).
> >
> > Linux cannot have a maximum of 1024 because this has been working fine
> > for a decade using 1028 in the common case.
> >
> 
> But the code does not call writev or similars directly, so there's no
> limit besides the sky.
> The result with 1028 is simply that you do 2 system calls instead of one.

Could you add something along those lines to the description? And turn a
sentence saying there's a bug into a description saying it's suboptimal.

With that: Acked-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v6 04/16] libs/guest: Use a single write_exact in write_headers

[Anthony PERARD]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]

On Tue, Jun 30, 2026 at 05:47:42PM +0100, Andrew Cooper wrote:
> On 19/06/2026 2:04 pm, Frediano Ziglio wrote:
> > diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
> > index eba33f861a..8c31f9f86c 100644
> > --- a/tools/libs/guest/xg_sr_save.c
> > +++ b/tools/libs/guest/xg_sr_save.c
> > +    } hdrs = {
> > +        {
> 
> .ihdr = {
> 
> > +            .marker  = IHDR_MARKER,
> > +            .id      = htonl(IHDR_ID),
> > +            .version = htonl(3),
> > +            .options = htons(IHDR_OPT_LITTLE_ENDIAN),
> > +        },
> > +        {
> 
> .dhdr = {

With that: Acked-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v6 06/16] libs/call: cache up to 4 pages in hypercall bounce buffers

[Anthony PERARD]

[-- Attachment #1: Type: text/plain, Size: 2324 bytes --]

On Tue, Jul 07, 2026 at 03:47:07PM +0100, Frediano Ziglio wrote:
> On Tue, 7 Jul 2026 at 14:51, Anthony PERARD <anthony.perard@vates.tech> wrote:
> > On Fri, Jun 19, 2026 at 02:04:51PM +0100, Frediano Ziglio wrote:
> > > diff --git a/tools/libs/call/buffer.c b/tools/libs/call/buffer.c
> > > index 155e4f9d43..2f0515c273 100644
> > > --- a/tools/libs/call/buffer.c
> > > +++ b/tools/libs/call/buffer.c
> > > @@ -49,6 +49,9 @@ static void *cache_alloc(xencall_handle *xcall, size_t nr_pages)
> > >  {
> > >      void *p = NULL;
> > >
> > > +    if ( nr_pages == 0 )
> > > +        return NULL;
> >
> > By doing that check here, we don't update the stat anymore. And it's
> > getting out-of-sync with the updates done in cache_free().
> >
> > Before, we where returning a cache entry for that, and cache_hit++. I
> > think it's ok to return cache_miss++ instead.
> >
> 
> Well... requesting 0 pages is weird by definition, even malloc(0) is
> not well defined.

malloc(0) isn't defined as weird, it is defined as
"implementation-defined" ;-). But the pointer that the cache function
handle isn't from malloc().

> In theory in this case returning NULL would cause cache_free to not be
> called as filtered by xencall_free_buffer_pages.

Yes, for cases where the allocator returned NULL. But I can't find any
guaranty of this. So I would prefer to have both cache_alloc() and
cache_free() behave the same way when faced with nr_pages==0, without
hindsight into the value of the pointer.

> 
> I think the most symmetric think would be adding a similar test in
> cache_free, like
> 
> static int cache_free(xencall_handle *xcall, void *p, size_t nr_pages)
> {
>     int rc = 0;
> 
>     if ( nr_pages == 0 )
>         return 1;
> 
>     cache_lock(xcall);
> 
> 
> (the return 1 is needed to prevent the attempt to munmap the pointer
> which does not make sense).

If we have a pointer that is not NULL, we must free it. Even if you
think it doesn't make sense. Also, there's no way to know, here, whether
munmap() or an other function is going to be used. So, cache_free() must
not say that it cached the pointer, and let the caller free it.

Cheers,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v6 07/16] libs/guest: avoids using 2 indexes

[Anthony PERARD]

[-- Attachment #1: Type: text/plain, Size: 517 bytes --]

On Fri, Jun 19, 2026 at 02:04:52PM +0100, Frediano Ziglio wrote:
> Simplify code, after the first scan of the various arrays we don't need to
> keep original types and PFNs but only the ones having data.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v6 10/16] libs/guest: add xg_foreignmemory_copy_{from,to}

[Anthony PERARD]

[-- Attachment #1: Type: text/plain, Size: 798 bytes --]

On Fri, Jun 19, 2026 at 02:04:55PM +0100, Frediano Ziglio wrote:
> This change prepare code to use a new "foreign copy" hypercall.
> The new hypercall will copy memory from/to a foreign domain.
> The new hypercall can be emulated with a sequence of:
> - map foreign memory;
> - copy memory;
> - unmap foreign memory.

I don't understand the point of this patch. The hypercall doesn't exist
so there's nothing to emulate.

I've notice there's a patch later in the series which introduce a new
hypercall, but the changes to the library should come after the
hypercall is been introduced, only then can we check if the emulation is
correct, or even needed.

Cheers,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v6 11/16] PoC: libs/guest: use foreign copy during migration

[Anthony PERARD]

[-- Attachment #1: Type: text/plain, Size: 1389 bytes --]


A note about the subject, a "PoC" or Proof-of-concept to me isn't
a patch that can be accepted, especialy if is a patch to an existing
library.

On Fri, Jun 19, 2026 at 02:04:56PM +0100, Frediano Ziglio wrote:
> From: Edwin Török <edwin.torok@citrix.com>
> 
> ministat confirms the improvement:
> 
> ```
> x baseline
> + foreigncopy
>     N           Min           Max        Median           Avg        Stddev
> x  20     1.1306997     1.1447931     1.1356569     1.1365742   0.003242175
> +  20     0.4311504    0.44180303    0.43616705    0.43600089  0.0031094689
> Difference at 95.0% confidence
> 	-0.700573 +/- 0.00203311
> 	-61.639% +/- 0.133355%
> 	(Student's t, pooled s = 0.00317652)
> ```

There's been some comment about this stat in previous version of the
series, and the description is still the same. Could you describe how
the stat have been generated, and what the number mean?

Also, what's the different between "baseline" and "foreigncopy". I've
only had a glimpse at this patch, and it just looks like the code have
been moved to a different part of the library, with somehow less lines
of code.

> 
> The tests pass too, which means that it has correctly migrated all guest
> memory.

Which tests?

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v6 13/16] privcmd: Add definition for new Linux privcmd to access new Xen hypercall

[Anthony PERARD]

[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]

On Fri, Jun 19, 2026 at 02:04:58PM +0100, Frediano Ziglio wrote:
> diff --git a/tools/include/xen-sys/Linux/privcmd.h b/tools/include/xen-sys/Linux/privcmd.h
> index 607dfa2287..7a3c41308b 100644
> --- a/tools/include/xen-sys/Linux/privcmd.h
> +++ b/tools/include/xen-sys/Linux/privcmd.h
> @@ -100,6 +100,14 @@ typedef struct privcmd_pcidev_get_gsi {
>  	__u32 gsi;
>  } privcmd_pcidev_get_gsi_t;
>  
> +typedef struct privcmd_foreigncopy {
> +	domid_t dom;          /* Foreign domain. */
> +	__u16 dir;            /* Direction,  0 from, 1 to. */
> +	__u32 num;            /* Number of pages to copy. */
> +	const xen_pfn_t __user *pfns; /* Array of pfns. */
> +	void __user *buffer;  /* Buffer to copy to/from. */
> +} privcmd_foreigncopy_t;
> +
>  /*
>   * @cmd: IOCTL_PRIVCMD_HYPERCALL
>   * @arg: &privcmd_hypercall_t
> @@ -121,6 +129,8 @@ typedef struct privcmd_pcidev_get_gsi {
>  	_IOC(_IOC_NONE, 'P', 7, sizeof(privcmd_mmap_resource_t))
>  #define IOCTL_PRIVCMD_PCIDEV_GET_GSI			\
>  	_IOC(_IOC_NONE, 'P', 10, sizeof(privcmd_pcidev_get_gsi_t))
> +#define IOCTL_PRIVCMD_FOREIGNCOPY				\
> +	_IOWR('P', 11, privcmd_foreigncopy_t)
>  #define IOCTL_PRIVCMD_UNIMPLEMENTED				\
>  	_IOC(_IOC_NONE, 'P', 0xFF, 0)
>  

I don't think I can accept this patch until the changes have been added
to Linux. Also, I think it would be fine to squash this
changes into the patch that start using this new hypercall.


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v6 15/16] libs/guest: finalize PoC

[Anthony PERARD]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

A note about the subject: When taken out of the context of this patch
series (so once commited), we don't know what "PoC" is refering to. In
"libs/guest: finalize PoC", it looks like "libxenguest" was the PoC, but
it isn't.

It feels like this patch wants to be merged into that other PoC patch,
and have a patch description completely rewritten to have something that
doesn't looks like the description of an experiment.

Cheers,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v6 06/16] libs/call: cache up to 4 pages in hypercall bounce buffers

[Frediano Ziglio]

On Wed, 8 Jul 2026 at 14:19, Anthony PERARD <anthony.perard@vates.tech> wrote:
>
> On Tue, Jul 07, 2026 at 03:47:07PM +0100, Frediano Ziglio wrote:
> > On Tue, 7 Jul 2026 at 14:51, Anthony PERARD <anthony.perard@vates.tech> wrote:
> > > On Fri, Jun 19, 2026 at 02:04:51PM +0100, Frediano Ziglio wrote:
> > > > diff --git a/tools/libs/call/buffer.c b/tools/libs/call/buffer.c
> > > > index 155e4f9d43..2f0515c273 100644
> > > > --- a/tools/libs/call/buffer.c
> > > > +++ b/tools/libs/call/buffer.c
> > > > @@ -49,6 +49,9 @@ static void *cache_alloc(xencall_handle *xcall, size_t nr_pages)
> > > >  {
> > > >      void *p = NULL;
> > > >
> > > > +    if ( nr_pages == 0 )
> > > > +        return NULL;
> > >
> > > By doing that check here, we don't update the stat anymore. And it's
> > > getting out-of-sync with the updates done in cache_free().
> > >
> > > Before, we where returning a cache entry for that, and cache_hit++. I
> > > think it's ok to return cache_miss++ instead.
> > >
> >
> > Well... requesting 0 pages is weird by definition, even malloc(0) is
> > not well defined.
>
> malloc(0) isn't defined as weird, it is defined as
> "implementation-defined" ;-). But the pointer that the cache function
> handle isn't from malloc().
>
> > In theory in this case returning NULL would cause cache_free to not be
> > called as filtered by xencall_free_buffer_pages.
>
> Yes, for cases where the allocator returned NULL. But I can't find any
> guaranty of this. So I would prefer to have both cache_alloc() and
> cache_free() behave the same way when faced with nr_pages==0, without
> hindsight into the value of the pointer.
>
> >
> > I think the most symmetric think would be adding a similar test in
> > cache_free, like
> >
> > static int cache_free(xencall_handle *xcall, void *p, size_t nr_pages)
> > {
> >     int rc = 0;
> >
> >     if ( nr_pages == 0 )
> >         return 1;
> >
> >     cache_lock(xcall);
> >
> >
> > (the return 1 is needed to prevent the attempt to munmap the pointer
> > which does not make sense).
>
> If we have a pointer that is not NULL, we must free it. Even if you
> think it doesn't make sense. Also, there's no way to know, here, whether
> munmap() or an other function is going to be used. So, cache_free() must
> not say that it cached the pointer, and let the caller free it.
>

Changed to return 0, NULL pointer is handled by the caller anyway.

> Cheers,
>
>
> --
> Anthony Perard | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech

Re: [PATCH v6 11/16] PoC: libs/guest: use foreign copy during migration

[Frediano Ziglio]

On Wed, 8 Jul 2026 at 14:55, Anthony PERARD <anthony.perard@vates.tech> wrote:
>
>
> A note about the subject, a "PoC" or Proof-of-concept to me isn't
> a patch that can be accepted, especialy if is a patch to an existing
> library.
>

Yes, mainly it should be merged with the final "finalize PoC".
Still nice for review at the moment.

> On Fri, Jun 19, 2026 at 02:04:56PM +0100, Frediano Ziglio wrote:
> > From: Edwin Török <edwin.torok@citrix.com>
> >
> > ministat confirms the improvement:
> >
> > ```
> > x baseline
> > + foreigncopy
> >     N           Min           Max        Median           Avg        Stddev
> > x  20     1.1306997     1.1447931     1.1356569     1.1365742   0.003242175
> > +  20     0.4311504    0.44180303    0.43616705    0.43600089  0.0031094689
> > Difference at 95.0% confidence
> >       -0.700573 +/- 0.00203311
> >       -61.639% +/- 0.133355%
> >       (Student's t, pooled s = 0.00317652)
> > ```
>
> There's been some comment about this stat in previous version of the
> series, and the description is still the same. Could you describe how
> the stat have been generated, and what the number mean?
>

We managed to ask the author but got not much clue. Mainly timing.
Probably better to do new statistics.

> Also, what's the different between "baseline" and "foreigncopy". I've
> only had a glimpse at this patch, and it just looks like the code have
> been moved to a different part of the library, with somehow less lines
> of code.
>

"baseline" I suppose without these changes, "foreigncopy" will the changes.

> >
> > The tests pass too, which means that it has correctly migrated all guest
> > memory.
>
> Which tests?
>

Different migrations with multiple OSes and configuration (for
instance PV and not PV, these from me).

> Thanks,
>
>

Frediano

Re: [PATCH v6 13/16] privcmd: Add definition for new Linux privcmd to access new Xen hypercall

[Frediano Ziglio]

On Wed, 8 Jul 2026 at 14:59, Anthony PERARD <anthony.perard@vates.tech> wrote:
>
> On Fri, Jun 19, 2026 at 02:04:58PM +0100, Frediano Ziglio wrote:
> > diff --git a/tools/include/xen-sys/Linux/privcmd.h b/tools/include/xen-sys/Linux/privcmd.h
> > index 607dfa2287..7a3c41308b 100644
> > --- a/tools/include/xen-sys/Linux/privcmd.h
> > +++ b/tools/include/xen-sys/Linux/privcmd.h
> > @@ -100,6 +100,14 @@ typedef struct privcmd_pcidev_get_gsi {
> >       __u32 gsi;
> >  } privcmd_pcidev_get_gsi_t;
> >
> > +typedef struct privcmd_foreigncopy {
> > +     domid_t dom;          /* Foreign domain. */
> > +     __u16 dir;            /* Direction,  0 from, 1 to. */
> > +     __u32 num;            /* Number of pages to copy. */
> > +     const xen_pfn_t __user *pfns; /* Array of pfns. */
> > +     void __user *buffer;  /* Buffer to copy to/from. */
> > +} privcmd_foreigncopy_t;
> > +
> >  /*
> >   * @cmd: IOCTL_PRIVCMD_HYPERCALL
> >   * @arg: &privcmd_hypercall_t
> > @@ -121,6 +129,8 @@ typedef struct privcmd_pcidev_get_gsi {
> >       _IOC(_IOC_NONE, 'P', 7, sizeof(privcmd_mmap_resource_t))
> >  #define IOCTL_PRIVCMD_PCIDEV_GET_GSI                 \
> >       _IOC(_IOC_NONE, 'P', 10, sizeof(privcmd_pcidev_get_gsi_t))
> > +#define IOCTL_PRIVCMD_FOREIGNCOPY                            \
> > +     _IOWR('P', 11, privcmd_foreigncopy_t)
> >  #define IOCTL_PRIVCMD_UNIMPLEMENTED                          \
> >       _IOC(_IOC_NONE, 'P', 0xFF, 0)
> >
>
> I don't think I can accept this patch until the changes have been added
> to Linux. Also, I think it would be fine to squash this
> changes into the patch that start using this new hypercall.
>

The last patch is for Linux, but I saw no comments on it.
For the merge, yes, it can be done, not a big deal.

Frediano

Re: [PATCH v6 15/16] libs/guest: finalize PoC

[Frediano Ziglio]

On Wed, 8 Jul 2026 at 15:12, Anthony PERARD <anthony.perard@vates.tech> wrote:
>
> A note about the subject: When taken out of the context of this patch
> series (so once commited), we don't know what "PoC" is refering to. In
> "libs/guest: finalize PoC", it looks like "libxenguest" was the PoC, but
> it isn't.

It's referring to the previous "PoC: libs/guest: use foreign copy
during migration" commit.

> It feels like this patch wants to be merged into that other PoC patch,
> and have a patch description completely rewritten to have something that
> doesn't looks like the description of an experiment.
>

Definitively.

> Cheers,
>
>

Frediano

Re: [PATCH v6 10/16] libs/guest: add xg_foreignmemory_copy_{from,to}

[Frediano Ziglio]

On Wed, 8 Jul 2026 at 14:32, Anthony PERARD <anthony.perard@vates.tech> wrote:
>
> On Fri, Jun 19, 2026 at 02:04:55PM +0100, Frediano Ziglio wrote:
> > This change prepare code to use a new "foreign copy" hypercall.
> > The new hypercall will copy memory from/to a foreign domain.
> > The new hypercall can be emulated with a sequence of:
> > - map foreign memory;
> > - copy memory;
> > - unmap foreign memory.
>
> I don't understand the point of this patch. The hypercall doesn't exist
> so there's nothing to emulate.

I think that imitating something else is the definition of "emulate",
we know what the hypercall should do so we emulate the wanted
behavior.

> I've notice there's a patch later in the series which introduce a new
> hypercall, but the changes to the library should come after the
> hypercall is been introduced, only then can we check if the emulation is
> correct, or even needed.

There are other changes after the hypercall.

In this case the new hypercall is to replace something that is already
there. The base idea is that the new hypercall is able to do 3 steps
together.
The reason to introduce the emulation first is that you can refactor
on the emulation without having to introduce the new hypercall.
Introducing the hypercall first would make testing more complicated as
bugs on the hypercall have to be taken into account and considered.
Also it is easier that way to enable or disable new code. For instance
you want to test for performance regression (in this case the code
emulated should not perform worse).

In the beginning the PoC was much more hacky and it was more similar
to the idea you have probably in mind. But a big part of the "hack"
was removing code, in particular the entire support for PV and the
verification code. Obviously that hacks could not be accepted so
instead I decided to change the code in a more incremental way not
removing things but instead changing to make it easier to use the
future hypercall. So I was able to test all cases (like PV)
incrementally and keep it working.

You are however the second person (after Andrew) to ask this, so a big
comment (probably in the commit message) is due.

>
> Cheers,
>

Frediano

Re: [PATCH Linux v6 16/16] xen/privcmd: Add new ABI to allow copying foreign memory

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 379 bytes --]

On 19.06.26 15:05, Frediano Ziglio wrote:
> This new ABI allows to copy foreign domain memory to/from a buffer.
> This avoids having to map/copy/unmap foreign memory which is
> expensive.
> This operation is done particularly when migrating VMs.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH Linux v6 16/16] xen/privcmd: Add new ABI to allow copying foreign memory

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1454 bytes --]

On 19.06.26 15:05, Frediano Ziglio wrote:
> This new ABI allows to copy foreign domain memory to/from a buffer.
> This avoids having to map/copy/unmap foreign memory which is
> expensive.
> This operation is done particularly when migrating VMs.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>

While doing a test build I got the following build failures for 32-bit arm:

   CC      arch/arm/xen/enlighten.o
In file included from /home/gross/korg/src/arch/arm/include/asm/xen/interface.h:1,
                  from /home/gross/korg/src/include/xen/interface/xen.h:13,
                  from /home/gross/korg/src/include/xen/xen.h:52,
                  from /home/gross/korg/src/arch/arm/xen/enlighten.c:2:
/home/gross/korg/src/include/xen/arm/interface.h:22:35: error: unknown type name 
'__guest_handle_uint8_t'
    22 | #define GUEST_HANDLE(name)        __guest_handle_ ## name
       |                                   ^~~~~~~~~~~~~~~
/home/gross/korg/src/include/xen/interface/memory.h:361:5: note: in expansion of 
macro 'GUEST_HANDLE'
   361 |     GUEST_HANDLE(uint8_t) buffer;
       |     ^~~~~~~~~~~~
make[5]: *** [/home/gross/korg/src/scripts/Makefile.build:289: 
arch/arm/xen/enlighten.o] Error 1
make[4]: *** [/home/gross/korg/src/scripts/Makefile.build:549: arch/arm/xen] Error 2
make[3]: *** [/home/gross/korg/src/scripts/Makefile.build:549: arch/arm] Error 2


Please fix those.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 491 bytes --]

Re: [PATCH Linux v6 16/16] xen/privcmd: Add new ABI to allow copying foreign memory

[Frediano Ziglio]

On Mon, 3 Aug 2026 at 15:05, Juergen Gross <jgross@suse.com> wrote:
>
> On 19.06.26 15:05, Frediano Ziglio wrote:
> > This new ABI allows to copy foreign domain memory to/from a buffer.
> > This avoids having to map/copy/unmap foreign memory which is
> > expensive.
> > This operation is done particularly when migrating VMs.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
>
> While doing a test build I got the following build failures for 32-bit arm:
>
>    CC      arch/arm/xen/enlighten.o
> In file included from /home/gross/korg/src/arch/arm/include/asm/xen/interface.h:1,
>                   from /home/gross/korg/src/include/xen/interface/xen.h:13,
>                   from /home/gross/korg/src/include/xen/xen.h:52,
>                   from /home/gross/korg/src/arch/arm/xen/enlighten.c:2:
> /home/gross/korg/src/include/xen/arm/interface.h:22:35: error: unknown type name
> '__guest_handle_uint8_t'
>     22 | #define GUEST_HANDLE(name)        __guest_handle_ ## name
>        |                                   ^~~~~~~~~~~~~~~
> /home/gross/korg/src/include/xen/interface/memory.h:361:5: note: in expansion of
> macro 'GUEST_HANDLE'
>    361 |     GUEST_HANDLE(uint8_t) buffer;
>        |     ^~~~~~~~~~~~
> make[5]: *** [/home/gross/korg/src/scripts/Makefile.build:289:
> arch/arm/xen/enlighten.o] Error 1
> make[4]: *** [/home/gross/korg/src/scripts/Makefile.build:549: arch/arm/xen] Error 2
> make[3]: *** [/home/gross/korg/src/scripts/Makefile.build:549: arch/arm] Error 2
>
>
> Please fix those.
>
>
> Juergen

Mumble...

I suppose this would fix it

diff --git a/include/xen/arm/interface.h b/include/xen/arm/interface.h
index c3eada2642aa..7e79853b188d 100644
--- a/include/xen/arm/interface.h
+++ b/include/xen/arm/interface.h
@@ -53,6 +53,7 @@ DEFINE_GUEST_HANDLE(int);
 DEFINE_GUEST_HANDLE(void);
 DEFINE_GUEST_HANDLE(uint64_t);
 DEFINE_GUEST_HANDLE(uint32_t);
+DEFINE_GUEST_HANDLE(uint8_t);
 DEFINE_GUEST_HANDLE(xen_pfn_t);
 DEFINE_GUEST_HANDLE(xen_ulong_t);

Frediano

Re: [PATCH v6 12/16] xen: implement new foreign copy hypercall

[Frediano Ziglio]

On Mon, 29 Jun 2026 at 07:59, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 26.06.2026 16:14, Frediano Ziglio wrote:
> > On Wed, 24 Jun 2026 at 07:44, Jan Beulich <jbeulich@suse.com> wrote:
> >> On 23.06.2026 23:18, Frediano Ziglio wrote:
> >>> On Tue, 23 Jun 2026 at 14:21, Jan Beulich <jbeulich@suse.com> wrote:
> >>>> On 23.06.2026 12:55, Frediano Ziglio wrote:
> >>>>> On Mon, 22 Jun 2026 at 11:34, Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>> On 19.06.2026 15:04, Frediano Ziglio wrote:
> >>>>>>> --- a/xen/common/memory.c
> >>>>>>> +++ b/xen/common/memory.c
> >>>>>>> @@ -1545,6 +1545,139 @@ static int acquire_resource(
> >>>>>>>      return rc;
> >>>>>>>  }
> >>>>>>>
> >>>>>>> +/*
> >>>>>>> + * The "noinline" qualifier avoids the compiler to create a large function
> >>>>>>> + * consuming quite a lot of stack.
> >>>>>>> + */
> >>>>>>> +static int noinline mem_foreigncopy(
> >>>>>>> +    XEN_GUEST_HANDLE_PARAM(xen_foreigncopy_t) arg)
> >>>>>>> +{
> >>>>>>> +    struct domain *d, *const currd = current->domain;
> >>>>>>> +    xen_foreigncopy_t copy;
> >>>>>>> +    int rc, direction;
> >>>>>>> +
> >>>>>>> +    if ( copy_from_guest(&copy, arg, 1) )
> >>>>>>> +        return -EFAULT;
> >>>>>>> +
> >>>>>>> +    if ( copy.flags & ~XENMEM_foreigncopy_direction )
> >>>>>>> +        return -EINVAL;
> >>>>>>> +
> >>>>>>> +    direction = copy.flags & XENMEM_foreigncopy_direction;
> >>>>>>> +
> >>>>>>> +    rc = rcu_lock_remote_domain_by_id(copy.domid, &d);
> >>>>>>
> >>>>>> Iirc I did ask before why this isn't ..._by_any_id().
> >>>>>
> >>>>> I probably was confused by the question about MMUEXT and the 2 domains.
> >>>>> There are different similar hypercalls (like the mentioned MMUEXT but
> >>>>> also hypercalls to map foreign domain memory) that have this check
> >>>>> (not the same domain). Any domain has, obviously, access to its own
> >>>>> memory, so it should not have to use hypercall to access its own
> >>>>> memory. If it does it looks like a mistake causing performance issues
> >>>>> or an attempt to circumvent security; in either case you would like to
> >>>>> avoid it.
> >>>>
> >>>> No. Self-grants are possible as well, for example, and for a good reason.
> >>>> Allowing normally-remote operations on oneself helps with testing, for
> >>>> example. It may also help avoid needing to special-case "self" in code
> >>>> which needs to cover both cases.
> >>>
> >>> But this is not a grant, it's a copy.
> >>
> >> Sure, but the underlying principle is what matters. Plus you don't prevent
> >> self-copy by using ..._by_id(), you only preclude the use of DOMID_SELF.
> >
> > Sure about this?
>
> No, I'm sorry: I (repeatedly) managed to ignore the "remote" in the function
> called. That said, my request stands: No arbitrary restrictions please. If
> you can properly justify a restriction, that's a different thing.
>

Not strong about it.
I'll change to rcu_lock_domain_by_any_id.

> >>>>>>> +    XEN_GUEST_HANDLE(uint8) buffer;
> >>>>>>> +};
> >>>>>>
> >>>>>> What was (again) left unaddressed is the question towards using GFNs on both
> >>>>>> sides of the copy. This would eliminate the need for the flags field, taken
> >>>>>> by a 2nd domid_t one then.
> >>>>>>
> >>>>>
> >>>>> This was addressed in
> >>>>> https://lists.xenproject.org/archives/html/xen-devel/2026-06/msg00567.html
> >>>>
> >>>> Well, yes, but not in a satisfactory way. Back channels tell me that you
> >>>> actually got the same feedback already on internal review. Which makes it
> >>>> all the more puzzling that you insist on doing it differently. Multiple
> >>>> maintainers asking for the same thing may be an indication of something.
> >>>
> >>> Not needing to have backchannel feedback, I already wrote that a
> >>> similar approach was tried and made the code more complicated.
> >>
> >> Even if indeed so: Yet at the same time more flexible.
> >>
> >>> Both maintainers didn't comment on my replies so I assume they were
> >>> fine with it.
> >>> And you are failing to provide positive feedback.
> >>> I asked (that one internally) for examples of guest buffers provided
> >>> as frame numbers but I got no answer (or better the answer was more
> >>> "currently there are not").
> >>> Also note that the location of xen_foreigncopy_t structure is also
> >>> provided using a guest pointer.
> >>> I remember there were some discussions about ABI changes (2/3 years
> >>> ago) to address this and other issues but I cannot see much progress.
> >>
> >> And it's that (very slowly progressing effort) which made me ask. The
> >> fewer virtual addresses we bake into new sub-ops, the better for that
> >> effort. And no, that doesn't go as far as completely eliminating
> >> handles (presently representing virtual addresses) - that needs to be
> >> part of the new ABI.
> >
> > In other words, you want me to code something temporary that you
> > already know that needs to be changed.
>
> What do you mean by "temporary"? We will need to live with the present
> ABI for the foreseeable future. The new ABI's requirements haven't even
> been spelled out yet. Patches to allow use of physical addresses in
> place of virtual ones were actually turned down on the grounds of there
> not having been a write-down of all requirements.
>

Temporary in the sense that there will be new ABIs to deal with not
using virtual addresses.
The second sentence is a bit contradictory. You want me to address the
virtual address complaint but you are telling me that the change will
be turned down if I don't address everything. And this is why this is
out of scope here.

> >> To preempt the argument towards "fewer virtual addresses" not really
> >> being true when changing from handle-to-uint8 to handle-to-pfn: The
> >> former won't be able to express a buffer mapped contiguously in VA
> >> space, but discontiguous in PA space. The latter will, simply be
> >> avoiding buffer VAs in the first place (the array of frame numbers
> >> can e.g. be placed in a dedicated hypercall argument area known to be
> >> physically contiguous).
> >
> > If it's mapped continuously in VA and you pass the VA I don't
> > understand the problem. From the way I see it's more the latter that's
> > the problem.
>
> I'm talking of the future, where VAs wouldn't be used anymore. The
> buffer you use couldn't be described by a single PA, unless the caller
> took specific measures up front.
>

If you read my reply I suggested a way to avoid virtual addresses completely.

> Jan

About the P2M type check it turned out that I was wrong with the
checking. The MMAP way use MMU_UPDATE calls which do not care about
P2M type at all. Changing the code to

...
        for ( unsigned int i = 0; i < todo; i++ )
        {
            struct page_info *foreign_page;
            mfn_t foreign_mfn;
            void *foreign;
            p2m_type_t p2mt;
            p2m_query_t q = (direction == XENMEM_foreigncopy_to) ?
                            P2M_ALLOC | P2M_UNSHARE : P2M_ALLOC;

            foreign_page = get_page_from_gfn(d, gfn_list[i], &p2mt, q);

            if ( unlikely(p2m_is_paged(p2mt)) )
            {
                if ( foreign_page )
                    put_page(foreign_page);
                p2m_mem_paging_populate(d, _gfn(gfn_list[i]));
                p2mt = p2m_ram_paging_in;
                foreign_page = NULL;
            }

            if ( unlikely(!foreign_page) )
            {
                rc = -ENOENT;
                if ( p2mt != p2m_ram_paging_in )
                {
                    gdprintk(XENLOG_WARNING,
                             "Error accessing foreign gfn %" PRI_gfn "\n",
                             gfn_list[i]);
                    rc = -EINVAL;
                }
                copy.nr_frames -= i;
                guest_handle_add_offset(copy.frame_list, i);
                goto out;
            }
...

About the XSM part I have now

...
    /*
     * Check we are allowed to map and access these foreign pages.
     */
    if ( direction == XENMEM_foreigncopy_from )
        rc = xsm_foreigncopy_from(XSM_TARGET, currd, d);
    else
        rc = xsm_foreigncopy_to(XSM_TARGET, currd, d);
    if ( rc )
        goto out;
...

I wrote some code for the compat mode but I need to test it.
Still I think that adding it it's a mistake, it's just a new, probably
unused, ABI that must be maintained till a probable "no virtual
address" ABI will replace it.

Frediano

Re: [PATCH Linux v6 16/16] xen/privcmd: Add new ABI to allow copying foreign memory

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2866 bytes --]

On 03.08.26 16:23, Frediano Ziglio wrote:
> On Mon, 3 Aug 2026 at 15:05, Juergen Gross <jgross@suse.com> wrote:
>>
>> On 19.06.26 15:05, Frediano Ziglio wrote:
>>> This new ABI allows to copy foreign domain memory to/from a buffer.
>>> This avoids having to map/copy/unmap foreign memory which is
>>> expensive.
>>> This operation is done particularly when migrating VMs.
>>>
>>> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
>>
>> While doing a test build I got the following build failures for 32-bit arm:
>>
>>     CC      arch/arm/xen/enlighten.o
>> In file included from /home/gross/korg/src/arch/arm/include/asm/xen/interface.h:1,
>>                    from /home/gross/korg/src/include/xen/interface/xen.h:13,
>>                    from /home/gross/korg/src/include/xen/xen.h:52,
>>                    from /home/gross/korg/src/arch/arm/xen/enlighten.c:2:
>> /home/gross/korg/src/include/xen/arm/interface.h:22:35: error: unknown type name
>> '__guest_handle_uint8_t'
>>      22 | #define GUEST_HANDLE(name)        __guest_handle_ ## name
>>         |                                   ^~~~~~~~~~~~~~~
>> /home/gross/korg/src/include/xen/interface/memory.h:361:5: note: in expansion of
>> macro 'GUEST_HANDLE'
>>     361 |     GUEST_HANDLE(uint8_t) buffer;
>>         |     ^~~~~~~~~~~~
>> make[5]: *** [/home/gross/korg/src/scripts/Makefile.build:289:
>> arch/arm/xen/enlighten.o] Error 1
>> make[4]: *** [/home/gross/korg/src/scripts/Makefile.build:549: arch/arm/xen] Error 2
>> make[3]: *** [/home/gross/korg/src/scripts/Makefile.build:549: arch/arm] Error 2
>>
>>
>> Please fix those.
>>
>>
>> Juergen
> 
> Mumble...
> 
> I suppose this would fix it
> 
> diff --git a/include/xen/arm/interface.h b/include/xen/arm/interface.h
> index c3eada2642aa..7e79853b188d 100644
> --- a/include/xen/arm/interface.h
> +++ b/include/xen/arm/interface.h
> @@ -53,6 +53,7 @@ DEFINE_GUEST_HANDLE(int);
>   DEFINE_GUEST_HANDLE(void);
>   DEFINE_GUEST_HANDLE(uint64_t);
>   DEFINE_GUEST_HANDLE(uint32_t);
> +DEFINE_GUEST_HANDLE(uint8_t);
>   DEFINE_GUEST_HANDLE(xen_pfn_t);
>   DEFINE_GUEST_HANDLE(xen_ulong_t);
> 
> Frediano
> 

And now another one:

/home/gross/korg/src/drivers/xen/privcmd.c: In function 'privcmd_ioctl_foreigncopy':
/home/gross/korg/src/drivers/xen/privcmd.c:1591:29: error: incompatible types 
when assigning to type 'const xen_pfn_t *' {aka 'const long long unsigned int 
*'} from type '__guest_handle_xen_pfn_t'
  1591 |                 copy.pfns = xcopy.frame_list;
       |                             ^~~~~
/home/gross/korg/src/drivers/xen/privcmd.c:1592:31: error: incompatible types 
when assigning to type 'void *' from type '__guest_handle_uint8_t'
  1592 |                 copy.buffer = xcopy.buffer;
       |                               ^~~~~


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]