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

On Mon, 29 Jun 2026 at 07:59, Jan Beulich <[email protected]> wrote:
>
> On 26.06.2026 16:14, Frediano Ziglio wrote:
> > On Wed, 24 Jun 2026 at 07:44, Jan Beulich <[email protected]> wrote:
> >> On 23.06.2026 23:18, Frediano Ziglio wrote:
> >>> On Tue, 23 Jun 2026 at 14:21, Jan Beulich <[email protected]> wrote:
> >>>> On 23.06.2026 12:55, Frediano Ziglio wrote:
> >>>>> On Mon, 22 Jun 2026 at 11:34, Jan Beulich <[email protected]> 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.
> >>>
> >>> But this is not a grant, it's a copy.
> >>
> >> Sure, but the underlying principle is what matters. Plus you don't prevent
> >> self-copy by using ..._by_id(), you only preclude the use of DOMID_SELF.
> >
> > Sure about this?
>
> No, I'm sorry: I (repeatedly) managed to ignore the "remote" in the function
> called. That said, my request stands: No arbitrary restrictions please. If
> you can properly justify a restriction, that's a different thing.
>

Not strong about it.
I'll change to rcu_lock_domain_by_any_id.

> >>>>>>> +    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.
> >>>
> >>> Not needing to have backchannel feedback, I already wrote that a
> >>> similar approach was tried and made the code more complicated.
> >>
> >> Even if indeed so: Yet at the same time more flexible.
> >>
> >>> Both maintainers didn't comment on my replies so I assume they were
> >>> fine with it.
> >>> And you are failing to provide positive feedback.
> >>> I asked (that one internally) for examples of guest buffers provided
> >>> as frame numbers but I got no answer (or better the answer was more
> >>> "currently there are not").
> >>> Also note that the location of xen_foreigncopy_t structure is also
> >>> provided using a guest pointer.
> >>> I remember there were some discussions about ABI changes (2/3 years
> >>> ago) to address this and other issues but I cannot see much progress.
> >>
> >> And it's that (very slowly progressing effort) which made me ask. The
> >> fewer virtual addresses we bake into new sub-ops, the better for that
> >> effort. And no, that doesn't go as far as completely eliminating
> >> handles (presently representing virtual addresses) - that needs to be
> >> part of the new ABI.
> >
> > In other words, you want me to code something temporary that you
> > already know that needs to be changed.
>
> What do you mean by "temporary"? We will need to live with the present
> ABI for the foreseeable future. The new ABI's requirements haven't even
> been spelled out yet. Patches to allow use of physical addresses in
> place of virtual ones were actually turned down on the grounds of there
> not having been a write-down of all requirements.
>

Temporary in the sense that there will be new ABIs to deal with not
using virtual addresses.
The second sentence is a bit contradictory. You want me to address the
virtual address complaint but you are telling me that the change will
be turned down if I don't address everything. And this is why this is
out of scope here.

> >> To preempt the argument towards "fewer virtual addresses" not really
> >> being true when changing from handle-to-uint8 to handle-to-pfn: The
> >> former won't be able to express a buffer mapped contiguously in VA
> >> space, but discontiguous in PA space. The latter will, simply be
> >> avoiding buffer VAs in the first place (the array of frame numbers
> >> can e.g. be placed in a dedicated hypercall argument area known to be
> >> physically contiguous).
> >
> > If it's mapped continuously in VA and you pass the VA I don't
> > understand the problem. From the way I see it's more the latter that's
> > the problem.
>
> I'm talking of the future, where VAs wouldn't be used anymore. The
> buffer you use couldn't be described by a single PA, unless the caller
> took specific measures up front.
>

If you read my reply I suggested a way to avoid virtual addresses completely.

> Jan

About the P2M type check it turned out that I was wrong with the
checking. The MMAP way use MMU_UPDATE calls which do not care about
P2M type at all. Changing the code to

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

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

About the XSM part I have now

...
    /*
     * Check we are allowed to map and access these foreign pages.
     */
    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;
...

I wrote some code for the compat mode but I need to test it.
Still I think that adding it it's a mistake, it's just a new, probably
unused, ABI that must be maintained till a probable "no virtual
address" ABI will replace it.

Frediano