Xen devel - Sep 2012 - [PATCH 09/11] add iomem support to libxl

This patch adds a new option for xen config files for
directly mapping hardware io memory into a vm.

Signed-off-by: Matthew Fioravante <matthew.fioravante@jhuapl.edu>

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 013270d..428da21 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -496,6 +496,17 @@ is given in hexadecimal and may either a span e.g.
C<2f8-2ff>
 It is recommended to use this option only for trusted VMs under
 administrator control.
 
+=item B<iomem=[ "IOMEM_START,NUM_PAGES",
"IOMEM_START,NUM_PAGES", ... ]>
+
+Allow guest to access specific hardware I/O memory pages. B<IOMEM_START>
+is a physical page number. B<NUM_PAGES> is the number
+of pages beginning with B<START_PAGE> to allow access. Both values
+must be given in hexadecimal.
+
+It is recommended to use this option only for trusted VMs under
+administrator control.
+
+
 =item B<irqs=[ NUMBER, NUMBER, ... ]>
 
 Allow a guest to access specific physical IRQs.
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index ef17f05..6cb586b 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -963,6 +963,24 @@ static void domcreate_launch_dm(libxl__egc *egc,
libxl__multidev *multidev,
         }
     }
 
+    for (i = 0; i < d_config->b_info.num_iomem; i++) {
+        libxl_iomem_range *io = &d_config->b_info.iomem[i];
+
+        LOG(DEBUG, "dom%d iomem %"PRIx64"-%"PRIx64,
+            domid, io->start, io->start + io->number - 1);
+
+        ret = xc_domain_iomem_permission(CTX->xch, domid,
+                                          io->start, io->number, 1);
+        if ( ret<0 ) {
+            LOGE(ERROR,
+                 "failed give dom%d access to iomem range
%"PRIx64"-%"PRIx64,
+                 domid, io->start, io->start + io->number - 1);
+            ret = ERROR_FAIL;
+        }
+    }
+
+
+
     for (i = 0; i < d_config->num_nics; i++) {
         /* We have to init the nic here, because we still haven''t
          * called libxl_device_nic_add at this point, but qemu needs
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 6d5c578..cf83c60 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -140,6 +140,11 @@ libxl_ioport_range = Struct("ioport_range", [
     ("number", uint32),
     ])
 
+libxl_iomem_range = Struct("iomem_range", [
+    ("start", uint64),
+    ("number", uint64),
+    ])
+
 libxl_vga_interface_info = Struct("vga_interface_info", [
     ("kind",    libxl_vga_interface_type),
     ])
@@ -284,6 +289,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
 
     ("ioports",          Array(libxl_ioport_range,
"num_ioports")),
     ("irqs",             Array(uint32, "num_irqs")),
+    ("iomem",            Array(libxl_iomem_range,
"num_iomem")),
 
     ("u", KeyedUnion(None, libxl_domain_type, "type",
                 [("hvm", Struct(None, [("firmware",        
string),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 1627cac..fe8e925 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -574,8 +574,8 @@ static void parse_config_data(const char *config_source,
     long l;
     XLU_Config *config;
     XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids;
-    XLU_ConfigList *ioports, *irqs;
-    int num_ioports, num_irqs;
+    XLU_ConfigList *ioports, *irqs, *iomem;
+    int num_ioports, num_irqs, num_iomem;
     int pci_power_mgmt = 0;
     int pci_msitranslate = 0;
     int pci_permissive = 0;
@@ -1005,6 +1005,30 @@ static void parse_config_data(const char *config_source,
         }
     }
 
+    if (!xlu_cfg_get_list(config, "iomem", &iomem,
&num_iomem, 0)) {
+        b_info->num_iomem = num_iomem;
+        b_info->iomem = calloc(num_iomem, sizeof(*b_info->iomem));
+        if (b_info->iomem == NULL) {
+            fprintf(stderr, "unable to allocate memory for iomem\n");
+            exit(-1);
+        }
+        for (i = 0; i < num_iomem; i++) {
+            buf = xlu_cfg_get_listitem (iomem, i);
+            if (!buf) {
+                fprintf(stderr,
+                        "xl: Unable to get element %d in iomem
list\n", i);
+                exit(1);
+            }
+            if(sscanf(buf, "%" SCNx64",%" SCNx64,
&b_info->iomem[i].start, &b_info->iomem[i].number) != 2) {
+               fprintf(stderr,
+                       "xl: Invalid argument parsing iomem: %s\n",
buf);
+               exit(1);
+            }
+        }
+    }
+
+
+
     if (!xlu_cfg_get_list (config, "disk", &vbds, 0, 0)) {
         d_config->num_disks = 0;
         d_config->disks = NULL;
-- 
1.7.9.5

Previously device ids in libxl were treated as integers meaning they
were being initialized to 0, which is a valid device id. This patch
makes devid its own type in libxl and initializes it to -1, an invalid
value.

This fixes a bug where if you try to do a xl DEV-attach multiple
time it will continuously try to reattach device 0 instead of
generating a new device id.

Signed-off-by: Matthew Fioravante <matthew.fioravante@jhuapl.edu>

diff --git a/tools/libxl/gentest.py b/tools/libxl/gentest.py
index 2915f71..84b4fd7 100644
--- a/tools/libxl/gentest.py
+++ b/tools/libxl/gentest.py
@@ -60,7 +60,7 @@ def gen_rand_init(ty, v, indent = "    ", parent =
None):
                                         passby=idl.PASS_BY_REFERENCE))
     elif ty.typename in ["libxl_uuid", "libxl_mac",
"libxl_hwcap"]:
         s += "rand_bytes((uint8_t *)%s, sizeof(*%s));\n" % (v,v)
-    elif ty.typename in ["libxl_domid"] or isinstance(ty,
idl.Number):
+    elif ty.typename in ["libxl_domid", "libxl_devid"] or
isinstance(ty, idl.Number):
         s += "%s = rand() %% (sizeof(%s)*8);\n" % \
              (ty.pass_arg(v, parent is None),
               ty.pass_arg(v, parent is None))
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 599c7f1..7a7c419 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -307,6 +307,7 @@ void libxl_cpuid_dispose(libxl_cpuid_policy_list
*cpuid_list);
 #define LIBXL_PCI_FUNC_ALL (~0U)
 
 typedef uint32_t libxl_domid;
+typedef int libxl_devid;
 
 /*
  * Formatting Enumerations.
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index cf83c60..de111a6 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -8,6 +8,7 @@ namespace("libxl_")
 libxl_defbool = Builtin("defbool", passby=PASS_BY_REFERENCE)
 
 libxl_domid = Builtin("domid", json_fn =
"yajl_gen_integer", autogenerate_json = False)
+libxl_devid = Builtin("devid", json_fn =
"yajl_gen_integer", autogenerate_json = False, signed = True,
init_val="-1")
 libxl_uuid = Builtin("uuid", passby=PASS_BY_REFERENCE)
 libxl_mac = Builtin("mac", passby=PASS_BY_REFERENCE)
 libxl_bitmap = Builtin("bitmap",
dispose_fn="libxl_bitmap_dispose", passby=PASS_BY_REFERENCE)
@@ -343,7 +344,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
 
 libxl_device_vfb = Struct("device_vfb", [
     ("backend_domid", libxl_domid),
-    ("devid",         integer),
+    ("devid",         libxl_devid),
     ("vnc",           libxl_vnc_info),
     ("sdl",           libxl_sdl_info),
     # set keyboard layout, default is en-us keyboard
@@ -352,7 +353,7 @@ libxl_device_vfb = Struct("device_vfb", [
 
 libxl_device_vkb = Struct("device_vkb", [
     ("backend_domid", libxl_domid),
-    ("devid", integer),
+    ("devid", libxl_devid),
     ])
 
 libxl_device_disk = Struct("device_disk", [
@@ -369,7 +370,7 @@ libxl_device_disk = Struct("device_disk", [
 
 libxl_device_nic = Struct("device_nic", [
     ("backend_domid", libxl_domid),
-    ("devid", integer),
+    ("devid", libxl_devid),
     ("mtu", integer),
     ("model", string),
     ("mac", libxl_mac),
@@ -399,7 +400,7 @@ libxl_diskinfo = Struct("diskinfo", [
     ("backend_id", uint32),
     ("frontend", string),
     ("frontend_id", uint32),
-    ("devid", integer),
+    ("devid", libxl_devid),
     ("state", integer),
     ("evtch", integer),
     ("rref", integer),
@@ -410,7 +411,7 @@ libxl_nicinfo = Struct("nicinfo", [
     ("backend_id", uint32),
     ("frontend", string),
     ("frontend_id", uint32),
-    ("devid", integer),
+    ("devid", libxl_devid),
     ("state", integer),
     ("evtch", integer),
     ("rref_tx", integer),
diff --git a/tools/ocaml/libs/xl/genwrap.py b/tools/ocaml/libs/xl/genwrap.py
index 42f374e..97d088d 100644
--- a/tools/ocaml/libs/xl/genwrap.py
+++ b/tools/ocaml/libs/xl/genwrap.py
@@ -10,6 +10,7 @@ builtins = {
     "int":                  ("int",                   
"%(c)s = Int_val(%(o)s)",            "Val_int(%(c)s)"  ),
     "char *":               ("string",                
"%(c)s = dup_String_val(gc, %(o)s)",
"caml_copy_string(%(c)s)"),
     "libxl_domid":          ("domid",                 
"%(c)s = Int_val(%(o)s)",            "Val_int(%(c)s)"  ),
+    "libxl_devid":          ("devid",                 
"%(c)s = Int_val(%(o)s)",            "Val_int(%(c)s)"  ),
     "libxl_defbool":        ("bool option",           
"%(c)s = Defbool_val(%(o)s)",        "Val_defbool(%(c)s)" ),
     "libxl_uuid":           ("int array",             
"Uuid_val(gc, lg, &%(c)s, %(o)s)",  
"Val_uuid(&%(c)s)"),
     "libxl_key_value_list": ("(string * string) list",
None,                                None),
@@ -41,8 +42,8 @@ def stub_fn_name(ty, name):
     return "stub_xl_%s_%s" % (ty.rawname,name)
     
 def ocaml_type_of(ty):
-    if ty.rawname == "domid":
-        return "domid"
+    if ty.rawname in ["domid","devid"]:
+        return ty.rawname
     elif isinstance(ty,idl.UInt):
         if ty.width in [8, 16]:
             # handle as ints
diff --git a/tools/ocaml/libs/xl/xenlight.ml.in
b/tools/ocaml/libs/xl/xenlight.ml.in
index c47623c..dcc1a38 100644
--- a/tools/ocaml/libs/xl/xenlight.ml.in
+++ b/tools/ocaml/libs/xl/xenlight.ml.in
@@ -16,6 +16,7 @@
 exception Error of string
 
 type domid = int
+type devid = int
 
 (* @@LIBXL_TYPES@@ *)
 
diff --git a/tools/ocaml/libs/xl/xenlight.mli.in
b/tools/ocaml/libs/xl/xenlight.mli.in
index 4717bac..3fd0165 100644
--- a/tools/ocaml/libs/xl/xenlight.mli.in
+++ b/tools/ocaml/libs/xl/xenlight.mli.in
@@ -16,6 +16,7 @@
 exception Error of string
 
 type domid = int
+type devid = int
 
 (* @@LIBXL_TYPES@@ *)
 
diff --git a/tools/python/xen/lowlevel/xl/xl.c
b/tools/python/xen/lowlevel/xl/xl.c
index 0551c76..32f982a 100644
--- a/tools/python/xen/lowlevel/xl/xl.c
+++ b/tools/python/xen/lowlevel/xl/xl.c
@@ -281,6 +281,11 @@ int attrib__libxl_domid_set(PyObject *v, libxl_domid
*domid) {
     return 0;
 }
 
+int attrib__libxl_devid_set(PyObject *v, libxl_devid *devid) {
+   *devid = PyInt_AsLong(v);
+   return 0;
+}
+
 int attrib__struct_in_addr_set(PyObject *v, struct in_addr *pptr)
 {
     PyErr_SetString(PyExc_NotImplementedError, "Setting in_addr");
@@ -342,6 +347,10 @@ PyObject *attrib__libxl_domid_get(libxl_domid *domid) {
     return PyInt_FromLong(*domid);
 }
 
+PyObject *attrib__libxl_devid_get(libxl_devid *devid) {
+    return PyInt_FromLong(*devid);
+}
+
 PyObject *attrib__struct_in_addr_get(struct in_addr *pptr)
 {
     PyErr_SetString(PyExc_NotImplementedError, "Getting in_addr");
-- 
1.7.9.5

This patch adds vtpm support to libxl. It adds vtpm parsing to config
files and 3 new xl commands:
vtpm-attach
vtpm-detach
vtpm-list

Signed-off-by: Matthew Fioravante <matthew.fioravante@jhuapl.edu>

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 428da21..edeeefa 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -298,6 +298,35 @@ Specifies the networking provision (both emulated network
adapters,
 and Xen virtual interfaces) to provided to the guest.  See
 F<docs/misc/xl-network-configuration.markdown>.
 
+=item B<vtpm=[ "VTPM_SPEC_STRING", "VTPM_SPEC_STRING",
...]>
+
+Specifies the virtual trusted platform module to be
+provided to the guest. Please see F<docs/misc/vtpm.txt>
+for more details.
+
+Each B<VTPM_SPEC_STRING> is a comma-separated list of C<KEY=VALUE>
+settings, from the following list:
+
+=over 4
+
+=item C<backend=DOMAIN>
+
+Specify the backend domain name of id. This value must be
+set if you are using the vtpm domain model. If this domain
+is a guest, the backend should be set to the vtpm domain name.
+If this domain is a vtpm, the backend should be set to the
+vtpm manager domain name. The default value is domain 0,
+which should be used if you are running the vtpm process model.
+
+=item C<uuid=UUID>
+
+Specify the uuid of this vtpm device. The uuid is used to uniquely
+identify the vtpm device. You can create one using the uuidgen
+program on unix systems. If left unspecified, a new uuid
+will be randomly generated every time the domain boots.
+
+=back
+
 =item B<vfb=[ "VFB_SPEC_STRING", "VFB_SPEC_STRING",
...]>
 
 Specifies the paravirtual framebuffer devices which should be supplied
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 25ce777..be9ad4c 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1101,6 +1101,31 @@ List virtual network interfaces for a domain.
 
 =back
 
+=head2 VTPM DEVICES
+
+=over 4
+
+=item B<vtpm-attach> I<domain-id> I<vtpm-device>
+
+Creates a new vtpm device in the domain specified by I<domain-id>.
+I<vtpm-device> describes the device to attach, using the same format as
the
+B<vtpm> string in the domain config file. See L<xl.cfg> for
+more information.
+
+=item B<vtpm-detach> I<domain-id> I<devid|uuid>
+
+Removes the vtpm device from the domain specified by I<domain-id>.
+I<devid> is the numeric device id given to the virtual trusted
+platform module device. You will need to run B<xl vtpm-list> to determine
that number.
+Alternatively the I<uuid> of the vtpm can be used to
+select the virtual device to detach.
+
+=item B<vtpm-list> I<domain-id>
+
+List virtual trusted platform modules for a domain.
+
+=back
+
 =head1 PCI PASS-THROUGH
 
 =over 4
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 1606eb1..f613487 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1726,6 +1726,246 @@ out:
 }
 
 /******************************************************************************/
+int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm)
+{
+   if(libxl_uuid_is_nil(&vtpm->uuid)) {
+      libxl_uuid_generate(&vtpm->uuid);
+   }
+   return 0;
+}
+
+static int libxl__device_from_vtpm(libxl__gc *gc, uint32_t domid,
+                                   libxl_device_vtpm *vtpm,
+                                   libxl__device *device)
+{
+   device->backend_devid   = vtpm->devid;
+   device->backend_domid   = vtpm->backend_domid;
+   device->backend_kind    = LIBXL__DEVICE_KIND_VTPM;
+   device->devid           = vtpm->devid;
+   device->domid           = domid;
+   device->kind            = LIBXL__DEVICE_KIND_VTPM;
+
+   return 0;
+}
+
+void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
+                           libxl_device_vtpm *vtpm,
+                           libxl__ao_device *aodev)
+{
+    STATE_AO_GC(aodev->ao);
+    flexarray_t *front;
+    flexarray_t *back;
+    libxl__device *device;
+    char *dompath, **l;
+    unsigned int nb, rc;
+
+    rc = libxl__device_vtpm_setdefault(gc, vtpm);
+    if (rc) goto out;
+
+    front = flexarray_make(16, 1);
+    if (!front) {
+        rc = ERROR_NOMEM;
+        goto out;
+    }
+    back = flexarray_make(16, 1);
+    if (!back) {
+        rc = ERROR_NOMEM;
+        goto out;
+    }
+
+    if(vtpm->devid == -1) {
+        if (!(dompath = libxl__xs_get_dompath(gc, domid))) {
+            rc = ERROR_FAIL;
+            goto out_free;
+        }
+        l = libxl__xs_directory(gc, XBT_NULL, libxl__sprintf(gc,
"%s/device/vtpm", dompath), &nb);
+        if(l == NULL || nb == 0) {
+            vtpm->devid = 0;
+        } else {
+            vtpm->devid = strtoul(l[nb - 1], NULL, 10) + 1;
+        }
+    }
+
+    GCNEW(device);
+    rc = libxl__device_from_vtpm(gc, domid, vtpm, device);
+    if ( rc != 0 ) goto out_free;
+
+    flexarray_append(back, "frontend-id");
+    flexarray_append(back, libxl__sprintf(gc, "%d", domid));
+    flexarray_append(back, "online");
+    flexarray_append(back, "1");
+    flexarray_append(back, "state");
+    flexarray_append(back, libxl__sprintf(gc, "%d", 1));
+
+    flexarray_append(back, "uuid");
+    flexarray_append(back, libxl__sprintf(gc, LIBXL_UUID_FMT,
LIBXL_UUID_BYTES(vtpm->uuid)));
+    flexarray_append(back, "instance"); /* MAYBE CAN GET RID OF THIS
*/
+    flexarray_append(back, "0");
+    flexarray_append(back, "pref_instance"); /* MAYBE CAN GET RID OF
THIS */
+    flexarray_append(back, "0");
+    flexarray_append(back, "resume");
+    flexarray_append(back, "False");
+    flexarray_append(back, "ready"); /* MAYBE CAN GET RID OF THIS */
+    flexarray_append(back, "1");
+
+    flexarray_append(front, "backend-id");
+    flexarray_append(front, libxl__sprintf(gc, "%d",
vtpm->backend_domid));
+    flexarray_append(front, "state");
+    flexarray_append(front, libxl__sprintf(gc, "%d", 1));
+    flexarray_append(front, "handle");
+    flexarray_append(front, libxl__sprintf(gc, "%d",
vtpm->devid));
+
+    libxl__device_generic_add(gc, XBT_NULL, device,
+                             libxl__xs_kvs_of_flexarray(gc, back,
back->count),
+                             libxl__xs_kvs_of_flexarray(gc, front,
front->count));
+
+    aodev->dev = device;
+    aodev->action = DEVICE_CONNECT;
+    libxl__wait_device_connection(egc, aodev);
+
+    rc = 0;
+out_free:
+    flexarray_free(back);
+    flexarray_free(front);
+out:
+    aodev->rc = rc;
+    if(rc) aodev->callback(egc, aodev);
+    return;
+}
+
+static void libxl__device_vtpm_from_xs_fe(libxl__gc *gc,
+                                          const char* fe_path,
+                                          libxl_device_vtpm *vtpm)
+{
+   char* tmp;
+
+   memset(vtpm, 0, sizeof(*vtpm));
+   tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/handle",
fe_path));
+   if (tmp) {
+      vtpm->devid = atoi(tmp);
+   }
+   tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc,
"%s/backend-id", fe_path));
+   if(tmp) {
+      vtpm->backend_domid = atoi(tmp);
+   }
+   tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/uuid",
fe_path));
+   if(tmp) {
+      libxl_uuid_from_string(&(vtpm->uuid), tmp);
+   }
+}
+
+libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t domid, int
*num)
+{
+    GC_INIT(ctx);
+
+    libxl_device_vtpm* vtpms = NULL;
+    char* fe_path = NULL;
+    char** dir = NULL;
+    unsigned int ndirs = 0;
+
+    *num = 0;
+
+    fe_path = libxl__sprintf(gc, "%s/device/vtpm",
libxl__xs_get_dompath(gc, domid));
+    dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &ndirs);
+    if(dir) {
+       vtpms = malloc(sizeof(*vtpms) * ndirs);
+       libxl_device_vtpm* vtpm;
+       libxl_device_vtpm* end = vtpms + ndirs;
+       for(vtpm = vtpms; vtpm < end; ++vtpm, ++dir) {
+          const char* path = libxl__sprintf(gc, "%s/%s", fe_path,
*dir);
+          libxl__device_vtpm_from_xs_fe(gc, path, vtpm);
+       }
+    }
+    *num = ndirs;
+
+    GC_FREE;
+    return vtpms;
+}
+
+int libxl_device_vtpm_getinfo(libxl_ctx *ctx, uint32_t domid,
+                               libxl_device_vtpm *vtpm, libxl_vtpminfo
*vtpminfo)
+{
+    GC_INIT(ctx);
+    char *dompath, *vtpmpath;
+    char *val;
+    int rc = 0;
+
+    libxl_vtpminfo_init(vtpminfo);
+    dompath = libxl__xs_get_dompath(gc, domid);
+    vtpminfo->devid = vtpm->devid;
+
+    vtpmpath = libxl__sprintf(gc, "%s/device/vtpm/%d", dompath,
vtpminfo->devid);
+    vtpminfo->backend = xs_read(ctx->xsh, XBT_NULL,
+                                libxl__sprintf(gc, "%s/backend",
vtpmpath), NULL);
+    if (!vtpminfo->backend) {
+        goto err;
+    }
+    if(!libxl__xs_read(gc, XBT_NULL, vtpminfo->backend)) {
+       goto err;
+    }
+
+    val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc,
"%s/backend-id", vtpmpath));
+    vtpminfo->backend_id = val ? strtoul(val, NULL, 10) : -1;
+    val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/state",
vtpmpath));
+    vtpminfo->state = val ? strtoul(val, NULL, 10) : -1;
+    val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc,
"%s/event-channel", vtpmpath));
+    vtpminfo->evtch = val ? strtoul(val, NULL, 10) : -1;
+    val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc,
"%s/ring-ref", vtpmpath));
+    vtpminfo->rref = val ? strtoul(val, NULL, 10) : -1;
+    vtpminfo->frontend = xs_read(ctx->xsh, XBT_NULL,
+                                 libxl__sprintf(gc, "%s/frontend",
vtpminfo->backend), NULL);
+    val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc,
"%s/frontend-id", vtpminfo->backend));
+    vtpminfo->frontend_id = val ? strtoul(val, NULL, 10) : -1;
+
+    val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/uuid",
vtpminfo->backend));
+    if(val == NULL) {
+       LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "%s/uuid does not exist!\n",
vtpminfo->backend);
+       goto err;
+    }
+    if(libxl_uuid_from_string(&(vtpminfo->uuid), val)) {
+       LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "%s/uuid is a malformed uuid??
(%s) Probably a bug!\n", vtpminfo->backend, val);
+       goto err;
+    }
+
+    goto exit;
+err:
+    rc = ERROR_FAIL;
+exit:
+    GC_FREE;
+    return rc;
+}
+
+int libxl_devid_to_device_vtpm(libxl_ctx *ctx, uint32_t domid,
+                              int devid, libxl_device_vtpm *vtpm)
+{
+    libxl_device_vtpm *vtpms;
+    int nb, i;
+    int rc;
+
+    vtpms = libxl_device_vtpm_list(ctx, domid, &nb);
+    if (!vtpms)
+        return ERROR_FAIL;
+
+    memset(vtpm, 0, sizeof (libxl_device_vtpm));
+    rc = 1;
+    for (i = 0; i < nb; ++i) {
+        if(devid == vtpms[i].devid) {
+            vtpm->backend_domid = vtpms[i].backend_domid;
+            vtpm->devid = vtpms[i].devid;
+            libxl_uuid_copy(&vtpm->uuid, &vtpms[i].uuid);
+            rc = 0;
+            break;
+        }
+    }
+
+    for (i=0; i<nb; i++)
+        libxl_device_vtpm_dispose(&vtpms[i]);
+    free(vtpms);
+    return rc;
+}
+
+
+/******************************************************************************/
 
 int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk)
 {
@@ -3174,6 +3414,10 @@ DEFINE_DEVICE_REMOVE(vkb, destroy, 1)
 DEFINE_DEVICE_REMOVE(vfb, remove, 0)
 DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
 
+/* vtpm */
+DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
+DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
+
 #undef DEFINE_DEVICE_REMOVE
 
 /******************************************************************************/
@@ -3208,6 +3452,9 @@ DEFINE_DEVICE_ADD(disk)
 /* nic */
 DEFINE_DEVICE_ADD(nic)
 
+/* vtpm */
+DEFINE_DEVICE_ADD(vtpm)
+
 #undef DEFINE_DEVICE_ADD
 
 /******************************************************************************/
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 7a7c419..3cb9ff8 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -478,13 +478,14 @@ typedef struct {
     libxl_domain_create_info c_info;
     libxl_domain_build_info b_info;
 
-    int num_disks, num_nics, num_pcidevs, num_vfbs, num_vkbs;
+    int num_disks, num_nics, num_pcidevs, num_vfbs, num_vkbs, num_vtpms;
 
     libxl_device_disk *disks;
     libxl_device_nic *nics;
     libxl_device_pci *pcidevs;
     libxl_device_vfb *vfbs;
     libxl_device_vkb *vkbs;
+    libxl_device_vtpm *vtpms;
 
     libxl_action_on_shutdown on_poweroff;
     libxl_action_on_shutdown on_reboot;
@@ -745,6 +746,23 @@ libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx,
uint32_t domid, int *num
 int libxl_device_nic_getinfo(libxl_ctx *ctx, uint32_t domid,
                               libxl_device_nic *nic, libxl_nicinfo *nicinfo);
 
+/* Virtual TPMs */
+int libxl_device_vtpm_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vtpm
*vtpm,
+                          const libxl_asyncop_how *ao_how)
+                          LIBXL_EXTERNAL_CALLERS_ONLY;
+int libxl_device_vtpm_remove(libxl_ctx *ctx, uint32_t domid,
+                            libxl_device_vtpm *vtpm,
+                            const libxl_asyncop_how *ao_how)
+                            LIBXL_EXTERNAL_CALLERS_ONLY;
+int libxl_device_vtpm_destroy(libxl_ctx *ctx, uint32_t domid,
+                              libxl_device_vtpm *vtpm,
+                              const libxl_asyncop_how *ao_how)
+                              LIBXL_EXTERNAL_CALLERS_ONLY;
+
+libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t domid, int
*num);
+int libxl_device_vtpm_getinfo(libxl_ctx *ctx, uint32_t domid,
+                               libxl_device_vtpm *vtpm, libxl_vtpminfo
*vtpminfo);
+
 /* Keyboard */
 int libxl_device_vkb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vkb *vkb,
                          const libxl_asyncop_how *ao_how)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 6cb586b..0eb6a9e 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -55,6 +55,10 @@ void libxl_domain_config_dispose(libxl_domain_config
*d_config)
         libxl_device_vkb_dispose(&d_config->vkbs[i]);
     free(d_config->vkbs);
 
