Re: [Xen-devel] [PATCH v3 1/2] x86/mem-sharing: Bulk mem-sharing entire domains

2016-05-17 Thread Jan Beulich 
>>> On 13.05.16 at 18:29,  wrote:
> On Fri, May 13, 2016 at 10:12 AM, Jan Beulich  wrote:
> On 13.05.16 at 17:31,  wrote:
>>> On Fri, May 13, 2016 at 9:09 AM, Jan Beulich  wrote:
>>> On 13.05.16 at 16:50,  wrote:
> On Fri, May 13, 2016 at 6:00 AM, Jan Beulich  wrote:
> On 12.05.16 at 17:25,  wrote:
>>> @@ -1468,6 +1505,69 @@ int
>>> +if ( rc )
>>> +{
>>> +rcu_unlock_domain(cd);
>>> +goto out;
>>> +}
>>> +
>>> +if ( !mem_sharing_enabled(cd) )
>>> +{
>>> +rcu_unlock_domain(cd);
>>> +rc = -EINVAL;
>>> +goto out;
>>> +}
>>> +
>>> +if ( !atomic_read(>pause_count) ||
>>> + !atomic_read(>pause_count) )
>>> +{
>>> +rcu_unlock_domain(cd);
>>> +rc = -EINVAL;
>>> +goto out;
>>> +}
>>> +
>>> +max_sgfn = domain_get_maximum_gpfn(d);
>>> +max_cgfn = domain_get_maximum_gpfn(cd);
>>> +
>>> +if ( max_sgfn != max_cgfn || max_sgfn < mso.u.bulk.start )
>>
>> Why would the two domains need to agree in their maximum
>> GPFN? There's nothing similar in this file so far. Nor does the
>> right side of the || match anything pre-existing...
>
> The use-case for this function is to deduplicate identical VMs, not to
> blindly share pages across arbitrary domains. So this is a safety
> check to avoid accidentally running this function on domains that
> obviously are not identical. The right hand size is a safety check
> against not properly initialized input structs where the start point
> is obviously outside the memory of the domain.

 Is that use case the only possible one, or merely the one you care
 about? In the former case, I'd be okay as long as a respctive
 comment got added.
>>>
>>> I would say "blind" sharing like this only makes sense for identical
>>> VMs. Replacing the memory of the VM with that of another one not in
>>> the same state will lead to some spectacular crash for sure, so we
>>> should do at least some sanity checking before.
>>
>> Crash? Pages would be shared only if their contents match (and
>> unshared when they're about to diverge), and once that's
>> guaranteed, I don't see how it matters whether the two involved
>> guests are identical. I agree that between dissimilar guests the
>> potential for sharing is likely much lower, but that's about all.
> 
> No, that's not how it works. Xen's mem_sharing doesn't do _any_ checks
> on the contents of the pages before sharing. It's up to the user to
> know what he is doing.

Ok, okay - that explains to me why this feature is rarely used and
not fully supported.

Jan


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

Re: [Xen-devel] [PATCH v3 1/2] x86/mem-sharing: Bulk mem-sharing entire domains

2016-05-13 Thread Tamas K Lengyel 
On Fri, May 13, 2016 at 10:12 AM, Jan Beulich  wrote:
 On 13.05.16 at 17:31,  wrote:
>> On Fri, May 13, 2016 at 9:09 AM, Jan Beulich  wrote:
>> On 13.05.16 at 16:50,  wrote:
 On Fri, May 13, 2016 at 6:00 AM, Jan Beulich  wrote:
 On 12.05.16 at 17:25,  wrote:
>> @@ -1468,6 +1505,69 @@ int
>> +if ( rc )
>> +{
>> +rcu_unlock_domain(cd);
>> +goto out;
>> +}
>> +
>> +if ( !mem_sharing_enabled(cd) )
>> +{
>> +rcu_unlock_domain(cd);
>> +rc = -EINVAL;
>> +goto out;
>> +}
>> +
>> +if ( !atomic_read(>pause_count) ||
>> + !atomic_read(>pause_count) )
>> +{
>> +rcu_unlock_domain(cd);
>> +rc = -EINVAL;
>> +goto out;
>> +}
>> +
>> +max_sgfn = domain_get_maximum_gpfn(d);
>> +max_cgfn = domain_get_maximum_gpfn(cd);
>> +
>> +if ( max_sgfn != max_cgfn || max_sgfn < mso.u.bulk.start )
>
> Why would the two domains need to agree in their maximum
> GPFN? There's nothing similar in this file so far. Nor does the
> right side of the || match anything pre-existing...

 The use-case for this function is to deduplicate identical VMs, not to
 blindly share pages across arbitrary domains. So this is a safety
 check to avoid accidentally running this function on domains that
 obviously are not identical. The right hand size is a safety check
 against not properly initialized input structs where the start point
 is obviously outside the memory of the domain.
>>>
>>> Is that use case the only possible one, or merely the one you care
>>> about? In the former case, I'd be okay as long as a respctive
>>> comment got added.
>>
>> I would say "blind" sharing like this only makes sense for identical
>> VMs. Replacing the memory of the VM with that of another one not in
>> the same state will lead to some spectacular crash for sure, so we
>> should do at least some sanity checking before.
>
> Crash? Pages would be shared only if their contents match (and
> unshared when they're about to diverge), and once that's
> guaranteed, I don't see how it matters whether the two involved
> guests are identical. I agree that between dissimilar guests the
> potential for sharing is likely much lower, but that's about all.

No, that's not how it works. Xen's mem_sharing doesn't do _any_ checks
on the contents of the pages before sharing. It's up to the user to
know what he is doing.

>
>> @@ -488,7 +489,18 @@ struct xen_mem_sharing_op {
>>  uint64_aligned_t client_gfn;/* IN: the client gfn */
>>  uint64_aligned_t client_handle; /* IN: handle to the client 
>> page */
>>  domid_t  client_domain; /* IN: the client domain id */
>> -} share;
>> +} share;
>> +struct mem_sharing_op_bulk { /* OP_BULK_SHARE */
>> +uint64_aligned_t start;  /* IN: start gfn. Set to 0 
>> for
>> +full deduplication. 
>> Field is
>> +used internally and may 
>> change
>> +when the hypercall 
>> returns. */
>> +uint64_aligned_t shared; /* OUT: the number of gfns
>> +that are shared after 
>> this
>> +operation including 
>> pages
>> +already shared before */
>> +domid_t client_domain;   /* IN: the client domain 
>> id */
>> +} bulk;
>
> Let's not repeat pre-existing mistakes: There is explicit padding
> missing here, which then also ought to be checked to be zero on
> input.

 This struct is part of a union and is smaller then largest struct in
 the union, even with padding. So how would padding have any effect on
 anything?
>>>
>>> Being able to use the space for future extensions without having
>>> to bump the interface version. In domctl-s it's not as important
>>> due to them being separately versioned, but anyway.
>>
>> I don't really follow, we are not growing the union here. This struct
>> is still smaller then the space available in the union, so what would
>> prevent us from later on expending this struct to the size of the
>> union without padding?
>
> If a future hypervisor expects some value in a field that's now
> (anonymous) padding, and a caller written

Re: [Xen-devel] [PATCH v3 1/2] x86/mem-sharing: Bulk mem-sharing entire domains

2016-05-13 Thread Jan Beulich 
>>> On 13.05.16 at 17:35,  wrote:
> On 05/13/2016 11:09 AM, Jan Beulich wrote:
> On 13.05.16 at 16:50,  wrote:
> [...]
> @@ -1468,6 +1505,69 @@ int 
> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>   }
>   break;
>
> +case XENMEM_sharing_op_bulk_share:
> +{
> +unsigned long max_sgfn, max_cgfn;
> +struct domain *cd;
> +
> +rc = -EINVAL;
> +if ( !mem_sharing_enabled(d) )
> +goto out;
> +
> +rc = 
> rcu_lock_live_remote_domain_by_id(mso.u.bulk.client_domain,
> +   );
> +if ( rc )
> +goto out;
> +
> +rc = xsm_mem_sharing_op(XSM_DM_PRIV, d, cd, mso.op);

 Either you pass XENMEM_sharing_op_share here, or you need to
 update xen/xsm/flask/policy/access_vectors (even if it's only a
 comment which needs updating).
>>>
>>> Right, it should actually be sharing_op_share here.
>>>

 That said - are this and the similar pre-existing XSM checks actually
 correct? I.e. is one of the two domains here really controlling the
 other? I would have expected that a tool stack domain initiates the
 sharing between two domains it controls...
>>>
>>> Not sure what was the original rationale behind it either.
>>
>> Daniel - any opinion on this one?
> 
> This hook checks two permissions; the primary check is that current (which
> is not either argument) can perform HVM__MEM_SHARING on (cd).  When XSM is
> disabled, this is checked as device model permissions.  I don't think this
> is what you were asking about, because this is actually a control operation.
> 
> The other permission check invoked by this hook, only when XSM is enabled,
> is a check for HVM__SHARE_MEM between (d) and (cd).  This is to allow a
> security policy to be written that forbids memory sharing between different
> users but allow it between VMs belonging to a single user (as an example).

Ah, I see - I missed the use of current->domain. But the asymmetry
still seems odd: In a sharing operation, both domains are equally
affected, and hence current->domain should have control over both,
and the second check should be done both ways (unless
domain_has_perm()'s first two arguments are treated equally, which
it doesn't look like is the case).

Jan


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

Re: [Xen-devel] [PATCH v3 1/2] x86/mem-sharing: Bulk mem-sharing entire domains

2016-05-13 Thread Jan Beulich 
>>> On 13.05.16 at 17:31,  wrote:
> On Fri, May 13, 2016 at 9:09 AM, Jan Beulich  wrote:
> On 13.05.16 at 16:50,  wrote:
>>> On Fri, May 13, 2016 at 6:00 AM, Jan Beulich  wrote:
>>> On 12.05.16 at 17:25,  wrote:
> @@ -1468,6 +1505,69 @@ int 
> +if ( rc )
> +{
> +rcu_unlock_domain(cd);
> +goto out;
> +}
> +
> +if ( !mem_sharing_enabled(cd) )
> +{
> +rcu_unlock_domain(cd);
> +rc = -EINVAL;
> +goto out;
> +}
> +
> +if ( !atomic_read(>pause_count) ||
> + !atomic_read(>pause_count) )
> +{
> +rcu_unlock_domain(cd);
> +rc = -EINVAL;
> +goto out;
> +}
> +
> +max_sgfn = domain_get_maximum_gpfn(d);
> +max_cgfn = domain_get_maximum_gpfn(cd);
> +
> +if ( max_sgfn != max_cgfn || max_sgfn < mso.u.bulk.start )

 Why would the two domains need to agree in their maximum
 GPFN? There's nothing similar in this file so far. Nor does the
 right side of the || match anything pre-existing...
>>>
>>> The use-case for this function is to deduplicate identical VMs, not to
>>> blindly share pages across arbitrary domains. So this is a safety
>>> check to avoid accidentally running this function on domains that
>>> obviously are not identical. The right hand size is a safety check
>>> against not properly initialized input structs where the start point
>>> is obviously outside the memory of the domain.
>>
>> Is that use case the only possible one, or merely the one you care
>> about? In the former case, I'd be okay as long as a respctive
>> comment got added.
> 
> I would say "blind" sharing like this only makes sense for identical
> VMs. Replacing the memory of the VM with that of another one not in
> the same state will lead to some spectacular crash for sure, so we
> should do at least some sanity checking before.

Crash? Pages would be shared only if their contents match (and
unshared when they're about to diverge), and once that's
guaranteed, I don't see how it matters whether the two involved
guests are identical. I agree that between dissimilar guests the
potential for sharing is likely much lower, but that's about all.

> @@ -488,7 +489,18 @@ struct xen_mem_sharing_op {
>  uint64_aligned_t client_gfn;/* IN: the client gfn */
>  uint64_aligned_t client_handle; /* IN: handle to the client 
> page */
>  domid_t  client_domain; /* IN: the client domain id */
> -} share;
> +} share;
> +struct mem_sharing_op_bulk { /* OP_BULK_SHARE */
> +uint64_aligned_t start;  /* IN: start gfn. Set to 0 
> for
> +full deduplication. 
> Field is
> +used internally and may 
> change
> +when the hypercall 
> returns. */
> +uint64_aligned_t shared; /* OUT: the number of gfns
> +that are shared after 
> this
> +operation including pages
> +already shared before */
> +domid_t client_domain;   /* IN: the client domain id 
> */
> +} bulk;

 Let's not repeat pre-existing mistakes: There is explicit padding
 missing here, which then also ought to be checked to be zero on
 input.
>>>
>>> This struct is part of a union and is smaller then largest struct in
>>> the union, even with padding. So how would padding have any effect on
>>> anything?
>>
>> Being able to use the space for future extensions without having
>> to bump the interface version. In domctl-s it's not as important
>> due to them being separately versioned, but anyway.
> 
> I don't really follow, we are not growing the union here. This struct
> is still smaller then the space available in the union, so what would
> prevent us from later on expending this struct to the size of the
> union without padding?

If a future hypervisor expects some value in a field that's now
(anonymous) padding, and a caller written against the headers
with just your patch applied passes a structure with random
data in those padding fields to that new hypervisor, what do
you expect to happen?

Jan

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

Re: [Xen-devel] [PATCH v3 1/2] x86/mem-sharing: Bulk mem-sharing entire domains

2016-05-13 Thread Daniel De Graaf 

On 05/13/2016 11:09 AM, Jan Beulich wrote:

On 13.05.16 at 16:50,  wrote:

[...]

@@ -1468,6 +1505,69 @@ int 
mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
  }
  break;

+case XENMEM_sharing_op_bulk_share:
+{
+unsigned long max_sgfn, max_cgfn;
+struct domain *cd;
+
+rc = -EINVAL;
+if ( !mem_sharing_enabled(d) )
+goto out;
+
+rc = rcu_lock_live_remote_domain_by_id(mso.u.bulk.client_domain,
+   );
+if ( rc )
+goto out;
+
+rc = xsm_mem_sharing_op(XSM_DM_PRIV, d, cd, mso.op);


Either you pass XENMEM_sharing_op_share here, or you need to
update xen/xsm/flask/policy/access_vectors (even if it's only a
comment which needs updating).


Right, it should actually be sharing_op_share here.



That said - are this and the similar pre-existing XSM checks actually
correct? I.e. is one of the two domains here really controlling the
other? I would have expected that a tool stack domain initiates the
sharing between two domains it controls...


Not sure what was the original rationale behind it either.


Daniel - any opinion on this one?


This hook checks two permissions; the primary check is that current (which
is not either argument) can perform HVM__MEM_SHARING on (cd).  When XSM is
disabled, this is checked as device model permissions.  I don't think this
is what you were asking about, because this is actually a control operation.

The other permission check invoked by this hook, only when XSM is enabled,
is a check for HVM__SHARE_MEM between (d) and (cd).  This is to allow a
security policy to be written that forbids memory sharing between different
users but allow it between VMs belonging to a single user (as an example).

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

Re: [Xen-devel] [PATCH v3 1/2] x86/mem-sharing: Bulk mem-sharing entire domains

2016-05-13 Thread Tamas K Lengyel 
On Fri, May 13, 2016 at 9:09 AM, Jan Beulich  wrote:
 On 13.05.16 at 16:50,  wrote:
>> On Fri, May 13, 2016 at 6:00 AM, Jan Beulich  wrote:
>> On 12.05.16 at 17:25,  wrote:
 +if ( !rc )
 +mem_sharing_share_pages(d, bulk->start, sh, cd, 
 bulk->start, ch);
>>>
>>> You shouldn't be ignoring errors here.
>>
>> The only error this function returns is if the sh/ch handles are
>> invalid. However we obtained those just now from successful
>> nominations, so we are guaranteed to have valid handles. This error
>> checking is only important when nominations/sharing happen
>> independently where the handle may go stale in-between. Here that is
>> not possible.
>
> You describe the state of things right now. What if someone
> adds an error path to that function without inspecting all callers,
> as it is clear that the function could have returned errors before.
> Ignoring errors is plain bad. If you're sure there can't be errors,
> at least add a respective ASSERT().
>

Sure, that sounds like a reasonable middle ground.

 +}
 +
 +++(bulk->start);
>>>
>>> Pointless parentheses.
>>
>> Pointless but I prefer this style.
>
> But it's in contrast to what we do almost everywhere else ...
>

Alright..

 +/* Check for continuation if it's not the last iteration. */
 +if ( bulk->start < max && hypercall_preempt_check() )
>>>
>>> The loop head has <=; why < here?
>>
>> Because we only do preempt check if there are more then one pages left
>> (as the comment states).
>
> No, the comment says "last iteration", whereas the check is for
> "next to last" (as the increment has already happened). And I
> also don't see when between any two iterations preemption is
> possible, just not between the second from last and the last one.
>

Ah I see, yes you are right.

 +break;
 +}
 +}
 +
 +/* We only propagate -ENOMEM so reset rc here */
 +if ( rc < 0 && rc != -ENOMEM )
 +rc = 0;
