Re: [PATCH v10 1/9] xen: vnuma topology and subop hypercalls

[Elena Ufimtseva <ufimtseva@gmail.com>]

On Wed, Sep 3, 2014 at 9:31 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 03.09.14 at 06:21, <ufimtseva@gmail.com> 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