[Qemu-devel] [PATCH v3 0/3] TPM NVRAM persistent storage

[Qemu-devel] [PATCH v3 0/3] TPM NVRAM persistent storage

[Corey Bryant]

This patch series provides persistent storage support that a TPM
can use to store NVRAM data.  It uses QEMU's block driver to store
data on a drive image.  The libtpms TPM 1.2 backend will be the
initial user of this functionality to store data that must persist
through a reboot or migration.  A sample command line may look like
this:

qemu-system-x86_64 ...
-drive file=/path/to/nvram.qcow2,id=drive-nvram0-0-0
-tpmdev libtpms,id=tpm-tpm0
-device tpm-tis,tpmdev=tpm-tpm0,id=tpm0,drive=drive-nvram0-0-0

Thanks,
Corey

Corey Bryant (3):
  nvram: Add TPM NVRAM implementation
  nvram: Add tpm-tis drive support
  TPM NVRAM test

 hw/tpm/Makefile.objs     |    1 +
 hw/tpm/tpm_int.h         |    2 +
 hw/tpm/tpm_nvram.c       |  324 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/tpm/tpm_nvram.h       |   25 ++++
 hw/tpm/tpm_passthrough.c |   85 ++++++++++++
 hw/tpm/tpm_tis.c         |    8 +
 6 files changed, 445 insertions(+), 0 deletions(-)
 create mode 100644 hw/tpm/tpm_nvram.c
 create mode 100644 hw/tpm/tpm_nvram.h

[Qemu-devel] [PATCH v3 1/3] nvram: Add TPM NVRAM implementation

[Corey Bryant]

Provides TPM NVRAM implementation that enables storing of TPM
NVRAM data in a persistent image file.  The block driver is
used to read/write the drive image.  This will enable, for
example, an encrypted QCOW2 image to be used to store sensitive
keys.

This patch provides APIs that a TPM backend can use to read and
write data.

Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
---
v2
 -Use non bit-rotting DPRINTF (stefanha@redhat.com)
 -Use DIV_ROUND_UP (stefanha@redhat.com)
 -Use bdrv_pread/bdrv_pwrite in coroutine - causes global
  sector to byte I/O changes (stefanha@redhat.com, kwolf@redhat.com)
 -Add tpm_nvram_required_size_kb() and update tpm_nvram_adjust_size()
 -Drop qemu_aio_wait() from tpm_nvram_do_co_read/write() and move
  any completion code path to coroutines
 -Replace tpm_nvram_rwrequest_free() with g_free(rwr)
 -Remove unneeded arg checks from tpm_nvram_read/write()
  (stefanha@redhat.com)
 -Init rwr->rc to -EINPROGRESS and wrap qemu_cond_wait() with check
  (stefanha@redhat.com)
 -Add init of completion_mutex and completion condition

v3
 -Set rwr->rc within locked completion_mutex (stefanha@redhat.com)
 -Move tpm_nvram_adjust_size to tpm_nvram_co_write
  (stefanha@redhat.com)
---
 hw/tpm/Makefile.objs |    1 +
 hw/tpm/tpm_nvram.c   |  324 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/tpm/tpm_nvram.h   |   25 ++++
 3 files changed, 350 insertions(+), 0 deletions(-)
 create mode 100644 hw/tpm/tpm_nvram.c
 create mode 100644 hw/tpm/tpm_nvram.h

diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
index 99f5983..49faef4 100644
--- a/hw/tpm/Makefile.objs
+++ b/hw/tpm/Makefile.objs
@@ -1,2 +1,3 @@
 common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
+common-obj-$(CONFIG_TPM_TIS) += tpm_nvram.o
 common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
diff --git a/hw/tpm/tpm_nvram.c b/hw/tpm/tpm_nvram.c
new file mode 100644
index 0000000..a0b547b
--- /dev/null
+++ b/hw/tpm/tpm_nvram.c
@@ -0,0 +1,324 @@
+/*
+ * TPM NVRAM - enables storage of persistent NVRAM data on an image file
+ *
+ * Copyright (C) 2013 IBM Corporation
+ *
+ * Authors:
+ *  Stefan Berger    <stefanb@us.ibm.com>
+ *  Corey Bryant     <coreyb@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "tpm_nvram.h"
+#include "block/block_int.h"
+#include "qemu/thread.h"
+#include "sysemu/sysemu.h"
+
+#define TPM_NVRAM_DEBUG 0
+#define DPRINTF(fmt, ...) \
+    do { \
+        if (TPM_NVRAM_DEBUG) { \
+            fprintf(stderr, fmt, ## __VA_ARGS__); \
+        } \
+    } while (0)
+
+/* Read/write request data */
+typedef struct TPMNvramRWRequest {
+    BlockDriverState *bdrv;
+    bool is_write;
+    uint64_t offset;
+    uint8_t **blob_r;
+    uint8_t *blob_w;
+    uint32_t size;
+    int rc;
+
+    QemuMutex completion_mutex;
+    QemuCond completion;
+
+    QSIMPLEQ_ENTRY(TPMNvramRWRequest) list;
+} TPMNvramRWRequest;
+
+/* Mutex protected queue of read/write requests */
+static QemuMutex tpm_nvram_rwrequests_mutex;
+static QSIMPLEQ_HEAD(, TPMNvramRWRequest) tpm_nvram_rwrequests =
+    QSIMPLEQ_HEAD_INITIALIZER(tpm_nvram_rwrequests);
+
+static QEMUBH *tpm_nvram_bh;
+
+/*
+ * Get the disk size in kilobytes needed to store a blob (rounded up to next kb)
+ */
+static uint64_t tpm_nvram_required_size_kb(uint64_t offset, uint32_t size)
+{
+    uint64_t required_size = offset + size;
+    return DIV_ROUND_UP(required_size, 1024);
+}
+
+/*
+ * Increase the drive size if it's too small to store the blob
+ */
+static int tpm_nvram_adjust_size(BlockDriverState *bdrv, uint64_t offset,
+                                 uint32_t size)
+{
+    int rc = 0;
+    int64_t drive_size, required_size;
+
+    rc = bdrv_getlength(bdrv);
+    if (rc < 0) {
+        DPRINTF("%s: Unable to determine TPM NVRAM drive size\n", __func__);
+        return rc;
+    }
+
+    drive_size = rc;
+    required_size = tpm_nvram_required_size_kb(offset, size) * 1024;
+
+    if (drive_size < required_size) {
+        rc = bdrv_truncate(bdrv, required_size);
+        if (rc < 0) {
+            DPRINTF("%s: TPM NVRAM drive too small\n", __func__);
+        }
+    }
+
+    return rc;
+}
+
+/*
+ * Coroutine that reads a blob from the drive asynchronously
+ */
+static void coroutine_fn tpm_nvram_co_read(void *opaque)
+{
+    int rc;
+    TPMNvramRWRequest *rwr = opaque;
+
+    *rwr->blob_r = g_malloc(rwr->size);
+
+    rc = bdrv_pread(rwr->bdrv, rwr->offset, *rwr->blob_r, rwr->size);
+    if (rc != rwr->size) {
+        g_free(*rwr->blob_r);
+        *rwr->blob_r = NULL;
+    }
+
+    qemu_mutex_lock(&rwr->completion_mutex);
+    rwr->rc = rc;
+    qemu_cond_signal(&rwr->completion);
+    qemu_mutex_unlock(&rwr->completion_mutex);
+}
+
+/*
+ * Coroutine that writes a blob to the drive asynchronously
+ */
+static void coroutine_fn tpm_nvram_co_write(void *opaque)
+{
+    int rc;
+    TPMNvramRWRequest *rwr = opaque;
+
+    rc = tpm_nvram_adjust_size(rwr->bdrv, rwr->offset, rwr->size);
+    if (rc < 0) {
+        goto exit;
+    }
+
+    rc = bdrv_pwrite(rwr->bdrv, rwr->offset, rwr->blob_w, rwr->size);
+
+exit:
+    qemu_mutex_lock(&rwr->completion_mutex);
+    rwr->rc = rc;
+    qemu_cond_signal(&rwr->completion);
+    qemu_mutex_unlock(&rwr->completion_mutex);
+}
+
+/*
+ * Enter a coroutine to read a blob from the drive
+ */
+static void tpm_nvram_do_co_read(TPMNvramRWRequest *rwr)
+{
+    Coroutine *co;
+
+    co = qemu_coroutine_create(tpm_nvram_co_read);
+    qemu_coroutine_enter(co, rwr);
+}
+
+/*
+ * Enter a coroutine to write a blob to the drive
+ */
+static void tpm_nvram_do_co_write(TPMNvramRWRequest *rwr)
+{
+    int rc;
+    Coroutine *co;
+
+    co = qemu_coroutine_create(tpm_nvram_co_write);
+    qemu_coroutine_enter(co, rwr);
+}
+
+/*
+ * Initialization for read requests
+ */
+static TPMNvramRWRequest *tpm_nvram_rwrequest_init_read(BlockDriverState *bdrv,
+                                                        uint64_t offset,
+                                                        uint8_t **blob,
+                                                        uint32_t size)
+{
+    TPMNvramRWRequest *rwr;
+
+    rwr = g_new0(TPMNvramRWRequest, 1);
+    rwr->bdrv = bdrv;
+    rwr->is_write = false;
+    rwr->offset = offset;
+    rwr->blob_r = blob;
+    rwr->size = size;
+    rwr->rc = -EINPROGRESS;
+
+    qemu_mutex_init(&rwr->completion_mutex);
+    qemu_cond_init(&rwr->completion);
+
+    return rwr;
+}
+
+/*
+ * Initialization for write requests
+ */
+static TPMNvramRWRequest *tpm_nvram_rwrequest_init_write(BlockDriverState *bdrv,
+                                                         uint64_t offset,
+                                                         uint8_t *blob,
+                                                         uint32_t size)
+{
+    TPMNvramRWRequest *rwr;
+
+    rwr = g_new0(TPMNvramRWRequest, 1);
+    rwr->bdrv = bdrv;
+    rwr->is_write = true;
+    rwr->offset = offset;
+    rwr->blob_w = blob;
+    rwr->size = size;
+    rwr->rc = -EINPROGRESS;
+
+    qemu_mutex_init(&rwr->completion_mutex);
+    qemu_cond_init(&rwr->completion);
+
+    return rwr;
+}
+
+/*
+ * Execute a read or write of TPM NVRAM blob data
+ */
+static void tpm_nvram_rwrequest_exec(TPMNvramRWRequest *rwr)
+{
+    if (rwr->is_write) {
+        tpm_nvram_do_co_write(rwr);
+    } else {
+        tpm_nvram_do_co_read(rwr);
+    }
+}
+
+/*
+ * Bottom-half callback that is invoked by QEMU's main thread to
+ * process TPM NVRAM read/write requests.
+ */
+static void tpm_nvram_rwrequest_callback(void *opaque)
+{
+    TPMNvramRWRequest *rwr, *next;
+
+    qemu_mutex_lock(&tpm_nvram_rwrequests_mutex);
+
+    QSIMPLEQ_FOREACH_SAFE(rwr, &tpm_nvram_rwrequests, list, next) {
+        QSIMPLEQ_REMOVE(&tpm_nvram_rwrequests, rwr, TPMNvramRWRequest, list);
+
+        qemu_mutex_unlock(&tpm_nvram_rwrequests_mutex);
+        tpm_nvram_rwrequest_exec(rwr);
+        qemu_mutex_lock(&tpm_nvram_rwrequests_mutex);
+    }
+
+    qemu_mutex_unlock(&tpm_nvram_rwrequests_mutex);
+}
+
+/*
+ * Schedule a bottom-half to read or write a blob to the TPM NVRAM drive
+ */
+static void tpm_nvram_rwrequest_schedule(TPMNvramRWRequest *rwr)
+{
+    qemu_mutex_lock(&tpm_nvram_rwrequests_mutex);
+    QSIMPLEQ_INSERT_TAIL(&tpm_nvram_rwrequests, rwr, list);
+    qemu_mutex_unlock(&tpm_nvram_rwrequests_mutex);
+
+    qemu_bh_schedule(tpm_nvram_bh);
+
+    /* Wait for completion of the read/write request */
+    qemu_mutex_lock(&rwr->completion_mutex);
+    while (rwr->rc == -EINPROGRESS) {
+        qemu_cond_wait(&rwr->completion, &rwr->completion_mutex);
+    }
+    qemu_mutex_unlock(&rwr->completion_mutex);
+}
+
+/*
+ * Initialize a TPM NVRAM drive
+ */
+int tpm_nvram_init(BlockDriverState *bdrv)
+{
+    qemu_mutex_init(&tpm_nvram_rwrequests_mutex);
+    tpm_nvram_bh = qemu_bh_new(tpm_nvram_rwrequest_callback, NULL);
+
+    if (bdrv_is_read_only(bdrv)) {
+        DPRINTF("%s: TPM NVRAM drive '%s' is read-only\n", __func__,
+                bdrv->filename);
+        return -EPERM;
+    }
+
+    bdrv_lock_medium(bdrv, true);
+
+    DPRINTF("%s: TPM NVRAM drive '%s' initialized successfully\n", __func__,
+            bdrv->filename);
+
+    return 0;
+}
+
+/*
+ * Read a TPM NVRAM blob from the drive
+ */
+int tpm_nvram_read(BlockDriverState *bdrv, uint64_t offset,
+                   uint8_t **blob, uint32_t size)
+{
+    int rc;
+    TPMNvramRWRequest *rwr;
+
+    *blob = NULL;
+
+    rwr = tpm_nvram_rwrequest_init_read(bdrv, offset, blob, size);
+    tpm_nvram_rwrequest_schedule(rwr);
+    rc = rwr->rc;
+
+    if (rc != rwr->size) {
+        DPRINTF("%s: TPM NVRAM read failed\n", __func__);
+    } else {
+        DPRINTF("%s: TPM NVRAM read successful: offset=%"PRIu64", "
+                "size=%"PRIu32"\n", __func__, rwr->offset, rwr->size);
+    }
+
+    g_free(rwr);
+    return rc;
+}
+
+/*
+ * Write a TPM NVRAM blob to the drive
+ */
+int tpm_nvram_write(BlockDriverState *bdrv, uint64_t offset,
+                    uint8_t *blob, uint32_t size)
+{
+    int rc;
+    TPMNvramRWRequest *rwr;
+
+    rwr = tpm_nvram_rwrequest_init_write(bdrv, offset, blob, size);
+    tpm_nvram_rwrequest_schedule(rwr);
+    rc = rwr->rc;
+
+    if (rc != rwr->size) {
+        DPRINTF("%s: TPM NVRAM write failed\n", __func__);
+    } else {
+        DPRINTF("%s: TPM NVRAM write successful: offset=%"PRIu64", "
+                "size=%"PRIu32"\n", __func__, rwr->offset, rwr->size);
+    }
+
+    g_free(rwr);
+    return rc;
+}
diff --git a/hw/tpm/tpm_nvram.h b/hw/tpm/tpm_nvram.h
new file mode 100644
index 0000000..4b5bbe0
--- /dev/null
+++ b/hw/tpm/tpm_nvram.h
@@ -0,0 +1,25 @@
+/*
+ * TPM NVRAM - enables storage of persistent NVRAM data on an image file
+ *
+ * Copyright (C) 2013 IBM Corporation
+ *
+ * Authors:
+ *  Stefan Berger    <stefanb@us.ibm.com>
+ *  Corey Bryant     <coreyb@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef TPM_TPM_NVRAM_H
+#define TPM_TPM_NVRAM_H
+
+#include "block/block.h"
+
+int tpm_nvram_init(BlockDriverState *bdrv);
+int tpm_nvram_read(BlockDriverState *bdrv, uint64_t offset,
+                   uint8_t **blob, uint32_t size);
+int tpm_nvram_write(BlockDriverState *bdrv, uint64_t offset,
+                    uint8_t *blob, uint32_t size);
+
+#endif
-- 
1.7.1

[Qemu-devel] [PATCH v3 2/3] nvram: Add tpm-tis drive support

[Corey Bryant]

Add a drive property to the tpm-tis device and initialize the TPM
NVRAM if a drive is specified.

Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
---
v2
  -No changes

v3
  -No changes
---
 hw/tpm/tpm_int.h |    2 ++
 hw/tpm/tpm_tis.c |    8 ++++++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/hw/tpm/tpm_int.h b/hw/tpm/tpm_int.h
index 2f582ca..05471ef 100644
--- a/hw/tpm/tpm_int.h
+++ b/hw/tpm/tpm_int.h
@@ -29,6 +29,8 @@ struct TPMState {
 
     char *backend;
     TPMBackend *be_driver;
+
+    BlockDriverState *bdrv;
 };
 
 #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index d4d8152..8648b3b 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -27,6 +27,7 @@
 #include "hw/i386/pc.h"
 #include "hw/pci/pci_ids.h"
 #include "tpm_tis.h"
+#include "tpm_nvram.h"
 #include "qemu-common.h"
 
 /*#define DEBUG_TIS */
