Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@xxxxxxxxxx]
> Sent: 19 October 2017 14:29
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>;
> George Dunlap <George.Dunlap@xxxxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>; Julien Grall <julien.grall@xxxxxxx>; Jan Beulich
> <jbeulich@xxxxxxxx>; Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>; Roger
> Pau Monne <roger.pau@xxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> HYPERVISOR_memory_op to acquire guest resources
> 
> Hi,
> 
> On 10/19/2017 01:57 PM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Julien Grall [mailto:julien.grall@xxxxxxxxxx]
> >> Sent: 19 October 2017 13:23
> >> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu
> >> <wei.liu2@xxxxxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>;
> >> George Dunlap <George.Dunlap@xxxxxxxxxx>; Andrew Cooper
> >> <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> Tim
> >> (Xen.org) <tim@xxxxxxx>; Julien Grall <julien.grall@xxxxxxx>; Jan
> Beulich
> >> <jbeulich@xxxxxxxx>; Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>; Roger
> >> Pau Monne <roger.pau@xxxxxxxxxx>
> >> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
> >> HYPERVISOR_memory_op to acquire guest resources
> >>
> >> Hi,
> >>
> >> On 17/10/17 14:24, Paul Durrant wrote:
> >>> Certain memory resources associated with a guest are not necessarily
> >>> present in the guest P2M.
> >>>
> >>> This patch adds the boilerplate for new memory op to allow such a
> >> resource
> >>> to be priv-mapped directly, by either a PV or HVM tools domain.
> >>>
> >>> NOTE: Whilst the new op is not intrinsicly specific to the x86 
> >>> architecture,
> >>>         I have no means to test it on an ARM platform and so cannot verify
> >>>         that it functions correctly.
> >>>
> >>> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> >>> ---
> >>
> >> [...]
> >>
> >>> diff --git a/xen/common/memory.c b/xen/common/memory.c
> >>> index ad987e0f29..cdd2e030cf 100644
> >>> --- a/xen/common/memory.c
> >>> +++ b/xen/common/memory.c
> >>> @@ -965,6 +965,95 @@ static long xatp_permission_check(struct
> domain
> >> *d, unsigned int space)
> >>
> >> [...]
> >>
> >>> +    if ( rc )
> >>> +        goto out;
> >>> +
> >>> +    if ( !paging_mode_translate(currd) )
> >>> +    {
> >>> +        if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
> >>> +            rc = -EFAULT;
> >>> +    }
> >>> +    else
> >>> +    {
> >>> +        xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> >>> +        unsigned int i;
> >>> +
> >>> +        rc = -EFAULT;
> >>> +        if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
> >>> +            goto out;
> >>> +
> >>> +        for ( i = 0; i < xmar.nr_frames; i++ )
> >>> +        {
> >>> +            rc = set_foreign_p2m_entry(currd, gfn_list[i],
> >>> +                                       _mfn(mfn_list[i]));
> >>
> >> Something looks a bit odd to me here. When I read foreign mapping, I
> >> directly associate to mapping from a foreign domain.
> >>
> >> On Arm, we will always get a reference on that page to prevent it
> >> disappearing if the foreign domain is destroyed but the mapping is still
> >> present.
> >>
> >> This reference will either be put with an unmapped hypercall or while
> >> teardown the domain.
> >>
> >> Per my understanding, this MFN does not belong to any domain (or at
> >> least currd). Right?
> >
> > No. The mfns do belong to the target domain.
> 
> To be fully safe, you need to take a reference on each page you mapped.
> So who is going to get a reference on them? Who is going to drop that?
> 

Yes, that's true but it's also true of priv mapping AIUI. I think the correct 
fix is to deal with this in set_p2m_foreign_entry() so that it is fixed for 
both cases. I don't think it is something that ought to be addressed here... 
unless I'm missing something.

> >
> >> So there is no way to get/put a reference on that
> >> page. So I am unconvinced that this is very safe.
> >>
> >> Also looking at the x86 side, I can't find such reference in the foreign
> >> path in p2m_add_foreign. Did I miss anything?
> >
> > No, I don't think there is any reference counting there... but this is no
> different to priv mapping. I'm not trying to fix the mapping infrastructure at
> this point.
> >
> >>
> >> Note that x86 does not handle p2m teardown with foreign map at the
> >> moment (see p2m_add_foreign).
> >>
> >> You are by-passing this check and I can't see how this would be safe for
> >> the x86 side too.
> >>
> >
> > I don't follow. What check am I by-passing that is covered when priv
> mapping?
> 
>      /*
>       * hvm fixme: until support is added to p2m teardown code to
> cleanup any
>       * foreign entries, limit this to hardware domain only.
>       */
> 
> How this is safe with your new solution? That looks like a regression...

Well, the new hypercall is tools-only but I can add the extra check for the 
hardware domain although it's probably redundant in practice.

> 
> [...]
> 
> >>> +     *          will be populated with the MFNs of the resource.
> >>> +     *          If the tools domain is HVM then it is expected that, on
> >>> +     *          entry, frame_list will be populated with a list of GFNs
> >>> +     *          that will be mapped to the MFNs of the resource.
> >>> +     *          If -EIO is returned then the frame_list has only been
> >>> +     *          partially mapped and it is up to the caller to unmap all
> >>> +     *          the GFNs.
> >>> +     *          This parameter may be NULL if nr_frames is 0.
> >>> +     */
> >>> +    XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
> >>> +};
> >>> +typedef struct xen_mem_acquire_resource
> >> xen_mem_acquire_resource_t;
> >>> +DEFINE_XEN_GUEST_HANDLE(xen_mem_acquire_resource_t);
> >>> +
> >>>    #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
> >>>
> >>>    /*
> >>
> >
> > Sorry to be getting frustrated with this, but I'm wondering how many more
> colours I need to paint this bike-shed.
> 
> I don't know how x86 looks like and maybe this is fine for Andrew and
> Jan. But for Arm, it does not look correct.
> 
> To give you an idea, my first thought to implement your newly wrongly
> named function was to just call p2m_set_entry with p2m_map_foreign. But
> from this discussion it would look plain wrong.
> 
> So this means the interface is not clear enough.

I'd prefer to make the whole thing x86-only since that's the only platform on 
which I can test it, and indeed the code used to be x86-only. Jan objected to 
this so all I'm trying to achieve is that it builds for ARM. Please can you and 
Jan reach agreement on where the code should live and how, if at all, it should 
be #ifdef-ed?

  Paul

> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel