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

[Jan Beulich <jbeulich@suse.com>]

On 26.06.2026 16:14, Frediano Ziglio wrote:
> On Wed, 24 Jun 2026 at 07:44, Jan Beulich <jbeulich@suse.com> wrote:
>> On 23.06.2026 23:18, Frediano Ziglio wrote:
>>> On Tue, 23 Jun 2026 at 14:21, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 23.06.2026 12:55, Frediano Ziglio wrote:
>>>>> On Mon, 22 Jun 2026 at 11:34, Jan Beulich <jbeulich@suse.com> 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.

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

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

Jan