[PATCH 1/2] xl: add cpuid parameter

[PATCH 1/2] xl: add cpuid parameter

[Andre Przywara]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]

Hi,

this one adds a cpuid parameter into libxl_domain_build_info and uses 
it's content while setting up the domain. This is a placeholder for now, 
since the parsing is only implemented in the next patch.
Please review, I am especially interested if I got the IDL stuff right.

Regards,
Andre.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12

[-- Attachment #2: 0001-introduce-cpuid-interface-to-domain-build.patch --]
[-- Type: text/x-patch, Size: 4204 bytes --]

>From 73b3800344ba977de88cd79a3d7c3bfd558fd307 Mon Sep 17 00:00:00 2001
From: Andre Przywara <andre.przywara@amd.com>
Date: Tue, 24 Aug 2010 09:35:51 +0200
Subject: [PATCH 1/2] introduce cpuid interface to domain build

---
 tools/libxl/libxl.c      |   14 ++++++++++++++
 tools/libxl/libxl.h      |    6 ++++++
 tools/libxl/libxl.idl    |    2 ++
 tools/libxl/libxl_dom.c  |    6 ++++++
 tools/libxl/xl_cmdimpl.c |    1 +
 5 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 099d82e..da9c7fd 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -98,6 +98,20 @@ void libxl_key_value_list_destroy(libxl_key_value_list kvl)
     free(kvl);
 }
 
