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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.06.16 at 01:24, <tamas@xxxxxxxxxxxxx> wrote:
> @@ -1468,6 +1516,79 @@ 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( mso.u.bulk._pad[0] || mso.u.bulk._pad[1] || 
> mso.u.bulk._pad[2] )

With the missing blank added here (doable on commit as well),
hypervisor side
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> +                goto out;
> +
> +            if ( !mem_sharing_enabled(d) )
> +                goto out;

Irrespective of the above I think a more descriptive error than
-EINVAL would be quite reasonable to expect for a sharing operation
attempted without having enabled sharing. By the name of it (but
not by its conventional meaning) -EILSEQ might be a candidate.

> +            rc = rcu_lock_live_remote_domain_by_id(mso.u.bulk.client_domain,
> +                                                   &cd);
> +            if ( rc )
> +                goto out;
> +
> +            /*
> +             * We reuse XENMEM_sharing_op_share XSM check here as this is 
> essentially
> +             * the same concept repeated over multiple pages.
> +             */
> +            rc = xsm_mem_sharing_op(XSM_DM_PRIV, d, cd, 
> XENMEM_sharing_op_share);
> +            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);

I'd also recommend adding a new label below to avoid these repeated
rcu_unlock_domain(cd) invocations.

But for both of these - you're the maintainer, so you know best.

Jan


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