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

[Frediano Ziglio <freddy77@xxxxxxxxx>]

On Mon, 8 Jun 2026 at 15:59, Teddy Astie <teddy.astie@xxxxxxxxxx> wrote:
>
> Le 03/06/2026 à 15:08, Frediano Ziglio a écrit :
> > From: Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>
> >
> > Add a sub hypercall to __HYPERVISOR_memory_op to allow to
> > read/write memory from/to a foreign domain.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>
> > ---
> >   xen/common/memory.c         | 133 ++++++++++++++++++++++++++++++++++++
> >   xen/include/public/memory.h |  40 ++++++++++-
> >   2 files changed, 172 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/common/memory.c b/xen/common/memory.c
> > index 3672bda025..6a2d9c3190 100644
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -1545,6 +1545,132 @@ static int acquire_resource(
> >       return rc;
> >   }
> >
> > +/*
> > + * The "noinline" qualifier avoid 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 ( !arch_acquire_resource_check(currd) )
> > +        return -EACCES;
> > +
> > +    if ( copy_from_guest(&copy, arg, 1) )
> > +        return -EFAULT;
> > +
> > +    if ( copy.flags & ~1u )
> > +        return -EINVAL;
> > +
> > +    direction = copy.flags & XENMEM_foreigncopy_direction;
> > +
> > +    if ( copy.nr_frames == 0 )
> > +        return 0;
> > +
> > +    rc = rcu_lock_remote_domain_by_id(copy.domid, &d);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    /*
> > +     * 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 i = 0; i < todo; i++ )
> > +        {
> > +            struct page_info *foreign_page;
> > +            void *foreign;
> > +            p2m_type_t p2mt;
> > +
> > +            foreign_page = get_page_from_gfn(d, gfn_list[i], &p2mt, 
> > P2M_ALLOC);
> > +
> > +            if ( unlikely(p2mt != p2m_ram_rw
> > +#ifdef CONFIG_X86
> > +                 && p2mt != p2m_ram_logdirty
> > +#endif
> > +                 ) && foreign_page )
> > +            {
> > +                put_page(foreign_page);
> > +                foreign_page = NULL;
> > +            }
> > +            if ( unlikely(!foreign_page) )
> > +            {
> > +                gdprintk(XENLOG_WARNING,
> > +                         "Error accessing foreign mfn %" PRI_mfn "\n",
> > +                         gfn_list[i]);
> > +                rc = -EINVAL;
> > +                copy.nr_frames -= i;
> > +                guest_handle_add_offset(copy.frame_list, i);
> > +                goto out;
> > +            }
> > +
> > +            /* A page is dirtied when it's being copied to. */
> > +            if ( direction == XENMEM_foreigncopy_to )
> > +                paging_mark_dirty(d, page_to_mfn(foreign_page));
> > +
> > +            foreign = map_domain_page(page_to_mfn(foreign_page));
> > +            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 copying to mfn %" PRI_mfn "\n", 
> > 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 = -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 +2138,13 @@ long do_memory_op(unsigned long cmd, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >               start_extent);
> >           break;
> >
> > +    case XENMEM_foreigncopy:
> > +        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..b48d1f378f 100644
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -740,7 +740,45 @@ 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.
> > + */
> > +#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;
> > +
>
> The interface only allows copies to be made at page granularity, while
> that can be ok, that probably wants to be stated.
>
> > +    /*
> > +     * IN
> > +     *
> > +     * Frames to be copied.
> > +     */
> > +    XEN_GUEST_HANDLE(xen_pfn_t) frame_list;
> > +
> > +    /*
> > +     * IN/OUT
> > +     *
> > +     * Userspace buffer to read/write from.
> > +     */
> > +    XEN_GUEST_HANDLE(uint8) buffer;
> > +};
>
> The interface looks a bit asymetric, on one hand, it takes pfns and on
> the other hand, it takes a guest virtual address.
>
> Though, using guest pointers is not great for PVH domains, as it
> requires expensive pagewalks (especially for a lot of pages).
>
> Would it be preferable to have a list of gmfn for both sides ?
>

All this was considered and the interface could accept either virtual
pointer or GFNs.
However, the code was more complicated and I think to resolve this
correctly is out of scope here.
How is the structure passed using the hypercall? With a virtual
pointer? Why not complain also about that pointer? It would be good
for PVH not to use virtual addresses at all.
The PFN/virtual should be solved more generically.

> > +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__ */
> >
>
> Teddy

Frediano