+void libxl_cpuid_destroy(libxl_cpuid_type *cpuid)
+{
+    int i, j;
+
+    if (cpuid == NULL)
+        return;
+    for (i = 0; cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
+        for (j = 0; j < 4; j++)
+            if (cpuid[i].policy[j] != NULL)
+                free(cpuid[i].policy[j]);
+    }
+    free(cpuid);
+}
+
 /******************************************************************************/
 
 int libxl_domain_make(libxl_ctx *ctx, libxl_domain_create_info *info,
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index f6480fb..b8fa8ca 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -178,6 +178,11 @@ typedef enum {
     NICTYPE_VIF,
 } libxl_nic_type;
 
+typedef struct {
+    uint32_t input[2];
+    char *policy[4];
+} libxl_cpuid_type;
+
 #define LIBXL_PCI_FUNC_ALL (~0U)
 
 #include "_libxl_types.h"
@@ -237,6 +242,7 @@ int libxl_domain_preserve(libxl_ctx *ctx, uint32_t domid, libxl_domain_create_in
 void libxl_string_list_destroy(libxl_string_list sl);
 void libxl_key_value_list_destroy(libxl_key_value_list kvl);
 void libxl_file_reference_destroy(libxl_file_reference *f);
+void libxl_cpuid_destroy(libxl_cpuid_type *cpuid);
 
 /*
  * Run the configured bootloader for a PV domain and update
diff --git a/tools/libxl/libxl.idl b/tools/libxl/libxl.idl
index f59950d..dc988d5 100644
--- 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_type = Builtin("cpuid_type", destructor_fn="libxl_cpuid_destroy")
 
 libxl_string_list = Builtin("string_list", destructor_fn="libxl_string_list_destroy")
 libxl_key_value_list = Builtin("key_value_list", destructor_fn="libxl_key_value_list_destroy")
@@ -97,6 +98,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("shadow_memkb",    uint32),
     ("disable_migrate", bool),
     ("kernel",          libxl_file_reference),
+    ("cpuid",           Reference(libxl_cpuid_type)),
     ("hvm",             integer),
     ("u", KeyedUnion(None, "hvm",
                 [("hvm", "%s", Struct(None,
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index a07bea7..ec4e15f 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -91,9 +91,15 @@ int build_post(libxl_ctx *ctx, uint32_t domid,
     xs_transaction_t t;
     char **ents;
     int i;
+    char *cpuid_res[4];
 
 #if defined(__i386__) || defined(__x86_64__)
     xc_cpuid_apply_policy(ctx->xch, domid);
+    if (info->cpuid != NULL) {
+        for (i = 0; info->cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++)
+            xc_cpuid_set(ctx->xch, domid, info->cpuid[i].input,
+                         (const char**)(info->cpuid[i].policy), cpuid_res);
+    }
 #endif
 
     ents = libxl_calloc(&gc, 12 + (info->max_vcpus * 2) + 2, sizeof(char *));
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index ae4fcc3..0fa516b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -268,6 +268,7 @@ static void init_build_info(libxl_domain_build_info *b_info, libxl_domain_create
     b_info->max_memkb = 32 * 1024;
     b_info->target_memkb = b_info->max_memkb;
     b_info->disable_migrate = 0;
+    b_info->cpuid = NULL;
     if (c_info->hvm) {
         b_info->shadow_memkb = 0; /* Set later */
         b_info->video_memkb = 8 * 1024;
-- 
1.6.4


[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH 1/2] xl: add cpuid parameter

[Gianni Tedesco]

On Fri, 2010-08-27 at 13:56 +0100, Andre Przywara wrote:
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 099d82e..da9c7fd 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -98,6 +98,20 @@ void
> libxl_key_value_list_destroy(libxl_key_value_list kvl)
>      free(kvl);
>  }
>  
> +void libxl_cpuid_destroy(libxl_cpuid_type *cpuid)
> +{
> +    int i, j;
> +
> +    if (cpuid == NULL)
> +        return;
> +    for (i = 0; cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
> +        for (j = 0; j < 4; j++)
> +            if (cpuid[i].policy[j] != NULL)
> +                free(cpuid[i].policy[j]);
> +    }
> +    free(cpuid);
> +}

This can be auto-generated. Also libxl_*_destroy() functions never call
free on the passed pointer. Hence ending _destroy() rather than _free().

Re: [PATCH 1/2] xl: add cpuid parameter

[Ian Campbell]

On Fri, 2010-08-27 at 15:07 +0100, Gianni Tedesco wrote:
> On Fri, 2010-08-27 at 13:56 +0100, Andre Przywara wrote:

Thanks Andre.

> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 099d82e..da9c7fd 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -98,6 +98,20 @@ void
> > libxl_key_value_list_destroy(libxl_key_value_list kvl)
> >      free(kvl);
> >  }
> >  
> > +void libxl_cpuid_destroy(libxl_cpuid_type *cpuid)
> > +{
> > +    int i, j;
> > +
> > +    if (cpuid == NULL)
> > +        return;
> > +    for (i = 0; cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
> > +        for (j = 0; j < 4; j++)
> > +            if (cpuid[i].policy[j] != NULL)
> > +                free(cpuid[i].policy[j]);
> > +    }
> > +    free(cpuid);
> > +}
> 
> This can be auto-generated.

The type is defined as a Builtin in the IDL, in that case hand coding
the destructor is valid.

>  Also libxl_*_destroy() functions never call
> free on the passed pointer. Hence ending _destroy() rather than _free().

There are some exceptions, such as the FOO_list builtins but as it
stands I don't think this is one of them. The free() _should_ have come
from the use of the Reference type in the IDL. This would be the first
non-const use of Reference it though so it is possible that gentypes.py
is not 100% correct in its handling of Reference types.

However I think overall it would be better to define an explicit list
type e.g. "typedef libxl_cpuid_type *libxl_cpuid_type_list" and use that
as the IDL builtin (without the Reference since it already has one),
similar to the existing string list builtins. In that case it would be
valid for libxl_cpuid_type_list_destroy to free the actual list as well
as the contents.

I don't particularly like the name "libxl_cpuid_type" though, is there a
more descriptive name? libxl_cpuid_policy perhaps? The opaque arrays
could do with a comment or two as well.

Ian.

Re: [PATCH 1/2] xl: add cpuid parameter

[Gianni Tedesco]

