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

[Jan Beulich <jbeulich@suse.com>]

On 13.08.2026 16:03, Frediano Ziglio wrote:
> On Thu, 13 Aug 2026 at 10:41, Jan Beulich <jbeulich@suse.com> 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@citrix.com>
>>
>> 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.

None of this happened in public, though? In which case, how would I know?

>>> ---
>>>  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 don't understand how use of guest (not userspace) handles would get in
the way of anything.

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

Thanks.

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

Well, as per below, I pointed you at a particular pending patch, a single
hunk of which could be broken out.

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

Doesn't really matter. Unsigned types want using for values which can only
be non-negative. IF nothing else, then for consistency and doc purposes.

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

How's that relevant? That's operating on page tables, when here we want to
_prevent_ to copy into page tables (or descriptor ones, for that matter).

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

Both Andrew and Roger confirm that this is supposed to work also from PVH
Dom0 (not sure why you keep saying "guest"), and also used to work. If it
doesn't, it would be a regression, and it would help if you supplied more
detail on the observed failure.

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

Nit: There's now a stray "the".

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

I'm not asking that you tidy up other hooks. What I'm asking is that new
hooks please be const-correct. Yet of course I'm not a maintainer of XSM,
so Daniel may tell you otherwise.

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

Daniel, can you clarify this please?

Jan