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

On 14.08.2026 15:47, Frediano Ziglio wrote:
> On Thu, 13 Aug 2026 at 15:22, Jan Beulich <[email protected]> wrote:
>> On 13.08.2026 16:03, Frediano Ziglio wrote:
>>> On Thu, 13 Aug 2026 at 10:41, Jan Beulich <[email protected]> wrote:
>>>> On 10.08.2026 12:30, Frediano Ziglio wrote:
>>>>> ---
>>>>>  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.
> 
> In this case userspace is not a typo. For HVM
> copy_from_user_hvm/copy_to_user_hvm are used and these functions
> accept only kernel space pointers.

I fear you've now completely lost me.

>>>>> --- 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.
> 
> Yes, but my changes would have to have casts from const pointers to
> no-const pointers to avoid warnings and the patch you are pointing to
> would have to remove these casts. I find this less clean than having
> one patch using the current code style (that is no-const pointers) and
> another that changes the style entirely.
> But obviously this is just my opinion.

Such casts would be unacceptable. What instead I have been trying to convey:
Your patch wants to gain a dependency on my patch. And if my patch would
take too long to make it in, that one hunk could be broken out into a
separate, easy to get in patch.

>>>>> +            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).
> 
> This new ABI is to better support migration.
> We are migrating all the VM status including page tables... how can we
> not be able to write them but migrate them from one  host to another ?
> You are basically explaining why changing the check the migration fails.

No, what I'm trying to explain is that without such a check, you introduce
a security issue (of privilege escalation kind). I hope you agree that we
cannot knowingly allow such code to be committed.

To migrate-in page tables, you'd need to copy their contents before they
obtain their PGT_l<N>_page_table type, so that upon being converted to page
tables, they can be properly audited by the mm.c functions we have for that
exact purpose.

>>>>> --- 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.
> 
> Indeed I tested the migration of various domains (PV, HVM, PV-in-PVH),
> but only access to added hypercall from PV and HVM. I should add a
> test from a PVH guest.
> I say guest because to test HVM I used a hack to allow all guests (not
> only dom0).

And why would testing from PVH Dom0 not do?

Jan