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

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

[Mukesh Rathor]

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(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d00d9a2..6603752 100644
--- 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)
 {
     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);
             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);
+}
+
+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;
+
+        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) )
+            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

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

[Jan Beulich]

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

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

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

[Konrad Rzeszutek Wilk]

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
> 

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

[Jan Beulich]

>>> On 18.03.13 at 21:15, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 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.

I wasn't really aware of that, but I certainly do mind implementing
a hypercall usable by a PV or HVM guest only half way. Or did I
overlook code refusing the particular sub-op for plain PV and plain
HVM guests?

Jan

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

[Konrad Rzeszutek Wilk]

On Tue, Mar 19, 2013 at 08:40:06AM +0000, Jan Beulich wrote:
> >>> On 18.03.13 at 21:15, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > 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.
> 
> I wasn't really aware of that, but I certainly do mind implementing
> a hypercall usable by a PV or HVM guest only half way. Or did I
> overlook code refusing the particular sub-op for plain PV and plain
> HVM guests?

There were some patches in the series (I can't recall the names of them
thought) that had some of the PV hypercalls return -EINVAL (or -ENOSYS
as I suggested).

I think it might be better to introduce one patch that would neuter
the PV/HVM hypercalls that we don't want to support or can't yet
(for example b/c we haven't fixed the bugs).

Are you OK if those paths are done in the common code? Meaning say
for the PHYSDEV_op hypercalls some return -ENOSYS?

Or would doing it via the grand hypercall[0->X] table that was in
one of the patches and filtering the unsafe hypercalls? That seems
a much easier way and won't mess with the generic code paths?


> 
> Jan
> 

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

[Jan Beulich]

>>> On 19.03.13 at 14:40, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> There were some patches in the series (I can't recall the names of them
> thought) that had some of the PV hypercalls return -EINVAL (or -ENOSYS
> as I suggested).
> 
> I think it might be better to introduce one patch that would neuter
> the PV/HVM hypercalls that we don't want to support or can't yet
> (for example b/c we haven't fixed the bugs).
> 
> Are you OK if those paths are done in the common code? Meaning say
> for the PHYSDEV_op hypercalls some return -ENOSYS?

If intended to be permanent, then of course.

If intended to be temporary (until fixed), then yes as long as an
accompanying comment makes this clear (and greppable for).

Jan

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

[Mukesh Rathor]

On Mon, 18 Mar 2013 11:38:35 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 16.03.13 at 01:20, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > +        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
> would need to copy back any part of the structure.

Ah, I see the struct got updated. Konrad, do you have updated version
of the struct with following added to the end:

struct xen_add_to_physmap_range {
....
    /* OUT */
        
    /* Per index error code. */
    XEN_GUEST_HANDLE(int) errs;

in the latest linux tree?

thanks,
Mukesh

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

[Konrad Rzeszutek Wilk]

On Wed, Mar 20, 2013 at 06:01:39PM -0700, Mukesh Rathor wrote:
> On Mon, 18 Mar 2013 11:38:35 +0000
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
> > >>> On 16.03.13 at 01:20, Mukesh Rathor <mukesh.rathor@oracle.com>
> > >>> wrote:
> > > +        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
> > would need to copy back any part of the structure.
> 
> Ah, I see the struct got updated. Konrad, do you have updated version
> of the struct with following added to the end:
> 
> struct xen_add_to_physmap_range {
> ....
>     /* OUT */
>         
>     /* Per index error code. */
>     XEN_GUEST_HANDLE(int) errs;
> 
> in the latest linux tree?

Yes. 5caed269ea867f36225376a6546411ed7c106226
 xen: implement updated XENMEM_add_to_physmap_range ABI
    
    Allows for more fine grained error reporting. Only used by PVH and
    ARM both of which are marked EXPERIMENTAL precisely because the ABI
    is not yet stable
    

> 
> thanks,
> Mukesh
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

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

[Konrad Rzeszutek Wilk]

On Fri, Mar 15, 2013 at 05:20:18PM -0700, Mukesh Rathor 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(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index d00d9a2..6603752 100644
> --- 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)
>  {
>      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);
>              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);
> +}
> +
> +static noinline int xenmem_add_to_physmap_range(struct domain *d,
> +                                       struct xen_add_to_physmap_range *xatpr)

noinline?

> +{
> +    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;
> +

Should you also set?
	xatp.err = 0;

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

And in case of error, like this, se xatp.err to rc?

> +    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) )
> +            rc = -EFAULT;

That is a bit odd. You are only copying in the values in case
of error? But what if the kernel gave you an 'xatpr' that has
garbage for the xatp.err field (which is marked as OUT). Should
we do the copy_to_guest irrespective of the rc?

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

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

[Konrad Rzeszutek Wilk]

On Fri, Mar 15, 2013 at 05:20:18PM -0700, Mukesh Rathor 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.

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

You could also add in the comment section here:

XENMAPSPACE_gmfn_foreign should never be used in XENMEM_add_to_physmap
hypercall. Before this patch we would wound down to
xenmem_add_to_physmap_once which would return -EINVAL (as mfn=0).

Now we do it straight away by returning -EINVAL.

> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
>  xen/arch/x86/mm.c |   82 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 79 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index d00d9a2..6603752 100644
> --- 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)
>  {
>      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);
>              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);
> +}
> +
> +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;
> +
> +        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) )
> +            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
> 

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

[Jan Beulich]

>>> On 18.03.13 at 15:28, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Fri, Mar 15, 2013 at 05:20:18PM -0700, Mukesh Rathor 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.
> 
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

I'm confused: A little earlier you sent a response raising several
issues, and now you send a Reviewed-by?

Jan

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

[Konrad Rzeszutek Wilk]

On Mon, Mar 18, 2013 at 03:25:51PM +0000, Jan Beulich wrote:
> >>> On 18.03.13 at 15:28, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > On Fri, Mar 15, 2013 at 05:20:18PM -0700, Mukesh Rathor 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.
> > 
> > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> I'm confused: A little earlier you sent a response raising several
> issues, and now you send a Reviewed-by?

Sorry, I looked at it a first and it looked OK, so I did the Reviewed-by.

Then looked at some other patches, some cog in my brain did a late page-fault
on this patch and I went back in here. Then realized that there certain
things I was not sure off and asked Mukesh.

So, Reviewed-by rescinded for this patch right now. Thanks for catching that
process error.

> 
> Jan
> 

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

[Tim Deegan]

At 17:20 -0700 on 15 Mar (1363368018), Mukesh Rathor wrote:
> +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 )

copy_from_guest_offset() returns the number of bytes that could not be
copied, which would never be < 0.

> +            goto out;
> +
> +        rc = copy_from_guest_offset(&gpfn, xatpr->gpfns, xatpr->size-1, 1);
> +        if ( rc < 0 )

Ditto.

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

Whitespace missing.

> +            goto out;
> +
> +        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)
> @@ -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;

There should be an XSM hook here somewhere.

> +        rc = xenmem_add_to_physmap_range(d, &xatpr);
> +
> +        rcu_unlock_domain(d);
> +
> +        if ( rc && copy_to_guest(arg, &xatpr, 1) )
> +            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