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

[Frediano Ziglio <freddy77@xxxxxxxxx>]

On Thu, 13 Aug 2026 at 10:41, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.
>

Yes, updated, I realized it after sending it.

> 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.
>

All other maintainers, after replying to their comments didn't
disagree with me, and this was different versions before, so stop
counting them, it's only you that don't like this at the moment.
I already explained why this is consistent with all other current hypercall.
I asked for a practical change to the API and nobody (including you)
has a proposal. I made a proposal and nobody replied to it.
If the issue is just adding a comment in the commit message I can do it.

> > ---
> >  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.
>

Yes, I was just going to reply.
I spent multiple days trying to implement the compat case or simply
HVM support with an issue after the other:
- multiple distributions removed the 32 bit support so it was hard to
have a setup;
- the original hypercall this PR is trying to optimise is supported
only in PV (so no HVM or compat guests);
- migration and other operations can work only on PV (like dm_op
operation) due to the usage of userspace handles used.
I tried to bypass the above limitations to have the new hypercall
tested and I manage to have 64 bit HVM working but the result is a
collection of nasty hacks and supporting them properly is a different
bigger task.
Surely a comment in the commit message on this is worth it.

> > --- 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?
>

Changed

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

Just rebased on master, all XSM hooks accept no-const pointers to domains.
So the suggested change would create warnings.

> > +    xen_foreigncopy_t copy;
> > +    int rc, direction;
>
> Plain int for rc is fine of course, but direction can't go negative, can it?
>

No, but the default type for constants is int and "flags" is promoted to int.

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

Changed.

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

It seems sensible, I didn't know it, I'll try it.

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

Changed

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

Unfortunately that does not work.
The code is coherent with MMU_UPDATE.
I'll write a comment.

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

Yes, add an "uncopied" variable and return -EFAULT.

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

Changed.

> > --- 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?
>

Maybe:
    This call is meant to replace expensive operations (mmap/copy/munmap) during
    migration which can only be issued from PV guests.

You can migrate any domain. Just from a PV guest (this is not a regression).

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

Updated to

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

    /*
     * IN/OUT
     *
     * Frames to be copied.
     * On output updated to the point to the first frame unhandled, 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 page pointer unhandled.
     */
    XEN_GUEST_HANDLE(uint8) buffer;


> > --- 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.
>

It would honestly make sense if they were all const.
However at the moment this would not be coherent with the current code.
For instance these functions call "xsm_default_action" which does not
accept constant domains (xen/include/xsm/dummy.h).
I think the most clean thing would be first to change xsm arguments to
const first.
But it looks a bit out of scope with this PR.

> 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.
>

It was a comment from Daniel.

> > --- 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?
>

Usually for memory you don't have write-only due to cache reasons.
But I suppose in this case only writint can work.
I'll change and test.

> Jan

Frediano