@@ -849,6 +850,7 @@ static Property tpm_tis_properties[] = {
     DEFINE_PROP_UINT32("irq", TPMState,
                        s.tis.irq_num, TPM_TIS_IRQ),
     DEFINE_PROP_STRING("tpmdev", TPMState, backend),
+    DEFINE_PROP_DRIVE("drive", TPMState, bdrv),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -864,6 +866,12 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (s->bdrv && tpm_nvram_init(s->bdrv)) {
+        error_setg(errp, "tpm_tis: backend drive with id %s could not "
+                   "initialize TPM NVRAM drive", s->backend);
+        return;
+    }
+
     s->be_driver->fe_model = TPM_MODEL_TPM_TIS;
 
     if (tpm_backend_init(s->be_driver, s, tpm_tis_receive_cb)) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH v3 0/3] TPM NVRAM persistent storage

[Stefan Hajnoczi]

On Thu, Jun 06, 2013 at 09:32:42AM -0400, Corey Bryant wrote:
> This patch series provides persistent storage support that a TPM
> can use to store NVRAM data.  It uses QEMU's block driver to store
> data on a drive image.  The libtpms TPM 1.2 backend will be the
> initial user of this functionality to store data that must persist
> through a reboot or migration.  A sample command line may look like
> this:
> 
> qemu-system-x86_64 ...
> -drive file=/path/to/nvram.qcow2,id=drive-nvram0-0-0
> -tpmdev libtpms,id=tpm-tpm0
> -device tpm-tis,tpmdev=tpm-tpm0,id=tpm0,drive=drive-nvram0-0-0
> 
> Thanks,
> Corey
> 
> Corey Bryant (3):
>   nvram: Add TPM NVRAM implementation
>   nvram: Add tpm-tis drive support
>   TPM NVRAM test
> 
>  hw/tpm/Makefile.objs     |    1 +
>  hw/tpm/tpm_int.h         |    2 +
>  hw/tpm/tpm_nvram.c       |  324 ++++++++++++++++++++++++++++++++++++++++++++++
>  hw/tpm/tpm_nvram.h       |   25 ++++
>  hw/tpm/tpm_passthrough.c |   85 ++++++++++++
>  hw/tpm/tpm_tis.c         |    8 +
>  6 files changed, 445 insertions(+), 0 deletions(-)
>  create mode 100644 hw/tpm/tpm_nvram.c
>  create mode 100644 hw/tpm/tpm_nvram.h
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH v3 0/3] TPM NVRAM persistent storage

[Corey Bryant]

On 06/07/2013 03:41 AM, Stefan Hajnoczi wrote:
> On Thu, Jun 06, 2013 at 09:32:42AM -0400, Corey Bryant wrote:
>> This patch series provides persistent storage support that a TPM
>> can use to store NVRAM data.  It uses QEMU's block driver to store
>> data on a drive image.  The libtpms TPM 1.2 backend will be the
>> initial user of this functionality to store data that must persist
>> through a reboot or migration.  A sample command line may look like
>> this:
>>
>> qemu-system-x86_64 ...
>> -drive file=/path/to/nvram.qcow2,id=drive-nvram0-0-0
>> -tpmdev libtpms,id=tpm-tpm0
>> -device tpm-tis,tpmdev=tpm-tpm0,id=tpm0,drive=drive-nvram0-0-0
>>
>> Thanks,
>> Corey
>>
>> Corey Bryant (3):
>>    nvram: Add TPM NVRAM implementation
>>    nvram: Add tpm-tis drive support
>>    TPM NVRAM test
>>
>>   hw/tpm/Makefile.objs     |    1 +
>>   hw/tpm/tpm_int.h         |    2 +
>>   hw/tpm/tpm_nvram.c       |  324 ++++++++++++++++++++++++++++++++++++++++++++++
>>   hw/tpm/tpm_nvram.h       |   25 ++++
>>   hw/tpm/tpm_passthrough.c |   85 ++++++++++++
>>   hw/tpm/tpm_tis.c         |    8 +
>>   6 files changed, 445 insertions(+), 0 deletions(-)
>>   create mode 100644 hw/tpm/tpm_nvram.c
>>   create mode 100644 hw/tpm/tpm_nvram.h
>>
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>
>

Thanks again for the review!

-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [PATCH v3 0/3] TPM NVRAM persistent storage

[Anthony Liguori]

Corey Bryant <coreyb@linux.vnet.ibm.com> writes:

> This patch series provides persistent storage support that a TPM
> can use to store NVRAM data.  It uses QEMU's block driver to store
> data on a drive image.  The libtpms TPM 1.2 backend will be the
> initial user of this functionality to store data that must persist
> through a reboot or migration.  A sample command line may look like
> this:

This should be folded into the libtpms backend series.

There are no users for this so this would just be untestable code in the
tree subject to bitrot.

Regards,

Anthony Liguori

