Re: [PATCH v10 7/10] xen: implement new foreign copy hypercall

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.08.2026 12:30, 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@xxxxxxxxxx>

First: Can you please update your recipient list before sending a new version?
Roger's move pre-dates this submission by quite a few days.

Then: The asymmetry of the new sub-op also continues to have no justification
at all. If you insist on not using two (domid,list-of-gfns) tuples to describe
the buffers, despite multiple maintainers having asked you to do so, please
put down a word explaining that decision.

> ---
>  xen/common/memory.c         | 149 ++++++++++++++++++++++++++++++++++++
>  xen/include/public/memory.h |  45 ++++++++++-
>  xen/include/xsm/dummy.h     |  14 ++++
>  xen/include/xsm/hooks.h     |   2 +
>  xen/xsm/flask/hooks.c       |  10 +++
>  5 files changed, 219 insertions(+), 1 deletion(-)

As before: If you insist on not implementing the compat case, that decision
wants justifying in the description. Without that it'll look like an
oversight.

> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1548,6 +1548,141 @@ 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(

I'm wondering: Is the "mem" prefix really meaningful for a static function in
a file named memory.c?

> +    XEN_GUEST_HANDLE_PARAM(xen_foreigncopy_t) arg)
> +{
> +    struct domain *d, *const currd = current->domain;

With the comment on the new XSM hooks (below) in mind: currd wants to be
pointer-to-const.

> +    xen_foreigncopy_t copy;
> +    int rc, direction;

Plain int for rc is fine of course, but direction can't go negative, can it?

> +    if ( copy_from_guest(&copy, arg, 1) )
> +        return -EFAULT;
> +
> +    if ( copy.flags & ~XENMEM_foreigncopy_direction )
> +        return -EINVAL;
> +
> +    direction = copy.flags & XENMEM_foreigncopy_direction;
> +
> +    d = rcu_lock_domain_by_any_id(copy.domid);
> +    if ( !d )
> +        return -ESRCH;
> +
> +    /*
> +     * Check we are allowed to map and access these foreign pages.
> +     */

This really means to be a single-line comment.

> +    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;
> +
> +    while ( copy.nr_frames )
> +    {
> +        /*
> +         * 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;
> +            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);

Is there a reason check_get_page_from_gfn() can't or shouldn't be used here?
That would then also make this code properly deal with shared pages, and
refuse use against MMIO ones.

> +            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;
> +            }
> +
> +            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);

This may better be folded into the "else" below.

> +            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);

What I continue to be missing prior to this is the obtaining of a writable
page ref. That's, as previously said, imperative for PV guests and at the
very least advisable for HVM ones. (I really wonder how many more times I
need to comment on this.)

> +            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;

rc at this point holds the number of bytes which could not be copied. That's
not suitable as a return value from this function. (Strictly speaking this
is also an abuse of rc, as copy_{to,from}_*() return unsigned quantities.)

> +            }
> +
> +            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);

Nit: Indentation (the anchor point is the start of the function name, not
the start of the statement).

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -740,7 +740,50 @@ 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.
> + * This calls is meant to replace expensive operations during migration which

Nit: "This call is ..." However, is ...

> + * are only supported for PV guests.

... this entire sentence really worth to have here (it looks more like
something to have in the description)? For it to be possible to find if
someone considered using those "expensive operations", I think it would need
to be less vague and name those operations. Furthermore, if those other
operations were supported only for PV guests, how would migration work for
non-PV ones?

> + */
> +#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.

I think there's a comma wanted after "parameter".

> +     * On output updated number of frames left (0 if success).

I further think that adding "to" after "updated" would help here.

> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -569,6 +569,20 @@ static XSM_INLINE int cf_check xsm_map_gmfn_foreign(
>      return xsm_default_action(action, d, t);
>  }
>  
> +static XSM_INLINE int cf_check xsm_foreigncopy_from(
> +    XSM_DEFAULT_ARG struct domain *d, struct domain *t)

I think these and ...

> +{
> +    XSM_ASSERT_ACTION(XSM_TARGET);
> +    return xsm_default_action(action, d, t);
> +}
> +
> +static XSM_INLINE int cf_check xsm_foreigncopy_to(
> +    XSM_DEFAULT_ARG struct domain *d, struct domain *t)

... want to be pointer-to-const right away, requiring to re-base over "XSM:
make Argo hooks well-formed ones" (or alternatively requiring to split out
the change there to xsm_default_action()). We really should avoid gaining
...

> --- a/xen/include/xsm/hooks.h
> +++ b/xen/include/xsm/hooks.h
> @@ -58,6 +58,8 @@ XSM_HOOK(int, add_to_physmap, struct domain *, struct 
> domain *)
>  XSM_HOOK(int, remove_from_physmap, struct domain *, struct domain *)
>  XSM_HOOK(int, map_gmfn_foreign, struct domain *, struct domain *)
>  XSM_HOOK(int, claim_pages, struct domain *)
> +XSM_HOOK(int, foreigncopy_from, struct domain *, struct domain *);
> +XSM_HOOK(int, foreigncopy_to, struct domain *, struct domain *);

... new hooks with non-const-correct parameters.

Further: Why two new hooks? See e.g. "XSM: fold xsm_{,un}map_domain_pirq()
hooks", "XSM: fold xsm_{,un}map_domain_irq() hooks", or "XSM: fold
xsm_{,un}bind_pt_irq() hooks": We'd like to reduce the number of hooks, to
reduce (when non-dummy XSM is in use) the number of cf_check entry points.

> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1368,6 +1368,16 @@ static int cf_check flask_map_gmfn_foreign(struct 
> domain *d, struct domain *t)
>      return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | 
> MMU__MAP_WRITE);
>  }
>  
> +static int cf_check flask_foreigncopy_from(struct domain *d, struct domain 
> *t)
> +{
> +    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ);
> +}
> +
> +static int cf_check flask_foreigncopy_to(struct domain *d, struct domain *t)
> +{
> +    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | 
> MMU__MAP_WRITE);

Why both READ and WRITE?

Jan