>>>
>>> What's the rationale for discarding all other errors? At least the
>>> patch description, but perhaps even the comment (which btw
>>> is lacking a full stop) should be explaining this.
>>
>> The reason we swallow errors here other then ENOMEM is that it's quite
>> possible that max_gpfn page is unsharable for example, thus rc would
>> have an EINVAL final error value. However, we don't care about the
>> success/fail of individual pages, we only care about the overall
>> state. For that only ENOMEM is critical.
>
> And you think no possible caller would care about the hypercall
> reporting success yet not everything having got done that was
> requested? Sounds strange to me, but as said - at least a bold
> comment please.

The user has no way of knowing what pages will be sharable to begin
with, nor does he has any recourse when something doesn't share. The
request is thus not to "share everything" but to share "as much as
possible". I'll state this explicitly in a comment.

>
 @@ -1468,6 +1505,69 @@ int 
 mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
  }
  break;

 +case XENMEM_sharing_op_bulk_share:
 +{
 +unsigned long max_sgfn, max_cgfn;
 +struct domain *cd;
 +
 +rc = -EINVAL;
 +if ( !mem_sharing_enabled(d) )
 +goto out;
 +
 +rc = 
 rcu_lock_live_remote_domain_by_id(mso.u.bulk.client_domain,
 +   );
 +if ( rc )
 +goto out;
 +
 +rc = xsm_mem_sharing_op(XSM_DM_PRIV, d, cd, mso.op);
>>>
>>> Either you pass XENMEM_sharing_op_share here, or you need to
>>> update xen/xsm/flask/policy/access_vectors (even if it's only a
>>> comment which needs updating).
>>
>> Right, it should actually be sharing_op_share here.
>>
>>>
>>> That said - are this and the similar pre-existing XSM checks actually
>>> correct? I.e. is one of the two domains here really controlling the
>>> other? I would have expected that a tool stack domain initiates the
>>> sharing between two domains it controls...
>>
>> Not sure what was the original rationale behind it either.
>
> Daniel - any opinion on this one?
>
 +if ( rc )
 +{
 +rcu_unlock_domain(cd);
 +goto out;
 +}
 +
 +if ( !mem_sharing_enabled(cd) )
 +{
 +rcu_unlock_domain(cd);
 +rc = -EINVAL;
 +goto out;
 +}
 +
 +if ( !atomic_read(>pause_count) ||
 + !atomic_read(>pause_count) )
 +{
 +rcu_unlock_domain(cd);
 +

Re: [Xen-devel] [PATCH v3 1/2] x86/mem-sharing: Bulk mem-sharing entire domains

2016-05-13 Thread Jan Beulich 
>>> On 13.05.16 at 16:50,  wrote:
> On Fri, May 13, 2016 at 6:00 AM, Jan Beulich  wrote:
> On 12.05.16 at 17:25,  wrote:
>>> +if ( !rc )
>>> +mem_sharing_share_pages(d, bulk->start, sh, cd, 
>>> bulk->start, ch);
>>
>> You shouldn't be ignoring errors here.
> 
> The only error this function returns is if the sh/ch handles are
> invalid. However we obtained those just now from successful
> nominations, so we are guaranteed to have valid handles. This error
> checking is only important when nominations/sharing happen
> independently where the handle may go stale in-between. Here that is
> not possible.

You describe the state of things right now. What if someone
adds an error path to that function without inspecting all callers,
as it is clear that the function could have returned errors before.
Ignoring errors is plain bad. If you're sure there can't be errors,
at least add a respective ASSERT().

>>> +}
>>> +
>>> +++(bulk->start);
>>
>> Pointless parentheses.
> 
> Pointless but I prefer this style.

But it's in contrast to what we do almost everywhere else ...

>>> +/* Check for continuation if it's not the last iteration. */
>>> +if ( bulk->start < max && hypercall_preempt_check() )
>>
>> The loop head has <=; why < here?
> 
> Because we only do preempt check if there are more then one pages left
> (as the comment states).

No, the comment says "last iteration", whereas the check is for
"next to last" (as the increment has already happened). And I
also don't see when between any two iterations preemption is
possible, just not between the second from last and the last one.

>>> +break;
>>> +}
>>> +}
>>> +
>>> +/* We only propagate -ENOMEM so reset rc here */
>>> +if ( rc < 0 && rc != -ENOMEM )
>>> +rc = 0;
>>
>> What's the rationale for discarding all other errors? At least the
>> patch description, but perhaps even the comment (which btw
>> is lacking a full stop) should be explaining this.
> 
> The reason we swallow errors here other then ENOMEM is that it's quite
> possible that max_gpfn page is unsharable for example, thus rc would
> have an EINVAL final error value. However, we don't care about the
> success/fail of individual pages, we only care about the overall
> state. For that only ENOMEM is critical.

