Re: [Xen-devel] [PATCH v13 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.10.17 at 18:48, <paul.durrant@xxxxxxxxxx> wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -965,6 +965,94 @@ static long xatp_permission_check(struct domain *d, 
> unsigned int space)
>      return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>  }
>  
> +static int acquire_resource(
> +    XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
> +{
> +    struct domain *d, *currd = current->domain;
> +    xen_mem_acquire_resource_t xmar;
> +    /*
> +     * The mfn_list and gfn_list (below) arrays are ok on stack for the
> +     * moment since they are small, but if they need to grow in future
> +     * use-cases then per-CPU arrays or heap allocations may be required.
> +     */
> +    xen_pfn_t mfn_list[2];
> +    int rc;
> +
> +    if ( copy_from_guest(&xmar, arg, 1) )
> +        return -EFAULT;
> +
> +    if ( xmar.pad != 0 )
> +        return -EINVAL;
> +
> +    if ( guest_handle_is_null(xmar.frame_list) )
> +    {
> +        if ( xmar.nr_frames > 0 )

I generally consider "!= 0" (which then could be omitted altogether
better than "> 0" when the quantity is unsigned, to avoid giving
the impression that negative values might also take the other path.

> +            return -EINVAL;
> +
> +        xmar.nr_frames = ARRAY_SIZE(mfn_list);
> +
> +        if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
> +            return -EFAULT;
> +
> +        return 0;
> +    }
> +
> +    if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
> +        return -E2BIG;
> +
> +    rc = rcu_lock_remote_domain_by_id(xmar.domid, &d);
> +    if ( rc )
> +        return rc;
> +
> +    rc = xsm_domain_resource_map(XSM_DM_PRIV, d);
> +    if ( rc )
> +        goto out;
> +
> +    switch ( xmar.type )
> +    {
> +    default:
> +        rc = -EOPNOTSUPP;
> +        break;
> +    }
> +
> +    if ( rc )
> +        goto out;
> +
> +    if ( !paging_mode_translate(currd) )
> +    {
> +        if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
> +            rc = -EFAULT;
> +    }
> +    else
> +    {
> +        xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> +        unsigned int i;
> +
> +        rc = -EFAULT;
> +        if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
> +            goto out;

This will result in requests with nr_frames being zero to fail with
-EFAULT afaict. Let's please have such no-op requests succeed.

> +        for ( i = 0; i < xmar.nr_frames; i++ )
> +        {
> +            rc = set_foreign_p2m_entry(currd, gfn_list[i],
> +                                       _mfn(mfn_list[i]));
> +            if ( rc )
> +            {
> +                /*
> +                 * Make sure rc is -EIO for any iteration other than
> +                 * the first.
> +                 */
> +                rc = (i != 0) ? -EIO : rc;

Along the lines of what I've said above, "!=0" could be dropped
here, too.

I won't insist on the cosmetic remarks to be taken care of, but
the return value aspect should be fixed for
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
to apply.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel