[PATCH v3 0/7] vTPM: Xen stubdom vTPM for HVM virtual machine

[PATCH v3 0/7] vTPM: Xen stubdom vTPM for HVM virtual machine

[Quan Xu]

This patch series are only the Xen part to enable stubdom vTPM for HVM virtual machine.
it will work w/ Qemu patch series and seaBios patch series. Change QEMU_STUBDOM_VTPM compile
option from 'n' to 'y', when the Qemu/SeaBios patch series are merged.

========================
    *INTRODUCTION*
========================
The goal of virtual Trusted Platform Module (vTPM) is to provide a TPM functionality to virtual
machines (Fedora, Ubuntu, Redhat, Windows .etc). This allows programs to interact with a TPM in
a virtual machine the same way they interact with a TPM on the physical system. Each virtual
machine gets its own unique, emulated, software TPM. Each major component of vTPM is implemented
as a stubdom, providing secure separation guaranteed by the hypervisor.

The vTPM stubdom is a Xen mini-OS domain that emulates a TPM for the virtual machine to use. It
is a small wrapper around the Berlios TPM emulator. TPM commands are passed from mini-os TPM
backend driver.

========================
     *ARCHITECTURE*
========================
The architecture of stubdom vTPM for HVM virtual machine:

            +--------------------+
            | Windows/Linux DomU | ...
            |        |  ^        |
            |        v  |        |
            |  Qemu tpm1.2 Tis   |
            |        |  ^        |
            |        v  |        |
            | XenStubdoms backend|
            +--------------------+
                     |  ^
                     v  |
            +--------------------+
            |      XenDevOps     |
            +--------------------+
                     |  ^
                     v  |
            +--------------------+
            |  mini-os/tpmback   |
            |        |  ^        |
            |        v  |        |
            |   vtpm-stubdom     | ...
            |        |  ^        |
            |        v  |        |
            |  mini-os/tpmfront  |
            +--------------------+
                     |  ^
                     v  |
            +--------------------+
            |  mini-os/tpmback   |
            |        |  ^        |
            |        v  |        |
            |  vtpmmgr-stubdom   |
            |        |  ^        |
            |        v  |        |
            |  mini-os/tpm_tis   |
            +--------------------+
                     |  ^
                     v  |
            +--------------------+
            |    Hardware TPM    |
            +--------------------+

 * Windows/Linux DomU:
    The HVM based guest that wants to use a vTPM. There may be
    more than one of these.

 * Qemu tpm1.2 Tis:
    Implementation of the tpm1.2 Tis interface for HVM virtual
    machines. It is Qemu emulation device.

 * vTPM xenstubdoms driver:
    Qemu vTPM driver. This driver provides vtpm initialization
    and sending data and commends to a para-virtualized vtpm
    stubdom.

 * XenDevOps:
    Register Xen stubdom vTPM frontend driver, and transfer any
    request/repond between TPM xenstubdoms driver and Xen vTPM
    stubdom. Facilitate communications between Xen vTPM stubdom
    and vTPM xenstubdoms driver.

 * mini-os/tpmback:
    Mini-os TPM backend driver. The Linux frontend driver connects
    to this backend driver to facilitate communications between the
    Linux DomU and its vTPM. This driver is also used by vtpmmgr
    stubdom to communicate with vtpm-stubdom.

 * vtpm-stubdom:
    A mini-os stub domain that implements a vTPM. There is a
    one to one mapping between running vtpm-stubdom instances and
    logical vtpms on the system. The vTPM Platform Configuration
    Registers (PCRs) are all initialized to zero.

 * mini-os/tpmfront:
    Mini-os TPM frontend driver. The vTPM mini-os domain vtpm
    stubdom uses this driver to communicate with vtpmmgr-stubdom.
    This driver could also be used separately to implement a mini-os
    domain that wishes to use a vTPM of its own.

 * vtpmmgr-stubdom:
    A mini-os domain that implements the vTPM manager. There is only
    one vTPM manager and it should be running during the entire lifetime
    of the machine. vtpmmgr domain securely stores encryption keys for
    each of the vtpms and accesses to the hardware TPM to get the root of
    trust for the entire system.

 * mini-os/tpm_tis:
    Mini-os TPM version 1.2 TPM Interface Specification (TIS) driver.
    This driver used by vtpmmgr-stubdom to talk directly to the hardware
    TPM. Communication is facilitated by mapping hardware memory pages
    into vtpmmgr stubdom.

 * Hardware TPM: The physical TPM 1.2 that is soldered onto the motherboard.

========================
    *BUILD & TEST*
========================
The following steps are how to build and test it:

1. SeaBios with my patch against upstream seabios is not submitted. I will
submit seabios patch later. Now I archive my seabios patch against upstream
seabios in Github: https://github.com/virt2x/seabios2 , try to build it for
test.

Configure it with Xen,
--- <Xen> Config.mk
    -SEABIOS_UPSTREAM_URL ?= git://xenbits.xen.org/seabios.git
    +SEABIOS_UPSTREAM_URL ?= https://github.com/virt2x/seabios2
    [...]
    -SEABIOS_UPSTREAM_REVISION ?= rel-1.7.5
    +SEABIOS_UPSTREAM_REVISION ?= ea94c083cc15875f46f0bf288b6531154b866f5a

2. QEMU with my patch against upstream QEMU is
    '[PATCH v3 0/5] QEMU:Xen stubdom vTPM for HVM virtual machine'.
I archive my QEMU patch series again Upstream QEMU in github:
    https://github.com/virt2x/qemu-xen-unstable2

Configure it with Xen,
--- <Xen> Config.mk

    -QEMU_UPSTREAM_URL ?= git://xenbits.xen.org/qemu-upstream-unstable.git
    +QEMU_UPSTREAM_URL ?= https://github.com/virt2x/qemu-xen-unstable2
    -QEMU_UPSTREAM_REVISION ?= qemu-xen-4.5.0-rc1
    +QEMU_UPSTREAM_REVISION ?= 9acbbde6c0ecb2b142998ff79af17ce69d0e190c

3. build/install Xen
Change QEMU_STUBDOM_VTPM option from 'n' to 'y'
    QEMU_STUBDOM_VTPM ?= y

./configure --prefix=/usr
make dist
make install

4. try to launch vtpmmgr / vtpm domain via <Xen>/docs/misc/vtpm-platforms.txt.
The reader is assumed to have familiarity with building and installing Xen, Linux,
and a basic understanding of the TPM and vTPM concepts.

The Linux / Windows HVM guest configuration file needs to be modified to include the
following line:

    [..]
    vtpm=["backend=domu-vtpm"]
    device_model_version = 'qemu-xen'
    acpi = 1
    [..]

#(domu-vtpm is the name vtpm domain, A mini-os stub domain that implements a vTPM)

5. enable native TPM 1.2 drvier in HVM virtual machine. for example enable tpm_tis.ko
in Linux HVM virtual machine.
If you have trousers and tpm_tools installed on the guest, the tpm_version command should
return the following:

The version command should return the following:
  TPM 1.2 Version Info:
  Chip Version:        1.2.0.7
  Spec Level:          2
  Errata Revision:     1
  TPM Vendor ID:       ETHZ
  TPM Version:         01010000
  Manufacturer Info:   4554485a

Or check it with sysfs, /sys/class/misc/tpm0

--Changes in v2:
  -Delete HVM_PARAM_STUBDOM_VTPM parameter, QEMU Reads Xen vTPM status via XenStore.


--Changes in v3:
-Code style enhancement
-Remove the redundant parameter 'num_vtpms' in libxl_domain_build_info{}
-Change hardcode vtpm ID from xenvtpm0 to xenvtpm%{DomainID}
-Move libxl__device_hvm_vtpm_add from libxl__spawn_local_dm() to domcreate_launch_dm()
-Redesign vtpm xensotre for HVM virtual machine
-Delete the domain type reading for domid
-refactor libxl__device_vtpm_add()
-rename libxl__device_hvm_vtpm_add() to libxl__device_vtpm_add_hvm()

