Re: [Xen-devel] [RFC XEN PATCH 04/16] xen/x86: add XENMEM_populate_pmemmap to map host pmem pages to guest

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.10.16 at 02:32, <haozhong.zhang@xxxxxxxxx> wrote:
> +static int pmem_assign_pages(struct domain *d,
> +                             struct page_info *pg, unsigned int order)

What is the order parameter good for here, when the only caller
passes zero?

> +{
> +    int rc = 0;
> +    unsigned long i;
> +
> +    spin_lock(&d->pmem_lock);
> +
> +    if ( unlikely(d->is_dying) )
> +    {
> +        rc = -EINVAL;
> +        goto out;
> +    }
> +
> +    for ( i = 0; i < (1 << order); i++ )
> +    {
> +        ASSERT(page_get_owner(&pg[i]) == NULL);
> +        ASSERT((pg[i].count_info & ~(PGC_allocated | 1)) == 0);

Why is PGC_allocated | 1 allowed to be set here?

> @@ -1329,6 +1330,36 @@ long do_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>      }
>  #endif
>  
> +    case XENMEM_populate_pmemmap:
> +    {
> +        struct xen_pmemmap pmemmap;
> +        struct xen_pmemmap_args args;
> +
> +        if ( copy_from_guest(&pmemmap, arg, 1) )
> +            return -EFAULT;
> +
> +        d = rcu_lock_domain_by_any_id(pmemmap.domid);
> +        if ( !d )
> +            return -EINVAL;

I don't think you mean DOMID_SELF to be used here. And you're
lacking an XSM check in any event.

> +        args.domain = d;
> +        args.mfn = pmemmap.mfn;
> +        args.gpfn = pmemmap.gpfn;
> +        args.nr_mfns = pmemmap.nr_mfns;
> +        args.nr_done = start_extent;
> +        args.preempted = 0;
> +
> +        rc = pmem_populate(&args);

Please make sure you don't break the ARM build here.

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -646,7 +646,19 @@ 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 28 */
> +#define XENMEM_populate_pmemmap 28
> +
> +struct xen_pmemmap {
> +    /* IN */
> +    domid_t domid;
> +    xen_pfn_t mfn;
> +    xen_pfn_t gpfn;
> +    unsigned int nr_mfns;
> +};

You'll clearly need to add compat mode argument translation code.

Also, may I suggest xen_pmem_map (and similarly elsewhere), to
avoid mistaking this for "physical memory map" or some such (i.e.
keeping pmem and map sufficiently separated)?

Also I think patch 5 needs to be merged here, or be moved ahead
of this one, to avoid the intermediate broken state (leaking all the
pmem pages assigned to a domain).

Jan


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