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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.06.2026 23:47, Frediano Ziglio wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1545,6 +1545,133 @@ 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 & ~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;

Extending my v4 remark: How about a "fully foreign" copy? I.e. one between two
pages in two foreign domains? Getting me back also to the un-answered v4
question of mine as to MMUEXT_COPY_PAGE.

Further, as to the order of checks: I'm not going to insist on re-ordering, yet
I'd like to point out that copying 0 pages to/from a bad domid might better
yield an error.

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

As per

K:      xsm_.*
K:      \b(xsm|XSM)\b

in ./MAINTAINERS please Cc the XSM/Flask maintainer for such changes. I for one
question the re-use of an existing predicate here.

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

Nit: "unsigned int" please (like you have it above).

> +        {
> +            struct page_info *foreign_page;
> +            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

What about, for example, p2m_ram_ro? Or p2m_ram_shared? Or p2m_grant_map_*?
Etc. Any artificial constraining wants justifying in the description and/or
mentioning in the public header.

> +            foreign_page = get_page_from_gfn(d, gfn_list[i], &p2mt, 
> P2M_ALLOC);
> +
> +            if ( unlikely(p2m_to_mask(p2mt) & valid_mask) && foreign_page )

Was this meant to be

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

Latch page_to_mfn(foreign_page) into a local variable to avoid the redundant
translation (which the compiler may not be able to fold)?

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

It's not quite clear whether replacing an earlier non-zero value held in
rc is a good thing here. The only time when it's strictly necessary is
when you schedule a continuation. I'd suggest to add "&& rc >= 0".

> @@ -2012,6 +2139,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;

Please address review comments (verbally or by code changes) before submitting
a new version. Here I had asked "Why make this different from other continuable
sub-ops?"

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

There's still "resource" here, when this really is about RAM (memory) only,
not any other kind of resource.

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

This isn't just an input, as you update the field (and the handles below).
This property of fields wants reflecting here, so callers know that they (a)
can't re-use the struct on a subsequent call without re-initializing the
fields which may have changed, and (b) can't put the struct in r/o memory.

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

With these two handles, there continues to be a need to (explicitly) deal
with the compat case as well.

Jan