On Fri, 2010-08-27 at 15:51 +0100, Ian Campbell wrote:
> On Fri, 2010-08-27 at 15:07 +0100, Gianni Tedesco wrote:
> > On Fri, 2010-08-27 at 13:56 +0100, Andre Przywara wrote:
> 
> Thanks Andre.
> 
> > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > index 099d82e..da9c7fd 100644
> > > --- a/tools/libxl/libxl.c
> > > +++ b/tools/libxl/libxl.c
> > > @@ -98,6 +98,20 @@ void
> > > libxl_key_value_list_destroy(libxl_key_value_list kvl)
> > >      free(kvl);
> > >  }
> > >  
> > > +void libxl_cpuid_destroy(libxl_cpuid_type *cpuid)
> > > +{
> > > +    int i, j;
> > > +
> > > +    if (cpuid == NULL)
> > > +        return;
> > > +    for (i = 0; cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
> > > +        for (j = 0; j < 4; j++)
> > > +            if (cpuid[i].policy[j] != NULL)
> > > +                free(cpuid[i].policy[j]);
> > > +    }
> > > +    free(cpuid);
> > > +}
> > 
> > This can be auto-generated.
> 
> The type is defined as a Builtin in the IDL, in that case hand coding
> the destructor is valid.
> 
> >  Also libxl_*_destroy() functions never call
> > free on the passed pointer. Hence ending _destroy() rather than _free().
> 
> There are some exceptions, such as the FOO_list builtins but as it
> stands I don't think this is one of them. The free() _should_ have come
> from the use of the Reference type in the IDL. This would be the first
> non-const use of Reference it though so it is possible that gentypes.py
> is not 100% correct in its handling of Reference types.

Hmm, string list and kv list it appears. Well in that case they ought be
renamed _free() - it's very confusing otherwise.

> However I think overall it would be better to define an explicit list
> type e.g. "typedef libxl_cpuid_type *libxl_cpuid_type_list" and use that
> as the IDL builtin (without the Reference since it already has one),
> similar to the existing string list builtins. In that case it would be
> valid for libxl_cpuid_type_list_destroy to free the actual list as well
> as the contents.

That's fine with me as long as a decent naming convention is kept.

Re: [PATCH 1/2] xl: add cpuid parameter

[Ian Campbell]

On Fri, 2010-08-27 at 15:51 +0100, Ian Campbell wrote:
> This would be the first non-const use of Reference it though so it is
> possible that gentypes.py is not 100% correct in its handling of
> Reference types.

I think there is a bug in this handling and I think this patch fixes it.

Subject: libxl: correctly free Reference types in autogenerated destroy functions