+    for (i=0; i<d_config->num_vtpms; i++)
+       libxl_device_vtpm_dispose(&d_config->vtpms[i]);
+    free(d_config->vtpms);
+
     libxl_domain_create_info_dispose(&d_config->c_info);
     libxl_domain_build_info_dispose(&d_config->b_info);
 }
@@ -601,6 +605,8 @@ static void domcreate_bootloader_done(libxl__egc *egc,
 static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *aodevs,
                                 int ret);
 
+static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev *multidev,
+                                   int ret);
 static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *aodevs,
                                  int ret);
 
@@ -1084,13 +1090,13 @@ static void domcreate_devmodel_started(libxl__egc *egc,
     if (d_config->num_nics > 0) {
         /* Attach nics */
         libxl__multidev_begin(ao, &dcs->multidev);
-        dcs->multidev.callback = domcreate_attach_pci;
+        dcs->multidev.callback = domcreate_attach_vtpms;
         libxl__add_nics(egc, ao, domid, d_config, &dcs->multidev);
         libxl__multidev_prepared(egc, &dcs->multidev, 0);
         return;
     }
 
-    domcreate_attach_pci(egc, &dcs->multidev, 0);
+    domcreate_attach_vtpms(egc, &dcs->multidev, 0);
     return;
 
 error_out:
@@ -1098,6 +1104,36 @@ error_out:
     domcreate_complete(egc, dcs, ret);
 }
 
