From mboxrd@z Thu Jan 1 00:00:00 1970 From: Elena Ufimtseva Subject: Re: [PATCH v10 1/9] xen: vnuma topology and subop hypercalls Date: Wed, 3 Sep 2014 21:35:57 -0400 Message-ID: References: <1409718096-3145-1-git-send-email-ufimtseva@gmail.com> <1409718096-3145-2-git-send-email-ufimtseva@gmail.com> <540734480200007800030589@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <540734480200007800030589@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Keir Fraser , Ian Campbell , Li Yechen , George Dunlap , Matt Wilson , Dario Faggioli , Stefano Stabellini , Ian Jackson , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On Wed, Sep 3, 2014 at 9:31 AM, Jan Beulich wrote: >>>> 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. Answering the original question of yours, no, this is not used anywhere. And I guess I really do not need it as there is no code in hypervisor that will act on if such condition does or does not hold true. > >> + 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). Yes. > >> + 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. Yes, it is. > >> + 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. Yes, true, will fix it. > >> + { >> + 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. Yes, I did it the other way. I was looking at the other examples and used common style, or at least I thought so. > > Jan -- Elena