xl: add cpuid parameter

PATCH In review Andre Przywara 4 participants 8 messages Aug 27, 2010 – Aug 31, 2010

lore

Share & tools

lore

Andre Przywara

author

2010-08-27 12:56 UTC #0 lore raw

PATCH 1/2 xl: add cpuid parameter

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 <[email protected]>

Signature · 3 lines

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

Quoted 1 line

From 73b3800344ba977de88cd79a3d7c3bfd558fd307 Mon Sep 17 00:00:00 2001

From: Andre Przywara <[email protected]>
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(-)

tools/libxl/libxl.c +14 -0

index 099d82e..da9c7fd 100644
@@ -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,

tools/libxl/libxl.h +6 -0

index f6480fb..b8fa8ca 100644
@@ -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

tools/libxl/libxl.idl +2 -0

index f59950d..dc988d5 100644
@@ -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,

tools/libxl/libxl_dom.c +6 -0

index a07bea7..ec4e15f 100644
@@ -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 *));

tools/libxl/xl_cmdimpl.c +1 -0

index ae4fcc3..0fa516b 100644
@@ -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;

Signature · 1 line

1.6.4

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Gianni Tedesco

2010-08-27 14:07 UTC #1 lore raw

Quoted 12 lines · Andre Przywara

tools/libxl/libxl.c +3 -0

index 099d82e..da9c7fd 100644
@@ -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;

tools/libxl/libxl.c void · line 114

Quoted 10 lines

tools/libxl/libxl.c +10 -0

+
+    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().

Ian Campbell

2010-08-27 14:51 UTC #2 lore raw

Quoted 1 line · Gianni Tedesco

On Fri, 2010-08-27 at 13:56 +0100, Andre Przywara wrote:

Thanks Andre.

Quoted 9 lines

tools/libxl/libxl.c +0 -0

index 099d82e..da9c7fd 100644
@@ -98,6 +98,20 @@ void
 libxl_key_value_list_destroy(libxl_key_value_list kvl)
     free(kvl);
 }

Quoted 15 lines

tools/libxl/libxl.c void libxl_key_value_list_destroy(libxl_key_value_list kvl) · line 113

Quoted 13 lines

tools/libxl/libxl.c +13 -0

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

Quoted 2 lines

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.

Gianni Tedesco

2010-08-27 14:56 UTC #3 lore raw

Quoted 33 lines · Ian Campbell

Quoted 1 line · Gianni Tedesco
On Fri, 2010-08-27 at 13:56 +0100, Andre Przywara wrote:
Thanks Andre.
Quoted 9 lines
Quoted 9 lines
tools/libxl/libxl.c +0 -0
index 099d82e..da9c7fd 100644
@@ -98,6 +98,20 @@ void
 libxl_key_value_list_destroy(libxl_key_value_list kvl)
     free(kvl);
 }
 
Quoted 15 lines
tools/libxl/libxl.c void libxl_key_value_list_destroy(libxl_key_value_list kvl) · line 113
Quoted 13 lines
tools/libxl/libxl.c +13 -0
+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.

Quoted 8 lines

Quoted 2 lines
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.

Quoted 6 lines

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.

Ian Campbell

2010-08-27 14:59 UTC #7 lore raw

In reply to Ian Campbell

Quoted 3 lines · Ian Campbell

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 <[email protected]>

tools/libxl/gentypes.py +7 -5

diff -r a1612bdf9cb3 tools/libxl/gentypes.py
@@ -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)

Ian Campbell

2010-08-27 15:07 UTC #4 lore raw

In reply to Gianni Tedesco

Quoted 37 lines · On Fri, 2010-08-27 at 15:56 +0100, Gianni Tedesco (3P) wrote:

Quoted 36 lines · Ian Campbell
Quoted 1 line · Gianni Tedesco
On Fri, 2010-08-27 at 13:56 +0100, Andre Przywara wrote:
Thanks Andre.
Quoted 9 lines
Quoted 9 lines
tools/libxl/libxl.c +0 -0
index 099d82e..da9c7fd 100644
@@ -98,6 +98,20 @@ void
 libxl_key_value_list_destroy(libxl_key_value_list kvl)
     free(kvl);
 }
 
Quoted 15 lines
tools/libxl/libxl.c void libxl_key_value_list_destroy(libxl_key_value_list kvl) · line 113
Quoted 13 lines
tools/libxl/libxl.c +13 -0
+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.
Quoted 2 lines
Also libxl_*_destroy() functions never call
free on the passed pointer. Hence ending _destroy() rather than _free().

Quoted 8 lines

Quoted 5 lines
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.

Ian Campbell

2010-08-27 15:36 UTC #5 lore raw

Quoted 47 lines · Ian Campbell

Quoted 37 lines · On Fri, 2010-08-27 at 15:56 +0100, Gianni Tedesco (3P) wrote:
Quoted 36 lines · Ian Campbell
Quoted 1 line · Gianni Tedesco
On Fri, 2010-08-27 at 13:56 +0100, Andre Przywara wrote:
Thanks Andre.
Quoted 9 lines
Quoted 9 lines
tools/libxl/libxl.c +0 -0
index 099d82e..da9c7fd 100644
@@ -98,6 +98,20 @@ void
 libxl_key_value_list_destroy(libxl_key_value_list kvl)
     free(kvl);
 }
 
Quoted 15 lines
tools/libxl/libxl.c void libxl_key_value_list_destroy(libxl_key_value_list kvl) · line 113
Quoted 13 lines
tools/libxl/libxl.c +13 -0
+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.
Quoted 2 lines
Also libxl_*_destroy() functions never call
free on the passed pointer. Hence ending _destroy() rather than _free().
Quoted 8 lines
Quoted 5 lines
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.

Quoted 7 lines

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.

Gianni Tedesco

2010-08-31 15:35 UTC #6 lore raw

In reply to Ian Campbell

Quoted 47 lines · Ian Campbell

Quoted 37 lines · On Fri, 2010-08-27 at 15:56 +0100, Gianni Tedesco (3P) wrote:
Quoted 36 lines · Ian Campbell
Quoted 1 line · Gianni Tedesco
On Fri, 2010-08-27 at 13:56 +0100, Andre Przywara wrote:
Thanks Andre.
Quoted 9 lines
Quoted 9 lines
tools/libxl/libxl.c +0 -0
index 099d82e..da9c7fd 100644
@@ -98,6 +98,20 @@ void
 libxl_key_value_list_destroy(libxl_key_value_list kvl)
     free(kvl);
 }
 
Quoted 15 lines
tools/libxl/libxl.c void libxl_key_value_list_destroy(libxl_key_value_list kvl) · line 113
Quoted 13 lines
tools/libxl/libxl.c +13 -0
+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.
Quoted 2 lines
Also libxl_*_destroy() functions never call
free on the passed pointer. Hence ending _destroy() rather than _free().
Quoted 8 lines
Quoted 5 lines
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.

Quoted 7 lines

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.