Quan Xu (7):
  vTPM: event channel bind interdomain with para/hvm virtual machine
  vTPM: limit libxl__add_vtpms() function to para virtual machine
  vTPM: add TPM TCPA and SSDT for HVM virtual machine when vTPM is added
  vTPM: add vTPM device for HVM virtual machine
  vTPM: Delete the xenstore directory of frontend device
  vTPM: Parse envent string from QEMU frontend
  vTPM: add QEMU_STUBDOM_VTPM compile option

 Config.mk                             |   4 +
 extras/mini-os/tpmback.c              |  46 +++++----
 tools/Makefile                        |   7 ++
 tools/firmware/hvmloader/acpi/build.c |   7 +-
 tools/libxl/libxl.c                   | 176 ++++++++++++++++++++++++++++++++--
 tools/libxl/libxl.h                   |   7 +-
 tools/libxl/libxl_create.c            |  30 +++++-
 tools/libxl/libxl_device.c            |  99 ++++++++++++++++++-
 tools/libxl/libxl_dm.c                |  12 +++
 tools/libxl/libxl_internal.h          |   6 +-
 tools/libxl/xl_cmdimpl.c              |   4 +-
 11 files changed, 359 insertions(+), 39 deletions(-)

-- 
1.8.3.2

[PATCH v3 1/7] vTPM: event channel bind interdomain with para/hvm virtual machine

[Quan Xu]

Signed-off-by: Quan Xu <quan.xu@intel.com>
---
 extras/mini-os/tpmback.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/extras/mini-os/tpmback.c b/extras/mini-os/tpmback.c
index 00b66e8..8a0a983 100644
--- a/extras/mini-os/tpmback.c
+++ b/extras/mini-os/tpmback.c
@@ -608,18 +608,21 @@ int connect_fe(tpmif_t* tpmif)
    }
    free(value);
 
-   domid = tpmif->domid;
-   if((tpmif->page = gntmap_map_grant_refs(&gtpmdev.map, 1, &domid, 0, &ringref, PROT_READ | PROT_WRITE)) == NULL) {
-      TPMBACK_ERR("Failed to map grant reference %u/%u\n", (unsigned int) tpmif->domid, tpmif->handle);
-      return -1;
-   }
+    domid = (unsigned int)tpmif->domid;
+    if ((tpmif->page = gntmap_map_grant_refs(&gtpmdev.map, 1, &domid, 0, &ringref,
+                                             PROT_READ | PROT_WRITE)) == NULL) {
+        TPMBACK_ERR("Failed to map grant reference %u/%u\n",
+                    tpmif->domid, tpmif->handle);
+        return -1;
+    }
+
+    /* Bind the event channel */
+    if ((evtchn_bind_interdomain(domid, evtchn, tpmback_handler, tpmif, &tpmif->evtchn))) {
+        TPMBACK_ERR("%u/%u Unable to bind to interdomain event channel!\n",
+                    (unsigned int) tpmif->domid, tpmif->handle);
+        goto error_post_map;
+    }
 
-   /*Bind the event channel */
-   if((evtchn_bind_interdomain(tpmif->domid, evtchn, tpmback_handler, tpmif, &tpmif->evtchn)))
-   {
-      TPMBACK_ERR("%u/%u Unable to bind to interdomain event channel!\n", (unsigned int) tpmif->domid, tpmif->handle);
-      goto error_post_map;
-   }
    unmask_evtchn(tpmif->evtchn);
 
    /* Write the ready flag and change status to connected */
-- 
1.8.3.2

[PATCH v3 2/7] vTPM: limit libxl__add_vtpms() function to para virtual machine

[Quan Xu]

Signed-off-by: Quan Xu <quan.xu@intel.com>
---
 tools/libxl/libxl_create.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index b1ff5ae..66877b3 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1358,8 +1358,15 @@ static void domcreate_attach_vtpms(libxl__egc *egc,
        goto error_out;
    }
 
