* [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11)
@ 2017-11-10 14:11 Stefan Berger
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 01/13] tpm_tis: convert uint32_t to size_t Stefan Berger
` (13 more replies)
0 siblings, 14 replies; 33+ messages in thread
From: Stefan Berger @ 2017-11-10 14:11 UTC (permalink / raw)
To: qemu-devel, marcandre.lureau; +Cc: amarnath.valluri, Stefan Berger
This set of patches implements support for migrating the state of the
external 'swtpm' TPM emulator as well as that of the emulated device
interfaces. I have primarily tested this with the TIS and TPM 1.2 so
far, but it also seems to work with TPM 2.
The TIS is simplified first by reducing the number of buffers and read
and write offsets into these buffers. Following the state machine of the
TIS, a single buffer and r/w offset is enough for all localities since
only one locality can ever be active.
This series applies on top of my tpm-next branch.
One of the challenges that is addressed by this set of patches is the fact
that the TPM emulator may be processing a command while the state
serialization of the devices is supposed to happen. A necessary first step
has been implemented here that ensures that a response has been received
from the exernal emulator and the bottom half function, which delivers the
response and adjusts device registers (TIS or CRB), has been executed,
before the device's state is serialized.
A subsequent extension may need to address the live migration loop and delay
the serialization of devices until the response from the external TPM has
been received. Though the likelihood that someone executes a long-lasting
TPM command while this is occurring is certainly rare.
Stefan
Stefan Berger (13):
tpm_tis: convert uint32_t to size_t
tpm_tis: limit size of buffer from backend
tpm_tis: remove TPMSizeBuffer usage
tpm_tis: move buffers from localities into common location
tpm_tis: merge read and write buffer into single buffer
tpm_tis: move r/w_offsets to TPMState
tpm_tis: merge r/w_offset into rw_offset
tpm: Implement tpm_sized_buffer_reset
tpm: Introduce condition to notify waiters of completed command
tpm: Introduce condition in TPM backend for notification
tpm: implement tpm_backend_wait_cmd_completed
tpm: extend TPM emulator with state migration support
tpm_tis: extend TPM TIS with state migration support
backends/tpm.c | 29 +++++
hw/tpm/tpm_emulator.c | 303 +++++++++++++++++++++++++++++++++++++++++--
hw/tpm/tpm_tis.c | 216 +++++++++++++++++-------------
hw/tpm/tpm_util.c | 7 +
hw/tpm/tpm_util.h | 7 +
include/sysemu/tpm_backend.h | 22 ++++
6 files changed, 483 insertions(+), 101 deletions(-)
--
2.5.5
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v3 01/13] tpm_tis: convert uint32_t to size_t
2017-11-10 14:11 [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Stefan Berger
@ 2017-11-10 14:11 ` Stefan Berger
2017-12-21 14:11 ` Marc-André Lureau
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 02/13] tpm_tis: limit size of buffer from backend Stefan Berger
` (12 subsequent siblings)
13 siblings, 1 reply; 33+ messages in thread
From: Stefan Berger @ 2017-11-10 14:11 UTC (permalink / raw)
To: qemu-devel, marcandre.lureau; +Cc: amarnath.valluri, Stefan Berger
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
hw/tpm/tpm_tis.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index dd43630..69fe531 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -974,7 +974,7 @@ static const MemoryRegionOps tpm_tis_memory_ops = {
},
};
-static int tpm_tis_do_startup_tpm(TPMState *s, uint32_t buffersize)
+static int tpm_tis_do_startup_tpm(TPMState *s, size_t buffersize)
{
return tpm_backend_startup_tpm(s->be_driver, buffersize);
}
--
2.5.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v3 02/13] tpm_tis: limit size of buffer from backend
2017-11-10 14:11 [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Stefan Berger
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 01/13] tpm_tis: convert uint32_t to size_t Stefan Berger
@ 2017-11-10 14:11 ` Stefan Berger
2017-12-21 14:11 ` Marc-André Lureau
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 03/13] tpm_tis: remove TPMSizeBuffer usage Stefan Berger
` (11 subsequent siblings)
13 siblings, 1 reply; 33+ messages in thread
From: Stefan Berger @ 2017-11-10 14:11 UTC (permalink / raw)
To: qemu-devel, marcandre.lureau; +Cc: amarnath.valluri, Stefan Berger
This is a preparatory patch for the subsequent ones where we
get rid of the flexibility of supporting any kind of buffer size
that the backend may support. We keep the size at 4096, which is
also the size the external emulator supports. So, limit the size
of the buffer we can support and pass it back to the backend.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
hw/tpm/tpm_tis.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 69fe531..90c6df2 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -1008,7 +1008,8 @@ static void tpm_tis_reset(DeviceState *dev)
int c;
s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver);
- s->be_buffer_size = tpm_backend_get_buffer_size(s->be_driver);
+ s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver),
+ TPM_TIS_BUFFER_MAX);
tpm_backend_reset(s->be_driver);
@@ -1040,7 +1041,7 @@ static void tpm_tis_reset(DeviceState *dev)
tpm_tis_realloc_buffer(&s->loc[c].r_buffer, s->be_buffer_size);
}
- tpm_tis_do_startup_tpm(s, 0);
+ tpm_tis_do_startup_tpm(s, s->be_buffer_size);
}
static const VMStateDescription vmstate_tpm_tis = {
--
2.5.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v3 03/13] tpm_tis: remove TPMSizeBuffer usage
2017-11-10 14:11 [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Stefan Berger
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 01/13] tpm_tis: convert uint32_t to size_t Stefan Berger
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 02/13] tpm_tis: limit size of buffer from backend Stefan Berger
@ 2017-11-10 14:11 ` Stefan Berger
2017-12-21 14:11 ` Marc-André Lureau
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 04/13] tpm_tis: move buffers from localities into common location Stefan Berger
` (10 subsequent siblings)
13 siblings, 1 reply; 33+ messages in thread
From: Stefan Berger @ 2017-11-10 14:11 UTC (permalink / raw)
To: qemu-devel, marcandre.lureau; +Cc: amarnath.valluri, Stefan Berger
Remove usage of TPMSizeBuffer. The size of the buffers is limited now
by s->be_buffer_size, which is the size of the buffer the TIS has
negotiated with the backend.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
hw/tpm/tpm_tis.c | 68 ++++++++++++++++++++++++--------------------------------
1 file changed, 29 insertions(+), 39 deletions(-)
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 90c6df2..ddfcfc9 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -64,8 +64,8 @@ typedef struct TPMLocality {
uint16_t w_offset;
uint16_t r_offset;
- TPMSizedBuffer w_buffer;
- TPMSizedBuffer r_buffer;
+ unsigned char w_buffer[TPM_TIS_BUFFER_MAX];
+ unsigned char r_buffer[TPM_TIS_BUFFER_MAX];
} TPMLocality;
typedef struct TPMState {
@@ -215,23 +215,19 @@ static uint8_t tpm_tis_locality_from_addr(hwaddr addr)
return (uint8_t)((addr >> TPM_TIS_LOCALITY_SHIFT) & 0x7);
}
-static uint32_t tpm_tis_get_size_from_buffer(const TPMSizedBuffer *sb)
-{
- return tpm_cmd_get_size(sb->buffer);
-}
-
-static void tpm_tis_show_buffer(const TPMSizedBuffer *sb, const char *string)
+static void tpm_tis_show_buffer(const unsigned char *buffer,
+ size_t buffer_size, const char *string)
{
#ifdef DEBUG_TIS
uint32_t len, i;
- len = tpm_tis_get_size_from_buffer(sb);
+ len = MIN(tpm_cmd_get_size(buffer), buffer_size);
DPRINTF("tpm_tis: %s length = %d\n", string, len);
for (i = 0; i < len; i++) {
if (i && !(i % 16)) {
DPRINTF("\n");
}
- DPRINTF("%.2X ", sb->buffer[i]);
+ DPRINTF("%.2X ", buffer[i]);
}
DPRINTF("\n");
#endif
@@ -263,7 +259,8 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
{
TPMLocality *locty_data = &s->loc[locty];
- tpm_tis_show_buffer(&s->loc[locty].w_buffer, "tpm_tis: To TPM");
+ tpm_tis_show_buffer(s->loc[locty].w_buffer, s->be_buffer_size,
+ "tpm_tis: To TPM");
/*
* w_offset serves as length indicator for length of data;
@@ -273,10 +270,10 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
s->cmd = (TPMBackendCmd) {
.locty = locty,
- .in = locty_data->w_buffer.buffer,
+ .in = locty_data->w_buffer,
.in_len = locty_data->w_offset,
- .out = locty_data->r_buffer.buffer,
- .out_len = locty_data->r_buffer.size
+ .out = locty_data->r_buffer,
+ .out_len = s->be_buffer_size,
};
tpm_backend_deliver_request(s->be_driver, &s->cmd);
@@ -427,7 +424,8 @@ static void tpm_tis_request_completed(TPMIf *ti)
s->loc[locty].r_offset = 0;
s->loc[locty].w_offset = 0;
- tpm_tis_show_buffer(&s->loc[locty].r_buffer, "tpm_tis: From TPM");
+ tpm_tis_show_buffer(s->loc[locty].r_buffer, s->be_buffer_size,
+ "tpm_tis: From TPM");
if (TPM_TIS_IS_VALID_LOCTY(s->next_locty)) {
tpm_tis_abort(s, locty);
@@ -446,9 +444,10 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty)
uint16_t len;
if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) {
- len = tpm_tis_get_size_from_buffer(&s->loc[locty].r_buffer);
+ len = MIN(tpm_cmd_get_size(&s->loc[locty].r_buffer),
+ s->be_buffer_size);
- ret = s->loc[locty].r_buffer.buffer[s->loc[locty].r_offset++];
+ ret = s->loc[locty].r_buffer[s->loc[locty].r_offset++];
if (s->loc[locty].r_offset >= len) {
/* got last byte */
tpm_tis_sts_set(&s->loc[locty], TPM_TIS_STS_VALID);
@@ -494,11 +493,12 @@ static void tpm_tis_dump_state(void *opaque, hwaddr addr)
"tpm_tis: result buffer : ",
s->loc[locty].r_offset);
for (idx = 0;
- idx < tpm_tis_get_size_from_buffer(&s->loc[locty].r_buffer);
+ idx < MIN(tpm_cmd_get_size(&s->loc[locty].r_buffer),
+ s->be_buffer_size);
idx++) {
DPRINTF("%c%02x%s",
s->loc[locty].r_offset == idx ? '>' : ' ',
- s->loc[locty].r_buffer.buffer[idx],
+ s->loc[locty].r_buffer[idx],
((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
}
DPRINTF("\n"
@@ -506,11 +506,12 @@ static void tpm_tis_dump_state(void *opaque, hwaddr addr)
"tpm_tis: request buffer: ",
s->loc[locty].w_offset);
for (idx = 0;
- idx < tpm_tis_get_size_from_buffer(&s->loc[locty].w_buffer);
+ idx < MIN(tpm_cmd_get_size(s->loc[locty].w_buffer),
+ s->be_buffer_size);
idx++) {
DPRINTF("%c%02x%s",
s->loc[locty].w_offset == idx ? '>' : ' ',
- s->loc[locty].w_buffer.buffer[idx],
+ s->loc[locty].w_buffer[idx],
((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
}
DPRINTF("\n");
@@ -572,11 +573,11 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
if (s->active_locty == locty) {
if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) {
val = TPM_TIS_BURST_COUNT(
- tpm_tis_get_size_from_buffer(&s->loc[locty].r_buffer)
+ MIN(tpm_cmd_get_size(&s->loc[locty].r_buffer),
+ s->be_buffer_size)
- s->loc[locty].r_offset) | s->loc[locty].sts;
} else {
- avail = s->loc[locty].w_buffer.size
- - s->loc[locty].w_offset;
+ avail = s->be_buffer_size - s->loc[locty].w_offset;
/*
* byte-sized reads should not return 0x00 for 0x100
* available bytes.
@@ -924,9 +925,9 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
}
while ((s->loc[locty].sts & TPM_TIS_STS_EXPECT) && size > 0) {
- if (s->loc[locty].w_offset < s->loc[locty].w_buffer.size) {
- s->loc[locty].w_buffer.
- buffer[s->loc[locty].w_offset++] = (uint8_t)val;
+ if (s->loc[locty].w_offset < s->be_buffer_size) {
+ s->loc[locty].w_buffer[s->loc[locty].w_offset++] =
+ (uint8_t)val;
val >>= 8;
size--;
} else {
@@ -940,7 +941,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
/* we have a packet length - see if we have all of it */
bool need_irq = !(s->loc[locty].sts & TPM_TIS_STS_VALID);
- len = tpm_tis_get_size_from_buffer(&s->loc[locty].w_buffer);
+ len = tpm_cmd_get_size(&s->loc[locty].w_buffer);
if (len > s->loc[locty].w_offset) {
tpm_tis_sts_set(&s->loc[locty],
TPM_TIS_STS_EXPECT | TPM_TIS_STS_VALID);
@@ -979,15 +980,6 @@ static int tpm_tis_do_startup_tpm(TPMState *s, size_t buffersize)
return tpm_backend_startup_tpm(s->be_driver, buffersize);
}
-static void tpm_tis_realloc_buffer(TPMSizedBuffer *sb,
- size_t wanted_size)
-{
- if (sb->size != wanted_size) {
- sb->buffer = g_realloc(sb->buffer, wanted_size);
- sb->size = wanted_size;
- }
-}
-
/*
* Get the TPMVersion of the backend device being used
*/
@@ -1036,9 +1028,7 @@ static void tpm_tis_reset(DeviceState *dev)
s->loc[c].state = TPM_TIS_STATE_IDLE;
s->loc[c].w_offset = 0;
- tpm_tis_realloc_buffer(&s->loc[c].w_buffer, s->be_buffer_size);
s->loc[c].r_offset = 0;
- tpm_tis_realloc_buffer(&s->loc[c].r_buffer, s->be_buffer_size);
}
tpm_tis_do_startup_tpm(s, s->be_buffer_size);
--
2.5.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v3 04/13] tpm_tis: move buffers from localities into common location
2017-11-10 14:11 [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Stefan Berger
` (2 preceding siblings ...)
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 03/13] tpm_tis: remove TPMSizeBuffer usage Stefan Berger
@ 2017-11-10 14:11 ` Stefan Berger
2017-12-21 14:11 ` Marc-André Lureau
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 05/13] tpm_tis: merge read and write buffer into single buffer Stefan Berger
` (9 subsequent siblings)
13 siblings, 1 reply; 33+ messages in thread
From: Stefan Berger @ 2017-11-10 14:11 UTC (permalink / raw)
To: qemu-devel, marcandre.lureau; +Cc: amarnath.valluri, Stefan Berger
One read buffer and one write buffer is sufficient for all localities.
The localities cannot all be active at the same time, and only the active
locality can use the r/w buffers. Inactive localities will require the
COMMAND_READY flag to be set on the STS register to move to the READY
state, which then enables access to using the buffer for writing of a
command, while all other localities are inactive.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
hw/tpm/tpm_tis.c | 34 +++++++++++++++-------------------
1 file changed, 15 insertions(+), 19 deletions(-)
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index ddfcfc9..f6f5f17 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -64,16 +64,14 @@ typedef struct TPMLocality {
uint16_t w_offset;
uint16_t r_offset;
- unsigned char w_buffer[TPM_TIS_BUFFER_MAX];
- unsigned char r_buffer[TPM_TIS_BUFFER_MAX];
} TPMLocality;
typedef struct TPMState {
ISADevice busdev;
MemoryRegion mmio;
- uint32_t offset;
- uint8_t buf[TPM_TIS_BUFFER_MAX];
+ unsigned char w_buffer[TPM_TIS_BUFFER_MAX];
+ unsigned char r_buffer[TPM_TIS_BUFFER_MAX];
uint8_t active_locty;
uint8_t aborting_locty;
@@ -259,7 +257,7 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
{
TPMLocality *locty_data = &s->loc[locty];
- tpm_tis_show_buffer(s->loc[locty].w_buffer, s->be_buffer_size,
+ tpm_tis_show_buffer(s->w_buffer, s->be_buffer_size,
"tpm_tis: To TPM");
/*
@@ -270,9 +268,9 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
s->cmd = (TPMBackendCmd) {
.locty = locty,
- .in = locty_data->w_buffer,
+ .in = s->w_buffer,
.in_len = locty_data->w_offset,
- .out = locty_data->r_buffer,
+ .out = s->r_buffer,
.out_len = s->be_buffer_size,
};
@@ -424,7 +422,7 @@ static void tpm_tis_request_completed(TPMIf *ti)
s->loc[locty].r_offset = 0;
s->loc[locty].w_offset = 0;
- tpm_tis_show_buffer(s->loc[locty].r_buffer, s->be_buffer_size,
+ tpm_tis_show_buffer(s->r_buffer, s->be_buffer_size,
"tpm_tis: From TPM");
if (TPM_TIS_IS_VALID_LOCTY(s->next_locty)) {
@@ -444,10 +442,10 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty)
uint16_t len;
if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) {
- len = MIN(tpm_cmd_get_size(&s->loc[locty].r_buffer),
+ len = MIN(tpm_cmd_get_size(&s->r_buffer),
s->be_buffer_size);
- ret = s->loc[locty].r_buffer[s->loc[locty].r_offset++];
+ ret = s->r_buffer[s->loc[locty].r_offset++];
if (s->loc[locty].r_offset >= len) {
/* got last byte */
tpm_tis_sts_set(&s->loc[locty], TPM_TIS_STS_VALID);
@@ -493,12 +491,11 @@ static void tpm_tis_dump_state(void *opaque, hwaddr addr)
"tpm_tis: result buffer : ",
s->loc[locty].r_offset);
for (idx = 0;
- idx < MIN(tpm_cmd_get_size(&s->loc[locty].r_buffer),
- s->be_buffer_size);
+ idx < MIN(tpm_cmd_get_size(&s->r_buffer), s->be_buffer_size);
idx++) {
DPRINTF("%c%02x%s",
s->loc[locty].r_offset == idx ? '>' : ' ',
- s->loc[locty].r_buffer[idx],
+ s->r_buffer[idx],
((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
}
DPRINTF("\n"
@@ -506,12 +503,11 @@ static void tpm_tis_dump_state(void *opaque, hwaddr addr)
"tpm_tis: request buffer: ",
s->loc[locty].w_offset);
for (idx = 0;
- idx < MIN(tpm_cmd_get_size(s->loc[locty].w_buffer),
- s->be_buffer_size);
+ idx < MIN(tpm_cmd_get_size(s->w_buffer), s->be_buffer_size);
idx++) {
DPRINTF("%c%02x%s",
s->loc[locty].w_offset == idx ? '>' : ' ',
- s->loc[locty].w_buffer[idx],
+ s->w_buffer[idx],
((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
}
DPRINTF("\n");
@@ -573,7 +569,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
if (s->active_locty == locty) {
if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) {
val = TPM_TIS_BURST_COUNT(
- MIN(tpm_cmd_get_size(&s->loc[locty].r_buffer),
+ MIN(tpm_cmd_get_size(&s->r_buffer),
s->be_buffer_size)
- s->loc[locty].r_offset) | s->loc[locty].sts;
} else {
@@ -926,7 +922,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
while ((s->loc[locty].sts & TPM_TIS_STS_EXPECT) && size > 0) {
if (s->loc[locty].w_offset < s->be_buffer_size) {
- s->loc[locty].w_buffer[s->loc[locty].w_offset++] =
+ s->w_buffer[s->loc[locty].w_offset++] =
(uint8_t)val;
val >>= 8;
size--;
@@ -941,7 +937,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
/* we have a packet length - see if we have all of it */
bool need_irq = !(s->loc[locty].sts & TPM_TIS_STS_VALID);
- len = tpm_cmd_get_size(&s->loc[locty].w_buffer);
+ len = tpm_cmd_get_size(&s->w_buffer);
if (len > s->loc[locty].w_offset) {
tpm_tis_sts_set(&s->loc[locty],
TPM_TIS_STS_EXPECT | TPM_TIS_STS_VALID);
--
2.5.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v3 05/13] tpm_tis: merge read and write buffer into single buffer
2017-11-10 14:11 [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Stefan Berger
` (3 preceding siblings ...)
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 04/13] tpm_tis: move buffers from localities into common location Stefan Berger
@ 2017-11-10 14:11 ` Stefan Berger
2017-12-21 14:41 ` Marc-André Lureau
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 06/13] tpm_tis: move r/w_offsets to TPMState Stefan Berger
` (8 subsequent siblings)
13 siblings, 1 reply; 33+ messages in thread
From: Stefan Berger @ 2017-11-10 14:11 UTC (permalink / raw)
To: qemu-devel, marcandre.lureau; +Cc: amarnath.valluri, Stefan Berger
Since we can only be in read or write mode, we can merge the buffers
into a single buffer.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
hw/tpm/tpm_tis.c | 29 ++++++++++++++---------------
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index f6f5f17..0b6dd7f 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -70,8 +70,7 @@ typedef struct TPMState {
ISADevice busdev;
MemoryRegion mmio;
- unsigned char w_buffer[TPM_TIS_BUFFER_MAX];
- unsigned char r_buffer[TPM_TIS_BUFFER_MAX];
+ unsigned char buffer[TPM_TIS_BUFFER_MAX];
uint8_t active_locty;
uint8_t aborting_locty;
@@ -257,7 +256,7 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
{
TPMLocality *locty_data = &s->loc[locty];
- tpm_tis_show_buffer(s->w_buffer, s->be_buffer_size,
+ tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
"tpm_tis: To TPM");
/*
@@ -268,9 +267,9 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
s->cmd = (TPMBackendCmd) {
.locty = locty,
- .in = s->w_buffer,
+ .in = s->buffer,
.in_len = locty_data->w_offset,
- .out = s->r_buffer,
+ .out = s->buffer,
.out_len = s->be_buffer_size,
};
@@ -422,7 +421,7 @@ static void tpm_tis_request_completed(TPMIf *ti)
s->loc[locty].r_offset = 0;
s->loc[locty].w_offset = 0;
- tpm_tis_show_buffer(s->r_buffer, s->be_buffer_size,
+ tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
"tpm_tis: From TPM");
if (TPM_TIS_IS_VALID_LOCTY(s->next_locty)) {
@@ -442,10 +441,10 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty)
uint16_t len;
if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) {
- len = MIN(tpm_cmd_get_size(&s->r_buffer),
+ len = MIN(tpm_cmd_get_size(&s->buffer),
s->be_buffer_size);
- ret = s->r_buffer[s->loc[locty].r_offset++];
+ ret = s->buffer[s->loc[locty].r_offset++];
if (s->loc[locty].r_offset >= len) {
/* got last byte */
tpm_tis_sts_set(&s->loc[locty], TPM_TIS_STS_VALID);
@@ -491,11 +490,11 @@ static void tpm_tis_dump_state(void *opaque, hwaddr addr)
"tpm_tis: result buffer : ",
s->loc[locty].r_offset);
for (idx = 0;
- idx < MIN(tpm_cmd_get_size(&s->r_buffer), s->be_buffer_size);
+ idx < MIN(tpm_cmd_get_size(&s->buffer), s->be_buffer_size);
idx++) {
DPRINTF("%c%02x%s",
s->loc[locty].r_offset == idx ? '>' : ' ',
- s->r_buffer[idx],
+ s->buffer[idx],
((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
}
DPRINTF("\n"
@@ -503,11 +502,11 @@ static void tpm_tis_dump_state(void *opaque, hwaddr addr)
"tpm_tis: request buffer: ",
s->loc[locty].w_offset);
for (idx = 0;
- idx < MIN(tpm_cmd_get_size(s->w_buffer), s->be_buffer_size);
+ idx < MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size);
idx++) {
DPRINTF("%c%02x%s",
s->loc[locty].w_offset == idx ? '>' : ' ',
- s->w_buffer[idx],
+ s->buffer[idx],
((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
}
DPRINTF("\n");
@@ -569,7 +568,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
if (s->active_locty == locty) {
if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) {
val = TPM_TIS_BURST_COUNT(
- MIN(tpm_cmd_get_size(&s->r_buffer),
+ MIN(tpm_cmd_get_size(&s->buffer),
s->be_buffer_size)
- s->loc[locty].r_offset) | s->loc[locty].sts;
} else {
@@ -922,7 +921,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
while ((s->loc[locty].sts & TPM_TIS_STS_EXPECT) && size > 0) {
if (s->loc[locty].w_offset < s->be_buffer_size) {
- s->w_buffer[s->loc[locty].w_offset++] =
+ s->buffer[s->loc[locty].w_offset++] =
(uint8_t)val;
val >>= 8;
size--;
@@ -937,7 +936,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
/* we have a packet length - see if we have all of it */
bool need_irq = !(s->loc[locty].sts & TPM_TIS_STS_VALID);
- len = tpm_cmd_get_size(&s->w_buffer);
+ len = tpm_cmd_get_size(&s->buffer);
if (len > s->loc[locty].w_offset) {
tpm_tis_sts_set(&s->loc[locty],
TPM_TIS_STS_EXPECT | TPM_TIS_STS_VALID);
--
2.5.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v3 06/13] tpm_tis: move r/w_offsets to TPMState
2017-11-10 14:11 [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Stefan Berger
` (4 preceding siblings ...)
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 05/13] tpm_tis: merge read and write buffer into single buffer Stefan Berger
@ 2017-11-10 14:11 ` Stefan Berger
2017-12-21 14:41 ` Marc-André Lureau
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 07/13] tpm_tis: merge r/w_offset into rw_offset Stefan Berger
` (7 subsequent siblings)
13 siblings, 1 reply; 33+ messages in thread
From: Stefan Berger @ 2017-11-10 14:11 UTC (permalink / raw)
To: qemu-devel, marcandre.lureau; +Cc: amarnath.valluri, Stefan Berger
Now that we have a single buffer, we also only need a single set of
read/write offsets into that buffer. This works since only one
locality can be active.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
hw/tpm/tpm_tis.c | 57 +++++++++++++++++++++++++++-----------------------------
1 file changed, 27 insertions(+), 30 deletions(-)
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 0b6dd7f..dfdddd3 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -61,9 +61,6 @@ typedef struct TPMLocality {
uint32_t iface_id;
uint32_t inte;
uint32_t ints;
-
- uint16_t w_offset;
- uint16_t r_offset;
} TPMLocality;
typedef struct TPMState {
@@ -71,6 +68,8 @@ typedef struct TPMState {
MemoryRegion mmio;
unsigned char buffer[TPM_TIS_BUFFER_MAX];
+ uint16_t w_offset;
+ uint16_t r_offset;
uint8_t active_locty;
uint8_t aborting_locty;
@@ -254,8 +253,6 @@ static void tpm_tis_sts_set(TPMLocality *l, uint32_t flags)
*/
static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
{
- TPMLocality *locty_data = &s->loc[locty];
-
tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
"tpm_tis: To TPM");
@@ -268,7 +265,7 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
s->cmd = (TPMBackendCmd) {
.locty = locty,
.in = s->buffer,
- .in_len = locty_data->w_offset,
+ .in_len = s->w_offset,
.out = s->buffer,
.out_len = s->be_buffer_size,
};
@@ -350,8 +347,8 @@ static void tpm_tis_new_active_locality(TPMState *s, uint8_t new_active_locty)
/* abort -- this function switches the locality */
static void tpm_tis_abort(TPMState *s, uint8_t locty)
{
- s->loc[locty].r_offset = 0;
- s->loc[locty].w_offset = 0;
+ s->r_offset = 0;
+ s->w_offset = 0;
DPRINTF("tpm_tis: tis_abort: new active locality is %d\n", s->next_locty);
@@ -418,8 +415,8 @@ static void tpm_tis_request_completed(TPMIf *ti)
tpm_tis_sts_set(&s->loc[locty],
TPM_TIS_STS_VALID | TPM_TIS_STS_DATA_AVAILABLE);
s->loc[locty].state = TPM_TIS_STATE_COMPLETION;
- s->loc[locty].r_offset = 0;
- s->loc[locty].w_offset = 0;
+ s->r_offset = 0;
+ s->w_offset = 0;
tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
"tpm_tis: From TPM");
@@ -444,14 +441,14 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty)
len = MIN(tpm_cmd_get_size(&s->buffer),
s->be_buffer_size);
- ret = s->buffer[s->loc[locty].r_offset++];
- if (s->loc[locty].r_offset >= len) {
+ ret = s->buffer[s->r_offset++];
+ if (s->r_offset >= len) {
/* got last byte */
tpm_tis_sts_set(&s->loc[locty], TPM_TIS_STS_VALID);
tpm_tis_raise_irq(s, locty, TPM_TIS_INT_STS_VALID);
}
DPRINTF("tpm_tis: tpm_tis_data_read byte 0x%02x [%d]\n",
- ret, s->loc[locty].r_offset - 1);
+ ret, s->r_offset - 1);
}
return ret;
@@ -488,24 +485,24 @@ static void tpm_tis_dump_state(void *opaque, hwaddr addr)
DPRINTF("tpm_tis: read offset : %d\n"
"tpm_tis: result buffer : ",
- s->loc[locty].r_offset);
+ s->r_offset);
for (idx = 0;
idx < MIN(tpm_cmd_get_size(&s->buffer), s->be_buffer_size);
idx++) {
DPRINTF("%c%02x%s",
- s->loc[locty].r_offset == idx ? '>' : ' ',
+ s->r_offset == idx ? '>' : ' ',
s->buffer[idx],
((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
}
DPRINTF("\n"
"tpm_tis: write offset : %d\n"
"tpm_tis: request buffer: ",
- s->loc[locty].w_offset);
+ s->w_offset);
for (idx = 0;
idx < MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size);
idx++) {
DPRINTF("%c%02x%s",
- s->loc[locty].w_offset == idx ? '>' : ' ',
+ s->w_offset == idx ? '>' : ' ',
s->buffer[idx],
((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
}
@@ -570,9 +567,9 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
val = TPM_TIS_BURST_COUNT(
MIN(tpm_cmd_get_size(&s->buffer),
s->be_buffer_size)
- - s->loc[locty].r_offset) | s->loc[locty].sts;
+ - s->r_offset) | s->loc[locty].sts;
} else {
- avail = s->be_buffer_size - s->loc[locty].w_offset;
+ avail = s->be_buffer_size - s->w_offset;
/*
* byte-sized reads should not return 0x00 for 0x100
* available bytes.
@@ -836,8 +833,8 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
switch (s->loc[locty].state) {
case TPM_TIS_STATE_READY:
- s->loc[locty].w_offset = 0;
- s->loc[locty].r_offset = 0;
+ s->w_offset = 0;
+ s->r_offset = 0;
break;
case TPM_TIS_STATE_IDLE:
@@ -855,8 +852,8 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
break;
case TPM_TIS_STATE_COMPLETION:
- s->loc[locty].w_offset = 0;
- s->loc[locty].r_offset = 0;
+ s->w_offset = 0;
+ s->r_offset = 0;
/* shortcut to ready state with C/R set */
s->loc[locty].state = TPM_TIS_STATE_READY;
if (!(s->loc[locty].sts & TPM_TIS_STS_COMMAND_READY)) {
@@ -882,7 +879,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
} else if (val == TPM_TIS_STS_RESPONSE_RETRY) {
switch (s->loc[locty].state) {
case TPM_TIS_STATE_COMPLETION:
- s->loc[locty].r_offset = 0;
+ s->r_offset = 0;
tpm_tis_sts_set(&s->loc[locty],
TPM_TIS_STS_VALID|
TPM_TIS_STS_DATA_AVAILABLE);
@@ -920,8 +917,8 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
}
while ((s->loc[locty].sts & TPM_TIS_STS_EXPECT) && size > 0) {
- if (s->loc[locty].w_offset < s->be_buffer_size) {
- s->buffer[s->loc[locty].w_offset++] =
+ if (s->w_offset < s->be_buffer_size) {
+ s->buffer[s->w_offset++] =
(uint8_t)val;
val >>= 8;
size--;
@@ -931,13 +928,13 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
}
/* check for complete packet */
- if (s->loc[locty].w_offset > 5 &&
+ if (s->w_offset > 5 &&
(s->loc[locty].sts & TPM_TIS_STS_EXPECT)) {
/* we have a packet length - see if we have all of it */
bool need_irq = !(s->loc[locty].sts & TPM_TIS_STS_VALID);
len = tpm_cmd_get_size(&s->buffer);
- if (len > s->loc[locty].w_offset) {
+ if (len > s->w_offset) {
tpm_tis_sts_set(&s->loc[locty],
TPM_TIS_STS_EXPECT | TPM_TIS_STS_VALID);
} else {
@@ -1022,8 +1019,8 @@ static void tpm_tis_reset(DeviceState *dev)
s->loc[c].ints = 0;
s->loc[c].state = TPM_TIS_STATE_IDLE;
- s->loc[c].w_offset = 0;
- s->loc[c].r_offset = 0;
+ s->w_offset = 0;
+ s->r_offset = 0;
}
tpm_tis_do_startup_tpm(s, s->be_buffer_size);
--
2.5.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v3 07/13] tpm_tis: merge r/w_offset into rw_offset
2017-11-10 14:11 [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Stefan Berger
` (5 preceding siblings ...)
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 06/13] tpm_tis: move r/w_offsets to TPMState Stefan Berger
@ 2017-11-10 14:11 ` Stefan Berger
2017-12-21 14:41 ` Marc-André Lureau
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 08/13] tpm: Implement tpm_sized_buffer_reset Stefan Berger
` (6 subsequent siblings)
13 siblings, 1 reply; 33+ messages in thread
From: Stefan Berger @ 2017-11-10 14:11 UTC (permalink / raw)
To: qemu-devel, marcandre.lureau; +Cc: amarnath.valluri, Stefan Berger
We can now merge the r_offset and w_offset into a single rw_offset.
This is possible since when the offset is used for writing in
RECEPTION state then reads are ignore. Conversly, when the offset
is used for reading when in COMPLETION state, then writes are
ignored.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
hw/tpm/tpm_tis.c | 60 ++++++++++++++++++++------------------------------------
1 file changed, 21 insertions(+), 39 deletions(-)
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index dfdddd3..7d7e2cd 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -68,8 +68,7 @@ typedef struct TPMState {
MemoryRegion mmio;
unsigned char buffer[TPM_TIS_BUFFER_MAX];
- uint16_t w_offset;
- uint16_t r_offset;
+ uint16_t rw_offset;
uint8_t active_locty;
uint8_t aborting_locty;
@@ -257,7 +256,7 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
"tpm_tis: To TPM");
/*
- * w_offset serves as length indicator for length of data;
+ * rw_offset serves as length indicator for length of data;
* it's reset when the response comes back
*/
s->loc[locty].state = TPM_TIS_STATE_EXECUTION;
@@ -265,7 +264,7 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
s->cmd = (TPMBackendCmd) {
.locty = locty,
.in = s->buffer,
- .in_len = s->w_offset,
+ .in_len = s->rw_offset,
.out = s->buffer,
.out_len = s->be_buffer_size,
};
@@ -347,8 +346,7 @@ static void tpm_tis_new_active_locality(TPMState *s, uint8_t new_active_locty)
/* abort -- this function switches the locality */
static void tpm_tis_abort(TPMState *s, uint8_t locty)
{
- s->r_offset = 0;
- s->w_offset = 0;
+ s->rw_offset = 0;
DPRINTF("tpm_tis: tis_abort: new active locality is %d\n", s->next_locty);
@@ -415,8 +413,7 @@ static void tpm_tis_request_completed(TPMIf *ti)
tpm_tis_sts_set(&s->loc[locty],
TPM_TIS_STS_VALID | TPM_TIS_STS_DATA_AVAILABLE);
s->loc[locty].state = TPM_TIS_STATE_COMPLETION;
- s->r_offset = 0;
- s->w_offset = 0;
+ s->rw_offset = 0;
tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
"tpm_tis: From TPM");
@@ -441,14 +438,14 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty)
len = MIN(tpm_cmd_get_size(&s->buffer),
s->be_buffer_size);
- ret = s->buffer[s->r_offset++];
- if (s->r_offset >= len) {
+ ret = s->buffer[s->rw_offset++];
+ if (s->rw_offset >= len) {
/* got last byte */
tpm_tis_sts_set(&s->loc[locty], TPM_TIS_STS_VALID);
tpm_tis_raise_irq(s, locty, TPM_TIS_INT_STS_VALID);
}
DPRINTF("tpm_tis: tpm_tis_data_read byte 0x%02x [%d]\n",
- ret, s->r_offset - 1);
+ ret, s->rw_offset - 1);
}
return ret;
@@ -483,26 +480,14 @@ static void tpm_tis_dump_state(void *opaque, hwaddr addr)
(int)tpm_tis_mmio_read(opaque, base + regs[idx], 4));
}
- DPRINTF("tpm_tis: read offset : %d\n"
+ DPRINTF("tpm_tis: r/w offset : %d\n"
"tpm_tis: result buffer : ",
- s->r_offset);
+ s->rw_offset);
for (idx = 0;
idx < MIN(tpm_cmd_get_size(&s->buffer), s->be_buffer_size);
idx++) {
DPRINTF("%c%02x%s",
- s->r_offset == idx ? '>' : ' ',
- s->buffer[idx],
- ((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
- }
- DPRINTF("\n"
- "tpm_tis: write offset : %d\n"
- "tpm_tis: request buffer: ",
- s->w_offset);
- for (idx = 0;
- idx < MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size);
- idx++) {
- DPRINTF("%c%02x%s",
- s->w_offset == idx ? '>' : ' ',
+ s->rw_offset == idx ? '>' : ' ',
s->buffer[idx],
((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
}
@@ -567,9 +552,9 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
val = TPM_TIS_BURST_COUNT(
MIN(tpm_cmd_get_size(&s->buffer),
s->be_buffer_size)
- - s->r_offset) | s->loc[locty].sts;
+ - s->rw_offset) | s->loc[locty].sts;
} else {
- avail = s->be_buffer_size - s->w_offset;
+ avail = s->be_buffer_size - s->rw_offset;
/*
* byte-sized reads should not return 0x00 for 0x100
* available bytes.
@@ -833,8 +818,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
switch (s->loc[locty].state) {
case TPM_TIS_STATE_READY:
- s->w_offset = 0;
- s->r_offset = 0;
+ s->rw_offset = 0;
break;
case TPM_TIS_STATE_IDLE:
@@ -852,8 +836,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
break;
case TPM_TIS_STATE_COMPLETION:
- s->w_offset = 0;
- s->r_offset = 0;
+ s->rw_offset = 0;
/* shortcut to ready state with C/R set */
s->loc[locty].state = TPM_TIS_STATE_READY;
if (!(s->loc[locty].sts & TPM_TIS_STS_COMMAND_READY)) {
@@ -879,7 +862,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
} else if (val == TPM_TIS_STS_RESPONSE_RETRY) {
switch (s->loc[locty].state) {
case TPM_TIS_STATE_COMPLETION:
- s->r_offset = 0;
+ s->rw_offset = 0;
tpm_tis_sts_set(&s->loc[locty],
TPM_TIS_STS_VALID|
TPM_TIS_STS_DATA_AVAILABLE);
@@ -917,8 +900,8 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
}
while ((s->loc[locty].sts & TPM_TIS_STS_EXPECT) && size > 0) {
- if (s->w_offset < s->be_buffer_size) {
- s->buffer[s->w_offset++] =
+ if (s->rw_offset < s->be_buffer_size) {
+ s->buffer[s->rw_offset++] =
(uint8_t)val;
val >>= 8;
size--;
@@ -928,13 +911,13 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
}
/* check for complete packet */
- if (s->w_offset > 5 &&
+ if (s->rw_offset > 5 &&
(s->loc[locty].sts & TPM_TIS_STS_EXPECT)) {
/* we have a packet length - see if we have all of it */
bool need_irq = !(s->loc[locty].sts & TPM_TIS_STS_VALID);
len = tpm_cmd_get_size(&s->buffer);
- if (len > s->w_offset) {
+ if (len > s->rw_offset) {
tpm_tis_sts_set(&s->loc[locty],
TPM_TIS_STS_EXPECT | TPM_TIS_STS_VALID);
} else {
@@ -1019,8 +1002,7 @@ static void tpm_tis_reset(DeviceState *dev)
s->loc[c].ints = 0;
s->loc[c].state = TPM_TIS_STATE_IDLE;
- s->w_offset = 0;
- s->r_offset = 0;
+ s->rw_offset = 0;
}
tpm_tis_do_startup_tpm(s, s->be_buffer_size);
--
2.5.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v3 08/13] tpm: Implement tpm_sized_buffer_reset
2017-11-10 14:11 [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Stefan Berger
` (6 preceding siblings ...)
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 07/13] tpm_tis: merge r/w_offset into rw_offset Stefan Berger
@ 2017-11-10 14:11 ` Stefan Berger
2017-12-21 14:44 ` Marc-André Lureau
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 09/13] tpm: Introduce condition to notify waiters of completed command Stefan Berger
` (5 subsequent siblings)
13 siblings, 1 reply; 33+ messages in thread
From: Stefan Berger @ 2017-11-10 14:11 UTC (permalink / raw)
To: qemu-devel, marcandre.lureau; +Cc: amarnath.valluri, Stefan Berger
Move the definition of TPMSizedBuffer out of tpm_tis.c into tpm_util.h
and implement tpm_sized_buffer_reset() for the following patches to use.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
hw/tpm/tpm_tis.c | 5 -----
hw/tpm/tpm_util.c | 7 +++++++
hw/tpm/tpm_util.h | 7 +++++++
3 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 7d7e2cd..035c6ef 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -48,11 +48,6 @@ typedef enum {
TPM_TIS_STATE_RECEPTION,
} TPMTISState;
-typedef struct TPMSizedBuffer {
- uint32_t size;
- uint8_t *buffer;
-} TPMSizedBuffer;
-
/* locality data -- all fields are persisted */
typedef struct TPMLocality {
TPMTISState state;
diff --git a/hw/tpm/tpm_util.c b/hw/tpm/tpm_util.c
index a317243..bf97811 100644
--- a/hw/tpm/tpm_util.c
+++ b/hw/tpm/tpm_util.c
@@ -288,3 +288,10 @@ int tpm_util_get_buffer_size(int tpm_fd, TPMVersion tpm_version,
return 0;
}
+
+void tpm_sized_buffer_reset(TPMSizedBuffer *tsb)
+{
+ g_free(tsb->buffer);
+ tsb->buffer = NULL;
+ tsb->size = 0;
+}
diff --git a/hw/tpm/tpm_util.h b/hw/tpm/tpm_util.h
index 1c17e39..26c9613 100644
--- a/hw/tpm/tpm_util.h
+++ b/hw/tpm/tpm_util.h
@@ -39,4 +39,11 @@ static inline uint32_t tpm_cmd_get_size(const void *b)
int tpm_util_get_buffer_size(int tpm_fd, TPMVersion tpm_version,
size_t *buffersize);
+typedef struct TPMSizedBuffer {
+ uint32_t size;
+ uint8_t *buffer;
+} TPMSizedBuffer;
+
+void tpm_sized_buffer_reset(TPMSizedBuffer *tsb);
+
#endif /* TPM_TPM_UTIL_H */
--
2.5.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v3 09/13] tpm: Introduce condition to notify waiters of completed command
2017-11-10 14:11 [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Stefan Berger
` (7 preceding siblings ...)
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 08/13] tpm: Implement tpm_sized_buffer_reset Stefan Berger
@ 2017-11-10 14:11 ` Stefan Berger
2017-12-22 13:24 ` Marc-André Lureau
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 10/13] tpm: Introduce condition in TPM backend for notification Stefan Berger
` (4 subsequent siblings)
13 siblings, 1 reply; 33+ messages in thread
From: Stefan Berger @ 2017-11-10 14:11 UTC (permalink / raw)
To: qemu-devel, marcandre.lureau; +Cc: amarnath.valluri, Stefan Berger
Introduce a lock and a condition to notify anyone waiting for the completion
of the execution of a TPM command by the backend (thread). The backend
uses the condition to signal anyone waiting for command completion.
We need to place the condition in two locations: one is invoked by the
backend thread, the other by the bottom half thread.
We will use the signaling to wait for command completion before VM
suspend.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
hw/tpm/tpm_tis.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 035c6ef..86e9a92 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -80,6 +80,9 @@ typedef struct TPMState {
TPMVersion be_tpm_version;
size_t be_buffer_size;
+
+ QemuMutex state_lock;
+ QemuCond cmd_complete;
} TPMState;
#define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
@@ -405,6 +408,8 @@ static void tpm_tis_request_completed(TPMIf *ti)
}
}
+ qemu_mutex_lock(&s->state_lock);
+
tpm_tis_sts_set(&s->loc[locty],
TPM_TIS_STS_VALID | TPM_TIS_STS_DATA_AVAILABLE);
s->loc[locty].state = TPM_TIS_STATE_COMPLETION;
@@ -419,6 +424,10 @@ static void tpm_tis_request_completed(TPMIf *ti)
tpm_tis_raise_irq(s, locty,
TPM_TIS_INT_DATA_AVAILABLE | TPM_TIS_INT_STS_VALID);
+
+ /* notify of completed command */
+ qemu_cond_signal(&s->cmd_complete);
+ qemu_mutex_unlock(&s->state_lock);
}
/*
@@ -1046,6 +1055,9 @@ static void tpm_tis_initfn(Object *obj)
memory_region_init_io(&s->mmio, OBJECT(s), &tpm_tis_memory_ops,
s, "tpm-tis-mmio",
TPM_TIS_NUM_LOCALITIES << TPM_TIS_LOCALITY_SHIFT);
+
+ qemu_mutex_init(&s->state_lock);
+ qemu_cond_init(&s->cmd_complete);
}
static void tpm_tis_class_init(ObjectClass *klass, void *data)
--
2.5.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v3 10/13] tpm: Introduce condition in TPM backend for notification
2017-11-10 14:11 [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Stefan Berger
` (8 preceding siblings ...)
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 09/13] tpm: Introduce condition to notify waiters of completed command Stefan Berger
@ 2017-11-10 14:11 ` Stefan Berger
2017-12-27 14:19 ` Stefan Berger
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 11/13] tpm: implement tpm_backend_wait_cmd_completed Stefan Berger
` (3 subsequent siblings)
13 siblings, 1 reply; 33+ messages in thread
From: Stefan Berger @ 2017-11-10 14:11 UTC (permalink / raw)
To: qemu-devel, marcandre.lureau; +Cc: amarnath.valluri, Stefan Berger
TPM backends will suspend independently of the frontends. Also
here we need to be able to wait for the TPM command to have been
completely processed.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
backends/tpm.c | 19 +++++++++++++++++++
include/sysemu/tpm_backend.h | 14 ++++++++++++++
2 files changed, 33 insertions(+)
diff --git a/backends/tpm.c b/backends/tpm.c
index 91222c5..bf0e120 100644
--- a/backends/tpm.c
+++ b/backends/tpm.c
@@ -20,6 +20,14 @@
#include "qemu/thread.h"
#include "qemu/main-loop.h"
+void tpm_backend_cmd_completed(TPMBackend *s)
+{
+ qemu_mutex_lock(&s->state_lock);
+ s->tpm_busy = false;
+ qemu_cond_signal(&s->cmd_complete);
+ qemu_mutex_unlock(&s->state_lock);
+}
+
static void tpm_backend_request_completed_bh(void *opaque)
{
TPMBackend *s = TPM_BACKEND(opaque);
@@ -36,6 +44,9 @@ static void tpm_backend_worker_thread(gpointer data, gpointer user_data)
k->handle_request(s, (TPMBackendCmd *)data);
qemu_bh_schedule(s->bh);
+
+ /* result delivered */
+ tpm_backend_cmd_completed(s);
}
static void tpm_backend_thread_end(TPMBackend *s)
@@ -64,6 +75,10 @@ int tpm_backend_init(TPMBackend *s, TPMIf *tpmif, Error **errp)
object_ref(OBJECT(tpmif));
s->had_startup_error = false;
+ s->tpm_busy = false;
+
+ qemu_mutex_init(&s->state_lock);
+ qemu_cond_init(&s->cmd_complete);
return 0;
}
@@ -93,6 +108,10 @@ bool tpm_backend_had_startup_error(TPMBackend *s)
void tpm_backend_deliver_request(TPMBackend *s, TPMBackendCmd *cmd)
{
+ qemu_mutex_lock(&s->state_lock);
+ s->tpm_busy = true;
+ qemu_mutex_unlock(&s->state_lock);
+
g_thread_pool_push(s->thread_pool, cmd, NULL);
}
diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
index 0d6c994..39598e3 100644
--- a/include/sysemu/tpm_backend.h
+++ b/include/sysemu/tpm_backend.h
@@ -18,6 +18,7 @@
#include "qapi-types.h"
#include "qemu/option.h"
#include "sysemu/tpm.h"
+#include "qemu/thread.h"
#define TYPE_TPM_BACKEND "tpm-backend"
#define TPM_BACKEND(obj) \
@@ -53,6 +54,10 @@ struct TPMBackend {
char *id;
QLIST_ENTRY(TPMBackend) list;
+
+ QemuMutex state_lock;
+ QemuCond cmd_complete; /* signaled once tpm_busy is false */
+ bool tpm_busy;
};
struct TPMBackendClass {
@@ -206,6 +211,15 @@ size_t tpm_backend_get_buffer_size(TPMBackend *s);
*/
TPMInfo *tpm_backend_query_tpm(TPMBackend *s);
+/**
+ * tpm_backend_cmd_completed:
+ * @s: the backend
+ *
+ * Mark the backend as not busy and notify anyone interested
+ * in the state changed
+ */
+void tpm_backend_cmd_completed(TPMBackend *s);
+
TPMBackend *qemu_find_tpm_be(const char *id);
#endif
--
2.5.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v3 11/13] tpm: implement tpm_backend_wait_cmd_completed
2017-11-10 14:11 [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Stefan Berger
` (9 preceding siblings ...)
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 10/13] tpm: Introduce condition in TPM backend for notification Stefan Berger
@ 2017-11-10 14:11 ` Stefan Berger
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 12/13] tpm: extend TPM emulator with state migration support Stefan Berger
` (2 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Stefan Berger @ 2017-11-10 14:11 UTC (permalink / raw)
To: qemu-devel, marcandre.lureau; +Cc: amarnath.valluri, Stefan Berger
Implement tpm_backend_wait_cmd_completed to synchronize with the
backend (thread) for the completion of a command.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
backends/tpm.c | 10 ++++++++++
include/sysemu/tpm_backend.h | 8 ++++++++
2 files changed, 18 insertions(+)
diff --git a/backends/tpm.c b/backends/tpm.c
index bf0e120..5e4de27 100644
--- a/backends/tpm.c
+++ b/backends/tpm.c
@@ -28,6 +28,16 @@ void tpm_backend_cmd_completed(TPMBackend *s)
qemu_mutex_unlock(&s->state_lock);
}
+void tpm_backend_wait_cmd_completed(TPMBackend *s)
+{
+ qemu_mutex_lock(&s->state_lock);
+
+ if (s->tpm_busy) {
+ qemu_cond_wait(&s->cmd_complete, &s->state_lock);
+ }
+ qemu_mutex_unlock(&s->state_lock);
+}
+
static void tpm_backend_request_completed_bh(void *opaque)
{
TPMBackend *s = TPM_BACKEND(opaque);
diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
index 39598e3..1170cb9 100644
--- a/include/sysemu/tpm_backend.h
+++ b/include/sysemu/tpm_backend.h
@@ -220,6 +220,14 @@ TPMInfo *tpm_backend_query_tpm(TPMBackend *s);
*/
void tpm_backend_cmd_completed(TPMBackend *s);
+/**
+ * tpm_backend_wait_cmd_completed:
+ * @s: the backend
+ *
+ * Wait the backend to not be busy anymore
+ */
+void tpm_backend_wait_cmd_completed(TPMBackend *s);
+
TPMBackend *qemu_find_tpm_be(const char *id);
#endif
--
2.5.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v3 12/13] tpm: extend TPM emulator with state migration support
2017-11-10 14:11 [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Stefan Berger
` (10 preceding siblings ...)
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 11/13] tpm: implement tpm_backend_wait_cmd_completed Stefan Berger
@ 2017-11-10 14:11 ` Stefan Berger
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 13/13] tpm_tis: extend TPM TIS " Stefan Berger
2017-12-22 12:49 ` [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Marc-André Lureau
13 siblings, 0 replies; 33+ messages in thread
From: Stefan Berger @ 2017-11-10 14:11 UTC (permalink / raw)
To: qemu-devel, marcandre.lureau; +Cc: amarnath.valluri, Stefan Berger
Extend the TPM emulator backend device with state migration support.
The external TPM emulator 'swtpm' provides a protocol over
its control channel to retrieve its state blobs. We implement
functions for getting and setting the different state blobs.
Since we have an external TPM emulator, we need to make sure
that we do not migrate the state for as long as it is busy
processing a request. We need to wait for notification that
the request has completed processing.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
hw/tpm/tpm_emulator.c | 303 ++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 293 insertions(+), 10 deletions(-)
diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
index 3ae8bf6..f05753f 100644
--- a/hw/tpm/tpm_emulator.c
+++ b/hw/tpm/tpm_emulator.c
@@ -61,6 +61,19 @@
#define TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(S, cap) (((S)->caps & (cap)) == (cap))
/* data structures */
+
+/* blobs from the TPM; part of VM state when migrating */
+typedef struct TPMBlobBuffers {
+ uint32_t permanent_flags;
+ TPMSizedBuffer permanent;
+
+ uint32_t volatil_flags;
+ TPMSizedBuffer volatil;
+
+ uint32_t savestate_flags;
+ TPMSizedBuffer savestate;
+} TPMBlobBuffers;
+
typedef struct TPMEmulator {
TPMBackend parent;
@@ -73,6 +86,8 @@ typedef struct TPMEmulator {
Error *migration_blocker;
QemuMutex mutex;
+
+ TPMBlobBuffers state_blobs;
} TPMEmulator;
@@ -315,18 +330,24 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
return 0;
}
-static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
+static int _tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize,
+ bool is_resume)
{
TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
ptm_init init;
ptm_res res;
+ DPRINTF("%s is_resume: %d", __func__, is_resume);
+
if (buffersize != 0 &&
tpm_emulator_set_buffer_size(tb, buffersize, NULL) < 0) {
goto err_exit;
}
- DPRINTF("%s", __func__);
+ if (is_resume) {
+ init.u.req.init_flags = cpu_to_be32(PTM_INIT_FLAG_DELETE_VOLATILE);
+ }
+
if (tpm_emulator_ctrlcmd(tpm_emu, CMD_INIT, &init, sizeof(init),
sizeof(init)) < 0) {
error_report("tpm-emulator: could not send INIT: %s",
@@ -345,6 +366,11 @@ err_exit:
return -1;
}
+static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
+{
+ return _tpm_emulator_startup_tpm(tb, buffersize, false);
+}
+
static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
{
TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
@@ -435,16 +461,21 @@ static size_t tpm_emulator_get_buffer_size(TPMBackend *tb)
static int tpm_emulator_block_migration(TPMEmulator *tpm_emu)
{
Error *err = NULL;
+ ptm_cap caps = PTM_CAP_GET_STATEBLOB | PTM_CAP_SET_STATEBLOB |
+ PTM_CAP_STOP;
- error_setg(&tpm_emu->migration_blocker,
- "Migration disabled: TPM emulator not yet migratable");
- migrate_add_blocker(tpm_emu->migration_blocker, &err);
- if (err) {
- error_report_err(err);
- error_free(tpm_emu->migration_blocker);
- tpm_emu->migration_blocker = NULL;
+ if (!TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(tpm_emu, caps)) {
+ error_setg(&tpm_emu->migration_blocker,
+ "Migration disabled: TPM emulator does not support "
+ "migration");
+ migrate_add_blocker(tpm_emu->migration_blocker, &err);
+ if (err) {
+ error_report_err(err);
+ error_free(tpm_emu->migration_blocker);
+ tpm_emu->migration_blocker = NULL;
- return -1;
+ return -1;
+ }
}
return 0;
@@ -573,6 +604,253 @@ static const QemuOptDesc tpm_emulator_cmdline_opts[] = {
{ /* end of list */ },
};
+/*
+ * Transfer a TPM state blob from the TPM into a provided buffer.
+ *
+ * @tpm_emu: TPMEmulator
+ * @type: the type of blob to transfer
+ * @tsb: the TPMSizeBuffer to fill with the blob
+ * @flags: the flags to return to the caller
+ */
+static int tpm_emulator_get_state_blob(TPMEmulator *tpm_emu,
+ uint8_t type,
+ TPMSizedBuffer *tsb,
+ uint32_t *flags)
+{
+ ptm_getstate pgs;
+ ptm_res res;
+ ssize_t n;
+ uint32_t totlength, length;
+
+ tpm_sized_buffer_reset(tsb);
+
+ pgs.u.req.state_flags = cpu_to_be32(PTM_STATE_FLAG_DECRYPTED);
+ pgs.u.req.type = cpu_to_be32(type);
+ pgs.u.req.offset = 0;
+
+ if (tpm_emulator_ctrlcmd(tpm_emu, CMD_GET_STATEBLOB,
+ &pgs, sizeof(pgs.u.req),
+ offsetof(ptm_getstate, u.resp.data)) < 0) {
+ error_report("tpm-emulator: could not get state blob type %d : %s",
+ type, strerror(errno));
+ return -1;
+ }
+
+ res = be32_to_cpu(pgs.u.resp.tpm_result);
+ if (res != 0 && (res & 0x800) == 0) {
+ error_report("tpm-emulator: Getting the stateblob (type %d) failed "
+ "with a TPM error 0x%x", type, res);
+ return -1;
+ }
+
+ totlength = be32_to_cpu(pgs.u.resp.totlength);
+ if (totlength >= 32 * 1024) {
+ error_report("tpm-emulator: TPM state blob (type %d) with %d bytes "
+ "too large to read", type, totlength);
+ return -1;
+ }
+
+ length = be32_to_cpu(pgs.u.resp.length);
+ if (totlength != length) {
+ error_report("tpm-emulator: Expecting to read %u bytes "
+ "but would get %u", totlength, length);
+ return -1;
+ }
+
+ *flags = be32_to_cpu(pgs.u.resp.state_flags);
+
+ tsb->buffer = g_malloc(totlength);
+
+ n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr, tsb->buffer, totlength);
+ if (n != totlength) {
+ error_report("tpm-emulator: Could not read stateblob (type %d) : %s",
+ type, strerror(errno));
+ return -1;
+ }
+ tsb->size = totlength;
+
+ DPRINTF("got state blob type %d, %d bytes, flags 0x%08x\n",
+ type, tsb->size, *flags);
+
+ return 0;
+}
+
+static int tpm_emulator_get_state_blobs(TPMEmulator *tpm_emu)
+{
+ TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
+
+ if (tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
+ &state_blobs->permanent,
+ &state_blobs->permanent_flags) < 0 ||
+ tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
+ &state_blobs->volatil,
+ &state_blobs->volatil_flags) < 0 ||
+ tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
+ &state_blobs->savestate,
+ &state_blobs->savestate_flags) < 0) {
+ goto err_exit;
+ }
+
+ return 0;
+
+ err_exit:
+ tpm_sized_buffer_reset(&state_blobs->volatil);
+ tpm_sized_buffer_reset(&state_blobs->permanent);
+ tpm_sized_buffer_reset(&state_blobs->savestate);
+
+ return -1;
+}
+
+/*
+ * Transfer a TPM state blob to the TPM emulator.
+ *
+ * @tpm_emu: TPMEmulator
+ * @type: the type of TPM state blob to transfer
+ * @tsb: TPMSizeBuffer containing the TPM state blob
+ * @flags: Flags describing the (encryption) state of the TPM state blob
+ */
+static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu,
+ uint32_t type,
+ TPMSizedBuffer *tsb,
+ uint32_t flags)
+{
+ uint32_t offset = 0;
+ ssize_t n;
+ ptm_setstate pss;
+ ptm_res tpm_result;
+
+ if (tsb->size == 0) {
+ return 0;
+ }
+
+ pss = (ptm_setstate) {
+ .u.req.state_flags = cpu_to_be32(flags),
+ .u.req.type = cpu_to_be32(type),
+ .u.req.length = cpu_to_be32(tsb->size),
+ };
+
+ /* write the header only */
+ if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_STATEBLOB, &pss,
+ offsetof(ptm_setstate, u.req.data), 0) < 0) {
+ error_report("tpm-emulator: could not set state blob type %d : %s",
+ type, strerror(errno));
+ return -1;
+ }
+
+ /* now the body */
+ n = qemu_chr_fe_write_all(&tpm_emu->ctrl_chr,
+ &tsb->buffer[offset], tsb->size);
+ if (n != tsb->size) {
+ error_report("tpm-emulator: Writing the stateblob (type %d) "
+ "failed: %s", type, strerror(errno));
+ return -1;
+ }
+
+ /* now get the result */
+ n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr,
+ (uint8_t *)&pss, sizeof(pss.u.resp));
+ if (n != sizeof(pss.u.resp)) {
+ error_report("tpm-emulator: Reading response from writing stateblob "
+ "(type %d) failed: %s", type, strerror(errno));
+ return -1;
+ }
+
+ tpm_result = be32_to_cpu(pss.u.resp.tpm_result);
+ if (tpm_result != 0) {
+ error_report("tpm-emulator: Setting the stateblob (type %d) failed "
+ "with a TPM error 0x%x", type, tpm_result);
+ return -1;
+ }
+
+ DPRINTF("set the state blob type %d, %d bytes, flags 0x%08x\n",
+ type, tsb->size, flags);
+
+ return 0;
+}
+
+static int tpm_emulator_set_state_blobs(TPMBackend *tb)
+{
+ TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
+ TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
+
+ DPRINTF("%s: %d", __func__, __LINE__);
+
+ if (tpm_emulator_stop_tpm(tb) < 0) {
+ return -1;
+ }
+
+ if (tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
+ &state_blobs->permanent,
+ state_blobs->permanent_flags) < 0 ||
+ tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
+ &state_blobs->volatil,
+ state_blobs->volatil_flags) < 0 ||
+ tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
+ &state_blobs->savestate,
+ state_blobs->savestate_flags) < 0) {
+ return -1;
+ }
+
+ DPRINTF("DONE SETTING STATEBLOBS");
+
+ return 0;
+}
+
+static int tpm_emulator_pre_save(void *opaque)
+{
+ TPMBackend *tb = opaque;
+ TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
+
+ DPRINTF("%s", __func__);
+
+ tpm_backend_wait_cmd_completed(tb);
+
+ /* get the state blobs from the TPM */
+ return tpm_emulator_get_state_blobs(tpm_emu);
+}
+
+static int tpm_emulator_post_load(void *opaque,
+ int version_id __attribute__((unused)))
+{
+ TPMBackend *tb = opaque;
+
+ if (tpm_emulator_set_state_blobs(tb) < 0) {
+ return 1;
+ }
+
+ return _tpm_emulator_startup_tpm(tb, 0, true);
+}
+
+static const VMStateDescription vmstate_tpm_emulator = {
+ .name = "tpm-emulator",
+ .version_id = 1,
+ .minimum_version_id = 0,
+ .minimum_version_id_old = 0,
+ .pre_save = tpm_emulator_pre_save,
+ .post_load = tpm_emulator_post_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(state_blobs.permanent_flags, TPMEmulator),
+ VMSTATE_UINT32(state_blobs.permanent.size, TPMEmulator),
+ VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.permanent.buffer,
+ TPMEmulator, 1, 0,
+ state_blobs.permanent.size),
+
+ VMSTATE_UINT32(state_blobs.volatil_flags, TPMEmulator),
+ VMSTATE_UINT32(state_blobs.volatil.size, TPMEmulator),
+ VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.volatil.buffer,
+ TPMEmulator, 1, 0,
+ state_blobs.volatil.size),
+
+ VMSTATE_UINT32(state_blobs.savestate_flags, TPMEmulator),
+ VMSTATE_UINT32(state_blobs.savestate.size, TPMEmulator),
+ VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.savestate.buffer,
+ TPMEmulator, 1, 0,
+ state_blobs.savestate.size),
+
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static void tpm_emulator_inst_init(Object *obj)
{
TPMEmulator *tpm_emu = TPM_EMULATOR(obj);
@@ -580,7 +858,10 @@ static void tpm_emulator_inst_init(Object *obj)
DPRINTF("%s", __func__);
tpm_emu->options = g_new0(TPMEmulatorOptions, 1);
tpm_emu->cur_locty_number = ~0;
+
qemu_mutex_init(&tpm_emu->mutex);
+
+ vmstate_register(NULL, -1, &vmstate_tpm_emulator, obj);
}
/*
@@ -617,6 +898,8 @@ static void tpm_emulator_inst_finalize(Object *obj)
}
qemu_mutex_destroy(&tpm_emu->mutex);
+
+ vmstate_unregister(NULL, &vmstate_tpm_emulator, obj);
}
static void tpm_emulator_class_init(ObjectClass *klass, void *data)
--
2.5.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v3 13/13] tpm_tis: extend TPM TIS with state migration support
2017-11-10 14:11 [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Stefan Berger
` (11 preceding siblings ...)
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 12/13] tpm: extend TPM emulator with state migration support Stefan Berger
@ 2017-11-10 14:11 ` Stefan Berger
2017-12-22 12:49 ` [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Marc-André Lureau
13 siblings, 0 replies; 33+ messages in thread
From: Stefan Berger @ 2017-11-10 14:11 UTC (permalink / raw)
To: qemu-devel, marcandre.lureau; +Cc: amarnath.valluri, Stefan Berger
Extend the TPM TIS interface with state migration support.
We need to synchronize with the backend thread to make sure that a command
being processed by the external TPM emulator has completed and its
response been received. In case the bottom half did not run, we run the
function it is supposed to run.
Since only 1 locality can be active at any time we only need
to store the command buffer of that active locality.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
hw/tpm/tpm_tis.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 68 insertions(+), 6 deletions(-)
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 86e9a92..c0a0204 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -393,12 +393,8 @@ static void tpm_tis_prep_abort(TPMState *s, uint8_t locty, uint8_t newlocty)
tpm_tis_abort(s, locty);
}
-/*
- * Callback from the TPM to indicate that the response was received.
- */
-static void tpm_tis_request_completed(TPMIf *ti)
+static void _tpm_tis_request_completed(TPMState *s)
{
- TPMState *s = TPM(ti);
uint8_t locty = s->cmd.locty;
uint8_t l;
@@ -431,6 +427,14 @@ static void tpm_tis_request_completed(TPMIf *ti)
}
/*
+ * Callback from the TPM to indicate that the response was received.
+ */
+static void tpm_tis_request_completed(TPMIf *ti)
+{
+ _tpm_tis_request_completed(TPM(ti));
+}
+
+/*
* Read a byte of response data
*/
static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty)
@@ -1012,9 +1016,67 @@ static void tpm_tis_reset(DeviceState *dev)
tpm_tis_do_startup_tpm(s, s->be_buffer_size);
}
+/* persistent state handling */
+
+static int tpm_tis_pre_save(void *opaque)
+{
+ TPMState *s = opaque;
+ uint8_t locty = s->active_locty;
+
+ DPRINTF("tpm_tis: suspend: locty = %d : rw_offset = %u\n",
+ locty, s->rw_offset);
+#ifdef DEBUG_TIS
+ tpm_tis_dump_state(opaque, 0);
+#endif
+
+ /*
+ * Synchronize with backend completion.
+ */
+ tpm_backend_wait_cmd_completed(s->be_driver);
+
+ if (TPM_TIS_IS_VALID_LOCTY(locty) &&
+ s->loc[locty].state == TPM_TIS_STATE_EXECUTION) {
+ /* bottom half did not run - run its function */
+ _tpm_tis_request_completed(s);
+ }
+
+ return 0;
+}
+
+static const VMStateDescription vmstate_locty = {
+ .name = "loc",
+ .version_id = 1,
+ .minimum_version_id = 0,
+ .minimum_version_id_old = 0,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(state, TPMLocality),
+ VMSTATE_UINT32(inte, TPMLocality),
+ VMSTATE_UINT32(ints, TPMLocality),
+ VMSTATE_UINT8(access, TPMLocality),
+ VMSTATE_UINT32(sts, TPMLocality),
+ VMSTATE_UINT32(iface_id, TPMLocality),
+ VMSTATE_END_OF_LIST(),
+ }
+};
+
static const VMStateDescription vmstate_tpm_tis = {
.name = "tpm",
- .unmigratable = 1,
+ .version_id = 1,
+ .minimum_version_id = 0,
+ .minimum_version_id_old = 0,
+ .pre_save = tpm_tis_pre_save,
+ .fields = (VMStateField[]) {
+ VMSTATE_BUFFER(buffer, TPMState),
+ VMSTATE_UINT16(rw_offset, TPMState),
+ VMSTATE_UINT8(active_locty, TPMState),
+ VMSTATE_UINT8(aborting_locty, TPMState),
+ VMSTATE_UINT8(next_locty, TPMState),
+
+ VMSTATE_STRUCT_ARRAY(loc, TPMState, TPM_TIS_NUM_LOCALITIES, 1,
+ vmstate_locty, TPMLocality),
+
+ VMSTATE_END_OF_LIST()
+ }
};
static Property tpm_tis_properties[] = {
--
2.5.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 04/13] tpm_tis: move buffers from localities into common location
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 04/13] tpm_tis: move buffers from localities into common location Stefan Berger
@ 2017-12-21 14:11 ` Marc-André Lureau
0 siblings, 0 replies; 33+ messages in thread
From: Marc-André Lureau @ 2017-12-21 14:11 UTC (permalink / raw)
To: Stefan Berger; +Cc: QEMU, Amarnath Valluri
Hi
On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> One read buffer and one write buffer is sufficient for all localities.
> The localities cannot all be active at the same time, and only the active
> locality can use the r/w buffers. Inactive localities will require the
> COMMAND_READY flag to be set on the STS register to move to the READY
> state, which then enables access to using the buffer for writing of a
> command, while all other localities are inactive.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
> hw/tpm/tpm_tis.c | 34 +++++++++++++++-------------------
> 1 file changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index ddfcfc9..f6f5f17 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -64,16 +64,14 @@ typedef struct TPMLocality {
>
> uint16_t w_offset;
> uint16_t r_offset;
> - unsigned char w_buffer[TPM_TIS_BUFFER_MAX];
> - unsigned char r_buffer[TPM_TIS_BUFFER_MAX];
> } TPMLocality;
>
> typedef struct TPMState {
> ISADevice busdev;
> MemoryRegion mmio;
>
> - uint32_t offset;
> - uint8_t buf[TPM_TIS_BUFFER_MAX];
Looks like those were never used. Make it a seperate cleanup?
> + unsigned char w_buffer[TPM_TIS_BUFFER_MAX];
> + unsigned char r_buffer[TPM_TIS_BUFFER_MAX];
>
> uint8_t active_locty;
> uint8_t aborting_locty;
> @@ -259,7 +257,7 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
> {
> TPMLocality *locty_data = &s->loc[locty];
>
> - tpm_tis_show_buffer(s->loc[locty].w_buffer, s->be_buffer_size,
> + tpm_tis_show_buffer(s->w_buffer, s->be_buffer_size,
> "tpm_tis: To TPM");
>
> /*
> @@ -270,9 +268,9 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
>
> s->cmd = (TPMBackendCmd) {
> .locty = locty,
> - .in = locty_data->w_buffer,
> + .in = s->w_buffer,
> .in_len = locty_data->w_offset,
> - .out = locty_data->r_buffer,
> + .out = s->r_buffer,
> .out_len = s->be_buffer_size,
> };
>
> @@ -424,7 +422,7 @@ static void tpm_tis_request_completed(TPMIf *ti)
> s->loc[locty].r_offset = 0;
> s->loc[locty].w_offset = 0;
>
> - tpm_tis_show_buffer(s->loc[locty].r_buffer, s->be_buffer_size,
> + tpm_tis_show_buffer(s->r_buffer, s->be_buffer_size,
> "tpm_tis: From TPM");
>
> if (TPM_TIS_IS_VALID_LOCTY(s->next_locty)) {
> @@ -444,10 +442,10 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty)
> uint16_t len;
>
> if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) {
> - len = MIN(tpm_cmd_get_size(&s->loc[locty].r_buffer),
> + len = MIN(tpm_cmd_get_size(&s->r_buffer),
> s->be_buffer_size);
>
> - ret = s->loc[locty].r_buffer[s->loc[locty].r_offset++];
> + ret = s->r_buffer[s->loc[locty].r_offset++];
> if (s->loc[locty].r_offset >= len) {
> /* got last byte */
> tpm_tis_sts_set(&s->loc[locty], TPM_TIS_STS_VALID);
> @@ -493,12 +491,11 @@ static void tpm_tis_dump_state(void *opaque, hwaddr addr)
> "tpm_tis: result buffer : ",
> s->loc[locty].r_offset);
> for (idx = 0;
> - idx < MIN(tpm_cmd_get_size(&s->loc[locty].r_buffer),
> - s->be_buffer_size);
> + idx < MIN(tpm_cmd_get_size(&s->r_buffer), s->be_buffer_size);
> idx++) {
> DPRINTF("%c%02x%s",
> s->loc[locty].r_offset == idx ? '>' : ' ',
> - s->loc[locty].r_buffer[idx],
> + s->r_buffer[idx],
> ((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
> }
> DPRINTF("\n"
> @@ -506,12 +503,11 @@ static void tpm_tis_dump_state(void *opaque, hwaddr addr)
> "tpm_tis: request buffer: ",
> s->loc[locty].w_offset);
> for (idx = 0;
> - idx < MIN(tpm_cmd_get_size(s->loc[locty].w_buffer),
> - s->be_buffer_size);
> + idx < MIN(tpm_cmd_get_size(s->w_buffer), s->be_buffer_size);
> idx++) {
> DPRINTF("%c%02x%s",
> s->loc[locty].w_offset == idx ? '>' : ' ',
> - s->loc[locty].w_buffer[idx],
> + s->w_buffer[idx],
> ((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
> }
> DPRINTF("\n");
> @@ -573,7 +569,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
> if (s->active_locty == locty) {
> if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) {
> val = TPM_TIS_BURST_COUNT(
> - MIN(tpm_cmd_get_size(&s->loc[locty].r_buffer),
> + MIN(tpm_cmd_get_size(&s->r_buffer),
> s->be_buffer_size)
> - s->loc[locty].r_offset) | s->loc[locty].sts;
> } else {
> @@ -926,7 +922,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
>
> while ((s->loc[locty].sts & TPM_TIS_STS_EXPECT) && size > 0) {
> if (s->loc[locty].w_offset < s->be_buffer_size) {
> - s->loc[locty].w_buffer[s->loc[locty].w_offset++] =
> + s->w_buffer[s->loc[locty].w_offset++] =
> (uint8_t)val;
> val >>= 8;
> size--;
> @@ -941,7 +937,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
> /* we have a packet length - see if we have all of it */
> bool need_irq = !(s->loc[locty].sts & TPM_TIS_STS_VALID);
>
> - len = tpm_cmd_get_size(&s->loc[locty].w_buffer);
> + len = tpm_cmd_get_size(&s->w_buffer);
> if (len > s->loc[locty].w_offset) {
> tpm_tis_sts_set(&s->loc[locty],
> TPM_TIS_STS_EXPECT | TPM_TIS_STS_VALID);
> --
> 2.5.5
>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 03/13] tpm_tis: remove TPMSizeBuffer usage
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 03/13] tpm_tis: remove TPMSizeBuffer usage Stefan Berger
@ 2017-12-21 14:11 ` Marc-André Lureau
0 siblings, 0 replies; 33+ messages in thread
From: Marc-André Lureau @ 2017-12-21 14:11 UTC (permalink / raw)
To: Stefan Berger; +Cc: QEMU, Amarnath Valluri
On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Remove usage of TPMSizeBuffer. The size of the buffers is limited now
> by s->be_buffer_size, which is the size of the buffer the TIS has
> negotiated with the backend.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/tpm/tpm_tis.c | 68 ++++++++++++++++++++++++--------------------------------
> 1 file changed, 29 insertions(+), 39 deletions(-)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 90c6df2..ddfcfc9 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -64,8 +64,8 @@ typedef struct TPMLocality {
>
> uint16_t w_offset;
> uint16_t r_offset;
> - TPMSizedBuffer w_buffer;
> - TPMSizedBuffer r_buffer;
> + unsigned char w_buffer[TPM_TIS_BUFFER_MAX];
> + unsigned char r_buffer[TPM_TIS_BUFFER_MAX];
> } TPMLocality;
>
> typedef struct TPMState {
> @@ -215,23 +215,19 @@ static uint8_t tpm_tis_locality_from_addr(hwaddr addr)
> return (uint8_t)((addr >> TPM_TIS_LOCALITY_SHIFT) & 0x7);
> }
>
> -static uint32_t tpm_tis_get_size_from_buffer(const TPMSizedBuffer *sb)
> -{
> - return tpm_cmd_get_size(sb->buffer);
> -}
> -
> -static void tpm_tis_show_buffer(const TPMSizedBuffer *sb, const char *string)
> +static void tpm_tis_show_buffer(const unsigned char *buffer,
> + size_t buffer_size, const char *string)
> {
> #ifdef DEBUG_TIS
> uint32_t len, i;
>
> - len = tpm_tis_get_size_from_buffer(sb);
> + len = MIN(tpm_cmd_get_size(buffer), buffer_size);
> DPRINTF("tpm_tis: %s length = %d\n", string, len);
> for (i = 0; i < len; i++) {
> if (i && !(i % 16)) {
> DPRINTF("\n");
> }
> - DPRINTF("%.2X ", sb->buffer[i]);
> + DPRINTF("%.2X ", buffer[i]);
> }
> DPRINTF("\n");
> #endif
> @@ -263,7 +259,8 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
> {
> TPMLocality *locty_data = &s->loc[locty];
>
> - tpm_tis_show_buffer(&s->loc[locty].w_buffer, "tpm_tis: To TPM");
> + tpm_tis_show_buffer(s->loc[locty].w_buffer, s->be_buffer_size,
> + "tpm_tis: To TPM");
>
> /*
> * w_offset serves as length indicator for length of data;
> @@ -273,10 +270,10 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
>
> s->cmd = (TPMBackendCmd) {
> .locty = locty,
> - .in = locty_data->w_buffer.buffer,
> + .in = locty_data->w_buffer,
> .in_len = locty_data->w_offset,
> - .out = locty_data->r_buffer.buffer,
> - .out_len = locty_data->r_buffer.size
> + .out = locty_data->r_buffer,
> + .out_len = s->be_buffer_size,
> };
>
> tpm_backend_deliver_request(s->be_driver, &s->cmd);
> @@ -427,7 +424,8 @@ static void tpm_tis_request_completed(TPMIf *ti)
> s->loc[locty].r_offset = 0;
> s->loc[locty].w_offset = 0;
>
> - tpm_tis_show_buffer(&s->loc[locty].r_buffer, "tpm_tis: From TPM");
> + tpm_tis_show_buffer(s->loc[locty].r_buffer, s->be_buffer_size,
> + "tpm_tis: From TPM");
>
> if (TPM_TIS_IS_VALID_LOCTY(s->next_locty)) {
> tpm_tis_abort(s, locty);
> @@ -446,9 +444,10 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty)
> uint16_t len;
>
> if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) {
> - len = tpm_tis_get_size_from_buffer(&s->loc[locty].r_buffer);
> + len = MIN(tpm_cmd_get_size(&s->loc[locty].r_buffer),
> + s->be_buffer_size);
>
> - ret = s->loc[locty].r_buffer.buffer[s->loc[locty].r_offset++];
> + ret = s->loc[locty].r_buffer[s->loc[locty].r_offset++];
> if (s->loc[locty].r_offset >= len) {
> /* got last byte */
> tpm_tis_sts_set(&s->loc[locty], TPM_TIS_STS_VALID);
> @@ -494,11 +493,12 @@ static void tpm_tis_dump_state(void *opaque, hwaddr addr)
> "tpm_tis: result buffer : ",
> s->loc[locty].r_offset);
> for (idx = 0;
> - idx < tpm_tis_get_size_from_buffer(&s->loc[locty].r_buffer);
> + idx < MIN(tpm_cmd_get_size(&s->loc[locty].r_buffer),
> + s->be_buffer_size);
> idx++) {
> DPRINTF("%c%02x%s",
> s->loc[locty].r_offset == idx ? '>' : ' ',
> - s->loc[locty].r_buffer.buffer[idx],
> + s->loc[locty].r_buffer[idx],
> ((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
> }
> DPRINTF("\n"
> @@ -506,11 +506,12 @@ static void tpm_tis_dump_state(void *opaque, hwaddr addr)
> "tpm_tis: request buffer: ",
> s->loc[locty].w_offset);
> for (idx = 0;
> - idx < tpm_tis_get_size_from_buffer(&s->loc[locty].w_buffer);
> + idx < MIN(tpm_cmd_get_size(s->loc[locty].w_buffer),
> + s->be_buffer_size);
> idx++) {
> DPRINTF("%c%02x%s",
> s->loc[locty].w_offset == idx ? '>' : ' ',
> - s->loc[locty].w_buffer.buffer[idx],
> + s->loc[locty].w_buffer[idx],
> ((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
> }
> DPRINTF("\n");
> @@ -572,11 +573,11 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
> if (s->active_locty == locty) {
> if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) {
> val = TPM_TIS_BURST_COUNT(
> - tpm_tis_get_size_from_buffer(&s->loc[locty].r_buffer)
> + MIN(tpm_cmd_get_size(&s->loc[locty].r_buffer),
> + s->be_buffer_size)
> - s->loc[locty].r_offset) | s->loc[locty].sts;
> } else {
> - avail = s->loc[locty].w_buffer.size
> - - s->loc[locty].w_offset;
> + avail = s->be_buffer_size - s->loc[locty].w_offset;
> /*
> * byte-sized reads should not return 0x00 for 0x100
> * available bytes.
> @@ -924,9 +925,9 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
> }
>
> while ((s->loc[locty].sts & TPM_TIS_STS_EXPECT) && size > 0) {
> - if (s->loc[locty].w_offset < s->loc[locty].w_buffer.size) {
> - s->loc[locty].w_buffer.
> - buffer[s->loc[locty].w_offset++] = (uint8_t)val;
> + if (s->loc[locty].w_offset < s->be_buffer_size) {
> + s->loc[locty].w_buffer[s->loc[locty].w_offset++] =
> + (uint8_t)val;
> val >>= 8;
> size--;
> } else {
> @@ -940,7 +941,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
> /* we have a packet length - see if we have all of it */
> bool need_irq = !(s->loc[locty].sts & TPM_TIS_STS_VALID);
>
> - len = tpm_tis_get_size_from_buffer(&s->loc[locty].w_buffer);
> + len = tpm_cmd_get_size(&s->loc[locty].w_buffer);
> if (len > s->loc[locty].w_offset) {
> tpm_tis_sts_set(&s->loc[locty],
> TPM_TIS_STS_EXPECT | TPM_TIS_STS_VALID);
> @@ -979,15 +980,6 @@ static int tpm_tis_do_startup_tpm(TPMState *s, size_t buffersize)
> return tpm_backend_startup_tpm(s->be_driver, buffersize);
> }
>
> -static void tpm_tis_realloc_buffer(TPMSizedBuffer *sb,
> - size_t wanted_size)
> -{
> - if (sb->size != wanted_size) {
> - sb->buffer = g_realloc(sb->buffer, wanted_size);
> - sb->size = wanted_size;
> - }
> -}
> -
> /*
> * Get the TPMVersion of the backend device being used
> */
> @@ -1036,9 +1028,7 @@ static void tpm_tis_reset(DeviceState *dev)
> s->loc[c].state = TPM_TIS_STATE_IDLE;
>
> s->loc[c].w_offset = 0;
> - tpm_tis_realloc_buffer(&s->loc[c].w_buffer, s->be_buffer_size);
> s->loc[c].r_offset = 0;
> - tpm_tis_realloc_buffer(&s->loc[c].r_buffer, s->be_buffer_size);
> }
>
> tpm_tis_do_startup_tpm(s, s->be_buffer_size);
> --
> 2.5.5
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 02/13] tpm_tis: limit size of buffer from backend
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 02/13] tpm_tis: limit size of buffer from backend Stefan Berger
@ 2017-12-21 14:11 ` Marc-André Lureau
0 siblings, 0 replies; 33+ messages in thread
From: Marc-André Lureau @ 2017-12-21 14:11 UTC (permalink / raw)
To: Stefan Berger; +Cc: QEMU, Amarnath Valluri
On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> This is a preparatory patch for the subsequent ones where we
> get rid of the flexibility of supporting any kind of buffer size
> that the backend may support. We keep the size at 4096, which is
> also the size the external emulator supports. So, limit the size
> of the buffer we can support and pass it back to the backend.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/tpm/tpm_tis.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 69fe531..90c6df2 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -1008,7 +1008,8 @@ static void tpm_tis_reset(DeviceState *dev)
> int c;
>
> s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver);
> - s->be_buffer_size = tpm_backend_get_buffer_size(s->be_driver);
> + s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver),
> + TPM_TIS_BUFFER_MAX);
>
> tpm_backend_reset(s->be_driver);
>
> @@ -1040,7 +1041,7 @@ static void tpm_tis_reset(DeviceState *dev)
> tpm_tis_realloc_buffer(&s->loc[c].r_buffer, s->be_buffer_size);
> }
>
> - tpm_tis_do_startup_tpm(s, 0);
> + tpm_tis_do_startup_tpm(s, s->be_buffer_size);
> }
>
> static const VMStateDescription vmstate_tpm_tis = {
> --
> 2.5.5
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 01/13] tpm_tis: convert uint32_t to size_t
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 01/13] tpm_tis: convert uint32_t to size_t Stefan Berger
@ 2017-12-21 14:11 ` Marc-André Lureau
0 siblings, 0 replies; 33+ messages in thread
From: Marc-André Lureau @ 2017-12-21 14:11 UTC (permalink / raw)
To: Stefan Berger; +Cc: QEMU, Amarnath Valluri
On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
> hw/tpm/tpm_tis.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index dd43630..69fe531 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -974,7 +974,7 @@ static const MemoryRegionOps tpm_tis_memory_ops = {
> },
> };
>
> -static int tpm_tis_do_startup_tpm(TPMState *s, uint32_t buffersize)
> +static int tpm_tis_do_startup_tpm(TPMState *s, size_t buffersize)
> {
> return tpm_backend_startup_tpm(s->be_driver, buffersize);
> }
> --
> 2.5.5
>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 06/13] tpm_tis: move r/w_offsets to TPMState
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 06/13] tpm_tis: move r/w_offsets to TPMState Stefan Berger
@ 2017-12-21 14:41 ` Marc-André Lureau
2017-12-21 14:44 ` Stefan Berger
0 siblings, 1 reply; 33+ messages in thread
From: Marc-André Lureau @ 2017-12-21 14:41 UTC (permalink / raw)
To: Stefan Berger; +Cc: QEMU, Amarnath Valluri
Hi
On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Now that we have a single buffer, we also only need a single set of
> read/write offsets into that buffer. This works since only one
> locality can be active.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
> hw/tpm/tpm_tis.c | 57 +++++++++++++++++++++++++++-----------------------------
> 1 file changed, 27 insertions(+), 30 deletions(-)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 0b6dd7f..dfdddd3 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -61,9 +61,6 @@ typedef struct TPMLocality {
> uint32_t iface_id;
> uint32_t inte;
> uint32_t ints;
> -
> - uint16_t w_offset;
> - uint16_t r_offset;
> } TPMLocality;
>
> typedef struct TPMState {
> @@ -71,6 +68,8 @@ typedef struct TPMState {
> MemoryRegion mmio;
>
> unsigned char buffer[TPM_TIS_BUFFER_MAX];
> + uint16_t w_offset;
> + uint16_t r_offset;
>
> uint8_t active_locty;
> uint8_t aborting_locty;
> @@ -254,8 +253,6 @@ static void tpm_tis_sts_set(TPMLocality *l, uint32_t flags)
> */
> static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
> {
> - TPMLocality *locty_data = &s->loc[locty];
> -
> tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
> "tpm_tis: To TPM");
>
> @@ -268,7 +265,7 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
> s->cmd = (TPMBackendCmd) {
> .locty = locty,
> .in = s->buffer,
> - .in_len = locty_data->w_offset,
> + .in_len = s->w_offset,
> .out = s->buffer,
> .out_len = s->be_buffer_size,
> };
> @@ -350,8 +347,8 @@ static void tpm_tis_new_active_locality(TPMState *s, uint8_t new_active_locty)
> /* abort -- this function switches the locality */
> static void tpm_tis_abort(TPMState *s, uint8_t locty)
> {
> - s->loc[locty].r_offset = 0;
> - s->loc[locty].w_offset = 0;
> + s->r_offset = 0;
> + s->w_offset = 0;
>
> DPRINTF("tpm_tis: tis_abort: new active locality is %d\n", s->next_locty);
>
> @@ -418,8 +415,8 @@ static void tpm_tis_request_completed(TPMIf *ti)
> tpm_tis_sts_set(&s->loc[locty],
> TPM_TIS_STS_VALID | TPM_TIS_STS_DATA_AVAILABLE);
> s->loc[locty].state = TPM_TIS_STATE_COMPLETION;
> - s->loc[locty].r_offset = 0;
> - s->loc[locty].w_offset = 0;
> + s->r_offset = 0;
> + s->w_offset = 0;
>
> tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
> "tpm_tis: From TPM");
> @@ -444,14 +441,14 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty)
> len = MIN(tpm_cmd_get_size(&s->buffer),
> s->be_buffer_size);
>
> - ret = s->buffer[s->loc[locty].r_offset++];
> - if (s->loc[locty].r_offset >= len) {
> + ret = s->buffer[s->r_offset++];
> + if (s->r_offset >= len) {
> /* got last byte */
> tpm_tis_sts_set(&s->loc[locty], TPM_TIS_STS_VALID);
> tpm_tis_raise_irq(s, locty, TPM_TIS_INT_STS_VALID);
> }
> DPRINTF("tpm_tis: tpm_tis_data_read byte 0x%02x [%d]\n",
> - ret, s->loc[locty].r_offset - 1);
> + ret, s->r_offset - 1);
> }
>
> return ret;
> @@ -488,24 +485,24 @@ static void tpm_tis_dump_state(void *opaque, hwaddr addr)
>
> DPRINTF("tpm_tis: read offset : %d\n"
> "tpm_tis: result buffer : ",
> - s->loc[locty].r_offset);
> + s->r_offset);
> for (idx = 0;
> idx < MIN(tpm_cmd_get_size(&s->buffer), s->be_buffer_size);
> idx++) {
> DPRINTF("%c%02x%s",
> - s->loc[locty].r_offset == idx ? '>' : ' ',
> + s->r_offset == idx ? '>' : ' ',
> s->buffer[idx],
> ((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
> }
> DPRINTF("\n"
> "tpm_tis: write offset : %d\n"
> "tpm_tis: request buffer: ",
> - s->loc[locty].w_offset);
> + s->w_offset);
> for (idx = 0;
> idx < MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size);
> idx++) {
> DPRINTF("%c%02x%s",
> - s->loc[locty].w_offset == idx ? '>' : ' ',
> + s->w_offset == idx ? '>' : ' ',
> s->buffer[idx],
> ((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
> }
> @@ -570,9 +567,9 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
> val = TPM_TIS_BURST_COUNT(
> MIN(tpm_cmd_get_size(&s->buffer),
> s->be_buffer_size)
> - - s->loc[locty].r_offset) | s->loc[locty].sts;
> + - s->r_offset) | s->loc[locty].sts;
> } else {
> - avail = s->be_buffer_size - s->loc[locty].w_offset;
> + avail = s->be_buffer_size - s->w_offset;
> /*
> * byte-sized reads should not return 0x00 for 0x100
> * available bytes.
> @@ -836,8 +833,8 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
> switch (s->loc[locty].state) {
>
> case TPM_TIS_STATE_READY:
> - s->loc[locty].w_offset = 0;
> - s->loc[locty].r_offset = 0;
> + s->w_offset = 0;
> + s->r_offset = 0;
> break;
>
> case TPM_TIS_STATE_IDLE:
> @@ -855,8 +852,8 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
> break;
>
> case TPM_TIS_STATE_COMPLETION:
> - s->loc[locty].w_offset = 0;
> - s->loc[locty].r_offset = 0;
> + s->w_offset = 0;
> + s->r_offset = 0;
> /* shortcut to ready state with C/R set */
> s->loc[locty].state = TPM_TIS_STATE_READY;
> if (!(s->loc[locty].sts & TPM_TIS_STS_COMMAND_READY)) {
> @@ -882,7 +879,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
> } else if (val == TPM_TIS_STS_RESPONSE_RETRY) {
> switch (s->loc[locty].state) {
> case TPM_TIS_STATE_COMPLETION:
> - s->loc[locty].r_offset = 0;
> + s->r_offset = 0;
> tpm_tis_sts_set(&s->loc[locty],
> TPM_TIS_STS_VALID|
> TPM_TIS_STS_DATA_AVAILABLE);
> @@ -920,8 +917,8 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
> }
>
> while ((s->loc[locty].sts & TPM_TIS_STS_EXPECT) && size > 0) {
> - if (s->loc[locty].w_offset < s->be_buffer_size) {
> - s->buffer[s->loc[locty].w_offset++] =
> + if (s->w_offset < s->be_buffer_size) {
> + s->buffer[s->w_offset++] =
> (uint8_t)val;
> val >>= 8;
> size--;
> @@ -931,13 +928,13 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
> }
>
> /* check for complete packet */
> - if (s->loc[locty].w_offset > 5 &&
> + if (s->w_offset > 5 &&
> (s->loc[locty].sts & TPM_TIS_STS_EXPECT)) {
> /* we have a packet length - see if we have all of it */
> bool need_irq = !(s->loc[locty].sts & TPM_TIS_STS_VALID);
>
> len = tpm_cmd_get_size(&s->buffer);
> - if (len > s->loc[locty].w_offset) {
> + if (len > s->w_offset) {
> tpm_tis_sts_set(&s->loc[locty],
> TPM_TIS_STS_EXPECT | TPM_TIS_STS_VALID);
> } else {
> @@ -1022,8 +1019,8 @@ static void tpm_tis_reset(DeviceState *dev)
> s->loc[c].ints = 0;
> s->loc[c].state = TPM_TIS_STATE_IDLE;
>
> - s->loc[c].w_offset = 0;
> - s->loc[c].r_offset = 0;
> + s->w_offset = 0;
> + s->r_offset = 0;
> }
>
> tpm_tis_do_startup_tpm(s, s->be_buffer_size);
> --
> 2.5.5
>
>
Looks good, but I wonder why it's not part of "tpm_tis: move buffers
from localities into common location"
Not a big deal though, so
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 07/13] tpm_tis: merge r/w_offset into rw_offset
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 07/13] tpm_tis: merge r/w_offset into rw_offset Stefan Berger
@ 2017-12-21 14:41 ` Marc-André Lureau
0 siblings, 0 replies; 33+ messages in thread
From: Marc-André Lureau @ 2017-12-21 14:41 UTC (permalink / raw)
To: Stefan Berger; +Cc: QEMU, Amarnath Valluri
Hi
On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> We can now merge the r_offset and w_offset into a single rw_offset.
> This is possible since when the offset is used for writing in
> RECEPTION state then reads are ignore. Conversly, when the offset
Conversely
> is used for reading when in COMPLETION state, then writes are
> ignored.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/tpm/tpm_tis.c | 60 ++++++++++++++++++++------------------------------------
> 1 file changed, 21 insertions(+), 39 deletions(-)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index dfdddd3..7d7e2cd 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -68,8 +68,7 @@ typedef struct TPMState {
> MemoryRegion mmio;
>
> unsigned char buffer[TPM_TIS_BUFFER_MAX];
> - uint16_t w_offset;
> - uint16_t r_offset;
> + uint16_t rw_offset;
>
> uint8_t active_locty;
> uint8_t aborting_locty;
> @@ -257,7 +256,7 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
> "tpm_tis: To TPM");
>
> /*
> - * w_offset serves as length indicator for length of data;
> + * rw_offset serves as length indicator for length of data;
> * it's reset when the response comes back
> */
> s->loc[locty].state = TPM_TIS_STATE_EXECUTION;
> @@ -265,7 +264,7 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
> s->cmd = (TPMBackendCmd) {
> .locty = locty,
> .in = s->buffer,
> - .in_len = s->w_offset,
> + .in_len = s->rw_offset,
> .out = s->buffer,
> .out_len = s->be_buffer_size,
> };
> @@ -347,8 +346,7 @@ static void tpm_tis_new_active_locality(TPMState *s, uint8_t new_active_locty)
> /* abort -- this function switches the locality */
> static void tpm_tis_abort(TPMState *s, uint8_t locty)
> {
> - s->r_offset = 0;
> - s->w_offset = 0;
> + s->rw_offset = 0;
>
> DPRINTF("tpm_tis: tis_abort: new active locality is %d\n", s->next_locty);
>
> @@ -415,8 +413,7 @@ static void tpm_tis_request_completed(TPMIf *ti)
> tpm_tis_sts_set(&s->loc[locty],
> TPM_TIS_STS_VALID | TPM_TIS_STS_DATA_AVAILABLE);
> s->loc[locty].state = TPM_TIS_STATE_COMPLETION;
> - s->r_offset = 0;
> - s->w_offset = 0;
> + s->rw_offset = 0;
>
> tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
> "tpm_tis: From TPM");
> @@ -441,14 +438,14 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty)
> len = MIN(tpm_cmd_get_size(&s->buffer),
> s->be_buffer_size);
>
> - ret = s->buffer[s->r_offset++];
> - if (s->r_offset >= len) {
> + ret = s->buffer[s->rw_offset++];
> + if (s->rw_offset >= len) {
> /* got last byte */
> tpm_tis_sts_set(&s->loc[locty], TPM_TIS_STS_VALID);
> tpm_tis_raise_irq(s, locty, TPM_TIS_INT_STS_VALID);
> }
> DPRINTF("tpm_tis: tpm_tis_data_read byte 0x%02x [%d]\n",
> - ret, s->r_offset - 1);
> + ret, s->rw_offset - 1);
> }
>
> return ret;
> @@ -483,26 +480,14 @@ static void tpm_tis_dump_state(void *opaque, hwaddr addr)
> (int)tpm_tis_mmio_read(opaque, base + regs[idx], 4));
> }
>
> - DPRINTF("tpm_tis: read offset : %d\n"
> + DPRINTF("tpm_tis: r/w offset : %d\n"
> "tpm_tis: result buffer : ",
> - s->r_offset);
> + s->rw_offset);
> for (idx = 0;
> idx < MIN(tpm_cmd_get_size(&s->buffer), s->be_buffer_size);
> idx++) {
> DPRINTF("%c%02x%s",
> - s->r_offset == idx ? '>' : ' ',
> - s->buffer[idx],
> - ((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
> - }
> - DPRINTF("\n"
> - "tpm_tis: write offset : %d\n"
> - "tpm_tis: request buffer: ",
> - s->w_offset);
> - for (idx = 0;
> - idx < MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size);
> - idx++) {
> - DPRINTF("%c%02x%s",
> - s->w_offset == idx ? '>' : ' ',
> + s->rw_offset == idx ? '>' : ' ',
> s->buffer[idx],
> ((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
> }
> @@ -567,9 +552,9 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
> val = TPM_TIS_BURST_COUNT(
> MIN(tpm_cmd_get_size(&s->buffer),
> s->be_buffer_size)
> - - s->r_offset) | s->loc[locty].sts;
> + - s->rw_offset) | s->loc[locty].sts;
> } else {
> - avail = s->be_buffer_size - s->w_offset;
> + avail = s->be_buffer_size - s->rw_offset;
> /*
> * byte-sized reads should not return 0x00 for 0x100
> * available bytes.
> @@ -833,8 +818,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
> switch (s->loc[locty].state) {
>
> case TPM_TIS_STATE_READY:
> - s->w_offset = 0;
> - s->r_offset = 0;
> + s->rw_offset = 0;
> break;
>
> case TPM_TIS_STATE_IDLE:
> @@ -852,8 +836,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
> break;
>
> case TPM_TIS_STATE_COMPLETION:
> - s->w_offset = 0;
> - s->r_offset = 0;
> + s->rw_offset = 0;
> /* shortcut to ready state with C/R set */
> s->loc[locty].state = TPM_TIS_STATE_READY;
> if (!(s->loc[locty].sts & TPM_TIS_STS_COMMAND_READY)) {
> @@ -879,7 +862,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
> } else if (val == TPM_TIS_STS_RESPONSE_RETRY) {
> switch (s->loc[locty].state) {
> case TPM_TIS_STATE_COMPLETION:
> - s->r_offset = 0;
> + s->rw_offset = 0;
> tpm_tis_sts_set(&s->loc[locty],
> TPM_TIS_STS_VALID|
> TPM_TIS_STS_DATA_AVAILABLE);
> @@ -917,8 +900,8 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
> }
>
> while ((s->loc[locty].sts & TPM_TIS_STS_EXPECT) && size > 0) {
> - if (s->w_offset < s->be_buffer_size) {
> - s->buffer[s->w_offset++] =
> + if (s->rw_offset < s->be_buffer_size) {
> + s->buffer[s->rw_offset++] =
> (uint8_t)val;
> val >>= 8;
> size--;
> @@ -928,13 +911,13 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
> }
>
> /* check for complete packet */
> - if (s->w_offset > 5 &&
> + if (s->rw_offset > 5 &&
> (s->loc[locty].sts & TPM_TIS_STS_EXPECT)) {
> /* we have a packet length - see if we have all of it */
> bool need_irq = !(s->loc[locty].sts & TPM_TIS_STS_VALID);
>
> len = tpm_cmd_get_size(&s->buffer);
> - if (len > s->w_offset) {
> + if (len > s->rw_offset) {
> tpm_tis_sts_set(&s->loc[locty],
> TPM_TIS_STS_EXPECT | TPM_TIS_STS_VALID);
> } else {
> @@ -1019,8 +1002,7 @@ static void tpm_tis_reset(DeviceState *dev)
> s->loc[c].ints = 0;
> s->loc[c].state = TPM_TIS_STATE_IDLE;
>
> - s->w_offset = 0;
> - s->r_offset = 0;
> + s->rw_offset = 0;
> }
>
> tpm_tis_do_startup_tpm(s, s->be_buffer_size);
> --
> 2.5.5
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 05/13] tpm_tis: merge read and write buffer into single buffer
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 05/13] tpm_tis: merge read and write buffer into single buffer Stefan Berger
@ 2017-12-21 14:41 ` Marc-André Lureau
0 siblings, 0 replies; 33+ messages in thread
From: Marc-André Lureau @ 2017-12-21 14:41 UTC (permalink / raw)
To: Stefan Berger; +Cc: QEMU, Amarnath Valluri
On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Since we can only be in read or write mode, we can merge the buffers
> into a single buffer.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/tpm/tpm_tis.c | 29 ++++++++++++++---------------
> 1 file changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index f6f5f17..0b6dd7f 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -70,8 +70,7 @@ typedef struct TPMState {
> ISADevice busdev;
> MemoryRegion mmio;
>
> - unsigned char w_buffer[TPM_TIS_BUFFER_MAX];
> - unsigned char r_buffer[TPM_TIS_BUFFER_MAX];
> + unsigned char buffer[TPM_TIS_BUFFER_MAX];
>
> uint8_t active_locty;
> uint8_t aborting_locty;
> @@ -257,7 +256,7 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
> {
> TPMLocality *locty_data = &s->loc[locty];
>
> - tpm_tis_show_buffer(s->w_buffer, s->be_buffer_size,
> + tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
> "tpm_tis: To TPM");
>
> /*
> @@ -268,9 +267,9 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t locty)
>
> s->cmd = (TPMBackendCmd) {
> .locty = locty,
> - .in = s->w_buffer,
> + .in = s->buffer,
> .in_len = locty_data->w_offset,
> - .out = s->r_buffer,
> + .out = s->buffer,
> .out_len = s->be_buffer_size,
> };
>
> @@ -422,7 +421,7 @@ static void tpm_tis_request_completed(TPMIf *ti)
> s->loc[locty].r_offset = 0;
> s->loc[locty].w_offset = 0;
>
> - tpm_tis_show_buffer(s->r_buffer, s->be_buffer_size,
> + tpm_tis_show_buffer(s->buffer, s->be_buffer_size,
> "tpm_tis: From TPM");
>
> if (TPM_TIS_IS_VALID_LOCTY(s->next_locty)) {
> @@ -442,10 +441,10 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty)
> uint16_t len;
>
> if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) {
> - len = MIN(tpm_cmd_get_size(&s->r_buffer),
> + len = MIN(tpm_cmd_get_size(&s->buffer),
> s->be_buffer_size);
>
> - ret = s->r_buffer[s->loc[locty].r_offset++];
> + ret = s->buffer[s->loc[locty].r_offset++];
> if (s->loc[locty].r_offset >= len) {
> /* got last byte */
> tpm_tis_sts_set(&s->loc[locty], TPM_TIS_STS_VALID);
> @@ -491,11 +490,11 @@ static void tpm_tis_dump_state(void *opaque, hwaddr addr)
> "tpm_tis: result buffer : ",
> s->loc[locty].r_offset);
> for (idx = 0;
> - idx < MIN(tpm_cmd_get_size(&s->r_buffer), s->be_buffer_size);
> + idx < MIN(tpm_cmd_get_size(&s->buffer), s->be_buffer_size);
> idx++) {
> DPRINTF("%c%02x%s",
> s->loc[locty].r_offset == idx ? '>' : ' ',
> - s->r_buffer[idx],
> + s->buffer[idx],
> ((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
> }
> DPRINTF("\n"
> @@ -503,11 +502,11 @@ static void tpm_tis_dump_state(void *opaque, hwaddr addr)
> "tpm_tis: request buffer: ",
> s->loc[locty].w_offset);
> for (idx = 0;
> - idx < MIN(tpm_cmd_get_size(s->w_buffer), s->be_buffer_size);
> + idx < MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size);
> idx++) {
> DPRINTF("%c%02x%s",
> s->loc[locty].w_offset == idx ? '>' : ' ',
> - s->w_buffer[idx],
> + s->buffer[idx],
> ((idx & 0xf) == 0xf) ? "\ntpm_tis: " : "");
> }
> DPRINTF("\n");
> @@ -569,7 +568,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
> if (s->active_locty == locty) {
> if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) {
> val = TPM_TIS_BURST_COUNT(
> - MIN(tpm_cmd_get_size(&s->r_buffer),
> + MIN(tpm_cmd_get_size(&s->buffer),
> s->be_buffer_size)
> - s->loc[locty].r_offset) | s->loc[locty].sts;
> } else {
> @@ -922,7 +921,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
>
> while ((s->loc[locty].sts & TPM_TIS_STS_EXPECT) && size > 0) {
> if (s->loc[locty].w_offset < s->be_buffer_size) {
> - s->w_buffer[s->loc[locty].w_offset++] =
> + s->buffer[s->loc[locty].w_offset++] =
> (uint8_t)val;
> val >>= 8;
> size--;
> @@ -937,7 +936,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
> /* we have a packet length - see if we have all of it */
> bool need_irq = !(s->loc[locty].sts & TPM_TIS_STS_VALID);
>
> - len = tpm_cmd_get_size(&s->w_buffer);
> + len = tpm_cmd_get_size(&s->buffer);
> if (len > s->loc[locty].w_offset) {
> tpm_tis_sts_set(&s->loc[locty],
> TPM_TIS_STS_EXPECT | TPM_TIS_STS_VALID);
> --
> 2.5.5
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 08/13] tpm: Implement tpm_sized_buffer_reset
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 08/13] tpm: Implement tpm_sized_buffer_reset Stefan Berger
@ 2017-12-21 14:44 ` Marc-André Lureau
0 siblings, 0 replies; 33+ messages in thread
From: Marc-André Lureau @ 2017-12-21 14:44 UTC (permalink / raw)
To: Stefan Berger; +Cc: QEMU, Amarnath Valluri
On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Move the definition of TPMSizedBuffer out of tpm_tis.c into tpm_util.h
> and implement tpm_sized_buffer_reset() for the following patches to use.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Or maybe it's time to use qemu Buffer or glib GArray...
could be done later so,
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/tpm/tpm_tis.c | 5 -----
> hw/tpm/tpm_util.c | 7 +++++++
> hw/tpm/tpm_util.h | 7 +++++++
> 3 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 7d7e2cd..035c6ef 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -48,11 +48,6 @@ typedef enum {
> TPM_TIS_STATE_RECEPTION,
> } TPMTISState;
>
> -typedef struct TPMSizedBuffer {
> - uint32_t size;
> - uint8_t *buffer;
> -} TPMSizedBuffer;
> -
> /* locality data -- all fields are persisted */
> typedef struct TPMLocality {
> TPMTISState state;
> diff --git a/hw/tpm/tpm_util.c b/hw/tpm/tpm_util.c
> index a317243..bf97811 100644
> --- a/hw/tpm/tpm_util.c
> +++ b/hw/tpm/tpm_util.c
> @@ -288,3 +288,10 @@ int tpm_util_get_buffer_size(int tpm_fd, TPMVersion tpm_version,
>
> return 0;
> }
> +
> +void tpm_sized_buffer_reset(TPMSizedBuffer *tsb)
> +{
> + g_free(tsb->buffer);
> + tsb->buffer = NULL;
> + tsb->size = 0;
> +}
> diff --git a/hw/tpm/tpm_util.h b/hw/tpm/tpm_util.h
> index 1c17e39..26c9613 100644
> --- a/hw/tpm/tpm_util.h
> +++ b/hw/tpm/tpm_util.h
> @@ -39,4 +39,11 @@ static inline uint32_t tpm_cmd_get_size(const void *b)
> int tpm_util_get_buffer_size(int tpm_fd, TPMVersion tpm_version,
> size_t *buffersize);
>
> +typedef struct TPMSizedBuffer {
> + uint32_t size;
> + uint8_t *buffer;
> +} TPMSizedBuffer;
> +
> +void tpm_sized_buffer_reset(TPMSizedBuffer *tsb);
> +
> #endif /* TPM_TPM_UTIL_H */
> --
> 2.5.5
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 06/13] tpm_tis: move r/w_offsets to TPMState
2017-12-21 14:41 ` Marc-André Lureau
@ 2017-12-21 14:44 ` Stefan Berger
0 siblings, 0 replies; 33+ messages in thread
From: Stefan Berger @ 2017-12-21 14:44 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: QEMU, Amarnath Valluri
On 12/21/2017 09:41 AM, Marc-André Lureau wrote:
> Hi
>
> On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> Now that we have a single buffer, we also only need a single set of
>> read/write offsets into that buffer. This works since only one
>> locality can be active.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>> hw/tpm/tpm_tis.c | 57 +++++++++++++++++++++++++++-----------------------------
>> 1 file changed, 27 insertions(+), 30 deletions(-)
>>
>> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
>> index 0b6dd7f..dfdddd3 100644
>> waddr addr,
>> }
>>
>> while ((s->loc[locty].sts & TPM_TIS_STS_EXPECT) && size > 0) {
>> - if (s->loc[locty].w_offset < s->be_buffer_size) {
>> - s->buffer[s->loc[locty].w_offset++] =
>> + if (s->w_offset < s->be_buffer_size) {
>> + s->buffer[s->w_offset++] =
>> (uint8_t)val;
>> val >>= 8;
>> size--;
>> @@ -931,13 +928,13 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
>> }
>>
>> /* check for complete packet */
>> - if (s->loc[locty].w_offset > 5 &&
>> + if (s->w_offset > 5 &&
>> (s->loc[locty].sts & TPM_TIS_STS_EXPECT)) {
>> /* we have a packet length - see if we have all of it */
>> bool need_irq = !(s->loc[locty].sts & TPM_TIS_STS_VALID);
>>
>> len = tpm_cmd_get_size(&s->buffer);
>> - if (len > s->loc[locty].w_offset) {
>> + if (len > s->w_offset) {
>> tpm_tis_sts_set(&s->loc[locty],
>> TPM_TIS_STS_EXPECT | TPM_TIS_STS_VALID);
>> } else {
>> @@ -1022,8 +1019,8 @@ static void tpm_tis_reset(DeviceState *dev)
>> s->loc[c].ints = 0;
>> s->loc[c].state = TPM_TIS_STATE_IDLE;
>>
>> - s->loc[c].w_offset = 0;
>> - s->loc[c].r_offset = 0;
>> + s->w_offset = 0;
>> + s->r_offset = 0;
>> }
>>
>> tpm_tis_do_startup_tpm(s, s->be_buffer_size);
>> --
>> 2.5.5
>>
>>
> Looks good, but I wonder why it's not part of "tpm_tis: move buffers
> from localities into common location"
>
> Not a big deal though, so
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
I thought it may be easier to review/follow if I do all these steps
separately. I could squash it together.
Stefan
>
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11)
2017-11-10 14:11 [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Stefan Berger
` (12 preceding siblings ...)
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 13/13] tpm_tis: extend TPM TIS " Stefan Berger
@ 2017-12-22 12:49 ` Marc-André Lureau
2017-12-22 15:59 ` Stefan Berger
13 siblings, 1 reply; 33+ messages in thread
From: Marc-André Lureau @ 2017-12-22 12:49 UTC (permalink / raw)
To: Stefan Berger; +Cc: QEMU, Amarnath Valluri
Hi
On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> This set of patches implements support for migrating the state of the
> external 'swtpm' TPM emulator as well as that of the emulated device
> interfaces. I have primarily tested this with the TIS and TPM 1.2 so
> far, but it also seems to work with TPM 2.
>
> The TIS is simplified first by reducing the number of buffers and read
> and write offsets into these buffers. Following the state machine of the
> TIS, a single buffer and r/w offset is enough for all localities since
> only one locality can ever be active.
>
> This series applies on top of my tpm-next branch.
>
> One of the challenges that is addressed by this set of patches is the fact
> that the TPM emulator may be processing a command while the state
> serialization of the devices is supposed to happen. A necessary first step
> has been implemented here that ensures that a response has been received
> from the exernal emulator and the bottom half function, which delivers the
> response and adjusts device registers (TIS or CRB), has been executed,
> before the device's state is serialized.
>
> A subsequent extension may need to address the live migration loop and delay
> the serialization of devices until the response from the external TPM has
> been received. Though the likelihood that someone executes a long-lasting
> TPM command while this is occurring is certainly rare.
>
> Stefan
>
> Stefan Berger (13):
> tpm_tis: convert uint32_t to size_t
> tpm_tis: limit size of buffer from backend
> tpm_tis: remove TPMSizeBuffer usage
> tpm_tis: move buffers from localities into common location
> tpm_tis: merge read and write buffer into single buffer
> tpm_tis: move r/w_offsets to TPMState
> tpm_tis: merge r/w_offset into rw_offset
> tpm: Implement tpm_sized_buffer_reset
ok for the above (you could queue/pull-req this now)
> tpm: Introduce condition to notify waiters of completed command
> tpm: Introduce condition in TPM backend for notification
> tpm: implement tpm_backend_wait_cmd_completed
> tpm: extend TPM emulator with state migration support
> tpm_tis: extend TPM TIS with state migration support
Much of the complexity from this migration series comes with the
handling & synchronization of the IO thread.
I think having a seperate thread doesn't bring much today TPM thread.
it is a workaround for the chardev API being mostly synchronous. Am I
wrong? (yes, passthrough doesn't use chardev, but it should probably
use qio or chardev internally)
Other kind of devices using a seperate process suffer the same
problem. Just grep for qemu_chr_fe_write and look for the preceding
comment, it's a common flaw in qemu code base. Code use the
synchronous API, and sometime use non-blocking write
(hw/usb/redirect.c seems quite better!)
I would like to get rid of the seperate thread in TPM before we add
migration support. We should try to improve the chardev API to make it
easier to do non-blocking IO. This should considerably simplify the
above patches and benefit the rest of qemu (instead of having everyone
doing its own thing with seperate thread or other kind of continuation
state).
What do you think?
>
> backends/tpm.c | 29 +++++
> hw/tpm/tpm_emulator.c | 303 +++++++++++++++++++++++++++++++++++++++++--
> hw/tpm/tpm_tis.c | 216 +++++++++++++++++-------------
> hw/tpm/tpm_util.c | 7 +
> hw/tpm/tpm_util.h | 7 +
> include/sysemu/tpm_backend.h | 22 ++++
> 6 files changed, 483 insertions(+), 101 deletions(-)
>
> --
> 2.5.5
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 09/13] tpm: Introduce condition to notify waiters of completed command
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 09/13] tpm: Introduce condition to notify waiters of completed command Stefan Berger
@ 2017-12-22 13:24 ` Marc-André Lureau
2017-12-27 14:17 ` Stefan Berger
0 siblings, 1 reply; 33+ messages in thread
From: Marc-André Lureau @ 2017-12-22 13:24 UTC (permalink / raw)
To: Stefan Berger; +Cc: QEMU, Amarnath Valluri
On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Introduce a lock and a condition to notify anyone waiting for the completion
> of the execution of a TPM command by the backend (thread). The backend
> uses the condition to signal anyone waiting for command completion.
> We need to place the condition in two locations: one is invoked by the
> backend thread, the other by the bottom half thread.
> We will use the signaling to wait for command completion before VM
> suspend.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
> hw/tpm/tpm_tis.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 035c6ef..86e9a92 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -80,6 +80,9 @@ typedef struct TPMState {
> TPMVersion be_tpm_version;
>
> size_t be_buffer_size;
> +
> + QemuMutex state_lock;
> + QemuCond cmd_complete;
Looks like the cond is unused in the following patches.
> } TPMState;
>
> #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
> @@ -405,6 +408,8 @@ static void tpm_tis_request_completed(TPMIf *ti)
> }
> }
>
> + qemu_mutex_lock(&s->state_lock);
> +
I get grumpy with multiple threads... Potentially, many threads will
want to use TPMState: main thread, backend thread (hopefully not with
the bh), migration thread (with the next patches...), and vcpu thread.
It's hard to review which part of TPMState needs mux and which
doesn't. Why do you lock only in this function? After checking
s->cmd.selftest_done & modifying s->loc[locty].sts...
> tpm_tis_sts_set(&s->loc[locty],
> TPM_TIS_STS_VALID | TPM_TIS_STS_DATA_AVAILABLE);
> s->loc[locty].state = TPM_TIS_STATE_COMPLETION;
> @@ -419,6 +424,10 @@ static void tpm_tis_request_completed(TPMIf *ti)
>
> tpm_tis_raise_irq(s, locty,
> TPM_TIS_INT_DATA_AVAILABLE | TPM_TIS_INT_STS_VALID);
> +
> + /* notify of completed command */
> + qemu_cond_signal(&s->cmd_complete);
> + qemu_mutex_unlock(&s->state_lock);
> }
>
> /*
> @@ -1046,6 +1055,9 @@ static void tpm_tis_initfn(Object *obj)
> memory_region_init_io(&s->mmio, OBJECT(s), &tpm_tis_memory_ops,
> s, "tpm-tis-mmio",
> TPM_TIS_NUM_LOCALITIES << TPM_TIS_LOCALITY_SHIFT);
> +
> + qemu_mutex_init(&s->state_lock);
> + qemu_cond_init(&s->cmd_complete);
> }
>
> static void tpm_tis_class_init(ObjectClass *klass, void *data)
> --
> 2.5.5
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11)
2017-12-22 12:49 ` [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Marc-André Lureau
@ 2017-12-22 15:59 ` Stefan Berger
2017-12-22 16:13 ` Marc-André Lureau
0 siblings, 1 reply; 33+ messages in thread
From: Stefan Berger @ 2017-12-22 15:59 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: QEMU, Amarnath Valluri
On 12/22/2017 07:49 AM, Marc-André Lureau wrote:
> Hi
>
> On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> This set of patches implements support for migrating the state of the
>> external 'swtpm' TPM emulator as well as that of the emulated device
>> interfaces. I have primarily tested this with the TIS and TPM 1.2 so
>> far, but it also seems to work with TPM 2.
>>
>> The TIS is simplified first by reducing the number of buffers and read
>> and write offsets into these buffers. Following the state machine of the
>> TIS, a single buffer and r/w offset is enough for all localities since
>> only one locality can ever be active.
>>
>> This series applies on top of my tpm-next branch.
>>
>> One of the challenges that is addressed by this set of patches is the fact
>> that the TPM emulator may be processing a command while the state
>> serialization of the devices is supposed to happen. A necessary first step
>> has been implemented here that ensures that a response has been received
>> from the exernal emulator and the bottom half function, which delivers the
>> response and adjusts device registers (TIS or CRB), has been executed,
>> before the device's state is serialized.
>>
>> A subsequent extension may need to address the live migration loop and delay
>> the serialization of devices until the response from the external TPM has
>> been received. Though the likelihood that someone executes a long-lasting
>> TPM command while this is occurring is certainly rare.
>>
>> Stefan
>>
>> Stefan Berger (13):
>> tpm_tis: convert uint32_t to size_t
>> tpm_tis: limit size of buffer from backend
>> tpm_tis: remove TPMSizeBuffer usage
>> tpm_tis: move buffers from localities into common location
>> tpm_tis: merge read and write buffer into single buffer
>> tpm_tis: move r/w_offsets to TPMState
>> tpm_tis: merge r/w_offset into rw_offset
>> tpm: Implement tpm_sized_buffer_reset
> ok for the above (you could queue/pull-req this now)
>
>> tpm: Introduce condition to notify waiters of completed command
>> tpm: Introduce condition in TPM backend for notification
>> tpm: implement tpm_backend_wait_cmd_completed
>> tpm: extend TPM emulator with state migration support
>> tpm_tis: extend TPM TIS with state migration support
> Much of the complexity from this migration series comes with the
> handling & synchronization of the IO thread.
>
> I think having a seperate thread doesn't bring much today TPM thread.
> it is a workaround for the chardev API being mostly synchronous. Am I
> wrong? (yes, passthrough doesn't use chardev, but it should probably
> use qio or chardev internally)
>
> Other kind of devices using a seperate process suffer the same
> problem. Just grep for qemu_chr_fe_write and look for the preceding
> comment, it's a common flaw in qemu code base. Code use the
> synchronous API, and sometime use non-blocking write
> (hw/usb/redirect.c seems quite better!)
>
> I would like to get rid of the seperate thread in TPM before we add
> migration support. We should try to improve the chardev API to make it
> easier to do non-blocking IO. This should considerably simplify the
> above patches and benefit the rest of qemu (instead of having everyone
> doing its own thing with seperate thread or other kind of continuation
> state).
>
> What do you think?
I am wondering whether it will help us to get rid of the
conditions/notifications, like patches 9-11 of this series. I doubt 12
and 13 will change. At some point device state is supposed to be written
out and in case of the TPM we have to wait for the response to have come
back into the backend. We won't start listening on the file descriptor
for an outstanding response, so I guess we will still wait for a
notification in that case as well. So I am not sure which parts are
going to be simpler...
Stefan
>
>> backends/tpm.c | 29 +++++
>> hw/tpm/tpm_emulator.c | 303 +++++++++++++++++++++++++++++++++++++++++--
>> hw/tpm/tpm_tis.c | 216 +++++++++++++++++-------------
>> hw/tpm/tpm_util.c | 7 +
>> hw/tpm/tpm_util.h | 7 +
>> include/sysemu/tpm_backend.h | 22 ++++
>> 6 files changed, 483 insertions(+), 101 deletions(-)
>>
>> --
>> 2.5.5
>>
>>
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11)
2017-12-22 15:59 ` Stefan Berger
@ 2017-12-22 16:13 ` Marc-André Lureau
2017-12-22 17:47 ` Stefan Berger
2017-12-27 15:00 ` Stefan Berger
0 siblings, 2 replies; 33+ messages in thread
From: Marc-André Lureau @ 2017-12-22 16:13 UTC (permalink / raw)
To: Stefan Berger; +Cc: QEMU, Amarnath Valluri
Hi
On Fri, Dec 22, 2017 at 4:59 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> On 12/22/2017 07:49 AM, Marc-André Lureau wrote:
>>
>> Hi
>>
>> On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
>> <stefanb@linux.vnet.ibm.com> wrote:
>>>
>>> This set of patches implements support for migrating the state of the
>>> external 'swtpm' TPM emulator as well as that of the emulated device
>>> interfaces. I have primarily tested this with the TIS and TPM 1.2 so
>>> far, but it also seems to work with TPM 2.
>>>
>>> The TIS is simplified first by reducing the number of buffers and read
>>> and write offsets into these buffers. Following the state machine of the
>>> TIS, a single buffer and r/w offset is enough for all localities since
>>> only one locality can ever be active.
>>>
>>> This series applies on top of my tpm-next branch.
>>>
>>> One of the challenges that is addressed by this set of patches is the
>>> fact
>>> that the TPM emulator may be processing a command while the state
>>> serialization of the devices is supposed to happen. A necessary first
>>> step
>>> has been implemented here that ensures that a response has been received
>>> from the exernal emulator and the bottom half function, which delivers
>>> the
>>> response and adjusts device registers (TIS or CRB), has been executed,
>>> before the device's state is serialized.
>>>
>>> A subsequent extension may need to address the live migration loop and
>>> delay
>>> the serialization of devices until the response from the external TPM has
>>> been received. Though the likelihood that someone executes a long-lasting
>>> TPM command while this is occurring is certainly rare.
>>>
>>> Stefan
>>>
>>> Stefan Berger (13):
>>> tpm_tis: convert uint32_t to size_t
>>> tpm_tis: limit size of buffer from backend
>>> tpm_tis: remove TPMSizeBuffer usage
>>> tpm_tis: move buffers from localities into common location
>>> tpm_tis: merge read and write buffer into single buffer
>>> tpm_tis: move r/w_offsets to TPMState
>>> tpm_tis: merge r/w_offset into rw_offset
>>> tpm: Implement tpm_sized_buffer_reset
>>
>> ok for the above (you could queue/pull-req this now)
>>
>>> tpm: Introduce condition to notify waiters of completed command
>>> tpm: Introduce condition in TPM backend for notification
>>> tpm: implement tpm_backend_wait_cmd_completed
>>> tpm: extend TPM emulator with state migration support
>>> tpm_tis: extend TPM TIS with state migration support
>>
>> Much of the complexity from this migration series comes with the
>> handling & synchronization of the IO thread.
>>
>> I think having a seperate thread doesn't bring much today TPM thread.
>> it is a workaround for the chardev API being mostly synchronous. Am I
>> wrong? (yes, passthrough doesn't use chardev, but it should probably
>> use qio or chardev internally)
>>
>> Other kind of devices using a seperate process suffer the same
>> problem. Just grep for qemu_chr_fe_write and look for the preceding
>> comment, it's a common flaw in qemu code base. Code use the
>> synchronous API, and sometime use non-blocking write
>> (hw/usb/redirect.c seems quite better!)
>>
>> I would like to get rid of the seperate thread in TPM before we add
>> migration support. We should try to improve the chardev API to make it
>> easier to do non-blocking IO. This should considerably simplify the
>> above patches and benefit the rest of qemu (instead of having everyone
>> doing its own thing with seperate thread or other kind of continuation
>> state).
>>
>> What do you think?
>
>
> I am wondering whether it will help us to get rid of the
> conditions/notifications, like patches 9-11 of this series. I doubt 12 and
> 13 will change. At some point device state is supposed to be written out and
> in case of the TPM we have to wait for the response to have come back into
> the backend. We won't start listening on the file descriptor for an
> outstanding response, so I guess we will still wait for a notification in
> that case as well. So I am not sure which parts are going to be simpler...
Why would qemu need to wait for completion of emulator? Couldn't qemu
& emulator save the current state, including in-flights commands?
That's apparently what usbredir does.
>
> Stefan
>
>
>>
>>> backends/tpm.c | 29 +++++
>>> hw/tpm/tpm_emulator.c | 303
>>> +++++++++++++++++++++++++++++++++++++++++--
>>> hw/tpm/tpm_tis.c | 216 +++++++++++++++++-------------
>>> hw/tpm/tpm_util.c | 7 +
>>> hw/tpm/tpm_util.h | 7 +
>>> include/sysemu/tpm_backend.h | 22 ++++
>>> 6 files changed, 483 insertions(+), 101 deletions(-)
>>>
>>> --
>>> 2.5.5
>>>
>>>
>>
>>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11)
2017-12-22 16:13 ` Marc-André Lureau
@ 2017-12-22 17:47 ` Stefan Berger
2017-12-22 17:52 ` Marc-André Lureau
2017-12-27 15:00 ` Stefan Berger
1 sibling, 1 reply; 33+ messages in thread
From: Stefan Berger @ 2017-12-22 17:47 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Amarnath Valluri, QEMU
On 12/22/2017 11:13 AM, Marc-André Lureau wrote:
> Hi
>
> On Fri, Dec 22, 2017 at 4:59 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> On 12/22/2017 07:49 AM, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
>>> <stefanb@linux.vnet.ibm.com> wrote:
>>>> This set of patches implements support for migrating the state of the
>>>> external 'swtpm' TPM emulator as well as that of the emulated device
>>>> interfaces. I have primarily tested this with the TIS and TPM 1.2 so
>>>> far, but it also seems to work with TPM 2.
>>>>
>>>> The TIS is simplified first by reducing the number of buffers and read
>>>> and write offsets into these buffers. Following the state machine of the
>>>> TIS, a single buffer and r/w offset is enough for all localities since
>>>> only one locality can ever be active.
>>>>
>>>> This series applies on top of my tpm-next branch.
>>>>
>>>> One of the challenges that is addressed by this set of patches is the
>>>> fact
>>>> that the TPM emulator may be processing a command while the state
>>>> serialization of the devices is supposed to happen. A necessary first
>>>> step
>>>> has been implemented here that ensures that a response has been received
>>>> from the exernal emulator and the bottom half function, which delivers
>>>> the
>>>> response and adjusts device registers (TIS or CRB), has been executed,
>>>> before the device's state is serialized.
>>>>
>>>> A subsequent extension may need to address the live migration loop and
>>>> delay
>>>> the serialization of devices until the response from the external TPM has
>>>> been received. Though the likelihood that someone executes a long-lasting
>>>> TPM command while this is occurring is certainly rare.
>>>>
>>>> Stefan
>>>>
>>>> Stefan Berger (13):
>>>> tpm_tis: convert uint32_t to size_t
>>>> tpm_tis: limit size of buffer from backend
>>>> tpm_tis: remove TPMSizeBuffer usage
>>>> tpm_tis: move buffers from localities into common location
>>>> tpm_tis: merge read and write buffer into single buffer
>>>> tpm_tis: move r/w_offsets to TPMState
>>>> tpm_tis: merge r/w_offset into rw_offset
>>>> tpm: Implement tpm_sized_buffer_reset
>>> ok for the above (you could queue/pull-req this now)
>>>
>>>> tpm: Introduce condition to notify waiters of completed command
>>>> tpm: Introduce condition in TPM backend for notification
>>>> tpm: implement tpm_backend_wait_cmd_completed
>>>> tpm: extend TPM emulator with state migration support
>>>> tpm_tis: extend TPM TIS with state migration support
>>> Much of the complexity from this migration series comes with the
>>> handling & synchronization of the IO thread.
>>>
>>> I think having a seperate thread doesn't bring much today TPM thread.
>>> it is a workaround for the chardev API being mostly synchronous. Am I
>>> wrong? (yes, passthrough doesn't use chardev, but it should probably
>>> use qio or chardev internally)
>>>
>>> Other kind of devices using a seperate process suffer the same
>>> problem. Just grep for qemu_chr_fe_write and look for the preceding
>>> comment, it's a common flaw in qemu code base. Code use the
>>> synchronous API, and sometime use non-blocking write
>>> (hw/usb/redirect.c seems quite better!)
>>>
>>> I would like to get rid of the seperate thread in TPM before we add
>>> migration support. We should try to improve the chardev API to make it
>>> easier to do non-blocking IO. This should considerably simplify the
>>> above patches and benefit the rest of qemu (instead of having everyone
>>> doing its own thing with seperate thread or other kind of continuation
>>> state).
>>>
>>> What do you think?
>>
>> I am wondering whether it will help us to get rid of the
>> conditions/notifications, like patches 9-11 of this series. I doubt 12 and
>> 13 will change. At some point device state is supposed to be written out and
>> in case of the TPM we have to wait for the response to have come back into
>> the backend. We won't start listening on the file descriptor for an
>> outstanding response, so I guess we will still wait for a notification in
>> that case as well. So I am not sure which parts are going to be simpler...
> Why would qemu need to wait for completion of emulator? Couldn't qemu
> & emulator save the current state, including in-flights commands?
> That's apparently what usbredir does.
How could we make sure whether the PCRExtend has internally completed
the extensions of the PCR but we haven't received the response yet and
would just send the same command again? In general, the TPM cannot be
sent the same commands twice due to (sevaral) commands
(cryptographically) altering the internal state of the TPM.
Stefan
>
>> Stefan
>>
>>
>>>> backends/tpm.c | 29 +++++
>>>> hw/tpm/tpm_emulator.c | 303
>>>> +++++++++++++++++++++++++++++++++++++++++--
>>>> hw/tpm/tpm_tis.c | 216 +++++++++++++++++-------------
>>>> hw/tpm/tpm_util.c | 7 +
>>>> hw/tpm/tpm_util.h | 7 +
>>>> include/sysemu/tpm_backend.h | 22 ++++
>>>> 6 files changed, 483 insertions(+), 101 deletions(-)
>>>>
>>>> --
>>>> 2.5.5
>>>>
>>>>
>>>
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11)
2017-12-22 17:47 ` Stefan Berger
@ 2017-12-22 17:52 ` Marc-André Lureau
2017-12-22 19:18 ` Stefan Berger
0 siblings, 1 reply; 33+ messages in thread
From: Marc-André Lureau @ 2017-12-22 17:52 UTC (permalink / raw)
To: Stefan Berger; +Cc: Amarnath Valluri, QEMU
Hi
On Fri, Dec 22, 2017 at 6:47 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> On 12/22/2017 11:13 AM, Marc-André Lureau wrote:
>>
>> Hi
>>
>> On Fri, Dec 22, 2017 at 4:59 PM, Stefan Berger
>> <stefanb@linux.vnet.ibm.com> wrote:
>>>
>>> On 12/22/2017 07:49 AM, Marc-André Lureau wrote:
>>>>
>>>> Hi
>>>>
>>>> On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
>>>> <stefanb@linux.vnet.ibm.com> wrote:
>>>>>
>>>>> This set of patches implements support for migrating the state of the
>>>>> external 'swtpm' TPM emulator as well as that of the emulated device
>>>>> interfaces. I have primarily tested this with the TIS and TPM 1.2 so
>>>>> far, but it also seems to work with TPM 2.
>>>>>
>>>>> The TIS is simplified first by reducing the number of buffers and read
>>>>> and write offsets into these buffers. Following the state machine of
>>>>> the
>>>>> TIS, a single buffer and r/w offset is enough for all localities since
>>>>> only one locality can ever be active.
>>>>>
>>>>> This series applies on top of my tpm-next branch.
>>>>>
>>>>> One of the challenges that is addressed by this set of patches is the
>>>>> fact
>>>>> that the TPM emulator may be processing a command while the state
>>>>> serialization of the devices is supposed to happen. A necessary first
>>>>> step
>>>>> has been implemented here that ensures that a response has been
>>>>> received
>>>>> from the exernal emulator and the bottom half function, which delivers
>>>>> the
>>>>> response and adjusts device registers (TIS or CRB), has been executed,
>>>>> before the device's state is serialized.
>>>>>
>>>>> A subsequent extension may need to address the live migration loop and
>>>>> delay
>>>>> the serialization of devices until the response from the external TPM
>>>>> has
>>>>> been received. Though the likelihood that someone executes a
>>>>> long-lasting
>>>>> TPM command while this is occurring is certainly rare.
>>>>>
>>>>> Stefan
>>>>>
>>>>> Stefan Berger (13):
>>>>> tpm_tis: convert uint32_t to size_t
>>>>> tpm_tis: limit size of buffer from backend
>>>>> tpm_tis: remove TPMSizeBuffer usage
>>>>> tpm_tis: move buffers from localities into common location
>>>>> tpm_tis: merge read and write buffer into single buffer
>>>>> tpm_tis: move r/w_offsets to TPMState
>>>>> tpm_tis: merge r/w_offset into rw_offset
>>>>> tpm: Implement tpm_sized_buffer_reset
>>>>
>>>> ok for the above (you could queue/pull-req this now)
>>>>
>>>>> tpm: Introduce condition to notify waiters of completed command
>>>>> tpm: Introduce condition in TPM backend for notification
>>>>> tpm: implement tpm_backend_wait_cmd_completed
>>>>> tpm: extend TPM emulator with state migration support
>>>>> tpm_tis: extend TPM TIS with state migration support
>>>>
>>>> Much of the complexity from this migration series comes with the
>>>> handling & synchronization of the IO thread.
>>>>
>>>> I think having a seperate thread doesn't bring much today TPM thread.
>>>> it is a workaround for the chardev API being mostly synchronous. Am I
>>>> wrong? (yes, passthrough doesn't use chardev, but it should probably
>>>> use qio or chardev internally)
>>>>
>>>> Other kind of devices using a seperate process suffer the same
>>>> problem. Just grep for qemu_chr_fe_write and look for the preceding
>>>> comment, it's a common flaw in qemu code base. Code use the
>>>> synchronous API, and sometime use non-blocking write
>>>> (hw/usb/redirect.c seems quite better!)
>>>>
>>>> I would like to get rid of the seperate thread in TPM before we add
>>>> migration support. We should try to improve the chardev API to make it
>>>> easier to do non-blocking IO. This should considerably simplify the
>>>> above patches and benefit the rest of qemu (instead of having everyone
>>>> doing its own thing with seperate thread or other kind of continuation
>>>> state).
>>>>
>>>> What do you think?
>>>
>>>
>>> I am wondering whether it will help us to get rid of the
>>> conditions/notifications, like patches 9-11 of this series. I doubt 12
>>> and
>>> 13 will change. At some point device state is supposed to be written out
>>> and
>>> in case of the TPM we have to wait for the response to have come back
>>> into
>>> the backend. We won't start listening on the file descriptor for an
>>> outstanding response, so I guess we will still wait for a notification in
>>> that case as well. So I am not sure which parts are going to be
>>> simpler...
>>
>> Why would qemu need to wait for completion of emulator? Couldn't qemu
>> & emulator save the current state, including in-flights commands?
>> That's apparently what usbredir does.
>
>
> How could we make sure whether the PCRExtend has internally completed the
> extensions of the PCR but we haven't received the response yet and would
> just send the same command again? In general, the TPM cannot be sent the
> same commands twice due to (sevaral) commands (cryptographically) altering
> the internal state of the TPM.
>
Why would it send the same command again? If the commands is still
in-flight on emulator side, ex:
- send PCRExtend cmd
- send SaveState cmd
- receive SaveState reply
Then the received state should be able to restore the emulator so that
PCRExtend reply is received after migration.
Wouldn't that work?
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11)
2017-12-22 17:52 ` Marc-André Lureau
@ 2017-12-22 19:18 ` Stefan Berger
0 siblings, 0 replies; 33+ messages in thread
From: Stefan Berger @ 2017-12-22 19:18 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Amarnath Valluri, QEMU
On 12/22/2017 12:52 PM, Marc-André Lureau wrote:
> Hi
>
> On Fri, Dec 22, 2017 at 6:47 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> On 12/22/2017 11:13 AM, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Fri, Dec 22, 2017 at 4:59 PM, Stefan Berger
>>> <stefanb@linux.vnet.ibm.com> wrote:
>>>> On 12/22/2017 07:49 AM, Marc-André Lureau wrote:
>>>>> Hi
>>>>>
>>>>> On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
>>>>> <stefanb@linux.vnet.ibm.com> wrote:
>>>>>> This set of patches implements support for migrating the state of the
>>>>>> external 'swtpm' TPM emulator as well as that of the emulated device
>>>>>> interfaces. I have primarily tested this with the TIS and TPM 1.2 so
>>>>>> far, but it also seems to work with TPM 2.
>>>>>>
>>>>>> The TIS is simplified first by reducing the number of buffers and read
>>>>>> and write offsets into these buffers. Following the state machine of
>>>>>> the
>>>>>> TIS, a single buffer and r/w offset is enough for all localities since
>>>>>> only one locality can ever be active.
>>>>>>
>>>>>> This series applies on top of my tpm-next branch.
>>>>>>
>>>>>> One of the challenges that is addressed by this set of patches is the
>>>>>> fact
>>>>>> that the TPM emulator may be processing a command while the state
>>>>>> serialization of the devices is supposed to happen. A necessary first
>>>>>> step
>>>>>> has been implemented here that ensures that a response has been
>>>>>> received
>>>>>> from the exernal emulator and the bottom half function, which delivers
>>>>>> the
>>>>>> response and adjusts device registers (TIS or CRB), has been executed,
>>>>>> before the device's state is serialized.
>>>>>>
>>>>>> A subsequent extension may need to address the live migration loop and
>>>>>> delay
>>>>>> the serialization of devices until the response from the external TPM
>>>>>> has
>>>>>> been received. Though the likelihood that someone executes a
>>>>>> long-lasting
>>>>>> TPM command while this is occurring is certainly rare.
>>>>>>
>>>>>> Stefan
>>>>>>
>>>>>> Stefan Berger (13):
>>>>>> tpm_tis: convert uint32_t to size_t
>>>>>> tpm_tis: limit size of buffer from backend
>>>>>> tpm_tis: remove TPMSizeBuffer usage
>>>>>> tpm_tis: move buffers from localities into common location
>>>>>> tpm_tis: merge read and write buffer into single buffer
>>>>>> tpm_tis: move r/w_offsets to TPMState
>>>>>> tpm_tis: merge r/w_offset into rw_offset
>>>>>> tpm: Implement tpm_sized_buffer_reset
>>>>> ok for the above (you could queue/pull-req this now)
>>>>>
>>>>>> tpm: Introduce condition to notify waiters of completed command
>>>>>> tpm: Introduce condition in TPM backend for notification
>>>>>> tpm: implement tpm_backend_wait_cmd_completed
>>>>>> tpm: extend TPM emulator with state migration support
>>>>>> tpm_tis: extend TPM TIS with state migration support
>>>>> Much of the complexity from this migration series comes with the
>>>>> handling & synchronization of the IO thread.
>>>>>
>>>>> I think having a seperate thread doesn't bring much today TPM thread.
>>>>> it is a workaround for the chardev API being mostly synchronous. Am I
>>>>> wrong? (yes, passthrough doesn't use chardev, but it should probably
>>>>> use qio or chardev internally)
>>>>>
>>>>> Other kind of devices using a seperate process suffer the same
>>>>> problem. Just grep for qemu_chr_fe_write and look for the preceding
>>>>> comment, it's a common flaw in qemu code base. Code use the
>>>>> synchronous API, and sometime use non-blocking write
>>>>> (hw/usb/redirect.c seems quite better!)
>>>>>
>>>>> I would like to get rid of the seperate thread in TPM before we add
>>>>> migration support. We should try to improve the chardev API to make it
>>>>> easier to do non-blocking IO. This should considerably simplify the
>>>>> above patches and benefit the rest of qemu (instead of having everyone
>>>>> doing its own thing with seperate thread or other kind of continuation
>>>>> state).
>>>>>
>>>>> What do you think?
>>>>
>>>> I am wondering whether it will help us to get rid of the
>>>> conditions/notifications, like patches 9-11 of this series. I doubt 12
>>>> and
>>>> 13 will change. At some point device state is supposed to be written out
>>>> and
>>>> in case of the TPM we have to wait for the response to have come back
>>>> into
>>>> the backend. We won't start listening on the file descriptor for an
>>>> outstanding response, so I guess we will still wait for a notification in
>>>> that case as well. So I am not sure which parts are going to be
>>>> simpler...
>>> Why would qemu need to wait for completion of emulator? Couldn't qemu
>>> & emulator save the current state, including in-flights commands?
>>> That's apparently what usbredir does.
>>
>> How could we make sure whether the PCRExtend has internally completed the
>> extensions of the PCR but we haven't received the response yet and would
>> just send the same command again? In general, the TPM cannot be sent the
>> same commands twice due to (sevaral) commands (cryptographically) altering
>> the internal state of the TPM.
>>
> Why would it send the same command again? If the commands is still
> in-flight on emulator side, ex:
>
> - send PCRExtend cmd
> - send SaveState cmd
> - receive SaveState reply
>
> Then the received state should be able to restore the emulator so that
> PCRExtend reply is received after migration.
>
> Wouldn't that work?
If the command has completed entirely, we can also just wait for the
response to come back. Otherwise commands could be in the middle of
processing and what would we do then? Freeze the CPU state of the
process that's executing the swtpm? No, I think we need to wait for the
response to come back and then retrieve the state blobs of the TPM.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 09/13] tpm: Introduce condition to notify waiters of completed command
2017-12-22 13:24 ` Marc-André Lureau
@ 2017-12-27 14:17 ` Stefan Berger
0 siblings, 0 replies; 33+ messages in thread
From: Stefan Berger @ 2017-12-27 14:17 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: QEMU, Amarnath Valluri
On 12/22/2017 08:24 AM, Marc-André Lureau wrote:
> On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> Introduce a lock and a condition to notify anyone waiting for the completion
>> of the execution of a TPM command by the backend (thread). The backend
>> uses the condition to signal anyone waiting for command completion.
>> We need to place the condition in two locations: one is invoked by the
>> backend thread, the other by the bottom half thread.
>> We will use the signaling to wait for command completion before VM
>> suspend.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>> hw/tpm/tpm_tis.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
>> index 035c6ef..86e9a92 100644
>> --- a/hw/tpm/tpm_tis.c
>> +++ b/hw/tpm/tpm_tis.c
>> @@ -80,6 +80,9 @@ typedef struct TPMState {
>> TPMVersion be_tpm_version;
>>
>> size_t be_buffer_size;
>> +
>> + QemuMutex state_lock;
>> + QemuCond cmd_complete;
> Looks like the cond is unused in the following patches.
You are right. I will drop this patch. It may have been needed before
the refactoring you did.
Stefan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 10/13] tpm: Introduce condition in TPM backend for notification
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 10/13] tpm: Introduce condition in TPM backend for notification Stefan Berger
@ 2017-12-27 14:19 ` Stefan Berger
0 siblings, 0 replies; 33+ messages in thread
From: Stefan Berger @ 2017-12-27 14:19 UTC (permalink / raw)
To: qemu-devel, marcandre.lureau; +Cc: amarnath.valluri
On 11/10/2017 09:11 AM, Stefan Berger wrote:
> TPM backends will suspend independently of the frontends. Also
> here we need to be able to wait for the TPM command to have been
> completely processed.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
> backends/tpm.c | 19 +++++++++++++++++++
> include/sysemu/tpm_backend.h | 14 ++++++++++++++
> 2 files changed, 33 insertions(+)
>
> diff --git a/backends/tpm.c b/backends/tpm.c
> index 91222c5..bf0e120 100644
> --- a/backends/tpm.c
> +++ b/backends/tpm.c
> @@ -20,6 +20,14 @@
> #include "qemu/thread.h"
> #include "qemu/main-loop.h"
>
> +void tpm_backend_cmd_completed(TPMBackend *s)
> +{
> + qemu_mutex_lock(&s->state_lock);
> + s->tpm_busy = false;
> + qemu_cond_signal(&s->cmd_complete);
> + qemu_mutex_unlock(&s->state_lock);
> +}
> +
> static void tpm_backend_request_completed_bh(void *opaque)
> {
> TPMBackend *s = TPM_BACKEND(opaque);
> @@ -36,6 +44,9 @@ static void tpm_backend_worker_thread(gpointer data, gpointer user_data)
> k->handle_request(s, (TPMBackendCmd *)data);
>
> qemu_bh_schedule(s->bh);
> +
> + /* result delivered */
> + tpm_backend_cmd_completed(s);
> }
>
> static void tpm_backend_thread_end(TPMBackend *s)
> @@ -64,6 +75,10 @@ int tpm_backend_init(TPMBackend *s, TPMIf *tpmif, Error **errp)
> object_ref(OBJECT(tpmif));
>
> s->had_startup_error = false;
> + s->tpm_busy = false;
> +
> + qemu_mutex_init(&s->state_lock);
> + qemu_cond_init(&s->cmd_complete);
>
> return 0;
> }
> @@ -93,6 +108,10 @@ bool tpm_backend_had_startup_error(TPMBackend *s)
>
> void tpm_backend_deliver_request(TPMBackend *s, TPMBackendCmd *cmd)
> {
> + qemu_mutex_lock(&s->state_lock);
> + s->tpm_busy = true;
> + qemu_mutex_unlock(&s->state_lock);
> +
> g_thread_pool_push(s->thread_pool, cmd, NULL);
> }
>
> diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
> index 0d6c994..39598e3 100644
> --- a/include/sysemu/tpm_backend.h
> +++ b/include/sysemu/tpm_backend.h
> @@ -18,6 +18,7 @@
> #include "qapi-types.h"
> #include "qemu/option.h"
> #include "sysemu/tpm.h"
> +#include "qemu/thread.h"
>
> #define TYPE_TPM_BACKEND "tpm-backend"
> #define TPM_BACKEND(obj) \
> @@ -53,6 +54,10 @@ struct TPMBackend {
> char *id;
>
> QLIST_ENTRY(TPMBackend) list;
> +
> + QemuMutex state_lock;
This lock only locks the tpm_busy flag. So maybe I should rename it to
tpm_busy_lock. It's not meant to be a general state lock.
Stefan
> + QemuCond cmd_complete; /* signaled once tpm_busy is false */
> + bool tpm_busy;
> };
>
> struct TPMBackendClass {
> @@ -206,6 +211,15 @@ size_t tpm_backend_get_buffer_size(TPMBackend *s);
> */
> TPMInfo *tpm_backend_query_tpm(TPMBackend *s);
>
> +/**
> + * tpm_backend_cmd_completed:
> + * @s: the backend
> + *
> + * Mark the backend as not busy and notify anyone interested
> + * in the state changed
> + */
> +void tpm_backend_cmd_completed(TPMBackend *s);
> +
> TPMBackend *qemu_find_tpm_be(const char *id);
>
> #endif
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11)
2017-12-22 16:13 ` Marc-André Lureau
2017-12-22 17:47 ` Stefan Berger
@ 2017-12-27 15:00 ` Stefan Berger
1 sibling, 0 replies; 33+ messages in thread
From: Stefan Berger @ 2017-12-27 15:00 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: QEMU, Amarnath Valluri
On 12/22/2017 11:13 AM, Marc-André Lureau wrote:
> Hi
>
> On Fri, Dec 22, 2017 at 4:59 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> On 12/22/2017 07:49 AM, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
>>> <stefanb@linux.vnet.ibm.com> wrote:
>>>> This set of patches implements support for migrating the state of the
>>>> external 'swtpm' TPM emulator as well as that of the emulated device
>>>> interfaces. I have primarily tested this with the TIS and TPM 1.2 so
>>>> far, but it also seems to work with TPM 2.
>>>>
>>>> The TIS is simplified first by reducing the number of buffers and read
>>>> and write offsets into these buffers. Following the state machine of the
>>>> TIS, a single buffer and r/w offset is enough for all localities since
>>>> only one locality can ever be active.
>>>>
>>>> This series applies on top of my tpm-next branch.
>>>>
>>>> One of the challenges that is addressed by this set of patches is the
>>>> fact
>>>> that the TPM emulator may be processing a command while the state
>>>> serialization of the devices is supposed to happen. A necessary first
>>>> step
>>>> has been implemented here that ensures that a response has been received
>>>> from the exernal emulator and the bottom half function, which delivers
>>>> the
>>>> response and adjusts device registers (TIS or CRB), has been executed,
>>>> before the device's state is serialized.
>>>>
>>>> A subsequent extension may need to address the live migration loop and
>>>> delay
>>>> the serialization of devices until the response from the external TPM has
>>>> been received. Though the likelihood that someone executes a long-lasting
>>>> TPM command while this is occurring is certainly rare.
>>>>
>>>> Stefan
>>>>
>>>> Stefan Berger (13):
>>>> tpm_tis: convert uint32_t to size_t
>>>> tpm_tis: limit size of buffer from backend
>>>> tpm_tis: remove TPMSizeBuffer usage
>>>> tpm_tis: move buffers from localities into common location
>>>> tpm_tis: merge read and write buffer into single buffer
>>>> tpm_tis: move r/w_offsets to TPMState
>>>> tpm_tis: merge r/w_offset into rw_offset
>>>> tpm: Implement tpm_sized_buffer_reset
>>> ok for the above (you could queue/pull-req this now)
>>>
>>>> tpm: Introduce condition to notify waiters of completed command
>>>> tpm: Introduce condition in TPM backend for notification
>>>> tpm: implement tpm_backend_wait_cmd_completed
>>>> tpm: extend TPM emulator with state migration support
>>>> tpm_tis: extend TPM TIS with state migration support
>>> Much of the complexity from this migration series comes with the
>>> handling & synchronization of the IO thread.
>>>
>>> I think having a seperate thread doesn't bring much today TPM thread.
>>> it is a workaround for the chardev API being mostly synchronous. Am I
>>> wrong? (yes, passthrough doesn't use chardev, but it should probably
>>> use qio or chardev internally)
>>>
>>> Other kind of devices using a seperate process suffer the same
>>> problem. Just grep for qemu_chr_fe_write and look for the preceding
>>> comment, it's a common flaw in qemu code base. Code use the
>>> synchronous API, and sometime use non-blocking write
>>> (hw/usb/redirect.c seems quite better!)
>>>
>>> I would like to get rid of the seperate thread in TPM before we add
>>> migration support. We should try to improve the chardev API to make it
>>> easier to do non-blocking IO. This should considerably simplify the
>>> above patches and benefit the rest of qemu (instead of having everyone
>>> doing its own thing with seperate thread or other kind of continuation
>>> state).
>>>
>>> What do you think?
>>
>> I am wondering whether it will help us to get rid of the
>> conditions/notifications, like patches 9-11 of this series. I doubt 12 and
>> 13 will change. At some point device state is supposed to be written out and
>> in case of the TPM we have to wait for the response to have come back into
>> the backend. We won't start listening on the file descriptor for an
>> outstanding response, so I guess we will still wait for a notification in
>> that case as well. So I am not sure which parts are going to be simpler...
> Why would qemu need to wait for completion of emulator? Couldn't qemu
> & emulator save the current state, including in-flights commands?
> That's apparently what usbredir does.
The thread is sending the commands to the external emulator and waits
for the reception of the response. Once the response is received, the
delivery of the response is handed off to the bottom half. Using the
bottom half avoids synchronization primitives since we know that no
other thread is in the device emulator when it runs (except for our
thread sending to and receiving from the emulator).
This series of patches introduces a tpm_busy flag that the device
emulation thread sets once it hands off a command to the thread for
processing. It does this in tpm_backend_deliver_request(). It resets
that flag once it has scheduled the bottom half to run for delivering
the response to the device emulator (TIS, CRB etc.). This in turn is
happening in tpm_backend_worker_thread().
For the migration we have the following cases once the device (frontend
or backend) is supposed to write out its state:
[Once the state of the device is supposed to be written out we know what
the OS is paused and no further TIS registers will be written to after
that and no further thread is busy emulating writes to TIS registers.
TIS being an example here for 'device emulation'].
- no TPM command is currently processed: the tpm_busy flag is false and
we can write out the state immediately. The .pre_save function does
nothing in this case.
- a TPM command is currently processed: we have to wait for the response
to come back and the thread to set the tpm_busy flag to false. This is
what tpm_backend_wait_cmd_completed() is good for. Once that has
happened, the bottom half will have been scheduled by the thread but the
bottom half will not run at this point, so we have to do what the bottom
half is supposed to do. All this is happening in the .pre_save function.
Here we call tpm_backend_wait_cmd_completed() which returns once the
response has been received back. We then check a state variable that
indicates whether the front-end is executing a command and run the
bottom half's function. (Improvement: we could also return true in case
we had to wait for the response to come back and avoid the check on the
state variable and just run the bottom half's function due to the
preceding wait)
Your refactoring has actually helped a lot in simplifying this and
pushed a lot of the code into the common tpm_backend_* functions. The
code to suspend/resume looks rather simple at this point. It mostly
comes down to refactoring the function the bottom half is supposed to
run so it can be run from the .pre_save function as well.
TIS:
https://github.com/stefanberger/qemu-tpm/commit/3b0689619be9a1f3fbbea2aa2f98802512f96099
CRB:
https://github.com/stefanberger/qemu-tpm/commit/62db4ace06e77c8e5f8274e8cb9240e5f8d663ab
SPAPR:
https://github.com/stefanberger/qemu-tpm/commit/1f31cd8b7f690f750405d911923f624b5f856a04
I may now modify tpm_backend_wait_cmd_completed() to 'bool
run_bh_function = tpm_backend_wait_cmd_completed(s)'. This should help
the cause and make the 3 devices' suspend code even look more similar.
Regards,
Stefan
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2017-12-27 15:00 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-10 14:11 [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Stefan Berger
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 01/13] tpm_tis: convert uint32_t to size_t Stefan Berger
2017-12-21 14:11 ` Marc-André Lureau
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 02/13] tpm_tis: limit size of buffer from backend Stefan Berger
2017-12-21 14:11 ` Marc-André Lureau
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 03/13] tpm_tis: remove TPMSizeBuffer usage Stefan Berger
2017-12-21 14:11 ` Marc-André Lureau
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 04/13] tpm_tis: move buffers from localities into common location Stefan Berger
2017-12-21 14:11 ` Marc-André Lureau
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 05/13] tpm_tis: merge read and write buffer into single buffer Stefan Berger
2017-12-21 14:41 ` Marc-André Lureau
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 06/13] tpm_tis: move r/w_offsets to TPMState Stefan Berger
2017-12-21 14:41 ` Marc-André Lureau
2017-12-21 14:44 ` Stefan Berger
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 07/13] tpm_tis: merge r/w_offset into rw_offset Stefan Berger
2017-12-21 14:41 ` Marc-André Lureau
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 08/13] tpm: Implement tpm_sized_buffer_reset Stefan Berger
2017-12-21 14:44 ` Marc-André Lureau
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 09/13] tpm: Introduce condition to notify waiters of completed command Stefan Berger
2017-12-22 13:24 ` Marc-André Lureau
2017-12-27 14:17 ` Stefan Berger
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 10/13] tpm: Introduce condition in TPM backend for notification Stefan Berger
2017-12-27 14:19 ` Stefan Berger
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 11/13] tpm: implement tpm_backend_wait_cmd_completed Stefan Berger
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 12/13] tpm: extend TPM emulator with state migration support Stefan Berger
2017-11-10 14:11 ` [Qemu-devel] [PATCH v3 13/13] tpm_tis: extend TPM TIS " Stefan Berger
2017-12-22 12:49 ` [Qemu-devel] [PATCH v3 00/13] tpm: Extend TPM with state migration support (not 2.11) Marc-André Lureau
2017-12-22 15:59 ` Stefan Berger
2017-12-22 16:13 ` Marc-André Lureau
2017-12-22 17:47 ` Stefan Berger
2017-12-22 17:52 ` Marc-André Lureau
2017-12-22 19:18 ` Stefan Berger
2017-12-27 15:00 ` Stefan Berger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.