>
> qemu-system-x86_64 ...
> -drive file=/path/to/nvram.qcow2,id=drive-nvram0-0-0
> -tpmdev libtpms,id=tpm-tpm0
> -device tpm-tis,tpmdev=tpm-tpm0,id=tpm0,drive=drive-nvram0-0-0
>
> Thanks,
> Corey
>
> Corey Bryant (3):
>   nvram: Add TPM NVRAM implementation
>   nvram: Add tpm-tis drive support
>   TPM NVRAM test
>
>  hw/tpm/Makefile.objs     |    1 +
>  hw/tpm/tpm_int.h         |    2 +
>  hw/tpm/tpm_nvram.c       |  324 ++++++++++++++++++++++++++++++++++++++++++++++
>  hw/tpm/tpm_nvram.h       |   25 ++++
>  hw/tpm/tpm_passthrough.c |   85 ++++++++++++
>  hw/tpm/tpm_tis.c         |    8 +
>  6 files changed, 445 insertions(+), 0 deletions(-)
>  create mode 100644 hw/tpm/tpm_nvram.c
>  create mode 100644 hw/tpm/tpm_nvram.h

Re: [Qemu-devel] [PATCH v3 0/3] TPM NVRAM persistent storage

[Corey Bryant]

On 06/14/2013 10:01 AM, Anthony Liguori wrote:
> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>
>> This patch series provides persistent storage support that a TPM
>> can use to store NVRAM data.  It uses QEMU's block driver to store
>> data on a drive image.  The libtpms TPM 1.2 backend will be the
>> initial user of this functionality to store data that must persist
>> through a reboot or migration.  A sample command line may look like
>> this:
>
> This should be folded into the libtpms backend series.
>
> There are no users for this so this would just be untestable code in the
> tree subject to bitrot.
>
> Regards,
>
> Anthony Liguori
>

Fair enough.  I assume you're ok with this code though?

-- 
Regards,
Corey Bryant

>>
>> qemu-system-x86_64 ...
>> -drive file=/path/to/nvram.qcow2,id=drive-nvram0-0-0
>> -tpmdev libtpms,id=tpm-tpm0
>> -device tpm-tis,tpmdev=tpm-tpm0,id=tpm0,drive=drive-nvram0-0-0
>>
>> Thanks,
>> Corey
>>
>> Corey Bryant (3):
>>    nvram: Add TPM NVRAM implementation
>>    nvram: Add tpm-tis drive support
>>    TPM NVRAM test
>>
>>   hw/tpm/Makefile.objs     |    1 +
>>   hw/tpm/tpm_int.h         |    2 +
>>   hw/tpm/tpm_nvram.c       |  324 ++++++++++++++++++++++++++++++++++++++++++++++
>>   hw/tpm/tpm_nvram.h       |   25 ++++
>>   hw/tpm/tpm_passthrough.c |   85 ++++++++++++
>>   hw/tpm/tpm_tis.c         |    8 +
>>   6 files changed, 445 insertions(+), 0 deletions(-)
>>   create mode 100644 hw/tpm/tpm_nvram.c
>>   create mode 100644 hw/tpm/tpm_nvram.h
>
>
>
>

Re: [Qemu-devel] [PATCH v3 0/3] TPM NVRAM persistent storage

[Anthony Liguori]

Corey Bryant <coreyb@linux.vnet.ibm.com> writes:

> On 06/14/2013 10:01 AM, Anthony Liguori wrote:
>> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>>
>>> This patch series provides persistent storage support that a TPM
>>> can use to store NVRAM data.  It uses QEMU's block driver to store
>>> data on a drive image.  The libtpms TPM 1.2 backend will be the
>>> initial user of this functionality to store data that must persist
>>> through a reboot or migration.  A sample command line may look like
>>> this:
>>
>> This should be folded into the libtpms backend series.
>>
>> There are no users for this so this would just be untestable code in the
>> tree subject to bitrot.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>
> Fair enough.  I assume you're ok with this code though?

I don't understand why it's needed to be honest.  I suspect this has to
do with the fact that the libtpms implementation will need significant
reworking.

Regards,

Anthony Liguori

>
> -- 
> Regards,
> Corey Bryant
>
>>>
>>> qemu-system-x86_64 ...
>>> -drive file=/path/to/nvram.qcow2,id=drive-nvram0-0-0
>>> -tpmdev libtpms,id=tpm-tpm0
>>> -device tpm-tis,tpmdev=tpm-tpm0,id=tpm0,drive=drive-nvram0-0-0
>>>
>>> Thanks,
>>> Corey
>>>
>>> Corey Bryant (3):
>>>    nvram: Add TPM NVRAM implementation
>>>    nvram: Add tpm-tis drive support
>>>    TPM NVRAM test
>>>
>>>   hw/tpm/Makefile.objs     |    1 +
>>>   hw/tpm/tpm_int.h         |    2 +
>>>   hw/tpm/tpm_nvram.c       |  324 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   hw/tpm/tpm_nvram.h       |   25 ++++
>>>   hw/tpm/tpm_passthrough.c |   85 ++++++++++++
>>>   hw/tpm/tpm_tis.c         |    8 +
>>>   6 files changed, 445 insertions(+), 0 deletions(-)
>>>   create mode 100644 hw/tpm/tpm_nvram.c
>>>   create mode 100644 hw/tpm/tpm_nvram.h
>>
>>
>>
>>

Re: [Qemu-devel] [PATCH v3 0/3] TPM NVRAM persistent storage

[Stefan Berger]