-    /* Plug vtpm devices */
-   if (d_config->num_vtpms > 0) {
+    /*
+     * Plug vtpm devices only for PV guest. The xenstore directory is very
+     * different for PV guest and HVM guest, but it is still call it for
+     * creating HVM guest, and xl should create xenstore directory before
+     * spawning QEMU. So try to make it only for PV guest.
+     */
+    if (d_config->num_vtpms > 0 &&
+        d_config->b_info.type == LIBXL_DOMAIN_TYPE_PV) {
+
        /* Attach vtpms */
        libxl__multidev_begin(ao, &dcs->multidev);
        dcs->multidev.callback = domcreate_attach_pci;
-- 
1.8.3.2

[PATCH v3 3/7] vTPM: add TPM TCPA and SSDT for HVM virtual machine when vTPM is added

[Quan Xu]

Signed-off-by: Quan Xu <quan.xu@intel.com>
---
 tools/firmware/hvmloader/acpi/build.c | 7 ++++---
 tools/libxl/libxl_create.c            | 5 ++++-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/firmware/hvmloader/acpi/build.c b/tools/firmware/hvmloader/acpi/build.c
index 1431296..49f6772 100644
--- a/tools/firmware/hvmloader/acpi/build.c
+++ b/tools/firmware/hvmloader/acpi/build.c
@@ -313,9 +313,10 @@ static int construct_secondary_tables(unsigned long *table_ptrs,
 
     /* TPM TCPA and SSDT. */
     tis_hdr = (uint16_t *)0xFED40F00;
-    if ( (tis_hdr[0] == tis_signature[0]) &&
-         (tis_hdr[1] == tis_signature[1]) &&
-         (tis_hdr[2] == tis_signature[2]) )
+    if (((tis_hdr[0] == tis_signature[0]) &&
+        (tis_hdr[1] == tis_signature[1]) &&
+        (tis_hdr[2] == tis_signature[2])) ||
+        !strncmp(xenstore_read("platform/acpi_stubdom_vtpm", "1"), "1", 1))
     {
         ssdt = mem_alloc(sizeof(ssdt_tpm), 16);
         if (!ssdt) return -1;
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 66877b3..ffb124a 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -432,7 +432,7 @@ int libxl__domain_build(libxl__gc *gc,
         vments[4] = "start_time";
         vments[5] = libxl__sprintf(gc, "%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000);
 
-        localents = libxl__calloc(gc, 9, sizeof(char *));
+        localents = libxl__calloc(gc, 11, sizeof(char *));
         i = 0;
         localents[i++] = "platform/acpi";
         localents[i++] = libxl_defbool_val(info->u.hvm.acpi) ? "1" : "0";
@@ -440,6 +440,9 @@ int libxl__domain_build(libxl__gc *gc,
         localents[i++] = libxl_defbool_val(info->u.hvm.acpi_s3) ? "1" : "0";
         localents[i++] = "platform/acpi_s4";
         localents[i++] = libxl_defbool_val(info->u.hvm.acpi_s4) ? "1" : "0";
+        localents[i++] = "platform/acpi_stubdom_vtpm";
+        localents[i++] = (d_config->num_vtpms > 0) ? "1" : "0";
+
         if (info->u.hvm.mmio_hole_memkb) {
             uint64_t max_ram_below_4g =
                 (1ULL << 32) - (info->u.hvm.mmio_hole_memkb << 10);
-- 
1.8.3.2

[PATCH v3 4/7] vTPM: add vTPM device for HVM virtual machine

[Quan Xu]

refactor libxl__device_vtpm_add to call the right helpers
libxl__device_vtpm_add_{pv,hvm}. For HVM virtual machine,
it does not support hot-plug and hot-unplug, as it requires
SeaBios to initalize ACPI and virtual MMIO space for TPM
TIS when virtual machine starts.

Signed-off-by: Quan Xu <quan.xu@intel.com>
---
 tools/libxl/libxl.c          | 176 +++++++++++++++++++++++++++++++++++++++++--
 tools/libxl/libxl.h          |   7 +-
 tools/libxl/libxl_create.c   |  32 +++++---
 tools/libxl/libxl_device.c   |  38 +++++++++-
 tools/libxl/libxl_dm.c       |  12 +++
 tools/libxl/libxl_internal.h |   6 +-
 tools/libxl/xl_cmdimpl.c     |   4 +-
 7 files changed, 253 insertions(+), 22 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 18561fb..c2d4baa 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1904,6 +1904,25 @@ out:
     return;
 }
 
+static int libxl__frontend_device_nextid(libxl__gc *gc, uint32_t domid, char *device)
+{
+    char *dompath, **l;
+    unsigned int nb;
+    int nextid = -1;
+
+    if (!(dompath = libxl__xs_get_dompath(gc, domid)))
+        return nextid;
+
+    l = libxl__xs_directory(gc, XBT_NULL,
+            GCSPRINTF("/local/domain/0/frontend/%s/%u", device, domid), &nb);
+    if (l == NULL || nb == 0)
+        nextid = 0;
+    else
+        nextid = strtoul(l[nb - 1], NULL, 10) + 1;
+
+    return nextid;
+}
+
 /* common function to get next device id */
 static int libxl__device_nextid(libxl__gc *gc, uint32_t domid, char *device)
 {
@@ -1957,9 +1976,9 @@ static int libxl__device_from_vtpm(libxl__gc *gc, uint32_t domid,
    return 0;
 }
 
-void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
-                           libxl_device_vtpm *vtpm,
-                           libxl__ao_device *aodev)
+void libxl__device_vtpm_add_pv(libxl__egc *egc, uint32_t domid,
+                               libxl_device_vtpm *vtpm,
+                               libxl__ao_device *aodev)
 {
     STATE_AO_GC(aodev->ao);
     flexarray_t *front;
@@ -2073,6 +2092,134 @@ out:
     return;
 }
 
+void libxl__device_vtpm_add_hvm(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;
+    unsigned int rc;
+    xs_transaction_t t = XBT_NULL;
+    libxl_domain_config d_config;
+    libxl_device_vtpm vtpm_saved;
+    libxl__domain_userdata_lock *lock = NULL;
+
+    libxl_domain_config_init(&d_config);
+    libxl_device_vtpm_init(&vtpm_saved);
+    libxl_device_vtpm_copy(CTX, &vtpm_saved, vtpm);
+
+    rc = libxl__device_vtpm_setdefault(gc, vtpm);
+    if (rc)
+        goto out;
+
+    front = flexarray_make(gc, 16, 1);
+    back = flexarray_make(gc, 16, 1);
+
+    if ((vtpm->devid = libxl__frontend_device_nextid(gc,
+                           vtpm->backend_domid, "vtpm")) < 0) {
+        rc =  ERROR_FAIL;
+        goto out;
+    }
+
+    GCNEW(device);
+    rc = libxl__device_from_vtpm(gc, 0, vtpm, device);
+    if (rc != 0)
+        goto out;
+    flexarray_append(back, "frontend-id");
+    flexarray_append(back, "0");
+    flexarray_append(back, "online");
+    flexarray_append(back, "1");
+    flexarray_append(back, "state");
+    flexarray_append(back, "1");
+    flexarray_append(back, "handle");
+    flexarray_append(back, GCSPRINTF("%d", vtpm->devid));
+
+    flexarray_append(back, "uuid");
+    flexarray_append(back, GCSPRINTF(LIBXL_UUID_FMT,
+                     LIBXL_UUID_BYTES(vtpm->uuid)));
+    flexarray_append(back, "resume");
+    flexarray_append(back, "False");
+
+    flexarray_append(front, "backend-id");
+    flexarray_append(front, GCSPRINTF("%d", vtpm->backend_domid));
+    flexarray_append(front, "state");
+    flexarray_append(front, "1");
+    flexarray_append(front, "handle");
+    flexarray_append(front, GCSPRINTF("%d", vtpm->devid));
+
+    flexarray_append(front, "domain");
+    flexarray_append(front, GCSPRINTF("%s", libxl__domid_to_name(gc, domid)));
+
+    if (aodev->update_json) {
+        lock = libxl__lock_domain_userdata(gc, domid);
+        if (!lock) {
+            rc = ERROR_LOCK_FAIL;
+            goto out;
+        }
+
+        rc = libxl__get_domain_configuration(gc, domid, &d_config);
+        if (rc)
+            goto out;
+
+        DEVICE_ADD(vtpm, vtpms, domid, &vtpm_saved, COMPARE_DEVID, &d_config);
+    }
+
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &t);
+        if (rc)
+            goto out;
+
+        rc = libxl__device_exists(gc, t, device);
+        if (rc < 0)
+            goto out;
+        if (rc == 1) {
+
+            /* already exists in xenstore */
+            LOG(ERROR, "device already exists in xenstore");
+            aodev->action = LIBXL__DEVICE_ACTION_ADD; /* for error message */
+            rc = ERROR_DEVICE_EXISTS;
+            goto out;
+        }
+
+        if (aodev->update_json) {
+            rc = libxl__set_domain_configuration(gc, domid, &d_config);
+            if (rc)
+                goto out;
+        }
+
+        libxl__device_generic_add(gc, t, device,
+                                  libxl__xs_kvs_of_flexarray(gc, back,
+                                                             back->count),
+                                  libxl__xs_kvs_of_flexarray(gc, front,
+                                                             front->count),
+                                  NULL);
+
+        rc = libxl__xs_transaction_commit(gc, &t);
+        if (!rc)
+            break;
+        if (rc < 0)
+            goto out;
+    }
+
+    aodev->dev = device;
+    aodev->action = LIBXL__DEVICE_ACTION_ADD;
+    libxl__wait_device_connection(egc, aodev);
+    rc = 0;
+
+out:
+    libxl__xs_transaction_abort(gc, &t);
+    if (lock)
+        libxl__unlock_domain_userdata(lock);
+    libxl_device_vtpm_dispose(&vtpm_saved);
+    libxl_domain_config_dispose(&d_config);
+    aodev->rc = rc;
+    if (rc)
+        aodev->callback(egc, aodev);
+    return;
+}
+
 libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t domid, int *num)
 {
     GC_INIT(ctx);
@@ -4179,11 +4326,28 @@ DEFINE_DEVICE_ADD(disk)
 /* nic */
 DEFINE_DEVICE_ADD(nic)
 
-/* vtpm */
-DEFINE_DEVICE_ADD(vtpm)
-
 #undef DEFINE_DEVICE_ADD
 
+#define DEFINE_VTPM_ADD_PV(type)                                        \
+    int libxl_device_##type##_add_pv(libxl_ctx *ctx,                    \
+        uint32_t domid, libxl_device_##type *type,                      \
+        const libxl_asyncop_how *ao_how)                                \
+    {                                                                   \
+        AO_CREATE(ctx, domid, ao_how);                                  \
+        libxl__ao_device *aodev;                                        \
+                                                                        \
+        GCNEW(aodev);                                                   \
+        libxl__prepare_ao_device(ao, aodev);                            \
+        aodev->callback = device_addrm_aocomplete;                      \
+        aodev->update_json = true;                                      \
+        libxl__device_##type##_add_pv(egc, domid, type, aodev);         \
+                                                                        \
+        return AO_INPROGRESS;                                           \
+    }
+
+/* vtpm */
+DEFINE_VTPM_ADD_PV(vtpm)
+
 /******************************************************************************/
 
 /*
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index c3451bd..38d3542 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1183,9 +1183,10 @@ int libxl_device_channel_getinfo(libxl_ctx *ctx, uint32_t domid,
                                  libxl_channelinfo *channelinfo);
 
 /* 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_add_pv(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)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index ffb124a..b88e1cb 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -901,6 +901,14 @@ static void initiate_domain_create(libxl__egc *egc,
             d_config->nics[i].devid = ++last_devid;
     }
 
+    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM) {
+        for (i = 0; i < d_config->num_vtpms; i++) {
+            ret = libxl__device_vtpm_setdefault(gc, &d_config->vtpms[i]);
+            if (ret)
+                goto error_out;
+        }
+    }
+
     if (restore_fd >= 0) {
         LOG(DEBUG, "restoring, not running bootloader");
         domcreate_bootloader_done(egc, &dcs->bl, 0);
@@ -1244,6 +1252,13 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
         libxl__device_vkb_add(gc, domid, &vkb);
         libxl_device_vkb_dispose(&vkb);
 
+        /*
+         * Plug vtpm devices. dcs->multidev.callback is NULL, no need to call
+         * libxl__multidev_prepared().
+         */
+        libxl__multidev_begin(ao, &dcs->multidev);
+        libxl__add_vtpms(egc, ao, domid, d_config, &dcs->multidev);
+
         dcs->dmss.dm.guest_domid = domid;
         if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
             libxl__spawn_stub_dm(egc, &dcs->dmss);
@@ -1361,15 +1376,14 @@ static void domcreate_attach_vtpms(libxl__egc *egc,
        goto error_out;
    }
 
-    /*
-     * Plug vtpm devices only for PV guest. The xenstore directory is very
-     * different for PV guest and HVM guest, but it is still call it for
-     * creating HVM guest, and xl should create xenstore directory before
-     * spawning QEMU. So try to make it only for PV guest.
-     */
-    if (d_config->num_vtpms > 0 &&
-        d_config->b_info.type == LIBXL_DOMAIN_TYPE_PV) {
-
+   /*
+    * Plug vtpm devices only for PV guest. The xenstore directory is very
+    * different for PV guest and HVM guest, but it is still call it for
+    * creating HVM guest, and xl should create xenstore directory before
+    * spawning QEMU. So try to make it only for PV guest.
+    */
+   if (d_config->num_vtpms > 0 &&
+       d_config->b_info.type == LIBXL_DOMAIN_TYPE_PV) {
        /* Attach vtpms */
        libxl__multidev_begin(ao, &dcs->multidev);
        dcs->multidev.callback = domcreate_attach_pci;
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 4b51ded..b1a71fe 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -26,6 +26,13 @@ char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device)
     if (device->kind == LIBXL__DEVICE_KIND_CONSOLE && device->devid == 0)
         return GCSPRINTF("%s/console", dom_path);
 
+    /* vTPM for HVM virtual machine is a special case */
+    else if (device->backend_kind == LIBXL__DEVICE_KIND_VTPM &&
+             device->domid == 0)
+        return GCSPRINTF("/local/domain/0/frontend/%s/%u/%d",
+            libxl__device_kind_to_string(device->backend_kind),
+            device->backend_domid, device->devid);
+
     return GCSPRINTF("%s/device/%s/%d", dom_path,
                      libxl__device_kind_to_string(device->kind),
                      device->devid);
@@ -560,10 +567,39 @@ void libxl__multidev_prepared(libxl__egc *egc,
 
 DEFINE_DEVICES_ADD(disk)
 DEFINE_DEVICES_ADD(nic)
-DEFINE_DEVICES_ADD(vtpm)
 
 #undef DEFINE_DEVICES_ADD
 
+#define DEFINE_VTPM_ADD(_type)                                          \
+    void libxl__add_##_type##s(libxl__egc *egc, libxl__ao *ao,          \
+                               uint32_t domid,                          \
+                               libxl_domain_config *d_config,           \
+                               libxl__multidev *multidev)               \
+    {                                                                   \
+        AO_GC;                                                          \
+        int i;                                                          \
+        for (i = 0; i < d_config->num_##_type##s; i++) {                \
+            libxl__ao_device *aodev = libxl__multidev_prepare(multidev); \
+            switch(d_config->c_info.type) {                             \
+            case LIBXL_DOMAIN_TYPE_PV:                                  \
+                libxl__device_##_type##_add_pv(egc, domid,              \
+                                               &d_config->_type##s[i],  \
+                                               aodev);                  \
+                break;                                                  \
+            case LIBXL_DOMAIN_TYPE_HVM:                                 \
+                libxl__device_##_type##_add_hvm(egc, domid,             \
+                                               &d_config->_type##s[i],  \
+                                               aodev);                  \
+                break;                                                  \
+            default:                                                    \
+                break;                                                  \
+            }                                                           \
+        }                                                               \
+    }
+
+DEFINE_VTPM_ADD(vtpm)
+
+#undef DEFINE_VTPM_ADD
 /******************************************************************************/
 
 int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3e191c3..183910c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -414,6 +414,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
     const libxl_device_nic *nics = guest_config->nics;
     const int num_disks = guest_config->num_disks;
     const int num_nics = guest_config->num_nics;
+    const int num_vtpms = guest_config->num_vtpms;
     const libxl_vnc_info *vnc = libxl__dm_vnc(guest_config);
     const libxl_sdl_info *sdl = dm_sdl(guest_config);
     const char *keymap = dm_keymap(guest_config);
@@ -747,6 +748,17 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
         abort();
     }
 
+    /* Add vTPM parameters for HVM virtual machine */
+    if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
+        num_vtpms > 0) {
+        flexarray_vappend(dm_args, "-tpmdev",
+                          libxl__sprintf(gc, "xenstubdoms,id=xenvtpm%d",
+                                         guest_domid), NULL);
+        flexarray_vappend(dm_args,"-device",
+                          libxl__sprintf(gc, "tpm-tis,tpmdev=xenvtpm%d",
+                                         guest_domid), NULL);
+    }
+
     ram_size = libxl__sizekb_to_mb(b_info->max_memkb - b_info->video_memkb);
     flexarray_append(dm_args, "-m");
     flexarray_append(dm_args, libxl__sprintf(gc, "%"PRId64, ram_size));
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 4361421..c6c82dd 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2384,10 +2384,14 @@ _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,
+_hidden void libxl__device_vtpm_add_pv(libxl__egc *egc, uint32_t domid,
                                    libxl_device_vtpm *vtpm,
                                    libxl__ao_device *aodev);
 
+void libxl__device_vtpm_add_hvm(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);
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 3c9f146..e01d341 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -6490,8 +6490,8 @@ int main_vtpmattach(int argc, char **argv)
        return 0;
     }
 
-    if (libxl_device_vtpm_add(ctx, domid, &vtpm, 0)) {
-        fprintf(stderr, "libxl_device_vtpm_add failed.\n");
+    if (libxl_device_vtpm_add_pv(ctx, domid, &vtpm, 0)) {
+        fprintf(stderr, "libxl_device_vtpm_add_pv failed.\n");
         return 1;
     }
     libxl_device_vtpm_dispose(&vtpm);
-- 
1.8.3.2

[PATCH v3 5/7] vTPM: Delete the xenstore directory of frontend device

[Quan Xu]

when virtual machine is destroyed.

Signed-off-by: Quan Xu <quan.xu@intel.com>
---
 tools/libxl/libxl_device.c | 61 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index b1a71fe..668bf71 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -660,10 +660,11 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
 {
     STATE_AO_GC(drs->ao);
     uint32_t domid = drs->domid;
-    char *path;
-    unsigned int num_kinds, num_dev_xsentries;
-    char **kinds = NULL, **devs = NULL;
-    int i, j, rc = 0;
+    char *path, *dom_name, *name;
+    unsigned int num_kinds, num_fkinds, num_dev_xsentries, num_dev;
+    char **kinds = NULL, **fkinds = NULL, **devs = NULL, **sdevs = NULL,
+        **be_doms = NULL;
+    int i, j, k, rc = 0;
     libxl__device *dev;
     libxl__multidev *multidev = &drs->multidev;
     libxl__ao_device *aodev;
@@ -731,6 +732,58 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
         libxl__device_destroy(gc, dev);
     }
 
+    /*
+     * Frontend device, such as vTPM, is under:
+     * /local/domain/0/frontend/{type}/{backend_dom_id}/{dev}
+     */
+    path = GCSPRINTF("/local/domain/%d/frontend", 0);
+    fkinds = libxl__xs_directory(gc, XBT_NULL, path, &num_fkinds);
+    if (!fkinds) {
+        if (errno != ENOENT) {
+            LOGE(ERROR, "unable to get xenstore device listing %s", path);
+            goto out;
+        }
+        num_fkinds = 0;
+    }
+
+    name = libxl_domid_to_name(CTX, domid);
+
+    /* /local/domain/0/frontend/{type} */
+    for (i = 0; i < num_fkinds; i++) {
+        if (libxl__device_kind_from_string(fkinds[i], &kind))
+            continue;
+
+        path = GCSPRINTF("/local/domain/0/frontend/%s", fkinds[i]);
+        be_doms = libxl__xs_directory(gc, XBT_NULL, path, &num_dev_xsentries);
+        if (!be_doms)
+            continue;
+
+        /* /local/domain/0/frontend/{type}/{backend_dom_id} */
+        for (j = 0; j < num_dev_xsentries; j++) {
+            path = GCSPRINTF("/local/domain/0/frontend/%s/%d",
+                              fkinds[i], atoi(be_doms[j]));
+            sdevs = libxl__xs_directory(gc, XBT_NULL, path, &num_dev);
+
+            /* /local/domain/0/frontend/{type}/{backend_dom_id}/{dev} */
+            for (k = 0; k < num_dev; k++) {
+                path = GCSPRINTF("/local/domain/0/frontend/%s/%d/%d/domain",
+                                 fkinds[i], atoi(be_doms[j]), atoi(sdevs[k]));
+                dom_name = libxl__xs_read(gc, XBT_NULL, path);
+                if (strcmp(name, dom_name)) {
+                    continue;
+                }
+
+                path = GCSPRINTF("/local/domain/0/frontend/%s/%d/%d/backend",
+                                 fkinds[i], atoi(be_doms[j]), atoi(sdevs[k]));
+                path = libxl__xs_read(gc, XBT_NULL, path);
+                GCNEW(dev);
+                if (path && strcmp(path, "") &&
+                    libxl__parse_backend_path(gc, path, dev) == 0)
+                    libxl__device_destroy(gc, dev);
+            }
+        }
+    }
+
 out:
     libxl__multidev_prepared(egc, multidev, rc);
 }
-- 
1.8.3.2

[PATCH v3 6/7] vTPM: Parse envent string from QEMU frontend

[Quan Xu]

Signed-off-by: Quan Xu <quan.xu@intel.com>
---
 extras/mini-os/tpmback.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/extras/mini-os/tpmback.c b/extras/mini-os/tpmback.c
index 8a0a983..b8f4c8f 100644
--- a/extras/mini-os/tpmback.c
+++ b/extras/mini-os/tpmback.c
@@ -732,11 +732,22 @@ static int parse_eventstr(const char* evstr, domid_t* domid, unsigned int* handl
 	 return EV_NONE;
       }
       return EV_NEWFE;
-   } else if((ret = sscanf(evstr, "/local/domain/%u/device/vtpm/%u/%40s", &udomid, handle, cmd)) == 3) {
-      *domid = udomid;
-      if (!strcmp(cmd, "state"))
-	 return EV_STCHNG;
-   }
+
+    /* vtpm and PV virtual machines */
+    } else if((ret = sscanf(evstr, "/local/domain/%u/device/vtpm/%u/%40s",
+                            &udomid, handle, cmd)) == 3) {
+        *domid = udomid;
+        if (!strcmp(cmd, "state"))
+            return EV_STCHNG;
+
+    /* HVM virtual machines */
+    } else if((ret = sscanf(evstr, "/local/domain/0/frontend/vtpm/%u/%u/%40s",
+                            &udomid, handle, cmd)) == 3) {
+        *domid = 0;
+        if (!strcmp(cmd, "state"))
+            return EV_STCHNG;
+    }
+
    return EV_NONE;
 }
 
-- 
1.8.3.2

[PATCH v3 7/7] vTPM: add QEMU_STUBDOM_VTPM compile option

[Quan Xu]

Signed-off-by: Quan Xu <quan.xu@intel.com>
---
 Config.mk      | 4 ++++
 tools/Makefile | 7 +++++++
 2 files changed, 11 insertions(+)

diff --git a/Config.mk b/Config.mk
index a5b6c41..5a5f413 100644
--- a/Config.mk
+++ b/Config.mk
@@ -254,6 +254,10 @@ endif
 OVMF_UPSTREAM_REVISION ?= 447d264115c476142f884af0be287622cd244423
 QEMU_UPSTREAM_REVISION ?= qemu-xen-4.5.0-rc1
 SEABIOS_UPSTREAM_REVISION ?= rel-1.7.5
+
+# Qemu stubdom vtpm frontend.
+QEMU_STUBDOM_VTPM ?= n
+
 # Thu May 22 16:59:16 2014 -0400
 # python3 fixes for vgabios and csm builds.
 
diff --git a/tools/Makefile b/tools/Makefile
index af9798a..1044149 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -197,6 +197,12 @@ else
 QEMU_XEN_ENABLE_DEBUG :=
 endif
 
+ifeq ($(QEMU_STUBDOM_VTPM),y)
+QEMU_TPM_ARGS="--enable-tpm"
+else
+QEMU_TPM_ARGS="--disable-tpm"
+endif
+
 subdir-all-qemu-xen-dir: qemu-xen-dir-find
 	if test -d $(QEMU_UPSTREAM_LOC) ; then \
 		source=$(QEMU_UPSTREAM_LOC); \
@@ -222,6 +228,7 @@ subdir-all-qemu-xen-dir: qemu-xen-dir-find
 		--datadir=$(SHAREDIR)/qemu-xen \
 		--localstatedir=$(localstatedir) \
 		--disable-kvm \
+                $(QEMU_TPM_ARGS) \
 		--disable-docs \
 		--disable-guest-agent \
 		--python=$(PYTHON) \
-- 
1.8.3.2

Re: [PATCH v3 1/7] vTPM: event channel bind interdomain with para/hvm virtual machine

[Wei Liu]

On Tue, Mar 10, 2015 at 08:13:56AM -0400, Quan Xu wrote:
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> ---
>  extras/mini-os/tpmback.c | 25 ++++++++++++++-----------

FYI mini-os now lives in a separate tree.

http://xenbits.xen.org/gitweb/?p=mini-os.git;a=summary


>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/extras/mini-os/tpmback.c b/extras/mini-os/tpmback.c
> index 00b66e8..8a0a983 100644
> --- a/extras/mini-os/tpmback.c
> +++ b/extras/mini-os/tpmback.c
> @@ -608,18 +608,21 @@ int connect_fe(tpmif_t* tpmif)
>     }
>     free(value);
>  
> -   domid = tpmif->domid;
> -   if((tpmif->page = gntmap_map_grant_refs(&gtpmdev.map, 1, &domid, 0, &ringref, PROT_READ | PROT_WRITE)) == NULL) {
> -      TPMBACK_ERR("Failed to map grant reference %u/%u\n", (unsigned int) tpmif->domid, tpmif->handle);
> -      return -1;
> -   }
> +    domid = (unsigned int)tpmif->domid;
> +    if ((tpmif->page = gntmap_map_grant_refs(&gtpmdev.map, 1, &domid, 0, &ringref,
> +                                             PROT_READ | PROT_WRITE)) == NULL) {
> +        TPMBACK_ERR("Failed to map grant reference %u/%u\n",
> +                    tpmif->domid, tpmif->handle);
> +        return -1;
> +    }
> +
> +    /* Bind the event channel */
> +    if ((evtchn_bind_interdomain(domid, evtchn, tpmback_handler, tpmif, &tpmif->evtchn))) {
> +        TPMBACK_ERR("%u/%u Unable to bind to interdomain event channel!\n",
> +                    (unsigned int) tpmif->domid, tpmif->handle);
> +        goto error_post_map;
> +    }
>  
> -   /*Bind the event channel */
> -   if((evtchn_bind_interdomain(tpmif->domid, evtchn, tpmback_handler, tpmif, &tpmif->evtchn)))
> -   {
> -      TPMBACK_ERR("%u/%u Unable to bind to interdomain event channel!\n", (unsigned int) tpmif->domid, tpmif->handle);
> -      goto error_post_map;
> -   }
>     unmask_evtchn(tpmif->evtchn);
>  
>     /* Write the ready flag and change status to connected */
> -- 
> 1.8.3.2
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v3 1/7] vTPM: event channel bind interdomain with para/hvm virtual machine

[Xu, Quan]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org
> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Wei Liu
> Sent: Friday, March 13, 2015 10:03 PM
> To: Xu, Quan
> Cc: wei.liu2@citrix.com; keir@xen.org; ian.campbell@citrix.com;
> stefano.stabellini@eu.citrix.com; tim@xen.org; stefanb@linux.vnet.ibm.com;
> ian.jackson@eu.citrix.com; xen-devel@lists.xen.org; jbeulich@suse.com;
> samuel.thibault@ens-lyon.org; dgdegra@tycho.nsa.gov
> Subject: Re: [Xen-devel] [PATCH v3 1/7] vTPM: event channel bind interdomain
> with para/hvm virtual machine
> 
> On Tue, Mar 10, 2015 at 08:13:56AM -0400, Quan Xu wrote:
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> > ---
> >  extras/mini-os/tpmback.c | 25 ++++++++++++++-----------
> 
> FYI mini-os now lives in a separate tree.
> 
> http://xenbits.xen.org/gitweb/?p=mini-os.git;a=summary
> 

Thanks, I will send out the next version with this separate tree.

Quan

> 
> >  1 file changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/extras/mini-os/tpmback.c b/extras/mini-os/tpmback.c index
> > 00b66e8..8a0a983 100644
> > --- a/extras/mini-os/tpmback.c
> > +++ b/extras/mini-os/tpmback.c
> > @@ -608,18 +608,21 @@ int connect_fe(tpmif_t* tpmif)
> >     }
> >     free(value);
> >
> > -   domid = tpmif->domid;
> > -   if((tpmif->page = gntmap_map_grant_refs(&gtpmdev.map, 1, &domid, 0,
> &ringref, PROT_READ | PROT_WRITE)) == NULL) {
> > -      TPMBACK_ERR("Failed to map grant reference %u/%u\n", (unsigned
> int) tpmif->domid, tpmif->handle);
> > -      return -1;
> > -   }
> > +    domid = (unsigned int)tpmif->domid;
> > +    if ((tpmif->page = gntmap_map_grant_refs(&gtpmdev.map, 1, &domid,
> 0, &ringref,
> > +                                             PROT_READ |
> PROT_WRITE)) == NULL) {
> > +        TPMBACK_ERR("Failed to map grant reference %u/%u\n",
> > +                    tpmif->domid, tpmif->handle);
> > +        return -1;
> > +    }
> > +
> > +    /* Bind the event channel */
> > +    if ((evtchn_bind_interdomain(domid, evtchn, tpmback_handler, tpmif,
> &tpmif->evtchn))) {
> > +        TPMBACK_ERR("%u/%u Unable to bind to interdomain event
> channel!\n",
> > +                    (unsigned int) tpmif->domid, tpmif->handle);
> > +        goto error_post_map;
> > +    }
> >
> > -   /*Bind the event channel */
> > -   if((evtchn_bind_interdomain(tpmif->domid, evtchn, tpmback_handler,
> tpmif, &tpmif->evtchn)))
> > -   {
> > -      TPMBACK_ERR("%u/%u Unable to bind to interdomain event
> channel!\n", (unsigned int) tpmif->domid, tpmif->handle);
> > -      goto error_post_map;
> > -   }
> >     unmask_evtchn(tpmif->evtchn);
> >
> >     /* Write the ready flag and change status to connected */
> > --
> > 1.8.3.2
> >
> >
> > _______________________________________________
> > 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

Re: [PATCH v3 2/7] vTPM: limit libxl__add_vtpms() function to para virtual machine

[Wei Liu]

On Tue, Mar 10, 2015 at 08:13:57AM -0400, Quan Xu wrote:
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> ---
>  tools/libxl/libxl_create.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index b1ff5ae..66877b3 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -1358,8 +1358,15 @@ static void domcreate_attach_vtpms(libxl__egc *egc,
>         goto error_out;
>     }
>  
> -    /* Plug vtpm devices */
> -   if (d_config->num_vtpms > 0) {
> +    /*
> +     * Plug vtpm devices only for PV guest. The xenstore directory is very
> +     * different for PV guest and HVM guest, but it is still call it for
> +     * creating HVM guest, and xl should create xenstore directory before
> +     * spawning QEMU. So try to make it only for PV guest.
> +     */
> +    if (d_config->num_vtpms > 0 &&
> +        d_config->b_info.type == LIBXL_DOMAIN_TYPE_PV) {
> +

I'm not convinced that you can / should do this. This is a common entry
for both HVM and PV guest. If you end up removing this hunk in later
patch you need to rearrange your series to avoiding adding this hunk in
the first place.

Wei.

>         /* Attach vtpms */
>         libxl__multidev_begin(ao, &dcs->multidev);
>         dcs->multidev.callback = domcreate_attach_pci;
> -- 
> 1.8.3.2
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v3 3/7] vTPM: add TPM TCPA and SSDT for HVM virtual machine when vTPM is added

[Wei Liu]

On Tue, Mar 10, 2015 at 08:13:58AM -0400, Quan Xu wrote:
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> ---
>  tools/firmware/hvmloader/acpi/build.c | 7 ++++---
>  tools/libxl/libxl_create.c            | 5 ++++-
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/acpi/build.c b/tools/firmware/hvmloader/acpi/build.c
> index 1431296..49f6772 100644
> --- a/tools/firmware/hvmloader/acpi/build.c
> +++ b/tools/firmware/hvmloader/acpi/build.c
> @@ -313,9 +313,10 @@ static int construct_secondary_tables(unsigned long *table_ptrs,
>  
>      /* TPM TCPA and SSDT. */
>      tis_hdr = (uint16_t *)0xFED40F00;
> -    if ( (tis_hdr[0] == tis_signature[0]) &&
> -         (tis_hdr[1] == tis_signature[1]) &&
> -         (tis_hdr[2] == tis_signature[2]) )
> +    if (((tis_hdr[0] == tis_signature[0]) &&
> +        (tis_hdr[1] == tis_signature[1]) &&

These two lines are not necessary. They are not functional change and
violate coding style.

Wei.

Re: [PATCH v3 4/7] vTPM: add vTPM device for HVM virtual machine

[Wei Liu]

On Tue, Mar 10, 2015 at 08:13:59AM -0400, Quan Xu wrote:
> refactor libxl__device_vtpm_add to call the right helpers
> libxl__device_vtpm_add_{pv,hvm}. For HVM virtual machine,
> it does not support hot-plug and hot-unplug, as it requires
> SeaBios to initalize ACPI and virtual MMIO space for TPM
> TIS when virtual machine starts.
> 
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> ---
>  tools/libxl/libxl.c          | 176 +++++++++++++++++++++++++++++++++++++++++--
>  tools/libxl/libxl.h          |   7 +-
>  tools/libxl/libxl_create.c   |  32 +++++---
>  tools/libxl/libxl_device.c   |  38 +++++++++-
>  tools/libxl/libxl_dm.c       |  12 +++
>  tools/libxl/libxl_internal.h |   6 +-
>  tools/libxl/xl_cmdimpl.c     |   4 +-
>  7 files changed, 253 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 18561fb..c2d4baa 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1904,6 +1904,25 @@ out:
>      return;
>  }
>  
> +static int libxl__frontend_device_nextid(libxl__gc *gc, uint32_t domid, char *device)

Line too line.

> +{
> +    char *dompath, **l;
> +    unsigned int nb;
> +    int nextid = -1;
> +
> +    if (!(dompath = libxl__xs_get_dompath(gc, domid)))
> +        return nextid;
> +
> +    l = libxl__xs_directory(gc, XBT_NULL,
> +            GCSPRINTF("/local/domain/0/frontend/%s/%u", device, domid), &nb);

Please don't use hardcode /local/domain/0. And I suspect this will be
wrong mostly of the time since this function is not Dom0 only.

> +    if (l == NULL || nb == 0)
> +        nextid = 0;
> +    else
> +        nextid = strtoul(l[nb - 1], NULL, 10) + 1;
> +
> +    return nextid;
> +}

And this function looks very much like libxl__device_nextid. The only
difference is the xenstore path, right? Please factor out the common
bits of both function to a helper.

> +
>  /* common function to get next device id */
>  static int libxl__device_nextid(libxl__gc *gc, uint32_t domid, char *device)
>  {
> @@ -1957,9 +1976,9 @@ static int libxl__device_from_vtpm(libxl__gc *gc, uint32_t domid,
>     return 0;
>  }
>  
> -void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
> -                           libxl_device_vtpm *vtpm,
> -                           libxl__ao_device *aodev)
> +void libxl__device_vtpm_add_pv(libxl__egc *egc, uint32_t domid,
> +                               libxl_device_vtpm *vtpm,
> +                               libxl__ao_device *aodev)
>  {
>      STATE_AO_GC(aodev->ao);
>      flexarray_t *front;
> @@ -2073,6 +2092,134 @@ out:
>      return;
>  }
>  
> +void libxl__device_vtpm_add_hvm(libxl__egc *egc, uint32_t domid,
> +                                libxl_device_vtpm *vtpm,
> +                                libxl__ao_device *aodev)
> +{
[...]
> +    libxl_device_vtpm_dispose(&vtpm_saved);
> +    libxl_domain_config_dispose(&d_config);
> +    aodev->rc = rc;
> +    if (rc)
> +        aodev->callback(egc, aodev);
> +    return;
> +}
> +

This looks repetitive as well. I think the only difference between _pv
and _hvm is the xenstore nodes they write to. You need to do some
refactoring as well.

>  libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t domid, int *num)
>  {
>      GC_INIT(ctx);
> @@ -4179,11 +4326,28 @@ DEFINE_DEVICE_ADD(disk)
>  /* nic */
>  DEFINE_DEVICE_ADD(nic)
>  
> -/* vtpm */
> -DEFINE_DEVICE_ADD(vtpm)
> -
>  #undef DEFINE_DEVICE_ADD
>  
> +#define DEFINE_VTPM_ADD_PV(type)                                        \
> +    int libxl_device_##type##_add_pv(libxl_ctx *ctx,                    \
> +        uint32_t domid, libxl_device_##type *type,                      \
> +        const libxl_asyncop_how *ao_how)                                \
> +    {                                                                   \
> +        AO_CREATE(ctx, domid, ao_how);                                  \
> +        libxl__ao_device *aodev;                                        \
> +                                                                        \
> +        GCNEW(aodev);                                                   \
> +        libxl__prepare_ao_device(ao, aodev);                            \
> +        aodev->callback = device_addrm_aocomplete;                      \
> +        aodev->update_json = true;                                      \
> +        libxl__device_##type##_add_pv(egc, domid, type, aodev);         \
> +                                                                        \
> +        return AO_INPROGRESS;                                           \
> +    }
> +
> +/* vtpm */
> +DEFINE_VTPM_ADD_PV(vtpm)

No need to define a macro for this, since there is only one user of
this.  But after looking further down I think you're doing it wrong.

There should still be a unified libxl_device_vtpm_add function that
works for both PV and HVM (if this is the case because your change).

> +
>  /******************************************************************************/
>  
>  /*
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index c3451bd..38d3542 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1183,9 +1183,10 @@ int libxl_device_channel_getinfo(libxl_ctx *ctx, uint32_t domid,
>                                   libxl_channelinfo *channelinfo);
>  
>  /* 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_add_pv(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)
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index ffb124a..b88e1cb 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -901,6 +901,14 @@ static void initiate_domain_create(libxl__egc *egc,
>              d_config->nics[i].devid = ++last_devid;
>      }
>  
> +    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM) {
> +        for (i = 0; i < d_config->num_vtpms; i++) {
> +            ret = libxl__device_vtpm_setdefault(gc, &d_config->vtpms[i]);
> +            if (ret)
> +                goto error_out;
> +        }
> +    }
> +

Why do you not need to call libxl__device_vtpm_setdefault for PV guest?

>      if (restore_fd >= 0) {
>          LOG(DEBUG, "restoring, not running bootloader");
>          domcreate_bootloader_done(egc, &dcs->bl, 0);
> @@ -1244,6 +1252,13 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>          libxl__device_vkb_add(gc, domid, &vkb);
>          libxl_device_vkb_dispose(&vkb);
>  
> +        /*
> +         * Plug vtpm devices. dcs->multidev.callback is NULL, no need to call
> +         * libxl__multidev_prepared().
> +         */
> +        libxl__multidev_begin(ao, &dcs->multidev);
> +        libxl__add_vtpms(egc, ao, domid, d_config, &dcs->multidev);
> +

Why is vtpms added here instead of its usual location? I.e. the place
you stubbed out in previous patch. That is ...

>          dcs->dmss.dm.guest_domid = domid;
>          if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
>              libxl__spawn_stub_dm(egc, &dcs->dmss);
> @@ -1361,15 +1376,14 @@ static void domcreate_attach_vtpms(libxl__egc *egc,
>         goto error_out;
>     }
>  
> -    /*
> -     * Plug vtpm devices only for PV guest. The xenstore directory is very
> -     * different for PV guest and HVM guest, but it is still call it for
> -     * creating HVM guest, and xl should create xenstore directory before
> -     * spawning QEMU. So try to make it only for PV guest.
> -     */
> -    if (d_config->num_vtpms > 0 &&
> -        d_config->b_info.type == LIBXL_DOMAIN_TYPE_PV) {
> -
> +   /*
> +    * Plug vtpm devices only for PV guest. The xenstore directory is very
> +    * different for PV guest and HVM guest, but it is still call it for
> +    * creating HVM guest, and xl should create xenstore directory before
> +    * spawning QEMU. So try to make it only for PV guest.
> +    */
> +   if (d_config->num_vtpms > 0 &&
> +       d_config->b_info.type == LIBXL_DOMAIN_TYPE_PV) {

... here?

BTW this hunk doesn't have meaningful changes...

>         /* Attach vtpms */
>         libxl__multidev_begin(ao, &dcs->multidev);
>         dcs->multidev.callback = domcreate_attach_pci;
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 4b51ded..b1a71fe 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -26,6 +26,13 @@ char *libxl__device_frontend_path(libxl__gc *gc, libxl__device *device)
>      if (device->kind == LIBXL__DEVICE_KIND_CONSOLE && device->devid == 0)
>          return GCSPRINTF("%s/console", dom_path);
>  
> +    /* vTPM for HVM virtual machine is a special case */
> +    else if (device->backend_kind == LIBXL__DEVICE_KIND_VTPM &&
> +             device->domid == 0)
> +        return GCSPRINTF("/local/domain/0/frontend/%s/%u/%d",
> +            libxl__device_kind_to_string(device->backend_kind),
> +            device->backend_domid, device->devid);
> +

You need to define xenstore protocol first.

And, why is it special? I don't know because you haven't defined what
the protocol looks like.

>      return GCSPRINTF("%s/device/%s/%d", dom_path,
>                       libxl__device_kind_to_string(device->kind),
>                       device->devid);
> @@ -560,10 +567,39 @@ void libxl__multidev_prepared(libxl__egc *egc,
>  
>  DEFINE_DEVICES_ADD(disk)
>  DEFINE_DEVICES_ADD(nic)
> -DEFINE_DEVICES_ADD(vtpm)
>  
>  #undef DEFINE_DEVICES_ADD
>  
> +#define DEFINE_VTPM_ADD(_type)                                          \
> +    void libxl__add_##_type##s(libxl__egc *egc, libxl__ao *ao,          \
> +                               uint32_t domid,                          \
> +                               libxl_domain_config *d_config,           \
> +                               libxl__multidev *multidev)               \
> +    {                                                                   \
> +        AO_GC;                                                          \
> +        int i;                                                          \
> +        for (i = 0; i < d_config->num_##_type##s; i++) {                \
> +            libxl__ao_device *aodev = libxl__multidev_prepare(multidev); \
> +            switch(d_config->c_info.type) {                             \
> +            case LIBXL_DOMAIN_TYPE_PV:                                  \
> +                libxl__device_##_type##_add_pv(egc, domid,              \
> +                                               &d_config->_type##s[i],  \
> +                                               aodev);                  \
> +                break;                                                  \
> +            case LIBXL_DOMAIN_TYPE_HVM:                                 \
> +                libxl__device_##_type##_add_hvm(egc, domid,             \
> +                                               &d_config->_type##s[i],  \
> +                                               aodev);                  \
> +                break;                                                  \
> +            default:                                                    \
> +                break;                                                  \
> +            }                                                           \
> +        }                                                               \
> +    }
> +
> +DEFINE_VTPM_ADD(vtpm)
> +

It's fine by me that you just hand code this function without using this
macro.

But, I don't think the function body as-is is correct. There should not
be a loop. This function should only handle one device.

> +#undef DEFINE_VTPM_ADD
>  /******************************************************************************/
>  
>  int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 3e191c3..183910c 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -414,6 +414,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>      const libxl_device_nic *nics = guest_config->nics;
>      const int num_disks = guest_config->num_disks;
>      const int num_nics = guest_config->num_nics;
> +    const int num_vtpms = guest_config->num_vtpms;
>      const libxl_vnc_info *vnc = libxl__dm_vnc(guest_config);
>      const libxl_sdl_info *sdl = dm_sdl(guest_config);
>      const char *keymap = dm_keymap(guest_config);
> @@ -747,6 +748,17 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>          abort();
>      }
>  
> +    /* Add vTPM parameters for HVM virtual machine */
> +    if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
> +        num_vtpms > 0) {
> +        flexarray_vappend(dm_args, "-tpmdev",
> +                          libxl__sprintf(gc, "xenstubdoms,id=xenvtpm%d",
> +                                         guest_domid), NULL);

Please use GCSPRINTF.

> +        flexarray_vappend(dm_args,"-device",
> +                          libxl__sprintf(gc, "tpm-tis,tpmdev=xenvtpm%d",
> +                                         guest_domid), NULL);
> +    }
> +
>      ram_size = libxl__sizekb_to_mb(b_info->max_memkb - b_info->video_memkb);
>      flexarray_append(dm_args, "-m");
>      flexarray_append(dm_args, libxl__sprintf(gc, "%"PRId64, ram_size));
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 4361421..c6c82dd 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2384,10 +2384,14 @@ _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,
> +_hidden void libxl__device_vtpm_add_pv(libxl__egc *egc, uint32_t domid,
>                                     libxl_device_vtpm *vtpm,
>                                     libxl__ao_device *aodev);
>  
> +void libxl__device_vtpm_add_hvm(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);
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 3c9f146..e01d341 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -6490,8 +6490,8 @@ int main_vtpmattach(int argc, char **argv)
>         return 0;
>      }
>  
> -    if (libxl_device_vtpm_add(ctx, domid, &vtpm, 0)) {
> -        fprintf(stderr, "libxl_device_vtpm_add failed.\n");
> +    if (libxl_device_vtpm_add_pv(ctx, domid, &vtpm, 0)) {
> +        fprintf(stderr, "libxl_device_vtpm_add_pv failed.\n");

How do you add vtpm for hvm guest?

Wei.

>          return 1;
>      }
>      libxl_device_vtpm_dispose(&vtpm);
> -- 
> 1.8.3.2
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v3 5/7] vTPM: Delete the xenstore directory of frontend device

