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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 23 November 2017 16:42
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George
> Dunlap <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Daniel De Graaf
> <dgdegra@xxxxxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> Subject: Re: [PATCH v13 05/11] x86/mm: add HYPERVISOR_memory_op to
> acquire guest resources
> 
> >>> 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.

Oh, I was labouring under the assumption that a copy_from_guest() with a zero 
count would succeed... I should have checked.

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

The reason for the != 0 here is make the failure explicitly EIO for the case 
where at least one iteration has successfully set_foreign_p2m_entry() but a 
subsequent one has failed, which matches what is stated in the header. 
(Specifically it means that, on ARM, the -EOPNOTSUPP from 
set_foreign_p2m_entry() will get back to the caller).

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

Ok, I'll need your opinion on my response above then.

  Paul

> 
> Jan


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