On 06/14/2013 11:38 AM, Anthony Liguori wrote:
> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>
>> On 06/14/2013 10:01 AM, Anthony Liguori wrote:
>>> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>>>
>>>> This patch series provides persistent storage support that a TPM
>>>> can use to store NVRAM data.  It uses QEMU's block driver to store
>>>> data on a drive image.  The libtpms TPM 1.2 backend will be the
>>>> initial user of this functionality to store data that must persist
>>>> through a reboot or migration.  A sample command line may look like
>>>> this:
>>> This should be folded into the libtpms backend series.
>>>
>>> There are no users for this so this would just be untestable code in the
>>> tree subject to bitrot.
>>>
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>> Fair enough.  I assume you're ok with this code though?
> I don't understand why it's needed to be honest.  I suspect this has to
> do with the fact that the libtpms implementation will need significant
> reworking.

libtpms does not implement a file storage layer. It neither writes data 
into a FILE * nor into a QEMU BDRV. Instead it provides callbacks for 
users to implement the file storage layer. Is there a problem with that?

    Regards,
       Stefan

>
> Regards,
>
> Anthony Liguori
>
>> -- 
>> Regards,
>> Corey Bryant
>>
>>>> qemu-system-x86_64 ...
>>>> -drive file=/path/to/nvram.qcow2,id=drive-nvram0-0-0
>>>> -tpmdev libtpms,id=tpm-tpm0
>>>> -device tpm-tis,tpmdev=tpm-tpm0,id=tpm0,drive=drive-nvram0-0-0
>>>>
>>>> Thanks,
>>>> Corey
>>>>
>>>> Corey Bryant (3):
>>>>     nvram: Add TPM NVRAM implementation
>>>>     nvram: Add tpm-tis drive support
>>>>     TPM NVRAM test
>>>>
>>>>    hw/tpm/Makefile.objs     |    1 +
>>>>    hw/tpm/tpm_int.h         |    2 +
>>>>    hw/tpm/tpm_nvram.c       |  324 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>    hw/tpm/tpm_nvram.h       |   25 ++++
>>>>    hw/tpm/tpm_passthrough.c |   85 ++++++++++++
>>>>    hw/tpm/tpm_tis.c         |    8 +
>>>>    6 files changed, 445 insertions(+), 0 deletions(-)
>>>>    create mode 100644 hw/tpm/tpm_nvram.c
>>>>    create mode 100644 hw/tpm/tpm_nvram.h
>>>
>>>
>>>

Re: [Qemu-devel] [PATCH v3 0/3] TPM NVRAM persistent storage

[Corey Bryant]

On 06/14/2013 11:38 AM, Anthony Liguori wrote:
> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>
>> On 06/14/2013 10:01 AM, Anthony Liguori wrote:
>>> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>>>
>>>> This patch series provides persistent storage support that a TPM
>>>> can use to store NVRAM data.  It uses QEMU's block driver to store
>>>> data on a drive image.  The libtpms TPM 1.2 backend will be the
>>>> initial user of this functionality to store data that must persist
>>>> through a reboot or migration.  A sample command line may look like
>>>> this:
>>>
>>> This should be folded into the libtpms backend series.
>>>
>>> There are no users for this so this would just be untestable code in the
>>> tree subject to bitrot.
>>>
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>>
>> Fair enough.  I assume you're ok with this code though?
>
> I don't understand why it's needed to be honest.  I suspect this has to
> do with the fact that the libtpms implementation will need significant
> reworking.
>
> Regards,
>
> Anthony Liguori
>

In regards to why it is needed..  The QEMU software-emulated vTPM 
backend will pass callback functions to libtpms for writing/reading 
nvram data.  Those callbacks will use the code in this patch series to 
do the writing/reading of nvram data to/from image files so that the 
data persists through migration/reboot.

I'm not sure I completely understand your second sentence, but yes the 
software-emulated vTPM backend code for QEMU will certainly need rework 
to use the code in this patch series.

-- 
Regards,
Corey Bryant

>>
>> --
>> Regards,
>> Corey Bryant
>>
>>>>
>>>> qemu-system-x86_64 ...
>>>> -drive file=/path/to/nvram.qcow2,id=drive-nvram0-0-0
>>>> -tpmdev libtpms,id=tpm-tpm0
>>>> -device tpm-tis,tpmdev=tpm-tpm0,id=tpm0,drive=drive-nvram0-0-0
>>>>
>>>> Thanks,
>>>> Corey
>>>>
>>>> Corey Bryant (3):
>>>>     nvram: Add TPM NVRAM implementation
>>>>     nvram: Add tpm-tis drive support
>>>>     TPM NVRAM test
>>>>
>>>>    hw/tpm/Makefile.objs     |    1 +
>>>>    hw/tpm/tpm_int.h         |    2 +
>>>>    hw/tpm/tpm_nvram.c       |  324 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>    hw/tpm/tpm_nvram.h       |   25 ++++
>>>>    hw/tpm/tpm_passthrough.c |   85 ++++++++++++
>>>>    hw/tpm/tpm_tis.c         |    8 +
>>>>    6 files changed, 445 insertions(+), 0 deletions(-)
>>>>    create mode 100644 hw/tpm/tpm_nvram.c
>>>>    create mode 100644 hw/tpm/tpm_nvram.h
>>>
>>>
>>>
>>>
>
>
>

Re: [Qemu-devel] [PATCH v3 0/3] TPM NVRAM persistent storage

[Anthony Liguori]

Corey Bryant <coreyb@linux.vnet.ibm.com> writes:

> On 06/14/2013 11:38 AM, Anthony Liguori wrote:
>> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>>
>>> On 06/14/2013 10:01 AM, Anthony Liguori wrote:
>>>> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>>>>
>>>>> This patch series provides persistent storage support that a TPM
>>>>> can use to store NVRAM data.  It uses QEMU's block driver to store
>>>>> data on a drive image.  The libtpms TPM 1.2 backend will be the
>>>>> initial user of this functionality to store data that must persist
>>>>> through a reboot or migration.  A sample command line may look like
>>>>> this:
>>>>
>>>> This should be folded into the libtpms backend series.
>>>>
>>>> There are no users for this so this would just be untestable code in the
>>>> tree subject to bitrot.
>>>>
>>>> Regards,
>>>>
>>>> Anthony Liguori
>>>>
>>>
>>> Fair enough.  I assume you're ok with this code though?
>>
>> I don't understand why it's needed to be honest.  I suspect this has to
>> do with the fact that the libtpms implementation will need significant
>> reworking.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>
> In regards to why it is needed..  The QEMU software-emulated vTPM 
> backend will pass callback functions to libtpms for writing/reading 
> nvram data.  Those callbacks will use the code in this patch series to 
> do the writing/reading of nvram data to/from image files so that the 
> data persists through migration/reboot.
>
> I'm not sure I completely understand your second sentence, but yes the 
> software-emulated vTPM backend code for QEMU will certainly need rework 
> to use the code in this patch series.

I think it's easiest to discuss this in the context of the actual patch
series.

Regards,

Anthony Liguori

>
> -- 
> Regards,
> Corey Bryant
>
>>>
>>> --
>>> Regards,
>>> Corey Bryant
>>>
>>>>>
>>>>> qemu-system-x86_64 ...
>>>>> -drive file=/path/to/nvram.qcow2,id=drive-nvram0-0-0
>>>>> -tpmdev libtpms,id=tpm-tpm0
>>>>> -device tpm-tis,tpmdev=tpm-tpm0,id=tpm0,drive=drive-nvram0-0-0
>>>>>
>>>>> Thanks,
>>>>> Corey
>>>>>
>>>>> Corey Bryant (3):
>>>>>     nvram: Add TPM NVRAM implementation
>>>>>     nvram: Add tpm-tis drive support
>>>>>     TPM NVRAM test
>>>>>
>>>>>    hw/tpm/Makefile.objs     |    1 +
>>>>>    hw/tpm/tpm_int.h         |    2 +
>>>>>    hw/tpm/tpm_nvram.c       |  324 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    hw/tpm/tpm_nvram.h       |   25 ++++
>>>>>    hw/tpm/tpm_passthrough.c |   85 ++++++++++++
>>>>>    hw/tpm/tpm_tis.c         |    8 +
>>>>>    6 files changed, 445 insertions(+), 0 deletions(-)
>>>>>    create mode 100644 hw/tpm/tpm_nvram.c
>>>>>    create mode 100644 hw/tpm/tpm_nvram.h
>>>>
>>>>
>>>>
>>>>
>>
>>
>>

Re: [Qemu-devel] [PATCH v3 0/3] TPM NVRAM persistent storage

[Corey Bryant]

On 06/14/2013 11:56 AM, Anthony Liguori wrote:
> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>
>> On 06/14/2013 11:38 AM, Anthony Liguori wrote:
>>> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>>>
>>>> On 06/14/2013 10:01 AM, Anthony Liguori wrote:
>>>>> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>>>>>
>>>>>> This patch series provides persistent storage support that a TPM
>>>>>> can use to store NVRAM data.  It uses QEMU's block driver to store
>>>>>> data on a drive image.  The libtpms TPM 1.2 backend will be the
>>>>>> initial user of this functionality to store data that must persist
>>>>>> through a reboot or migration.  A sample command line may look like
>>>>>> this:
>>>>>
>>>>> This should be folded into the libtpms backend series.
>>>>>
>>>>> There are no users for this so this would just be untestable code in the
>>>>> tree subject to bitrot.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Anthony Liguori
>>>>>
>>>>
>>>> Fair enough.  I assume you're ok with this code though?
>>>
>>> I don't understand why it's needed to be honest.  I suspect this has to
>>> do with the fact that the libtpms implementation will need significant
>>> reworking.
>>>
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>>
>> In regards to why it is needed..  The QEMU software-emulated vTPM
>> backend will pass callback functions to libtpms for writing/reading
>> nvram data.  Those callbacks will use the code in this patch series to
>> do the writing/reading of nvram data to/from image files so that the
>> data persists through migration/reboot.
>>
>> I'm not sure I completely understand your second sentence, but yes the
>> software-emulated vTPM backend code for QEMU will certainly need rework
>> to use the code in this patch series.
>
> I think it's easiest to discuss this in the context of the actual patch
> series.
>
> Regards,
>
> Anthony Liguori
>

I suppose, but the earlier we can get feedback the better so that we 
don't waste any more time.  This NVRAM code alone has gone through far 
too many iterations as folks have asked for it to go in different 
directions, and we went in those directions to find that they were the 
wrong directions.  Anyway, for the record, this latest patch series 
adheres to the direction you suggested we take last month: 
http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg04275.html

-- 
Regards,
Corey Bryant