References types should be recursively destroyed and then the actual
reference itself should be destroyed.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r a1612bdf9cb3 tools/libxl/gentypes.py
--- a/tools/libxl/gentypes.py	Fri Aug 27 15:40:05 2010 +0100
+++ b/tools/libxl/gentypes.py	Fri Aug 27 15:57:25 2010 +0100
@@ -64,6 +64,11 @@ def libxl_C_type_destroy(ty, v, referenc
         deref = v + "->"
     else:
         deref = v + "."
+        
+    if ty.passby == libxltypes.PASS_BY_REFERENCE and not reference:
+        makeref = "&"
+    else:
+        makeref = ""
 
     s = ""
     if isinstance(ty, libxltypes.KeyedUnion):
@@ -76,6 +81,8 @@ def libxl_C_type_destroy(ty, v, referenc
             s += "}\n"
     elif isinstance(ty, libxltypes.Reference):
         s += libxl_C_type_destroy(ty.ref_type, v, True, indent, v)
+        if ty.destructor_fn is not None:
+            s += "%s(%s);\n" % (ty.destructor_fn, makeref + v)
     elif isinstance(ty, libxltypes.Struct) and (parent is None or ty.destructor_fn is None):
         for f in [f for f in ty.fields if not f.const]:
 
@@ -84,11 +91,6 @@ def libxl_C_type_destroy(ty, v, referenc
             else:
                 s += libxl_C_type_destroy(f.type, deref + f.name, False, "", deref)
     else:
-        if ty.passby == libxltypes.PASS_BY_REFERENCE and not reference:
-            makeref = "&"
-        else:
-            makeref = ""
-
         if ty.destructor_fn is not None:
             s += "%s(%s);\n" % (ty.destructor_fn, makeref + v)

Re: [PATCH 1/2] xl: add cpuid parameter

[Ian Campbell]

On Fri, 2010-08-27 at 15:56 +0100, Gianni Tedesco (3P) wrote:
> On Fri, 2010-08-27 at 15:51 +0100, Ian Campbell wrote:
> > On Fri, 2010-08-27 at 15:07 +0100, Gianni Tedesco wrote:
> > > On Fri, 2010-08-27 at 13:56 +0100, Andre Przywara wrote:
> > 
> > Thanks Andre.
> > 
> > > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > > index 099d82e..da9c7fd 100644
> > > > --- a/tools/libxl/libxl.c
> > > > +++ b/tools/libxl/libxl.c
> > > > @@ -98,6 +98,20 @@ void
> > > > libxl_key_value_list_destroy(libxl_key_value_list kvl)
> > > >      free(kvl);
> > > >  }
> > > >  
> > > > +void libxl_cpuid_destroy(libxl_cpuid_type *cpuid)
> > > > +{
> > > > +    int i, j;
> > > > +
> > > > +    if (cpuid == NULL)
> > > > +        return;
> > > > +    for (i = 0; cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
> > > > +        for (j = 0; j < 4; j++)
> > > > +            if (cpuid[i].policy[j] != NULL)
> > > > +                free(cpuid[i].policy[j]);
> > > > +    }
> > > > +    free(cpuid);
> > > > +}
> > > 
> > > This can be auto-generated.
> > 
> > The type is defined as a Builtin in the IDL, in that case hand coding
> > the destructor is valid.
> > 
> > >  Also libxl_*_destroy() functions never call
> > > free on the passed pointer. Hence ending _destroy() rather than _free().
> > 
> > There are some exceptions, such as the FOO_list builtins but as it
> > stands I don't think this is one of them. The free() _should_ have come
> > from the use of the Reference type in the IDL. This would be the first
> > non-const use of Reference it though so it is possible that gentypes.py
> > is not 100% correct in its handling of Reference types.
> 
> Hmm, string list and kv list it appears. Well in that case they ought be
> renamed _free() - it's very confusing otherwise.

In these cases the type itself is a list rather than having a list of a
given type, this is why it is appropriate for the destructor to destroy
the list itself as well as the contents.

It's a bit subtle but I think it is correct and consistent with the
behaviour of Reference types for example (modulo the bug I just sent a
patch for).

Ian.

Re: [PATCH 1/2] xl: add cpuid parameter

[Ian Campbell]

On Fri, 2010-08-27 at 16:07 +0100, Ian Campbell wrote:
> On Fri, 2010-08-27 at 15:56 +0100, Gianni Tedesco (3P) wrote:
> > On Fri, 2010-08-27 at 15:51 +0100, Ian Campbell wrote:
> > > On Fri, 2010-08-27 at 15:07 +0100, Gianni Tedesco wrote:
> > > > On Fri, 2010-08-27 at 13:56 +0100, Andre Przywara wrote:
> > > 
> > > Thanks Andre.
> > > 
> > > > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > > > index 099d82e..da9c7fd 100644
> > > > > --- a/tools/libxl/libxl.c
> > > > > +++ b/tools/libxl/libxl.c
> > > > > @@ -98,6 +98,20 @@ void
> > > > > libxl_key_value_list_destroy(libxl_key_value_list kvl)
> > > > >      free(kvl);
> > > > >  }
> > > > >  
> > > > > +void libxl_cpuid_destroy(libxl_cpuid_type *cpuid)
> > > > > +{
> > > > > +    int i, j;
> > > > > +
> > > > > +    if (cpuid == NULL)
> > > > > +        return;
> > > > > +    for (i = 0; cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
> > > > > +        for (j = 0; j < 4; j++)
> > > > > +            if (cpuid[i].policy[j] != NULL)
> > > > > +                free(cpuid[i].policy[j]);
> > > > > +    }
> > > > > +    free(cpuid);
> > > > > +}
> > > > 
> > > > This can be auto-generated.
> > > 
> > > The type is defined as a Builtin in the IDL, in that case hand coding
> > > the destructor is valid.
> > > 
> > > >  Also libxl_*_destroy() functions never call
> > > > free on the passed pointer. Hence ending _destroy() rather than _free().
> > > 
> > > There are some exceptions, such as the FOO_list builtins but as it
> > > stands I don't think this is one of them. The free() _should_ have come
> > > from the use of the Reference type in the IDL. This would be the first
> > > non-const use of Reference it though so it is possible that gentypes.py
> > > is not 100% correct in its handling of Reference types.
> > 
> > Hmm, string list and kv list it appears. Well in that case they ought be
> > renamed _free() - it's very confusing otherwise.
> 
> In these cases the type itself is a list rather than having a list of a
> given type, this is why it is appropriate for the destructor to destroy
> the list itself as well as the contents.
> 
> It's a bit subtle but I think it is correct and consistent with the
> behaviour of Reference types for example (modulo the bug I just sent a
> patch for).

Actually, what was wrong/confusing was that the destroy function was
written such that it used one of the references within the type in order
to pass-by-reference instead defining the type as pass-by-reference in
the IDL. The existing code works and is "correct" but is semantically
wrong.

I've just sent out a short series of patches to the autogenerator which
includes the fix for this issue.

Ian.

Re: [PATCH 1/2] xl: add cpuid parameter

[Gianni Tedesco]

On Fri, 2010-08-27 at 16:07 +0100, Ian Campbell wrote:
> On Fri, 2010-08-27 at 15:56 +0100, Gianni Tedesco (3P) wrote:
> > On Fri, 2010-08-27 at 15:51 +0100, Ian Campbell wrote:
> > > On Fri, 2010-08-27 at 15:07 +0100, Gianni Tedesco wrote:
> > > > On Fri, 2010-08-27 at 13:56 +0100, Andre Przywara wrote:
> > > 
> > > Thanks Andre.
> > > 
> > > > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > > > index 099d82e..da9c7fd 100644
> > > > > --- a/tools/libxl/libxl.c
> > > > > +++ b/tools/libxl/libxl.c
> > > > > @@ -98,6 +98,20 @@ void
> > > > > libxl_key_value_list_destroy(libxl_key_value_list kvl)
> > > > >      free(kvl);
> > > > >  }
> > > > >  
> > > > > +void libxl_cpuid_destroy(libxl_cpuid_type *cpuid)
> > > > > +{
> > > > > +    int i, j;
> > > > > +
> > > > > +    if (cpuid == NULL)
> > > > > +        return;
> > > > > +    for (i = 0; cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
> > > > > +        for (j = 0; j < 4; j++)
> > > > > +            if (cpuid[i].policy[j] != NULL)
> > > > > +                free(cpuid[i].policy[j]);
> > > > > +    }
> > > > > +    free(cpuid);
> > > > > +}
> > > > 
> > > > This can be auto-generated.
> > > 
> > > The type is defined as a Builtin in the IDL, in that case hand coding
> > > the destructor is valid.
> > > 
> > > >  Also libxl_*_destroy() functions never call
> > > > free on the passed pointer. Hence ending _destroy() rather than _free().
> > > 
> > > There are some exceptions, such as the FOO_list builtins but as it
> > > stands I don't think this is one of them. The free() _should_ have come
> > > from the use of the Reference type in the IDL. This would be the first
> > > non-const use of Reference it though so it is possible that gentypes.py
> > > is not 100% correct in its handling of Reference types.
> > 
> > Hmm, string list and kv list it appears. Well in that case they ought be
> > renamed _free() - it's very confusing otherwise.
> 
> In these cases the type itself is a list rather than having a list of a
> given type, this is why it is appropriate for the destructor to destroy
> the list itself as well as the contents.
> 
> It's a bit subtle but I think it is correct and consistent with the
> behaviour of Reference types for example (modulo the bug I just sent a
> patch for).

Actually I thought about this, for a bit too long because it is subtle,
but in the end I agree.