[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v13 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
> -----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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |