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

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

> > +            if ( rc )
> > +            {
> > +                /*
> > +                 * Make sure rc is -EIO for any interation other than
> > +                 * the first.
> > +                 */
> > +                rc = (i != 0) ? -EIO : rc;
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> > + out:
> > +    rcu_unlock_domain(d);
> > +    return rc;
> > +}
> > +
> >   long do_memory_op(unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> >   {
> >       struct domain *d, *curr_d = current->domain;
> > @@ -1406,6 +1495,11 @@ long do_memory_op(unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> >       }
> >   #endif
> >
> > +    case XENMEM_acquire_resource:
> > +        rc = acquire_resource(
> > +            guest_handle_cast(arg, xen_mem_acquire_resource_t));
> > +        break;
> > +
> >       default:
> >           rc = arch_memory_op(cmd, arg);
> >           break;
> > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> > index faadcfe8fe..a5caa747ce 100644
> > --- a/xen/include/asm-arm/p2m.h
> > +++ b/xen/include/asm-arm/p2m.h
> > @@ -346,6 +346,12 @@ static inline gfn_t gfn_next_boundary(gfn_t gfn,
> unsigned int order)
> >       return gfn_add(gfn, 1UL << order);
> >   }
> >
> > +static inline int set_foreign_p2m_entry(struct domain *d, unsigned long
> gfn,
> 
> Please modifify the prototype to use gfn_t.
> 

<sigh> I only put this stub in to match x86 so I can avoid the #ifdefs that Jan 
objects to. Which other bits of the universe do I need to fix?

> > +                                        mfn_t mfn)
> > +{
> > +    return -EOPNOTSUPP;
> > +} > +
> >   #endif /* _XEN_P2M_H */
> >
> 
> [...]
> 
> > diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> > index 29386df98b..18118ea5c6 100644
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -599,6 +599,47 @@ struct xen_reserved_device_memory_map {
> >   typedef struct xen_reserved_device_memory_map
> xen_reserved_device_memory_map_t;
> >   DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
> >
> > +/*
> > + * Get the pages for a particular guest resource, so that they can be
> > + * mapped directly by a tools domain.
> > + */
> > +#define XENMEM_acquire_resource 28
> > +struct xen_mem_acquire_resource {
> > +    /* IN - the domain whose resource is to be mapped */
> > +    domid_t domid;
> > +    /* IN - the type of resource */
> > +    uint16_t type;
> > +    /*
> > +     * IN - a type-specific resource identifier, which must be zero
> > +     *      unless stated otherwise.
> > +     */
> > +    uint32_t id;
> > +    /* IN/OUT - As an IN parameter number of frames of the resource
> 
> Coding style:
> 
> /*
>   *

Ok.

> 
> > +     *          to be mapped. However, if the specified value is 0 and
> > +     *          frame_list is NULL then this field will be set to the
> > +     *          maximum value supported by the implementation on return.
> > +     */
> > +    uint32_t nr_frames;
> > +    uint32_t pad;
> > +    /* IN - the index of the initial frame to be mapped. This parameter
> 
> Ditto
> 

Ok.

> > +     *      is ignored if nr_frames is 0.
> > +     */
> > +    uint64_aligned_t frame;
> > +    /* IN/OUT - If the tools domain is PV then, upon return, frame_list
> 
> Ditto
> > +     *          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.

  Paul


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