Re: [Xen-devel] [PATCH v9 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table

2017-10-11 Thread Jan Beulich 
>>> On 11.10.17 at 11:54,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: 11 October 2017 10:43
>> Hmm, that would be an option, but I'd prefer if we could get away
>> without. And no, I wasn't suggesting to introduce yet another
>> hypercall. Instead how about the handle being a null one asking
>> for the implementation limit to be returned in nr_frames (or, to
>> keep that IN only, in the re-purposed pad field)?
>> 
> 
> Ok, I'll make nr_frames IN/OUT. I guess I could define it to be set to the 
> implementation limit if the hypercall returns -E2BIG.

Sure, but please don't make this the _only_ way to find out the limit.

Jan


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

Re: [Xen-devel] [PATCH v9 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table

2017-10-11 Thread Paul Durrant 


> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 11 October 2017 10:43
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: Andrew Cooper <andrew.coop...@citrix.com>; George Dunlap
> <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Wei Liu
> <wei.l...@citrix.com>; Stefano Stabellini <sstabell...@kernel.org>; xen-
> de...@lists.xenproject.org; KonradRzeszutek Wilk
> <konrad.w...@oracle.com>; Tim (Xen.org) <t...@xen.org>
> Subject: RE: [Xen-devel] [PATCH v9 10/11] common: add a new mappable
> resource type: XENMEM_resource_grant_table
> 
> >>> On 11.10.17 at 10:54, <paul.durr...@citrix.com> wrote:
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: 11 October 2017 09:47
> >> >>> On 10.10.17 at 18:01, <paul.durr...@citrix.com> wrote:
> >> >> > @@ -993,6 +1018,11 @@ static int acquire_resource(const
> >> >> xen_mem_acquire_resource_t *xmar)
> >> >> >   xmar->nr_frames, mfn_list);
> >> >> >  break;
> >> >> >
> >> >> > +case XENMEM_resource_grant_table:
> >> >> > +rc = acquire_grant_table(d, xmar->id, xmar->frame,
> >> >> > + xmar->nr_frames, mfn_list);
> >> >> > +break;
> >> >>
> >> >> Is this really generally useful with mfn_list[] having just two entries?
> >> >>
> >> >
> >> > Good point. I'll increase the size of the array in this patch (to the
> >> > default table size of 32... I think that's a reasonable value to choose).
> >>
> >> I suppose for the only current use you have for this (seeding the
> >> grant table from the tool stack) even the two entries you have
> >> right now would suffice. If, however, a full grant table is supposed
> >> to be accessible this way, I can't see how a static upper limit will do.
> >> Or if you intend the caller to do multiple invocations in such a case,
> >> there ought to be a way to find out the (implementation) limit.
> >
> > I'm open to ideas but there clearly needs to be some sort of upper limit, or
> > we do away with being able to map multiple frames in a single invocation.
> The
> > dm_op hypercalls currently have a similar upper limit on the size of the
> > buffer array. I'd rather not have to introduce another hypercall just to 
> > find
> > out such a thing. It's a tools-only hypercall so could I not just add a
> > comment on what the limit currently is?
> 
> Hmm, that would be an option, but I'd prefer if we could get away
> without. And no, I wasn't suggesting to introduce yet another
> hypercall. Instead how about the handle being a null one asking
> for the implementation limit to be returned in nr_frames (or, to
> keep that IN only, in the re-purposed pad field)?
> 

Ok, I'll make nr_frames IN/OUT. I guess I could define it to be set to the 
implementation limit if the hypercall returns -E2BIG.

  Paul

> Jan


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

Re: [Xen-devel] [PATCH v9 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table

2017-10-11 Thread Jan Beulich 
>>> On 11.10.17 at 10:54,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: 11 October 2017 09:47
>> >>> On 10.10.17 at 18:01,  wrote:
>> >> > @@ -993,6 +1018,11 @@ static int acquire_resource(const
>> >> xen_mem_acquire_resource_t *xmar)
>> >> >   xmar->nr_frames, mfn_list);
>> >> >  break;
>> >> >
>> >> > +case XENMEM_resource_grant_table:
>> >> > +rc = acquire_grant_table(d, xmar->id, xmar->frame,
>> >> > + xmar->nr_frames, mfn_list);
>> >> > +break;
>> >>
>> >> Is this really generally useful with mfn_list[] having just two entries?
>> >>
>> >
>> > Good point. I'll increase the size of the array in this patch (to the
>> > default table size of 32... I think that's a reasonable value to choose).
>> 
>> I suppose for the only current use you have for this (seeding the
>> grant table from the tool stack) even the two entries you have
>> right now would suffice. If, however, a full grant table is supposed
>> to be accessible this way, I can't see how a static upper limit will do.
>> Or if you intend the caller to do multiple invocations in such a case,
>> there ought to be a way to find out the (implementation) limit.
> 
> I'm open to ideas but there clearly needs to be some sort of upper limit, or 
> we do away with being able to map multiple frames in a single invocation. The 
> dm_op hypercalls currently have a similar upper limit on the size of the 
> buffer array. I'd rather not have to introduce another hypercall just to find 
> out such a thing. It's a tools-only hypercall so could I not just add a 
> comment on what the limit currently is?

Hmm, that would be an option, but I'd prefer if we could get away
without. And no, I wasn't suggesting to introduce yet another
hypercall. Instead how about the handle being a null one asking
for the implementation limit to be returned in nr_frames (or, to
keep that IN only, in the re-purposed pad field)?

Jan


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

Re: [Xen-devel] [PATCH v9 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table

2017-10-11 Thread Paul Durrant 
> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 11 October 2017 09:47
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: Andrew Cooper <andrew.coop...@citrix.com>; George Dunlap
> <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Wei Liu
> <wei.l...@citrix.com>; Stefano Stabellini <sstabell...@kernel.org>; xen-
> de...@lists.xenproject.org; KonradRzeszutek Wilk
> <konrad.w...@oracle.com>; Tim (Xen.org) <t...@xen.org>
> Subject: RE: [Xen-devel] [PATCH v9 10/11] common: add a new mappable
> resource type: XENMEM_resource_grant_table
> 
> >>> On 10.10.17 at 18:01, <paul.durr...@citrix.com> wrote:
> >> > @@ -3795,6 +3807,18 @@ int gnttab_map_frame(struct domain *d,
> >> unsigned long idx, gfn_t gfn,
> >> >  return rc;
> >> >  }
> >> >
> >> > +int gnttab_get_frame(struct domain *d, unsigned long idx, mfn_t
> *mfn)
> >>
> >> const struct domain * (I realize now that the same should have
> >> been done for gnttab_map_frame() when it was introduced;
> >> perhaps you could change that at the same time).
> >
> > Again, the problem is that grow_table and functions it calls don't take a
> > const pointer. I tried cascading the const through to the underlying 
> > function
> > but the patch started to balloon so I think such work should be deferred.
> 
> Right, I had overlooked that. And it won't work, as
> share_xen_page_with_guest() can't be passed a const pointer.
> 
> >> > @@ -993,6 +1018,11 @@ static int acquire_resource(const
> >> xen_mem_acquire_resource_t *xmar)
> >> >   xmar->nr_frames, mfn_list);
> >> >  break;
> >> >
> >> > +case XENMEM_resource_grant_table:
> >> > +rc = acquire_grant_table(d, xmar->id, xmar->frame,
> >> > + xmar->nr_frames, mfn_list);
> >> > +break;
> >>
> >> Is this really generally useful with mfn_list[] having just two entries?
> >>
> >
> > Good point. I'll increase the size of the array in this patch (to the
> > default table size of 32... I think that's a reasonable value to choose).
> 
> I suppose for the only current use you have for this (seeding the
> grant table from the tool stack) even the two entries you have
> right now would suffice. If, however, a full grant table is supposed
> to be accessible this way, I can't see how a static upper limit will do.
> Or if you intend the caller to do multiple invocations in such a case,
> there ought to be a way to find out the (implementation) limit.

I'm open to ideas but there clearly needs to be some sort of upper limit, or we 
do away with being able to map multiple frames in a single invocation. The 
dm_op hypercalls currently have a similar upper limit on the size of the buffer 
array. I'd rather not have to introduce another hypercall just to find out such 
a thing. It's a tools-only hypercall so could I not just add a comment on what 
the limit currently is?

  Paul

> 
> Jan


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

Re: [Xen-devel] [PATCH v9 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table

2017-10-11 Thread Jan Beulich 
>>> On 10.10.17 at 18:01,  wrote:
>> > @@ -3795,6 +3807,18 @@ int gnttab_map_frame(struct domain *d,
>> unsigned long idx, gfn_t gfn,
>> >  return rc;
>> >  }
>> >
>> > +int gnttab_get_frame(struct domain *d, unsigned long idx, mfn_t *mfn)
>> 
>> const struct domain * (I realize now that the same should have
>> been done for gnttab_map_frame() when it was introduced;
>> perhaps you could change that at the same time).
> 
> Again, the problem is that grow_table and functions it calls don't take a 
> const pointer. I tried cascading the const through to the underlying function 
> but the patch started to balloon so I think such work should be deferred.

Right, I had overlooked that. And it won't work, as
share_xen_page_with_guest() can't be passed a const pointer.

>> > @@ -993,6 +1018,11 @@ static int acquire_resource(const
>> xen_mem_acquire_resource_t *xmar)
>> >   xmar->nr_frames, mfn_list);
>> >  break;
>> >
>> > +case XENMEM_resource_grant_table:
>> > +rc = acquire_grant_table(d, xmar->id, xmar->frame,
>> > + xmar->nr_frames, mfn_list);
>> > +break;
>> 
>> Is this really generally useful with mfn_list[] having just two entries?
>> 
> 
> Good point. I'll increase the size of the array in this patch (to the 
> default table size of 32... I think that's a reasonable value to choose).

I suppose for the only current use you have for this (seeding the
grant table from the tool stack) even the two entries you have
right now would suffice. If, however, a full grant table is supposed
to be accessible this way, I can't see how a static upper limit will do.
Or if you intend the caller to do multiple invocations in such a case,
there ought to be a way to find out the (implementation) limit.

Jan


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

Re: [Xen-devel] [PATCH v9 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table

2017-10-10 Thread Paul Durrant 
> -Original Message-
> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of Jan
> Beulich
> Sent: 10 October 2017 11:26
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: Stefano Stabellini <sstabell...@kernel.org>; Wei Liu
> <wei.l...@citrix.com>; KonradRzeszutek Wilk <konrad.w...@oracle.com>;
> George Dunlap <george.dun...@citrix.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Tim
> (Xen.org) <t...@xen.org>; xen-de...@lists.xenproject.org
> Subject: Re: [Xen-devel] [PATCH v9 10/11] common: add a new mappable
> resource type: XENMEM_resource_grant_table
> 
> >>> On 06.10.17 at 14:25, <paul.durr...@citrix.com> wrote:
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -3756,14 +3756,13 @@ int mem_sharing_gref_to_gfn(struct
> grant_table *gt, grant_ref_t ref,
> >  }
> >  #endif
> >
> > -int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> > - mfn_t *mfn)
> > +/* Caller must hold write lock as version may change and table may grow
> */
> > +static int gnttab_get_frame_locked(struct domain *d, unsigned long idx,
> > +   mfn_t *mfn)
> 
> I don't think this helper function needs to be passed d; gt appears
> to suffice.

Alas it does not. It calls through to gnttab_grow_table() which requires the 
domain.

> 
> > @@ -3787,6 +3786,19 @@ int gnttab_map_frame(struct domain *d,
> unsigned long idx, gfn_t gfn,
> >  rc = -EINVAL;
> >  }
> >
> > +return rc;
> > +}
> > +
> > +int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> > + mfn_t *mfn)
> > +{
> > +struct grant_table *gt = d->grant_table;
> > +int rc;
> > +
> > +grant_write_lock(gt);
> > +
> > +rc = gnttab_get_frame_locked(d, idx, mfn);
> > +
> >  if ( !rc )
> >  gnttab_set_frame_gfn(gt, idx, gfn);
> >
> > @@ -3795,6 +3807,18 @@ int gnttab_map_frame(struct domain *d,
> unsigned long idx, gfn_t gfn,
> >  return rc;
> >  }
> >
> > +int gnttab_get_frame(struct domain *d, unsigned long idx, mfn_t *mfn)
> 
> const struct domain * (I realize now that the same should have
> been done for gnttab_map_frame() when it was introduced;
> perhaps you could change that at the same time).

Again, the problem is that grow_table and functions it calls don't take a const 
pointer. I tried cascading the const through to the underlying function but the 
patch started to balloon so I think such work should be deferred.

> 
> > @@ -966,6 +967,30 @@ static long xatp_permission_check(struct domain
> *d,
> > unsigned int space)
> >  }
> >
> >  #ifdef CONFIG_X86
> > +static int acquire_grant_table(struct domain *d, unsigned int id,
> 
> This clearly isn't x86-specific code.

Ok.

> 
> > +   unsigned long frame,
> > +   unsigned long nr_frames,
> > +   unsigned long mfn_list[])
> > +{
> > +unsigned int i = nr_frames;
> 
> Silent truncation just like in the earlier patch.

Yes, nr_frames should be an unsigned int.

> 
> > +if ( id != 0 )
> 
> Do we want a constant here again? Also acquiring the status page
> MFNs via separate ID would seem better than re-using
> XENMAPIDX_grant_table_status here, the more that this uses bit
> 31 while right now your interface structure field is still 64 bits wide.
> 

Ok, that's a better way to do it.

> > @@ -993,6 +1018,11 @@ static int acquire_resource(const
> xen_mem_acquire_resource_t *xmar)
> >   xmar->nr_frames, mfn_list);
> >  break;
> >
> > +case XENMEM_resource_grant_table:
> > +rc = acquire_grant_table(d, xmar->id, xmar->frame,
> > + xmar->nr_frames, mfn_list);
> > +break;
> 
> Is this really generally useful with mfn_list[] having just two entries?
> 