+static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev *multidev,
int ret) {
+   libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
+   STATE_AO_GC(dcs->ao);
+   int domid = dcs->guest_domid;
+
+   libxl_domain_config* const d_config = dcs->guest_config;
+
+   if(ret) {
+      LOG(ERROR, "unable to add nic devices");
+      goto error_out;
+   }
+
+    /* Plug vtpm devices */
+    if (d_config->num_vtpms > 0) {
+        /* Attach vtpms */
+        libxl__multidev_begin(ao, &dcs->multidev);
+        dcs->multidev.callback = domcreate_attach_pci;
+        libxl__add_vtpms(egc, ao, domid, d_config, &dcs->multidev);
+        libxl__multidev_prepared(egc, &dcs->multidev, 0);
+        return;
+    }
+
+   domcreate_attach_pci(egc, multidev, 0);
+   return;
+
+error_out:
+   assert(ret);
+   domcreate_complete(egc, dcs, ret);
+}
+
 static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *multidev,
                                  int ret)
 {
@@ -1111,7 +1147,7 @@ static void domcreate_attach_pci(libxl__egc *egc,
libxl__multidev *multidev,
     libxl_domain_config *const d_config = dcs->guest_config;
 
     if (ret) {
-        LOG(ERROR, "unable to add nic devices");
+        LOG(ERROR, "unable to add vtpm devices");
         goto error_out;
     }
 
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index c3283f1..47a29f0 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -515,6 +515,7 @@ void libxl__multidev_prepared(libxl__egc *egc,
 
 DEFINE_DEVICES_ADD(disk)
 DEFINE_DEVICES_ADD(nic)
+DEFINE_DEVICES_ADD(vtpm)
 
 #undef DEFINE_DEVICES_ADD
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b6f54ba..e9a7cbb 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -954,6 +954,7 @@ _hidden int libxl__device_disk_setdefault(libxl__gc *gc,
                                           libxl_device_disk *disk);
 _hidden int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic,
                                          uint32_t domid);
+_hidden int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm
*vtpm);
 _hidden int libxl__device_vfb_setdefault(libxl__gc *gc, libxl_device_vfb *vfb);
 _hidden int libxl__device_vkb_setdefault(libxl__gc *gc, libxl_device_vkb *vkb);
 _hidden int libxl__device_pci_setdefault(libxl__gc *gc, libxl_device_pci *pci);
@@ -1975,6 +1976,10 @@ _hidden void libxl__device_nic_add(libxl__egc *egc,
uint32_t domid,
                                    libxl_device_nic *nic,
                                    libxl__ao_device *aodev);
 
+_hidden void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
+                                   libxl_device_vtpm *vtpm,
+                                   libxl__ao_device *aodev);
+
 /* Internal function to connect a vkb device */
 _hidden int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
                                   libxl_device_vkb *vkb);
@@ -2407,6 +2412,10 @@ _hidden void libxl__add_nics(libxl__egc *egc, libxl__ao
*ao, uint32_t domid,
                              libxl_domain_config *d_config,
                              libxl__multidev *multidev);
 
+_hidden void libxl__add_vtpms(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
+                             libxl_domain_config *d_config,
+                             libxl__multidev *multidev);
+
 /*----- device model creation -----*/
 
 /* First layer; wraps libxl__spawn_spawn. */
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index de111a6..7eac4a8 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -395,6 +395,12 @@ libxl_device_pci = Struct("device_pci", [
     ("permissive", bool),
     ])
 
+libxl_device_vtpm = Struct("device_vtpm", [
+    ("backend_domid",    libxl_domid),
+    ("devid",            libxl_devid),
+    ("uuid",             libxl_uuid),
+])
+
 libxl_diskinfo = Struct("diskinfo", [
     ("backend", string),
     ("backend_id", uint32),
@@ -418,6 +424,18 @@ libxl_nicinfo = Struct("nicinfo", [
     ("rref_rx", integer),
     ], dir=DIR_OUT)
 
+libxl_vtpminfo = Struct("vtpminfo", [
+    ("backend", string),
+    ("backend_id", uint32),
+    ("frontend", string),
+    ("frontend_id", uint32),
+    ("devid", libxl_devid),
+    ("state", integer),
+    ("evtch", integer),
+    ("rref", integer),
+    ("uuid", libxl_uuid),
+    ], dir=DIR_OUT)
+
 libxl_vcpuinfo = Struct("vcpuinfo", [
     ("vcpuid", uint32),
     ("cpu", uint32),
diff --git a/tools/libxl/libxl_types_internal.idl
b/tools/libxl/libxl_types_internal.idl
index 5ac8c9c..c40120e 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -19,6 +19,7 @@ libxl__device_kind = Enumeration("device_kind", [
     (5, "VFB"),
     (6, "VKBD"),
     (7, "CONSOLE"),
+    (8, "VTPM"),
     ])
 
 libxl__console_backend = Enumeration("console_backend", [
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 55cd299..73a158a 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -463,6 +463,35 @@ int libxl_pipe(libxl_ctx *ctx, int pipes[2])
     return 0;
 }
 
+int libxl_uuid_to_device_vtpm(libxl_ctx *ctx, uint32_t domid,
+                            libxl_uuid* uuid, libxl_device_vtpm *vtpm)
+{
+    libxl_device_vtpm *vtpms;
+    int nb, i;
+    int rc;
+
+    vtpms = libxl_device_vtpm_list(ctx, domid, &nb);
+    if (!vtpms)
+        return ERROR_FAIL;
+
+    memset(vtpm, 0, sizeof (libxl_device_vtpm));
+    rc = 1;
+    for (i = 0; i < nb; ++i) {
+        if(!libxl_uuid_compare(uuid, &vtpms[i].uuid)) {
+            vtpm->backend_domid = vtpms[i].backend_domid;
+            vtpm->devid = vtpms[i].devid;
+            libxl_uuid_copy(&vtpm->uuid, &vtpms[i].uuid);
+            rc = 0;
+            break;
+        }
+    }
+
+    for (i=0; i<nb; i++)
+        libxl_device_vtpm_dispose(&vtpms[i]);
+    free(vtpms);
+    return rc;
+}
+
 int libxl_mac_to_device_nic(libxl_ctx *ctx, uint32_t domid,
                             const char *mac, libxl_device_nic *nic)
 {
diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
index 83aaac7..40f3f30 100644
--- a/tools/libxl/libxl_utils.h
+++ b/tools/libxl/libxl_utils.h
@@ -64,6 +64,11 @@ int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
int devid,
 int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t domid, const char *vdev,
                                libxl_device_disk *disk);
 
+int libxl_uuid_to_device_vtpm(libxl_ctx *ctx, uint32_t domid,
+                               libxl_uuid *uuid, libxl_device_vtpm *vtpm);
+int libxl_devid_to_device_vtpm(libxl_ctx *ctx, uint32_t domid,
+                               int devid, libxl_device_vtpm *vtpm);
+
 int libxl_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *bitmap, int n_bits);
     /* Allocated bimap is from malloc, libxl_bitmap_dispose() to be
      * called by the application when done. */
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 0b2f848..be6f38b 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -79,6 +79,9 @@ int main_networkdetach(int argc, char **argv);
 int main_blockattach(int argc, char **argv);
 int main_blocklist(int argc, char **argv);
 int main_blockdetach(int argc, char **argv);
+int main_vtpmattach(int argc, char **argv);
+int main_vtpmlist(int argc, char **argv);
+int main_vtpmdetach(int argc, char **argv);
 int main_uptime(int argc, char **argv);
 int main_tmem_list(int argc, char **argv);
 int main_tmem_freeze(int argc, char **argv);
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index fe8e925..3f02f6d 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -573,7 +573,7 @@ static void parse_config_data(const char *config_source,
     const char *buf;
     long l;
     XLU_Config *config;
-    XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids;
+    XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms;
     XLU_ConfigList *ioports, *irqs, *iomem;
     int num_ioports, num_irqs, num_iomem;
     int pci_power_mgmt = 0;
@@ -1045,6 +1045,47 @@ static void parse_config_data(const char *config_source,
         }
     }
 
+    if (!xlu_cfg_get_list(config, "vtpm", &vtpms, 0, 0)) {
+        d_config->num_vtpms = 0;
+        d_config->vtpms = NULL;
+        while ((buf = xlu_cfg_get_listitem (vtpms, d_config->num_vtpms)) !=
NULL) {
+            libxl_device_vtpm *vtpm;
+            char * buf2 = strdup(buf);
+            char *p, *p2;
+
+            d_config->vtpms = (libxl_device_vtpm *)
realloc(d_config->vtpms, sizeof(libxl_device_vtpm) *
(d_config->num_vtpms+1));
+            vtpm = d_config->vtpms + d_config->num_vtpms;
+            libxl_device_vtpm_init(vtpm);
+            vtpm->devid = d_config->num_vtpms;
+
+            p = strtok(buf2, ",");
+            if (!p)
+                goto skip_vtpm;
+            do {
+                while(*p == '' '')
+                    ++p;
+                if ((p2 = strchr(p, ''='')) == NULL)
+                    break;
+                *p2 = ''\0'';
+                if (!strcmp(p, "backend")) {
+                    if(domain_qualifier_to_domid(p2 + 1,
&(vtpm->backend_domid), 0))
+                    {
+                        fprintf(stderr, "Specified backend domain does not
exist, defaulting to Dom0\n");
+                        vtpm->backend_domid = 0;
+                    }
+                } else if(!strcmp(p, "uuid")) {
+                    if( libxl_uuid_from_string(&vtpm->uuid, p2 + 1) ) {
+                        fprintf(stderr, "Failed to parse vtpm UUID:
%s\n", p2 + 1);
+                        exit(1);
+                    }
+                }
+            } while ((p = strtok(NULL, ",")) != NULL);
+skip_vtpm:
+            free(buf2);
+            d_config->num_vtpms++;
+        }
+    }
+
     if (!xlu_cfg_get_list (config, "vif", &nics, 0, 0)) {
         d_config->num_nics = 0;
         d_config->nics = NULL;
@@ -1070,7 +1111,7 @@ static void parse_config_data(const char *config_source,
 
             p = strtok(buf2, ",");
             if (!p)
-                goto skip;
+                goto skip_nic;
             do {
                 while (*p == '' '')
                     p++;
@@ -1134,7 +1175,7 @@ static void parse_config_data(const char *config_source,
                     fprintf(stderr, "the accel parameter for vifs is
currently not supported\n");
                 }
             } while ((p = strtok(NULL, ",")) != NULL);
-skip:
+skip_nic:
             free(buf2);
             d_config->num_nics++;
         }
@@ -5570,6 +5611,131 @@ int main_blockdetach(int argc, char **argv)
     return rc;
 }
 
+int main_vtpmattach(int argc, char **argv)
+{
+    int opt;
+    libxl_device_vtpm vtpm;
+    char *oparg;
+    unsigned int val;
+    uint32_t domid;
+
+    if ((opt = def_getopt(argc, argv, "", "vtpm-attach",
1)) != -1)
+        return opt;
+
+    if (domain_qualifier_to_domid(argv[optind], &domid, 0) < 0) {
+        fprintf(stderr, "%s is an invalid domain identifier\n",
argv[optind]);
+        return 1;
+    }
+    ++optind;
+
+    libxl_device_vtpm_init(&vtpm);
+    for (argv += optind, argc -= optind; argc > 0; ++argv, --argc) {
+        if (MATCH_OPTION("uuid", *argv, oparg)) {
+            if(libxl_uuid_from_string(&(vtpm.uuid), oparg)) {
+                fprintf(stderr, "Invalid uuid specified (%s)\n",
oparg);
+                return 1;
+            }
+        } else if (MATCH_OPTION("backend", *argv, oparg)) {
+            if(domain_qualifier_to_domid(oparg, &val, 0)) {
+                fprintf(stderr, "Specified backend domain does not exist,
defaulting to Dom0\n");
+                val = 0;
+            }
+            vtpm.backend_domid = val;
+        } else {
+            fprintf(stderr, "unrecognized argument `%s''\n",
*argv);
+            return 1;
+        }
+    }
+
+    if(dryrun_only) {
+       char* json = libxl_device_vtpm_to_json(ctx, &vtpm);
+       printf("vtpm: %s\n", json);
+       free(json);
+       libxl_device_vtpm_dispose(&vtpm);
+       if (ferror(stdout) || fflush(stdout)) { perror("stdout");
exit(-1); }
+       return 0;
+    }
+
+    if (libxl_device_vtpm_add(ctx, domid, &vtpm, 0)) {
+        fprintf(stderr, "libxl_device_vtpm_add failed.\n");
+        return 1;
+    }
+    libxl_device_vtpm_dispose(&vtpm);
+    return 0;
+}
+
+int main_vtpmlist(int argc, char **argv)
+{
+    int opt;
+    libxl_device_vtpm *vtpms;
+    libxl_vtpminfo vtpminfo;
+    int nb, i;
+
+    if ((opt = def_getopt(argc, argv, "", "vtpm-list", 1))
!= -1)
+        return opt;
+
+    /*      Idx  BE   UUID   Hdl  Sta  evch rref  BE-path */
+    printf("%-3s %-2s %-36s %-6s %-5s %-6s %-5s %-10s\n",
+           "Idx", "BE", "Uuid",
"handle", "state", "evt-ch", "ring-ref",
"BE-path");
+    for (argv += optind, argc -= optind; argc > 0; --argc, ++argv) {
+        uint32_t domid;
+        if (domain_qualifier_to_domid(*argv, &domid, 0) < 0) {
+            fprintf(stderr, "%s is an invalid domain identifier\n",
*argv);
+            continue;
+        }
+        if (!(vtpms = libxl_device_vtpm_list(ctx, domid, &nb))) {
+            continue;
+        }
+        for (i = 0; i < nb; ++i) {
+           if(!libxl_device_vtpm_getinfo(ctx, domid, &vtpms[i],
&vtpminfo)) {
+              /*      Idx  BE     UUID             Hdl Sta evch rref BE-path*/
+              printf("%-3d %-2d " LIBXL_UUID_FMT " %6d %5d %6d
%8d %-30s\n",
+                    vtpminfo.devid, vtpminfo.backend_id,
+                    LIBXL_UUID_BYTES(vtpminfo.uuid),
+                    vtpminfo.devid, vtpminfo.state, vtpminfo.evtch,
+                    vtpminfo.rref, vtpminfo.backend);
+
+              libxl_vtpminfo_dispose(&vtpminfo);
+           }
+           libxl_device_vtpm_dispose(&vtpms[i]);
+        }
+        free(vtpms);
+    }
+    return 0;
+}
+
+int main_vtpmdetach(int argc, char **argv)
+{
+    uint32_t domid;
+    int opt, rc=0;
+    libxl_device_vtpm vtpm;
+    libxl_uuid uuid;
+
+    if ((opt = def_getopt(argc, argv, "", "vtpm-detach",
2)) != -1)
+        return opt;
+
+    domid = find_domain(argv[optind]);
+
+    if ( libxl_uuid_from_string(&uuid, argv[optind+1])) {
+        if (libxl_devid_to_device_vtpm(ctx, domid, atoi(argv[optind+1]),
&vtpm)) {
+            fprintf(stderr, "Unknown device %s.\n", argv[optind+1]);
+            return 1;
+        }
+    } else {
+        if (libxl_uuid_to_device_vtpm(ctx, domid, &uuid, &vtpm)) {
+            fprintf(stderr, "Unknown device %s.\n", argv[optind+1]);
+            return 1;
+        }
+    }
+    rc = libxl_device_vtpm_remove(ctx, domid, &vtpm, 0);
+    if (rc) {
+        fprintf(stderr, "libxl_device_vtpm_remove failed.\n");
+    }
+    libxl_device_vtpm_dispose(&vtpm);
+    return rc;
+}
+
+
 static char *uptime_to_string(unsigned long uptime, int short_mode)
 {
     int sec, min, hour, day;
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 85ea768..7c018eb 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -338,6 +338,21 @@ struct cmd_spec cmd_table[] = {
       "Destroy a domain''s virtual block device",
       "<Domain> <DevId>",
     },
+    { "vtpm-attach",
+      &main_vtpmattach, 0, 1,
+      "Create a new virtual TPM device",
+      "<Domain> [uuid=<uuid>]
[backend=<BackDomain>]",
+    },
+    { "vtpm-list",
+      &main_vtpmlist, 0, 0,
+      "List virtual TPM devices for a domain",
+      "<Domain(s)>",
+    },
+    { "vtpm-detach",
+      &main_vtpmdetach, 0, 1,
+      "Destroy a domain''s virtual TPM device",
+      "<Domain> <DevId|uuid>",
+    },
     { "uptime",
       &main_uptime, 0, 0,
       "Print uptime for all/some domains",
-- 
1.7.9.5

On Thu, Sep 27, 2012 at 6:10 PM, Matthew Fioravante
<matthew.fioravante@jhuapl.edu> wrote:
> Previously device ids in libxl were treated as integers meaning they
> were being initialized to 0, which is a valid device id. This patch
> makes devid its own type in libxl and initializes it to -1, an invalid
> value.
>
> This fixes a bug where if you try to do a xl DEV-attach multiple
> time it will continuously try to reattach device 0 instead of
> generating a new device id.
>
> Signed-off-by: Matthew Fioravante <matthew.fioravante@jhuapl.edu>
Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>

Should this be backported to 4.2-testing?

 -George
>
> diff --git a/tools/libxl/gentest.py b/tools/libxl/gentest.py
> index 2915f71..84b4fd7 100644
> --- a/tools/libxl/gentest.py
> +++ b/tools/libxl/gentest.py
> @@ -60,7 +60,7 @@ def gen_rand_init(ty, v, indent = "    ",
parent = None):
>                                          passby=idl.PASS_BY_REFERENCE))
>      elif ty.typename in ["libxl_uuid", "libxl_mac",
"libxl_hwcap"]:
>          s += "rand_bytes((uint8_t *)%s, sizeof(*%s));\n" % (v,v)
> -    elif ty.typename in ["libxl_domid"] or isinstance(ty,
idl.Number):
> +    elif ty.typename in ["libxl_domid", "libxl_devid"]
or isinstance(ty, idl.Number):
>          s += "%s = rand() %% (sizeof(%s)*8);\n" % \
>               (ty.pass_arg(v, parent is None),
>                ty.pass_arg(v, parent is None))
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 599c7f1..7a7c419 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -307,6 +307,7 @@ void libxl_cpuid_dispose(libxl_cpuid_policy_list
*cpuid_list);
>  #define LIBXL_PCI_FUNC_ALL (~0U)
>
>  typedef uint32_t libxl_domid;
> +typedef int libxl_devid;
>
>  /*
>   * Formatting Enumerations.
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index cf83c60..de111a6 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -8,6 +8,7 @@ namespace("libxl_")
>  libxl_defbool = Builtin("defbool", passby=PASS_BY_REFERENCE)
>
>  libxl_domid = Builtin("domid", json_fn =
"yajl_gen_integer", autogenerate_json = False)
> +libxl_devid = Builtin("devid", json_fn =
"yajl_gen_integer", autogenerate_json = False, signed = True,
init_val="-1")
>  libxl_uuid = Builtin("uuid", passby=PASS_BY_REFERENCE)
>  libxl_mac = Builtin("mac", passby=PASS_BY_REFERENCE)
>  libxl_bitmap = Builtin("bitmap",
dispose_fn="libxl_bitmap_dispose", passby=PASS_BY_REFERENCE)
> @@ -343,7 +344,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
>
>  libxl_device_vfb = Struct("device_vfb", [
>      ("backend_domid", libxl_domid),
> -    ("devid",         integer),
> +    ("devid",         libxl_devid),
>      ("vnc",           libxl_vnc_info),
>      ("sdl",           libxl_sdl_info),
>      # set keyboard layout, default is en-us keyboard
> @@ -352,7 +353,7 @@ libxl_device_vfb = Struct("device_vfb", [
>
>  libxl_device_vkb = Struct("device_vkb", [
>      ("backend_domid", libxl_domid),
> -    ("devid", integer),
> +    ("devid", libxl_devid),
>      ])
>
>  libxl_device_disk = Struct("device_disk", [
> @@ -369,7 +370,7 @@ libxl_device_disk = Struct("device_disk", [
>
>  libxl_device_nic = Struct("device_nic", [
>      ("backend_domid", libxl_domid),
> -    ("devid", integer),
> +    ("devid", libxl_devid),
>      ("mtu", integer),
>      ("model", string),
>      ("mac", libxl_mac),
> @@ -399,7 +400,7 @@ libxl_diskinfo = Struct("diskinfo", [
>      ("backend_id", uint32),
>      ("frontend", string),
>      ("frontend_id", uint32),
> -    ("devid", integer),
> +    ("devid", libxl_devid),
>      ("state", integer),
>      ("evtch", integer),
>      ("rref", integer),
> @@ -410,7 +411,7 @@ libxl_nicinfo = Struct("nicinfo", [
>      ("backend_id", uint32),
>      ("frontend", string),
>      ("frontend_id", uint32),
> -    ("devid", integer),
> +    ("devid", libxl_devid),
>      ("state", integer),
>      ("evtch", integer),
>      ("rref_tx", integer),
> diff --git a/tools/ocaml/libs/xl/genwrap.py
b/tools/ocaml/libs/xl/genwrap.py
> index 42f374e..97d088d 100644
> --- a/tools/ocaml/libs/xl/genwrap.py
> +++ b/tools/ocaml/libs/xl/genwrap.py
> @@ -10,6 +10,7 @@ builtins = {
>      "int":                  ("int",                   
"%(c)s = Int_val(%(o)s)",            "Val_int(%(c)s)"  ),
>      "char *":               ("string",                
"%(c)s = dup_String_val(gc, %(o)s)",
"caml_copy_string(%(c)s)"),
>      "libxl_domid":          ("domid",                 
"%(c)s = Int_val(%(o)s)",            "Val_int(%(c)s)"  ),
> +    "libxl_devid":          ("devid",                 
"%(c)s = Int_val(%(o)s)",            "Val_int(%(c)s)"  ),
>      "libxl_defbool":        ("bool option",           
"%(c)s = Defbool_val(%(o)s)",        "Val_defbool(%(c)s)" ),
>      "libxl_uuid":           ("int array",             
"Uuid_val(gc, lg, &%(c)s, %(o)s)",  
"Val_uuid(&%(c)s)"),
>      "libxl_key_value_list": ("(string * string) list",
None,                                None),
> @@ -41,8 +42,8 @@ def stub_fn_name(ty, name):
>      return "stub_xl_%s_%s" % (ty.rawname,name)
>
>  def ocaml_type_of(ty):
> -    if ty.rawname == "domid":
> -        return "domid"
> +    if ty.rawname in ["domid","devid"]:
> +        return ty.rawname
>      elif isinstance(ty,idl.UInt):
>          if ty.width in [8, 16]:
>              # handle as ints
> diff --git a/tools/ocaml/libs/xl/xenlight.ml.in
b/tools/ocaml/libs/xl/xenlight.ml.in
> index c47623c..dcc1a38 100644
> --- a/tools/ocaml/libs/xl/xenlight.ml.in
> +++ b/tools/ocaml/libs/xl/xenlight.ml.in
> @@ -16,6 +16,7 @@
>  exception Error of string
>
>  type domid = int
> +type devid = int
>
>  (* @@LIBXL_TYPES@@ *)
>
> diff --git a/tools/ocaml/libs/xl/xenlight.mli.in
b/tools/ocaml/libs/xl/xenlight.mli.in
> index 4717bac..3fd0165 100644
> --- a/tools/ocaml/libs/xl/xenlight.mli.in
> +++ b/tools/ocaml/libs/xl/xenlight.mli.in
> @@ -16,6 +16,7 @@
>  exception Error of string
>
>  type domid = int
> +type devid = int
>
>  (* @@LIBXL_TYPES@@ *)
>
> diff --git a/tools/python/xen/lowlevel/xl/xl.c
b/tools/python/xen/lowlevel/xl/xl.c
> index 0551c76..32f982a 100644
> --- a/tools/python/xen/lowlevel/xl/xl.c
> +++ b/tools/python/xen/lowlevel/xl/xl.c
> @@ -281,6 +281,11 @@ int attrib__libxl_domid_set(PyObject *v, libxl_domid
*domid) {
>      return 0;
>  }
>
> +int attrib__libxl_devid_set(PyObject *v, libxl_devid *devid) {
> +   *devid = PyInt_AsLong(v);
> +   return 0;
> +}
> +
>  int attrib__struct_in_addr_set(PyObject *v, struct in_addr *pptr)
>  {
>      PyErr_SetString(PyExc_NotImplementedError, "Setting
in_addr");
> @@ -342,6 +347,10 @@ PyObject *attrib__libxl_domid_get(libxl_domid *domid)
{
>      return PyInt_FromLong(*domid);
>  }
>
> +PyObject *attrib__libxl_devid_get(libxl_devid *devid) {
> +    return PyInt_FromLong(*devid);
> +}
> +
>  PyObject *attrib__struct_in_addr_get(struct in_addr *pptr)
>  {
>      PyErr_SetString(PyExc_NotImplementedError, "Getting
in_addr");
> --
> 1.7.9.5
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Thu, Sep 27, 2012 at 6:11 PM, Matthew Fioravante
<matthew.fioravante@jhuapl.edu> wrote:
> This patch adds vtpm support to libxl. It adds vtpm parsing to config
> files and 3 new xl commands:
> vtpm-attach
> vtpm-detach
> vtpm-list
>
> Signed-off-by: Matthew Fioravante <matthew.fioravante@jhuapl.edu>
Overall looks good to me -- just a few comments below about the config
file handling (see below).

Thanks for all your work on this.
> @@ -601,6 +605,8 @@ static void domcreate_bootloader_done(libxl__egc *egc,
>  static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *aodevs,
>                                  int ret);
>
> +static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev
*multidev,
> +                                   int ret);
>  static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *aodevs,
>                                   int ret);
>
> @@ -1084,13 +1090,13 @@ static void domcreate_devmodel_started(libxl__egc
*egc,
>      if (d_config->num_nics > 0) {
>          /* Attach nics */
>          libxl__multidev_begin(ao, &dcs->multidev);
> -        dcs->multidev.callback = domcreate_attach_pci;
> +        dcs->multidev.callback = domcreate_attach_vtpms;
Wow -- what a weird convention you''ve had to try to figure out and
modify here.  Well done. :-)
>          libxl__add_nics(egc, ao, domid, d_config, &dcs->multidev);
>          libxl__multidev_prepared(egc, &dcs->multidev, 0);
>          return;
>      }
>
> -    domcreate_attach_pci(egc, &dcs->multidev, 0);
> +    domcreate_attach_vtpms(egc, &dcs->multidev, 0);
>      return;
>
>  error_out:
> @@ -1098,6 +1104,36 @@ error_out:
>      domcreate_complete(egc, dcs, ret);
>  }
>
> +static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev
*multidev, int ret) {
> +   libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs,
multidev);
> +   STATE_AO_GC(dcs->ao);
> +   int domid = dcs->guest_domid;
> +
> +   libxl_domain_config* const d_config = dcs->guest_config;
> +
> +   if(ret) {
> +      LOG(ERROR, "unable to add nic devices");
> +      goto error_out;
> +   }
> +
> +    /* Plug vtpm devices */
> +    if (d_config->num_vtpms > 0) {
> +        /* Attach vtpms */
> +        libxl__multidev_begin(ao, &dcs->multidev);
> +        dcs->multidev.callback = domcreate_attach_pci;
> +        libxl__add_vtpms(egc, ao, domid, d_config, &dcs->multidev);
> +        libxl__multidev_prepared(egc, &dcs->multidev, 0);
> +        return;
> +    }
> +
> +   domcreate_attach_pci(egc, multidev, 0);
> +   return;
> +
> +error_out:
> +   assert(ret);
> +   domcreate_complete(egc, dcs, ret);
> +}
> +
>  static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev
*multidev,
>                                   int ret)
>  {
> @@ -1111,7 +1147,7 @@ static void domcreate_attach_pci(libxl__egc *egc,
libxl__multidev *multidev,
>      libxl_domain_config *const d_config = dcs->guest_config;
>
>      if (ret) {
> -        LOG(ERROR, "unable to add nic devices");
> +        LOG(ERROR, "unable to add vtpm devices");
>          goto error_out;
>      }
>
[snip]
> @@ -1045,6 +1045,47 @@ static void parse_config_data(const char
*config_source,
>          }
>      }
>
> +    if (!xlu_cfg_get_list(config, "vtpm", &vtpms, 0, 0)) {
> +        d_config->num_vtpms = 0;
> +        d_config->vtpms = NULL;
> +        while ((buf = xlu_cfg_get_listitem (vtpms,
d_config->num_vtpms)) != NULL) {
> +            libxl_device_vtpm *vtpm;
> +            char * buf2 = strdup(buf);
> +            char *p, *p2;
> +
> +            d_config->vtpms = (libxl_device_vtpm *)
realloc(d_config->vtpms, sizeof(libxl_device_vtpm) *
(d_config->num_vtpms+1));
> +            vtpm = d_config->vtpms + d_config->num_vtpms;
> +            libxl_device_vtpm_init(vtpm);
> +            vtpm->devid = d_config->num_vtpms;
> +
> +            p = strtok(buf2, ",");
> +            if (!p)
> +                goto skip_vtpm;
Is the purpose of this so that you can have "empty" vtpm slots?
(Since even when skipping, you still increment the num_vtpms counter?)
> +            do {
> +                while(*p == '' '')
> +                    ++p;
> +                if ((p2 = strchr(p, ''='')) == NULL)
> +                    break;
> +                *p2 = ''\0'';
> +                if (!strcmp(p, "backend")) {
> +                    if(domain_qualifier_to_domid(p2 + 1,
&(vtpm->backend_domid), 0))
> +                    {
> +                        fprintf(stderr, "Specified backend domain
does not exist, defaulting to Dom0\n");
> +                        vtpm->backend_domid = 0;
So wait; if someone specifies a domain, and that turns out not to
work, you just change it to 0?  It seems like if the *specified*
domain doesn''t exist, the command should fail instead of choosing
something at random.
> +                    }
> +                } else if(!strcmp(p, "uuid")) {
> +                    if( libxl_uuid_from_string(&vtpm->uuid, p2 + 1)
) {
> +                        fprintf(stderr, "Failed to parse vtpm UUID:
%s\n", p2 + 1);
> +                        exit(1);
> +                    }
> +                }
If I''m parsing this right, it looks like you will just silently ignore
other arguments -- it seems like throwing an error would be better;
especially to catch things like typos.  Otherwise, if someone does
something like "uid=[whatever]", it will act like uuid isn''t
there and
create a new one, instead of alerting the user to the fact that he''d
made a typo in the config file.

 -George

On Thu, Sep 27, 2012 at 6:10 PM, Matthew Fioravante
<matthew.fioravante@jhuapl.edu> wrote:
> This patch adds a new option for xen config files for
> directly mapping hardware io memory into a vm.
>
> Signed-off-by: Matthew Fioravante <matthew.fioravante@jhuapl.edu>
Looks good to me.

Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
>
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 013270d..428da21 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -496,6 +496,17 @@ is given in hexadecimal and may either a span e.g.
C<2f8-2ff>
>  It is recommended to use this option only for trusted VMs under
>  administrator control.
>
> +=item B<iomem=[ "IOMEM_START,NUM_PAGES",
"IOMEM_START,NUM_PAGES", ... ]>
> +
> +Allow guest to access specific hardware I/O memory pages.
B<IOMEM_START>
> +is a physical page number. B<NUM_PAGES> is the number
> +of pages beginning with B<START_PAGE> to allow access. Both values
> +must be given in hexadecimal.
> +
> +It is recommended to use this option only for trusted VMs under
> +administrator control.
> +
> +
>  =item B<irqs=[ NUMBER, NUMBER, ... ]>
>
>  Allow a guest to access specific physical IRQs.
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index ef17f05..6cb586b 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -963,6 +963,24 @@ static void domcreate_launch_dm(libxl__egc *egc,
libxl__multidev *multidev,
>          }
>      }
>
> +    for (i = 0; i < d_config->b_info.num_iomem; i++) {
> +        libxl_iomem_range *io = &d_config->b_info.iomem[i];
> +
> +        LOG(DEBUG, "dom%d iomem %"PRIx64"-%"PRIx64,
> +            domid, io->start, io->start + io->number - 1);
> +
> +        ret = xc_domain_iomem_permission(CTX->xch, domid,
> +                                          io->start, io->number, 1);
> +        if ( ret<0 ) {
> +            LOGE(ERROR,
> +                 "failed give dom%d access to iomem range
%"PRIx64"-%"PRIx64,
> +                 domid, io->start, io->start + io->number - 1);
> +            ret = ERROR_FAIL;
> +        }
> +    }
> +
> +
> +
>      for (i = 0; i < d_config->num_nics; i++) {
>          /* We have to init the nic here, because we still haven''t
>           * called libxl_device_nic_add at this point, but qemu needs
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 6d5c578..cf83c60 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -140,6 +140,11 @@ libxl_ioport_range = Struct("ioport_range",
[
>      ("number", uint32),
>      ])
>
> +libxl_iomem_range = Struct("iomem_range", [
> +    ("start", uint64),
> +    ("number", uint64),
> +    ])
> +
>  libxl_vga_interface_info = Struct("vga_interface_info", [
>      ("kind",    libxl_vga_interface_type),
>      ])
> @@ -284,6 +289,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
>
>      ("ioports",          Array(libxl_ioport_range,
"num_ioports")),
>      ("irqs",             Array(uint32, "num_irqs")),
> +    ("iomem",            Array(libxl_iomem_range,
"num_iomem")),
>
>      ("u", KeyedUnion(None, libxl_domain_type, "type",
>                  [("hvm", Struct(None, [("firmware",   
string),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 1627cac..fe8e925 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -574,8 +574,8 @@ static void parse_config_data(const char
*config_source,
>      long l;
>      XLU_Config *config;
>      XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids;
> -    XLU_ConfigList *ioports, *irqs;
> -    int num_ioports, num_irqs;
> +    XLU_ConfigList *ioports, *irqs, *iomem;
> +    int num_ioports, num_irqs, num_iomem;
>      int pci_power_mgmt = 0;
>      int pci_msitranslate = 0;
>      int pci_permissive = 0;
> @@ -1005,6 +1005,30 @@ static void parse_config_data(const char
*config_source,
>          }
>      }
>
> +    if (!xlu_cfg_get_list(config, "iomem", &iomem,
&num_iomem, 0)) {
> +        b_info->num_iomem = num_iomem;
> +        b_info->iomem = calloc(num_iomem, sizeof(*b_info->iomem));
> +        if (b_info->iomem == NULL) {
> +            fprintf(stderr, "unable to allocate memory for
iomem\n");
> +            exit(-1);
> +        }
> +        for (i = 0; i < num_iomem; i++) {
> +            buf = xlu_cfg_get_listitem (iomem, i);
> +            if (!buf) {
> +                fprintf(stderr,
> +                        "xl: Unable to get element %d in iomem
list\n", i);
> +                exit(1);
> +            }
> +            if(sscanf(buf, "%" SCNx64",%" SCNx64,
&b_info->iomem[i].start, &b_info->iomem[i].number) != 2) {
> +               fprintf(stderr,
> +                       "xl: Invalid argument parsing iomem:
%s\n", buf);
> +               exit(1);
> +            }
> +        }
> +    }
> +
> +
> +
>      if (!xlu_cfg_get_list (config, "disk", &vbds, 0, 0)) {
>          d_config->num_disks = 0;
>          d_config->disks = NULL;
> --
> 1.7.9.5
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On 09/28/2012 11:03 AM, George Dunlap wrote:
> On Thu, Sep 27, 2012 at 6:11 PM, Matthew Fioravante
> <matthew.fioravante@jhuapl.edu> wrote:
>> This patch adds vtpm support to libxl. It adds vtpm parsing to config
>> files and 3 new xl commands:
>> vtpm-attach
>> vtpm-detach
>> vtpm-list
>>
>> Signed-off-by: Matthew Fioravante <matthew.fioravante@jhuapl.edu>
> Overall looks good to me -- just a few comments below about the config
> file handling (see below).
>
> Thanks for all your work on this.
>
>> @@ -601,6 +605,8 @@ static void domcreate_bootloader_done(libxl__egc
*egc,
>>  static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev
*aodevs,
>>                                  int ret);
>>
>> +static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev
*multidev,
>> +                                   int ret);
>>  static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev
*aodevs,
>>                                   int ret);
>>
>> @@ -1084,13 +1090,13 @@ static void
domcreate_devmodel_started(libxl__egc *egc,
>>      if (d_config->num_nics > 0) {
>>          /* Attach nics */
>>          libxl__multidev_begin(ao, &dcs->multidev);
>> -        dcs->multidev.callback = domcreate_attach_pci;
>> +        dcs->multidev.callback = domcreate_attach_vtpms;
> Wow -- what a weird convention you''ve had to try to figure out and
> modify here.  Well done. :-)
It was really tricky. Is there no better way to handle asynchronous
code? This method seems really error prone and almost impossible to
follow.
>
>>          libxl__add_nics(egc, ao, domid, d_config,
&dcs->multidev);
>>          libxl__multidev_prepared(egc, &dcs->multidev, 0);
>>          return;
>>      }
>>
>> -    domcreate_attach_pci(egc, &dcs->multidev, 0);
>> +    domcreate_attach_vtpms(egc, &dcs->multidev, 0);
>>      return;
>>
>>  error_out:
>> @@ -1098,6 +1104,36 @@ error_out:
>>      domcreate_complete(egc, dcs, ret);
>>  }
>>
>> +static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev
*multidev, int ret) {
>> +   libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs,
multidev);
>> +   STATE_AO_GC(dcs->ao);
>> +   int domid = dcs->guest_domid;
>> +
>> +   libxl_domain_config* const d_config = dcs->guest_config;
>> +
>> +   if(ret) {
>> +      LOG(ERROR, "unable to add nic devices");
>> +      goto error_out;
>> +   }
>> +
>> +    /* Plug vtpm devices */
>> +    if (d_config->num_vtpms > 0) {
>> +        /* Attach vtpms */
>> +        libxl__multidev_begin(ao, &dcs->multidev);
>> +        dcs->multidev.callback = domcreate_attach_pci;
>> +        libxl__add_vtpms(egc, ao, domid, d_config,
&dcs->multidev);
>> +        libxl__multidev_prepared(egc, &dcs->multidev, 0);
>> +        return;
>> +    }
>> +
>> +   domcreate_attach_pci(egc, multidev, 0);
>> +   return;
>> +
>> +error_out:
>> +   assert(ret);
>> +   domcreate_complete(egc, dcs, ret);
>> +}
>> +
>>  static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev
*multidev,
>>                                   int ret)
>>  {
>> @@ -1111,7 +1147,7 @@ static void domcreate_attach_pci(libxl__egc *egc,
libxl__multidev *multidev,
>>      libxl_domain_config *const d_config = dcs->guest_config;
>>
>>      if (ret) {
>> -        LOG(ERROR, "unable to add nic devices");
>> +        LOG(ERROR, "unable to add vtpm devices");
>>          goto error_out;
>>      }
>>
> [snip]
>> @@ -1045,6 +1045,47 @@ static void parse_config_data(const char
*config_source,
>>          }
>>      }
>>
>> +    if (!xlu_cfg_get_list(config, "vtpm", &vtpms, 0, 0))
{
>> +        d_config->num_vtpms = 0;
>> +        d_config->vtpms = NULL;
>> +        while ((buf = xlu_cfg_get_listitem (vtpms,
d_config->num_vtpms)) != NULL) {
>> +            libxl_device_vtpm *vtpm;
>> +            char * buf2 = strdup(buf);
>> +            char *p, *p2;
>> +
>> +            d_config->vtpms = (libxl_device_vtpm *)
realloc(d_config->vtpms, sizeof(libxl_device_vtpm) *
(d_config->num_vtpms+1));
>> +            vtpm = d_config->vtpms + d_config->num_vtpms;
>> +            libxl_device_vtpm_init(vtpm);
>> +            vtpm->devid = d_config->num_vtpms;
>> +
>> +            p = strtok(buf2, ",");
>> +            if (!p)
>> +                goto skip_vtpm;
> Is the purpose of this so that you can have "empty" vtpm slots?
> (Since even when skipping, you still increment the num_vtpms counter?)
That would make a default vtpm with a randomly generated uuid and
backend=dom0. Considering that were getting rid of the process model, it
probably makes sense to force the user to specify a backend domain id
because no vtpm device will ever connect to dom0 anymore.

See comments about uuid below.
>
>> +            do {
>> +                while(*p == '' '')
>> +                    ++p;
>> +                if ((p2 = strchr(p, ''='')) == NULL)
>> +                    break;
>> +                *p2 = ''\0'';
>> +                if (!strcmp(p, "backend")) {
>> +                    if(domain_qualifier_to_domid(p2 + 1,
&(vtpm->backend_domid), 0))
>> +                    {
>> +                        fprintf(stderr, "Specified backend domain
does not exist, defaulting to Dom0\n");
>> +                        vtpm->backend_domid = 0;
> So wait; if someone specifies a domain, and that turns out not to
> work, you just change it to 0?  It seems like if the *specified*
> domain doesn''t exist, the command should fail instead of choosing
> something at random.
I agree.
>
>> +                    }
>> +                } else if(!strcmp(p, "uuid")) {
>> +                    if( libxl_uuid_from_string(&vtpm->uuid, p2
+ 1) ) {
>> +                        fprintf(stderr, "Failed to parse vtpm
UUID: %s\n", p2 + 1);
>> +                        exit(1);
>> +                    }
>> +                }
> If I''m parsing this right, it looks like you will just silently
ignore
> other arguments -- it seems like throwing an error would be better;
> especially to catch things like typos.  Otherwise, if someone does
> something like "uid=[whatever]", it will act like uuid
isn''t there and
> create a new one, instead of alerting the user to the fact that
he''d
> made a typo in the config file.
The behavior here is there if the user passes an invalid uuid string it
will fail with a parse error, but if the user does not specify a uuid at
all, one will be randomly generated. Random generation happens in the
vtpm types constructor in the xl type system.

This brings up a bigger wart in the vtpm implementation.

Its kind of confusing now because the linux guest uses a tpmfront/back
pair to talk to the vtpm, and then vtpm uses another tpmfront/back pair
to talk to the manager. You have to specify the uuid on the vtpm''s
tpmfront device because that is the device the manager sees. You do not
have to specify one on the linux domains device.

I''d argue that now, especially with the process model gone, the uuid
should be a parameter of the vtpm itself and not the tpmfront/back
communication channels.

The problem is that this uuid needs to specified by the "control
domain"
or dom0. By attaching the uuid to the device, the manager can trust the
uuid attached to whoever is trying to connect to him.

One idea is to make the uuid a commandline parameter for the mini-os
domain and have the vtpm pass the id down to the manager. That means
however that any rogue domain could potentially connect to the manager
and send him someone elses uuid, and thus being able to access the vtpms
stored secrets.

However one could argue that no domain would be able to connect to the
manager to do this anyway because they would have to create a
tpmfront/back device pair and the only way to do that is to break into
the "control domain." If one can do this, then one could just as
easily
set their device uuid to whatever they want.

I hope all that made sense. Do you see any flaws in my reasoning? If so
I should probably get uuids out of the vtpm devices and simplify this
whole thing.

>
>  -George



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

On Fri, Sep 28, 2012 at 5:06 PM, Matthew Fioravante
<matthew.fioravante@jhuapl.edu> wrote:
> On 09/28/2012 11:03 AM, George Dunlap wrote:
>> On Thu, Sep 27, 2012 at 6:11 PM, Matthew Fioravante
>> <matthew.fioravante@jhuapl.edu> wrote:
>>> This patch adds vtpm support to libxl. It adds vtpm parsing to
config
>>> files and 3 new xl commands:
>>> vtpm-attach
>>> vtpm-detach
>>> vtpm-list
>>>
>>> Signed-off-by: Matthew Fioravante
<matthew.fioravante@jhuapl.edu>
>> Overall looks good to me -- just a few comments below about the config
>> file handling (see below).
>>
>> Thanks for all your work on this.
>>
>>> @@ -601,6 +605,8 @@ static void
domcreate_bootloader_done(libxl__egc *egc,
>>>  static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev
*aodevs,
>>>                                  int ret);
>>>
>>> +static void domcreate_attach_vtpms(libxl__egc *egc,
libxl__multidev *multidev,
>>> +                                   int ret);
>>>  static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev
*aodevs,
>>>                                   int ret);
>>>
>>> @@ -1084,13 +1090,13 @@ static void
domcreate_devmodel_started(libxl__egc *egc,
>>>      if (d_config->num_nics > 0) {
>>>          /* Attach nics */
>>>          libxl__multidev_begin(ao, &dcs->multidev);
>>> -        dcs->multidev.callback = domcreate_attach_pci;
>>> +        dcs->multidev.callback = domcreate_attach_vtpms;
>> Wow -- what a weird convention you''ve had to try to figure out
and
>> modify here.  Well done. :-)
> It was really tricky. Is there no better way to handle asynchronous
> code? This method seems really error prone and almost impossible to follow.
Well I didn''t write it. :-)  I haven''t taken the time to
figure out
why it might have been written that way; but at first glance, I tend
to agree with you.  For about 10 minutes I was convinced you had made
some kind of weird error, by sprinkling "vtpm" around things that
obviously were supposed to be about nics and pci devices, until I
realized you were just following the existing "call chain" convention.
>>> +            p = strtok(buf2, ",");
>>> +            if (!p)
>>> +                goto skip_vtpm;
>> Is the purpose of this so that you can have "empty" vtpm
slots?
>> (Since even when skipping, you still increment the num_vtpms counter?)
> That would make a default vtpm with a randomly generated uuid and
> backend=dom0. Considering that were getting rid of the process model, it
> probably makes sense to force the user to specify a backend domain id
> because no vtpm device will ever connect to dom0 anymore.
Ah, right.  Either way is OK with me, but a comment would be useful. :-)
>>
>>> +                    }
>>> +                } else if(!strcmp(p, "uuid")) {
>>> +                    if( libxl_uuid_from_string(&vtpm->uuid,
p2 + 1) ) {
>>> +                        fprintf(stderr, "Failed to parse vtpm
UUID: %s\n", p2 + 1);
>>> +                        exit(1);
>>> +                    }
>>> +                }
>> If I''m parsing this right, it looks like you will just
silently ignore
>> other arguments -- it seems like throwing an error would be better;
>> especially to catch things like typos.  Otherwise, if someone does
>> something like "uid=[whatever]", it will act like uuid
isn''t there and
>> create a new one, instead of alerting the user to the fact that
he''d
>> made a typo in the config file.
> The behavior here is there if the user passes an invalid uuid string it
> will fail with a parse error, but if the user does not specify a uuid at
> all, one will be randomly generated. Random generation happens in the
> vtpm types constructor in the xl type system.
I think you misunderstood my comment; I''m not actually talking about
the uuid clause that''s there, but the "none of the above"
clause
that''s missing.  The code says (in pseudocode):

if("backend")
 parse backend;
else if("uuid")
 parse uuid;

But what if it''s neither "backend" or "uuid", but
something else --
say, "uid" or "backedn"?  Then instead of giving an error,
it will
just skip that argument and go on to the next one; and if the user
*intended* to type "backend" instead of "backedn", it will
silently
use the default, giving her no clue as to what the problem might be.
I''m proposing adding (again in pseudocode):

else
  error("Unrecognized argument: %s\n", p);

Does that make sense?
> This brings up a bigger wart in the vtpm implementation.
It''s 5:30pm on a Friday, so I''m going to put off grokking the
rest of
this until Monday morning. :-)

Have a good weekend,
 -George
>
> Its kind of confusing now because the linux guest uses a tpmfront/back
> pair to talk to the vtpm, and then vtpm uses another tpmfront/back pair
> to talk to the manager. You have to specify the uuid on the vtpm''s
> tpmfront device because that is the device the manager sees. You do not
> have to specify one on the linux domains device.
>
> I''d argue that now, especially with the process model gone, the
uuid
> should be a parameter of the vtpm itself and not the tpmfront/back
> communication channels.
>
> The problem is that this uuid needs to specified by the "control
domain"
> or dom0. By attaching the uuid to the device, the manager can trust the
> uuid attached to whoever is trying to connect to him.
>
> One idea is to make the uuid a commandline parameter for the mini-os
> domain and have the vtpm pass the id down to the manager. That means
> however that any rogue domain could potentially connect to the manager
> and send him someone elses uuid, and thus being able to access the vtpms
> stored secrets.
>
> However one could argue that no domain would be able to connect to the
> manager to do this anyway because they would have to create a
> tpmfront/back device pair and the only way to do that is to break into
> the "control domain." If one can do this, then one could just as
easily
> set their device uuid to whatever they want.
>
> I hope all that made sense. Do you see any flaws in my reasoning? If so
> I should probably get uuids out of the vtpm devices and simplify this
> whole thing.
>
>
>>
>>  -George
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

On 09/28/2012 12:39 PM, George Dunlap wrote:
> On Fri, Sep 28, 2012 at 5:06 PM, Matthew Fioravante
> <matthew.fioravante@jhuapl.edu> wrote:
>> On 09/28/2012 11:03 AM, George Dunlap wrote:
>>> On Thu, Sep 27, 2012 at 6:11 PM, Matthew Fioravante
>>> <matthew.fioravante@jhuapl.edu> wrote:
>>>> This patch adds vtpm support to libxl. It adds vtpm parsing to
config
>>>> files and 3 new xl commands:
>>>> vtpm-attach
>>>> vtpm-detach
>>>> vtpm-list
>>>>
>>>> Signed-off-by: Matthew Fioravante
<matthew.fioravante@jhuapl.edu>
>>> Overall looks good to me -- just a few comments below about the
config
>>> file handling (see below).
>>>
>>> Thanks for all your work on this.
>>>
>>>> @@ -601,6 +605,8 @@ static void
domcreate_bootloader_done(libxl__egc *egc,
>>>>  static void domcreate_launch_dm(libxl__egc *egc,
libxl__multidev *aodevs,
>>>>                                  int ret);
>>>>
>>>> +static void domcreate_attach_vtpms(libxl__egc *egc,
libxl__multidev *multidev,
>>>> +                                   int ret);
>>>>  static void domcreate_attach_pci(libxl__egc *egc,
libxl__multidev *aodevs,
>>>>                                   int ret);
>>>>
>>>> @@ -1084,13 +1090,13 @@ static void
domcreate_devmodel_started(libxl__egc *egc,
>>>>      if (d_config->num_nics > 0) {
>>>>          /* Attach nics */
>>>>          libxl__multidev_begin(ao, &dcs->multidev);
>>>> -        dcs->multidev.callback = domcreate_attach_pci;
>>>> +        dcs->multidev.callback = domcreate_attach_vtpms;
>>> Wow -- what a weird convention you''ve had to try to figure
out and
>>> modify here.  Well done. :-)
>> It was really tricky. Is there no better way to handle asynchronous
>> code? This method seems really error prone and almost impossible to
follow.
> Well I didn''t write it. :-)  I haven''t taken the time to
figure out
> why it might have been written that way; but at first glance, I tend
> to agree with you.  For about 10 minutes I was convinced you had made
> some kind of weird error, by sprinkling "vtpm" around things that
> obviously were supposed to be about nics and pci devices, until I
> realized you were just following the existing "call chain"
convention.
>
>>>> +            p = strtok(buf2, ",");
>>>> +            if (!p)
>>>> +                goto skip_vtpm;
>>> Is the purpose of this so that you can have "empty" vtpm
slots?
>>> (Since even when skipping, you still increment the num_vtpms
counter?)
>> That would make a default vtpm with a randomly generated uuid and
>> backend=dom0. Considering that were getting rid of the process model,
it
>> probably makes sense to force the user to specify a backend domain id
>> because no vtpm device will ever connect to dom0 anymore.
> Ah, right.  Either way is OK with me, but a comment would be useful. :-)
>
>>>> +                    }
>>>> +                } else if(!strcmp(p, "uuid")) {
>>>> +                    if(
libxl_uuid_from_string(&vtpm->uuid, p2 + 1) ) {
>>>> +                        fprintf(stderr, "Failed to parse
vtpm UUID: %s\n", p2 + 1);
>>>> +                        exit(1);
>>>> +                    }
>>>> +                }
>>> If I''m parsing this right, it looks like you will just
silently ignore
>>> other arguments -- it seems like throwing an error would be better;
>>> especially to catch things like typos.  Otherwise, if someone does
>>> something like "uid=[whatever]", it will act like uuid
isn''t there and
>>> create a new one, instead of alerting the user to the fact that
he''d
>>> made a typo in the config file.
>> The behavior here is there if the user passes an invalid uuid string it
>> will fail with a parse error, but if the user does not specify a uuid
at
>> all, one will be randomly generated. Random generation happens in the
>> vtpm types constructor in the xl type system.
> I think you misunderstood my comment; I''m not actually talking
about
> the uuid clause that''s there, but the "none of the
above" clause
> that''s missing.  The code says (in pseudocode):
>
> if("backend")
>  parse backend;
> else if("uuid")
>  parse uuid;
>
> But what if it''s neither "backend" or "uuid",
but something else --
> say, "uid" or "backedn"?  Then instead of giving an
error, it will
> just skip that argument and go on to the next one; and if the user
> *intended* to type "backend" instead of "backedn", it
will silently
> use the default, giving her no clue as to what the problem might be.
> I''m proposing adding (again in pseudocode):
>
> else
>   error("Unrecognized argument: %s\n", p);
>
> Does that make sense?
>
Yeah, didn''t see that. Good catch.
>> This brings up a bigger wart in the vtpm implementation.
> It''s 5:30pm on a Friday, so I''m going to put off grokking
the rest of
> this until Monday morning. :-)
Agreed, enjoy your weekend :)
> Have a good weekend,
>  -George
>
>> Its kind of confusing now because the linux guest uses a tpmfront/back
>> pair to talk to the vtpm, and then vtpm uses another tpmfront/back pair
>> to talk to the manager. You have to specify the uuid on the
vtpm''s
>> tpmfront device because that is the device the manager sees. You do not
>> have to specify one on the linux domains device.
>>
>> I''d argue that now, especially with the process model gone,
the uuid
>> should be a parameter of the vtpm itself and not the tpmfront/back
>> communication channels.
>>
>> The problem is that this uuid needs to specified by the "control
domain"
>> or dom0. By attaching the uuid to the device, the manager can trust the
>> uuid attached to whoever is trying to connect to him.
>>
>> One idea is to make the uuid a commandline parameter for the mini-os
>> domain and have the vtpm pass the id down to the manager. That means
>> however that any rogue domain could potentially connect to the manager
>> and send him someone elses uuid, and thus being able to access the
vtpms
>> stored secrets.
>>
>> However one could argue that no domain would be able to connect to the
>> manager to do this anyway because they would have to create a
>> tpmfront/back device pair and the only way to do that is to break into
>> the "control domain." If one can do this, then one could just
as easily
>> set their device uuid to whatever they want.
>>
>> I hope all that made sense. Do you see any flaws in my reasoning? If so
>> I should probably get uuids out of the vtpm devices and simplify this
>> whole thing.
>>
>>
>>>  -George
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>>



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

On 09/28/2012 12:41 PM, Matthew Fioravante wrote:
> On 09/28/2012 12:39 PM, George Dunlap wrote:
>> On Fri, Sep 28, 2012 at 5:06 PM, Matthew Fioravante
>> <matthew.fioravante@jhuapl.edu> wrote:
>>> On 09/28/2012 11:03 AM, George Dunlap wrote:
>>>> On Thu, Sep 27, 2012 at 6:11 PM, Matthew Fioravante
>>>> <matthew.fioravante@jhuapl.edu> wrote:
>>>>> This patch adds vtpm support to libxl. It adds vtpm parsing
to config
>>>>> files and 3 new xl commands:
>>>>> vtpm-attach
>>>>> vtpm-detach
>>>>> vtpm-list
>>>>>
>>>>> Signed-off-by: Matthew Fioravante
<matthew.fioravante@jhuapl.edu>
>>>> Overall looks good to me -- just a few comments below about the
config
>>>> file handling (see below).
>>>>
>>>> Thanks for all your work on this.
>>>>
>>>>> @@ -601,6 +605,8 @@ static void
domcreate_bootloader_done(libxl__egc *egc,
>>>>>  static void domcreate_launch_dm(libxl__egc *egc,
libxl__multidev *aodevs,
>>>>>                                  int ret);
>>>>>
>>>>> +static void domcreate_attach_vtpms(libxl__egc *egc,
libxl__multidev *multidev,
>>>>> +                                   int ret);
>>>>>  static void domcreate_attach_pci(libxl__egc *egc,
libxl__multidev *aodevs,
>>>>>                                   int ret);
>>>>>
>>>>> @@ -1084,13 +1090,13 @@ static void
domcreate_devmodel_started(libxl__egc *egc,
>>>>>      if (d_config->num_nics > 0) {
>>>>>          /* Attach nics */
>>>>>          libxl__multidev_begin(ao, &dcs->multidev);
>>>>> -        dcs->multidev.callback = domcreate_attach_pci;
>>>>> +        dcs->multidev.callback =
domcreate_attach_vtpms;
>>>> Wow -- what a weird convention you''ve had to try to
figure out and
>>>> modify here.  Well done. :-)
>>> It was really tricky. Is there no better way to handle asynchronous
>>> code? This method seems really error prone and almost impossible to
follow.
>> Well I didn''t write it. :-)  I haven''t taken the time
to figure out
>> why it might have been written that way; but at first glance, I tend
>> to agree with you.  For about 10 minutes I was convinced you had made
>> some kind of weird error, by sprinkling "vtpm" around things
that
>> obviously were supposed to be about nics and pci devices, until I
>> realized you were just following the existing "call chain"
convention.
>>
>>>>> +            p = strtok(buf2, ",");
>>>>> +            if (!p)
>>>>> +                goto skip_vtpm;
>>>> Is the purpose of this so that you can have "empty"
vtpm slots?
>>>> (Since even when skipping, you still increment the num_vtpms
counter?)
>>> That would make a default vtpm with a randomly generated uuid and
>>> backend=dom0. Considering that were getting rid of the process
model, it
>>> probably makes sense to force the user to specify a backend domain
id
>>> because no vtpm device will ever connect to dom0 anymore.
>> Ah, right.  Either way is OK with me, but a comment would be useful.
:-)
>>
>>>>> +                    }
>>>>> +                } else if(!strcmp(p, "uuid")) {
>>>>> +                    if(
libxl_uuid_from_string(&vtpm->uuid, p2 + 1) ) {
>>>>> +                        fprintf(stderr, "Failed to
parse vtpm UUID: %s\n", p2 + 1);
>>>>> +                        exit(1);
>>>>> +                    }
>>>>> +                }
>>>> If I''m parsing this right, it looks like you will just
silently ignore
>>>> other arguments -- it seems like throwing an error would be
better;
>>>> especially to catch things like typos.  Otherwise, if someone
does
>>>> something like "uid=[whatever]", it will act like
uuid isn''t there and
>>>> create a new one, instead of alerting the user to the fact that
he''d
>>>> made a typo in the config file.
>>> The behavior here is there if the user passes an invalid uuid
string it
>>> will fail with a parse error, but if the user does not specify a
uuid at
>>> all, one will be randomly generated. Random generation happens in
the
>>> vtpm types constructor in the xl type system.
>> I think you misunderstood my comment; I''m not actually talking
about
>> the uuid clause that''s there, but the "none of the
above" clause
>> that''s missing.  The code says (in pseudocode):
>>
>> if("backend")
>>  parse backend;
>> else if("uuid")
>>  parse uuid;
>>
>> But what if it''s neither "backend" or
"uuid", but something else --
>> say, "uid" or "backedn"?  Then instead of giving an
error, it will
>> just skip that argument and go on to the next one; and if the user
>> *intended* to type "backend" instead of "backedn",
it will silently
>> use the default, giving her no clue as to what the problem might be.
>> I''m proposing adding (again in pseudocode):
>>
>> else
>>   error("Unrecognized argument: %s\n", p);
>>
>> Does that make sense?
>>
> Yeah, didn''t see that. Good catch.
>>> This brings up a bigger wart in the vtpm implementation.
>> It''s 5:30pm on a Friday, so I''m going to put off
grokking the rest of
>> this until Monday morning. :-)
> Agreed, enjoy your weekend :)
>> Have a good weekend,
>>  -George
>>
>>> Its kind of confusing now because the linux guest uses a
tpmfront/back
>>> pair to talk to the vtpm, and then vtpm uses another tpmfront/back
pair
>>> to talk to the manager. You have to specify the uuid on the
vtpm''s
>>> tpmfront device because that is the device the manager sees. You do
not
>>> have to specify one on the linux domains device.
>>>
>>> I''d argue that now, especially with the process model
gone, the uuid
>>> should be a parameter of the vtpm itself and not the tpmfront/back
>>> communication channels.
>>>
>>> The problem is that this uuid needs to specified by the
"control domain"
>>> or dom0. By attaching the uuid to the device, the manager can trust
the
>>> uuid attached to whoever is trying to connect to him.
>>>
>>> One idea is to make the uuid a commandline parameter for the
mini-os
>>> domain and have the vtpm pass the id down to the manager. That
means
>>> however that any rogue domain could potentially connect to the
manager
>>> and send him someone elses uuid, and thus being able to access the
vtpms
>>> stored secrets.
>>>
>>> However one could argue that no domain would be able to connect to
the
>>> manager to do this anyway because they would have to create a
>>> tpmfront/back device pair and the only way to do that is to break
into
>>> the "control domain." If one can do this, then one could
just as easily
>>> set their device uuid to whatever they want.
>>>
>>> I hope all that made sense. Do you see any flaws in my reasoning?
If so
>>> I should probably get uuids out of the vtpm devices and simplify
this
>>> whole thing.
Actually thinking about it more, uuids have to be attached to the
driver. If 2 vtpms connect to the manager, one could send the uuid of
the other and get access to someone elses secrets.