And you think no possible caller would care about the hypercall
reporting success yet not everything having got done that was
requested? Sounds strange to me, but as said - at least a bold
comment please.

>>> @@ -1468,6 +1505,69 @@ int 
>>> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>>>  }
>>>  break;
>>>
>>> +case XENMEM_sharing_op_bulk_share:
>>> +{
>>> +unsigned long max_sgfn, max_cgfn;
>>> +struct domain *cd;
>>> +
>>> +rc = -EINVAL;
>>> +if ( !mem_sharing_enabled(d) )
>>> +goto out;
>>> +
>>> +rc = 
>>> rcu_lock_live_remote_domain_by_id(mso.u.bulk.client_domain,
>>> +   );
>>> +if ( rc )
>>> +goto out;
>>> +
>>> +rc = xsm_mem_sharing_op(XSM_DM_PRIV, d, cd, mso.op);
>>
>> Either you pass XENMEM_sharing_op_share here, or you need to
>> update xen/xsm/flask/policy/access_vectors (even if it's only a
>> comment which needs updating).
> 
> Right, it should actually be sharing_op_share here.
> 
>>
>> That said - are this and the similar pre-existing XSM checks actually
>> correct? I.e. is one of the two domains here really controlling the
>> other? I would have expected that a tool stack domain initiates the
>> sharing between two domains it controls...
> 
> Not sure what was the original rationale behind it either.

Daniel - any opinion on this one?

>>> +if ( rc )
>>> +{
>>> +rcu_unlock_domain(cd);
>>> +goto out;
>>> +}
>>> +
>>> +if ( !mem_sharing_enabled(cd) )
>>> +{
>>> +rcu_unlock_domain(cd);
>>> +rc = -EINVAL;
>>> +goto out;
>>> +}
>>> +
>>> +if ( !atomic_read(>pause_count) ||
>>> + !atomic_read(>pause_count) )
>>> +{
>>> +rcu_unlock_domain(cd);
>>> +rc = -EINVAL;
>>> +goto out;
>>> +}
>>> +
>>> +max_sgfn = domain_get_maximum_gpfn(d);
>>> +max_cgfn = domain_get_maximum_gpfn(cd);
>>> +
>>> +if ( max_sgfn != max_cgfn || max_sgfn < mso.u.bulk.start )
>>
>> Why would the two domains need to agree in their maximum
>> GPFN? There's nothing similar in this file so far. Nor does the
>> right side of the || match anything pre-existing...
> 
> The use-case for this function is to deduplicate identical VMs, not to
> blindly share pages

Re: [Xen-devel] [PATCH v3 1/2] x86/mem-sharing: Bulk mem-sharing entire domains

2016-05-13 Thread Tamas K Lengyel 
On Fri, May 13, 2016 at 6:00 AM, Jan Beulich  wrote:
 On 12.05.16 at 17:25,  wrote:
>> --- a/xen/arch/x86/mm/mem_sharing.c
>> +++ b/xen/arch/x86/mm/mem_sharing.c
>> @@ -1294,6 +1294,43 @@ int relinquish_shared_pages(struct domain *d)
>>  return rc;
>>  }
>>
>> +static int bulk_share(struct domain *d, struct domain *cd, unsigned long 
>> max,
>> +  struct mem_sharing_op_bulk *bulk)
>> +{
>> +int rc;
>> +shr_handle_t sh, ch;
>> +
>> +while( bulk->start <= max )
>> +{
>> +rc = mem_sharing_nominate_page(d, bulk->start, 0, );
>> +if ( rc == -ENOMEM )
>> +break;
>> +if ( !rc )
>> +{
>> +rc = mem_sharing_nominate_page(cd, bulk->start, 0, );
>> +if ( rc == -ENOMEM )
>> +break;
>
> If we get to this break, how will the caller know that the first
> nomination succeeded but the second didn't? Or perhaps there
> is some undo logic missing here?

No, there is not. There really is no "unnominate" feature of memshare.
So even if the user is calling nominate manually from userspace it
won't have the option to unnominate the page so that error condition
is currently useless for the user.

>
>> +if ( !rc )
>> +mem_sharing_share_pages(d, bulk->start, sh, cd, 
>> bulk->start, ch);
>
> You shouldn't be ignoring errors here.

The only error this function returns is if the sh/ch handles are
invalid. However we obtained those just now from successful
nominations, so we are guaranteed to have valid handles. This error
checking is only important when nominations/sharing happen
independently where the handle may go stale in-between. Here that is
not possible.

>
>> +}
>> +
>> +++(bulk->start);
>
> Pointless parentheses.

Pointless but I prefer this style.

>
>> +/* Check for continuation if it's not the last iteration. */
>> +if ( bulk->start < max && hypercall_preempt_check() )
>
> The loop head has <=; why < here?

Because we only do preempt check if there are more then one pages left
(as the comment states).

>> +{
>> +rc = 1;
>
> I'd recommend using -ERESTART here, as we do elsewhere.
>

Ack.

>> +break;
>> +}
>> +}
>> +
>> +/* We only propagate -ENOMEM so reset rc here */
>> +if ( rc < 0 && rc != -ENOMEM )
>> +rc = 0;
>
> What's the rationale for discarding all other errors? At least the
> patch description, but perhaps even the comment (which btw
> is lacking a full stop) should be explaining this.

The reason we swallow errors here other then ENOMEM is that it's quite
possible that max_gpfn page is unsharable for example, thus rc would
have an EINVAL final error value. However, we don't care about the
success/fail of individual pages, we only care about the overall
state. For that only ENOMEM is critical.

>
>> @@ -1468,6 +1505,69 @@ int 
>> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>>  }
>>  break;
>>
>> +case XENMEM_sharing_op_bulk_share:
>> +{
>> +unsigned long max_sgfn, max_cgfn;
>> +struct domain *cd;
>> +
>> +rc = -EINVAL;
>> +if ( !mem_sharing_enabled(d) )
>> +goto out;
>> +
>> +rc = rcu_lock_live_remote_domain_by_id(mso.u.bulk.client_domain,
>> +   );
>> +if ( rc )
>> +goto out;
>> +
>> +rc = xsm_mem_sharing_op(XSM_DM_PRIV, d, cd, mso.op);
>
> Either you pass XENMEM_sharing_op_share here, or you need to
> update xen/xsm/flask/policy/access_vectors (even if it's only a
> comment which needs updating).

Right, it should actually be sharing_op_share here.

>
> That said - are this and the similar pre-existing XSM checks actually
> correct? I.e. is one of the two domains here really controlling the
> other? I would have expected that a tool stack domain initiates the
> sharing between two domains it controls...

Not sure what was the original rationale behind it either.

>
>> +if ( rc )
>> +{
>> +rcu_unlock_domain(cd);
>> +goto out;
>> +}
>> +
>> +if ( !mem_sharing_enabled(cd) )
>> +{
>> +rcu_unlock_domain(cd);
>> +rc = -EINVAL;
>> +goto out;
>> +}
>> +
>> +if ( !atomic_read(>pause_count) ||
>> + !atomic_read(>pause_count) )
>> +{
>> +rcu_unlock_domain(cd);
>> +rc = -EINVAL;
>> +goto out;
>> +}
>> +
>> +max_sgfn = domain_get_maximum_gpfn(d);
>> +max_cgfn = domain_get_maximum_gpfn(cd);
>> +
>> +if ( max_sgfn != max_cgfn || max_sgfn < mso.u.bulk.start )
>
> Why would the two domains need to agree in their maximum
> GPFN? There's nothing

Re: [Xen-devel] [PATCH v3 1/2] x86/mem-sharing: Bulk mem-sharing entire domains

2016-05-13 Thread Jan Beulich 
>>> On 12.05.16 at 17:25,  wrote:
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1294,6 +1294,43 @@ int relinquish_shared_pages(struct domain *d)
>  return rc;
>  }
>  
> +static int bulk_share(struct domain *d, struct domain *cd, unsigned long max,
> +  struct mem_sharing_op_bulk *bulk)
> +{
> +int rc;
> +shr_handle_t sh, ch;
> +
> +while( bulk->start <= max )
> +{
> +rc = mem_sharing_nominate_page(d, bulk->start, 0, );
> +if ( rc == -ENOMEM )
> +break;
> +if ( !rc )
> +{
> +rc = mem_sharing_nominate_page(cd, bulk->start, 0, );
> +if ( rc == -ENOMEM )
> +break;

If we get to this break, how will the caller know that the first
nomination succeeded but the second didn't? Or perhaps there
is some undo logic missing here?

> +if ( !rc )
> +mem_sharing_share_pages(d, bulk->start, sh, cd, bulk->start, 
> ch);

You shouldn't be ignoring errors here.

> +}
> +
> +++(bulk->start);

Pointless parentheses.

> +/* Check for continuation if it's not the last iteration. */
> +if ( bulk->start < max && hypercall_preempt_check() )

The loop head has <=; why < here?

> +{
> +rc = 1;

I'd recommend using -ERESTART here, as we do elsewhere.

> +break;
> +}
> +}
> +
> +/* We only propagate -ENOMEM so reset rc here */
> +if ( rc < 0 && rc != -ENOMEM )
> +rc = 0;

What's the rationale for discarding all other errors? At least the
patch description, but perhaps even the comment (which btw
is lacking a full stop) should be explaining this.

> @@ -1468,6 +1505,69 @@ int 
> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>  }
>  break;
>  
> +case XENMEM_sharing_op_bulk_share:
> +{
> +unsigned long max_sgfn, max_cgfn;
> +struct domain *cd;
> +
> +rc = -EINVAL;
> +if ( !mem_sharing_enabled(d) )
> +goto out;
> +
> +rc = rcu_lock_live_remote_domain_by_id(mso.u.bulk.client_domain,
> +   );
> +if ( rc )
> +goto out;
> +
> +rc = xsm_mem_sharing_op(XSM_DM_PRIV, d, cd, mso.op);

Either you pass XENMEM_sharing_op_share here, or you need to
update xen/xsm/flask/policy/access_vectors (even if it's only a
comment which needs updating).

That said - are this and the similar pre-existing XSM checks actually
correct? I.e. is one of the two domains here really controlling the
other? I would have expected that a tool stack domain initiates the
sharing between two domains it controls...

> +if ( rc )
> +{
> +rcu_unlock_domain(cd);
> +goto out;
> +}
> +
> +if ( !mem_sharing_enabled(cd) )
> +{
> +rcu_unlock_domain(cd);
> +rc = -EINVAL;
> +goto out;
> +}
> +
> +if ( !atomic_read(>pause_count) ||
> + !atomic_read(>pause_count) )
> +{
> +rcu_unlock_domain(cd);
> +rc = -EINVAL;
> +goto out;
> +}
> +
> +max_sgfn = domain_get_maximum_gpfn(d);
> +max_cgfn = domain_get_maximum_gpfn(cd);
> +
> +if ( max_sgfn != max_cgfn || max_sgfn < mso.u.bulk.start )

Why would the two domains need to agree in their maximum
GPFN? There's nothing similar in this file so far. Nor does the
right side of the || match anything pre-existing...

> @@ -488,7 +489,18 @@ struct xen_mem_sharing_op {
>  uint64_aligned_t client_gfn;/* IN: the client gfn */
>  uint64_aligned_t client_handle; /* IN: handle to the client page 
> */
>  domid_t  client_domain; /* IN: the client domain id */
> -} share; 
> +} share;
> +struct mem_sharing_op_bulk { /* OP_BULK_SHARE */
> +uint64_aligned_t start;  /* IN: start gfn. Set to 0 for
> +full deduplication. Field is
> +used internally and may 
> change
> +when the hypercall returns. 
> */
> +uint64_aligned_t shared; /* OUT: the number of gfns
> +that are shared after this
> +operation including pages
> +already shared before */
> +domid_t client_domain;   /* IN: the client domain id */
> +} bulk;

