diff mbox

[v3,1/2] x86/mem-sharing: Bulk mem-sharing entire domains

Message ID 1463066752-23969-1-git-send-email-tamas@tklengyel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tamas K Lengyel May 12, 2016, 3:25 p.m. UTC
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 <tamas@tklengyel.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Ian Jackson <ian.jackson@eu.citrix.com>
George Dunlap <george.dunlap@eu.citrix.com>
Jan Beulich <jbeulich@suse.com>
Andrew Cooper <andrew.cooper3@citrix.com>

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

Comments

Jan Beulich May 13, 2016, noon UTC | #1
>>> On 12.05.16 at 17:25, <tamas@tklengyel.com> 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, &sh);
> +        if ( rc == -ENOMEM )
> +            break;
> +        if ( !rc )
> +        {
> +            rc = mem_sharing_nominate_page(cd, bulk->start, 0, &ch);
> +            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,
> +                                                   &cd);
> +            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(&d->pause_count) ||
> +                 !atomic_read(&cd->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
Tamas K Lengyel May 13, 2016, 2:50 p.m. UTC | #2
On Fri, May 13, 2016 at 6:00 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 12.05.16 at 17:25, <tamas@tklengyel.com> 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, &sh);
>> +        if ( rc == -ENOMEM )
>> +            break;
>> +        if ( !rc )
>> +        {
>> +            rc = mem_sharing_nominate_page(cd, bulk->start, 0, &ch);
>> +            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,
>> +                                                   &cd);
>> +            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(&d->pause_count) ||
>> +                 !atomic_read(&cd->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.

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

Thanks,
Tamas
Jan Beulich May 13, 2016, 3:09 p.m. UTC | #3
>>> On 13.05.16 at 16:50, <tamas@tklengyel.com> wrote:
> On Fri, May 13, 2016 at 6:00 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 12.05.16 at 17:25, <tamas@tklengyel.com> 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,
>>> +                                                   &cd);
>>> +            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(&d->pause_count) ||
>>> +                 !atomic_read(&cd->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.

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

Jan
Tamas K Lengyel May 13, 2016, 3:31 p.m. UTC | #4
On Fri, May 13, 2016 at 9:09 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 13.05.16 at 16:50, <tamas@tklengyel.com> wrote:
>> On Fri, May 13, 2016 at 6:00 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 12.05.16 at 17:25, <tamas@tklengyel.com> 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,
>>>> +                                                   &cd);
>>>> +            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(&d->pause_count) ||
>>>> +                 !atomic_read(&cd->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.

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

Tamas
Daniel De Graaf May 13, 2016, 3:35 p.m. UTC | #5
On 05/13/2016 11:09 AM, Jan Beulich wrote:
>>>> On 13.05.16 at 16:50, <tamas@tklengyel.com> 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,
>>>> +                                                   &cd);
>>>> +            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).
Jan Beulich May 13, 2016, 4:12 p.m. UTC | #6
>>> On 13.05.16 at 17:31, <tamas@tklengyel.com> wrote:
> On Fri, May 13, 2016 at 9:09 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 13.05.16 at 16:50, <tamas@tklengyel.com> wrote:
>>> On Fri, May 13, 2016 at 6:00 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 12.05.16 at 17:25, <tamas@tklengyel.com> 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(&d->pause_count) ||
>>>>> +                 !atomic_read(&cd->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
Jan Beulich May 13, 2016, 4:19 p.m. UTC | #7
>>> On 13.05.16 at 17:35, <dgdegra@tycho.nsa.gov> wrote:
> On 05/13/2016 11:09 AM, Jan Beulich wrote:
>>>>> On 13.05.16 at 16:50, <tamas@tklengyel.com> 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,
>>>>> +                                                   &cd);
>>>>> +            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
Tamas K Lengyel May 13, 2016, 4:29 p.m. UTC | #8
On Fri, May 13, 2016 at 10:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 13.05.16 at 17:31, <tamas@tklengyel.com> wrote:
>> On Fri, May 13, 2016 at 9:09 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 13.05.16 at 16:50, <tamas@tklengyel.com> wrote:
>>>> On Fri, May 13, 2016 at 6:00 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 12.05.16 at 17:25, <tamas@tklengyel.com> 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(&d->pause_count) ||
>>>>>> +                 !atomic_read(&cd->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 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?

I see, ok.

Tamas
Jan Beulich May 17, 2016, 7:49 a.m. UTC | #9
>>> On 13.05.16 at 18:29, <tamas@tklengyel.com> wrote:
> On Fri, May 13, 2016 at 10:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 13.05.16 at 17:31, <tamas@tklengyel.com> wrote:
>>> On Fri, May 13, 2016 at 9:09 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 13.05.16 at 16:50, <tamas@tklengyel.com> wrote:
>>>>> On Fri, May 13, 2016 at 6:00 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>> On 12.05.16 at 17:25, <tamas@tklengyel.com> 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(&d->pause_count) ||
>>>>>>> +                 !atomic_read(&cd->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
diff mbox

Patch

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, &mso);
 }
 
+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(&mso, 0, sizeof(mso));
+
+    mso.op = XENMEM_sharing_op_bulk_share;
+    mso.u.bulk.client_domain = client_domain;
+
+    rc = xc_memshr_memop(xch, source_domain, &mso);
+    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, &sh);
+        if ( rc == -ENOMEM )
+            break;
+        if ( !rc )
+        {
+            rc = mem_sharing_nominate_page(cd, bulk->start, 0, &ch);
+            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 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,
+                                                   &cd);
+            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(&d->pause_count) ||
+                 !atomic_read(&cd->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 )
+            {
+                rcu_unlock_domain(cd);
+                rc = -EINVAL;
+                goto out;
+            }
+
+            rc = bulk_share(d, cd, max_sgfn, &mso.u.bulk);
+            if ( rc > 0 )
+            {
+                if ( __copy_to_guest(arg, &mso, 1) )
+                    rc = -EFAULT;
+                else
+                    rc = hypercall_create_continuation(__HYPERVISOR_memory_op,
+                                                       "lh", XENMEM_sharing_op,
+                                                       arg);
+            }
+            else
+                mso.u.bulk.shared = atomic_read(&cd->shr_pages);
+
+            rcu_unlock_domain(cd);
+        }
+        break;
+
         case XENMEM_sharing_op_debug_gfn:
         {
             unsigned long gfn = mso.u.debug.u.gfn;
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index fe52ee1..202250a 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -453,6 +453,7 @@  DEFINE_XEN_GUEST_HANDLE(xen_mem_access_op_t);
 #define XENMEM_sharing_op_debug_gref        5
 #define XENMEM_sharing_op_add_physmap       6
 #define XENMEM_sharing_op_audit             7
+#define XENMEM_sharing_op_bulk_share        8
 
 #define XENMEM_SHARING_OP_S_HANDLE_INVALID  (-10)
 #define XENMEM_SHARING_OP_C_HANDLE_INVALID  (-9)
@@ -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;
         struct mem_sharing_op_debug {     /* OP_DEBUG_xxx */
             union {
                 uint64_aligned_t gfn;      /* IN: gfn to debug          */