TL;DR version

uuids must remain part of the driver.
>>>
>>>>  -George
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
>>>
>



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

On Thu, 2012-09-27 at 18:10 +0100, Matthew Fioravante
wrote:
> This patch adds a new option for xen config files for
> directly mapping hardware io memory into a vm.
> 
> Signed-off-by: Matthew Fioravante <matthew.fioravante@jhuapl.edu>
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 013270d..428da21 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -496,6 +496,17 @@ is given in hexadecimal and may either a span e.g.
C<2f8-2ff>
>  It is recommended to use this option only for trusted VMs under
>  administrator control.
>  
> +=item B<iomem=[ "IOMEM_START,NUM_PAGES",
"IOMEM_START,NUM_PAGES", ... ]>
> +
> +Allow guest to access specific hardware I/O memory pages.
B<IOMEM_START>
> +is a physical page number. B<NUM_PAGES> is the number
> +of pages beginning with B<START_PAGE> to allow access. Both values
> +must be given in hexadecimal.
> +
> +It is recommended to use this option only for trusted VMs under
> +administrator control.
> +
> +
>  =item B<irqs=[ NUMBER, NUMBER, ... ]>
>  
>  Allow a guest to access specific physical IRQs.
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index ef17f05..6cb586b 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -963,6 +963,24 @@ static void domcreate_launch_dm(libxl__egc *egc,
libxl__multidev *multidev,
>          }
>      }
>  
> +    for (i = 0; i < d_config->b_info.num_iomem; i++) {
> +        libxl_iomem_range *io = &d_config->b_info.iomem[i];
> +
> +        LOG(DEBUG, "dom%d iomem %"PRIx64"-%"PRIx64,
> +            domid, io->start, io->start + io->number - 1);
> +
> +        ret = xc_domain_iomem_permission(CTX->xch, domid,
> +                                          io->start, io->number, 1);
> +        if ( ret<0 ) {
This should be
          if (ret < 0) {
> +            LOGE(ERROR,
> +                 "failed give dom%d access to iomem range
%"PRIx64"-%"PRIx64,
> +                 domid, io->start, io->start + io->number - 1);
> +            ret = ERROR_FAIL;
> +        }
> +    }
> +
> +
> +
>      for (i = 0; i < d_config->num_nics; i++) {
>          /* We have to init the nic here, because we still haven''t
>           * called libxl_device_nic_add at this point, but qemu needs
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 6d5c578..cf83c60 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -140,6 +140,11 @@ libxl_ioport_range = Struct("ioport_range",
[
>      ("number", uint32),
>      ])
>  
> +libxl_iomem_range = Struct("iomem_range", [
> +    ("start", uint64),
> +    ("number", uint64),
> +    ])
> +
>  libxl_vga_interface_info = Struct("vga_interface_info", [
>      ("kind",    libxl_vga_interface_type),
>      ])
> @@ -284,6 +289,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
>  
>      ("ioports",          Array(libxl_ioport_range,
"num_ioports")),
>      ("irqs",             Array(uint32, "num_irqs")),
> +    ("iomem",            Array(libxl_iomem_range,
"num_iomem")),
>  
>      ("u", KeyedUnion(None, libxl_domain_type, "type",
>                  [("hvm", Struct(None, [("firmware",   
string),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 1627cac..fe8e925 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -574,8 +574,8 @@ static void parse_config_data(const char
*config_source,
>      long l;
>      XLU_Config *config;
>      XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids;
> -    XLU_ConfigList *ioports, *irqs;
> -    int num_ioports, num_irqs;
> +    XLU_ConfigList *ioports, *irqs, *iomem;
> +    int num_ioports, num_irqs, num_iomem;
>      int pci_power_mgmt = 0;
>      int pci_msitranslate = 0;
>      int pci_permissive = 0;
> @@ -1005,6 +1005,30 @@ static void parse_config_data(const char
*config_source,
>          }
>      }
>  
> +    if (!xlu_cfg_get_list(config, "iomem", &iomem,
&num_iomem, 0)) {
> +        b_info->num_iomem = num_iomem;
> +        b_info->iomem = calloc(num_iomem, sizeof(*b_info->iomem));
> +        if (b_info->iomem == NULL) {
> +            fprintf(stderr, "unable to allocate memory for
iomem\n");
> +            exit(-1);
> +        }
> +        for (i = 0; i < num_iomem; i++) {
> +            buf = xlu_cfg_get_listitem (iomem, i);
> +            if (!buf) {
> +                fprintf(stderr,
> +                        "xl: Unable to get element %d in iomem
list\n", i);
> +                exit(1);
> +            }
> +            if(sscanf(buf, "%" SCNx64",%" SCNx64,
&b_info->iomem[i].start, &b_info->iomem[i].number) != 2) {
Can we split this over multiple lines please, to keep it under the 75-80
column limit mention in coding style.
> +               fprintf(stderr,
> +                       "xl: Invalid argument parsing iomem:
%s\n", buf);
> +               exit(1);
> +            }
> +        }
> +    }
> +
> +
> +
>      if (!xlu_cfg_get_list (config, "disk", &vbds, 0, 0)) {
>          d_config->num_disks = 0;
>          d_config->disks = NULL;

On Fri, 2012-09-28 at 14:23 +0100, George Dunlap wrote:
> On Thu, Sep 27, 2012 at 6:10 PM, Matthew Fioravante
> <matthew.fioravante@jhuapl.edu> wrote:
> > Previously device ids in libxl were treated as integers meaning they
> > were being initialized to 0, which is a valid device id. This patch
> > makes devid its own type in libxl and initializes it to -1, an invalid
> > value.
> >
> > This fixes a bug where if you try to do a xl DEV-attach multiple
> > time it will continuously try to reattach device 0 instead of
> > generating a new device id.
> >
> > Signed-off-by: Matthew Fioravante
<matthew.fioravante@jhuapl.edu>
> 
> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> Should this be backported to 4.2-testing?
That''s a good question. I think it is basically a bugfix to the API and
since basically the only way to correctly use the current interface is
for the application to always initialise that field changing the initial
value seems harmless from a backwards compat PoV.

Ian.
> 
>  -George
> 
> >
> > diff --git a/tools/libxl/gentest.py b/tools/libxl/gentest.py
> > index 2915f71..84b4fd7 100644
> > --- a/tools/libxl/gentest.py
> > +++ b/tools/libxl/gentest.py
> > @@ -60,7 +60,7 @@ def gen_rand_init(ty, v, indent = "    ",
parent = None):
> >                                         
passby=idl.PASS_BY_REFERENCE))
> >      elif ty.typename in ["libxl_uuid",
"libxl_mac", "libxl_hwcap"]:
> >          s += "rand_bytes((uint8_t *)%s, sizeof(*%s));\n" %
(v,v)
> > -    elif ty.typename in ["libxl_domid"] or isinstance(ty,
idl.Number):
> > +    elif ty.typename in ["libxl_domid",
"libxl_devid"] or isinstance(ty, idl.Number):
> >          s += "%s = rand() %% (sizeof(%s)*8);\n" % \
> >               (ty.pass_arg(v, parent is None),
> >                ty.pass_arg(v, parent is None))
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 599c7f1..7a7c419 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -307,6 +307,7 @@ void libxl_cpuid_dispose(libxl_cpuid_policy_list
*cpuid_list);
> >  #define LIBXL_PCI_FUNC_ALL (~0U)
> >
> >  typedef uint32_t libxl_domid;
> > +typedef int libxl_devid;
> >
> >  /*
> >   * Formatting Enumerations.
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index cf83c60..de111a6 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -8,6 +8,7 @@ namespace("libxl_")
> >  libxl_defbool = Builtin("defbool",
passby=PASS_BY_REFERENCE)
> >
> >  libxl_domid = Builtin("domid", json_fn =
"yajl_gen_integer", autogenerate_json = False)
> > +libxl_devid = Builtin("devid", json_fn =
"yajl_gen_integer", autogenerate_json = False, signed = True,
init_val="-1")
> >  libxl_uuid = Builtin("uuid", passby=PASS_BY_REFERENCE)
> >  libxl_mac = Builtin("mac", passby=PASS_BY_REFERENCE)
> >  libxl_bitmap = Builtin("bitmap",
dispose_fn="libxl_bitmap_dispose", passby=PASS_BY_REFERENCE)
> > @@ -343,7 +344,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
> >
> >  libxl_device_vfb = Struct("device_vfb", [
> >      ("backend_domid", libxl_domid),
> > -    ("devid",         integer),
> > +    ("devid",         libxl_devid),
> >      ("vnc",           libxl_vnc_info),
> >      ("sdl",           libxl_sdl_info),
> >      # set keyboard layout, default is en-us keyboard
> > @@ -352,7 +353,7 @@ libxl_device_vfb = Struct("device_vfb",
[
> >
> >  libxl_device_vkb = Struct("device_vkb", [
> >      ("backend_domid", libxl_domid),
> > -    ("devid", integer),
> > +    ("devid", libxl_devid),
> >      ])
> >
> >  libxl_device_disk = Struct("device_disk", [
> > @@ -369,7 +370,7 @@ libxl_device_disk =
Struct("device_disk", [
> >
> >  libxl_device_nic = Struct("device_nic", [
> >      ("backend_domid", libxl_domid),
> > -    ("devid", integer),
> > +    ("devid", libxl_devid),
> >      ("mtu", integer),
> >      ("model", string),
> >      ("mac", libxl_mac),
> > @@ -399,7 +400,7 @@ libxl_diskinfo = Struct("diskinfo", [
> >      ("backend_id", uint32),
> >      ("frontend", string),
> >      ("frontend_id", uint32),
> > -    ("devid", integer),
> > +    ("devid", libxl_devid),
> >      ("state", integer),
> >      ("evtch", integer),
> >      ("rref", integer),
> > @@ -410,7 +411,7 @@ libxl_nicinfo = Struct("nicinfo", [
> >      ("backend_id", uint32),
> >      ("frontend", string),
> >      ("frontend_id", uint32),
> > -    ("devid", integer),
> > +    ("devid", libxl_devid),
> >      ("state", integer),
> >      ("evtch", integer),
> >      ("rref_tx", integer),
> > diff --git a/tools/ocaml/libs/xl/genwrap.py
b/tools/ocaml/libs/xl/genwrap.py
> > index 42f374e..97d088d 100644
> > --- a/tools/ocaml/libs/xl/genwrap.py
> > +++ b/tools/ocaml/libs/xl/genwrap.py
> > @@ -10,6 +10,7 @@ builtins = {
> >      "int":                  ("int",              
"%(c)s = Int_val(%(o)s)",            "Val_int(%(c)s)"  ),
> >      "char *":               ("string",           
"%(c)s = dup_String_val(gc, %(o)s)",
"caml_copy_string(%(c)s)"),
> >      "libxl_domid":          ("domid",            
"%(c)s = Int_val(%(o)s)",            "Val_int(%(c)s)"  ),
> > +    "libxl_devid":          ("devid",            
"%(c)s = Int_val(%(o)s)",            "Val_int(%(c)s)"  ),
> >      "libxl_defbool":        ("bool option",      
"%(c)s = Defbool_val(%(o)s)",        "Val_defbool(%(c)s)" ),
> >      "libxl_uuid":           ("int array",        
"Uuid_val(gc, lg, &%(c)s, %(o)s)",  
"Val_uuid(&%(c)s)"),
> >      "libxl_key_value_list": ("(string * string)
list", None,                                None),
> > @@ -41,8 +42,8 @@ def stub_fn_name(ty, name):
> >      return "stub_xl_%s_%s" % (ty.rawname,name)
> >
> >  def ocaml_type_of(ty):
> > -    if ty.rawname == "domid":
> > -        return "domid"
> > +    if ty.rawname in ["domid","devid"]:
> > +        return ty.rawname
> >      elif isinstance(ty,idl.UInt):
> >          if ty.width in [8, 16]:
> >              # handle as ints
> > diff --git a/tools/ocaml/libs/xl/xenlight.ml.in
b/tools/ocaml/libs/xl/xenlight.ml.in
> > index c47623c..dcc1a38 100644
> > --- a/tools/ocaml/libs/xl/xenlight.ml.in
> > +++ b/tools/ocaml/libs/xl/xenlight.ml.in
> > @@ -16,6 +16,7 @@
> >  exception Error of string
> >
> >  type domid = int
> > +type devid = int
> >
> >  (* @@LIBXL_TYPES@@ *)
> >
> > diff --git a/tools/ocaml/libs/xl/xenlight.mli.in
b/tools/ocaml/libs/xl/xenlight.mli.in
> > index 4717bac..3fd0165 100644
> > --- a/tools/ocaml/libs/xl/xenlight.mli.in
> > +++ b/tools/ocaml/libs/xl/xenlight.mli.in
> > @@ -16,6 +16,7 @@
> >  exception Error of string
> >
> >  type domid = int
> > +type devid = int
> >
> >  (* @@LIBXL_TYPES@@ *)
> >
> > diff --git a/tools/python/xen/lowlevel/xl/xl.c
b/tools/python/xen/lowlevel/xl/xl.c
> > index 0551c76..32f982a 100644
> > --- a/tools/python/xen/lowlevel/xl/xl.c
> > +++ b/tools/python/xen/lowlevel/xl/xl.c
> > @@ -281,6 +281,11 @@ int attrib__libxl_domid_set(PyObject *v,
libxl_domid *domid) {
> >      return 0;
> >  }
> >
> > +int attrib__libxl_devid_set(PyObject *v, libxl_devid *devid) {
> > +   *devid = PyInt_AsLong(v);
> > +   return 0;
> > +}
> > +
> >  int attrib__struct_in_addr_set(PyObject *v, struct in_addr *pptr)
> >  {
> >      PyErr_SetString(PyExc_NotImplementedError, "Setting
in_addr");
> > @@ -342,6 +347,10 @@ PyObject *attrib__libxl_domid_get(libxl_domid
*domid) {
> >      return PyInt_FromLong(*domid);
> >  }
> >
> > +PyObject *attrib__libxl_devid_get(libxl_devid *devid) {
> > +    return PyInt_FromLong(*devid);
> > +}
> > +
> >  PyObject *attrib__struct_in_addr_get(struct in_addr *pptr)
> >  {
> >      PyErr_SetString(PyExc_NotImplementedError, "Getting
in_addr");
> > --
> > 1.7.9.5
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel

On Mon, 2012-10-01 at 19:40 +0100, Matthew Fioravante wrote:
> Actually thinking about it more, uuids have to be attached to the
> driver. If 2 vtpms connect to the manager, one could send the uuid of
> the other and get access to someone elses secrets.
> 
> TL;DR version
> 
> uuids must remain part of the driver.
What stops the driver lying about the UUID in a similar way?

Ian.

On Fri, 2012-09-28 at 17:39 +0100, George Dunlap wrote:
> >>> @@ -1084,13 +1090,13 @@ static void
domcreate_devmodel_started(libxl__egc *egc,
> >>>      if (d_config->num_nics > 0) {
> >>>          /* Attach nics */
> >>>          libxl__multidev_begin(ao, &dcs->multidev);
> >>> -        dcs->multidev.callback = domcreate_attach_pci;
> >>> +        dcs->multidev.callback = domcreate_attach_vtpms;
> >> Wow -- what a weird convention you''ve had to try to
figure out and
> >> modify here.  Well done. :-)
> > It was really tricky. Is there no better way to handle asynchronous
> > code? This method seems really error prone and almost impossible to
follow.
> 
> Well I didn''t write it. :-)  I haven''t taken the time to
figure out
> why it might have been written that way; but at first glance, I tend
> to agree with you.  For about 10 minutes I was convinced you had made
> some kind of weird error, by sprinkling "vtpm" around things that
> obviously were supposed to be about nics and pci devices, until I
> realized you were just following the existing "call chain"
convention.
One big improvement would be to stop treating the preamble of "do_bar"
as the error handling of the preceding "do_foo", in the case where bar
happens to follow foo in the async chain.

