From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v10 1/9] xen: vnuma topology and subop hypercalls Date: Wed, 03 Sep 2014 14:31:20 +0100 Message-ID: <540734480200007800030589@mail.emea.novell.com> References: <1409718096-3145-1-git-send-email-ufimtseva@gmail.com> <1409718096-3145-2-git-send-email-ufimtseva@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1409718096-3145-2-git-send-email-ufimtseva@gmail.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Elena Ufimtseva Cc: keir@xen.org, Ian.Campbell@citrix.com, lccycc123@gmail.com, george.dunlap@eu.citrix.com, msw@linux.com, dario.faggioli@citrix.com, stefano.stabellini@eu.citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org >>> On 03.09.14 at 06:21, wrote: > +static struct vnuma_info *vnuma_init(const struct xen_domctl_vnuma *uinfo, > + const struct domain *d) > +{ > + unsigned int nr_vnodes; > + int i, ret = -EINVAL; "i" really ought to be unsigned too. > + struct vnuma_info *info; > + > + nr_vnodes = uinfo->nr_vnodes; > + > + if ( nr_vnodes == 0 || nr_vnodes > uinfo->nr_vmemranges || The earlier question on the second of these checks stands. > + uinfo->nr_vcpus != d->max_vcpus || uinfo->pad != 0 ) > + return ERR_PTR(ret); > + > + info = vnuma_alloc(nr_vnodes, uinfo->nr_vmemranges, d->max_vcpus); > + if ( IS_ERR(info) ) > + return NULL; I think you'd be better off returning info here (see below). > + case XEN_DOMCTL_setvnumainfo: > + { > + struct vnuma_info *vnuma; > + > + vnuma = vnuma_init(&op->u.vnuma, d); > + if ( IS_ERR(vnuma) ) > + { > + ret = -PTR_ERR(vnuma); The negation seems wrong. > + break; > + } > + > + ASSERT(vnuma != NULL); This will trigger if the allocation earlier on fails, and you have vnuma_init() return NULL. > + case XENMEM_get_vnumainfo: > + { > + struct vnuma_topology_info topology; > + struct domain *d; > + unsigned int dom_vnodes, dom_vranges, dom_vcpus; > + struct vnuma_info tmp; > + > + /* > + * Guest passes nr_vnodes, number of regions and nr_vcpus thus > + * we know how much memory guest has allocated. > + */ > + if ( copy_from_guest(&topology, arg, 1 )) > + return -EFAULT; > + > + if ( (d = rcu_lock_domain_by_any_id(topology.domid)) == NULL ) > + return -ESRCH; > + > + read_lock(&d->vnuma_rwlock); > + > + if ( d->vnuma == NULL ) > + { > + read_unlock(&d->vnuma_rwlock); > + rcu_unlock_domain(d); > + return -EOPNOTSUPP; > + } > + > + dom_vnodes = d->vnuma->nr_vnodes; > + dom_vranges = d->vnuma->nr_vmemranges; > + dom_vcpus = d->max_vcpus; > + > + /* > + * Copied from guest values may differ from domain vnuma config. > + * Check here guest parameters make sure we dont overflow. > + * Additionaly check padding. > + */ > + if ( topology.nr_vnodes < dom_vnodes || > + topology.nr_vcpus < dom_vcpus || > + topology.nr_vmemranges < dom_vranges || > + topology.pad != 0 ) This last one clearly is -EINVAL, not -ENOBUFS. And for simple error handling the check could be done earlier on. > + { > + read_unlock(&d->vnuma_rwlock); > + rcu_unlock_domain(d); > + > + topology.nr_vnodes = dom_vnodes; > + topology.nr_vcpus = dom_vcpus; > + topology.nr_vmemranges = dom_vranges; > + > + /* Copy back needed values. */ > + __copy_to_guest(arg, &topology, 1); > + > + return -ENOBUFS; > + } > + > + read_unlock(&d->vnuma_rwlock); > + > + tmp.vdistance = xmalloc_array(unsigned int, dom_vnodes * dom_vnodes); > + tmp.vmemrange = xmalloc_array(vmemrange_t, dom_vranges); > + tmp.vcpu_to_vnode = xmalloc_array(unsigned int, dom_vcpus); > + > + if ( tmp.vdistance == NULL || tmp.vmemrange == NULL || > + tmp.vcpu_to_vnode == NULL ) > + { > + rc = -ENOMEM; > + goto vnumainfo_out; > + } > + > + /* > + * Check if vnuma info has changed and if the allocated arrays > + * are not big enough. > + */ > + read_lock(&d->vnuma_rwlock); > + > + if ( dom_vnodes < d->vnuma->nr_vnodes || > + dom_vranges < d->vnuma->nr_vmemranges || > + dom_vcpus < d->max_vcpus ) > + { > + read_unlock(&d->vnuma_rwlock); > + rc = -EAGAIN; > + goto vnumainfo_out; > + } > + > + dom_vnodes = d->vnuma->nr_vnodes; > + dom_vranges = d->vnuma->nr_vmemranges; > + dom_vcpus = d->max_vcpus; > + > + memcpy(tmp.vmemrange, d->vnuma->vmemrange, > + sizeof(*d->vnuma->vmemrange) * dom_vranges); > + memcpy(tmp.vdistance, d->vnuma->vdistance, > + sizeof(*d->vnuma->vdistance) * dom_vnodes * dom_vnodes); > + memcpy(tmp.vcpu_to_vnode, d->vnuma->vcpu_to_vnode, > + sizeof(*d->vnuma->vcpu_to_vnode) * dom_vcpus); > + > + read_unlock(&d->vnuma_rwlock); > + > + if ( copy_to_guest(topology.vmemrange.h, tmp.vmemrange, > + dom_vranges) != 0 ) > + goto vnumainfo_out; > + > + if ( copy_to_guest(topology.vdistance.h, tmp.vdistance, > + dom_vnodes * dom_vnodes) != 0 ) > + goto vnumainfo_out; > + > + if ( copy_to_guest(topology.vcpu_to_vnode.h, tmp.vcpu_to_vnode, > + dom_vcpus) != 0 ) > + goto vnumainfo_out; > + > + topology.nr_vnodes = dom_vnodes; > + topology.nr_vcpus = dom_vcpus; > + topology.nr_vmemranges = dom_vranges; > + > + if ( __copy_to_guest(arg, &topology, 1) != 0 ) > + rc = -EFAULT; > + else rc = 0; Now that's not a conditional operator and not a single line. But it'll be okay anyway if you split the "else ..." into two lines; I'm just confused since you said you would do the suggested conversion. Jan