[Wei Liu]

On Tue, Mar 10, 2015 at 08:14:00AM -0400, Quan Xu wrote:
> when virtual machine is destroyed.
> 
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> ---
>  tools/libxl/libxl_device.c | 61 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 57 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index b1a71fe..668bf71 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -660,10 +660,11 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
>  {
>      STATE_AO_GC(drs->ao);
>      uint32_t domid = drs->domid;
> -    char *path;
> -    unsigned int num_kinds, num_dev_xsentries;
> -    char **kinds = NULL, **devs = NULL;
> -    int i, j, rc = 0;
> +    char *path, *dom_name, *name;
> +    unsigned int num_kinds, num_fkinds, num_dev_xsentries, num_dev;
> +    char **kinds = NULL, **fkinds = NULL, **devs = NULL, **sdevs = NULL,
> +        **be_doms = NULL;
> +    int i, j, k, rc = 0;
>      libxl__device *dev;
>      libxl__multidev *multidev = &drs->multidev;
>      libxl__ao_device *aodev;
> @@ -731,6 +732,58 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
>          libxl__device_destroy(gc, dev);
>      }
>  
> +    /*
> +     * Frontend device, such as vTPM, is under:
> +     * /local/domain/0/frontend/{type}/{backend_dom_id}/{dev}

Again, don't use hardcoded /local/domain/0.

I doubt that if you will ever need this patch if you have established a
clear xenstore protocol.

Have a look at docs/misc/xenstore-paths.markdown. (I know that document
can be vague so don't hesitate to ask questions)

Wei.

Re: [PATCH v3 6/7] vTPM: Parse envent string from QEMU frontend

[Wei Liu]

On Tue, Mar 10, 2015 at 08:14:01AM -0400, Quan Xu wrote:
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> ---
>  extras/mini-os/tpmback.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/extras/mini-os/tpmback.c b/extras/mini-os/tpmback.c
> index 8a0a983..b8f4c8f 100644
> --- a/extras/mini-os/tpmback.c
> +++ b/extras/mini-os/tpmback.c
> @@ -732,11 +732,22 @@ static int parse_eventstr(const char* evstr, domid_t* domid, unsigned int* handl
>  	 return EV_NONE;
>        }
>        return EV_NEWFE;
> -   } else if((ret = sscanf(evstr, "/local/domain/%u/device/vtpm/%u/%40s", &udomid, handle, cmd)) == 3) {
> -      *domid = udomid;
> -      if (!strcmp(cmd, "state"))
> -	 return EV_STCHNG;
> -   }
> +
> +    /* vtpm and PV virtual machines */
> +    } else if((ret = sscanf(evstr, "/local/domain/%u/device/vtpm/%u/%40s",
> +                            &udomid, handle, cmd)) == 3) {
> +        *domid = udomid;
> +        if (!strcmp(cmd, "state"))
> +            return EV_STCHNG;
> +
> +    /* HVM virtual machines */
> +    } else if((ret = sscanf(evstr, "/local/domain/0/frontend/vtpm/%u/%u/%40s",

Again, /local/domain/0. :-/

Wei.

Re: [PATCH v3 1/7] vTPM: event channel bind interdomain with para/hvm virtual machine

[Ian Campbell]

On Tue, 2015-03-10 at 08:13 -0400, Quan Xu wrote:
 
Isn't this patch just a coding style change? If there is any substance I
can't spot it, please can you do them separately, or else say in the
commit message that this is a coding style cleanup and "no functional
change" etc.

Also, you seem to have reindented everything by one more space than the
surrounding code, which doesn't seem right.

> Signed-off-by: Quan Xu <quan.xu@intel.com>
> ---
>  extras/mini-os/tpmback.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/extras/mini-os/tpmback.c b/extras/mini-os/tpmback.c
> index 00b66e8..8a0a983 100644
> --- a/extras/mini-os/tpmback.c
> +++ b/extras/mini-os/tpmback.c
> @@ -608,18 +608,21 @@ int connect_fe(tpmif_t* tpmif)
>     }
>     free(value);
>  
> -   domid = tpmif->domid;
> -   if((tpmif->page = gntmap_map_grant_refs(&gtpmdev.map, 1, &domid, 0, &ringref, PROT_READ | PROT_WRITE)) == NULL) {
> -      TPMBACK_ERR("Failed to map grant reference %u/%u\n", (unsigned int) tpmif->domid, tpmif->handle);
> -      return -1;
> -   }
> +    domid = (unsigned int)tpmif->domid;
> +    if ((tpmif->page = gntmap_map_grant_refs(&gtpmdev.map, 1, &domid, 0, &ringref,
> +                                             PROT_READ | PROT_WRITE)) == NULL) {
> +        TPMBACK_ERR("Failed to map grant reference %u/%u\n",
> +                    tpmif->domid, tpmif->handle);
> +        return -1;
> +    }
> +
> +    /* Bind the event channel */
> +    if ((evtchn_bind_interdomain(domid, evtchn, tpmback_handler, tpmif, &tpmif->evtchn))) {
> +        TPMBACK_ERR("%u/%u Unable to bind to interdomain event channel!\n",
> +                    (unsigned int) tpmif->domid, tpmif->handle);
> +        goto error_post_map;
> +    }
>  
> -   /*Bind the event channel */
> -   if((evtchn_bind_interdomain(tpmif->domid, evtchn, tpmback_handler, tpmif, &tpmif->evtchn)))
> -   {
> -      TPMBACK_ERR("%u/%u Unable to bind to interdomain event channel!\n", (unsigned int) tpmif->domid, tpmif->handle);
> -      goto error_post_map;
> -   }
>     unmask_evtchn(tpmif->evtchn);
>  
>     /* Write the ready flag and change status to connected */

Re: [PATCH v3 2/7] vTPM: limit libxl__add_vtpms() function to para virtual machine

[Ian Campbell]

On Fri, 2015-03-13 at 15:41 +0000, Wei Liu wrote:
> On Tue, Mar 10, 2015 at 08:13:57AM -0400, Quan Xu wrote:
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> > ---
> >  tools/libxl/libxl_create.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > index b1ff5ae..66877b3 100644
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -1358,8 +1358,15 @@ static void domcreate_attach_vtpms(libxl__egc *egc,
> >         goto error_out;
> >     }
> >  
> > -    /* Plug vtpm devices */
> > -   if (d_config->num_vtpms > 0) {
> > +    /*
> > +     * Plug vtpm devices only for PV guest. The xenstore directory is very
> > +     * different for PV guest and HVM guest, but it is still call it for
> > +     * creating HVM guest, and xl should create xenstore directory before
> > +     * spawning QEMU. So try to make it only for PV guest.
> > +     */
> > +    if (d_config->num_vtpms > 0 &&
> > +        d_config->b_info.type == LIBXL_DOMAIN_TYPE_PV) {
> > +
> 
> I'm not convinced that you can / should do this. This is a common entry
> for both HVM and PV guest. If you end up removing this hunk in later
> patch you need to rearrange your series to avoiding adding this hunk in
> the first place.

Agreed, although I also couldn't really parse the comment to fully
understand what the intention was.

> 
> Wei.
> 
> >         /* Attach vtpms */
> >         libxl__multidev_begin(ao, &dcs->multidev);
> >         dcs->multidev.callback = domcreate_attach_pci;
> > -- 
> > 1.8.3.2
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel

Re: [PATCH v3 3/7] vTPM: add TPM TCPA and SSDT for HVM virtual machine when vTPM is added

[Ian Campbell]

On Fri, 2015-03-13 at 15:46 +0000, Wei Liu wrote:
> On Tue, Mar 10, 2015 at 08:13:58AM -0400, Quan Xu wrote:
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> > ---
> >  tools/firmware/hvmloader/acpi/build.c | 7 ++++---
> >  tools/libxl/libxl_create.c            | 5 ++++-
> >  2 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/firmware/hvmloader/acpi/build.c b/tools/firmware/hvmloader/acpi/build.c
> > index 1431296..49f6772 100644
> > --- a/tools/firmware/hvmloader/acpi/build.c
> > +++ b/tools/firmware/hvmloader/acpi/build.c
> > @@ -313,9 +313,10 @@ static int construct_secondary_tables(unsigned long *table_ptrs,
> >  
> >      /* TPM TCPA and SSDT. */
> >      tis_hdr = (uint16_t *)0xFED40F00;
> > -    if ( (tis_hdr[0] == tis_signature[0]) &&
> > -         (tis_hdr[1] == tis_signature[1]) &&
> > -         (tis_hdr[2] == tis_signature[2]) )
> > +    if (((tis_hdr[0] == tis_signature[0]) &&
> > +        (tis_hdr[1] == tis_signature[1]) &&
> 
> These two lines are not necessary. They are not functional change and
> violate coding style.

Yes, this series so far has been full of this sort of thing. which is
rather obscuring what is going on.

I think I'll wait for a v4 with these issues fixed before I look at the
rest of it.

Ian.