For example nic attach''s completion handler should be called
domcreate_nicattach_done, which then calls the next step instead of
putting the nic error handling at the front of domcreate_attach_pci
because that happens to be the next call in the chain. It''s not quite
ideal but it would be substantially less confusing.

If there was further some way to make the what to call next step table
driven then even better.

Ian.

On 10/02/2012 09:34 AM, Ian Campbell wrote:
> On Thu, 2012-09-27 at 18:10 +0100, Matthew Fioravante wrote:
>> This patch adds a new option for xen config files for
>> directly mapping hardware io memory into a vm.
>>
>> Signed-off-by: Matthew Fioravante <matthew.fioravante@jhuapl.edu>
>>
>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
>> index 013270d..428da21 100644
>> --- a/docs/man/xl.cfg.pod.5
>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -496,6 +496,17 @@ is given in hexadecimal and may either a span e.g.
C<2f8-2ff>
>>  It is recommended to use this option only for trusted VMs under
>>  administrator control.
>>  
>> +=item B<iomem=[ "IOMEM_START,NUM_PAGES",
"IOMEM_START,NUM_PAGES", ... ]>
>> +
>> +Allow guest to access specific hardware I/O memory pages.
B<IOMEM_START>
>> +is a physical page number. B<NUM_PAGES> is the number
>> +of pages beginning with B<START_PAGE> to allow access. Both
values
>> +must be given in hexadecimal.
>> +
>> +It is recommended to use this option only for trusted VMs under
>> +administrator control.
>> +
>> +
>>  =item B<irqs=[ NUMBER, NUMBER, ... ]>
>>  
>>  Allow a guest to access specific physical IRQs.
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index ef17f05..6cb586b 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -963,6 +963,24 @@ static void domcreate_launch_dm(libxl__egc *egc,
libxl__multidev *multidev,
>>          }
>>      }
>>  
>> +    for (i = 0; i < d_config->b_info.num_iomem; i++) {
>> +        libxl_iomem_range *io = &d_config->b_info.iomem[i];
>> +
>> +        LOG(DEBUG, "dom%d iomem
%"PRIx64"-%"PRIx64,
>> +            domid, io->start, io->start + io->number - 1);
>> +
>> +        ret = xc_domain_iomem_permission(CTX->xch, domid,
>> +                                          io->start, io->number,
1);
>> +        if ( ret<0 ) {
> This should be
>           if (ret < 0) {
Looks like it was like that also for ioports and irqs. I fixed all
3.
>
>> +            LOGE(ERROR,
>> +                 "failed give dom%d access to iomem range
%"PRIx64"-%"PRIx64,
>> +                 domid, io->start, io->start + io->number -
1);
>> +            ret = ERROR_FAIL;
>> +        }
>> +    }
>> +
>> +
>> +
>>      for (i = 0; i < d_config->num_nics; i++) {
>>          /* We have to init the nic here, because we still
haven''t
>>           * called libxl_device_nic_add at this point, but qemu needs
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 6d5c578..cf83c60 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -140,6 +140,11 @@ libxl_ioport_range =
Struct("ioport_range", [
>>      ("number", uint32),
>>      ])
>>  
>> +libxl_iomem_range = Struct("iomem_range", [
>> +    ("start", uint64),
>> +    ("number", uint64),
>> +    ])
>> +
>>  libxl_vga_interface_info = Struct("vga_interface_info", [
>>      ("kind",    libxl_vga_interface_type),
>>      ])
>> @@ -284,6 +289,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
>>  
>>      ("ioports",          Array(libxl_ioport_range,
"num_ioports")),
>>      ("irqs",             Array(uint32,
"num_irqs")),
>> +    ("iomem",            Array(libxl_iomem_range,
"num_iomem")),
>>  
>>      ("u", KeyedUnion(None, libxl_domain_type,
"type",
>>                  [("hvm", Struct(None,
[("firmware",         string),
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 1627cac..fe8e925 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -574,8 +574,8 @@ static void parse_config_data(const char
*config_source,
>>      long l;
>>      XLU_Config *config;
>>      XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids;
>> -    XLU_ConfigList *ioports, *irqs;
>> -    int num_ioports, num_irqs;
>> +    XLU_ConfigList *ioports, *irqs, *iomem;
>> +    int num_ioports, num_irqs, num_iomem;
>>      int pci_power_mgmt = 0;
>>      int pci_msitranslate = 0;
>>      int pci_permissive = 0;
>> @@ -1005,6 +1005,30 @@ static void parse_config_data(const char
*config_source,
>>          }
>>      }
>>  
>> +    if (!xlu_cfg_get_list(config, "iomem", &iomem,
&num_iomem, 0)) {
>> +        b_info->num_iomem = num_iomem;
>> +        b_info->iomem = calloc(num_iomem,
sizeof(*b_info->iomem));
>> +        if (b_info->iomem == NULL) {
>> +            fprintf(stderr, "unable to allocate memory for
iomem\n");
>> +            exit(-1);
>> +        }
>> +        for (i = 0; i < num_iomem; i++) {
>> +            buf = xlu_cfg_get_listitem (iomem, i);
>> +            if (!buf) {
>> +                fprintf(stderr,
>> +                        "xl: Unable to get element %d in iomem
list\n", i);
>> +                exit(1);
>> +            }
>> +            if(sscanf(buf, "%" SCNx64",%" SCNx64,
&b_info->iomem[i].start, &b_info->iomem[i].number) != 2) {
> Can we split this over multiple lines please, to keep it under the 75-80
> column limit mention in coding style.
fixed
>
>> +               fprintf(stderr,
>> +                       "xl: Invalid argument parsing iomem:
%s\n", buf);
>> +               exit(1);
>> +            }
>> +        }
>> +    }
>> +
>> +
>> +
>>      if (!xlu_cfg_get_list (config, "disk", &vbds, 0, 0))
{
>>          d_config->num_disks = 0;
>>          d_config->disks = NULL;
>



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

On 10/02/2012 09:44 AM, Ian Campbell wrote:
> On Mon, 2012-10-01 at 19:40 +0100, Matthew Fioravante wrote:
>
>> Actually thinking about it more, uuids have to be attached to the
>> driver. If 2 vtpms connect to the manager, one could send the uuid of
>> the other and get access to someone elses secrets.
>>
>> TL;DR version
>>
>> uuids must remain part of the driver.
> What stops the driver lying about the UUID in a similar way?
The tpm backend driver (in the manager) will read the uuid from
xenstore. So as long as we trust xenstore ( and the entities who created
the entry, named libxl in dom0), we can trust the uuid and thus the
identity of the vtpm connecting to us.
>
> Ian.
>



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

On Tue, 2012-10-02 at 15:31 +0100, Matthew Fioravante
wrote:
> On 10/02/2012 09:44 AM, Ian Campbell wrote:
> > On Mon, 2012-10-01 at 19:40 +0100, Matthew Fioravante wrote:
> >
> >> Actually thinking about it more, uuids have to be attached to the
> >> driver. If 2 vtpms connect to the manager, one could send the uuid
of
> >> the other and get access to someone elses secrets.
> >>
> >> TL;DR version
> >>
> >> uuids must remain part of the driver.
> > What stops the driver lying about the UUID in a similar way?
> The tpm backend driver (in the manager) will read the uuid from
> xenstore. So as long as we trust xenstore ( and the entities who created
> the entry, named libxl in dom0), we can trust the uuid and thus the
> identity of the vtpm connecting to us.
OK, why does that same argument not apply when the toolstack passes the
UUID to the manager domain instead?

Ian.

On 10/02/2012 10:43 AM, Ian Campbell wrote:
> On Tue, 2012-10-02 at 15:31 +0100, Matthew Fioravante wrote:
>> On 10/02/2012 09:44 AM, Ian Campbell wrote:
>>> On Mon, 2012-10-01 at 19:40 +0100, Matthew Fioravante wrote:
>>>
>>>> Actually thinking about it more, uuids have to be attached to
the
>>>> driver. If 2 vtpms connect to the manager, one could send the
uuid of
>>>> the other and get access to someone elses secrets.
>>>>
>>>> TL;DR version
>>>>
>>>> uuids must remain part of the driver.
>>> What stops the driver lying about the UUID in a similar way?
>> The tpm backend driver (in the manager) will read the uuid from
>> xenstore. So as long as we trust xenstore ( and the entities who
created
>> the entry, named libxl in dom0), we can trust the uuid and thus the
>> identity of the vtpm connecting to us.
> OK, why does that same argument not apply when the toolstack passes the
> UUID to the manager domain instead?
The other implementation I mentioned is where the toolstack would just
create a front/back pair and the vtpm itself would send its uuid. If the
toolstack can be fooled ( as simple as replacing the vtpm kernel image)
then it will connect a bogus vtpm to the manager which can then send the
uuid of any real vtpm and get at its secrets.

In essense, attaching the uuid to the driver is basically a convenient
way of having the toolstack domain tell the vtpm manager the identity of
the vtpm. If it wasn''t attached to the driver there would have to be
some other daemon or special case code to do this.

This problem might go away later if/when we have measured boot of vtpms
but that is not currently implemented.

>
> Ian.
>
>



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

On Tue, 2012-10-02 at 14:36 +0100, Ian Campbell wrote:
> On Fri, 2012-09-28 at 14:23 +0100, George Dunlap wrote:
> > On Thu, Sep 27, 2012 at 6:10 PM, Matthew Fioravante
> > <matthew.fioravante@jhuapl.edu> wrote:
> > > Previously device ids in libxl were treated as integers meaning
they
> > > were being initialized to 0, which is a valid device id. This
patch
> > > makes devid its own type in libxl and initializes it to -1, an
invalid
> > > value.
> > >
> > > This fixes a bug where if you try to do a xl DEV-attach multiple
> > > time it will continuously try to reattach device 0 instead of
> > > generating a new device id.
> > >
> > > Signed-off-by: Matthew Fioravante
<matthew.fioravante@jhuapl.edu>
> > 
> > Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
I''ve applied this.