Good point. I'll increase the size of the array in this patch (to the default 
table size of 32... I think that's a reasonable value to choose).

> Jan
> 
> 
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v9 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table

2017-10-10 Thread Jan Beulich 
>>> On 06.10.17 at 14:25,  wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3756,14 +3756,13 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, 
> grant_ref_t ref,
>  }
>  #endif
>  
> -int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> - mfn_t *mfn)
> +/* Caller must hold write lock as version may change and table may grow */
> +static int gnttab_get_frame_locked(struct domain *d, unsigned long idx,
> +   mfn_t *mfn)

I don't think this helper function needs to be passed d; gt appears
to suffice.

> @@ -3787,6 +3786,19 @@ int gnttab_map_frame(struct domain *d, unsigned long 
> idx, gfn_t gfn,
>  rc = -EINVAL;
>  }
>  
> +return rc;
> +}
> +
> +int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> + mfn_t *mfn)
> +{
> +struct grant_table *gt = d->grant_table;
> +int rc;
> +
> +grant_write_lock(gt);
> +
> +rc = gnttab_get_frame_locked(d, idx, mfn);
> +
>  if ( !rc )
>  gnttab_set_frame_gfn(gt, idx, gfn);
>  
> @@ -3795,6 +3807,18 @@ int gnttab_map_frame(struct domain *d, unsigned long 
> idx, gfn_t gfn,
>  return rc;
>  }
>  
> +int gnttab_get_frame(struct domain *d, unsigned long idx, mfn_t *mfn)

const struct domain * (I realize now that the same should have
been done for gnttab_map_frame() when it was introduced;
perhaps you could change that at the same time).

> @@ -966,6 +967,30 @@ static long xatp_permission_check(struct domain *d, 
> unsigned int space)
>  }
>  
>  #ifdef CONFIG_X86
> +static int acquire_grant_table(struct domain *d, unsigned int id,

This clearly isn't x86-specific code.

> +   unsigned long frame,
> +   unsigned long nr_frames,
> +   unsigned long mfn_list[])
> +{
> +unsigned int i = nr_frames;

Silent truncation just like in the earlier patch.

> +if ( id != 0 )

Do we want a constant here again? Also acquiring the status page
MFNs via separate ID would seem better than re-using
XENMAPIDX_grant_table_status here, the more that this uses bit
31 while right now your interface structure field is still 64 bits wide.

> @@ -993,6 +1018,11 @@ static int acquire_resource(const 
> xen_mem_acquire_resource_t *xmar)
>   xmar->nr_frames, mfn_list);
>  break;
>  
> +case XENMEM_resource_grant_table:
> +rc = acquire_grant_table(d, xmar->id, xmar->frame,
> + xmar->nr_frames, mfn_list);
> +break;

Is this really generally useful with mfn_list[] having just two entries?

Jan


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

[Xen-devel] [PATCH v9 10/11] common: add a new mappable resource type: XENMEM_resource_grant_table

2017-10-06 Thread Paul Durrant 
This patch allows grant table frames to be mapped using the
XENMEM_acquire_resource memory op.

Signed-off-by: Paul Durrant 
---
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Jan Beulich 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Tim Deegan 
Cc: Wei Liu 

v8:
 - The functionality was originally incorporated into the earlier patch
   "x86/mm: add HYPERVISOR_memory_op to acquire guest resources".
---
 xen/common/grant_table.c  | 32 
 xen/common/memory.c   | 30 ++
 xen/include/public/memory.h   |  5 +
 xen/include/xen/grant_table.h |  1 +
 4 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 6d20b17739..12623c9984 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3756,14 +3756,13 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, 
grant_ref_t ref,
 }
 #endif
 
-int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
- mfn_t *mfn)
+/* Caller must hold write lock as version may change and table may grow */
+static int gnttab_get_frame_locked(struct domain *d, unsigned long idx,
+   mfn_t *mfn)
 {
 int rc = 0;
 struct grant_table *gt = d->grant_table;
 
-grant_write_lock(gt);
-
 if ( gt->gt_version == 0 )
 gt->gt_version = 1;
 
@@ -3787,6 +3786,19 @@ int gnttab_map_frame(struct domain *d, unsigned long 
idx, gfn_t gfn,
 rc = -EINVAL;
 }
 
+return rc;
+}
+
+int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
+ mfn_t *mfn)
+{
+struct grant_table *gt = d->grant_table;
+int rc;
+
+grant_write_lock(gt);
+
+rc = gnttab_get_frame_locked(d, idx, mfn);
+
 if ( !rc )
 gnttab_set_frame_gfn(gt, idx, gfn);
 
@@ -3795,6 +3807,18 @@ int gnttab_map_frame(struct domain *d, unsigned long 
idx, gfn_t gfn,
 return rc;
 }
 
+int gnttab_get_frame(struct domain *d, unsigned long idx, mfn_t *mfn)
+{
+struct grant_table *gt = d->grant_table;
+int rc;
+
+grant_write_lock(gt); /* write lock is required as table may grow */
+rc = gnttab_get_frame_locked(d, idx, mfn);
+grant_write_unlock(gt);
+
+return rc;
+}
+
 static void gnttab_usage_print(struct domain *rd)
 {
 int first = 1;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 80a3f42875..c4e175da83 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -966,6 +967,30 @@ static long xatp_permission_check(struct domain *d, 
unsigned int space)
 }
 
 #ifdef CONFIG_X86
+static int acquire_grant_table(struct domain *d, unsigned int id,
+   unsigned long frame,
+   unsigned long nr_frames,
+   unsigned long mfn_list[])
+{
+unsigned int i = nr_frames;
+
+if ( id != 0 )
+return -EINVAL;
+
+while ( i-- != 0 )
+{
+mfn_t mfn = INVALID_MFN;
+int rc = gnttab_get_frame(d, frame + i, );
+
+if ( rc )
+return rc;
+
+mfn_list[i] = mfn_x(mfn);
+}
+
+return 0;
+}
+
 static int acquire_resource(const xen_mem_acquire_resource_t *xmar)
 {
 struct domain *d, *currd = current->domain;
@@ -993,6 +1018,11 @@ static int acquire_resource(const 
xen_mem_acquire_resource_t *xmar)
  xmar->nr_frames, mfn_list);
 break;
 
+case XENMEM_resource_grant_table:
+rc = acquire_grant_table(d, xmar->id, xmar->frame,
+ xmar->nr_frames, mfn_list);
+break;
+
 default:
 rc = -EOPNOTSUPP;
 break;
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index e30a4d9794..2099b09cf5 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -611,6 +611,7 @@ struct xen_mem_acquire_resource {
 uint16_t type;
 
 #define XENMEM_resource_ioreq_server 0
+#define XENMEM_resource_grant_table 1
 
 /*
  * IN - a type-specific resource identifier, which must be zero
@@ -628,6 +629,10 @@ struct xen_mem_acquire_resource {
  *   page
  * frame == 1 -> ioreq
  *   page
+ * type == XENMEM_resource_grant_table -> frame has same semantics
+ *as idx passed to
+ *XENMEM_add_to_physmap for
+ *XENMAPSPACE_grant_table.
  */