>>
>> --
>> Regards,
>> Corey Bryant
>>
>>>>
>>>> --
>>>> Regards,
>>>> Corey Bryant
>>>>
>>>>>>
>>>>>> qemu-system-x86_64 ...
>>>>>> -drive file=/path/to/nvram.qcow2,id=drive-nvram0-0-0
>>>>>> -tpmdev libtpms,id=tpm-tpm0
>>>>>> -device tpm-tis,tpmdev=tpm-tpm0,id=tpm0,drive=drive-nvram0-0-0
>>>>>>
>>>>>> Thanks,
>>>>>> Corey
>>>>>>
>>>>>> Corey Bryant (3):
>>>>>>      nvram: Add TPM NVRAM implementation
>>>>>>      nvram: Add tpm-tis drive support
>>>>>>      TPM NVRAM test
>>>>>>
>>>>>>     hw/tpm/Makefile.objs     |    1 +
>>>>>>     hw/tpm/tpm_int.h         |    2 +
>>>>>>     hw/tpm/tpm_nvram.c       |  324 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>     hw/tpm/tpm_nvram.h       |   25 ++++
>>>>>>     hw/tpm/tpm_passthrough.c |   85 ++++++++++++
>>>>>>     hw/tpm/tpm_tis.c         |    8 +
>>>>>>     6 files changed, 445 insertions(+), 0 deletions(-)
>>>>>>     create mode 100644 hw/tpm/tpm_nvram.c
>>>>>>     create mode 100644 hw/tpm/tpm_nvram.h
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
>
>
>

Re: [Qemu-devel] [PATCH v3 0/3] TPM NVRAM persistent storage

[Anthony Liguori]

Corey Bryant <coreyb@linux.vnet.ibm.com> writes:

> On 06/14/2013 11:56 AM, Anthony Liguori wrote:
>> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>>
>>> On 06/14/2013 11:38 AM, Anthony Liguori wrote:
>>>> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>>>>
>>>>> On 06/14/2013 10:01 AM, Anthony Liguori wrote:
>>>>>> Corey Bryant <coreyb@linux.vnet.ibm.com> writes:
>>>>>>
>>>>>>> This patch series provides persistent storage support that a TPM
>>>>>>> can use to store NVRAM data.  It uses QEMU's block driver to store
>>>>>>> data on a drive image.  The libtpms TPM 1.2 backend will be the
>>>>>>> initial user of this functionality to store data that must persist
>>>>>>> through a reboot or migration.  A sample command line may look like
>>>>>>> this:
>>>>>>
>>>>>> This should be folded into the libtpms backend series.
>>>>>>
>>>>>> There are no users for this so this would just be untestable code in the
>>>>>> tree subject to bitrot.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Anthony Liguori
>>>>>>
>>>>>
>>>>> Fair enough.  I assume you're ok with this code though?
>>>>
>>>> I don't understand why it's needed to be honest.  I suspect this has to
>>>> do with the fact that the libtpms implementation will need significant
>>>> reworking.
>>>>
>>>> Regards,
>>>>
>>>> Anthony Liguori
>>>>
>>>
>>> In regards to why it is needed..  The QEMU software-emulated vTPM
>>> backend will pass callback functions to libtpms for writing/reading
>>> nvram data.  Those callbacks will use the code in this patch series to
>>> do the writing/reading of nvram data to/from image files so that the
>>> data persists through migration/reboot.
>>>
>>> I'm not sure I completely understand your second sentence, but yes the
>>> software-emulated vTPM backend code for QEMU will certainly need rework
>>> to use the code in this patch series.
>>
>> I think it's easiest to discuss this in the context of the actual patch
>> series.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>
> I suppose, but the earlier we can get feedback the better so that we 
> don't waste any more time.  This NVRAM code alone has gone through far 
> too many iterations as folks have asked for it to go in different 
> directions, and we went in those directions to find that they were the 
> wrong directions.

Yes, it's iterating because the context is missing.

> Anyway, for the record, this latest patch series 
> adheres to the direction you suggested we take last month: 
> http://lists.nongnu.org/archive/html/qemu-devel/2013-05/msg04275.html

No, my last suggestion was to just do bdrv_aio_write() within the device
itself.  I realize there is some complexity because libtpms is threaded
and this is exactly why the context matters so much.

But code isn't merged unless it's useful on its own.  This code is not
useful on its own.

Regards,

Anthony Liguori

>
> -- 
> Regards,
> Corey Bryant
>
>>>
>>> --
>>> Regards,
>>> Corey Bryant
>>>
>>>>>
>>>>> --
>>>>> Regards,
>>>>> Corey Bryant
>>>>>
>>>>>>>
>>>>>>> qemu-system-x86_64 ...
>>>>>>> -drive file=/path/to/nvram.qcow2,id=drive-nvram0-0-0
>>>>>>> -tpmdev libtpms,id=tpm-tpm0
>>>>>>> -device tpm-tis,tpmdev=tpm-tpm0,id=tpm0,drive=drive-nvram0-0-0
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Corey
>>>>>>>
>>>>>>> Corey Bryant (3):
>>>>>>>      nvram: Add TPM NVRAM implementation
>>>>>>>      nvram: Add tpm-tis drive support
>>>>>>>      TPM NVRAM test
>>>>>>>
>>>>>>>     hw/tpm/Makefile.objs     |    1 +
>>>>>>>     hw/tpm/tpm_int.h         |    2 +
>>>>>>>     hw/tpm/tpm_nvram.c       |  324 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>     hw/tpm/tpm_nvram.h       |   25 ++++
>>>>>>>     hw/tpm/tpm_passthrough.c |   85 ++++++++++++
>>>>>>>     hw/tpm/tpm_tis.c         |    8 +
>>>>>>>     6 files changed, 445 insertions(+), 0 deletions(-)
>>>>>>>     create mode 100644 hw/tpm/tpm_nvram.c
>>>>>>>     create mode 100644 hw/tpm/tpm_nvram.h
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>
>>
>>