Re: [PATCH 2/18 V2]: PVH xen: add XENMEM_add_to_physmap_range

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Mar 18, 2013 at 11:38:35AM +0000, Jan Beulich wrote:
> >>> On 16.03.13 at 01:20, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> > In this patch we add a new function xenmem_add_to_physmap_range(), and
> > change xenmem_add_to_physmap_once parameters so it can be called from
> > xenmem_add_to_physmap_range. There is no PVH specific change here.
> > 
> > Changes in V2:
> >    - Do not break parameter so xenmem_add_to_physmap_once() but pass in
> >      struct xen_add_to_physmap.
> > 
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> > ---
> >  xen/arch/x86/mm.c |   82 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 files changed, 79 insertions(+), 3 deletions(-)
> 
> This continued to lack compat mode support (i.e. modification to
> xen/arch/x86/x86_64/compat/mm.c:compat_arch_memory_op()).

Do we need it? Only 64-bit kernels can use PVH - and that was from the start the
idea.


> 
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -4268,7 +4268,8 @@ static int handle_iomem_range(unsigned long s, unsigned long e, void *p)
> >  
> >  static int xenmem_add_to_physmap_once(
> >      struct domain *d,
> > -    const struct xen_add_to_physmap *xatp)
> > +    const struct xen_add_to_physmap *xatp,
> > +    domid_t foreign_domid)
> 
> So you add this new parameter but don't use it?
> 
> >  {
> >      struct page_info *page = NULL;
> >      unsigned long gfn = 0; /* gcc ... */
> > @@ -4395,7 +4396,7 @@ static int xenmem_add_to_physmap(struct domain *d,
> >          start_xatp = *xatp;
> >          while ( xatp->size > 0 )
> >          {
> > -            rc = xenmem_add_to_physmap_once(d, xatp);
> > +            rc = xenmem_add_to_physmap_once(d, xatp, -1);
> 
> And if it indeed is being used, please use a proper DOMID_* value
> here.
> 
> >              if ( rc < 0 )
> >                  return rc;
> >  
> > @@ -4421,7 +4422,52 @@ static int xenmem_add_to_physmap(struct domain *d,
> >          return rc;
> >      }
> >  
> > -    return xenmem_add_to_physmap_once(d, xatp);
> > +    return xenmem_add_to_physmap_once(d, xatp, -1);
> 
> And here.
> 
> > +}
> > +
> > +static noinline int xenmem_add_to_physmap_range(struct domain *d,
> > +                                       struct xen_add_to_physmap_range *xatpr)
> > +{
> > +    int rc;
> > +
> > +    /* Process entries in reverse order to allow continuations */
> > +    while ( xatpr->size > 0 )
> > +    {
> > +        xen_ulong_t idx;
> > +        xen_pfn_t gpfn;
> > +        struct xen_add_to_physmap xatp;
> > +
> > +        rc = copy_from_guest_offset(&idx, xatpr->idxs, xatpr->size-1, 1);
> > +        if ( rc < 0 )
> > +            goto out;
> > +
> > +        rc = copy_from_guest_offset(&gpfn, xatpr->gpfns, xatpr->size-1, 1);
> > +        if ( rc < 0 )
> > +            goto out;
> > +
> > +        xatp.space = xatpr->space;
> > +        xatp.idx = idx;
> > +        xatp.gpfn = gpfn;
> > +        rc = xenmem_add_to_physmap_once(d, &xatp, xatpr->foreign_domid);
> > +
> > +        if (rc)
> > +            goto out;
> 
> That doesn't seem right, together with you apparently not using
> the "errs" array altogether.
> 
> > +
> > +        xatpr->size--;
> > +
> > +        /* Check for continuation if it's not the last interation */
> > +        if ( xatpr->size > 0 && hypercall_preempt_check() )
> > +        {
> > +            rc = -EAGAIN;
> > +            goto out;
> > +        }
> > +    }
> > +
> > +    rc = 0;
> > +
> > +out:
> > +    return rc;
> > +
> >  }
> >  
> >  long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
> > @@ -4438,6 +4484,10 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
> >          if ( copy_from_guest(&xatp, arg, 1) )
> >              return -EFAULT;
> >  
> > +        /* This one is only supported for add_to_physmap_range */
> > +        if ( xatp.space == XENMAPSPACE_gmfn_foreign )
> > +            return -EINVAL;
> > +
> >          d = rcu_lock_domain_by_any_id(xatp.domid);
> >          if ( d == NULL )
> >              return -ESRCH;
> > @@ -4465,6 +4515,32 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
> >          return rc;
> >      }
> >  
> > +    case XENMEM_add_to_physmap_range:
> > +    {
> > +        struct xen_add_to_physmap_range xatpr;
> > +        struct domain *d;
> > +
> > +        if ( copy_from_guest(&xatpr, arg, 1) )
> > +            return -EFAULT;
> > +
> > +        rc = rcu_lock_target_domain_by_id(xatpr.domid, &d);
> > +        if ( rc != 0 )
> > +            return rc;
> > +
> > +        rc = xenmem_add_to_physmap_range(d, &xatpr);
> > +
> > +        rcu_unlock_domain(d);
> > +
> > +        if ( rc && copy_to_guest(arg, &xatpr, 1) )
> 
> For one, shouldn't this be "!rc"?
> 
> And then you update ->size, but that one is specified to be only
> and IN field. And considering that "errs" is the only OUT one, yet
> that isn't even formally correct (because the field itself is an IN,
> its what it points to where the output goes), I don't see why you

The 'err' is not formally correct? The memory.h says:

258     /* OUT */
259 
260     /* Per index error code. */
261     XEN_GUEST_HANDLE(int) errs;
262 };


or are you referring to 'size' which I agree with - it is part of
'IN'.

> would need to copy back any part of the structure.
> 
> Jan
> 
> > +            rc = -EFAULT;
> > +
> > +        if ( rc == -EAGAIN )
> > +            rc = hypercall_create_continuation(
> > +                __HYPERVISOR_memory_op, "ih", op, arg);
> > +
> > +        return rc;
> > +    }
> > +
> >      case XENMEM_set_memory_map:
> >      {
> >          struct xen_foreign_memory_map fmap;
> > -- 
> > 1.7.2.3
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org 
> > http://lists.xen.org/xen-devel 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>