Re: [Xen-devel] [PATCH v6 01/10] xen: vnuma topology and subop hypercalls

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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