Let's not repeat pre-existing mistakes: There is explicit padding
missing here, which then also ought to be checked to be zero on
input.

Jan

[Xen-devel] [PATCH v3 1/2] x86/mem-sharing: Bulk mem-sharing entire domains

2016-05-12 Thread Tamas K Lengyel 
Currently mem-sharing can be performed on a page-by-page base from the control
domain. However, when completely deduplicating (cloning) a VM, this requires
at least 3 hypercalls per page. As the user has to loop through all pages up
to max_gpfn, this process is very slow and wasteful.

This patch introduces a new mem_sharing memop for bulk deduplication where
the user doesn't have to separately nominate each page in both the source and
destination domain, and the looping over all pages happen in the hypervisor.
This significantly reduces the overhead of completely deduplicating entire
domains.

Signed-off-by: Tamas K Lengyel 
Acked-by: Wei Liu 
---
Ian Jackson 
George Dunlap 
Jan Beulich 
Andrew Cooper 

v3: Bail if domains are not paused
Rename bulk_share struct to just bulk
Return -ENOMEM error if nomination fails
Return total number of shared pages (not keeping separate count)
v2: Stash hypercall continuation start point in xen_mem_sharing_op_t
Return number of successfully shared pages in xen_mem_sharing_op_t
---
 tools/libxc/include/xenctrl.h |  15 +++
 tools/libxc/xc_memshr.c   |  19 
 xen/arch/x86/mm/mem_sharing.c | 100 ++
 xen/include/public/memory.h   |  14 +-
 4 files changed, 147 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index dc54612..29ff13e 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2327,6 +2327,21 @@ int xc_memshr_add_to_physmap(xc_interface *xch,
 domid_t client_domain,
 unsigned long client_gfn);
 
