Re: [Xen-devel] [PATCH v3 1/7] xen: vNUMA support for PV guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.11.13 at 21:24, Elena Ufimtseva <ufimtseva@xxxxxxxxx> wrote:
> @@ -889,6 +890,84 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>      }
>      break;
>  
> +    case XEN_DOMCTL_setvnumainfo:
> +    {
> +        unsigned int dist_size, nr_vnodes;
> +        
> +        ret = -EINVAL;
> +        
> +        /* 
> +         * If number of vnodes was set before,
> +         * dont initilize it again.
> +         */
> +        //ASSERT(d->vnuma.nr_vnodes > 0);

Please, finally, clean up your patches before submitting. I'm not
going to point out further obvious violations below.

> --- /dev/null
> +++ b/xen/include/public/vnuma.h
> @@ -0,0 +1,56 @@
> +#ifndef _XEN_PUBLIC_VNUMA_H
> +#define _XEN_PUBLIC_VNUMA_H
> +
> +#include "xen.h"
> +
> +/*
> + * Following structures are used to represent vNUMA 
> + * topology to guest if requested.
> + */
> +
> +/* 
> + * Memory ranges can be used to define
> + * vNUMA memory node boundaries by the 
> + * linked list. As of now, only one range
> + * per domain is suported.
> + */
> +struct vmemrange {
> +    uint64_t start, end;
> +    uint64_t __padm;

What are you padding here?

> +};
> +
> +typedef struct vmemrange vmemrange_t;
> +DEFINE_XEN_GUEST_HANDLE(vmemrange_t);
> +
> +/* 
> + * vNUMA topology specifies vNUMA node
> + * number, distance table, memory ranges and
> + * vcpu mapping provided for guests.
> + */
> +
> +struct vnuma_topology_info {
> +    /* IN */
> +    domid_t domid;
> +    uint32_t _pad;
> +    /* OUT */
> +    union {
> +        XEN_GUEST_HANDLE(uint) h;
> +        uint64_t    _padn;

No need for the odd trailing n (and other characters below).

One thing that I only realized - what information is the guest
supposed to use to know the number of virtual nodes it has
(needed to size the arrays the handles here point to)?

> +    } nr_vnodes;
> +    union {
> +        XEN_GUEST_HANDLE(uint) h;
> +        uint64_t    _padd;
> +    } vdistance;
> +    union {
> +        XEN_GUEST_HANDLE(uint) h;

And this one is as problematic - I don't think the guest necessarily
knows what max_vcpus the hypervisor has set for it.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel