Re: [XEN][vNUMA][PATCH 3/9] public interface

[Keir Fraser <keir.fraser@eu.citrix.com>]

> +#ifndef __XEN_PUBLIC_DOM_NUMA_X86_H__
> +#define __XEN_PUBLIC_DOM_NUMA_X86_H__
> +
> +/* struct xc_cpumask : static structure */
> +#define XEN_CPUMASK_BITS_PER_BYTE 8
> +#define XEN_CPUMASK_BITS_TO_BYTES(bits) \
> +    (((bits)+XEN_CPUMASK_BITS_PER_BYTE-1)/XEN_CPUMASK_BITS_PER_BYTE)
> 
> +#define XEN_MAX_VCPUS 128
> +#define XEN_CPUMASK_DECLARE_BITMAP(name,bits) \
> +    uint8_t name[XEN_CPUMASK_BITS_TO_BYTES(bits)]
> +struct xen_cpumask{ XEN_CPUMASK_DECLARE_BITMAP(bits, XEN_MAX_VCPUS); };
> +#define XEN_CPUMASK_BITMAP(maskp) ((maskp)->bits)

What are xc_cpumask (a libxc concept) related definitions doing in a
hypervisor public header? These aren't even used in this header file. Below
I suggest a vcpu_to_vnode[] array, which probably gets rid of the need for
this bitmask stuff anyway.

> +#define XEN_MAX_VNODES 4

A small number to be statically defined. Better to make your structure
extensible I think, perhaps including pointers out to vnode-indexed arrays?

> +/* vnodes are 1GB-aligned */
> +#define XEN_MIN_VNODE_SHIFT (30)
> +
> +struct xen_vnode_info {
> +    uint8_t vnode_id;
> +    uint8_t mnode_id;

How do vnodes and mnodes differ? Why should a guest care about or need to
know about both, whatever they are?

> +    uint32_t nr_pages;

Not an address range? Is that implicitly worked out somehow? Should be
commented, but even better just a <start,end> range explicitly given?

> +    struct xen_cpumask vcpu_mask; /* vnode_to_vcpumask */
> +};

Why not have a single integer array vcpu_to_vnode[] in the main
xen_domain_numa_info structure?

> +#define XEN_DOM_NUMA_INTERFACE_VERSION  0x01
> +
> +#define XEN_DOM_NUMA_CONFINE    0x01
> +#define XEN_DOM_NUMA_SPLIT      0x02
> +#define XEN_DOM_NUMA_STRIPE     0x03
> +#define XEN_DOM_NUMA_DONTCARE   0x04

What should the guest do with these? You're rather light on comments in this
critical interface-defining header file.

> +struct xen_domain_numa_info {
> +    uint8_t version;
> +    uint8_t type;
> +
> +    uint8_t nr_vcpus;
> +    uint8_t nr_vnodes;
> +
> +    /* XXX: hvm_info_table uses 32-bit for high_mem_pgend,
> +     * so we should be fine 32-bits too*/
> +    uint32_t nr_pages;

If this is going to be visible outside HVMloader (e.g., in PV guests) then
just make it a uint64_aligned_t and be done with it.

> +    /* Only (nr_vnodes) entries are filled */
> +    struct xen_vnode_info vnode_info[XEN_MAX_VNODES];
> +    /* Only (nr_vnodes*nr_vnodes) entries are filled */
> +    uint8_t vnode_distance[XEN_MAX_VNODES*XEN_MAX_VNODES];

As suggested above, make these pointers out to dynamic-sized arrays. No need
for XEN_MAX_VNODES at all.

 -- Keir

> +};
> +
> +#endif

On 05/07/2010 09:52, "Dulloor" <dulloor@gmail.com> wrote:

> oops .. sorry, here it is.
> 
> -dulloor
> 
> On Mon, Jul 5, 2010 at 12:39 AM, Keir Fraser <keir.fraser@eu.citrix.com>
> wrote:
>> This patch is incomplete.
>> 
>> 
>> On 03/07/2010 00:54, "Dulloor" <dulloor@gmail.com> wrote:
>> 
>>> Implement the structure that will be shared with hvmloader (with HVMs)
>>> and directly with the VMs (with PV).
>>> 
>>> -dulloor
>>> 
>>> Signed-off-by : Dulloor <dulloor@gmail.com>
>> 
>> 
>>