+/* Allows to deduplicate the entire memory of a client domain in bulk. Using
+ * this function is equivalent of calling xc_memshr_nominate_gfn for each gfn
+ * in the two domains followed by xc_memshr_share_gfns. If successfull,
+ * returns the number of shared pages in 'shared'. Both domains must be paused.
+ *
+ * May fail with -EINVAL if the source and client domain have different
+ * memory size or if memory sharing is not enabled on either of the domains.
+ * May also fail with -ENOMEM if there isn't enough memory available to store
+ * the sharing metadata before deduplication can happen.
+ */
+int xc_memshr_bulk_share(xc_interface *xch,
+ domid_t source_domain,
+ domid_t client_domain,
+ uint64_t *shared);
+
 /* Debug calls: return the number of pages referencing the shared frame backing
  * the input argument. Should be one or greater. 
  *
diff --git a/tools/libxc/xc_memshr.c b/tools/libxc/xc_memshr.c
index deb0aa4..71350d2 100644
--- a/tools/libxc/xc_memshr.c
+++ b/tools/libxc/xc_memshr.c
@@ -181,6 +181,25 @@ int xc_memshr_add_to_physmap(xc_interface *xch,
 return xc_memshr_memop(xch, source_domain, );
 }
 
+int xc_memshr_bulk_share(xc_interface *xch,
+ domid_t source_domain,
+ domid_t client_domain,
+ uint64_t *shared)
+{
+int rc;
+xen_mem_sharing_op_t mso;
+
+memset(, 0, sizeof(mso));
+
+mso.op = XENMEM_sharing_op_bulk_share;
+mso.u.bulk.client_domain = client_domain;
+
+rc = xc_memshr_memop(xch, source_domain, );
+if ( !rc && shared ) *shared = mso.u.bulk.shared;
+
+return rc;
+}
+
 int xc_memshr_domain_resume(xc_interface *xch,
 domid_t domid)
 {
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index a522423..06176aa 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1294,6 +1294,43 @@ int relinquish_shared_pages(struct domain *d)
 return rc;
 }
 
+static int bulk_share(struct domain *d, struct domain *cd, unsigned long max,
+  struct mem_sharing_op_bulk *bulk)
+{
+int rc;
+shr_handle_t sh, ch;
+
+while( bulk->start <= max )
+{
+rc = mem_sharing_nominate_page(d, bulk->start, 0, );
+if ( rc == -ENOMEM )
+break;
+if ( !rc )
+{
+rc = mem_sharing_nominate_page(cd, bulk->start, 0, );
+if ( rc == -ENOMEM )
+break;
+if ( !rc )
+mem_sharing_share_pages(d, bulk->start, sh, cd, bulk->start, 
ch);
+}
+
+++(bulk->start);
+
+/* Check for continuation if it's not the last iteration. */
+if ( bulk->start < max && hypercall_preempt_check() )
+{
+rc = 1;
+break;
+}
+}
+
+/* We only propagate -ENOMEM so reset rc here */
+if ( rc < 0 && rc != -ENOMEM )
+rc = 0;
+
+return rc;
+}
+
 int mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
 {
 int rc;
@@ -1468,6 +1505,69 @@ int

Re: [Xen-devel] [PATCH v3 1/2] x86/mem-sharing: Bulk mem-sharing entire domains

2015-10-19 Thread Jan Beulich 
>>> On 16.10.15 at 19:02,  wrote:
> On Fri, Oct 16, 2015 at 12:46 AM, Jan Beulich  wrote:
>> >>> On 15.10.15 at 20:09,  wrote:
>> > +rc = -EFAULT;
>> > +else
>> > +rc = 
>> > hypercall_create_continuation(__HYPERVISOR_memory_op,
>> > +   "lh", 
>> > XENMEM_sharing_op,
>> > +   arg);
>> > +} else {
>>
>> Coding style.
> 
> Coding style matches the rest of the file.

Sorry, but this is not Xen coding style, and hence if there are other
bad examples in the file, these shouldn't be used as an excuse.

>> > +mso.u.bulk.applied = atomic_read(>shr_pages);
>>
>> Is there no possibility for something else to also modify d->shr_pages
>> in parallel, misguiding the caller when looking at the output field. Also
>> you're not copying this back to guest memory...
> 
> It shouldn't be an issue as I don't really care about how many pages were
> shared by the operation itself, I care about the total number of pages
> shared on the domain. If there were already some pages shared before this
> operation, those should be counted here again. I could of course issue a
> separate getdomaininfo to get this same information, it's just more
> expedient to report it right away instead of having to do a separate call.
> By default shr_pages will be equivalent to the number of pages successfully
> shared during this operation. If someone decides to also do unsharing in
> parallel to this op (..how would that make sense?).. well, that's not
> supported right now so all bets are off from my perspective.

But the field being named "applied" suggests a different meaning.

> Also, we are copying it back to guest memory when the operation finishes
> for all mso. It's not bulk specific, applies to all !rc cases further down.

Oh, okay.

>> > @@ -482,7 +483,16 @@ struct xen_mem_sharing_op {
>> >  uint64_aligned_t client_gfn;/* IN: the client gfn */
>> >  uint64_aligned_t client_handle; /* IN: handle to the client
>> page */
>> >  domid_t  client_domain; /* IN: the client domain id */
>> > -} share;
>> > +} share;
>> > +struct mem_sharing_op_bulk { /* OP_BULK_SHARE */
>> > +domid_t client_domain;   /* IN: the client domain
>> id */
>>
>> Explicit padding here please (albeit I see already from the context
>> that this isn't being done in e.g. the share sub-structure above
>> either).
> 
> Not sure what you mean by explicit padding here. The way it's right now
> matches pretty much what was already in place.

domid_t is a 16-bit value, i.e. there's a gap after this field which
would better be made explicit (as we do in many places elsewhere,
albeit - as said - sadly not in the example visible in patch context
here).

Jan


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

Re: [Xen-devel] [PATCH v3 1/2] x86/mem-sharing: Bulk mem-sharing entire domains

2015-10-16 Thread Tamas K Lengyel 
On Fri, Oct 16, 2015 at 12:46 AM, Jan Beulich  wrote:

> >>> On 15.10.15 at 20:09,  wrote:
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -1293,6 +1293,42 @@ int relinquish_shared_pages(struct domain *d)
> >  return rc;
> >  }
> >
> > +static int bulk_share(struct domain *d, struct domain *cd, unsigned
> long max,
> > +  struct mem_sharing_op_bulk *bulk)
> > +{
> > +int rc = 0;
> > +shr_handle_t sh, ch;
> > +
> > +while( bulk->start <= max )
> > +{
> > +rc = mem_sharing_nominate_page(d, bulk->start, 0, );
> > +if ( rc == -ENOMEM )
> > +break;
> > +else if ( rc )
>
> Pointless else.
>

Pointless but harmless.


>
> > +goto next;
> > +
> > +rc = mem_sharing_nominate_page(cd, bulk->start, 0, );
> > +if ( rc == -ENOMEM )
> > +break;
> > +else if ( rc )
> > +goto next;
> > +
> > +mem_sharing_share_pages(d, bulk->start, sh, cd, bulk->start,
> ch);
> > +
> > +next:
>
> Labels indented by at least one space please.
>

Yes, keep forgetting..


>
> > @@ -1467,6 +1503,69 @@ int
> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
> >  }
> >  break;
> >
> > +case XENMEM_sharing_op_bulk_share:
> > +{
> > +unsigned long max_sgfn, max_cgfn;
> > +struct domain *cd;
> > +
> > +rc = -EINVAL;
> > +if ( !mem_sharing_enabled(d) )
> > +goto out;
> > +
> > +rc =
> rcu_lock_live_remote_domain_by_id(mso.u.bulk.client_domain,
> > +   );
> > +if ( rc )
> > +goto out;
> > +
> > +rc = xsm_mem_sharing_op(XSM_DM_PRIV, d, cd, mso.op);
> > +if ( rc )
> > +{
> > +rcu_unlock_domain(cd);
> > +goto out;
> > +}
> > +
> > +if ( !mem_sharing_enabled(cd) )
> > +{
> > +rcu_unlock_domain(cd);
> > +rc = -EINVAL;
> > +goto out;
> > +}
> > +
> > +if ( !atomic_read(>pause_count) ||
> > + !atomic_read(>pause_count) )
>
> As said in the reply to your respective inquiry - you want to look at
> controller_pause_count here instead.
>

Thanks!


>
> > +{
> > +rcu_unlock_domain(cd);
>
> Considering how many times you do this, perhaps worth putting
> a label at the (current) success path's unlock?
>

I'll take a look.


>
> > +rc = -EINVAL;
> > +goto out;
> > +}
> > +
> > +max_sgfn = domain_get_maximum_gpfn(d);
> > +max_cgfn = domain_get_maximum_gpfn(cd);
> > +
> > +if ( max_sgfn != max_cgfn || max_sgfn < mso.u.bulk.start )
> > +{
> > +rcu_unlock_domain(cd);
> > +rc = -EINVAL;
> > +goto out;
> > +}
> > +
> > +rc = bulk_share(d, cd, max_sgfn, );
> > +if ( rc > 0 )
> > +{
> > +if ( __copy_to_guest(arg, , 1) )
>
> Wouldn't copying just the one field you care about suffice here?
>

Sure.


>
> > +rc = -EFAULT;
> > +else
> > +rc =
> hypercall_create_continuation(__HYPERVISOR_memory_op,
> > +   "lh",
> XENMEM_sharing_op,
> > +   arg);
> > +} else {
>
> Coding style.
>

Coding style matches the rest of the file.


>
> > +mso.u.bulk.applied = atomic_read(>shr_pages);
>
> Is there no possibility for something else to also modify d->shr_pages
> in parallel, misguiding the caller when looking at the output field. Also
> you're not copying this back to guest memory...
>

It shouldn't be an issue as I don't really care about how many pages were
shared by the operation itself, I care about the total number of pages
shared on the domain. If there were already some pages shared before this
operation, those should be counted here again. I could of course issue a
separate getdomaininfo to get this same information, it's just more
expedient to report it right away instead of having to do a separate call.
By default shr_pages will be equivalent to the number of pages successfully
shared during this operation. If someone decides to also do unsharing in
parallel to this op (..how would that make sense?).. well, that's not
supported right now so all bets are off from my perspective.

Also, we are copying it back to guest memory when the operation finishes
for all mso. It's not bulk specific, applies to all !rc cases further down.


>
> > @@ -482,7 +483,16 @@ struct xen_mem_sharing_op {
> >  uint64_aligned_t client_gfn;/* IN: the client gfn */
> >

[Xen-devel] [PATCH v3 1/2] x86/mem-sharing: Bulk mem-sharing entire domains

2015-10-15 Thread Tamas K Lengyel 
Currently mem-sharing can be performed on a page-by-page base from the control
domain. However, when completely deduplicating (cloning) a VM, this requires
at least 3 hypercalls per page. As the user has to loop through all pages up
to max_gpfn, this process is very slow and wasteful.

This patch introduces a new mem_sharing memop for bulk deduplication where
the user doesn't have to separately nominate each page in both the source and
destination domain, and the looping over all pages happen in the hypervisor.
This significantly reduces the overhead of completely deduplicating entire
domains.

Signed-off-by: Tamas K Lengyel 
Cc: Ian Jackson 
Cc: Stefano Stabellini 
Cc: Ian Campbell 
Cc: Wei Liu 
Cc: George Dunlap 
Cc: Keir Fraser 
Cc: Jan Beulich 
Cc: Andrew Cooper 
Acked-by: Wei Liu 
---
v3: Bail if domains are not paused
Rename bulk_share struct to just bulk
Return -ENOMEM error if nomination fails
Return total number of shared pages (not keeping separate count)
v2: Stash hypercall continuation start point in xen_mem_sharing_op_t
Return number of successfully shared pages in xen_mem_sharing_op_t
---
 tools/libxc/include/xenctrl.h | 15 +++
 tools/libxc/xc_memshr.c   | 19 +
 xen/arch/x86/mm/mem_sharing.c | 99 +++
 xen/include/public/memory.h   | 12 +-
 4 files changed, 144 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 3bfa00b..3093a7c 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2594,6 +2594,21 @@ int xc_memshr_add_to_physmap(xc_interface *xch,
 domid_t client_domain,
 unsigned long client_gfn);
 
+/* Allows to deduplicate the entire memory of a client domain in bulk. Using
+ * this function is equivalent of calling xc_memshr_nominate_gfn for each gfn
+ * in the two domains followed by xc_memshr_share_gfns. If successfull,
+ * returns the number of shared pages in 'shared'. Both domains must be paused.
+ *
+ * May fail with -EINVAL if the source and client domain have different
+ * memory size or if memory sharing is not enabled on either of the domains.
+ * May also fail with -ENOMEM if there isn't enough memory available to store
+ * the sharing metadata before deduplication can happen.
+ */
+int xc_memshr_bulk_share(xc_interface *xch,
+ domid_t source_domain,
+ domid_t client_domain,
+ uint64_t *shared);
+
 /* Debug calls: return the number of pages referencing the shared frame backing
  * the input argument. Should be one or greater. 
  *
diff --git a/tools/libxc/xc_memshr.c b/tools/libxc/xc_memshr.c
index deb0aa4..d38a6a9 100644
--- a/tools/libxc/xc_memshr.c
+++ b/tools/libxc/xc_memshr.c
@@ -181,6 +181,25 @@ int xc_memshr_add_to_physmap(xc_interface *xch,
 return xc_memshr_memop(xch, source_domain, );
 }
 
+int xc_memshr_bulk_share(xc_interface *xch,
+ domid_t source_domain,
+ domid_t client_domain,
+ uint64_t *shared)
+{
+int rc;
+xen_mem_sharing_op_t mso;
+
+memset(, 0, sizeof(mso));
+
+mso.op = XENMEM_sharing_op_bulk_share;
+mso.u.bulk.client_domain = client_domain;
+
+rc = xc_memshr_memop(xch, source_domain, );
+if ( !rc && shared ) *shared = mso.u.bulk.applied;
+
+return rc;
+}
+
 int xc_memshr_domain_resume(xc_interface *xch,
 domid_t domid)
 {
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index a95e105..b398d5a 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1293,6 +1293,42 @@ int relinquish_shared_pages(struct domain *d)
 return rc;
 }
 
+static int bulk_share(struct domain *d, struct domain *cd, unsigned long max,
+  struct mem_sharing_op_bulk *bulk)
+{
+int rc = 0;
+shr_handle_t sh, ch;
+
+while( bulk->start <= max )
+{
+rc = mem_sharing_nominate_page(d, bulk->start, 0, );
+if ( rc == -ENOMEM )
+break;
+else if ( rc )
+goto next;
+
+rc = mem_sharing_nominate_page(cd, bulk->start, 0, );
+if ( rc == -ENOMEM )
+break;
+else if ( rc )
+goto next;
+
+mem_sharing_share_pages(d, bulk->start, sh, cd, bulk->start, ch);
+
+next:
+++(bulk->start);
+
+/* Check for continuation if it's not the last iteration. */
+if ( bulk->start < max && hypercall_preempt_check() )
+{
+rc = 1;
+break;
+}
+}
+
+return rc;
+}
+
 int