* [PATCH v5 0/5] Add vTPM emulator support for ppc64 platform
@ 2019-12-12 20:24 Stefan Berger
2019-12-12 20:24 ` [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface Stefan Berger
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Stefan Berger @ 2019-12-12 20:24 UTC (permalink / raw)
To: qemu-ppc; +Cc: marcandre.lureau, Stefan Berger, qemu-devel, david
The following series of patches adds vTPM emulator support for the
ppc64 platform (pSeries).
It can be tested as follows with swtpm/libtpms:
mkdir /tmp/mytpm1
swtpm socket --tpmstate dir=/tmp/mytpm1 \
--ctrl type=unixio,path=/tmp/mytpm1/swtpm-sock \
--log level=20
If TPM 2 is desired, add --tpm2 as parameter to the above.
In another terminal start QEMU:
sudo ./ppc64-softmmu/qemu-system-ppc64 -display sdl \
-machine pseries,accel=kvm \
-m 1024 -bios slof.bin -boot menu=on \
-nodefaults -device VGA -device pci-ohci -device usb-kbd \
-chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm \
-device tpm-spapr,tpmdev=tpm0 \
-device spapr-vscsi,id=scsi0,reg=0x00002000 \
-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,id=virtio-disk0 \
-drive file=test.img,format=raw,if=none,id=drive-virtio-disk0
Links:
- libtpms: https://github.com/stefanberger/libtpms/wiki
- swtpm: https://github.com/stefanberger/swtpm/wiki
Changes:
v4->v5:
- use runstate_check(RUN_STATE_FINISH_MIGRATE) to check whether devices
are suspending; ditch 3 patches in this series that tried to do similar
v3->v4:
- addressed comments to v3
- reworked suspend/resume support that requires extensions to backends
v2->v3:
- patch 1: a TPM 2 is identified by IBM,vtpm20 in the compatible node
- patch 1: convert to tracing to display Tx and Rx buffers
- added documentation patch
- added patch to enable TPM device as part of pSeries
v1->v2:
- followed Cedric Le Goater's suggestions to patch 1
- send appropriate CRQ error responses if DMA read or write fails
- renamed tpm_spapr_got_payload to tpm_spapr_process_cmd and
pass endianess-adjusted data pointer from CRQ to it
Regards,
Stefan
Stefan Berger (5):
tpm_spapr: Support TPM for ppc64 using CRQ based interface
tpm: Return bool from tpm_backend_finish_sync
tpm_spapr: Support suspend and resume
hw/ppc/Kconfig: Enable TPM_SPAPR as part of PSERIES config
docs: tpm: Add example command line for ppc64 and tpm-spapr
backends/tpm.c | 6 +-
docs/specs/tpm.txt | 20 +-
hw/ppc/Kconfig | 1 +
hw/tpm/Kconfig | 6 +
hw/tpm/Makefile.objs | 1 +
hw/tpm/tpm_spapr.c | 458 +++++++++++++++++++++++++++++++++++
hw/tpm/trace-events | 14 ++
include/sysemu/tpm.h | 3 +
include/sysemu/tpm_backend.h | 4 +-
qapi/tpm.json | 6 +-
10 files changed, 513 insertions(+), 6 deletions(-)
create mode 100644 hw/tpm/tpm_spapr.c
--
2.21.0
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
2019-12-12 20:24 [PATCH v5 0/5] Add vTPM emulator support for ppc64 platform Stefan Berger
@ 2019-12-12 20:24 ` Stefan Berger
2019-12-12 20:33 ` Eric Blake
2019-12-13 5:34 ` David Gibson
2019-12-12 20:24 ` [PATCH v5 2/5] tpm: Return bool from tpm_backend_finish_sync Stefan Berger
` (3 subsequent siblings)
4 siblings, 2 replies; 19+ messages in thread
From: Stefan Berger @ 2019-12-12 20:24 UTC (permalink / raw)
To: qemu-ppc; +Cc: marcandre.lureau, Stefan Berger, qemu-devel, david
Implement support for TPM on ppc64 by implementing the vTPM CRQ interface
as a frontend. It can use the tpm_emulator driver backend with the external
swtpm.
The Linux vTPM driver for ppc64 works with this emulation.
This TPM emulator also handles the TPM 2 case.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
index 4c8ee87d67..66a570aac1 100644
--- a/hw/tpm/Kconfig
+++ b/hw/tpm/Kconfig
@@ -22,3 +22,9 @@ config TPM_EMULATOR
bool
default y
depends on TPMDEV
+
+config TPM_SPAPR
+ bool
+ default n
+ select TPMDEV
+ depends on PSERIES
diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
index de0b85d02a..85eb99ae05 100644
--- a/hw/tpm/Makefile.objs
+++ b/hw/tpm/Makefile.objs
@@ -4,3 +4,4 @@ common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
common-obj-$(CONFIG_TPM_CRB) += tpm_crb.o
common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
common-obj-$(CONFIG_TPM_EMULATOR) += tpm_emulator.o
+obj-$(CONFIG_TPM_SPAPR) += tpm_spapr.o
diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
new file mode 100644
index 0000000000..c4a67e2403
--- /dev/null
+++ b/hw/tpm/tpm_spapr.c
@@ -0,0 +1,405 @@
+/*
+ * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
+ *
+ * PAPR Virtual TPM
+ *
+ * Copyright (c) 2015, 2017 IBM Corporation.
+ *
+ * Authors:
+ * Stefan Berger <stefanb@linux.vnet.ibm.com>
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+
+#include "sysemu/tpm_backend.h"
+#include "tpm_int.h"
+#include "tpm_util.h"
+
+#include "hw/ppc/spapr.h"
+#include "hw/ppc/spapr_vio.h"
+#include "trace.h"
+
+#define DEBUG_SPAPR 0
+
+#define VIO_SPAPR_VTPM(obj) \
+ OBJECT_CHECK(SPAPRvTPMState, (obj), TYPE_TPM_SPAPR)
+
+typedef struct VioCRQ {
+ uint8_t valid; /* 0x80: cmd; 0xc0: init crq */
+ /* 0x81-0x83: CRQ message response */
+ uint8_t msg; /* see below */
+ uint16_t len; /* len of TPM request; len of TPM response */
+ uint32_t data; /* rtce_dma_handle when sending TPM request */
+ uint64_t reserved;
+} VioCRQ;
+
+typedef union TPMSpaprCRQ {
+ VioCRQ s;
+ uint8_t raw[sizeof(VioCRQ)];
+} TPMSpaprCRQ;
+
+#define SPAPR_VTPM_VALID_INIT_CRQ_COMMAND 0xC0
+#define SPAPR_VTPM_VALID_COMMAND 0x80
+#define SPAPR_VTPM_MSG_RESULT 0x80
+
+/* msg types for valid = SPAPR_VTPM_VALID_INIT_CRQ */
+#define SPAPR_VTPM_INIT_CRQ_RESULT 0x1
+#define SPAPR_VTPM_INIT_CRQ_COMPLETE_RESULT 0x2
+
+/* msg types for valid = SPAPR_VTPM_VALID_CMD */
+#define SPAPR_VTPM_GET_VERSION 0x1
+#define SPAPR_VTPM_TPM_COMMAND 0x2
+#define SPAPR_VTPM_GET_RTCE_BUFFER_SIZE 0x3
+#define SPAPR_VTPM_PREPARE_TO_SUSPEND 0x4
+
+/* response error messages */
+#define SPAPR_VTPM_VTPM_ERROR 0xff
+
+/* error codes */
+#define SPAPR_VTPM_ERR_COPY_IN_FAILED 0x3
+#define SPAPR_VTPM_ERR_COPY_OUT_FAILED 0x4
+
+#define MAX_BUFFER_SIZE TARGET_PAGE_SIZE
+
+typedef struct {
+ SpaprVioDevice vdev;
+
+ TPMSpaprCRQ crq; /* track single TPM command */
+
+ uint8_t state;
+#define SPAPR_VTPM_STATE_NONE 0
+#define SPAPR_VTPM_STATE_EXECUTION 1
+#define SPAPR_VTPM_STATE_COMPLETION 2
+
+ unsigned char buffer[MAX_BUFFER_SIZE];
+
+ TPMBackendCmd cmd;
+
+ TPMBackend *be_driver;
+ TPMVersion be_tpm_version;
+
+ size_t be_buffer_size;
+} SPAPRvTPMState;
+
+static void tpm_spapr_show_buffer(const unsigned char *buffer,
+ size_t buffer_size, const char *string)
+{
+ size_t len, i;
+ char *line_buffer, *p;
+
+ len = MIN(tpm_cmd_get_size(buffer), buffer_size);
+
+ /*
+ * allocate enough room for 3 chars per buffer entry plus a
+ * newline after every 16 chars and a final null terminator.
+ */
+ line_buffer = g_malloc(len * 3 + (len / 16) + 1);
+
+ for (i = 0, p = line_buffer; i < len; i++) {
+ if (i && !(i % 16)) {
+ p += sprintf(p, "\n");
+ }
+ p += sprintf(p, "%.2X ", buffer[i]);
+ }
+ trace_tpm_spapr_show_buffer(string, len, line_buffer);
+
+ g_free(line_buffer);
+}
+
+/*
+ * Send a request to the TPM.
+ */
+static void tpm_spapr_tpm_send(SPAPRvTPMState *s)
+{
+ if (trace_event_get_state_backends(TRACE_TPM_SPAPR_SHOW_BUFFER)) {
+ tpm_spapr_show_buffer(s->buffer, sizeof(s->buffer), "To TPM");
+ }
+
+ s->state = SPAPR_VTPM_STATE_EXECUTION;
+ s->cmd = (TPMBackendCmd) {
+ .locty = 0,
+ .in = s->buffer,
+ .in_len = MIN(tpm_cmd_get_size(s->buffer), sizeof(s->buffer)),
+ .out = s->buffer,
+ .out_len = sizeof(s->buffer),
+ };
+
+ tpm_backend_deliver_request(s->be_driver, &s->cmd);
+}
+
+static int tpm_spapr_process_cmd(SPAPRvTPMState *s, uint64_t dataptr)
+{
+ long rc;
+
+ /* a max. of be_buffer_size bytes can be transported */
+ rc = spapr_vio_dma_read(&s->vdev, dataptr,
+ s->buffer, s->be_buffer_size);
+ if (rc) {
+ error_report("tpm_spapr_got_payload: DMA read failure");
+ }
+ /* let vTPM handle any malformed request */
+ tpm_spapr_tpm_send(s);
+
+ return rc;
+}
+
+static int tpm_spapr_do_crq(struct SpaprVioDevice *dev, uint8_t *crq_data)
+{
+ SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
+ TPMSpaprCRQ local_crq;
+ TPMSpaprCRQ *crq = &s->crq; /* requests only */
+ int rc;
+
+ memcpy(&local_crq.raw, crq_data, sizeof(local_crq.raw));
+
+ trace_tpm_spapr_do_crq(local_crq.raw[0], local_crq.raw[1]);
+
+ switch (local_crq.s.valid) {
+ case SPAPR_VTPM_VALID_INIT_CRQ_COMMAND: /* Init command/response */
+
+ /* Respond to initialization request */
+ switch (local_crq.s.msg) {
+ case SPAPR_VTPM_INIT_CRQ_RESULT:
+ trace_tpm_spapr_do_crq_crq_result();
+ memset(local_crq.raw, 0, sizeof(local_crq.raw));
+ local_crq.s.valid = SPAPR_VTPM_VALID_INIT_CRQ_COMMAND;
+ local_crq.s.msg = SPAPR_VTPM_INIT_CRQ_RESULT;
+ spapr_vio_send_crq(dev, local_crq.raw);
+ break;
+
+ case SPAPR_VTPM_INIT_CRQ_COMPLETE_RESULT:
+ trace_tpm_spapr_do_crq_crq_complete_result();
+ memset(local_crq.raw, 0, sizeof(local_crq.raw));
+ local_crq.s.valid = SPAPR_VTPM_VALID_INIT_CRQ_COMMAND;
+ local_crq.s.msg = SPAPR_VTPM_INIT_CRQ_COMPLETE_RESULT;
+ spapr_vio_send_crq(dev, local_crq.raw);
+ break;
+ }
+
+ break;
+ case SPAPR_VTPM_VALID_COMMAND: /* Payloads */
+ switch (local_crq.s.msg) {
+ case SPAPR_VTPM_TPM_COMMAND:
+ trace_tpm_spapr_do_crq_tpm_command();
+ if (s->state == SPAPR_VTPM_STATE_EXECUTION) {
+ return H_BUSY;
+ }
+ /* this crq is tracked */
+ memcpy(crq->raw, crq_data, sizeof(crq->raw));
+
+ rc = tpm_spapr_process_cmd(s, be32_to_cpu(crq->s.data));
+
+ if (rc == H_SUCCESS) {
+ crq->s.valid = be16_to_cpu(0);
+ } else {
+ local_crq.s.valid = SPAPR_VTPM_MSG_RESULT;
+ local_crq.s.msg = SPAPR_VTPM_VTPM_ERROR;
+ local_crq.s.data = cpu_to_be32(SPAPR_VTPM_ERR_COPY_IN_FAILED);
+ spapr_vio_send_crq(dev, local_crq.raw);
+ }
+ break;
+
+ case SPAPR_VTPM_GET_RTCE_BUFFER_SIZE:
+ trace_tpm_spapr_do_crq_tpm_get_rtce_buffer_size(s->be_buffer_size);
+ local_crq.s.msg |= SPAPR_VTPM_MSG_RESULT;
+ local_crq.s.len = cpu_to_be16(s->be_buffer_size);
+ spapr_vio_send_crq(dev, local_crq.raw);
+ break;
+
+ case SPAPR_VTPM_GET_VERSION:
+ local_crq.s.msg |= SPAPR_VTPM_MSG_RESULT;
+ local_crq.s.len = cpu_to_be16(0);
+ switch (s->be_tpm_version) {
+ case TPM_VERSION_UNSPEC:
+ local_crq.s.data = cpu_to_be32(0);
+ break;
+ case TPM_VERSION_1_2:
+ local_crq.s.data = cpu_to_be32(1);
+ break;
+ case TPM_VERSION_2_0:
+ local_crq.s.data = cpu_to_be32(2);
+ break;
+ }
+ trace_tpm_spapr_do_crq_get_version(be32_to_cpu(local_crq.s.data));
+ spapr_vio_send_crq(dev, local_crq.raw);
+ break;
+
+ case SPAPR_VTPM_PREPARE_TO_SUSPEND:
+ trace_tpm_spapr_do_crq_prepare_to_suspend();
+ local_crq.s.msg |= SPAPR_VTPM_MSG_RESULT;
+ spapr_vio_send_crq(dev, local_crq.raw);
+ break;
+
+ default:
+ trace_tpm_spapr_do_crq_unknown_msg_type(crq->s.msg);
+ }
+ break;
+ default:
+ trace_tpm_spapr_do_crq_unknown_crq(local_crq.raw[0], local_crq.raw[1]);
+ };
+
+ return H_SUCCESS;
+}
+
+static void tpm_spapr_request_completed(TPMIf *ti, int ret)
+{
+ SPAPRvTPMState *s = VIO_SPAPR_VTPM(ti);
+ TPMSpaprCRQ *crq = &s->crq;
+ uint32_t len;
+ int rc;
+
+ s->state = SPAPR_VTPM_STATE_COMPLETION;
+
+ /* a max. of be_buffer_size bytes can be transported */
+ len = MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size);
+ rc = spapr_vio_dma_write(&s->vdev, be32_to_cpu(crq->s.data),
+ s->buffer, len);
+
+ if (trace_event_get_state_backends(TRACE_TPM_SPAPR_SHOW_BUFFER)) {
+ tpm_spapr_show_buffer(s->buffer, len, "From TPM");
+ }
+
+ crq->s.valid = SPAPR_VTPM_MSG_RESULT;
+ if (rc == H_SUCCESS) {
+ crq->s.msg = SPAPR_VTPM_TPM_COMMAND | SPAPR_VTPM_MSG_RESULT;
+ crq->s.len = cpu_to_be16(len);
+ } else {
+ error_report("%s: DMA write failure", __func__);
+ crq->s.msg = SPAPR_VTPM_VTPM_ERROR;
+ crq->s.len = cpu_to_be16(0);
+ crq->s.data = cpu_to_be32(SPAPR_VTPM_ERR_COPY_OUT_FAILED);
+ }
+
+ rc = spapr_vio_send_crq(&s->vdev, crq->raw);
+ if (rc) {
+ error_report("%s: Error sending response", __func__);
+ }
+}
+
+static int tpm_spapr_do_startup_tpm(SPAPRvTPMState *s, size_t buffersize)
+{
+ return tpm_backend_startup_tpm(s->be_driver, buffersize);
+}
+
+static void tpm_spapr_update_deviceclass(SpaprVioDevice *dev)
+{
+ SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
+ SpaprVioDeviceClass *k = VIO_SPAPR_DEVICE_GET_CLASS(dev);
+
+ switch (s->be_tpm_version) {
+ case TPM_VERSION_UNSPEC:
+ assert(false);
+ break;
+ case TPM_VERSION_1_2:
+ k->dt_name = "vtpm";
+ k->dt_type = "IBM,vtpm";
+ k->dt_compatible = "IBM,vtpm";
+ break;
+ case TPM_VERSION_2_0:
+ k->dt_name = "vtpm";
+ k->dt_type = "IBM,vtpm";
+ k->dt_compatible = "IBM,vtpm20";
+ break;
+ }
+}
+
+static void tpm_spapr_reset(SpaprVioDevice *dev)
+{
+ SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
+
+ s->state = SPAPR_VTPM_STATE_NONE;
+
+ s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver);
+ tpm_spapr_update_deviceclass(dev);
+
+ s->be_buffer_size = MAX(ROUND_UP(tpm_backend_get_buffer_size(s->be_driver),
+ TARGET_PAGE_SIZE),
+ sizeof(s->buffer));
+
+ tpm_backend_reset(s->be_driver);
+ tpm_spapr_do_startup_tpm(s, s->be_buffer_size);
+}
+
+static enum TPMVersion tpm_spapr_get_version(TPMIf *ti)
+{
+ SPAPRvTPMState *s = VIO_SPAPR_VTPM(ti);
+
+ if (tpm_backend_had_startup_error(s->be_driver)) {
+ return TPM_VERSION_UNSPEC;
+ }
+
+ return tpm_backend_get_tpm_version(s->be_driver);
+}
+
+static const VMStateDescription vmstate_spapr_vtpm = {
+ .name = "tpm-spapr",
+ .unmigratable = 1,
+};
+
+static Property tpm_spapr_properties[] = {
+ DEFINE_SPAPR_PROPERTIES(SPAPRvTPMState, vdev),
+ DEFINE_PROP_TPMBE("tpmdev", SPAPRvTPMState, be_driver),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void tpm_spapr_realizefn(SpaprVioDevice *dev, Error **errp)
+{
+ SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
+
+ if (!tpm_find()) {
+ error_setg(errp, "at most one TPM device is permitted");
+ return;
+ }
+
+ dev->crq.SendFunc = tpm_spapr_do_crq;
+
+ if (!s->be_driver) {
+ error_setg(errp, "'tpmdev' property is required");
+ return;
+ }
+}
+
+static void tpm_spapr_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ SpaprVioDeviceClass *k = VIO_SPAPR_DEVICE_CLASS(klass);
+ TPMIfClass *tc = TPM_IF_CLASS(klass);
+
+ k->realize = tpm_spapr_realizefn;
+ k->reset = tpm_spapr_reset;
+ k->signal_mask = 0x00000001;
+ set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+ dc->props = tpm_spapr_properties;
+ k->rtce_window_size = 0x10000000;
+ dc->vmsd = &vmstate_spapr_vtpm;
+
+ tc->model = TPM_MODEL_TPM_SPAPR;
+ tc->get_version = tpm_spapr_get_version;
+ tc->request_completed = tpm_spapr_request_completed;
+}
+
+static const TypeInfo tpm_spapr_info = {
+ .name = TYPE_TPM_SPAPR,
+ .parent = TYPE_VIO_SPAPR_DEVICE,
+ .instance_size = sizeof(SPAPRvTPMState),
+ .class_init = tpm_spapr_class_init,
+ .interfaces = (InterfaceInfo[]) {
+ { TYPE_TPM_IF },
+ { }
+ }
+};
+
+static void tpm_spapr_register_types(void)
+{
+ type_register_static(&tpm_spapr_info);
+}
+
+type_init(tpm_spapr_register_types)
diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
index 89804bcd64..6278a39618 100644
--- a/hw/tpm/trace-events
+++ b/hw/tpm/trace-events
@@ -55,3 +55,15 @@ tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
# tpm_ppi.c
tpm_ppi_memset(uint8_t *ptr, size_t size) "memset: %p %zu"
+
+# hw/tpm/tpm_spapr.c
+tpm_spapr_show_buffer(const char *direction, size_t len, const char *buf) "direction: %s len: %zu\n%s"
+tpm_spapr_do_crq(uint8_t raw1, uint8_t raw2) "1st 2 bytes in CRQ: 0x%02x 0x%02x"
+tpm_spapr_do_crq_crq_result(void) "SPAPR_VTPM_INIT_CRQ_RESULT"
+tpm_spapr_do_crq_crq_complete_result(void) "SPAPR_VTPM_INIT_CRQ_COMP_RESULT"
+tpm_spapr_do_crq_tpm_command(void) "got TPM command payload"
+tpm_spapr_do_crq_tpm_get_rtce_buffer_size(size_t buffersize) "response: buffer size is %zu"
+tpm_spapr_do_crq_get_version(uint32_t version) "response: version %u"
+tpm_spapr_do_crq_prepare_to_suspend(void) "response: preparing to suspend"
+tpm_spapr_do_crq_unknown_msg_type(uint8_t type) "Unknown message type 0x%02x"
+tpm_spapr_do_crq_unknown_crq(uint8_t raw1, uint8_t raw2) "unknown CRQ 0x%02x 0x%02x ..."
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index 5b541a71c8..15979a3647 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -45,11 +45,14 @@ typedef struct TPMIfClass {
#define TYPE_TPM_TIS "tpm-tis"
#define TYPE_TPM_CRB "tpm-crb"
+#define TYPE_TPM_SPAPR "tpm-spapr"
#define TPM_IS_TIS(chr) \
object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS)
#define TPM_IS_CRB(chr) \
object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
+#define TPM_IS_SPAPR(chr) \
+ object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)
/* returns NULL unless there is exactly one TPM device */
static inline TPMIf *tpm_find(void)
diff --git a/qapi/tpm.json b/qapi/tpm.json
index b30323bb6b..63878aa0f4 100644
--- a/qapi/tpm.json
+++ b/qapi/tpm.json
@@ -12,11 +12,11 @@
#
# @tpm-tis: TPM TIS model
# @tpm-crb: TPM CRB model (since 2.12)
+# @tpm-spapr: TPM SPAPR model (since 5.0)
#
# Since: 1.5
##
-{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb' ] }
-
+{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb', 'tpm-spapr' ] }
##
# @query-tpm-models:
#
@@ -29,7 +29,7 @@
# Example:
#
# -> { "execute": "query-tpm-models" }
-# <- { "return": [ "tpm-tis", "tpm-crb" ] }
+# <- { "return": [ "tpm-tis", "tpm-crb", "tpm-spapr" ] }
#
##
{ 'command': 'query-tpm-models', 'returns': ['TpmModel'] }
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v5 2/5] tpm: Return bool from tpm_backend_finish_sync
2019-12-12 20:24 [PATCH v5 0/5] Add vTPM emulator support for ppc64 platform Stefan Berger
2019-12-12 20:24 ` [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface Stefan Berger
@ 2019-12-12 20:24 ` Stefan Berger
2019-12-12 20:24 ` [PATCH v5 3/5] tpm_spapr: Support suspend and resume Stefan Berger
` (2 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Stefan Berger @ 2019-12-12 20:24 UTC (permalink / raw)
To: qemu-ppc; +Cc: marcandre.lureau, Stefan Berger, qemu-devel, david
Return true in case we had to wait for an outstanding response
to come back, false otherwise.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
diff --git a/backends/tpm.c b/backends/tpm.c
index 375587e743..1f75883d8a 100644
--- a/backends/tpm.c
+++ b/backends/tpm.c
@@ -49,11 +49,15 @@ static int tpm_backend_worker_thread(gpointer data)
return 0;
}
-void tpm_backend_finish_sync(TPMBackend *s)
+bool tpm_backend_finish_sync(TPMBackend *s)
{
+ bool ret = s->cmd != NULL;
+
while (s->cmd) {
aio_poll(qemu_get_aio_context(), true);
}
+
+ return ret;
}
enum TpmType tpm_backend_get_type(TPMBackend *s)
diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
index 9e7451fb52..c35fe85c62 100644
--- a/include/sysemu/tpm_backend.h
+++ b/include/sysemu/tpm_backend.h
@@ -200,8 +200,10 @@ size_t tpm_backend_get_buffer_size(TPMBackend *s);
*
* Finish the pending command synchronously (this will call aio_poll()
* on qemu main AIOContext until it ends)
+ *
+ * Returns true in case there was a pending command, false otherwise.
*/
-void tpm_backend_finish_sync(TPMBackend *s);
+bool tpm_backend_finish_sync(TPMBackend *s);
/**
* tpm_backend_query_tpm:
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v5 3/5] tpm_spapr: Support suspend and resume
2019-12-12 20:24 [PATCH v5 0/5] Add vTPM emulator support for ppc64 platform Stefan Berger
2019-12-12 20:24 ` [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface Stefan Berger
2019-12-12 20:24 ` [PATCH v5 2/5] tpm: Return bool from tpm_backend_finish_sync Stefan Berger
@ 2019-12-12 20:24 ` Stefan Berger
2019-12-13 5:39 ` David Gibson
2019-12-12 20:24 ` [PATCH v5 4/5] hw/ppc/Kconfig: Enable TPM_SPAPR as part of PSERIES config Stefan Berger
2019-12-12 20:24 ` [PATCH v5 5/5] docs: tpm: Add example command line for ppc64 and tpm-spapr Stefan Berger
4 siblings, 1 reply; 19+ messages in thread
From: Stefan Berger @ 2019-12-12 20:24 UTC (permalink / raw)
To: qemu-ppc; +Cc: marcandre.lureau, Stefan Berger, qemu-devel, david
Extend the tpm_spapr frontend with VM suspend and resume support.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
index c4a67e2403..8f5a142bd4 100644
--- a/hw/tpm/tpm_spapr.c
+++ b/hw/tpm/tpm_spapr.c
@@ -87,6 +87,8 @@ typedef struct {
TPMVersion be_tpm_version;
size_t be_buffer_size;
+
+ bool deliver_response; /* whether to deliver response after VM resume */
} SPAPRvTPMState;
static void tpm_spapr_show_buffer(const unsigned char *buffer,
@@ -256,6 +258,12 @@ static void tpm_spapr_request_completed(TPMIf *ti, int ret)
uint32_t len;
int rc;
+ if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
+ /* defer delivery of response until .post_load */
+ s->deliver_response |= true;
+ return;
+ }
+
s->state = SPAPR_VTPM_STATE_COMPLETION;
/* a max. of be_buffer_size bytes can be transported */
@@ -316,6 +324,7 @@ static void tpm_spapr_reset(SpaprVioDevice *dev)
SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
s->state = SPAPR_VTPM_STATE_NONE;
+ s->deliver_response = false;
s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver);
tpm_spapr_update_deviceclass(dev);
@@ -339,9 +348,53 @@ static enum TPMVersion tpm_spapr_get_version(TPMIf *ti)
return tpm_backend_get_tpm_version(s->be_driver);
}
+/* persistent state handling */
+
+static int tpm_spapr_pre_save(void *opaque)
+{
+ SPAPRvTPMState *s = opaque;
+
+ s->deliver_response |= tpm_backend_finish_sync(s->be_driver);
+
+ trace_tpm_spapr_pre_save(s->deliver_response);
+ /*
+ * we cannot deliver the results to the VM since DMA would touch VM memory
+ */
+
+ return 0;
+}
+
+static int tpm_spapr_post_load(void *opaque, int version_id)
+{
+ SPAPRvTPMState *s = opaque;
+
+ if (s->deliver_response) {
+ trace_tpm_spapr_post_load();
+ /* deliver the results to the VM via DMA */
+ tpm_spapr_request_completed(TPM_IF(s), 0);
+ s->deliver_response = false;
+ }
+
+ return 0;
+}
+
static const VMStateDescription vmstate_spapr_vtpm = {
.name = "tpm-spapr",
- .unmigratable = 1,
+ .version_id = 1,
+ .minimum_version_id = 0,
+ .minimum_version_id_old = 0,
+ .pre_save = tpm_spapr_pre_save,
+ .post_load = tpm_spapr_post_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_SPAPR_VIO(vdev, SPAPRvTPMState),
+
+ VMSTATE_UINT8(state, SPAPRvTPMState),
+ VMSTATE_BUFFER(buffer, SPAPRvTPMState),
+ /* remember DMA address */
+ VMSTATE_UINT32(crq.s.data, SPAPRvTPMState),
+ VMSTATE_BOOL(deliver_response, SPAPRvTPMState),
+ VMSTATE_END_OF_LIST(),
+ }
};
static Property tpm_spapr_properties[] = {
diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
index 6278a39618..d109661b96 100644
--- a/hw/tpm/trace-events
+++ b/hw/tpm/trace-events
@@ -67,3 +67,5 @@ tpm_spapr_do_crq_get_version(uint32_t version) "response: version %u"
tpm_spapr_do_crq_prepare_to_suspend(void) "response: preparing to suspend"
tpm_spapr_do_crq_unknown_msg_type(uint8_t type) "Unknown message type 0x%02x"
tpm_spapr_do_crq_unknown_crq(uint8_t raw1, uint8_t raw2) "unknown CRQ 0x%02x 0x%02x ..."
+tpm_spapr_pre_save(bool v) "TPM response to deliver after resume: %d"
+tpm_spapr_post_load(void) "Delivering TPM response after resume"
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v5 4/5] hw/ppc/Kconfig: Enable TPM_SPAPR as part of PSERIES config
2019-12-12 20:24 [PATCH v5 0/5] Add vTPM emulator support for ppc64 platform Stefan Berger
` (2 preceding siblings ...)
2019-12-12 20:24 ` [PATCH v5 3/5] tpm_spapr: Support suspend and resume Stefan Berger
@ 2019-12-12 20:24 ` Stefan Berger
2019-12-12 20:24 ` [PATCH v5 5/5] docs: tpm: Add example command line for ppc64 and tpm-spapr Stefan Berger
4 siblings, 0 replies; 19+ messages in thread
From: Stefan Berger @ 2019-12-12 20:24 UTC (permalink / raw)
To: qemu-ppc
Cc: marcandre.lureau, Stefan Berger, Stefan Berger, qemu-devel, david
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index f927ec9c74..b5b3519158 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -10,6 +10,7 @@ config PSERIES
select XICS_SPAPR
select XIVE_SPAPR
select MSI_NONBROKEN
+ select TPM_SPAPR
config SPAPR_RNG
bool
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v5 5/5] docs: tpm: Add example command line for ppc64 and tpm-spapr
2019-12-12 20:24 [PATCH v5 0/5] Add vTPM emulator support for ppc64 platform Stefan Berger
` (3 preceding siblings ...)
2019-12-12 20:24 ` [PATCH v5 4/5] hw/ppc/Kconfig: Enable TPM_SPAPR as part of PSERIES config Stefan Berger
@ 2019-12-12 20:24 ` Stefan Berger
4 siblings, 0 replies; 19+ messages in thread
From: Stefan Berger @ 2019-12-12 20:24 UTC (permalink / raw)
To: qemu-ppc
Cc: marcandre.lureau, Stefan Berger, Stefan Berger, qemu-devel, david
Add an example to the TPM docs for how to add a TPM SPAPR
device model to a QEMU VM emulating a pSeries machine.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
index 9c8cca042d..9c3e67d8a7 100644
--- a/docs/specs/tpm.txt
+++ b/docs/specs/tpm.txt
@@ -34,6 +34,12 @@ The CRB interface makes a memory mapped IO region in the area 0xfed40000 -
QEMU files related to TPM CRB interface:
- hw/tpm/tpm_crb.c
+
+pSeries (ppc64) machines offer a tpm-spapr device model.
+
+QEMU files related to the SPAPR interface:
+ - hw/tpm/tpm_spapr.c
+
= fw_cfg interface =
The bios/firmware may read the "etc/tpm/config" fw_cfg entry for
@@ -281,7 +287,7 @@ swtpm socket --tpmstate dir=/tmp/mytpm1 \
--log level=20
Command line to start QEMU with the TPM emulator device communicating with
-the swtpm:
+the swtpm (x86):
qemu-system-x86_64 -display sdl -accel kvm \
-m 1024 -boot d -bios bios-256k.bin -boot menu=on \
@@ -289,6 +295,18 @@ qemu-system-x86_64 -display sdl -accel kvm \
-tpmdev emulator,id=tpm0,chardev=chrtpm \
-device tpm-tis,tpmdev=tpm0 test.img
+In case a pSeries machine is emulated, use the following command line:
+
+qemu-system-ppc64 -display sdl -machine pseries,accel=kvm \
+ -m 1024 -bios slof.bin -boot menu=on \
+ -nodefaults -device VGA -device pci-ohci -device usb-kbd \
+ -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
+ -tpmdev emulator,id=tpm0,chardev=chrtpm \
+ -device tpm-spapr,tpmdev=tpm0 \
+ -device spapr-vscsi,id=scsi0,reg=0x00002000 \
+ -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,id=virtio-disk0 \
+ -drive file=test.img,format=raw,if=none,id=drive-virtio-disk0
+
In case SeaBIOS is used as firmware, it should show the TPM menu item
after entering the menu with 'ESC'.
--
2.21.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
2019-12-12 20:24 ` [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface Stefan Berger
@ 2019-12-12 20:33 ` Eric Blake
2019-12-12 20:34 ` Stefan Berger
2019-12-13 5:34 ` David Gibson
1 sibling, 1 reply; 19+ messages in thread
From: Eric Blake @ 2019-12-12 20:33 UTC (permalink / raw)
To: Stefan Berger, qemu-ppc; +Cc: marcandre.lureau, qemu-devel, david
On 12/12/19 2:24 PM, Stefan Berger wrote:
> Implement support for TPM on ppc64 by implementing the vTPM CRQ interface
> as a frontend. It can use the tpm_emulator driver backend with the external
> swtpm.
>
> The Linux vTPM driver for ppc64 works with this emulation.
>
> This TPM emulator also handles the TPM 2 case.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>
> diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
Odd that your diff doesn't include the usual --- marker or a diffstat.
> +++ b/hw/tpm/tpm_spapr.c
> @@ -0,0 +1,405 @@
> +/*
> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
> + *
> + * PAPR Virtual TPM
> + *
> + * Copyright (c) 2015, 2017 IBM Corporation.
Do you want to claim 2019?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
2019-12-12 20:33 ` Eric Blake
@ 2019-12-12 20:34 ` Stefan Berger
0 siblings, 0 replies; 19+ messages in thread
From: Stefan Berger @ 2019-12-12 20:34 UTC (permalink / raw)
To: Eric Blake, Stefan Berger, qemu-ppc; +Cc: marcandre.lureau, qemu-devel, david
On 12/12/19 3:33 PM, Eric Blake wrote:
> On 12/12/19 2:24 PM, Stefan Berger wrote:
>> Implement support for TPM on ppc64 by implementing the vTPM CRQ
>> interface
>> as a frontend. It can use the tpm_emulator driver backend with the
>> external
>> swtpm.
>>
>> The Linux vTPM driver for ppc64 works with this emulation.
>>
>> This TPM emulator also handles the TPM 2 case.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>>
>> diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
>
> Odd that your diff doesn't include the usual --- marker or a diffstat.
>
>
>> +++ b/hw/tpm/tpm_spapr.c
>> @@ -0,0 +1,405 @@
>> +/*
>> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware
>> System Emulator
>> + *
>> + * PAPR Virtual TPM
>> + *
>> + * Copyright (c) 2015, 2017 IBM Corporation.
>
> Do you want to claim 2019?
>
:-)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
2019-12-12 20:24 ` [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface Stefan Berger
2019-12-12 20:33 ` Eric Blake
@ 2019-12-13 5:34 ` David Gibson
2019-12-13 13:03 ` Stefan Berger
1 sibling, 1 reply; 19+ messages in thread
From: David Gibson @ 2019-12-13 5:34 UTC (permalink / raw)
To: Stefan Berger; +Cc: marcandre.lureau, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 19477 bytes --]
On Thu, Dec 12, 2019 at 03:24:26PM -0500, Stefan Berger wrote:
> Implement support for TPM on ppc64 by implementing the vTPM CRQ interface
> as a frontend. It can use the tpm_emulator driver backend with the external
> swtpm.
>
> The Linux vTPM driver for ppc64 works with this emulation.
>
> This TPM emulator also handles the TPM 2 case.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>
> diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
> index 4c8ee87d67..66a570aac1 100644
> --- a/hw/tpm/Kconfig
> +++ b/hw/tpm/Kconfig
> @@ -22,3 +22,9 @@ config TPM_EMULATOR
> bool
> default y
> depends on TPMDEV
> +
> +config TPM_SPAPR
> + bool
> + default n
> + select TPMDEV
> + depends on PSERIES
> diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
> index de0b85d02a..85eb99ae05 100644
> --- a/hw/tpm/Makefile.objs
> +++ b/hw/tpm/Makefile.objs
> @@ -4,3 +4,4 @@ common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
> common-obj-$(CONFIG_TPM_CRB) += tpm_crb.o
> common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
> common-obj-$(CONFIG_TPM_EMULATOR) += tpm_emulator.o
> +obj-$(CONFIG_TPM_SPAPR) += tpm_spapr.o
> diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
> new file mode 100644
> index 0000000000..c4a67e2403
> --- /dev/null
> +++ b/hw/tpm/tpm_spapr.c
> @@ -0,0 +1,405 @@
> +/*
> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
> + *
> + * PAPR Virtual TPM
> + *
> + * Copyright (c) 2015, 2017 IBM Corporation.
> + *
> + * Authors:
> + * Stefan Berger <stefanb@linux.vnet.ibm.com>
> + *
> + * This code is licensed under the GPL version 2 or later. See the
> + * COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> +#include "qapi/error.h"
> +#include "hw/qdev-properties.h"
> +#include "migration/vmstate.h"
> +
> +#include "sysemu/tpm_backend.h"
> +#include "tpm_int.h"
> +#include "tpm_util.h"
> +
> +#include "hw/ppc/spapr.h"
> +#include "hw/ppc/spapr_vio.h"
> +#include "trace.h"
> +
> +#define DEBUG_SPAPR 0
> +
> +#define VIO_SPAPR_VTPM(obj) \
> + OBJECT_CHECK(SPAPRvTPMState, (obj), TYPE_TPM_SPAPR)
> +
> +typedef struct VioCRQ {
How does this structure relate to the existing SpaprVioCrq?
Also we're now avoiding exceptions to StudlyCaps, because it causes
more confusion even if it is to match other capitalization
conventions. So, I'd suggest 'VioCrq', 'TpmSpaprCrq' etc.
> + uint8_t valid; /* 0x80: cmd; 0xc0: init crq */
> + /* 0x81-0x83: CRQ message response */
> + uint8_t msg; /* see below */
> + uint16_t len; /* len of TPM request; len of TPM response */
> + uint32_t data; /* rtce_dma_handle when sending TPM request */
> + uint64_t reserved;
> +} VioCRQ;
> +
> +typedef union TPMSpaprCRQ {
> + VioCRQ s;
> + uint8_t raw[sizeof(VioCRQ)];
> +} TPMSpaprCRQ;
A union just to get raw bytes seems a really weird thing to do (as
opposed to just casting to (char *))
> +
> +#define SPAPR_VTPM_VALID_INIT_CRQ_COMMAND 0xC0
> +#define SPAPR_VTPM_VALID_COMMAND 0x80
> +#define SPAPR_VTPM_MSG_RESULT 0x80
> +
> +/* msg types for valid = SPAPR_VTPM_VALID_INIT_CRQ */
> +#define SPAPR_VTPM_INIT_CRQ_RESULT 0x1
> +#define SPAPR_VTPM_INIT_CRQ_COMPLETE_RESULT 0x2
> +
> +/* msg types for valid = SPAPR_VTPM_VALID_CMD */
> +#define SPAPR_VTPM_GET_VERSION 0x1
> +#define SPAPR_VTPM_TPM_COMMAND 0x2
> +#define SPAPR_VTPM_GET_RTCE_BUFFER_SIZE 0x3
> +#define SPAPR_VTPM_PREPARE_TO_SUSPEND 0x4
> +
> +/* response error messages */
> +#define SPAPR_VTPM_VTPM_ERROR 0xff
> +
> +/* error codes */
> +#define SPAPR_VTPM_ERR_COPY_IN_FAILED 0x3
> +#define SPAPR_VTPM_ERR_COPY_OUT_FAILED 0x4
> +
> +#define MAX_BUFFER_SIZE TARGET_PAGE_SIZE
> +
> +typedef struct {
> + SpaprVioDevice vdev;
> +
> + TPMSpaprCRQ crq; /* track single TPM command */
> +
> + uint8_t state;
> +#define SPAPR_VTPM_STATE_NONE 0
> +#define SPAPR_VTPM_STATE_EXECUTION 1
> +#define SPAPR_VTPM_STATE_COMPLETION 2
I see this field written, but never read. What's up with that?
> +
> + unsigned char buffer[MAX_BUFFER_SIZE];
> +
> + TPMBackendCmd cmd;
> +
> + TPMBackend *be_driver;
> + TPMVersion be_tpm_version;
> +
> + size_t be_buffer_size;
> +} SPAPRvTPMState;
SpaprVtpmState
Or just SpaprTpmState, since we use just "tpm spapr" rather than
"vtpm" in plenty of other places.
> +
> +static void tpm_spapr_show_buffer(const unsigned char *buffer,
> + size_t buffer_size, const char *string)
> +{
> + size_t len, i;
> + char *line_buffer, *p;
> +
> + len = MIN(tpm_cmd_get_size(buffer), buffer_size);
> +
> + /*
> + * allocate enough room for 3 chars per buffer entry plus a
> + * newline after every 16 chars and a final null terminator.
> + */
> + line_buffer = g_malloc(len * 3 + (len / 16) + 1);
You can use g_strdup_printf() / g_string_append_printf() to avoid
fiddly messing around with allocations like this.
> +
> + for (i = 0, p = line_buffer; i < len; i++) {
> + if (i && !(i % 16)) {
> + p += sprintf(p, "\n");
> + }
> + p += sprintf(p, "%.2X ", buffer[i]);
> + }
> + trace_tpm_spapr_show_buffer(string, len, line_buffer);
> +
> + g_free(line_buffer);
> +}
> +
> +/*
> + * Send a request to the TPM.
> + */
> +static void tpm_spapr_tpm_send(SPAPRvTPMState *s)
> +{
> + if (trace_event_get_state_backends(TRACE_TPM_SPAPR_SHOW_BUFFER)) {
> + tpm_spapr_show_buffer(s->buffer, sizeof(s->buffer), "To TPM");
> + }
> +
> + s->state = SPAPR_VTPM_STATE_EXECUTION;
> + s->cmd = (TPMBackendCmd) {
> + .locty = 0,
> + .in = s->buffer,
> + .in_len = MIN(tpm_cmd_get_size(s->buffer), sizeof(s->buffer)),
> + .out = s->buffer,
> + .out_len = sizeof(s->buffer),
> + };
> +
> + tpm_backend_deliver_request(s->be_driver, &s->cmd);
> +}
> +
> +static int tpm_spapr_process_cmd(SPAPRvTPMState *s, uint64_t dataptr)
> +{
> + long rc;
> +
> + /* a max. of be_buffer_size bytes can be transported */
> + rc = spapr_vio_dma_read(&s->vdev, dataptr,
> + s->buffer, s->be_buffer_size);
> + if (rc) {
> + error_report("tpm_spapr_got_payload: DMA read failure");
> + }
> + /* let vTPM handle any malformed request */
> + tpm_spapr_tpm_send(s);
> +
> + return rc;
> +}
> +
> +static int tpm_spapr_do_crq(struct SpaprVioDevice *dev, uint8_t *crq_data)
> +{
> + SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
> + TPMSpaprCRQ local_crq;
> + TPMSpaprCRQ *crq = &s->crq; /* requests only */
> + int rc;
> +
> + memcpy(&local_crq.raw, crq_data, sizeof(local_crq.raw));
Again, no real need for a union here, you can just memcpy onto a
structure variable.
> + trace_tpm_spapr_do_crq(local_crq.raw[0], local_crq.raw[1]);
> +
> + switch (local_crq.s.valid) {
> + case SPAPR_VTPM_VALID_INIT_CRQ_COMMAND: /* Init command/response */
> +
> + /* Respond to initialization request */
> + switch (local_crq.s.msg) {
> + case SPAPR_VTPM_INIT_CRQ_RESULT:
> + trace_tpm_spapr_do_crq_crq_result();
> + memset(local_crq.raw, 0, sizeof(local_crq.raw));
> + local_crq.s.valid = SPAPR_VTPM_VALID_INIT_CRQ_COMMAND;
> + local_crq.s.msg = SPAPR_VTPM_INIT_CRQ_RESULT;
> + spapr_vio_send_crq(dev, local_crq.raw);
> + break;
> +
> + case SPAPR_VTPM_INIT_CRQ_COMPLETE_RESULT:
> + trace_tpm_spapr_do_crq_crq_complete_result();
> + memset(local_crq.raw, 0, sizeof(local_crq.raw));
> + local_crq.s.valid = SPAPR_VTPM_VALID_INIT_CRQ_COMMAND;
> + local_crq.s.msg = SPAPR_VTPM_INIT_CRQ_COMPLETE_RESULT;
> + spapr_vio_send_crq(dev, local_crq.raw);
> + break;
> + }
> +
> + break;
> + case SPAPR_VTPM_VALID_COMMAND: /* Payloads */
> + switch (local_crq.s.msg) {
> + case SPAPR_VTPM_TPM_COMMAND:
> + trace_tpm_spapr_do_crq_tpm_command();
> + if (s->state == SPAPR_VTPM_STATE_EXECUTION) {
> + return H_BUSY;
> + }
> + /* this crq is tracked */
> + memcpy(crq->raw, crq_data, sizeof(crq->raw));
> +
> + rc = tpm_spapr_process_cmd(s, be32_to_cpu(crq->s.data));
> +
> + if (rc == H_SUCCESS) {
> + crq->s.valid = be16_to_cpu(0);
> + } else {
> + local_crq.s.valid = SPAPR_VTPM_MSG_RESULT;
> + local_crq.s.msg = SPAPR_VTPM_VTPM_ERROR;
> + local_crq.s.data = cpu_to_be32(SPAPR_VTPM_ERR_COPY_IN_FAILED);
> + spapr_vio_send_crq(dev, local_crq.raw);
> + }
> + break;
> +
> + case SPAPR_VTPM_GET_RTCE_BUFFER_SIZE:
> + trace_tpm_spapr_do_crq_tpm_get_rtce_buffer_size(s->be_buffer_size);
> + local_crq.s.msg |= SPAPR_VTPM_MSG_RESULT;
> + local_crq.s.len = cpu_to_be16(s->be_buffer_size);
> + spapr_vio_send_crq(dev, local_crq.raw);
> + break;
> +
> + case SPAPR_VTPM_GET_VERSION:
> + local_crq.s.msg |= SPAPR_VTPM_MSG_RESULT;
> + local_crq.s.len = cpu_to_be16(0);
> + switch (s->be_tpm_version) {
> + case TPM_VERSION_UNSPEC:
> + local_crq.s.data = cpu_to_be32(0);
> + break;
> + case TPM_VERSION_1_2:
> + local_crq.s.data = cpu_to_be32(1);
> + break;
> + case TPM_VERSION_2_0:
> + local_crq.s.data = cpu_to_be32(2);
> + break;
To make the breakage obvious when/if the backend adds a new version we
should probably have a default: case with g_assert_not_reached() or
the like.
> + }
> + trace_tpm_spapr_do_crq_get_version(be32_to_cpu(local_crq.s.data));
> + spapr_vio_send_crq(dev, local_crq.raw);
> + break;
> +
> + case SPAPR_VTPM_PREPARE_TO_SUSPEND:
> + trace_tpm_spapr_do_crq_prepare_to_suspend();
> + local_crq.s.msg |= SPAPR_VTPM_MSG_RESULT;
> + spapr_vio_send_crq(dev, local_crq.raw);
> + break;
> +
> + default:
> + trace_tpm_spapr_do_crq_unknown_msg_type(crq->s.msg);
> + }
> + break;
> + default:
> + trace_tpm_spapr_do_crq_unknown_crq(local_crq.raw[0], local_crq.raw[1]);
> + };
> +
> + return H_SUCCESS;
> +}
> +
> +static void tpm_spapr_request_completed(TPMIf *ti, int ret)
> +{
> + SPAPRvTPMState *s = VIO_SPAPR_VTPM(ti);
> + TPMSpaprCRQ *crq = &s->crq;
> + uint32_t len;
> + int rc;
> +
> + s->state = SPAPR_VTPM_STATE_COMPLETION;
> +
> + /* a max. of be_buffer_size bytes can be transported */
> + len = MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size);
> + rc = spapr_vio_dma_write(&s->vdev, be32_to_cpu(crq->s.data),
> + s->buffer, len);
> +
> + if (trace_event_get_state_backends(TRACE_TPM_SPAPR_SHOW_BUFFER)) {
> + tpm_spapr_show_buffer(s->buffer, len, "From TPM");
> + }
> +
> + crq->s.valid = SPAPR_VTPM_MSG_RESULT;
> + if (rc == H_SUCCESS) {
> + crq->s.msg = SPAPR_VTPM_TPM_COMMAND | SPAPR_VTPM_MSG_RESULT;
> + crq->s.len = cpu_to_be16(len);
> + } else {
> + error_report("%s: DMA write failure", __func__);
> + crq->s.msg = SPAPR_VTPM_VTPM_ERROR;
> + crq->s.len = cpu_to_be16(0);
> + crq->s.data = cpu_to_be32(SPAPR_VTPM_ERR_COPY_OUT_FAILED);
> + }
> +
> + rc = spapr_vio_send_crq(&s->vdev, crq->raw);
> + if (rc) {
> + error_report("%s: Error sending response", __func__);
> + }
> +}
> +
> +static int tpm_spapr_do_startup_tpm(SPAPRvTPMState *s, size_t buffersize)
> +{
> + return tpm_backend_startup_tpm(s->be_driver, buffersize);
> +}
> +
> +static void tpm_spapr_update_deviceclass(SpaprVioDevice *dev)
> +{
> + SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
> + SpaprVioDeviceClass *k = VIO_SPAPR_DEVICE_GET_CLASS(dev);
> +
> + switch (s->be_tpm_version) {
> + case TPM_VERSION_UNSPEC:
> + assert(false);
> + break;
> + case TPM_VERSION_1_2:
> + k->dt_name = "vtpm";
> + k->dt_type = "IBM,vtpm";
> + k->dt_compatible = "IBM,vtpm";
> + break;
> + case TPM_VERSION_2_0:
> + k->dt_name = "vtpm";
> + k->dt_type = "IBM,vtpm";
> + k->dt_compatible = "IBM,vtpm20";
> + break;
Erk. Updating DeviceClass structures on the fly is hideously ugly.
We might need to take a different approach for this.
> + }
> +}
> +
> +static void tpm_spapr_reset(SpaprVioDevice *dev)
> +{
> + SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
> +
> + s->state = SPAPR_VTPM_STATE_NONE;
> +
> + s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver);
> + tpm_spapr_update_deviceclass(dev);
> +
> + s->be_buffer_size = MAX(ROUND_UP(tpm_backend_get_buffer_size(s->be_driver),
> + TARGET_PAGE_SIZE),
> + sizeof(s->buffer));
I'm very confused as to what be_buffer_size is supposed to represent.
it's not the backend size, because you're rounding that up and taking
MAX with another thing. But it's not the max safe size for this
locally, because it can be bigger than s->buffer.
> + tpm_backend_reset(s->be_driver);
> + tpm_spapr_do_startup_tpm(s, s->be_buffer_size);
> +}
> +
> +static enum TPMVersion tpm_spapr_get_version(TPMIf *ti)
> +{
> + SPAPRvTPMState *s = VIO_SPAPR_VTPM(ti);
> +
> + if (tpm_backend_had_startup_error(s->be_driver)) {
> + return TPM_VERSION_UNSPEC;
> + }
> +
> + return tpm_backend_get_tpm_version(s->be_driver);
> +}
> +
> +static const VMStateDescription vmstate_spapr_vtpm = {
> + .name = "tpm-spapr",
> + .unmigratable = 1,
> +};
> +
> +static Property tpm_spapr_properties[] = {
> + DEFINE_SPAPR_PROPERTIES(SPAPRvTPMState, vdev),
> + DEFINE_PROP_TPMBE("tpmdev", SPAPRvTPMState, be_driver),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void tpm_spapr_realizefn(SpaprVioDevice *dev, Error **errp)
> +{
> + SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
> +
> + if (!tpm_find()) {
This seems wrong, even though I also see it in existing TPM code.
AFAICT tpm_find() returns a TPMIf pointer for the existing TPM if it
exists, meaning !tpm_find() will be true if there is *not* an existing
TPM.
> + error_setg(errp, "at most one TPM device is permitted");
> + return;
> + }
> +
> + dev->crq.SendFunc = tpm_spapr_do_crq;
> +
> + if (!s->be_driver) {
> + error_setg(errp, "'tpmdev' property is required");
> + return;
> + }
> +}
> +
> +static void tpm_spapr_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + SpaprVioDeviceClass *k = VIO_SPAPR_DEVICE_CLASS(klass);
> + TPMIfClass *tc = TPM_IF_CLASS(klass);
> +
> + k->realize = tpm_spapr_realizefn;
> + k->reset = tpm_spapr_reset;
> + k->signal_mask = 0x00000001;
> + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> + dc->props = tpm_spapr_properties;
> + k->rtce_window_size = 0x10000000;
> + dc->vmsd = &vmstate_spapr_vtpm;
> +
> + tc->model = TPM_MODEL_TPM_SPAPR;
> + tc->get_version = tpm_spapr_get_version;
> + tc->request_completed = tpm_spapr_request_completed;
> +}
> +
> +static const TypeInfo tpm_spapr_info = {
> + .name = TYPE_TPM_SPAPR,
> + .parent = TYPE_VIO_SPAPR_DEVICE,
> + .instance_size = sizeof(SPAPRvTPMState),
> + .class_init = tpm_spapr_class_init,
> + .interfaces = (InterfaceInfo[]) {
> + { TYPE_TPM_IF },
> + { }
> + }
> +};
> +
> +static void tpm_spapr_register_types(void)
> +{
> + type_register_static(&tpm_spapr_info);
> +}
> +
> +type_init(tpm_spapr_register_types)
> diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> index 89804bcd64..6278a39618 100644
> --- a/hw/tpm/trace-events
> +++ b/hw/tpm/trace-events
> @@ -55,3 +55,15 @@ tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
>
> # tpm_ppi.c
> tpm_ppi_memset(uint8_t *ptr, size_t size) "memset: %p %zu"
> +
> +# hw/tpm/tpm_spapr.c
> +tpm_spapr_show_buffer(const char *direction, size_t len, const char *buf) "direction: %s len: %zu\n%s"
> +tpm_spapr_do_crq(uint8_t raw1, uint8_t raw2) "1st 2 bytes in CRQ: 0x%02x 0x%02x"
> +tpm_spapr_do_crq_crq_result(void) "SPAPR_VTPM_INIT_CRQ_RESULT"
> +tpm_spapr_do_crq_crq_complete_result(void) "SPAPR_VTPM_INIT_CRQ_COMP_RESULT"
> +tpm_spapr_do_crq_tpm_command(void) "got TPM command payload"
> +tpm_spapr_do_crq_tpm_get_rtce_buffer_size(size_t buffersize) "response: buffer size is %zu"
> +tpm_spapr_do_crq_get_version(uint32_t version) "response: version %u"
> +tpm_spapr_do_crq_prepare_to_suspend(void) "response: preparing to suspend"
> +tpm_spapr_do_crq_unknown_msg_type(uint8_t type) "Unknown message type 0x%02x"
> +tpm_spapr_do_crq_unknown_crq(uint8_t raw1, uint8_t raw2) "unknown CRQ 0x%02x 0x%02x ..."
> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> index 5b541a71c8..15979a3647 100644
> --- a/include/sysemu/tpm.h
> +++ b/include/sysemu/tpm.h
> @@ -45,11 +45,14 @@ typedef struct TPMIfClass {
>
> #define TYPE_TPM_TIS "tpm-tis"
> #define TYPE_TPM_CRB "tpm-crb"
> +#define TYPE_TPM_SPAPR "tpm-spapr"
>
> #define TPM_IS_TIS(chr) \
> object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS)
> #define TPM_IS_CRB(chr) \
> object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
> +#define TPM_IS_SPAPR(chr) \
> + object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)
>
> /* returns NULL unless there is exactly one TPM device */
> static inline TPMIf *tpm_find(void)
> diff --git a/qapi/tpm.json b/qapi/tpm.json
> index b30323bb6b..63878aa0f4 100644
> --- a/qapi/tpm.json
> +++ b/qapi/tpm.json
> @@ -12,11 +12,11 @@
> #
> # @tpm-tis: TPM TIS model
> # @tpm-crb: TPM CRB model (since 2.12)
> +# @tpm-spapr: TPM SPAPR model (since 5.0)
> #
> # Since: 1.5
> ##
> -{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb' ] }
> -
> +{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb', 'tpm-spapr' ] }
> ##
> # @query-tpm-models:
> #
> @@ -29,7 +29,7 @@
> # Example:
> #
> # -> { "execute": "query-tpm-models" }
> -# <- { "return": [ "tpm-tis", "tpm-crb" ] }
> +# <- { "return": [ "tpm-tis", "tpm-crb", "tpm-spapr" ] }
> #
> ##
> { 'command': 'query-tpm-models', 'returns': ['TpmModel'] }
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 3/5] tpm_spapr: Support suspend and resume
2019-12-12 20:24 ` [PATCH v5 3/5] tpm_spapr: Support suspend and resume Stefan Berger
@ 2019-12-13 5:39 ` David Gibson
2019-12-13 12:46 ` Stefan Berger
0 siblings, 1 reply; 19+ messages in thread
From: David Gibson @ 2019-12-13 5:39 UTC (permalink / raw)
To: Stefan Berger; +Cc: marcandre.lureau, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4368 bytes --]
On Thu, Dec 12, 2019 at 03:24:28PM -0500, Stefan Berger wrote:
> Extend the tpm_spapr frontend with VM suspend and resume support.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>
> diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
> index c4a67e2403..8f5a142bd4 100644
> --- a/hw/tpm/tpm_spapr.c
> +++ b/hw/tpm/tpm_spapr.c
> @@ -87,6 +87,8 @@ typedef struct {
> TPMVersion be_tpm_version;
>
> size_t be_buffer_size;
> +
> + bool deliver_response; /* whether to deliver response after VM resume */
> } SPAPRvTPMState;
>
> static void tpm_spapr_show_buffer(const unsigned char *buffer,
> @@ -256,6 +258,12 @@ static void tpm_spapr_request_completed(TPMIf *ti, int ret)
> uint32_t len;
> int rc;
>
> + if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
I'm trying to figure out the circumstances in which
request_completed() would get called before post_load on the
destination.
> + /* defer delivery of response until .post_load */
> + s->deliver_response |= true;
|= is a bitwise OR which is not what you want, although it will
*probably* work in practice. Better to just use
s->deliver_response = true;
> + return;
> + }
> +
> s->state = SPAPR_VTPM_STATE_COMPLETION;
>
> /* a max. of be_buffer_size bytes can be transported */
> @@ -316,6 +324,7 @@ static void tpm_spapr_reset(SpaprVioDevice *dev)
> SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
>
> s->state = SPAPR_VTPM_STATE_NONE;
> + s->deliver_response = false;
>
> s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver);
> tpm_spapr_update_deviceclass(dev);
> @@ -339,9 +348,53 @@ static enum TPMVersion tpm_spapr_get_version(TPMIf *ti)
> return tpm_backend_get_tpm_version(s->be_driver);
> }
>
> +/* persistent state handling */
> +
> +static int tpm_spapr_pre_save(void *opaque)
> +{
> + SPAPRvTPMState *s = opaque;
> +
> + s->deliver_response |= tpm_backend_finish_sync(s->be_driver);
Same problem here.
> + trace_tpm_spapr_pre_save(s->deliver_response);
> + /*
> + * we cannot deliver the results to the VM since DMA would touch VM memory
> + */
> +
> + return 0;
> +}
> +
> +static int tpm_spapr_post_load(void *opaque, int version_id)
> +{
> + SPAPRvTPMState *s = opaque;
> +
> + if (s->deliver_response) {
> + trace_tpm_spapr_post_load();
> + /* deliver the results to the VM via DMA */
> + tpm_spapr_request_completed(TPM_IF(s), 0);
> + s->deliver_response = false;
> + }
> +
> + return 0;
> +}
> +
> static const VMStateDescription vmstate_spapr_vtpm = {
> .name = "tpm-spapr",
> - .unmigratable = 1,
> + .version_id = 1,
> + .minimum_version_id = 0,
> + .minimum_version_id_old = 0,
> + .pre_save = tpm_spapr_pre_save,
> + .post_load = tpm_spapr_post_load,
> + .fields = (VMStateField[]) {
> + VMSTATE_SPAPR_VIO(vdev, SPAPRvTPMState),
> +
> + VMSTATE_UINT8(state, SPAPRvTPMState),
> + VMSTATE_BUFFER(buffer, SPAPRvTPMState),
Transferring the whole 4kiB buffer unconditionally when it mostly
won't have anything useful in it doesn't seem like a great idea.
> + /* remember DMA address */
> + VMSTATE_UINT32(crq.s.data, SPAPRvTPMState),
> + VMSTATE_BOOL(deliver_response, SPAPRvTPMState),
> + VMSTATE_END_OF_LIST(),
> + }
> };
>
> static Property tpm_spapr_properties[] = {
> diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> index 6278a39618..d109661b96 100644
> --- a/hw/tpm/trace-events
> +++ b/hw/tpm/trace-events
> @@ -67,3 +67,5 @@ tpm_spapr_do_crq_get_version(uint32_t version) "response: version %u"
> tpm_spapr_do_crq_prepare_to_suspend(void) "response: preparing to suspend"
> tpm_spapr_do_crq_unknown_msg_type(uint8_t type) "Unknown message type 0x%02x"
> tpm_spapr_do_crq_unknown_crq(uint8_t raw1, uint8_t raw2) "unknown CRQ 0x%02x 0x%02x ..."
> +tpm_spapr_pre_save(bool v) "TPM response to deliver after resume: %d"
> +tpm_spapr_post_load(void) "Delivering TPM response after resume"
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 3/5] tpm_spapr: Support suspend and resume
2019-12-13 5:39 ` David Gibson
@ 2019-12-13 12:46 ` Stefan Berger
2019-12-17 0:53 ` David Gibson
0 siblings, 1 reply; 19+ messages in thread
From: Stefan Berger @ 2019-12-13 12:46 UTC (permalink / raw)
To: David Gibson, Stefan Berger; +Cc: marcandre.lureau, qemu-ppc, qemu-devel
On 12/13/19 12:39 AM, David Gibson wrote:
> On Thu, Dec 12, 2019 at 03:24:28PM -0500, Stefan Berger wrote:
>> Extend the tpm_spapr frontend with VM suspend and resume support.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>
>> diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
>> index c4a67e2403..8f5a142bd4 100644
>> --- a/hw/tpm/tpm_spapr.c
>> +++ b/hw/tpm/tpm_spapr.c
>> @@ -87,6 +87,8 @@ typedef struct {
>> TPMVersion be_tpm_version;
>>
>> size_t be_buffer_size;
>> +
>> + bool deliver_response; /* whether to deliver response after VM resume */
>> } SPAPRvTPMState;
>>
>> static void tpm_spapr_show_buffer(const unsigned char *buffer,
>> @@ -256,6 +258,12 @@ static void tpm_spapr_request_completed(TPMIf *ti, int ret)
>> uint32_t len;
>> int rc;
>>
>> + if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
> I'm trying to figure out the circumstances in which
> request_completed() would get called before post_load on the
> destination.
This is on the source side where we must not deliver the response in
case the devices are now suspending but defer the delivery to after the
resume.
>
>> + /* defer delivery of response until .post_load */
>> + s->deliver_response |= true;
> |= is a bitwise OR which is not what you want, although it will
> *probably* work in practice. Better to just use
> s->deliver_response = true;
>
>> + return;
>> + }
>> +
>> s->state = SPAPR_VTPM_STATE_COMPLETION;
>>
>> /* a max. of be_buffer_size bytes can be transported */
>> @@ -316,6 +324,7 @@ static void tpm_spapr_reset(SpaprVioDevice *dev)
>> SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
>>
>> s->state = SPAPR_VTPM_STATE_NONE;
>> + s->deliver_response = false;
>>
>> s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver);
>> tpm_spapr_update_deviceclass(dev);
>> @@ -339,9 +348,53 @@ static enum TPMVersion tpm_spapr_get_version(TPMIf *ti)
>> return tpm_backend_get_tpm_version(s->be_driver);
>> }
>>
>> +/* persistent state handling */
>> +
>> +static int tpm_spapr_pre_save(void *opaque)
>> +{
>> + SPAPRvTPMState *s = opaque;
>> +
>> + s->deliver_response |= tpm_backend_finish_sync(s->be_driver);
> Same problem here.
>
>> + trace_tpm_spapr_pre_save(s->deliver_response);
>> + /*
>> + * we cannot deliver the results to the VM since DMA would touch VM memory
>> + */
>> +
>> + return 0;
>> +}
>> +
>> +static int tpm_spapr_post_load(void *opaque, int version_id)
>> +{
>> + SPAPRvTPMState *s = opaque;
>> +
>> + if (s->deliver_response) {
>> + trace_tpm_spapr_post_load();
>> + /* deliver the results to the VM via DMA */
>> + tpm_spapr_request_completed(TPM_IF(s), 0);
>> + s->deliver_response = false;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static const VMStateDescription vmstate_spapr_vtpm = {
>> .name = "tpm-spapr",
>> - .unmigratable = 1,
>> + .version_id = 1,
>> + .minimum_version_id = 0,
>> + .minimum_version_id_old = 0,
>> + .pre_save = tpm_spapr_pre_save,
>> + .post_load = tpm_spapr_post_load,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_SPAPR_VIO(vdev, SPAPRvTPMState),
>> +
>> + VMSTATE_UINT8(state, SPAPRvTPMState),
>> + VMSTATE_BUFFER(buffer, SPAPRvTPMState),
> Transferring the whole 4kiB buffer unconditionally when it mostly
> won't have anything useful in it doesn't seem like a great idea.
It's really only needed in case of a 'delayed response'. So, yeah, we
could transfer data in only that case then.
>
>> + /* remember DMA address */
>> + VMSTATE_UINT32(crq.s.data, SPAPRvTPMState),
>> + VMSTATE_BOOL(deliver_response, SPAPRvTPMState),
>> + VMSTATE_END_OF_LIST(),
>> + }
>> };
>>
>> static Property tpm_spapr_properties[] = {
>> diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
>> index 6278a39618..d109661b96 100644
>> --- a/hw/tpm/trace-events
>> +++ b/hw/tpm/trace-events
>> @@ -67,3 +67,5 @@ tpm_spapr_do_crq_get_version(uint32_t version) "response: version %u"
>> tpm_spapr_do_crq_prepare_to_suspend(void) "response: preparing to suspend"
>> tpm_spapr_do_crq_unknown_msg_type(uint8_t type) "Unknown message type 0x%02x"
>> tpm_spapr_do_crq_unknown_crq(uint8_t raw1, uint8_t raw2) "unknown CRQ 0x%02x 0x%02x ..."
>> +tpm_spapr_pre_save(bool v) "TPM response to deliver after resume: %d"
>> +tpm_spapr_post_load(void) "Delivering TPM response after resume"
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
2019-12-13 5:34 ` David Gibson
@ 2019-12-13 13:03 ` Stefan Berger
2019-12-17 0:29 ` David Gibson
0 siblings, 1 reply; 19+ messages in thread
From: Stefan Berger @ 2019-12-13 13:03 UTC (permalink / raw)
To: David Gibson, Stefan Berger; +Cc: marcandre.lureau, qemu-ppc, qemu-devel
On 12/13/19 12:34 AM, David Gibson wrote:
> On Thu, Dec 12, 2019 at 03:24:26PM -0500, Stefan Berger wrote:
>> Implement support for TPM on ppc64 by implementing the vTPM CRQ interface
>> as a frontend. It can use the tpm_emulator driver backend with the external
>> swtpm.
>>
>> The Linux vTPM driver for ppc64 works with this emulation.
>>
>> This TPM emulator also handles the TPM 2 case.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>>
>> diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
>> index 4c8ee87d67..66a570aac1 100644
>> --- a/hw/tpm/Kconfig
>> +++ b/hw/tpm/Kconfig
>> @@ -22,3 +22,9 @@ config TPM_EMULATOR
>> bool
>> default y
>> depends on TPMDEV
>> +
>> +config TPM_SPAPR
>> + bool
>> + default n
>> + select TPMDEV
>> + depends on PSERIES
>> diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
>> index de0b85d02a..85eb99ae05 100644
>> --- a/hw/tpm/Makefile.objs
>> +++ b/hw/tpm/Makefile.objs
>> @@ -4,3 +4,4 @@ common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
>> common-obj-$(CONFIG_TPM_CRB) += tpm_crb.o
>> common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
>> common-obj-$(CONFIG_TPM_EMULATOR) += tpm_emulator.o
>> +obj-$(CONFIG_TPM_SPAPR) += tpm_spapr.o
>> diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
>> new file mode 100644
>> index 0000000000..c4a67e2403
>> --- /dev/null
>> +++ b/hw/tpm/tpm_spapr.c
>> @@ -0,0 +1,405 @@
>> +/*
>> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
>> + *
>> + * PAPR Virtual TPM
>> + *
>> + * Copyright (c) 2015, 2017 IBM Corporation.
>> + *
>> + * Authors:
>> + * Stefan Berger <stefanb@linux.vnet.ibm.com>
>> + *
>> + * This code is licensed under the GPL version 2 or later. See the
>> + * COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>> +#include "qapi/error.h"
>> +#include "hw/qdev-properties.h"
>> +#include "migration/vmstate.h"
>> +
>> +#include "sysemu/tpm_backend.h"
>> +#include "tpm_int.h"
>> +#include "tpm_util.h"
>> +
>> +#include "hw/ppc/spapr.h"
>> +#include "hw/ppc/spapr_vio.h"
>> +#include "trace.h"
>> +
>> +#define DEBUG_SPAPR 0
>> +
>> +#define VIO_SPAPR_VTPM(obj) \
>> + OBJECT_CHECK(SPAPRvTPMState, (obj), TYPE_TPM_SPAPR)
>> +
>> +typedef struct VioCRQ {
> How does this structure relate to the existing SpaprVioCrq?
The existing one looks like this:
typedef struct SpaprVioCrq {
uint64_t qladdr;
uint32_t qsize;
uint32_t qnext;
int(*SendFunc)(struct SpaprVioDevice *vdev, uint8_t *crq);
} SpaprVioCrq;
I don't seem to find the fields there that we need for vTPM support.
>
> Also we're now avoiding exceptions to StudlyCaps, because it causes
> more confusion even if it is to match other capitalization
> conventions. So, I'd suggest 'VioCrq', 'TpmSpaprCrq' etc.
Will adjust.
>
>> + uint8_t valid; /* 0x80: cmd; 0xc0: init crq */
>> + /* 0x81-0x83: CRQ message response */
>> + uint8_t msg; /* see below */
>> + uint16_t len; /* len of TPM request; len of TPM response */
>> + uint32_t data; /* rtce_dma_handle when sending TPM request */
>> + uint64_t reserved;
>> +} VioCRQ;
>> +
>> +typedef union TPMSpaprCRQ {
>> + VioCRQ s;
>> + uint8_t raw[sizeof(VioCRQ)];
>> +} TPMSpaprCRQ;
> A union just to get raw bytes seems a really weird thing to do (as
> opposed to just casting to (char *))
Ok, I will change it.
>
>> +
>> +#define SPAPR_VTPM_VALID_INIT_CRQ_COMMAND 0xC0
>> +#define SPAPR_VTPM_VALID_COMMAND 0x80
>> +#define SPAPR_VTPM_MSG_RESULT 0x80
>> +
>> +/* msg types for valid = SPAPR_VTPM_VALID_INIT_CRQ */
>> +#define SPAPR_VTPM_INIT_CRQ_RESULT 0x1
>> +#define SPAPR_VTPM_INIT_CRQ_COMPLETE_RESULT 0x2
>> +
>> +/* msg types for valid = SPAPR_VTPM_VALID_CMD */
>> +#define SPAPR_VTPM_GET_VERSION 0x1
>> +#define SPAPR_VTPM_TPM_COMMAND 0x2
>> +#define SPAPR_VTPM_GET_RTCE_BUFFER_SIZE 0x3
>> +#define SPAPR_VTPM_PREPARE_TO_SUSPEND 0x4
>> +
>> +/* response error messages */
>> +#define SPAPR_VTPM_VTPM_ERROR 0xff
>> +
>> +/* error codes */
>> +#define SPAPR_VTPM_ERR_COPY_IN_FAILED 0x3
>> +#define SPAPR_VTPM_ERR_COPY_OUT_FAILED 0x4
>> +
>> +#define MAX_BUFFER_SIZE TARGET_PAGE_SIZE
>> +
>> +typedef struct {
>> + SpaprVioDevice vdev;
>> +
>> + TPMSpaprCRQ crq; /* track single TPM command */
>> +
>> + uint8_t state;
>> +#define SPAPR_VTPM_STATE_NONE 0
>> +#define SPAPR_VTPM_STATE_EXECUTION 1
>> +#define SPAPR_VTPM_STATE_COMPLETION 2
> I see this field written, but never read. What's up with that?
if (s->state == SPAPR_VTPM_STATE_EXECUTION) {
return H_BUSY;
}
Is this what you mean?
>
>> +
>> + unsigned char buffer[MAX_BUFFER_SIZE];
>> +
>> + TPMBackendCmd cmd;
>> +
>> + TPMBackend *be_driver;
>> + TPMVersion be_tpm_version;
>> +
>> + size_t be_buffer_size;
>> +} SPAPRvTPMState;
> SpaprVtpmState
>
> Or just SpaprTpmState, since we use just "tpm spapr" rather than
> "vtpm" in plenty of other places.
Will adjust.
>
>> +
>> +static void tpm_spapr_show_buffer(const unsigned char *buffer,
>> + size_t buffer_size, const char *string)
>> +{
>> + size_t len, i;
>> + char *line_buffer, *p;
>> +
>> + len = MIN(tpm_cmd_get_size(buffer), buffer_size);
>> +
>> + /*
>> + * allocate enough room for 3 chars per buffer entry plus a
>> + * newline after every 16 chars and a final null terminator.
>> + */
>> + line_buffer = g_malloc(len * 3 + (len / 16) + 1);
> You can use g_strdup_printf() / g_string_append_printf() to avoid
> fiddly messing around with allocations like this.
This is a 1:1 copy from the existing TIS driver.
>
>> +
>> + for (i = 0, p = line_buffer; i < len; i++) {
>> + if (i && !(i % 16)) {
>> + p += sprintf(p, "\n");
>> + }
>> + p += sprintf(p, "%.2X ", buffer[i]);
>> + }
>> + trace_tpm_spapr_show_buffer(string, len, line_buffer);
>> +
>> + g_free(line_buffer);
>> +}
>> +
>> +/*
>> + * Send a request to the TPM.
>> + */
>> +static void tpm_spapr_tpm_send(SPAPRvTPMState *s)
>> +{
>> + if (trace_event_get_state_backends(TRACE_TPM_SPAPR_SHOW_BUFFER)) {
>> + tpm_spapr_show_buffer(s->buffer, sizeof(s->buffer), "To TPM");
>> + }
>> +
>> + s->state = SPAPR_VTPM_STATE_EXECUTION;
>> + s->cmd = (TPMBackendCmd) {
>> + .locty = 0,
>> + .in = s->buffer,
>> + .in_len = MIN(tpm_cmd_get_size(s->buffer), sizeof(s->buffer)),
>> + .out = s->buffer,
>> + .out_len = sizeof(s->buffer),
>> + };
>> +
>> + tpm_backend_deliver_request(s->be_driver, &s->cmd);
>> +}
>> +
>> +static int tpm_spapr_process_cmd(SPAPRvTPMState *s, uint64_t dataptr)
>> +{
>> + long rc;
>> +
>> + /* a max. of be_buffer_size bytes can be transported */
>> + rc = spapr_vio_dma_read(&s->vdev, dataptr,
>> + s->buffer, s->be_buffer_size);
>> + if (rc) {
>> + error_report("tpm_spapr_got_payload: DMA read failure");
>> + }
>> + /* let vTPM handle any malformed request */
>> + tpm_spapr_tpm_send(s);
>> +
>> + return rc;
>> +}
>> +
>> +static int tpm_spapr_do_crq(struct SpaprVioDevice *dev, uint8_t *crq_data)
>> +{
>> + SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
>> + TPMSpaprCRQ local_crq;
>> + TPMSpaprCRQ *crq = &s->crq; /* requests only */
>> + int rc;
>> +
>> + memcpy(&local_crq.raw, crq_data, sizeof(local_crq.raw));
> Again, no real need for a union here, you can just memcpy onto a
> structure variable.
>
>> + trace_tpm_spapr_do_crq(local_crq.raw[0], local_crq.raw[1]);
>> +
>> + switch (local_crq.s.valid) {
>> + case SPAPR_VTPM_VALID_INIT_CRQ_COMMAND: /* Init command/response */
>> +
>> + /* Respond to initialization request */
>> + switch (local_crq.s.msg) {
>> + case SPAPR_VTPM_INIT_CRQ_RESULT:
>> + trace_tpm_spapr_do_crq_crq_result();
>> + memset(local_crq.raw, 0, sizeof(local_crq.raw));
>> + local_crq.s.valid = SPAPR_VTPM_VALID_INIT_CRQ_COMMAND;
>> + local_crq.s.msg = SPAPR_VTPM_INIT_CRQ_RESULT;
>> + spapr_vio_send_crq(dev, local_crq.raw);
>> + break;
>> +
>> + case SPAPR_VTPM_INIT_CRQ_COMPLETE_RESULT:
>> + trace_tpm_spapr_do_crq_crq_complete_result();
>> + memset(local_crq.raw, 0, sizeof(local_crq.raw));
>> + local_crq.s.valid = SPAPR_VTPM_VALID_INIT_CRQ_COMMAND;
>> + local_crq.s.msg = SPAPR_VTPM_INIT_CRQ_COMPLETE_RESULT;
>> + spapr_vio_send_crq(dev, local_crq.raw);
>> + break;
>> + }
>> +
>> + break;
>> + case SPAPR_VTPM_VALID_COMMAND: /* Payloads */
>> + switch (local_crq.s.msg) {
>> + case SPAPR_VTPM_TPM_COMMAND:
>> + trace_tpm_spapr_do_crq_tpm_command();
>> + if (s->state == SPAPR_VTPM_STATE_EXECUTION) {
>> + return H_BUSY;
>> + }
>> + /* this crq is tracked */
>> + memcpy(crq->raw, crq_data, sizeof(crq->raw));
>> +
>> + rc = tpm_spapr_process_cmd(s, be32_to_cpu(crq->s.data));
>> +
>> + if (rc == H_SUCCESS) {
>> + crq->s.valid = be16_to_cpu(0);
>> + } else {
>> + local_crq.s.valid = SPAPR_VTPM_MSG_RESULT;
>> + local_crq.s.msg = SPAPR_VTPM_VTPM_ERROR;
>> + local_crq.s.data = cpu_to_be32(SPAPR_VTPM_ERR_COPY_IN_FAILED);
>> + spapr_vio_send_crq(dev, local_crq.raw);
>> + }
>> + break;
>> +
>> + case SPAPR_VTPM_GET_RTCE_BUFFER_SIZE:
>> + trace_tpm_spapr_do_crq_tpm_get_rtce_buffer_size(s->be_buffer_size);
>> + local_crq.s.msg |= SPAPR_VTPM_MSG_RESULT;
>> + local_crq.s.len = cpu_to_be16(s->be_buffer_size);
>> + spapr_vio_send_crq(dev, local_crq.raw);
>> + break;
>> +
>> + case SPAPR_VTPM_GET_VERSION:
>> + local_crq.s.msg |= SPAPR_VTPM_MSG_RESULT;
>> + local_crq.s.len = cpu_to_be16(0);
>> + switch (s->be_tpm_version) {
>> + case TPM_VERSION_UNSPEC:
>> + local_crq.s.data = cpu_to_be32(0);
>> + break;
>> + case TPM_VERSION_1_2:
>> + local_crq.s.data = cpu_to_be32(1);
>> + break;
>> + case TPM_VERSION_2_0:
>> + local_crq.s.data = cpu_to_be32(2);
>> + break;
> To make the breakage obvious when/if the backend adds a new version we
> should probably have a default: case with g_assert_not_reached() or
> the like.
I will add this.
>
>> + }
>> + trace_tpm_spapr_do_crq_get_version(be32_to_cpu(local_crq.s.data));
>> + spapr_vio_send_crq(dev, local_crq.raw);
>> + break;
>> +
>> + case SPAPR_VTPM_PREPARE_TO_SUSPEND:
>> + trace_tpm_spapr_do_crq_prepare_to_suspend();
>> + local_crq.s.msg |= SPAPR_VTPM_MSG_RESULT;
>> + spapr_vio_send_crq(dev, local_crq.raw);
>> + break;
>> +
>> + default:
>> + trace_tpm_spapr_do_crq_unknown_msg_type(crq->s.msg);
>> + }
>> + break;
>> + default:
>> + trace_tpm_spapr_do_crq_unknown_crq(local_crq.raw[0], local_crq.raw[1]);
>> + };
>> +
>> + return H_SUCCESS;
>> +}
>> +
>> +static void tpm_spapr_request_completed(TPMIf *ti, int ret)
>> +{
>> + SPAPRvTPMState *s = VIO_SPAPR_VTPM(ti);
>> + TPMSpaprCRQ *crq = &s->crq;
>> + uint32_t len;
>> + int rc;
>> +
>> + s->state = SPAPR_VTPM_STATE_COMPLETION;
>> +
>> + /* a max. of be_buffer_size bytes can be transported */
>> + len = MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size);
>> + rc = spapr_vio_dma_write(&s->vdev, be32_to_cpu(crq->s.data),
>> + s->buffer, len);
>> +
>> + if (trace_event_get_state_backends(TRACE_TPM_SPAPR_SHOW_BUFFER)) {
>> + tpm_spapr_show_buffer(s->buffer, len, "From TPM");
>> + }
>> +
>> + crq->s.valid = SPAPR_VTPM_MSG_RESULT;
>> + if (rc == H_SUCCESS) {
>> + crq->s.msg = SPAPR_VTPM_TPM_COMMAND | SPAPR_VTPM_MSG_RESULT;
>> + crq->s.len = cpu_to_be16(len);
>> + } else {
>> + error_report("%s: DMA write failure", __func__);
>> + crq->s.msg = SPAPR_VTPM_VTPM_ERROR;
>> + crq->s.len = cpu_to_be16(0);
>> + crq->s.data = cpu_to_be32(SPAPR_VTPM_ERR_COPY_OUT_FAILED);
>> + }
>> +
>> + rc = spapr_vio_send_crq(&s->vdev, crq->raw);
>> + if (rc) {
>> + error_report("%s: Error sending response", __func__);
>> + }
>> +}
>> +
>> +static int tpm_spapr_do_startup_tpm(SPAPRvTPMState *s, size_t buffersize)
>> +{
>> + return tpm_backend_startup_tpm(s->be_driver, buffersize);
>> +}
>> +
>> +static void tpm_spapr_update_deviceclass(SpaprVioDevice *dev)
>> +{
>> + SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
>> + SpaprVioDeviceClass *k = VIO_SPAPR_DEVICE_GET_CLASS(dev);
>> +
>> + switch (s->be_tpm_version) {
>> + case TPM_VERSION_UNSPEC:
>> + assert(false);
>> + break;
>> + case TPM_VERSION_1_2:
>> + k->dt_name = "vtpm";
>> + k->dt_type = "IBM,vtpm";
>> + k->dt_compatible = "IBM,vtpm";
>> + break;
>> + case TPM_VERSION_2_0:
>> + k->dt_name = "vtpm";
>> + k->dt_type = "IBM,vtpm";
>> + k->dt_compatible = "IBM,vtpm20";
>> + break;
> Erk. Updating DeviceClass structures on the fly is hideously ugly.
> We might need to take a different approach for this.
Make a suggestion... Obviously, we can hard-initialize dt_name and
dt_type but dt_compatible can only be set after we have determined the
version of TPM.
>
>> + }
>> +}
>> +
>> +static void tpm_spapr_reset(SpaprVioDevice *dev)
>> +{
>> + SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
>> +
>> + s->state = SPAPR_VTPM_STATE_NONE;
>> +
>> + s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver);
>> + tpm_spapr_update_deviceclass(dev);
>> +
>> + s->be_buffer_size = MAX(ROUND_UP(tpm_backend_get_buffer_size(s->be_driver),
>> + TARGET_PAGE_SIZE),
>> + sizeof(s->buffer));
> I'm very confused as to what be_buffer_size is supposed to represent.
> it's not the backend size, because you're rounding that up and taking
> MAX with another thing. But it's not the max safe size for this
> locally, because it can be bigger than s->buffer.
Will adjust.
>
>> + tpm_backend_reset(s->be_driver);
>> + tpm_spapr_do_startup_tpm(s, s->be_buffer_size);
>> +}
>> +
>> +static enum TPMVersion tpm_spapr_get_version(TPMIf *ti)
>> +{
>> + SPAPRvTPMState *s = VIO_SPAPR_VTPM(ti);
>> +
>> + if (tpm_backend_had_startup_error(s->be_driver)) {
>> + return TPM_VERSION_UNSPEC;
>> + }
>> +
>> + return tpm_backend_get_tpm_version(s->be_driver);
>> +}
>> +
>> +static const VMStateDescription vmstate_spapr_vtpm = {
>> + .name = "tpm-spapr",
>> + .unmigratable = 1,
>> +};
>> +
>> +static Property tpm_spapr_properties[] = {
>> + DEFINE_SPAPR_PROPERTIES(SPAPRvTPMState, vdev),
>> + DEFINE_PROP_TPMBE("tpmdev", SPAPRvTPMState, be_driver),
>> + DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void tpm_spapr_realizefn(SpaprVioDevice *dev, Error **errp)
>> +{
>> + SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
>> +
>> + if (!tpm_find()) {
> This seems wrong, even though I also see it in existing TPM code.
> AFAICT tpm_find() returns a TPMIf pointer for the existing TPM if it
> exists, meaning !tpm_find() will be true if there is *not* an existing
> TPM.
/* returns NULL unless there is exactly one TPM device */
static inline TPMIf *tpm_find(void)
{
Object *obj = object_resolve_path_type("", TYPE_TPM_IF, NULL);
return TPM_IF(obj);
}
I would rather leave it like this.
>
>> + error_setg(errp, "at most one TPM device is permitted");
>> + return;
>> + }
>> +
>> + dev->crq.SendFunc = tpm_spapr_do_crq;
>> +
>> + if (!s->be_driver) {
>> + error_setg(errp, "'tpmdev' property is required");
>> + return;
>> + }
>> +}
>> +
>> +static void tpm_spapr_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> + SpaprVioDeviceClass *k = VIO_SPAPR_DEVICE_CLASS(klass);
>> + TPMIfClass *tc = TPM_IF_CLASS(klass);
>> +
>> + k->realize = tpm_spapr_realizefn;
>> + k->reset = tpm_spapr_reset;
>> + k->signal_mask = 0x00000001;
>> + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> + dc->props = tpm_spapr_properties;
>> + k->rtce_window_size = 0x10000000;
>> + dc->vmsd = &vmstate_spapr_vtpm;
>> +
>> + tc->model = TPM_MODEL_TPM_SPAPR;
>> + tc->get_version = tpm_spapr_get_version;
>> + tc->request_completed = tpm_spapr_request_completed;
>> +}
>> +
>> +static const TypeInfo tpm_spapr_info = {
>> + .name = TYPE_TPM_SPAPR,
>> + .parent = TYPE_VIO_SPAPR_DEVICE,
>> + .instance_size = sizeof(SPAPRvTPMState),
>> + .class_init = tpm_spapr_class_init,
>> + .interfaces = (InterfaceInfo[]) {
>> + { TYPE_TPM_IF },
>> + { }
>> + }
>> +};
>> +
>> +static void tpm_spapr_register_types(void)
>> +{
>> + type_register_static(&tpm_spapr_info);
>> +}
>> +
>> +type_init(tpm_spapr_register_types)
>> diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
>> index 89804bcd64..6278a39618 100644
>> --- a/hw/tpm/trace-events
>> +++ b/hw/tpm/trace-events
>> @@ -55,3 +55,15 @@ tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
>>
>> # tpm_ppi.c
>> tpm_ppi_memset(uint8_t *ptr, size_t size) "memset: %p %zu"
>> +
>> +# hw/tpm/tpm_spapr.c
>> +tpm_spapr_show_buffer(const char *direction, size_t len, const char *buf) "direction: %s len: %zu\n%s"
>> +tpm_spapr_do_crq(uint8_t raw1, uint8_t raw2) "1st 2 bytes in CRQ: 0x%02x 0x%02x"
>> +tpm_spapr_do_crq_crq_result(void) "SPAPR_VTPM_INIT_CRQ_RESULT"
>> +tpm_spapr_do_crq_crq_complete_result(void) "SPAPR_VTPM_INIT_CRQ_COMP_RESULT"
>> +tpm_spapr_do_crq_tpm_command(void) "got TPM command payload"
>> +tpm_spapr_do_crq_tpm_get_rtce_buffer_size(size_t buffersize) "response: buffer size is %zu"
>> +tpm_spapr_do_crq_get_version(uint32_t version) "response: version %u"
>> +tpm_spapr_do_crq_prepare_to_suspend(void) "response: preparing to suspend"
>> +tpm_spapr_do_crq_unknown_msg_type(uint8_t type) "Unknown message type 0x%02x"
>> +tpm_spapr_do_crq_unknown_crq(uint8_t raw1, uint8_t raw2) "unknown CRQ 0x%02x 0x%02x ..."
>> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
>> index 5b541a71c8..15979a3647 100644
>> --- a/include/sysemu/tpm.h
>> +++ b/include/sysemu/tpm.h
>> @@ -45,11 +45,14 @@ typedef struct TPMIfClass {
>>
>> #define TYPE_TPM_TIS "tpm-tis"
>> #define TYPE_TPM_CRB "tpm-crb"
>> +#define TYPE_TPM_SPAPR "tpm-spapr"
>>
>> #define TPM_IS_TIS(chr) \
>> object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS)
>> #define TPM_IS_CRB(chr) \
>> object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
>> +#define TPM_IS_SPAPR(chr) \
>> + object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)
>>
>> /* returns NULL unless there is exactly one TPM device */
>> static inline TPMIf *tpm_find(void)
>> diff --git a/qapi/tpm.json b/qapi/tpm.json
>> index b30323bb6b..63878aa0f4 100644
>> --- a/qapi/tpm.json
>> +++ b/qapi/tpm.json
>> @@ -12,11 +12,11 @@
>> #
>> # @tpm-tis: TPM TIS model
>> # @tpm-crb: TPM CRB model (since 2.12)
>> +# @tpm-spapr: TPM SPAPR model (since 5.0)
>> #
>> # Since: 1.5
>> ##
>> -{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb' ] }
>> -
>> +{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb', 'tpm-spapr' ] }
>> ##
>> # @query-tpm-models:
>> #
>> @@ -29,7 +29,7 @@
>> # Example:
>> #
>> # -> { "execute": "query-tpm-models" }
>> -# <- { "return": [ "tpm-tis", "tpm-crb" ] }
>> +# <- { "return": [ "tpm-tis", "tpm-crb", "tpm-spapr" ] }
>> #
>> ##
>> { 'command': 'query-tpm-models', 'returns': ['TpmModel'] }
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
2019-12-13 13:03 ` Stefan Berger
@ 2019-12-17 0:29 ` David Gibson
2019-12-17 19:44 ` Stefan Berger
0 siblings, 1 reply; 19+ messages in thread
From: David Gibson @ 2019-12-17 0:29 UTC (permalink / raw)
To: Stefan Berger; +Cc: marcandre.lureau, qemu-ppc, qemu-devel, Stefan Berger
[-- Attachment #1: Type: text/plain, Size: 23765 bytes --]
On Fri, Dec 13, 2019 at 08:03:36AM -0500, Stefan Berger wrote:
> On 12/13/19 12:34 AM, David Gibson wrote:
> > On Thu, Dec 12, 2019 at 03:24:26PM -0500, Stefan Berger wrote:
> > > Implement support for TPM on ppc64 by implementing the vTPM CRQ interface
> > > as a frontend. It can use the tpm_emulator driver backend with the external
> > > swtpm.
> > >
> > > The Linux vTPM driver for ppc64 works with this emulation.
> > >
> > > This TPM emulator also handles the TPM 2 case.
> > >
> > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > >
> > > diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
> > > index 4c8ee87d67..66a570aac1 100644
> > > --- a/hw/tpm/Kconfig
> > > +++ b/hw/tpm/Kconfig
> > > @@ -22,3 +22,9 @@ config TPM_EMULATOR
> > > bool
> > > default y
> > > depends on TPMDEV
> > > +
> > > +config TPM_SPAPR
> > > + bool
> > > + default n
> > > + select TPMDEV
> > > + depends on PSERIES
> > > diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
> > > index de0b85d02a..85eb99ae05 100644
> > > --- a/hw/tpm/Makefile.objs
> > > +++ b/hw/tpm/Makefile.objs
> > > @@ -4,3 +4,4 @@ common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
> > > common-obj-$(CONFIG_TPM_CRB) += tpm_crb.o
> > > common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
> > > common-obj-$(CONFIG_TPM_EMULATOR) += tpm_emulator.o
> > > +obj-$(CONFIG_TPM_SPAPR) += tpm_spapr.o
> > > diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
> > > new file mode 100644
> > > index 0000000000..c4a67e2403
> > > --- /dev/null
> > > +++ b/hw/tpm/tpm_spapr.c
> > > @@ -0,0 +1,405 @@
> > > +/*
> > > + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
> > > + *
> > > + * PAPR Virtual TPM
> > > + *
> > > + * Copyright (c) 2015, 2017 IBM Corporation.
> > > + *
> > > + * Authors:
> > > + * Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > + *
> > > + * This code is licensed under the GPL version 2 or later. See the
> > > + * COPYING file in the top-level directory.
> > > + *
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "qemu/error-report.h"
> > > +#include "qapi/error.h"
> > > +#include "hw/qdev-properties.h"
> > > +#include "migration/vmstate.h"
> > > +
> > > +#include "sysemu/tpm_backend.h"
> > > +#include "tpm_int.h"
> > > +#include "tpm_util.h"
> > > +
> > > +#include "hw/ppc/spapr.h"
> > > +#include "hw/ppc/spapr_vio.h"
> > > +#include "trace.h"
> > > +
> > > +#define DEBUG_SPAPR 0
> > > +
> > > +#define VIO_SPAPR_VTPM(obj) \
> > > + OBJECT_CHECK(SPAPRvTPMState, (obj), TYPE_TPM_SPAPR)
> > > +
> > > +typedef struct VioCRQ {
> > How does this structure relate to the existing SpaprVioCrq?
>
> The existing one looks like this:
>
> typedef struct SpaprVioCrq {
> uint64_t qladdr;
> uint32_t qsize;
> uint32_t qnext;
> int(*SendFunc)(struct SpaprVioDevice *vdev, uint8_t *crq);
> } SpaprVioCrq;
>
> I don't seem to find the fields there that we need for vTPM support.
Yeah, I can see the difference in the structures. What I'm after is
what is the difference in purpose which means they have different
content.
Having read through the whole series now, I *think* the answer is that
the tpm specific structure is one entry in the request queue for the
vtpm, whereas the VioCrq structure is a handle on an entire queue.
I think the tpm one needs a rename to reflect that a) it's vtpm
specific and b) it's not actually a queue, just part of it.
> > Also we're now avoiding exceptions to StudlyCaps, because it causes
> > more confusion even if it is to match other capitalization
> > conventions. So, I'd suggest 'VioCrq', 'TpmSpaprCrq' etc.
>
>
> Will adjust.
>
>
> >
> > > + uint8_t valid; /* 0x80: cmd; 0xc0: init crq */
> > > + /* 0x81-0x83: CRQ message response */
> > > + uint8_t msg; /* see below */
> > > + uint16_t len; /* len of TPM request; len of TPM response */
> > > + uint32_t data; /* rtce_dma_handle when sending TPM request */
> > > + uint64_t reserved;
> > > +} VioCRQ;
> > > +
> > > +typedef union TPMSpaprCRQ {
> > > + VioCRQ s;
> > > + uint8_t raw[sizeof(VioCRQ)];
> > > +} TPMSpaprCRQ;
> > A union just to get raw bytes seems a really weird thing to do (as
> > opposed to just casting to (char *))
>
>
> Ok, I will change it.
>
>
>
> >
> > > +
> > > +#define SPAPR_VTPM_VALID_INIT_CRQ_COMMAND 0xC0
> > > +#define SPAPR_VTPM_VALID_COMMAND 0x80
> > > +#define SPAPR_VTPM_MSG_RESULT 0x80
> > > +
> > > +/* msg types for valid = SPAPR_VTPM_VALID_INIT_CRQ */
> > > +#define SPAPR_VTPM_INIT_CRQ_RESULT 0x1
> > > +#define SPAPR_VTPM_INIT_CRQ_COMPLETE_RESULT 0x2
> > > +
> > > +/* msg types for valid = SPAPR_VTPM_VALID_CMD */
> > > +#define SPAPR_VTPM_GET_VERSION 0x1
> > > +#define SPAPR_VTPM_TPM_COMMAND 0x2
> > > +#define SPAPR_VTPM_GET_RTCE_BUFFER_SIZE 0x3
> > > +#define SPAPR_VTPM_PREPARE_TO_SUSPEND 0x4
> > > +
> > > +/* response error messages */
> > > +#define SPAPR_VTPM_VTPM_ERROR 0xff
> > > +
> > > +/* error codes */
> > > +#define SPAPR_VTPM_ERR_COPY_IN_FAILED 0x3
> > > +#define SPAPR_VTPM_ERR_COPY_OUT_FAILED 0x4
> > > +
> > > +#define MAX_BUFFER_SIZE TARGET_PAGE_SIZE
> > > +
> > > +typedef struct {
> > > + SpaprVioDevice vdev;
> > > +
> > > + TPMSpaprCRQ crq; /* track single TPM command */
> > > +
> > > + uint8_t state;
> > > +#define SPAPR_VTPM_STATE_NONE 0
> > > +#define SPAPR_VTPM_STATE_EXECUTION 1
> > > +#define SPAPR_VTPM_STATE_COMPLETION 2
> > I see this field written, but never read. What's up with that?
>
>
> if (s->state == SPAPR_VTPM_STATE_EXECUTION) {
> return H_BUSY;
> }
>
> Is this what you mean?
Oh, I must have missed that, sorry.
>
>
> >
> > > +
> > > + unsigned char buffer[MAX_BUFFER_SIZE];
> > > +
> > > + TPMBackendCmd cmd;
> > > +
> > > + TPMBackend *be_driver;
> > > + TPMVersion be_tpm_version;
> > > +
> > > + size_t be_buffer_size;
> > > +} SPAPRvTPMState;
> > SpaprVtpmState
> >
> > Or just SpaprTpmState, since we use just "tpm spapr" rather than
> > "vtpm" in plenty of other places.
>
>
> Will adjust.
>
>
> >
> > > +
> > > +static void tpm_spapr_show_buffer(const unsigned char *buffer,
> > > + size_t buffer_size, const char *string)
> > > +{
> > > + size_t len, i;
> > > + char *line_buffer, *p;
> > > +
> > > + len = MIN(tpm_cmd_get_size(buffer), buffer_size);
> > > +
> > > + /*
> > > + * allocate enough room for 3 chars per buffer entry plus a
> > > + * newline after every 16 chars and a final null terminator.
> > > + */
> > > + line_buffer = g_malloc(len * 3 + (len / 16) + 1);
> > You can use g_strdup_printf() / g_string_append_printf() to avoid
> > fiddly messing around with allocations like this.
>
> This is a 1:1 copy from the existing TIS driver.
Hm, right. Probably not a bad idea to move that out as a helper
function then.
> > > +
> > > + for (i = 0, p = line_buffer; i < len; i++) {
> > > + if (i && !(i % 16)) {
> > > + p += sprintf(p, "\n");
> > > + }
> > > + p += sprintf(p, "%.2X ", buffer[i]);
> > > + }
> > > + trace_tpm_spapr_show_buffer(string, len, line_buffer);
> > > +
> > > + g_free(line_buffer);
> > > +}
> > > +
> > > +/*
> > > + * Send a request to the TPM.
> > > + */
> > > +static void tpm_spapr_tpm_send(SPAPRvTPMState *s)
> > > +{
> > > + if (trace_event_get_state_backends(TRACE_TPM_SPAPR_SHOW_BUFFER)) {
> > > + tpm_spapr_show_buffer(s->buffer, sizeof(s->buffer), "To TPM");
> > > + }
> > > +
> > > + s->state = SPAPR_VTPM_STATE_EXECUTION;
> > > + s->cmd = (TPMBackendCmd) {
> > > + .locty = 0,
> > > + .in = s->buffer,
> > > + .in_len = MIN(tpm_cmd_get_size(s->buffer), sizeof(s->buffer)),
> > > + .out = s->buffer,
> > > + .out_len = sizeof(s->buffer),
> > > + };
> > > +
> > > + tpm_backend_deliver_request(s->be_driver, &s->cmd);
> > > +}
> > > +
> > > +static int tpm_spapr_process_cmd(SPAPRvTPMState *s, uint64_t dataptr)
> > > +{
> > > + long rc;
> > > +
> > > + /* a max. of be_buffer_size bytes can be transported */
> > > + rc = spapr_vio_dma_read(&s->vdev, dataptr,
> > > + s->buffer, s->be_buffer_size);
> > > + if (rc) {
> > > + error_report("tpm_spapr_got_payload: DMA read failure");
> > > + }
> > > + /* let vTPM handle any malformed request */
> > > + tpm_spapr_tpm_send(s);
> > > +
> > > + return rc;
> > > +}
> > > +
> > > +static int tpm_spapr_do_crq(struct SpaprVioDevice *dev, uint8_t *crq_data)
> > > +{
> > > + SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
> > > + TPMSpaprCRQ local_crq;
> > > + TPMSpaprCRQ *crq = &s->crq; /* requests only */
> > > + int rc;
> > > +
> > > + memcpy(&local_crq.raw, crq_data, sizeof(local_crq.raw));
> > Again, no real need for a union here, you can just memcpy onto a
> > structure variable.
> >
> > > + trace_tpm_spapr_do_crq(local_crq.raw[0], local_crq.raw[1]);
> > > +
> > > + switch (local_crq.s.valid) {
> > > + case SPAPR_VTPM_VALID_INIT_CRQ_COMMAND: /* Init command/response */
> > > +
> > > + /* Respond to initialization request */
> > > + switch (local_crq.s.msg) {
> > > + case SPAPR_VTPM_INIT_CRQ_RESULT:
> > > + trace_tpm_spapr_do_crq_crq_result();
> > > + memset(local_crq.raw, 0, sizeof(local_crq.raw));
> > > + local_crq.s.valid = SPAPR_VTPM_VALID_INIT_CRQ_COMMAND;
> > > + local_crq.s.msg = SPAPR_VTPM_INIT_CRQ_RESULT;
> > > + spapr_vio_send_crq(dev, local_crq.raw);
> > > + break;
> > > +
> > > + case SPAPR_VTPM_INIT_CRQ_COMPLETE_RESULT:
> > > + trace_tpm_spapr_do_crq_crq_complete_result();
> > > + memset(local_crq.raw, 0, sizeof(local_crq.raw));
> > > + local_crq.s.valid = SPAPR_VTPM_VALID_INIT_CRQ_COMMAND;
> > > + local_crq.s.msg = SPAPR_VTPM_INIT_CRQ_COMPLETE_RESULT;
> > > + spapr_vio_send_crq(dev, local_crq.raw);
> > > + break;
> > > + }
> > > +
> > > + break;
> > > + case SPAPR_VTPM_VALID_COMMAND: /* Payloads */
> > > + switch (local_crq.s.msg) {
> > > + case SPAPR_VTPM_TPM_COMMAND:
> > > + trace_tpm_spapr_do_crq_tpm_command();
> > > + if (s->state == SPAPR_VTPM_STATE_EXECUTION) {
> > > + return H_BUSY;
> > > + }
> > > + /* this crq is tracked */
> > > + memcpy(crq->raw, crq_data, sizeof(crq->raw));
> > > +
> > > + rc = tpm_spapr_process_cmd(s, be32_to_cpu(crq->s.data));
> > > +
> > > + if (rc == H_SUCCESS) {
> > > + crq->s.valid = be16_to_cpu(0);
> > > + } else {
> > > + local_crq.s.valid = SPAPR_VTPM_MSG_RESULT;
> > > + local_crq.s.msg = SPAPR_VTPM_VTPM_ERROR;
> > > + local_crq.s.data = cpu_to_be32(SPAPR_VTPM_ERR_COPY_IN_FAILED);
> > > + spapr_vio_send_crq(dev, local_crq.raw);
> > > + }
> > > + break;
> > > +
> > > + case SPAPR_VTPM_GET_RTCE_BUFFER_SIZE:
> > > + trace_tpm_spapr_do_crq_tpm_get_rtce_buffer_size(s->be_buffer_size);
> > > + local_crq.s.msg |= SPAPR_VTPM_MSG_RESULT;
> > > + local_crq.s.len = cpu_to_be16(s->be_buffer_size);
> > > + spapr_vio_send_crq(dev, local_crq.raw);
> > > + break;
> > > +
> > > + case SPAPR_VTPM_GET_VERSION:
> > > + local_crq.s.msg |= SPAPR_VTPM_MSG_RESULT;
> > > + local_crq.s.len = cpu_to_be16(0);
> > > + switch (s->be_tpm_version) {
> > > + case TPM_VERSION_UNSPEC:
> > > + local_crq.s.data = cpu_to_be32(0);
> > > + break;
> > > + case TPM_VERSION_1_2:
> > > + local_crq.s.data = cpu_to_be32(1);
> > > + break;
> > > + case TPM_VERSION_2_0:
> > > + local_crq.s.data = cpu_to_be32(2);
> > > + break;
> > To make the breakage obvious when/if the backend adds a new version we
> > should probably have a default: case with g_assert_not_reached() or
> > the like.
> I will add this.
> >
> > > + }
> > > + trace_tpm_spapr_do_crq_get_version(be32_to_cpu(local_crq.s.data));
> > > + spapr_vio_send_crq(dev, local_crq.raw);
> > > + break;
> > > +
> > > + case SPAPR_VTPM_PREPARE_TO_SUSPEND:
> > > + trace_tpm_spapr_do_crq_prepare_to_suspend();
> > > + local_crq.s.msg |= SPAPR_VTPM_MSG_RESULT;
> > > + spapr_vio_send_crq(dev, local_crq.raw);
> > > + break;
> > > +
> > > + default:
> > > + trace_tpm_spapr_do_crq_unknown_msg_type(crq->s.msg);
> > > + }
> > > + break;
> > > + default:
> > > + trace_tpm_spapr_do_crq_unknown_crq(local_crq.raw[0], local_crq.raw[1]);
> > > + };
> > > +
> > > + return H_SUCCESS;
> > > +}
> > > +
> > > +static void tpm_spapr_request_completed(TPMIf *ti, int ret)
> > > +{
> > > + SPAPRvTPMState *s = VIO_SPAPR_VTPM(ti);
> > > + TPMSpaprCRQ *crq = &s->crq;
> > > + uint32_t len;
> > > + int rc;
> > > +
> > > + s->state = SPAPR_VTPM_STATE_COMPLETION;
> > > +
> > > + /* a max. of be_buffer_size bytes can be transported */
> > > + len = MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size);
> > > + rc = spapr_vio_dma_write(&s->vdev, be32_to_cpu(crq->s.data),
> > > + s->buffer, len);
> > > +
> > > + if (trace_event_get_state_backends(TRACE_TPM_SPAPR_SHOW_BUFFER)) {
> > > + tpm_spapr_show_buffer(s->buffer, len, "From TPM");
> > > + }
> > > +
> > > + crq->s.valid = SPAPR_VTPM_MSG_RESULT;
> > > + if (rc == H_SUCCESS) {
> > > + crq->s.msg = SPAPR_VTPM_TPM_COMMAND | SPAPR_VTPM_MSG_RESULT;
> > > + crq->s.len = cpu_to_be16(len);
> > > + } else {
> > > + error_report("%s: DMA write failure", __func__);
> > > + crq->s.msg = SPAPR_VTPM_VTPM_ERROR;
> > > + crq->s.len = cpu_to_be16(0);
> > > + crq->s.data = cpu_to_be32(SPAPR_VTPM_ERR_COPY_OUT_FAILED);
> > > + }
> > > +
> > > + rc = spapr_vio_send_crq(&s->vdev, crq->raw);
> > > + if (rc) {
> > > + error_report("%s: Error sending response", __func__);
> > > + }
> > > +}
> > > +
> > > +static int tpm_spapr_do_startup_tpm(SPAPRvTPMState *s, size_t buffersize)
> > > +{
> > > + return tpm_backend_startup_tpm(s->be_driver, buffersize);
> > > +}
> > > +
> > > +static void tpm_spapr_update_deviceclass(SpaprVioDevice *dev)
> > > +{
> > > + SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
> > > + SpaprVioDeviceClass *k = VIO_SPAPR_DEVICE_GET_CLASS(dev);
> > > +
> > > + switch (s->be_tpm_version) {
> > > + case TPM_VERSION_UNSPEC:
> > > + assert(false);
> > > + break;
> > > + case TPM_VERSION_1_2:
> > > + k->dt_name = "vtpm";
> > > + k->dt_type = "IBM,vtpm";
> > > + k->dt_compatible = "IBM,vtpm";
> > > + break;
> > > + case TPM_VERSION_2_0:
> > > + k->dt_name = "vtpm";
> > > + k->dt_type = "IBM,vtpm";
> > > + k->dt_compatible = "IBM,vtpm20";
> > > + break;
> > Erk. Updating DeviceClass structures on the fly is hideously ugly.
> > We might need to take a different approach for this.
>
> Make a suggestion... Obviously, we can hard-initialize dt_name and dt_type
> but dt_compatible can only be set after we have determined the version of
> TPM.
As you say name and type can just be put into the class statically.
Since you need to change compatible based on an internal variable,
we'd need to replace the static dt_compatible in the class with a
callback.
> > > + }
> > > +}
> > > +
> > > +static void tpm_spapr_reset(SpaprVioDevice *dev)
> > > +{
> > > + SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
> > > +
> > > + s->state = SPAPR_VTPM_STATE_NONE;
> > > +
> > > + s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver);
> > > + tpm_spapr_update_deviceclass(dev);
> > > +
> > > + s->be_buffer_size = MAX(ROUND_UP(tpm_backend_get_buffer_size(s->be_driver),
> > > + TARGET_PAGE_SIZE),
> > > + sizeof(s->buffer));
> > I'm very confused as to what be_buffer_size is supposed to represent.
> > it's not the backend size, because you're rounding that up and taking
> > MAX with another thing. But it's not the max safe size for this
> > locally, because it can be bigger than s->buffer.
>
>
> Will adjust.
>
> >
> > > + tpm_backend_reset(s->be_driver);
> > > + tpm_spapr_do_startup_tpm(s, s->be_buffer_size);
> > > +}
> > > +
> > > +static enum TPMVersion tpm_spapr_get_version(TPMIf *ti)
> > > +{
> > > + SPAPRvTPMState *s = VIO_SPAPR_VTPM(ti);
> > > +
> > > + if (tpm_backend_had_startup_error(s->be_driver)) {
> > > + return TPM_VERSION_UNSPEC;
> > > + }
> > > +
> > > + return tpm_backend_get_tpm_version(s->be_driver);
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_spapr_vtpm = {
> > > + .name = "tpm-spapr",
> > > + .unmigratable = 1,
> > > +};
> > > +
> > > +static Property tpm_spapr_properties[] = {
> > > + DEFINE_SPAPR_PROPERTIES(SPAPRvTPMState, vdev),
> > > + DEFINE_PROP_TPMBE("tpmdev", SPAPRvTPMState, be_driver),
> > > + DEFINE_PROP_END_OF_LIST(),
> > > +};
> > > +
> > > +static void tpm_spapr_realizefn(SpaprVioDevice *dev, Error **errp)
> > > +{
> > > + SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
> > > +
> > > + if (!tpm_find()) {
> > This seems wrong, even though I also see it in existing TPM code.
> > AFAICT tpm_find() returns a TPMIf pointer for the existing TPM if it
> > exists, meaning !tpm_find() will be true if there is *not* an existing
> > TPM.
> /* returns NULL unless there is exactly one TPM device */
> static inline TPMIf *tpm_find(void)
> {
> Object *obj = object_resolve_path_type("", TYPE_TPM_IF, NULL);
>
> return TPM_IF(obj);
> }
>
>
> I would rather leave it like this.
Oh, I see. Seems confusing to me, but I guess that's the idiom, so
let it be.
> > > + error_setg(errp, "at most one TPM device is permitted");
> > > + return;
> > > + }
> > > +
> > > + dev->crq.SendFunc = tpm_spapr_do_crq;
> > > +
> > > + if (!s->be_driver) {
> > > + error_setg(errp, "'tpmdev' property is required");
> > > + return;
> > > + }
> > > +}
> > > +
> > > +static void tpm_spapr_class_init(ObjectClass *klass, void *data)
> > > +{
> > > + DeviceClass *dc = DEVICE_CLASS(klass);
> > > + SpaprVioDeviceClass *k = VIO_SPAPR_DEVICE_CLASS(klass);
> > > + TPMIfClass *tc = TPM_IF_CLASS(klass);
> > > +
> > > + k->realize = tpm_spapr_realizefn;
> > > + k->reset = tpm_spapr_reset;
> > > + k->signal_mask = 0x00000001;
> > > + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> > > + dc->props = tpm_spapr_properties;
> > > + k->rtce_window_size = 0x10000000;
> > > + dc->vmsd = &vmstate_spapr_vtpm;
> > > +
> > > + tc->model = TPM_MODEL_TPM_SPAPR;
> > > + tc->get_version = tpm_spapr_get_version;
> > > + tc->request_completed = tpm_spapr_request_completed;
> > > +}
> > > +
> > > +static const TypeInfo tpm_spapr_info = {
> > > + .name = TYPE_TPM_SPAPR,
> > > + .parent = TYPE_VIO_SPAPR_DEVICE,
> > > + .instance_size = sizeof(SPAPRvTPMState),
> > > + .class_init = tpm_spapr_class_init,
> > > + .interfaces = (InterfaceInfo[]) {
> > > + { TYPE_TPM_IF },
> > > + { }
> > > + }
> > > +};
> > > +
> > > +static void tpm_spapr_register_types(void)
> > > +{
> > > + type_register_static(&tpm_spapr_info);
> > > +}
> > > +
> > > +type_init(tpm_spapr_register_types)
> > > diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> > > index 89804bcd64..6278a39618 100644
> > > --- a/hw/tpm/trace-events
> > > +++ b/hw/tpm/trace-events
> > > @@ -55,3 +55,15 @@ tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
> > > # tpm_ppi.c
> > > tpm_ppi_memset(uint8_t *ptr, size_t size) "memset: %p %zu"
> > > +
> > > +# hw/tpm/tpm_spapr.c
> > > +tpm_spapr_show_buffer(const char *direction, size_t len, const char *buf) "direction: %s len: %zu\n%s"
> > > +tpm_spapr_do_crq(uint8_t raw1, uint8_t raw2) "1st 2 bytes in CRQ: 0x%02x 0x%02x"
> > > +tpm_spapr_do_crq_crq_result(void) "SPAPR_VTPM_INIT_CRQ_RESULT"
> > > +tpm_spapr_do_crq_crq_complete_result(void) "SPAPR_VTPM_INIT_CRQ_COMP_RESULT"
> > > +tpm_spapr_do_crq_tpm_command(void) "got TPM command payload"
> > > +tpm_spapr_do_crq_tpm_get_rtce_buffer_size(size_t buffersize) "response: buffer size is %zu"
> > > +tpm_spapr_do_crq_get_version(uint32_t version) "response: version %u"
> > > +tpm_spapr_do_crq_prepare_to_suspend(void) "response: preparing to suspend"
> > > +tpm_spapr_do_crq_unknown_msg_type(uint8_t type) "Unknown message type 0x%02x"
> > > +tpm_spapr_do_crq_unknown_crq(uint8_t raw1, uint8_t raw2) "unknown CRQ 0x%02x 0x%02x ..."
> > > diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> > > index 5b541a71c8..15979a3647 100644
> > > --- a/include/sysemu/tpm.h
> > > +++ b/include/sysemu/tpm.h
> > > @@ -45,11 +45,14 @@ typedef struct TPMIfClass {
> > > #define TYPE_TPM_TIS "tpm-tis"
> > > #define TYPE_TPM_CRB "tpm-crb"
> > > +#define TYPE_TPM_SPAPR "tpm-spapr"
> > > #define TPM_IS_TIS(chr) \
> > > object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS)
> > > #define TPM_IS_CRB(chr) \
> > > object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
> > > +#define TPM_IS_SPAPR(chr) \
> > > + object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)
> > > /* returns NULL unless there is exactly one TPM device */
> > > static inline TPMIf *tpm_find(void)
> > > diff --git a/qapi/tpm.json b/qapi/tpm.json
> > > index b30323bb6b..63878aa0f4 100644
> > > --- a/qapi/tpm.json
> > > +++ b/qapi/tpm.json
> > > @@ -12,11 +12,11 @@
> > > #
> > > # @tpm-tis: TPM TIS model
> > > # @tpm-crb: TPM CRB model (since 2.12)
> > > +# @tpm-spapr: TPM SPAPR model (since 5.0)
> > > #
> > > # Since: 1.5
> > > ##
> > > -{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb' ] }
> > > -
> > > +{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb', 'tpm-spapr' ] }
> > > ##
> > > # @query-tpm-models:
> > > #
> > > @@ -29,7 +29,7 @@
> > > # Example:
> > > #
> > > # -> { "execute": "query-tpm-models" }
> > > -# <- { "return": [ "tpm-tis", "tpm-crb" ] }
> > > +# <- { "return": [ "tpm-tis", "tpm-crb", "tpm-spapr" ] }
> > > #
> > > ##
> > > { 'command': 'query-tpm-models', 'returns': ['TpmModel'] }
>
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 3/5] tpm_spapr: Support suspend and resume
2019-12-13 12:46 ` Stefan Berger
@ 2019-12-17 0:53 ` David Gibson
0 siblings, 0 replies; 19+ messages in thread
From: David Gibson @ 2019-12-17 0:53 UTC (permalink / raw)
To: Stefan Berger; +Cc: marcandre.lureau, qemu-ppc, qemu-devel, Stefan Berger
[-- Attachment #1: Type: text/plain, Size: 5535 bytes --]
On Fri, Dec 13, 2019 at 07:46:44AM -0500, Stefan Berger wrote:
> On 12/13/19 12:39 AM, David Gibson wrote:
> > On Thu, Dec 12, 2019 at 03:24:28PM -0500, Stefan Berger wrote:
> > > Extend the tpm_spapr frontend with VM suspend and resume support.
> > >
> > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > >
> > > diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
> > > index c4a67e2403..8f5a142bd4 100644
> > > --- a/hw/tpm/tpm_spapr.c
> > > +++ b/hw/tpm/tpm_spapr.c
> > > @@ -87,6 +87,8 @@ typedef struct {
> > > TPMVersion be_tpm_version;
> > > size_t be_buffer_size;
> > > +
> > > + bool deliver_response; /* whether to deliver response after VM resume */
> > > } SPAPRvTPMState;
> > > static void tpm_spapr_show_buffer(const unsigned char *buffer,
> > > @@ -256,6 +258,12 @@ static void tpm_spapr_request_completed(TPMIf *ti, int ret)
> > > uint32_t len;
> > > int rc;
> > > + if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
> > I'm trying to figure out the circumstances in which
> > request_completed() would get called before post_load on the
> > destination.
>
> This is on the source side where we must not deliver the response in case
> the devices are now suspending but defer the delivery to after the
> resume.
Ah, I see. But in that case, AFAICT this means we've received the
completion when we're in the last stages of migration, which means
it's entirely possible we've already transferred the vtpm device's
state. So there's no guarantee that either the deliver_response
change here, or the response buffer will actually make it to the other
side.
> > > + /* defer delivery of response until .post_load */
> > > + s->deliver_response |= true;
> > |= is a bitwise OR which is not what you want, although it will
> > *probably* work in practice. Better to just use
> > s->deliver_response = true;
> >
> > > + return;
> > > + }
> > > +
> > > s->state = SPAPR_VTPM_STATE_COMPLETION;
> > > /* a max. of be_buffer_size bytes can be transported */
> > > @@ -316,6 +324,7 @@ static void tpm_spapr_reset(SpaprVioDevice *dev)
> > > SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
> > > s->state = SPAPR_VTPM_STATE_NONE;
> > > + s->deliver_response = false;
> > > s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver);
> > > tpm_spapr_update_deviceclass(dev);
> > > @@ -339,9 +348,53 @@ static enum TPMVersion tpm_spapr_get_version(TPMIf *ti)
> > > return tpm_backend_get_tpm_version(s->be_driver);
> > > }
> > > +/* persistent state handling */
> > > +
> > > +static int tpm_spapr_pre_save(void *opaque)
> > > +{
> > > + SPAPRvTPMState *s = opaque;
> > > +
> > > + s->deliver_response |= tpm_backend_finish_sync(s->be_driver);
> > Same problem here.
> >
> > > + trace_tpm_spapr_pre_save(s->deliver_response);
> > > + /*
> > > + * we cannot deliver the results to the VM since DMA would touch VM memory
> > > + */
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int tpm_spapr_post_load(void *opaque, int version_id)
> > > +{
> > > + SPAPRvTPMState *s = opaque;
> > > +
> > > + if (s->deliver_response) {
> > > + trace_tpm_spapr_post_load();
> > > + /* deliver the results to the VM via DMA */
> > > + tpm_spapr_request_completed(TPM_IF(s), 0);
> > > + s->deliver_response = false;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static const VMStateDescription vmstate_spapr_vtpm = {
> > > .name = "tpm-spapr",
> > > - .unmigratable = 1,
> > > + .version_id = 1,
> > > + .minimum_version_id = 0,
> > > + .minimum_version_id_old = 0,
> > > + .pre_save = tpm_spapr_pre_save,
> > > + .post_load = tpm_spapr_post_load,
> > > + .fields = (VMStateField[]) {
> > > + VMSTATE_SPAPR_VIO(vdev, SPAPRvTPMState),
> > > +
> > > + VMSTATE_UINT8(state, SPAPRvTPMState),
> > > + VMSTATE_BUFFER(buffer, SPAPRvTPMState),
> > Transferring the whole 4kiB buffer unconditionally when it mostly
> > won't have anything useful in it doesn't seem like a great idea.
>
>
> It's really only needed in case of a 'delayed response'. So, yeah, we could
> transfer data in only that case then.
>
>
> >
> > > + /* remember DMA address */
> > > + VMSTATE_UINT32(crq.s.data, SPAPRvTPMState),
> > > + VMSTATE_BOOL(deliver_response, SPAPRvTPMState),
> > > + VMSTATE_END_OF_LIST(),
> > > + }
> > > };
> > > static Property tpm_spapr_properties[] = {
> > > diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> > > index 6278a39618..d109661b96 100644
> > > --- a/hw/tpm/trace-events
> > > +++ b/hw/tpm/trace-events
> > > @@ -67,3 +67,5 @@ tpm_spapr_do_crq_get_version(uint32_t version) "response: version %u"
> > > tpm_spapr_do_crq_prepare_to_suspend(void) "response: preparing to suspend"
> > > tpm_spapr_do_crq_unknown_msg_type(uint8_t type) "Unknown message type 0x%02x"
> > > tpm_spapr_do_crq_unknown_crq(uint8_t raw1, uint8_t raw2) "unknown CRQ 0x%02x 0x%02x ..."
> > > +tpm_spapr_pre_save(bool v) "TPM response to deliver after resume: %d"
> > > +tpm_spapr_post_load(void) "Delivering TPM response after resume"
>
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
2019-12-17 0:29 ` David Gibson
@ 2019-12-17 19:44 ` Stefan Berger
2019-12-19 1:54 ` David Gibson
0 siblings, 1 reply; 19+ messages in thread
From: Stefan Berger @ 2019-12-17 19:44 UTC (permalink / raw)
To: David Gibson; +Cc: marcandre.lureau, qemu-ppc, qemu-devel, Stefan Berger
On 12/16/19 7:29 PM, David Gibson wrote:
> On Fri, Dec 13, 2019 at 08:03:36AM -0500, Stefan Berger wrote:
>> On 12/13/19 12:34 AM, David Gibson wrote:
>>
>> The existing one looks like this:
>>
>> typedef struct SpaprVioCrq {
>> uint64_t qladdr;
>> uint32_t qsize;
>> uint32_t qnext;
>> int(*SendFunc)(struct SpaprVioDevice *vdev, uint8_t *crq);
>> } SpaprVioCrq;
>>
>> I don't seem to find the fields there that we need for vTPM support.
> Yeah, I can see the difference in the structures. What I'm after is
> what is the difference in purpose which means they have different
> content.
>
> Having read through the whole series now, I *think* the answer is that
> the tpm specific structure is one entry in the request queue for the
> vtpm, whereas the VioCrq structure is a handle on an entire queue.
>
> I think the tpm one needs a rename to reflect that a) it's vtpm
> specific and b) it's not actually a queue, just part of it.
v6 has it as TpmCrq. It's local to the file, so from that perspective
it's specific to (v)TPM.
>> This is a 1:1 copy from the existing TIS driver.
> Hm, right. Probably not a bad idea to move that out as a helper
> function then.
In V7 then.
>>>> +static void tpm_spapr_update_deviceclass(SpaprVioDevice *dev)
>>>> +{
>>>> + SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
>>>> + SpaprVioDeviceClass *k = VIO_SPAPR_DEVICE_GET_CLASS(dev);
>>>> +
>>>> + switch (s->be_tpm_version) {
>>>> + case TPM_VERSION_UNSPEC:
>>>> + assert(false);
>>>> + break;
>>>> + case TPM_VERSION_1_2:
>>>> + k->dt_name = "vtpm";
>>>> + k->dt_type = "IBM,vtpm";
>>>> + k->dt_compatible = "IBM,vtpm";
>>>> + break;
>>>> + case TPM_VERSION_2_0:
>>>> + k->dt_name = "vtpm";
>>>> + k->dt_type = "IBM,vtpm";
>>>> + k->dt_compatible = "IBM,vtpm20";
>>>> + break;
>>> Erk. Updating DeviceClass structures on the fly is hideously ugly.
>>> We might need to take a different approach for this.
>> Make a suggestion... Obviously, we can hard-initialize dt_name and dt_type
>> but dt_compatible can only be set after we have determined the version of
>> TPM.
> As you say name and type can just be put into the class statically.
I did this in v6.
> Since you need to change compatible based on an internal variable,
> we'd need to replace the static dt_compatible in the class with a
> callback.
Why can we not initialize it once we know the version of TPM? From the
perspective of SLOF at least this seems to be building the device tree
fine since it sees the proper value...
Stefan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
2019-12-17 19:44 ` Stefan Berger
@ 2019-12-19 1:54 ` David Gibson
2019-12-19 1:59 ` Stefan Berger
0 siblings, 1 reply; 19+ messages in thread
From: David Gibson @ 2019-12-19 1:54 UTC (permalink / raw)
To: Stefan Berger; +Cc: marcandre.lureau, qemu-ppc, qemu-devel, Stefan Berger
[-- Attachment #1: Type: text/plain, Size: 3604 bytes --]
On Tue, Dec 17, 2019 at 02:44:04PM -0500, Stefan Berger wrote:
> On 12/16/19 7:29 PM, David Gibson wrote:
> > On Fri, Dec 13, 2019 at 08:03:36AM -0500, Stefan Berger wrote:
> > > On 12/13/19 12:34 AM, David Gibson wrote:
> > >
> > > The existing one looks like this:
> > >
> > > typedef struct SpaprVioCrq {
> > > uint64_t qladdr;
> > > uint32_t qsize;
> > > uint32_t qnext;
> > > int(*SendFunc)(struct SpaprVioDevice *vdev, uint8_t *crq);
> > > } SpaprVioCrq;
> > >
> > > I don't seem to find the fields there that we need for vTPM support.
> > Yeah, I can see the difference in the structures. What I'm after is
> > what is the difference in purpose which means they have different
> > content.
> >
> > Having read through the whole series now, I *think* the answer is that
> > the tpm specific structure is one entry in the request queue for the
> > vtpm, whereas the VioCrq structure is a handle on an entire queue.
> >
> > I think the tpm one needs a rename to reflect that a) it's vtpm
> > specific and b) it's not actually a queue, just part of it.
>
>
> v6 has it as TpmCrq. It's local to the file, so from that perspective it's
> specific to (v)TPM.
Ok.
> > > This is a 1:1 copy from the existing TIS driver.
> > Hm, right. Probably not a bad idea to move that out as a helper
> > function then.
>
>
> In V7 then.
Ok.
> > > > > +static void tpm_spapr_update_deviceclass(SpaprVioDevice *dev)
> > > > > +{
> > > > > + SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
> > > > > + SpaprVioDeviceClass *k = VIO_SPAPR_DEVICE_GET_CLASS(dev);
> > > > > +
> > > > > + switch (s->be_tpm_version) {
> > > > > + case TPM_VERSION_UNSPEC:
> > > > > + assert(false);
> > > > > + break;
> > > > > + case TPM_VERSION_1_2:
> > > > > + k->dt_name = "vtpm";
> > > > > + k->dt_type = "IBM,vtpm";
> > > > > + k->dt_compatible = "IBM,vtpm";
> > > > > + break;
> > > > > + case TPM_VERSION_2_0:
> > > > > + k->dt_name = "vtpm";
> > > > > + k->dt_type = "IBM,vtpm";
> > > > > + k->dt_compatible = "IBM,vtpm20";
> > > > > + break;
> > > > Erk. Updating DeviceClass structures on the fly is hideously ugly.
> > > > We might need to take a different approach for this.
> > > Make a suggestion... Obviously, we can hard-initialize dt_name and dt_type
> > > but dt_compatible can only be set after we have determined the version of
> > > TPM.
> > As you say name and type can just be put into the class statically.
>
>
> I did this in v6.
>
>
> > Since you need to change compatible based on an internal variable,
> > we'd need to replace the static dt_compatible in the class with a
> > callback.
>
>
> Why can we not initialize it once we know the version of TPM? From the
> perspective of SLOF at least this seems to be building the device tree fine
> since it sees the proper value...
Because it's a serious layering / isolation violation. You're
modifying QOM type information from the runtime code of a specific
instance. You get away with it (now) because there's only one
instance and the ordering of things happens to let it work, but that's
assuming way too much about QOM's implementation details.
As a rule, once the QOM classes are set up with their class_init
function, they should never be modified.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
2019-12-19 1:54 ` David Gibson
@ 2019-12-19 1:59 ` Stefan Berger
2019-12-19 5:13 ` David Gibson
0 siblings, 1 reply; 19+ messages in thread
From: Stefan Berger @ 2019-12-19 1:59 UTC (permalink / raw)
To: David Gibson; +Cc: marcandre.lureau, qemu-ppc, qemu-devel, Stefan Berger
On 12/18/19 8:54 PM, David Gibson wrote:
> On Tue, Dec 17, 2019 at 02:44:04PM -0500, Stefan Berger wrote:
>> On 12/16/19 7:29 PM, David Gibson wrote:
>>
>>
>>> Since you need to change compatible based on an internal variable,
>>> we'd need to replace the static dt_compatible in the class with a
>>> callback.
>>
>> Why can we not initialize it once we know the version of TPM? From the
>> perspective of SLOF at least this seems to be building the device tree fine
>> since it sees the proper value...
> Because it's a serious layering / isolation violation. You're
> modifying QOM type information from the runtime code of a specific
> instance. You get away with it (now) because there's only one
> instance and the ordering of things happens to let it work, but that's
> assuming way too much about QOM's implementation details.
>
> As a rule, once the QOM classes are set up with their class_init
> function, they should never be modified.
If we now add a get_dt_compatible() callback to the class that gets
invoked when dt_compatible is NULL, does this then solve the issue?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
2019-12-19 1:59 ` Stefan Berger
@ 2019-12-19 5:13 ` David Gibson
2019-12-19 5:14 ` David Gibson
0 siblings, 1 reply; 19+ messages in thread
From: David Gibson @ 2019-12-19 5:13 UTC (permalink / raw)
To: Stefan Berger; +Cc: marcandre.lureau, qemu-ppc, qemu-devel, Stefan Berger
[-- Attachment #1: Type: text/plain, Size: 1471 bytes --]
On Wed, Dec 18, 2019 at 08:59:18PM -0500, Stefan Berger wrote:
> On 12/18/19 8:54 PM, David Gibson wrote:
> > On Tue, Dec 17, 2019 at 02:44:04PM -0500, Stefan Berger wrote:
> > > On 12/16/19 7:29 PM, David Gibson wrote:
> > >
> > >
> > > > Since you need to change compatible based on an internal variable,
> > > > we'd need to replace the static dt_compatible in the class with a
> > > > callback.
> > >
> > > Why can we not initialize it once we know the version of TPM? From the
> > > perspective of SLOF at least this seems to be building the device tree fine
> > > since it sees the proper value...
> > Because it's a serious layering / isolation violation. You're
> > modifying QOM type information from the runtime code of a specific
> > instance. You get away with it (now) because there's only one
> > instance and the ordering of things happens to let it work, but that's
> > assuming way too much about QOM's implementation details.
> >
> > As a rule, once the QOM classes are set up with their class_init
> > function, they should never be modified.
>
>
> If we now add a get_dt_compatible() callback to the class that gets invoked
> when dt_compatible is NULL, does this then solve the issue?
Yes, that's what I'm suggesting.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface
2019-12-19 5:13 ` David Gibson
@ 2019-12-19 5:14 ` David Gibson
0 siblings, 0 replies; 19+ messages in thread
From: David Gibson @ 2019-12-19 5:14 UTC (permalink / raw)
To: Stefan Berger; +Cc: marcandre.lureau, qemu-ppc, qemu-devel, Stefan Berger
[-- Attachment #1: Type: text/plain, Size: 1727 bytes --]
On Thu, Dec 19, 2019 at 04:13:57PM +1100, David Gibson wrote:
> On Wed, Dec 18, 2019 at 08:59:18PM -0500, Stefan Berger wrote:
> > On 12/18/19 8:54 PM, David Gibson wrote:
> > > On Tue, Dec 17, 2019 at 02:44:04PM -0500, Stefan Berger wrote:
> > > > On 12/16/19 7:29 PM, David Gibson wrote:
> > > >
> > > >
> > > > > Since you need to change compatible based on an internal variable,
> > > > > we'd need to replace the static dt_compatible in the class with a
> > > > > callback.
> > > >
> > > > Why can we not initialize it once we know the version of TPM? From the
> > > > perspective of SLOF at least this seems to be building the device tree fine
> > > > since it sees the proper value...
> > > Because it's a serious layering / isolation violation. You're
> > > modifying QOM type information from the runtime code of a specific
> > > instance. You get away with it (now) because there's only one
> > > instance and the ordering of things happens to let it work, but that's
> > > assuming way too much about QOM's implementation details.
> > >
> > > As a rule, once the QOM classes are set up with their class_init
> > > function, they should never be modified.
> >
> >
> > If we now add a get_dt_compatible() callback to the class that gets invoked
> > when dt_compatible is NULL, does this then solve the issue?
>
> Yes, that's what I'm suggesting.
Well, almost. Actually I'd suggest the other way around - call the
callback method, but if that's NULL, fallback to the static value.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2019-12-19 5:16 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-12 20:24 [PATCH v5 0/5] Add vTPM emulator support for ppc64 platform Stefan Berger
2019-12-12 20:24 ` [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface Stefan Berger
2019-12-12 20:33 ` Eric Blake
2019-12-12 20:34 ` Stefan Berger
2019-12-13 5:34 ` David Gibson
2019-12-13 13:03 ` Stefan Berger
2019-12-17 0:29 ` David Gibson
2019-12-17 19:44 ` Stefan Berger
2019-12-19 1:54 ` David Gibson
2019-12-19 1:59 ` Stefan Berger
2019-12-19 5:13 ` David Gibson
2019-12-19 5:14 ` David Gibson
2019-12-12 20:24 ` [PATCH v5 2/5] tpm: Return bool from tpm_backend_finish_sync Stefan Berger
2019-12-12 20:24 ` [PATCH v5 3/5] tpm_spapr: Support suspend and resume Stefan Berger
2019-12-13 5:39 ` David Gibson
2019-12-13 12:46 ` Stefan Berger
2019-12-17 0:53 ` David Gibson
2019-12-12 20:24 ` [PATCH v5 4/5] hw/ppc/Kconfig: Enable TPM_SPAPR as part of PSERIES config Stefan Berger
2019-12-12 20:24 ` [PATCH v5 5/5] docs: tpm: Add example command line for ppc64 and tpm-spapr Stefan Berger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).