On Fri, Oct 9, 2015 at 1:51 AM, Jan Beulich <jbeul...@suse.com> wrote:
> >>> On 08.10.15 at 22:57, <ta...@tklengyel.com> wrote:
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -1293,6 +1293,37 @@ 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_share *bulk)
> > +{
> > + int rc = 0;
> > + shr_handle_t sh, ch;
> > +
> > + while( bulk->start <= max )
> > + {
> > + if ( mem_sharing_nominate_page(d, bulk->start, 0, &sh) != 0 )
> > + goto next;
> > +
> > + if ( mem_sharing_nominate_page(cd, bulk->start, 0, &ch) != 0 )
> > + goto next;
> > +
> > + if ( !mem_sharing_share_pages(d, bulk->start, sh, cd,
> bulk->start, ch) )
> > + ++(bulk->shared);
>
> Pointless parentheses.
>
>
Pointless but harmless and I like this style better.
> > +next:
>
> Labels indented by at least one space please.
>
Ack.
>
> > + ++(bulk->start);
> > +
> > + /* Check for continuation if it's not the last iteration. */
> > + if ( bulk->start < max && hypercall_preempt_check() )
> > + {
> > + rc = 1;
> > + break;
>
> This could simple be a return statement, avoiding the need for a
> local variable (the type of which would need to be changed, see
> below).
>
It's reused in the caller to indicate where the mso copyback happens and rc
is of type int in the caller.
>
> > + }
> > + }
> > +
> > + return rc;
> > +}
>
> So effectively the function right now returns a boolean. If that's
> intended, its return type should reflect that (but I then wonder
> whether the sense of the values shouldn't be inverted, to have
> "true" mean "done").
>
It does but it's return is assigned to rc which is used to decide where
copyback happens.
>
> > @@ -1467,6 +1498,59 @@ 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;
> > + }
> > +
> > + 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 )
>
> Are both domains required to be paused for this operation? If so,
> shouldn't you enforce this? If not, by the time you reach the if()
> the values are stale.
>
It's expected that the user has exclusive tool-side lock on the domains
before issuing this hypercall and that the domains are paused already.
>
> > + {
> > + rcu_unlock_domain(cd);
> > + rc = -EINVAL;
> > + goto out;
> > + }
> > +
> > + rc = bulk_share(d, cd, max_sgfn, &mso.u.bulk);
> > + if ( rc )
>
> With the boolean nature the use of "rc" here is rather confusing;
> I'd suggest avoiding the use of in intermediate variable in this case.
>
I don't see where the confusion is - rc indicates there is work left to do
and hypercall continuation needs to be setup. I could move this block into
bulk_share itself.
>
> > + {
> > + if ( __copy_to_guest(arg, &mso, 1) )
> > + rc = -EFAULT;
> > + else
> > + rc =
> hypercall_create_continuation(__HYPERVISOR_memory_op,
> > + "lh",
> XENMEM_sharing_op,
> > + arg);
> > + }
> > +
> > + 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 320de91..0299746 100644
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -447,6 +447,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)
> > @@ -482,7 +483,13 @@ 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_share { /* OP_BULK_SHARE */
>
> Is the _share suffix really useful here? Even more, if you dropped
> it and renamed "shared" below to "done", the same structure could
> be used for a hypothetical bulk-unshare operation.
>
I don't really have a use-case for that at the moment and having it simply
as "bulk" is not specific enough IMHO.
>
> > + domid_t client_domain; /* IN: the client domain
> id */
> > + uint64_aligned_t start; /* IN: start gfn (normally
> 0) */
>
> Marking this as just IN implies that the value doesn't change across
> the operation.
>
In my book IN means it's used strictly only to pass input and it's value
may or may not be the same afterwards.
>
> > + uint64_aligned_t shared; /* OUT: the number of
> > + successfully shared
> pages */
>
> For this to be correct, the value is required to be zero initialized by
> the caller at present. Either this needs to be documented, or you
> should zero the field on any non-continuation invocation.
>
Sure, I'll add a comment for that effect - the toolstack already memsets
the mso struct to zero but I agree it's better to explicitly state that.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
