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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi,

On 19/10/17 14:35, Paul Durrant wrote:
> > -----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.
For x86 maybe. For Arm, foreign mapping are also exposed to guest and we 
get a reference every time. It is probably an oversight on the x86 side.
> > > > 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.
Well a foreign domain can be nasty with you. Like removing the mapping 
from itself resulting to free the memory... So you may end up 
writing/read wrong mapping or even worth in another domain.
But that's may not impact this new hypercall.

> > [...]
> >
> > > > > +     *          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?
I am quite surprised of "it is tools-only" so it is fine to not protect 
it even if it is x86 only. That's probably going to bite us in the future.
Cheers,
--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel