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

[Frediano Ziglio <freddy77@xxxxxxxxx>]

On Mon, 15 Jun 2026 at 16:23, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 15.06.2026 17:07, Frediano Ziglio wrote:
> > On Mon, 15 Jun 2026 at 15:03, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >> On 15.06.2026 14:11, Frediano Ziglio wrote:
> >>> On Mon, 15 Jun 2026 at 08:41, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>>> On 13.06.2026 23:47, Frediano Ziglio wrote:
> >>>>> +        {
> >>>>> +            struct page_info *foreign_page;
> >>>>> +            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
> >>>>
> >>>> What about, for example, p2m_ram_ro? Or p2m_ram_shared? Or 
> >>>> p2m_grant_map_*?
> >>>> Etc. Any artificial constraining wants justifying in the description 
> >>>> and/or
> >>>> mentioning in the public header.
> >>>
> >>> The base of this was taken from migration code where there is such a 
> >>> check.
> >>> I suppose that adding p2m_ram_ro (where available) won't hurt.
> >>
> >> Just to mention, to avoid another round trip just because of this: 
> >> p2m_ram_ro
> >> has different meaning on x86 vs Arm/RISC-V.
> >
> > That's confusing... should not this be fixed somehow?
> > It won't save much from a round-trip. Should I allow it or not ?
>
> Ask the Arm maintainers. I raised this issue more than once, without any real
> success.
>
> >>> p2m_ram_shared I'm not sure but seems fine too.
> >>> For p2m_grant_map_* it feels a bit a security issue to me. It would
> >>> allow a guest to give access to pages of other domains. It's true that
> >>> the current domain would have to have write access to this domain
> >>> anyway but extend these permissions sounds something it should not be
> >>> able to do.
> >>
> >> It could copy the contents of the grant mapped page by other means. Why not
> >> allow it in this new sub-op as well then?
> >
> > I'm more afraid of writing the content of the grant pages than copying it.
>
> But the same is true for writing to the granted page: The domain could do so
> by other means.
>
> >> Talking of security: When the page you copy to is owned by a PV guest, I
> >> think you further need to obtain a PGT_writable type ref. (Of course it 
> >> then
> >> likely is easier to always do this, not just for PV.)
> >
> > Wondering how save/restore (or migration) works in this case.
>
> Save is not relevant here. Restore happens before the guest gains control.
> Which is entirely different from hypercall handling.
>
> >>>>> @@ -2012,6 +2139,13 @@ long do_memory_op(unsigned long cmd, 
> >>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>>>              start_extent);
> >>>>>          break;
> >>>>>
> >>>>> +    case XENMEM_foreigncopy:
> >>>>> +        if ( unlikely(start_extent) )
> >>>>> +            return -EINVAL;
> >>>>
> >>>> Please address review comments (verbally or by code changes) before 
> >>>> submitting
> >>>> a new version. Here I had asked "Why make this different from other 
> >>>> continuable
> >>>> sub-ops?"
> >>>>
> >>>
> >>> There's already a comment in the same file for similar reason
> >>>
> >>>     /*
> >>>      * Limiting nr_frames at (UINT_MAX >> MEMOP_EXTENT_SHIFT) isn't 
> >>> ideal.  If
> >>>      * it ever becomes a practical problem, we can switch to mutating
> >>>      * xmar.{frame,nr_frames,frame_list} in guest memory.
> >>>      */
> >>>
> >>> so to avoid the doubt and possible future change I mutate the structure.
> >>> Also I use the mutation to give more information to the caller, using
> >>> "start_entent" won't allow this.
> >>
> >> You'll want to mention this in the description and/or a code comment. It
> >> wants to become clear that the inconsistency in behavior (with other sub-
> >> ops) is deliberate rather than accidental.
> >>
> >
> > Added a comment in the code:
> >
> >         if ( copy.nr_frames && hypercall_preempt_check() )
> >         {
> >             /*
> >              * Instead of using "start_extent" we update the structure back,
> >              * we update it back in anyway to tell caller were the copy
> >              * stopped.
> >              */
> >             rc = hypercall_create_continuation(
> >                 __HYPERVISOR_memory_op, "lh", XENMEM_foreigncopy, arg);
> >             goto out;
> >         }
>
> Please can this go into the hunk that I commented on?
>
> >>>>> --- a/xen/include/public/memory.h
> >>>>> +++ b/xen/include/public/memory.h
> >>>>> @@ -740,7 +740,45 @@ 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.
> >>>>> + */
> >>>>> +#define XENMEM_foreigncopy 29
> >>>>> +struct xen_foreigncopy {
> >>>>> +    /* IN - The domain whose resource is to be copied. */
> >>>>
> >>>> There's still "resource" here, when this really is about RAM (memory) 
> >>>> only,
> >>>> not any other kind of resource.
> >>>>
> >>>>> +    domid_t domid;
> >>>>> +
> >>>>> +    /* IN - Flags. */
> >>>>> +#define XENMEM_foreigncopy_from 0
> >>>>> +#define XENMEM_foreigncopy_to 1
> >>>>> +#define XENMEM_foreigncopy_direction 1
> >>>>> +    uint16_t flags;
> >>>>> +
> >>>>> +    /*
> >>>>> +     * IN
> >>>>> +     *
> >>>>> +     * As an IN parameter number of frames of the domain to be copied.
> >>>>> +     */
> >>>>> +    uint32_t nr_frames;
> >>>>
> >>>> This isn't just an input, as you update the field (and the handles 
> >>>> below).
> >>>> This property of fields wants reflecting here, so callers know that they 
> >>>> (a)
> >>>> can't re-use the struct on a subsequent call without re-initializing the
> >>>> fields which may have changed, and (b) can't put the struct in r/o 
> >>>> memory.
> >>>>
> >>>
> >>> Update comments:
> >>>
> >>> /*
> >>>  * 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 on error updated number of frames left.
> >>>      */
> >>>     uint32_t nr_frames;
> >>
> >> This is updated not only on error, but also when encoding continuations.
> >>
> >
> > Yes, but this seems more an implementation detail to me. I don't think
> > the caller cares about how the continuation is implemented.
>
> You just can't know what a caller may or may not care about. You want to
> be precise.
>
> >>>>> +    /*
> >>>>> +     * IN
> >>>>> +     *
> >>>>> +     * Frames to be copied.
> >>>>> +     */
> >>>>> +    XEN_GUEST_HANDLE(xen_pfn_t) frame_list;
> >>>>> +
> >>>>> +    /*
> >>>>> +     * IN/OUT
> >>>>> +     *
> >>>>> +     * Userspace buffer to read/write from.
> >>>>> +     */
> >>>>> +    XEN_GUEST_HANDLE(uint8) buffer;
> >>>>
> >>>> With these two handles, there continues to be a need to (explicitly) deal
> >>>> with the compat case as well.
> >>>
> >>> I don't agree with this. Domains having access to other domains are
> >>> limited (like stub domains for Qemu) and won't be 32 bits today so why
> >>> allow 32 bits guests if not ever used?
> >>
> >> How do you know? Why shouldn't e.g. XTF be permitted to test this in all
> >> possible modes? And even if all arguments end up in favor of "no compat
> >> support", this then wants spelling out to make clear this wasn't an
> >> oversight, but rather a conscious decision.
> >>
> >
> > XTF can do something like
> >
> > #if COMPAT_GUEST
> >     /* Compat guests are not supported, return success. */
> >     return 0;
> > # endif
> >
> > (or can be done in the Makefiles I suppose).
> >
> > Added a comment in memory.h:
> >
> >     /*
> >      * 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.
> >      */
>
> Well, okay. Right now what I can say is that with this it's then going to be
> rather unlikely that I'd ack the overall change. You make assumptions on
> what people may or may not do. There are still benefits to 32-bit environments
> in certain situations, even more so that the x32 mode of x86-64 didn't really
> take off.
>

Won't it save some round trips if you just told this a couple of
emails earlier? If you already knew that compat is worth doing why
asking to put a comment on it?

Similar to the discussion about Arm, if you know the issue, can you
explain it in more details? The description for the constant is the
same in all architecture, so, what is the difference?

> Jan

Frediano