Re: [Xen-devel] [PATCH 1/5] libxl: introduce cpuid interface to domain build

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2010-09-08 at 10:19 +0100, Andre Przywara wrote:
> Add a cpuid parameter into libxl_domain_build_info and use
> it's content while setting up the domain. This is a only paving the way, 
> the real functionality is implemented in a later patch.
> 
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>

The destructor function should free the type but not the reference to
it, so:

> @@ -102,6 +102,21 @@ void
> libxl_key_value_list_destroy(libxl_key_value_list *pkvl)
>      free(kvl);
>  }
>  
> +void libxl_cpuid_destroy(libxl_cpuid_policy_list cpuid_list)

This should be *cpuid_list and the function should be adjusted to free
the elements of *cpuid_list but not cpuid_list itself.

> --- a/tools/libxl/libxl.idl
> +++ b/tools/libxl/libxl.idl
> @@ -11,6 +11,7 @@ libxl_console_consback = Builtin("console_consback")
>  libxl_console_constype = Builtin("console_constype")
>  libxl_disk_phystype = Builtin("disk_phystype")
>  libxl_nic_type = Builtin("nic_type")
> +libxl_cpuid_policy_list = Builtin("cpuid_policy_list", 
> destructor_fn="libxl_cpuid_destroy")

And this should have passby=PASS_BY_REFERENCE.

See 22078:573ddf5cc145 for a similar change to the libxl_string_list and
libxl_key_value_list destructor functions.

> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -172,6 +172,22 @@ typedef enum {
>      NICTYPE_VIF,
>  } libxl_nic_type;
>  
> +/* holds the CPUID response for a single CPUID leaf
> + * input contains the value of the EAX and ECX register,
> + * and each policy string contains a filter to apply to
> + * the host given values for that particular leaf.
> + */ 
> +struct libxl_cpuid_policy {
> +    uint32_t input[2];
> +    char *policy[4];
> +};

I realise (at least I think I do) that this is just exposing the
existing hypervisor/libxc interface warts and all but would this be more
obvious to users if it was:
struct libxl_cpuid_policy {
      uint32_t eax;
      uint32_t ecx;

      char *eax_filter;
      char *ebx_filter;
      char *ecx_filter;
      char *edx_filter;
}

could either translate in libxl or push the change down into libxc.

Alternatively
   #define LIBXL_CPUID_INPUT_EAX 0
   #define LIBXL_CPUID_INPUT_ECX 1

   #define LIBXL_CPUID_FILTER_EAX 0
   #define LIBXL_CPUID_FILTER_EBX 1
   ...
would at least make the code (or at least the data structure) a bit more
obvious.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel