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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.06.2026 12:55, Frediano Ziglio wrote:
> On Mon, 22 Jun 2026 at 11:34, Jan Beulich <jbeulich@xxxxxxxx> 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