[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 01/10] xen: vnuma topology and subop hypercalls
>>> On 18.07.14 at 07:50, <ufimtseva@xxxxxxxxx> wrote: > +static int vnuma_alloc(struct vnuma_info **vnuma, > + unsigned int nr_vnodes, > + unsigned int nr_vcpus, > + unsigned int dist_size) > +{ > + struct vnuma_info *v; > + > + if ( vnuma && *vnuma ) > + return -EINVAL; > + > + v = *vnuma; > + /* > + * check if any of xmallocs exeeds PAGE_SIZE. > + * If yes, consider it as an error for now. > + */ > + if ( nr_vnodes > PAGE_SIZE / sizeof(nr_vnodes) || > + nr_vcpus > PAGE_SIZE / sizeof(nr_vcpus) || > + nr_vnodes > PAGE_SIZE / sizeof(struct vmemrange) || > + dist_size > PAGE_SIZE / sizeof(dist_size) ) Three of the four checks are rather bogus - the types of the variables just happen to match the types of the respective array elements. Best to switch all of them to sizeof(*v->...). Plus I'm not sure about the dist_size check - in its current shape it's redundant with the nr_vnodes one (and really the function parameter seems pointless, i.e. could be calculated here), and it's questionable whether limiting that table against PAGE_SIZE isn't too restrictive. Also indentation seems broken here. > +static long vnuma_fallback(const struct domain *d, > + struct vnuma_info **vnuma) > +{ > + struct vnuma_info *v; > + long ret; > + > + > + /* Will not destroy vNUMA here, destroy before calling this. */ > + if ( vnuma && *vnuma ) > + return -EINVAL; > + > + v = *vnuma; > + ret = vnuma_alloc(&v, 1, d->max_vcpus, 1); > + if ( ret ) > + return ret; > + > + v->vmemrange[0].start = 0; > + v->vmemrange[0].end = d->max_pages << PAGE_SHIFT; Didn't we settle on using inclusive ranges to avoid problems at the address space end? Or was that in the context of some other series (likely by someone else)? > +long vnuma_init(const struct xen_domctl_vnuma *u_vnuma, > + const struct domain *d, > + struct vnuma_info **dst) > +{ > + unsigned int dist_size, nr_vnodes = 0; > + long ret; > + struct vnuma_info *v = NULL; > + > + ret = -EINVAL; > + > + /* If vNUMA topology already set, just exit. */ > + if ( !u_vnuma || *dst ) > + return ret; > + > + nr_vnodes = u_vnuma->nr_vnodes; > + > + if ( nr_vnodes == 0 ) > + return ret; > + > + if ( nr_vnodes > (UINT_MAX / nr_vnodes) ) > + return ret; > + > + dist_size = nr_vnodes * nr_vnodes; > + > + ret = vnuma_alloc(&v, nr_vnodes, d->max_vcpus, dist_size); > + if ( ret ) > + return ret; > + > + /* On failure, set only one vNUMA node and its success. */ > + ret = 0; Pointless - just use "return 0" below. > + > + if ( copy_from_guest(v->vdistance, u_vnuma->vdistance, dist_size) ) > + goto vnuma_onenode; > + if ( copy_from_guest(v->vmemrange, u_vnuma->vmemrange, nr_vnodes) ) > + goto vnuma_onenode; > + if ( copy_from_guest(v->vcpu_to_vnode, u_vnuma->vcpu_to_vnode, > + d->max_vcpus) ) Indentation. > + goto vnuma_onenode; > + if ( copy_from_guest(v->vnode_to_pnode, u_vnuma->vnode_to_pnode, > + nr_vnodes) ) Again. > + goto vnuma_onenode; > + > + v->nr_vnodes = nr_vnodes; > + *dst = v; > + > + return ret; > + > +vnuma_onenode: Labels are to be indented by at least one space. > + case XEN_DOMCTL_setvnumainfo: > + { > + struct vnuma_info *v = NULL; > + > + ret = -EFAULT; > + if ( guest_handle_is_null(op->u.vnuma.vdistance) || > + guest_handle_is_null(op->u.vnuma.vmemrange) || > + guest_handle_is_null(op->u.vnuma.vcpu_to_vnode) || > + guest_handle_is_null(op->u.vnuma.vnode_to_pnode) ) > + return ret; > + > + ret = -EINVAL; > + > + ret = vnuma_init(&op->u.vnuma, d, &v); > + if ( ret < 0 || v == NULL ) So when v == NULL you return success (ret being 0)? That second check is either pointless (could become an ASSERT()) or needs proper handling. > + break; > + > + /* overwrite vnuma for domain */ > + if ( !d->vnuma ) > + vnuma_destroy(d->vnuma); > + > + domain_lock(d); > + d->vnuma = v; > + domain_unlock(d); domain_lock()? What does this guard against? (We generally aim at removing uses of domain_lock() rather than adding new ones.) > + case XENMEM_get_vnumainfo: > + { > + struct vnuma_topology_info topology; > + struct domain *d; > + unsigned int dom_vnodes = 0; Pointless initializer. > + > + /* > + * guest passes nr_vnodes and nr_vcpus thus > + * we know how much memory guest has allocated. > + */ > + if ( copy_from_guest(&topology, arg, 1) || > + guest_handle_is_null(topology.vmemrange.h) || > + guest_handle_is_null(topology.vdistance.h) || > + guest_handle_is_null(topology.vcpu_to_vnode.h) ) > + return -EFAULT; > + > + if ( (d = rcu_lock_domain_by_any_id(topology.domid)) == NULL ) > + return -ESRCH; > + > + rc = -EOPNOTSUPP; > + if ( d->vnuma == NULL ) > + goto vnumainfo_out; > + > + if ( d->vnuma->nr_vnodes == 0 ) > + goto vnumainfo_out; Can this second condition validly (other than due to a race) be true if the first one wasn't? (And of course there's synchronization missing here, to avoid the race.) > + > + dom_vnodes = d->vnuma->nr_vnodes; > + > + /* > + * guest nr_cpus and nr_nodes may differ from domain vnuma config. > + * Check here guest nr_nodes and nr_cpus to make sure we dont > overflow. > + */ > + rc = -ENOBUFS; > + if ( topology.nr_vnodes < dom_vnodes || > + topology.nr_vcpus < d->max_vcpus ) > + goto vnumainfo_out; You ought to be passing back the needed values in the structure fields. > + > + rc = -EFAULT; > + > + if ( copy_to_guest(topology.vmemrange.h, d->vnuma->vmemrange, > + dom_vnodes) != 0 ) > + goto vnumainfo_out; > + > + if ( copy_to_guest(topology.vdistance.h, d->vnuma->vdistance, > + dom_vnodes * dom_vnodes) != 0 ) > + goto vnumainfo_out; > + > + if ( copy_to_guest(topology.vcpu_to_vnode.h, d->vnuma->vcpu_to_vnode, > + d->max_vcpus) != 0 ) > + goto vnumainfo_out; > + > + topology.nr_vnodes = dom_vnodes; And why not topology.nr_vcpus? > + > + if ( copy_to_guest(arg, &topology, 1) != 0 ) __copy_to_guest() please. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |