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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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

> +                                        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:

/*
 *

> +     *          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

> +     *      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__) */
>   
>   /*
Cheers,

--
Julien Grall

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