* [Qemu-devel] [RFC PATCH 0/2] spapr: Implement H_TPM_COMM for accessing host TPM device
@ 2019-07-12 1:19 Michael Roth
2019-07-12 1:19 ` [Qemu-devel] [PATCH 1/2] docs/specs: initial spec summary for Ultravisor-related hcalls Michael Roth
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Michael Roth @ 2019-07-12 1:19 UTC (permalink / raw)
To: qemu-devel; +Cc: linuxram, qemu-ppc, david
These patches are also available at:
https://github.com/mdroth/qemu/commits/spapr-tpm-hcall-v0
This patchset implements the H_TPM_COMM hypercall, which provides a way
for an Ultravisor to pass raw TPM commands on to a host's TPM device,
either directly or through a TPM Resource Manager (needed to support
multiple guests).
Secure Guests running on an Ultravisor have a symmetric key that is
encrypted using a public key that is bound to a trusted host's TPM
hardware. This hypercall provides a means to decrypt the symmetric
key on behalf of a Secure Guest using the host's TPM hardware.
More details are provided in the spec summary introduced in patch 1.
docs/specs/ppc-spapr-uv-hcalls.txt | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
hw/ppc/Makefile.objs | 1 +
hw/ppc/spapr.c | 27 +++++++++++++++++++++++++++
hw/ppc/spapr_hcall_tpm.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
hw/ppc/trace-events | 4 ++++
include/hw/ppc/spapr.h | 7 ++++++-
6 files changed, 247 insertions(+), 1 deletion(-)
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 1/2] docs/specs: initial spec summary for Ultravisor-related hcalls
2019-07-12 1:19 [Qemu-devel] [RFC PATCH 0/2] spapr: Implement H_TPM_COMM for accessing host TPM device Michael Roth
@ 2019-07-12 1:19 ` Michael Roth
2019-07-12 6:40 ` David Gibson
2019-07-12 1:19 ` [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall Michael Roth
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Michael Roth @ 2019-07-12 1:19 UTC (permalink / raw)
To: qemu-devel; +Cc: linuxram, qemu-ppc, david
For now this only covers hcalls relating to TPM communication since
it's the only one particularly important from a QEMU perspective atm,
but others can be added here where it makes sense.
The full specification for all hcalls/ucalls will eventually be made
available in the public/OpenPower version of the PAPR specification.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
docs/specs/ppc-spapr-uv-hcalls.txt | 74 ++++++++++++++++++++++++++++++
1 file changed, 74 insertions(+)
create mode 100644 docs/specs/ppc-spapr-uv-hcalls.txt
diff --git a/docs/specs/ppc-spapr-uv-hcalls.txt b/docs/specs/ppc-spapr-uv-hcalls.txt
new file mode 100644
index 0000000000..0278f89190
--- /dev/null
+++ b/docs/specs/ppc-spapr-uv-hcalls.txt
@@ -0,0 +1,74 @@
+On PPC64 systems supporting Protected Execution Facility (PEF), system
+memory can be placed in a secured region where only an "ultravisor"
+running in firmware can provide to access it. pseries guests on such
+systems can communicate with the ultravisor (via ultracalls) to switch to a
+secure VM mode (SVM) where the guest's memory is relocated to this secured
+region, making its memory inaccessible to normal processes/guests running on
+the host.
+
+The various ultracalls/hypercalls relating to SVM mode are currently
+only documented internally, but are planned for direct inclusion into the
+public OpenPOWER version of the PAPR specification (LoPAPR/LoPAR). An internal
+ACR has been filed to reserve a hypercall number range specific to this
+use-case to avoid any future conflicts with the internally-maintained PAPR
+specification. This document summarizes some of these details as they relate
+to QEMU.
+
+== hypercalls needed by the ultravisor ==
+
+Switching to SVM mode involves a number of hcalls issued by the ultravisor
+to the hypervisor to orchestrate the movement of guest memory to secure
+memory and various other aspects SVM mode. The below documents the hcalls
+relevant to QEMU.
+
+- H_TPM_COMM (0xef10)
+
+ For TPM_COMM_OP_EXECUTE operation:
+ Send a request to a TPM and receive a response, opening a new TPM session
+ if one has not already been opened.
+
+ For TPM_COMM_OP_CLOSE_SESSION operation:
+ Close the existing TPM session, if any.
+
+ Arguments:
+
+ r3 : H_TPM_COMM (0xef10)
+ r4 : TPM operation, one of:
+ TPM_COMM_OP_EXECUTE (0x1)
+ TPM_COMM_OP_CLOSE_SESSION (0x2)
+ r5 : in_buffer, guest physical address of buffer containing the request
+ - Caller may use the same address for both request and response
+ r6 : in_size, size of the in buffer, must
+ - Must be less than or equal to 4KB
+ r7 : out_buffer, guest physical address of buffer to store the response
+ - Caller may use the same address for both request and response
+ r8 : out_size, size of the out buffer
+ - Must be at least 4KB, as this is the maximum request/response size
+ supported by most TPM implementations, including the TPM Resource
+ Manager in the linux kernel.
+
+ Return values:
+
+ r3 : H_Success request processed successfully
+ H_PARAMETER invalid TPM operation
+ H_P2 in_buffer is invalid
+ H_P3 in_size is invalid
+ H_P4 out_buffer is invalid
+ H_P5 out_size is invalid
+ H_RESOURCE TPM is unavailable
+ r4 : For TPM_COMM_OP_EXECUTE, the size of the response will be stored here
+ upon success.
+
+ Use-case/notes:
+
+ SVM filesystems are encrypted using a symmetric key. This key is then
+ wrapped/encrypted using the public key of a trusted system which has the
+ private key stored in the system's TPM. An Ultravisor will use this
+ hcall to unwrap/unseal the symmetric key using the system's TPM device
+ or a TPM Resource Manager associated with the device.
+
+ The Ultravisor sets up a separate session key with the TPM in advance
+ during host system boot. All sensitive in and out values will be
+ encrypted using the session key. Though the hypervisor will see the 'in'
+ and 'out' buffers in raw form, any sensitive contents will generally be
+ encrypted using this session key.
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall
2019-07-12 1:19 [Qemu-devel] [RFC PATCH 0/2] spapr: Implement H_TPM_COMM for accessing host TPM device Michael Roth
2019-07-12 1:19 ` [Qemu-devel] [PATCH 1/2] docs/specs: initial spec summary for Ultravisor-related hcalls Michael Roth
@ 2019-07-12 1:19 ` Michael Roth
2019-07-12 6:46 ` David Gibson
2019-07-12 6:40 ` [Qemu-devel] [RFC PATCH 0/2] spapr: Implement H_TPM_COMM for accessing host TPM device David Gibson
2019-07-12 15:33 ` no-reply
3 siblings, 1 reply; 16+ messages in thread
From: Michael Roth @ 2019-07-12 1:19 UTC (permalink / raw)
To: qemu-devel; +Cc: linuxram, qemu-ppc, david
This implements the H_TPM_COMM hypercall, which is used by an
Ultravisor to pass TPM commands directly to the host's TPM device, or
a TPM Resource Manager associated with the device.
This also introduces a new pseries machine option which is used to
configure what TPM device to pass commands to, for example:
-machine pseries,...,tpm-device-file=/dev/tmprm0
By default, no tpm-device-file is defined and hcalls will return
H_RESOURCE.
The full specification for this hypercall can be found in
docs/specs/ppc-spapr-uv-hcalls.txt
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com
---
hw/ppc/Makefile.objs | 1 +
hw/ppc/spapr.c | 27 ++++++++
hw/ppc/spapr_hcall_tpm.c | 135 +++++++++++++++++++++++++++++++++++++++
hw/ppc/trace-events | 4 ++
include/hw/ppc/spapr.h | 7 +-
5 files changed, 173 insertions(+), 1 deletion(-)
create mode 100644 hw/ppc/spapr_hcall_tpm.c
diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index 9da93af905..5aa120cae6 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -5,6 +5,7 @@ obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o
obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
+obj-$(CONFIG_PSERIES) += spapr_hcall_tpm.o
obj-$(CONFIG_SPAPR_RNG) += spapr_rng.o
# IBM PowerNV
obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o pnv_occ.o pnv_bmc.o
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 821f0d4a49..eb3421673b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1776,6 +1776,10 @@ static void spapr_machine_reset(MachineState *machine)
*/
object_child_foreach_recursive(object_get_root(), spapr_reset_drcs, NULL);
+ if (spapr->tpm_device_file) {
+ spapr_hcall_tpm_reset();
+ }
+
spapr_clear_pending_events(spapr);
/*
@@ -3340,6 +3344,21 @@ static void spapr_set_host_serial(Object *obj, const char *value, Error **errp)
spapr->host_serial = g_strdup(value);
}
+static char *spapr_get_tpm_device_file(Object *obj, Error **errp)
+{
+ SpaprMachineState *spapr = SPAPR_MACHINE(obj);
+
+ return g_strdup(spapr->tpm_device_file);
+}
+
+static void spapr_set_tpm_device_file(Object *obj, const char *value, Error **errp)
+{
+ SpaprMachineState *spapr = SPAPR_MACHINE(obj);
+
+ g_free(spapr->tpm_device_file);
+ spapr->tpm_device_file = g_strdup(value);
+}
+
static void spapr_instance_init(Object *obj)
{
SpaprMachineState *spapr = SPAPR_MACHINE(obj);
@@ -3396,6 +3415,14 @@ static void spapr_instance_init(Object *obj)
&error_abort);
object_property_set_description(obj, "host-serial",
"Host serial number to advertise in guest device tree", &error_abort);
+ object_property_add_str(obj, "tpm-device-file",
+ spapr_get_tpm_device_file,
+ spapr_set_tpm_device_file, &error_abort);
+ object_property_set_description(obj, "tpm-device-file",
+ "Specifies the path to the TPM character device file to use"
+ " for TPM communication via hypercalls (usually a TPM"
+ " resource manager)",
+ &error_abort);
}
static void spapr_machine_finalizefn(Object *obj)
diff --git a/hw/ppc/spapr_hcall_tpm.c b/hw/ppc/spapr_hcall_tpm.c
new file mode 100644
index 0000000000..75e2b6d594
--- /dev/null
+++ b/hw/ppc/spapr_hcall_tpm.c
@@ -0,0 +1,135 @@
+/*
+ * SPAPR TPM Hypercall
+ *
+ * Copyright IBM Corp. 2019
+ *
+ * Authors:
+ * Michael Roth <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "cpu.h"
+#include "hw/ppc/spapr.h"
+#include "trace.h"
+
+#define TPM_SPAPR_BUFSIZE 4096
+
+enum {
+ TPM_COMM_OP_EXECUTE = 1,
+ TPM_COMM_OP_CLOSE_SESSION = 2,
+};
+
+static int tpm_devfd = -1;
+
+static ssize_t tpm_execute(SpaprMachineState *spapr, target_ulong *args)
+{
+ uint64_t data_in = ppc64_phys_to_real(args[1]);
+ target_ulong data_in_size = args[2];
+ uint64_t data_out = ppc64_phys_to_real(args[3]);
+ target_ulong data_out_size = args[4];
+ uint8_t buf_in[TPM_SPAPR_BUFSIZE];
+ uint8_t buf_out[TPM_SPAPR_BUFSIZE];
+ ssize_t ret;
+
+ trace_spapr_tpm_execute(data_in, data_in_size, data_out, data_out_size);
+
+ if (data_in_size > TPM_SPAPR_BUFSIZE) {
+ error_report("invalid TPM input buffer size: " TARGET_FMT_lu "\n",
+ data_in_size);
+ return H_P3;
+ }
+
+ if (data_out_size < TPM_SPAPR_BUFSIZE) {
+ error_report("invalid TPM output buffer size: " TARGET_FMT_lu "\n",
+ data_out_size);
+ return H_P5;
+ }
+
+ if (tpm_devfd == -1) {
+ tpm_devfd = open(spapr->tpm_device_file, O_RDWR);
+ if (tpm_devfd == -1) {
+ error_report("failed to open TPM device %s: %d",
+ spapr->tpm_device_file, errno);
+ return H_RESOURCE;
+ }
+ }
+
+ cpu_physical_memory_read(data_in, buf_in, data_in_size);
+
+ do {
+ ret = write(tpm_devfd, buf_in, data_in_size);
+ if (ret > 0) {
+ data_in_size -= ret;
+ }
+ } while ((ret >= 0 && data_in_size > 0) || (ret == -1 && errno == EINTR));
+
+ if (ret == -1) {
+ error_report("failed to write to TPM device %s: %d",
+ spapr->tpm_device_file, errno);
+ return H_RESOURCE;
+ }
+
+ do {
+ ret = read(tpm_devfd, buf_out, data_out_size);
+ } while (ret == 0 || (ret == -1 && errno == EINTR));
+
+ if (ret == -1) {
+ error_report("failed to read from TPM device %s: %d",
+ spapr->tpm_device_file, errno);
+ return H_RESOURCE;
+ }
+
+ cpu_physical_memory_write(data_out, buf_out, ret);
+ args[0] = ret;
+
+ return H_SUCCESS;
+}
+
+static target_ulong h_tpm_comm(PowerPCCPU *cpu,
+ SpaprMachineState *spapr,
+ target_ulong opcode,
+ target_ulong *args)
+{
+ target_ulong op = args[0];
+
+ trace_spapr_h_tpm_comm(spapr->tpm_device_file ?: "null", op);
+
+ if (!spapr->tpm_device_file) {
+ error_report("TPM device not specified");
+ return H_RESOURCE;
+ }
+
+ switch (op) {
+ case TPM_COMM_OP_EXECUTE:
+ return tpm_execute(spapr, args);
+ case TPM_COMM_OP_CLOSE_SESSION:
+ if (tpm_devfd != -1) {
+ close(tpm_devfd);
+ tpm_devfd = -1;
+ }
+ return H_SUCCESS;
+ default:
+ return H_PARAMETER;
+ }
+}
+
+void spapr_hcall_tpm_reset(void)
+{
+ if (tpm_devfd != -1) {
+ close(tpm_devfd);
+ tpm_devfd = -1;
+ }
+}
+
+static void hypercall_register_types(void)
+{
+ spapr_register_hypercall(H_TPM_COMM, h_tpm_comm);
+}
+
+type_init(hypercall_register_types)
diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
index f76448f532..96dad767a1 100644
--- a/hw/ppc/trace-events
+++ b/hw/ppc/trace-events
@@ -25,6 +25,10 @@ spapr_update_dt(unsigned cb) "New blob %u bytes"
spapr_update_dt_failed_size(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
spapr_update_dt_failed_check(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
+# spapr_hcall_tpm.c
+spapr_h_tpm_comm(const char *device_path, uint64_t operation) "tpm_device_path=%s operation=0x%"PRIu64
+spapr_tpm_execute(uint64_t data_in, uint64_t data_in_sz, uint64_t data_out, uint64_t data_out_sz) "data_in=0x%"PRIx64", data_in_sz=%"PRIu64", data_out=0x%"PRIx64", data_out_sz=%"PRIu64
+
# spapr_iommu.c
spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
spapr_iommu_get(uint64_t liobn, uint64_t ioba, uint64_t ret, uint64_t tce) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" ret=%"PRId64" tce=0x%"PRIx64
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 60553d32c4..7bd47575d7 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -198,6 +198,7 @@ struct SpaprMachineState {
SpaprXive *xive;
SpaprIrq *irq;
qemu_irq *qirqs;
+ char *tpm_device_file;
bool cmd_line_caps[SPAPR_CAP_NUM];
SpaprCapabilities def, eff, mig;
@@ -490,8 +491,9 @@ struct SpaprMachineState {
#define H_INT_ESB 0x3C8
#define H_INT_SYNC 0x3CC
#define H_INT_RESET 0x3D0
+#define H_TPM_COMM 0xEF10
-#define MAX_HCALL_OPCODE H_INT_RESET
+#define MAX_HCALL_OPCODE H_TPM_COMM
/* The hcalls above are standardized in PAPR and implemented by pHyp
* as well.
@@ -864,6 +866,9 @@ int spapr_caps_post_migration(SpaprMachineState *spapr);
void spapr_check_pagesize(SpaprMachineState *spapr, hwaddr pagesize,
Error **errp);
+
+void spapr_hcall_tpm_reset(void);
+
/*
* XIVE definitions
*/
--
2.17.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] docs/specs: initial spec summary for Ultravisor-related hcalls
2019-07-12 1:19 ` [Qemu-devel] [PATCH 1/2] docs/specs: initial spec summary for Ultravisor-related hcalls Michael Roth
@ 2019-07-12 6:40 ` David Gibson
2019-07-12 15:13 ` Michael Roth
0 siblings, 1 reply; 16+ messages in thread
From: David Gibson @ 2019-07-12 6:40 UTC (permalink / raw)
To: Michael Roth; +Cc: linuxram, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4874 bytes --]
On Thu, Jul 11, 2019 at 08:19:33PM -0500, Michael Roth wrote:
> For now this only covers hcalls relating to TPM communication since
> it's the only one particularly important from a QEMU perspective atm,
> but others can be added here where it makes sense.
>
> The full specification for all hcalls/ucalls will eventually be made
> available in the public/OpenPower version of the PAPR specification.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Thanks for adding this documentation. Is there a PAPR extension
proposal which covers this, which we could cite as the source?
> ---
> docs/specs/ppc-spapr-uv-hcalls.txt | 74 ++++++++++++++++++++++++++++++
> 1 file changed, 74 insertions(+)
> create mode 100644 docs/specs/ppc-spapr-uv-hcalls.txt
>
> diff --git a/docs/specs/ppc-spapr-uv-hcalls.txt b/docs/specs/ppc-spapr-uv-hcalls.txt
> new file mode 100644
> index 0000000000..0278f89190
> --- /dev/null
> +++ b/docs/specs/ppc-spapr-uv-hcalls.txt
> @@ -0,0 +1,74 @@
> +On PPC64 systems supporting Protected Execution Facility (PEF), system
> +memory can be placed in a secured region where only an "ultravisor"
> +running in firmware can provide to access it. pseries guests on such
> +systems can communicate with the ultravisor (via ultracalls) to switch to a
> +secure VM mode (SVM) where the guest's memory is relocated to this secured
> +region, making its memory inaccessible to normal processes/guests running on
> +the host.
> +
> +The various ultracalls/hypercalls relating to SVM mode are currently
> +only documented internally, but are planned for direct inclusion into the
> +public OpenPOWER version of the PAPR specification (LoPAPR/LoPAR). An internal
> +ACR has been filed to reserve a hypercall number range specific to this
> +use-case to avoid any future conflicts with the internally-maintained PAPR
> +specification. This document summarizes some of these details as they relate
> +to QEMU.
> +
> +== hypercalls needed by the ultravisor ==
> +
> +Switching to SVM mode involves a number of hcalls issued by the ultravisor
> +to the hypervisor to orchestrate the movement of guest memory to secure
> +memory and various other aspects SVM mode. The below documents the hcalls
> +relevant to QEMU.
> +
> +- H_TPM_COMM (0xef10)
> +
> + For TPM_COMM_OP_EXECUTE operation:
> + Send a request to a TPM and receive a response, opening a new TPM session
> + if one has not already been opened.
> +
> + For TPM_COMM_OP_CLOSE_SESSION operation:
> + Close the existing TPM session, if any.
> +
> + Arguments:
> +
> + r3 : H_TPM_COMM (0xef10)
> + r4 : TPM operation, one of:
> + TPM_COMM_OP_EXECUTE (0x1)
> + TPM_COMM_OP_CLOSE_SESSION (0x2)
> + r5 : in_buffer, guest physical address of buffer containing the request
> + - Caller may use the same address for both request and response
> + r6 : in_size, size of the in buffer, must
> + - Must be less than or equal to 4KB
> + r7 : out_buffer, guest physical address of buffer to store the response
> + - Caller may use the same address for both request and response
> + r8 : out_size, size of the out buffer
> + - Must be at least 4KB, as this is the maximum request/response size
> + supported by most TPM implementations, including the TPM Resource
> + Manager in the linux kernel.
> +
> + Return values:
> +
> + r3 : H_Success request processed successfully
> + H_PARAMETER invalid TPM operation
> + H_P2 in_buffer is invalid
> + H_P3 in_size is invalid
> + H_P4 out_buffer is invalid
> + H_P5 out_size is invalid
> + H_RESOURCE TPM is unavailable
> + r4 : For TPM_COMM_OP_EXECUTE, the size of the response will be stored here
> + upon success.
> +
> + Use-case/notes:
> +
> + SVM filesystems are encrypted using a symmetric key. This key is then
> + wrapped/encrypted using the public key of a trusted system which has the
> + private key stored in the system's TPM. An Ultravisor will use this
> + hcall to unwrap/unseal the symmetric key using the system's TPM device
> + or a TPM Resource Manager associated with the device.
> +
> + The Ultravisor sets up a separate session key with the TPM in advance
> + during host system boot. All sensitive in and out values will be
> + encrypted using the session key. Though the hypervisor will see the 'in'
> + and 'out' buffers in raw form, any sensitive contents will generally be
> + encrypted using this session key.
--
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] 16+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/2] spapr: Implement H_TPM_COMM for accessing host TPM device
2019-07-12 1:19 [Qemu-devel] [RFC PATCH 0/2] spapr: Implement H_TPM_COMM for accessing host TPM device Michael Roth
2019-07-12 1:19 ` [Qemu-devel] [PATCH 1/2] docs/specs: initial spec summary for Ultravisor-related hcalls Michael Roth
2019-07-12 1:19 ` [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall Michael Roth
@ 2019-07-12 6:40 ` David Gibson
2019-07-12 15:33 ` no-reply
3 siblings, 0 replies; 16+ messages in thread
From: David Gibson @ 2019-07-12 6:40 UTC (permalink / raw)
To: Michael Roth; +Cc: linuxram, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1637 bytes --]
On Thu, Jul 11, 2019 at 08:19:32PM -0500, Michael Roth wrote:
> These patches are also available at:
>
> https://github.com/mdroth/qemu/commits/spapr-tpm-hcall-v0
>
> This patchset implements the H_TPM_COMM hypercall, which provides a way
> for an Ultravisor to pass raw TPM commands on to a host's TPM device,
> either directly or through a TPM Resource Manager (needed to support
> multiple guests).
>
> Secure Guests running on an Ultravisor have a symmetric key that is
> encrypted using a public key that is bound to a trusted host's TPM
> hardware. This hypercall provides a means to decrypt the symmetric
> key on behalf of a Secure Guest using the host's TPM hardware.
>
> More details are provided in the spec summary introduced in patch 1.
This is obviously 4.2 material, other comments on the individual patches.
>
> docs/specs/ppc-spapr-uv-hcalls.txt | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/ppc/Makefile.objs | 1 +
> hw/ppc/spapr.c | 27 +++++++++++++++++++++++++++
> hw/ppc/spapr_hcall_tpm.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/ppc/trace-events | 4 ++++
> include/hw/ppc/spapr.h | 7 ++++++-
> 6 files changed, 247 insertions(+), 1 deletion(-)
>
>
--
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] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall
2019-07-12 1:19 ` [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall Michael Roth
@ 2019-07-12 6:46 ` David Gibson
2019-07-12 14:34 ` Michael Roth
0 siblings, 1 reply; 16+ messages in thread
From: David Gibson @ 2019-07-12 6:46 UTC (permalink / raw)
To: Michael Roth; +Cc: linuxram, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 10719 bytes --]
On Thu, Jul 11, 2019 at 08:19:34PM -0500, Michael Roth wrote:
> This implements the H_TPM_COMM hypercall, which is used by an
> Ultravisor to pass TPM commands directly to the host's TPM device, or
> a TPM Resource Manager associated with the device.
>
> This also introduces a new pseries machine option which is used to
> configure what TPM device to pass commands to, for example:
>
> -machine pseries,...,tpm-device-file=/dev/tmprm0
Bolting this into yet another machine parameter seems kind of ugly.
Wouldn't it make more sense to treat this as an virtual device (say
"spapr-vtpm"). Adding that device would enable the hcall, and would
have properties for the back end host device.
> By default, no tpm-device-file is defined and hcalls will return
> H_RESOURCE.
Wouldn't H_FUNCTION make more sense?
>
> The full specification for this hypercall can be found in
> docs/specs/ppc-spapr-uv-hcalls.txt
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com
> ---
> hw/ppc/Makefile.objs | 1 +
> hw/ppc/spapr.c | 27 ++++++++
> hw/ppc/spapr_hcall_tpm.c | 135 +++++++++++++++++++++++++++++++++++++++
> hw/ppc/trace-events | 4 ++
> include/hw/ppc/spapr.h | 7 +-
> 5 files changed, 173 insertions(+), 1 deletion(-)
> create mode 100644 hw/ppc/spapr_hcall_tpm.c
>
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index 9da93af905..5aa120cae6 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -5,6 +5,7 @@ obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
> obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o
> obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
> +obj-$(CONFIG_PSERIES) += spapr_hcall_tpm.o
> obj-$(CONFIG_SPAPR_RNG) += spapr_rng.o
> # IBM PowerNV
> obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o pnv_occ.o pnv_bmc.o
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 821f0d4a49..eb3421673b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1776,6 +1776,10 @@ static void spapr_machine_reset(MachineState *machine)
> */
> object_child_foreach_recursive(object_get_root(), spapr_reset_drcs, NULL);
>
> + if (spapr->tpm_device_file) {
> + spapr_hcall_tpm_reset();
> + }
> +
> spapr_clear_pending_events(spapr);
>
> /*
> @@ -3340,6 +3344,21 @@ static void spapr_set_host_serial(Object *obj, const char *value, Error **errp)
> spapr->host_serial = g_strdup(value);
> }
>
> +static char *spapr_get_tpm_device_file(Object *obj, Error **errp)
> +{
> + SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> +
> + return g_strdup(spapr->tpm_device_file);
> +}
> +
> +static void spapr_set_tpm_device_file(Object *obj, const char *value, Error **errp)
> +{
> + SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> +
> + g_free(spapr->tpm_device_file);
> + spapr->tpm_device_file = g_strdup(value);
> +}
> +
> static void spapr_instance_init(Object *obj)
> {
> SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> @@ -3396,6 +3415,14 @@ static void spapr_instance_init(Object *obj)
> &error_abort);
> object_property_set_description(obj, "host-serial",
> "Host serial number to advertise in guest device tree", &error_abort);
> + object_property_add_str(obj, "tpm-device-file",
> + spapr_get_tpm_device_file,
> + spapr_set_tpm_device_file, &error_abort);
> + object_property_set_description(obj, "tpm-device-file",
> + "Specifies the path to the TPM character device file to use"
> + " for TPM communication via hypercalls (usually a TPM"
> + " resource manager)",
> + &error_abort);
> }
>
> static void spapr_machine_finalizefn(Object *obj)
> diff --git a/hw/ppc/spapr_hcall_tpm.c b/hw/ppc/spapr_hcall_tpm.c
> new file mode 100644
> index 0000000000..75e2b6d594
> --- /dev/null
> +++ b/hw/ppc/spapr_hcall_tpm.c
> @@ -0,0 +1,135 @@
> +/*
> + * SPAPR TPM Hypercall
> + *
> + * Copyright IBM Corp. 2019
> + *
> + * Authors:
> + * Michael Roth <mdroth@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "qapi/error.h"
> +#include "qemu/error-report.h"
> +#include "cpu.h"
> +#include "hw/ppc/spapr.h"
> +#include "trace.h"
> +
> +#define TPM_SPAPR_BUFSIZE 4096
> +
> +enum {
> + TPM_COMM_OP_EXECUTE = 1,
> + TPM_COMM_OP_CLOSE_SESSION = 2,
> +};
> +
> +static int tpm_devfd = -1;
A global? Really? You can do better.
> +
> +static ssize_t tpm_execute(SpaprMachineState *spapr, target_ulong *args)
> +{
> + uint64_t data_in = ppc64_phys_to_real(args[1]);
> + target_ulong data_in_size = args[2];
> + uint64_t data_out = ppc64_phys_to_real(args[3]);
> + target_ulong data_out_size = args[4];
> + uint8_t buf_in[TPM_SPAPR_BUFSIZE];
> + uint8_t buf_out[TPM_SPAPR_BUFSIZE];
> + ssize_t ret;
> +
> + trace_spapr_tpm_execute(data_in, data_in_size, data_out, data_out_size);
> +
> + if (data_in_size > TPM_SPAPR_BUFSIZE) {
> + error_report("invalid TPM input buffer size: " TARGET_FMT_lu "\n",
> + data_in_size);
> + return H_P3;
> + }
> +
> + if (data_out_size < TPM_SPAPR_BUFSIZE) {
> + error_report("invalid TPM output buffer size: " TARGET_FMT_lu "\n",
> + data_out_size);
> + return H_P5;
> + }
> +
> + if (tpm_devfd == -1) {
> + tpm_devfd = open(spapr->tpm_device_file, O_RDWR);
> + if (tpm_devfd == -1) {
> + error_report("failed to open TPM device %s: %d",
> + spapr->tpm_device_file, errno);
> + return H_RESOURCE;
> + }
> + }
> +
> + cpu_physical_memory_read(data_in, buf_in, data_in_size);
> +
> + do {
> + ret = write(tpm_devfd, buf_in, data_in_size);
> + if (ret > 0) {
> + data_in_size -= ret;
> + }
> + } while ((ret >= 0 && data_in_size > 0) || (ret == -1 && errno == EINTR));
> +
> + if (ret == -1) {
> + error_report("failed to write to TPM device %s: %d",
> + spapr->tpm_device_file, errno);
> + return H_RESOURCE;
> + }
> +
> + do {
> + ret = read(tpm_devfd, buf_out, data_out_size);
> + } while (ret == 0 || (ret == -1 && errno == EINTR));
> +
> + if (ret == -1) {
> + error_report("failed to read from TPM device %s: %d",
> + spapr->tpm_device_file, errno);
> + return H_RESOURCE;
> + }
> +
> + cpu_physical_memory_write(data_out, buf_out, ret);
> + args[0] = ret;
> +
> + return H_SUCCESS;
> +}
> +
> +static target_ulong h_tpm_comm(PowerPCCPU *cpu,
> + SpaprMachineState *spapr,
> + target_ulong opcode,
> + target_ulong *args)
> +{
> + target_ulong op = args[0];
> +
> + trace_spapr_h_tpm_comm(spapr->tpm_device_file ?: "null", op);
> +
> + if (!spapr->tpm_device_file) {
> + error_report("TPM device not specified");
> + return H_RESOURCE;
> + }
> +
> + switch (op) {
> + case TPM_COMM_OP_EXECUTE:
> + return tpm_execute(spapr, args);
> + case TPM_COMM_OP_CLOSE_SESSION:
> + if (tpm_devfd != -1) {
> + close(tpm_devfd);
> + tpm_devfd = -1;
> + }
> + return H_SUCCESS;
> + default:
> + return H_PARAMETER;
> + }
> +}
> +
> +void spapr_hcall_tpm_reset(void)
> +{
> + if (tpm_devfd != -1) {
> + close(tpm_devfd);
> + tpm_devfd = -1;
> + }
> +}
> +
> +static void hypercall_register_types(void)
> +{
> + spapr_register_hypercall(H_TPM_COMM, h_tpm_comm);
> +}
> +
> +type_init(hypercall_register_types)
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index f76448f532..96dad767a1 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -25,6 +25,10 @@ spapr_update_dt(unsigned cb) "New blob %u bytes"
> spapr_update_dt_failed_size(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> spapr_update_dt_failed_check(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
>
> +# spapr_hcall_tpm.c
> +spapr_h_tpm_comm(const char *device_path, uint64_t operation) "tpm_device_path=%s operation=0x%"PRIu64
> +spapr_tpm_execute(uint64_t data_in, uint64_t data_in_sz, uint64_t data_out, uint64_t data_out_sz) "data_in=0x%"PRIx64", data_in_sz=%"PRIu64", data_out=0x%"PRIx64", data_out_sz=%"PRIu64
> +
> # spapr_iommu.c
> spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
> spapr_iommu_get(uint64_t liobn, uint64_t ioba, uint64_t ret, uint64_t tce) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" ret=%"PRId64" tce=0x%"PRIx64
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 60553d32c4..7bd47575d7 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -198,6 +198,7 @@ struct SpaprMachineState {
> SpaprXive *xive;
> SpaprIrq *irq;
> qemu_irq *qirqs;
> + char *tpm_device_file;
>
> bool cmd_line_caps[SPAPR_CAP_NUM];
> SpaprCapabilities def, eff, mig;
> @@ -490,8 +491,9 @@ struct SpaprMachineState {
> #define H_INT_ESB 0x3C8
> #define H_INT_SYNC 0x3CC
> #define H_INT_RESET 0x3D0
> +#define H_TPM_COMM 0xEF10
>
> -#define MAX_HCALL_OPCODE H_INT_RESET
> +#define MAX_HCALL_OPCODE H_TPM_COMM
>
> /* The hcalls above are standardized in PAPR and implemented by pHyp
> * as well.
> @@ -864,6 +866,9 @@ int spapr_caps_post_migration(SpaprMachineState *spapr);
>
> void spapr_check_pagesize(SpaprMachineState *spapr, hwaddr pagesize,
> Error **errp);
> +
> +void spapr_hcall_tpm_reset(void);
> +
> /*
> * XIVE definitions
> */
--
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] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall
2019-07-12 6:46 ` David Gibson
@ 2019-07-12 14:34 ` Michael Roth
2019-07-15 2:25 ` David Gibson
0 siblings, 1 reply; 16+ messages in thread
From: Michael Roth @ 2019-07-12 14:34 UTC (permalink / raw)
To: David Gibson; +Cc: linuxram, qemu-ppc, qemu-devel
Quoting David Gibson (2019-07-12 01:46:19)
> On Thu, Jul 11, 2019 at 08:19:34PM -0500, Michael Roth wrote:
> > This implements the H_TPM_COMM hypercall, which is used by an
> > Ultravisor to pass TPM commands directly to the host's TPM device, or
> > a TPM Resource Manager associated with the device.
> >
> > This also introduces a new pseries machine option which is used to
> > configure what TPM device to pass commands to, for example:
> >
> > -machine pseries,...,tpm-device-file=/dev/tmprm0
>
> Bolting this into yet another machine parameter seems kind of ugly.
> Wouldn't it make more sense to treat this as an virtual device (say
> "spapr-vtpm"). Adding that device would enable the hcall, and would
> have properties for the back end host device.
That does sound nicer.
Originally I had SpaprMachineClass implement the TYPE_TPM_IF interface so
we could define a TPM backend via -tpmdev passthrough,path=..., but after
some discussion with the TPM maintainer it didn't quite work for the main
use-case of passing through a TPM Resource Manager since it isn't suitable
for full vTPM front-ends (since multiple guests can interfere with each
other's operations when running the full gamut of TPM functionality).
I hadn't consider a stand-alone -device implementation though. It's not
a proper VIO or PCI device so there's no proper bus to attach it to. I
guess we would just make it a direct child of SpaprMachineState (sort
of like SpaprDrcClass), then we could define it via something like
-object spapr-tpm-proxy,path=....
I'll go ahead and give that a shot, assuming it seems reasonable to you.
>
> > By default, no tpm-device-file is defined and hcalls will return
> > H_RESOURCE.
>
> Wouldn't H_FUNCTION make more sense?
Yes, for this case it probably would.
Thanks for the suggestions!
>
> >
> > The full specification for this hypercall can be found in
> > docs/specs/ppc-spapr-uv-hcalls.txt
> >
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com
> > ---
> > hw/ppc/Makefile.objs | 1 +
> > hw/ppc/spapr.c | 27 ++++++++
> > hw/ppc/spapr_hcall_tpm.c | 135 +++++++++++++++++++++++++++++++++++++++
> > hw/ppc/trace-events | 4 ++
> > include/hw/ppc/spapr.h | 7 +-
> > 5 files changed, 173 insertions(+), 1 deletion(-)
> > create mode 100644 hw/ppc/spapr_hcall_tpm.c
> >
> > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > index 9da93af905..5aa120cae6 100644
> > --- a/hw/ppc/Makefile.objs
> > +++ b/hw/ppc/Makefile.objs
> > @@ -5,6 +5,7 @@ obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
> > obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o
> > obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
> > +obj-$(CONFIG_PSERIES) += spapr_hcall_tpm.o
> > obj-$(CONFIG_SPAPR_RNG) += spapr_rng.o
> > # IBM PowerNV
> > obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o pnv_occ.o pnv_bmc.o
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 821f0d4a49..eb3421673b 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1776,6 +1776,10 @@ static void spapr_machine_reset(MachineState *machine)
> > */
> > object_child_foreach_recursive(object_get_root(), spapr_reset_drcs, NULL);
> >
> > + if (spapr->tpm_device_file) {
> > + spapr_hcall_tpm_reset();
> > + }
> > +
> > spapr_clear_pending_events(spapr);
> >
> > /*
> > @@ -3340,6 +3344,21 @@ static void spapr_set_host_serial(Object *obj, const char *value, Error **errp)
> > spapr->host_serial = g_strdup(value);
> > }
> >
> > +static char *spapr_get_tpm_device_file(Object *obj, Error **errp)
> > +{
> > + SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > +
> > + return g_strdup(spapr->tpm_device_file);
> > +}
> > +
> > +static void spapr_set_tpm_device_file(Object *obj, const char *value, Error **errp)
> > +{
> > + SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > +
> > + g_free(spapr->tpm_device_file);
> > + spapr->tpm_device_file = g_strdup(value);
> > +}
> > +
> > static void spapr_instance_init(Object *obj)
> > {
> > SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > @@ -3396,6 +3415,14 @@ static void spapr_instance_init(Object *obj)
> > &error_abort);
> > object_property_set_description(obj, "host-serial",
> > "Host serial number to advertise in guest device tree", &error_abort);
> > + object_property_add_str(obj, "tpm-device-file",
> > + spapr_get_tpm_device_file,
> > + spapr_set_tpm_device_file, &error_abort);
> > + object_property_set_description(obj, "tpm-device-file",
> > + "Specifies the path to the TPM character device file to use"
> > + " for TPM communication via hypercalls (usually a TPM"
> > + " resource manager)",
> > + &error_abort);
> > }
> >
> > static void spapr_machine_finalizefn(Object *obj)
> > diff --git a/hw/ppc/spapr_hcall_tpm.c b/hw/ppc/spapr_hcall_tpm.c
> > new file mode 100644
> > index 0000000000..75e2b6d594
> > --- /dev/null
> > +++ b/hw/ppc/spapr_hcall_tpm.c
> > @@ -0,0 +1,135 @@
> > +/*
> > + * SPAPR TPM Hypercall
> > + *
> > + * Copyright IBM Corp. 2019
> > + *
> > + * Authors:
> > + * Michael Roth <mdroth@linux.vnet.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu-common.h"
> > +#include "qapi/error.h"
> > +#include "qemu/error-report.h"
> > +#include "cpu.h"
> > +#include "hw/ppc/spapr.h"
> > +#include "trace.h"
> > +
> > +#define TPM_SPAPR_BUFSIZE 4096
> > +
> > +enum {
> > + TPM_COMM_OP_EXECUTE = 1,
> > + TPM_COMM_OP_CLOSE_SESSION = 2,
> > +};
> > +
> > +static int tpm_devfd = -1;
>
> A global? Really? You can do better.
>
> > +
> > +static ssize_t tpm_execute(SpaprMachineState *spapr, target_ulong *args)
> > +{
> > + uint64_t data_in = ppc64_phys_to_real(args[1]);
> > + target_ulong data_in_size = args[2];
> > + uint64_t data_out = ppc64_phys_to_real(args[3]);
> > + target_ulong data_out_size = args[4];
> > + uint8_t buf_in[TPM_SPAPR_BUFSIZE];
> > + uint8_t buf_out[TPM_SPAPR_BUFSIZE];
> > + ssize_t ret;
> > +
> > + trace_spapr_tpm_execute(data_in, data_in_size, data_out, data_out_size);
> > +
> > + if (data_in_size > TPM_SPAPR_BUFSIZE) {
> > + error_report("invalid TPM input buffer size: " TARGET_FMT_lu "\n",
> > + data_in_size);
> > + return H_P3;
> > + }
> > +
> > + if (data_out_size < TPM_SPAPR_BUFSIZE) {
> > + error_report("invalid TPM output buffer size: " TARGET_FMT_lu "\n",
> > + data_out_size);
> > + return H_P5;
> > + }
> > +
> > + if (tpm_devfd == -1) {
> > + tpm_devfd = open(spapr->tpm_device_file, O_RDWR);
> > + if (tpm_devfd == -1) {
> > + error_report("failed to open TPM device %s: %d",
> > + spapr->tpm_device_file, errno);
> > + return H_RESOURCE;
> > + }
> > + }
> > +
> > + cpu_physical_memory_read(data_in, buf_in, data_in_size);
> > +
> > + do {
> > + ret = write(tpm_devfd, buf_in, data_in_size);
> > + if (ret > 0) {
> > + data_in_size -= ret;
> > + }
> > + } while ((ret >= 0 && data_in_size > 0) || (ret == -1 && errno == EINTR));
> > +
> > + if (ret == -1) {
> > + error_report("failed to write to TPM device %s: %d",
> > + spapr->tpm_device_file, errno);
> > + return H_RESOURCE;
> > + }
> > +
> > + do {
> > + ret = read(tpm_devfd, buf_out, data_out_size);
> > + } while (ret == 0 || (ret == -1 && errno == EINTR));
> > +
> > + if (ret == -1) {
> > + error_report("failed to read from TPM device %s: %d",
> > + spapr->tpm_device_file, errno);
> > + return H_RESOURCE;
> > + }
> > +
> > + cpu_physical_memory_write(data_out, buf_out, ret);
> > + args[0] = ret;
> > +
> > + return H_SUCCESS;
> > +}
> > +
> > +static target_ulong h_tpm_comm(PowerPCCPU *cpu,
> > + SpaprMachineState *spapr,
> > + target_ulong opcode,
> > + target_ulong *args)
> > +{
> > + target_ulong op = args[0];
> > +
> > + trace_spapr_h_tpm_comm(spapr->tpm_device_file ?: "null", op);
> > +
> > + if (!spapr->tpm_device_file) {
> > + error_report("TPM device not specified");
> > + return H_RESOURCE;
> > + }
> > +
> > + switch (op) {
> > + case TPM_COMM_OP_EXECUTE:
> > + return tpm_execute(spapr, args);
> > + case TPM_COMM_OP_CLOSE_SESSION:
> > + if (tpm_devfd != -1) {
> > + close(tpm_devfd);
> > + tpm_devfd = -1;
> > + }
> > + return H_SUCCESS;
> > + default:
> > + return H_PARAMETER;
> > + }
> > +}
> > +
> > +void spapr_hcall_tpm_reset(void)
> > +{
> > + if (tpm_devfd != -1) {
> > + close(tpm_devfd);
> > + tpm_devfd = -1;
> > + }
> > +}
> > +
> > +static void hypercall_register_types(void)
> > +{
> > + spapr_register_hypercall(H_TPM_COMM, h_tpm_comm);
> > +}
> > +
> > +type_init(hypercall_register_types)
> > diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> > index f76448f532..96dad767a1 100644
> > --- a/hw/ppc/trace-events
> > +++ b/hw/ppc/trace-events
> > @@ -25,6 +25,10 @@ spapr_update_dt(unsigned cb) "New blob %u bytes"
> > spapr_update_dt_failed_size(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > spapr_update_dt_failed_check(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> >
> > +# spapr_hcall_tpm.c
> > +spapr_h_tpm_comm(const char *device_path, uint64_t operation) "tpm_device_path=%s operation=0x%"PRIu64
> > +spapr_tpm_execute(uint64_t data_in, uint64_t data_in_sz, uint64_t data_out, uint64_t data_out_sz) "data_in=0x%"PRIx64", data_in_sz=%"PRIu64", data_out=0x%"PRIx64", data_out_sz=%"PRIu64
> > +
> > # spapr_iommu.c
> > spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
> > spapr_iommu_get(uint64_t liobn, uint64_t ioba, uint64_t ret, uint64_t tce) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" ret=%"PRId64" tce=0x%"PRIx64
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 60553d32c4..7bd47575d7 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -198,6 +198,7 @@ struct SpaprMachineState {
> > SpaprXive *xive;
> > SpaprIrq *irq;
> > qemu_irq *qirqs;
> > + char *tpm_device_file;
> >
> > bool cmd_line_caps[SPAPR_CAP_NUM];
> > SpaprCapabilities def, eff, mig;
> > @@ -490,8 +491,9 @@ struct SpaprMachineState {
> > #define H_INT_ESB 0x3C8
> > #define H_INT_SYNC 0x3CC
> > #define H_INT_RESET 0x3D0
> > +#define H_TPM_COMM 0xEF10
> >
> > -#define MAX_HCALL_OPCODE H_INT_RESET
> > +#define MAX_HCALL_OPCODE H_TPM_COMM
> >
> > /* The hcalls above are standardized in PAPR and implemented by pHyp
> > * as well.
> > @@ -864,6 +866,9 @@ int spapr_caps_post_migration(SpaprMachineState *spapr);
> >
> > void spapr_check_pagesize(SpaprMachineState *spapr, hwaddr pagesize,
> > Error **errp);
> > +
> > +void spapr_hcall_tpm_reset(void);
> > +
> > /*
> > * XIVE definitions
> > */
>
> --
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] docs/specs: initial spec summary for Ultravisor-related hcalls
2019-07-12 6:40 ` David Gibson
@ 2019-07-12 15:13 ` Michael Roth
2019-07-15 2:25 ` David Gibson
0 siblings, 1 reply; 16+ messages in thread
From: Michael Roth @ 2019-07-12 15:13 UTC (permalink / raw)
To: David Gibson; +Cc: linuxram, qemu-ppc, qemu-devel
Quoting David Gibson (2019-07-12 01:40:27)
> On Thu, Jul 11, 2019 at 08:19:33PM -0500, Michael Roth wrote:
> > For now this only covers hcalls relating to TPM communication since
> > it's the only one particularly important from a QEMU perspective atm,
> > but others can be added here where it makes sense.
> >
> > The full specification for all hcalls/ucalls will eventually be made
> > available in the public/OpenPower version of the PAPR specification.
> >
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> Thanks for adding this documentation. Is there a PAPR extension
> proposal which covers this, which we could cite as the source?
We have an internal document/repo that serves as a catch-all for the Ultravisor
related spec changes. We could make that available publically via github and
cite it here until it hits the full spec. Would that work?
>
> > ---
> > docs/specs/ppc-spapr-uv-hcalls.txt | 74 ++++++++++++++++++++++++++++++
> > 1 file changed, 74 insertions(+)
> > create mode 100644 docs/specs/ppc-spapr-uv-hcalls.txt
> >
> > diff --git a/docs/specs/ppc-spapr-uv-hcalls.txt b/docs/specs/ppc-spapr-uv-hcalls.txt
> > new file mode 100644
> > index 0000000000..0278f89190
> > --- /dev/null
> > +++ b/docs/specs/ppc-spapr-uv-hcalls.txt
> > @@ -0,0 +1,74 @@
> > +On PPC64 systems supporting Protected Execution Facility (PEF), system
> > +memory can be placed in a secured region where only an "ultravisor"
> > +running in firmware can provide to access it. pseries guests on such
> > +systems can communicate with the ultravisor (via ultracalls) to switch to a
> > +secure VM mode (SVM) where the guest's memory is relocated to this secured
> > +region, making its memory inaccessible to normal processes/guests running on
> > +the host.
> > +
> > +The various ultracalls/hypercalls relating to SVM mode are currently
> > +only documented internally, but are planned for direct inclusion into the
> > +public OpenPOWER version of the PAPR specification (LoPAPR/LoPAR). An internal
> > +ACR has been filed to reserve a hypercall number range specific to this
> > +use-case to avoid any future conflicts with the internally-maintained PAPR
> > +specification. This document summarizes some of these details as they relate
> > +to QEMU.
> > +
> > +== hypercalls needed by the ultravisor ==
> > +
> > +Switching to SVM mode involves a number of hcalls issued by the ultravisor
> > +to the hypervisor to orchestrate the movement of guest memory to secure
> > +memory and various other aspects SVM mode. The below documents the hcalls
> > +relevant to QEMU.
> > +
> > +- H_TPM_COMM (0xef10)
> > +
> > + For TPM_COMM_OP_EXECUTE operation:
> > + Send a request to a TPM and receive a response, opening a new TPM session
> > + if one has not already been opened.
> > +
> > + For TPM_COMM_OP_CLOSE_SESSION operation:
> > + Close the existing TPM session, if any.
> > +
> > + Arguments:
> > +
> > + r3 : H_TPM_COMM (0xef10)
> > + r4 : TPM operation, one of:
> > + TPM_COMM_OP_EXECUTE (0x1)
> > + TPM_COMM_OP_CLOSE_SESSION (0x2)
> > + r5 : in_buffer, guest physical address of buffer containing the request
> > + - Caller may use the same address for both request and response
> > + r6 : in_size, size of the in buffer, must
> > + - Must be less than or equal to 4KB
> > + r7 : out_buffer, guest physical address of buffer to store the response
> > + - Caller may use the same address for both request and response
> > + r8 : out_size, size of the out buffer
> > + - Must be at least 4KB, as this is the maximum request/response size
> > + supported by most TPM implementations, including the TPM Resource
> > + Manager in the linux kernel.
> > +
> > + Return values:
> > +
> > + r3 : H_Success request processed successfully
> > + H_PARAMETER invalid TPM operation
> > + H_P2 in_buffer is invalid
> > + H_P3 in_size is invalid
> > + H_P4 out_buffer is invalid
> > + H_P5 out_size is invalid
> > + H_RESOURCE TPM is unavailable
> > + r4 : For TPM_COMM_OP_EXECUTE, the size of the response will be stored here
> > + upon success.
> > +
> > + Use-case/notes:
> > +
> > + SVM filesystems are encrypted using a symmetric key. This key is then
> > + wrapped/encrypted using the public key of a trusted system which has the
> > + private key stored in the system's TPM. An Ultravisor will use this
> > + hcall to unwrap/unseal the symmetric key using the system's TPM device
> > + or a TPM Resource Manager associated with the device.
> > +
> > + The Ultravisor sets up a separate session key with the TPM in advance
> > + during host system boot. All sensitive in and out values will be
> > + encrypted using the session key. Though the hypervisor will see the 'in'
> > + and 'out' buffers in raw form, any sensitive contents will generally be
> > + encrypted using this session key.
>
> --
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/2] spapr: Implement H_TPM_COMM for accessing host TPM device
2019-07-12 1:19 [Qemu-devel] [RFC PATCH 0/2] spapr: Implement H_TPM_COMM for accessing host TPM device Michael Roth
` (2 preceding siblings ...)
2019-07-12 6:40 ` [Qemu-devel] [RFC PATCH 0/2] spapr: Implement H_TPM_COMM for accessing host TPM device David Gibson
@ 2019-07-12 15:33 ` no-reply
3 siblings, 0 replies; 16+ messages in thread
From: no-reply @ 2019-07-12 15:33 UTC (permalink / raw)
To: mdroth; +Cc: qemu-ppc, linuxram, qemu-devel, david
Patchew URL: https://patchew.org/QEMU/20190712011934.29863-1-mdroth@linux.vnet.ibm.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20190712011934.29863-1-mdroth@linux.vnet.ibm.com
Type: series
Subject: [Qemu-devel] [RFC PATCH 0/2] spapr: Implement H_TPM_COMM for accessing host TPM device
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Switched to a new branch 'test'
12b8055a19 spapr: initial implementation for H_TPM_COMM hcall
47c8841564 docs/specs: initial spec summary for Ultravisor-related hcalls
=== OUTPUT BEGIN ===
1/2 Checking commit 47c884156452 (docs/specs: initial spec summary for Ultravisor-related hcalls)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#18:
new file mode 100644
total: 0 errors, 1 warnings, 74 lines checked
Patch 1/2 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/2 Checking commit 12b8055a1905 (spapr: initial implementation for H_TPM_COMM hcall)
WARNING: line over 80 characters
#63: FILE: hw/ppc/spapr.c:3354:
+static void spapr_set_tpm_device_file(Object *obj, const char *value, Error **errp)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#90:
new file mode 100644
ERROR: Error messages should not contain newlines
#137: FILE: hw/ppc/spapr_hcall_tpm.c:43:
+ error_report("invalid TPM input buffer size: " TARGET_FMT_lu "\n",
ERROR: Error messages should not contain newlines
#143: FILE: hw/ppc/spapr_hcall_tpm.c:49:
+ error_report("invalid TPM output buffer size: " TARGET_FMT_lu "\n",
ERROR: switch and case should be at the same indent
#202: FILE: hw/ppc/spapr_hcall_tpm.c:108:
+ switch (op) {
+ case TPM_COMM_OP_EXECUTE:
[...]
+ case TPM_COMM_OP_CLOSE_SESSION:
[...]
+ default:
total: 3 errors, 2 warnings, 223 lines checked
Patch 2/2 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20190712011934.29863-1-mdroth@linux.vnet.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall
2019-07-12 14:34 ` Michael Roth
@ 2019-07-15 2:25 ` David Gibson
2019-07-16 16:30 ` Michael Roth
0 siblings, 1 reply; 16+ messages in thread
From: David Gibson @ 2019-07-15 2:25 UTC (permalink / raw)
To: Michael Roth; +Cc: linuxram, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 13101 bytes --]
On Fri, Jul 12, 2019 at 09:34:46AM -0500, Michael Roth wrote:
> Quoting David Gibson (2019-07-12 01:46:19)
> > On Thu, Jul 11, 2019 at 08:19:34PM -0500, Michael Roth wrote:
> > > This implements the H_TPM_COMM hypercall, which is used by an
> > > Ultravisor to pass TPM commands directly to the host's TPM device, or
> > > a TPM Resource Manager associated with the device.
> > >
> > > This also introduces a new pseries machine option which is used to
> > > configure what TPM device to pass commands to, for example:
> > >
> > > -machine pseries,...,tpm-device-file=/dev/tmprm0
> >
> > Bolting this into yet another machine parameter seems kind of ugly.
> > Wouldn't it make more sense to treat this as an virtual device (say
> > "spapr-vtpm"). Adding that device would enable the hcall, and would
> > have properties for the back end host device.
>
> That does sound nicer.
>
> Originally I had SpaprMachineClass implement the TYPE_TPM_IF interface so
> we could define a TPM backend via -tpmdev passthrough,path=..., but after
> some discussion with the TPM maintainer it didn't quite work for the main
> use-case of passing through a TPM Resource Manager since it isn't suitable
> for full vTPM front-ends (since multiple guests can interfere with each
> other's operations when running the full gamut of TPM functionality).
>
> I hadn't consider a stand-alone -device implementation though. It's not
> a proper VIO or PCI device so there's no proper bus to attach it to. I
> guess we would just make it a direct child of SpaprMachineState (sort
> of like SpaprDrcClass), then we could define it via something like
> -object spapr-tpm-proxy,path=....
It should be -device not -object, but otherwise that looks ok.
How does the TPM appear in the device tree?
> I'll go ahead and give that a shot, assuming it seems reasonable to you.
>
> >
> > > By default, no tpm-device-file is defined and hcalls will return
> > > H_RESOURCE.
> >
> > Wouldn't H_FUNCTION make more sense?
>
> Yes, for this case it probably would.
>
> Thanks for the suggestions!
>
> >
> > >
> > > The full specification for this hypercall can be found in
> > > docs/specs/ppc-spapr-uv-hcalls.txt
> > >
> > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com
> > > ---
> > > hw/ppc/Makefile.objs | 1 +
> > > hw/ppc/spapr.c | 27 ++++++++
> > > hw/ppc/spapr_hcall_tpm.c | 135 +++++++++++++++++++++++++++++++++++++++
> > > hw/ppc/trace-events | 4 ++
> > > include/hw/ppc/spapr.h | 7 +-
> > > 5 files changed, 173 insertions(+), 1 deletion(-)
> > > create mode 100644 hw/ppc/spapr_hcall_tpm.c
> > >
> > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > index 9da93af905..5aa120cae6 100644
> > > --- a/hw/ppc/Makefile.objs
> > > +++ b/hw/ppc/Makefile.objs
> > > @@ -5,6 +5,7 @@ obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
> > > obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > > obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o
> > > obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
> > > +obj-$(CONFIG_PSERIES) += spapr_hcall_tpm.o
> > > obj-$(CONFIG_SPAPR_RNG) += spapr_rng.o
> > > # IBM PowerNV
> > > obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o pnv_occ.o pnv_bmc.o
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 821f0d4a49..eb3421673b 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1776,6 +1776,10 @@ static void spapr_machine_reset(MachineState *machine)
> > > */
> > > object_child_foreach_recursive(object_get_root(), spapr_reset_drcs, NULL);
> > >
> > > + if (spapr->tpm_device_file) {
> > > + spapr_hcall_tpm_reset();
> > > + }
> > > +
> > > spapr_clear_pending_events(spapr);
> > >
> > > /*
> > > @@ -3340,6 +3344,21 @@ static void spapr_set_host_serial(Object *obj, const char *value, Error **errp)
> > > spapr->host_serial = g_strdup(value);
> > > }
> > >
> > > +static char *spapr_get_tpm_device_file(Object *obj, Error **errp)
> > > +{
> > > + SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > +
> > > + return g_strdup(spapr->tpm_device_file);
> > > +}
> > > +
> > > +static void spapr_set_tpm_device_file(Object *obj, const char *value, Error **errp)
> > > +{
> > > + SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > +
> > > + g_free(spapr->tpm_device_file);
> > > + spapr->tpm_device_file = g_strdup(value);
> > > +}
> > > +
> > > static void spapr_instance_init(Object *obj)
> > > {
> > > SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > @@ -3396,6 +3415,14 @@ static void spapr_instance_init(Object *obj)
> > > &error_abort);
> > > object_property_set_description(obj, "host-serial",
> > > "Host serial number to advertise in guest device tree", &error_abort);
> > > + object_property_add_str(obj, "tpm-device-file",
> > > + spapr_get_tpm_device_file,
> > > + spapr_set_tpm_device_file, &error_abort);
> > > + object_property_set_description(obj, "tpm-device-file",
> > > + "Specifies the path to the TPM character device file to use"
> > > + " for TPM communication via hypercalls (usually a TPM"
> > > + " resource manager)",
> > > + &error_abort);
> > > }
> > >
> > > static void spapr_machine_finalizefn(Object *obj)
> > > diff --git a/hw/ppc/spapr_hcall_tpm.c b/hw/ppc/spapr_hcall_tpm.c
> > > new file mode 100644
> > > index 0000000000..75e2b6d594
> > > --- /dev/null
> > > +++ b/hw/ppc/spapr_hcall_tpm.c
> > > @@ -0,0 +1,135 @@
> > > +/*
> > > + * SPAPR TPM Hypercall
> > > + *
> > > + * Copyright IBM Corp. 2019
> > > + *
> > > + * Authors:
> > > + * Michael Roth <mdroth@linux.vnet.ibm.com>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > + * See the COPYING file in the top-level directory.
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "qemu-common.h"
> > > +#include "qapi/error.h"
> > > +#include "qemu/error-report.h"
> > > +#include "cpu.h"
> > > +#include "hw/ppc/spapr.h"
> > > +#include "trace.h"
> > > +
> > > +#define TPM_SPAPR_BUFSIZE 4096
> > > +
> > > +enum {
> > > + TPM_COMM_OP_EXECUTE = 1,
> > > + TPM_COMM_OP_CLOSE_SESSION = 2,
> > > +};
> > > +
> > > +static int tpm_devfd = -1;
> >
> > A global? Really? You can do better.
> >
> > > +
> > > +static ssize_t tpm_execute(SpaprMachineState *spapr, target_ulong *args)
> > > +{
> > > + uint64_t data_in = ppc64_phys_to_real(args[1]);
> > > + target_ulong data_in_size = args[2];
> > > + uint64_t data_out = ppc64_phys_to_real(args[3]);
> > > + target_ulong data_out_size = args[4];
> > > + uint8_t buf_in[TPM_SPAPR_BUFSIZE];
> > > + uint8_t buf_out[TPM_SPAPR_BUFSIZE];
> > > + ssize_t ret;
> > > +
> > > + trace_spapr_tpm_execute(data_in, data_in_size, data_out, data_out_size);
> > > +
> > > + if (data_in_size > TPM_SPAPR_BUFSIZE) {
> > > + error_report("invalid TPM input buffer size: " TARGET_FMT_lu "\n",
> > > + data_in_size);
> > > + return H_P3;
> > > + }
> > > +
> > > + if (data_out_size < TPM_SPAPR_BUFSIZE) {
> > > + error_report("invalid TPM output buffer size: " TARGET_FMT_lu "\n",
> > > + data_out_size);
> > > + return H_P5;
> > > + }
> > > +
> > > + if (tpm_devfd == -1) {
> > > + tpm_devfd = open(spapr->tpm_device_file, O_RDWR);
> > > + if (tpm_devfd == -1) {
> > > + error_report("failed to open TPM device %s: %d",
> > > + spapr->tpm_device_file, errno);
> > > + return H_RESOURCE;
> > > + }
> > > + }
> > > +
> > > + cpu_physical_memory_read(data_in, buf_in, data_in_size);
> > > +
> > > + do {
> > > + ret = write(tpm_devfd, buf_in, data_in_size);
> > > + if (ret > 0) {
> > > + data_in_size -= ret;
> > > + }
> > > + } while ((ret >= 0 && data_in_size > 0) || (ret == -1 && errno == EINTR));
> > > +
> > > + if (ret == -1) {
> > > + error_report("failed to write to TPM device %s: %d",
> > > + spapr->tpm_device_file, errno);
> > > + return H_RESOURCE;
> > > + }
> > > +
> > > + do {
> > > + ret = read(tpm_devfd, buf_out, data_out_size);
> > > + } while (ret == 0 || (ret == -1 && errno == EINTR));
> > > +
> > > + if (ret == -1) {
> > > + error_report("failed to read from TPM device %s: %d",
> > > + spapr->tpm_device_file, errno);
> > > + return H_RESOURCE;
> > > + }
> > > +
> > > + cpu_physical_memory_write(data_out, buf_out, ret);
> > > + args[0] = ret;
> > > +
> > > + return H_SUCCESS;
> > > +}
> > > +
> > > +static target_ulong h_tpm_comm(PowerPCCPU *cpu,
> > > + SpaprMachineState *spapr,
> > > + target_ulong opcode,
> > > + target_ulong *args)
> > > +{
> > > + target_ulong op = args[0];
> > > +
> > > + trace_spapr_h_tpm_comm(spapr->tpm_device_file ?: "null", op);
> > > +
> > > + if (!spapr->tpm_device_file) {
> > > + error_report("TPM device not specified");
> > > + return H_RESOURCE;
> > > + }
> > > +
> > > + switch (op) {
> > > + case TPM_COMM_OP_EXECUTE:
> > > + return tpm_execute(spapr, args);
> > > + case TPM_COMM_OP_CLOSE_SESSION:
> > > + if (tpm_devfd != -1) {
> > > + close(tpm_devfd);
> > > + tpm_devfd = -1;
> > > + }
> > > + return H_SUCCESS;
> > > + default:
> > > + return H_PARAMETER;
> > > + }
> > > +}
> > > +
> > > +void spapr_hcall_tpm_reset(void)
> > > +{
> > > + if (tpm_devfd != -1) {
> > > + close(tpm_devfd);
> > > + tpm_devfd = -1;
> > > + }
> > > +}
> > > +
> > > +static void hypercall_register_types(void)
> > > +{
> > > + spapr_register_hypercall(H_TPM_COMM, h_tpm_comm);
> > > +}
> > > +
> > > +type_init(hypercall_register_types)
> > > diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> > > index f76448f532..96dad767a1 100644
> > > --- a/hw/ppc/trace-events
> > > +++ b/hw/ppc/trace-events
> > > @@ -25,6 +25,10 @@ spapr_update_dt(unsigned cb) "New blob %u bytes"
> > > spapr_update_dt_failed_size(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > > spapr_update_dt_failed_check(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > >
> > > +# spapr_hcall_tpm.c
> > > +spapr_h_tpm_comm(const char *device_path, uint64_t operation) "tpm_device_path=%s operation=0x%"PRIu64
> > > +spapr_tpm_execute(uint64_t data_in, uint64_t data_in_sz, uint64_t data_out, uint64_t data_out_sz) "data_in=0x%"PRIx64", data_in_sz=%"PRIu64", data_out=0x%"PRIx64", data_out_sz=%"PRIu64
> > > +
> > > # spapr_iommu.c
> > > spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
> > > spapr_iommu_get(uint64_t liobn, uint64_t ioba, uint64_t ret, uint64_t tce) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" ret=%"PRId64" tce=0x%"PRIx64
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index 60553d32c4..7bd47575d7 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -198,6 +198,7 @@ struct SpaprMachineState {
> > > SpaprXive *xive;
> > > SpaprIrq *irq;
> > > qemu_irq *qirqs;
> > > + char *tpm_device_file;
> > >
> > > bool cmd_line_caps[SPAPR_CAP_NUM];
> > > SpaprCapabilities def, eff, mig;
> > > @@ -490,8 +491,9 @@ struct SpaprMachineState {
> > > #define H_INT_ESB 0x3C8
> > > #define H_INT_SYNC 0x3CC
> > > #define H_INT_RESET 0x3D0
> > > +#define H_TPM_COMM 0xEF10
> > >
> > > -#define MAX_HCALL_OPCODE H_INT_RESET
> > > +#define MAX_HCALL_OPCODE H_TPM_COMM
> > >
> > > /* The hcalls above are standardized in PAPR and implemented by pHyp
> > > * as well.
> > > @@ -864,6 +866,9 @@ int spapr_caps_post_migration(SpaprMachineState *spapr);
> > >
> > > void spapr_check_pagesize(SpaprMachineState *spapr, hwaddr pagesize,
> > > Error **errp);
> > > +
> > > +void spapr_hcall_tpm_reset(void);
> > > +
> > > /*
> > > * XIVE definitions
> > > */
> >
>
--
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] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] docs/specs: initial spec summary for Ultravisor-related hcalls
2019-07-12 15:13 ` Michael Roth
@ 2019-07-15 2:25 ` David Gibson
2019-07-16 16:25 ` Michael Roth
0 siblings, 1 reply; 16+ messages in thread
From: David Gibson @ 2019-07-15 2:25 UTC (permalink / raw)
To: Michael Roth; +Cc: linuxram, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]
On Fri, Jul 12, 2019 at 10:13:48AM -0500, Michael Roth wrote:
> Quoting David Gibson (2019-07-12 01:40:27)
> > On Thu, Jul 11, 2019 at 08:19:33PM -0500, Michael Roth wrote:
> > > For now this only covers hcalls relating to TPM communication since
> > > it's the only one particularly important from a QEMU perspective atm,
> > > but others can be added here where it makes sense.
> > >
> > > The full specification for all hcalls/ucalls will eventually be made
> > > available in the public/OpenPower version of the PAPR specification.
> > >
> > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> >
> > Thanks for adding this documentation. Is there a PAPR extension
> > proposal which covers this, which we could cite as the source?
>
> We have an internal document/repo that serves as a catch-all for the Ultravisor
> related spec changes. We could make that available publically via github and
> cite it here until it hits the full spec. Would that work?
Yes, that sounds good.
--
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] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] docs/specs: initial spec summary for Ultravisor-related hcalls
2019-07-15 2:25 ` David Gibson
@ 2019-07-16 16:25 ` Michael Roth
0 siblings, 0 replies; 16+ messages in thread
From: Michael Roth @ 2019-07-16 16:25 UTC (permalink / raw)
To: David Gibson; +Cc: linuxram, qemu-ppc, qemu-devel
Quoting David Gibson (2019-07-14 21:25:55)
> On Fri, Jul 12, 2019 at 10:13:48AM -0500, Michael Roth wrote:
> > Quoting David Gibson (2019-07-12 01:40:27)
> > > On Thu, Jul 11, 2019 at 08:19:33PM -0500, Michael Roth wrote:
> > > > For now this only covers hcalls relating to TPM communication since
> > > > it's the only one particularly important from a QEMU perspective atm,
> > > > but others can be added here where it makes sense.
> > > >
> > > > The full specification for all hcalls/ucalls will eventually be made
> > > > available in the public/OpenPower version of the PAPR specification.
> > > >
> > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > >
> > > Thanks for adding this documentation. Is there a PAPR extension
> > > proposal which covers this, which we could cite as the source?
> >
> > We have an internal document/repo that serves as a catch-all for the Ultravisor
> > related spec changes. We could make that available publically via github and
> > cite it here until it hits the full spec. Would that work?
>
> Yes, that sounds good.
Ok, we're working on getting that posted externally. If it's not up in
time for next submission I will send a follow-up patch to add a
reference.
>
> --
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall
2019-07-15 2:25 ` David Gibson
@ 2019-07-16 16:30 ` Michael Roth
2019-07-17 1:29 ` David Gibson
0 siblings, 1 reply; 16+ messages in thread
From: Michael Roth @ 2019-07-16 16:30 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, linuxram, qemu-devel
Quoting David Gibson (2019-07-14 21:25:24)
> On Fri, Jul 12, 2019 at 09:34:46AM -0500, Michael Roth wrote:
> > Quoting David Gibson (2019-07-12 01:46:19)
> > > On Thu, Jul 11, 2019 at 08:19:34PM -0500, Michael Roth wrote:
> > > > This implements the H_TPM_COMM hypercall, which is used by an
> > > > Ultravisor to pass TPM commands directly to the host's TPM device, or
> > > > a TPM Resource Manager associated with the device.
> > > >
> > > > This also introduces a new pseries machine option which is used to
> > > > configure what TPM device to pass commands to, for example:
> > > >
> > > > -machine pseries,...,tpm-device-file=/dev/tmprm0
> > >
> > > Bolting this into yet another machine parameter seems kind of ugly.
> > > Wouldn't it make more sense to treat this as an virtual device (say
> > > "spapr-vtpm"). Adding that device would enable the hcall, and would
> > > have properties for the back end host device.
> >
> > That does sound nicer.
> >
> > Originally I had SpaprMachineClass implement the TYPE_TPM_IF interface so
> > we could define a TPM backend via -tpmdev passthrough,path=..., but after
> > some discussion with the TPM maintainer it didn't quite work for the main
> > use-case of passing through a TPM Resource Manager since it isn't suitable
> > for full vTPM front-ends (since multiple guests can interfere with each
> > other's operations when running the full gamut of TPM functionality).
> >
> > I hadn't consider a stand-alone -device implementation though. It's not
> > a proper VIO or PCI device so there's no proper bus to attach it to. I
> > guess we would just make it a direct child of SpaprMachineState (sort
> > of like SpaprDrcClass), then we could define it via something like
> > -object spapr-tpm-proxy,path=....
>
> It should be -device not -object, but otherwise that looks ok.
Ok, for some reason I thought -device needed either a specific bus or
needed to be a SysBusDevice to attach to main-system-bus, but maybe that
was just for qdev-managed reset handling. I've re-worked the series to
allow configuration via:
-device spapr-tpm-proxy,host_path=/dev/tpmrmX
>
> How does the TPM appear in the device tree?
Nothing in the guest, on the host it appears as:
./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57
./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/link-id
./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/linux,sml-size
./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/label
./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/compatible
./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/status
./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/reg
./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/phandle
./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/linux,sml-base
./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/name
>
> > I'll go ahead and give that a shot, assuming it seems reasonable to you.
> >
> > >
> > > > By default, no tpm-device-file is defined and hcalls will return
> > > > H_RESOURCE.
> > >
> > > Wouldn't H_FUNCTION make more sense?
> >
> > Yes, for this case it probably would.
> >
> > Thanks for the suggestions!
> >
> > >
> > > >
> > > > The full specification for this hypercall can be found in
> > > > docs/specs/ppc-spapr-uv-hcalls.txt
> > > >
> > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com
> > > > ---
> > > > hw/ppc/Makefile.objs | 1 +
> > > > hw/ppc/spapr.c | 27 ++++++++
> > > > hw/ppc/spapr_hcall_tpm.c | 135 +++++++++++++++++++++++++++++++++++++++
> > > > hw/ppc/trace-events | 4 ++
> > > > include/hw/ppc/spapr.h | 7 +-
> > > > 5 files changed, 173 insertions(+), 1 deletion(-)
> > > > create mode 100644 hw/ppc/spapr_hcall_tpm.c
> > > >
> > > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > > index 9da93af905..5aa120cae6 100644
> > > > --- a/hw/ppc/Makefile.objs
> > > > +++ b/hw/ppc/Makefile.objs
> > > > @@ -5,6 +5,7 @@ obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
> > > > obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > > > obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o
> > > > obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
> > > > +obj-$(CONFIG_PSERIES) += spapr_hcall_tpm.o
> > > > obj-$(CONFIG_SPAPR_RNG) += spapr_rng.o
> > > > # IBM PowerNV
> > > > obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o pnv_occ.o pnv_bmc.o
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 821f0d4a49..eb3421673b 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1776,6 +1776,10 @@ static void spapr_machine_reset(MachineState *machine)
> > > > */
> > > > object_child_foreach_recursive(object_get_root(), spapr_reset_drcs, NULL);
> > > >
> > > > + if (spapr->tpm_device_file) {
> > > > + spapr_hcall_tpm_reset();
> > > > + }
> > > > +
> > > > spapr_clear_pending_events(spapr);
> > > >
> > > > /*
> > > > @@ -3340,6 +3344,21 @@ static void spapr_set_host_serial(Object *obj, const char *value, Error **errp)
> > > > spapr->host_serial = g_strdup(value);
> > > > }
> > > >
> > > > +static char *spapr_get_tpm_device_file(Object *obj, Error **errp)
> > > > +{
> > > > + SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > > +
> > > > + return g_strdup(spapr->tpm_device_file);
> > > > +}
> > > > +
> > > > +static void spapr_set_tpm_device_file(Object *obj, const char *value, Error **errp)
> > > > +{
> > > > + SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > > +
> > > > + g_free(spapr->tpm_device_file);
> > > > + spapr->tpm_device_file = g_strdup(value);
> > > > +}
> > > > +
> > > > static void spapr_instance_init(Object *obj)
> > > > {
> > > > SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > > @@ -3396,6 +3415,14 @@ static void spapr_instance_init(Object *obj)
> > > > &error_abort);
> > > > object_property_set_description(obj, "host-serial",
> > > > "Host serial number to advertise in guest device tree", &error_abort);
> > > > + object_property_add_str(obj, "tpm-device-file",
> > > > + spapr_get_tpm_device_file,
> > > > + spapr_set_tpm_device_file, &error_abort);
> > > > + object_property_set_description(obj, "tpm-device-file",
> > > > + "Specifies the path to the TPM character device file to use"
> > > > + " for TPM communication via hypercalls (usually a TPM"
> > > > + " resource manager)",
> > > > + &error_abort);
> > > > }
> > > >
> > > > static void spapr_machine_finalizefn(Object *obj)
> > > > diff --git a/hw/ppc/spapr_hcall_tpm.c b/hw/ppc/spapr_hcall_tpm.c
> > > > new file mode 100644
> > > > index 0000000000..75e2b6d594
> > > > --- /dev/null
> > > > +++ b/hw/ppc/spapr_hcall_tpm.c
> > > > @@ -0,0 +1,135 @@
> > > > +/*
> > > > + * SPAPR TPM Hypercall
> > > > + *
> > > > + * Copyright IBM Corp. 2019
> > > > + *
> > > > + * Authors:
> > > > + * Michael Roth <mdroth@linux.vnet.ibm.com>
> > > > + *
> > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > + * See the COPYING file in the top-level directory.
> > > > + */
> > > > +
> > > > +#include "qemu/osdep.h"
> > > > +#include "qemu-common.h"
> > > > +#include "qapi/error.h"
> > > > +#include "qemu/error-report.h"
> > > > +#include "cpu.h"
> > > > +#include "hw/ppc/spapr.h"
> > > > +#include "trace.h"
> > > > +
> > > > +#define TPM_SPAPR_BUFSIZE 4096
> > > > +
> > > > +enum {
> > > > + TPM_COMM_OP_EXECUTE = 1,
> > > > + TPM_COMM_OP_CLOSE_SESSION = 2,
> > > > +};
> > > > +
> > > > +static int tpm_devfd = -1;
> > >
> > > A global? Really? You can do better.
> > >
> > > > +
> > > > +static ssize_t tpm_execute(SpaprMachineState *spapr, target_ulong *args)
> > > > +{
> > > > + uint64_t data_in = ppc64_phys_to_real(args[1]);
> > > > + target_ulong data_in_size = args[2];
> > > > + uint64_t data_out = ppc64_phys_to_real(args[3]);
> > > > + target_ulong data_out_size = args[4];
> > > > + uint8_t buf_in[TPM_SPAPR_BUFSIZE];
> > > > + uint8_t buf_out[TPM_SPAPR_BUFSIZE];
> > > > + ssize_t ret;
> > > > +
> > > > + trace_spapr_tpm_execute(data_in, data_in_size, data_out, data_out_size);
> > > > +
> > > > + if (data_in_size > TPM_SPAPR_BUFSIZE) {
> > > > + error_report("invalid TPM input buffer size: " TARGET_FMT_lu "\n",
> > > > + data_in_size);
> > > > + return H_P3;
> > > > + }
> > > > +
> > > > + if (data_out_size < TPM_SPAPR_BUFSIZE) {
> > > > + error_report("invalid TPM output buffer size: " TARGET_FMT_lu "\n",
> > > > + data_out_size);
> > > > + return H_P5;
> > > > + }
> > > > +
> > > > + if (tpm_devfd == -1) {
> > > > + tpm_devfd = open(spapr->tpm_device_file, O_RDWR);
> > > > + if (tpm_devfd == -1) {
> > > > + error_report("failed to open TPM device %s: %d",
> > > > + spapr->tpm_device_file, errno);
> > > > + return H_RESOURCE;
> > > > + }
> > > > + }
> > > > +
> > > > + cpu_physical_memory_read(data_in, buf_in, data_in_size);
> > > > +
> > > > + do {
> > > > + ret = write(tpm_devfd, buf_in, data_in_size);
> > > > + if (ret > 0) {
> > > > + data_in_size -= ret;
> > > > + }
> > > > + } while ((ret >= 0 && data_in_size > 0) || (ret == -1 && errno == EINTR));
> > > > +
> > > > + if (ret == -1) {
> > > > + error_report("failed to write to TPM device %s: %d",
> > > > + spapr->tpm_device_file, errno);
> > > > + return H_RESOURCE;
> > > > + }
> > > > +
> > > > + do {
> > > > + ret = read(tpm_devfd, buf_out, data_out_size);
> > > > + } while (ret == 0 || (ret == -1 && errno == EINTR));
> > > > +
> > > > + if (ret == -1) {
> > > > + error_report("failed to read from TPM device %s: %d",
> > > > + spapr->tpm_device_file, errno);
> > > > + return H_RESOURCE;
> > > > + }
> > > > +
> > > > + cpu_physical_memory_write(data_out, buf_out, ret);
> > > > + args[0] = ret;
> > > > +
> > > > + return H_SUCCESS;
> > > > +}
> > > > +
> > > > +static target_ulong h_tpm_comm(PowerPCCPU *cpu,
> > > > + SpaprMachineState *spapr,
> > > > + target_ulong opcode,
> > > > + target_ulong *args)
> > > > +{
> > > > + target_ulong op = args[0];
> > > > +
> > > > + trace_spapr_h_tpm_comm(spapr->tpm_device_file ?: "null", op);
> > > > +
> > > > + if (!spapr->tpm_device_file) {
> > > > + error_report("TPM device not specified");
> > > > + return H_RESOURCE;
> > > > + }
> > > > +
> > > > + switch (op) {
> > > > + case TPM_COMM_OP_EXECUTE:
> > > > + return tpm_execute(spapr, args);
> > > > + case TPM_COMM_OP_CLOSE_SESSION:
> > > > + if (tpm_devfd != -1) {
> > > > + close(tpm_devfd);
> > > > + tpm_devfd = -1;
> > > > + }
> > > > + return H_SUCCESS;
> > > > + default:
> > > > + return H_PARAMETER;
> > > > + }
> > > > +}
> > > > +
> > > > +void spapr_hcall_tpm_reset(void)
> > > > +{
> > > > + if (tpm_devfd != -1) {
> > > > + close(tpm_devfd);
> > > > + tpm_devfd = -1;
> > > > + }
> > > > +}
> > > > +
> > > > +static void hypercall_register_types(void)
> > > > +{
> > > > + spapr_register_hypercall(H_TPM_COMM, h_tpm_comm);
> > > > +}
> > > > +
> > > > +type_init(hypercall_register_types)
> > > > diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> > > > index f76448f532..96dad767a1 100644
> > > > --- a/hw/ppc/trace-events
> > > > +++ b/hw/ppc/trace-events
> > > > @@ -25,6 +25,10 @@ spapr_update_dt(unsigned cb) "New blob %u bytes"
> > > > spapr_update_dt_failed_size(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > > > spapr_update_dt_failed_check(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > > >
> > > > +# spapr_hcall_tpm.c
> > > > +spapr_h_tpm_comm(const char *device_path, uint64_t operation) "tpm_device_path=%s operation=0x%"PRIu64
> > > > +spapr_tpm_execute(uint64_t data_in, uint64_t data_in_sz, uint64_t data_out, uint64_t data_out_sz) "data_in=0x%"PRIx64", data_in_sz=%"PRIu64", data_out=0x%"PRIx64", data_out_sz=%"PRIu64
> > > > +
> > > > # spapr_iommu.c
> > > > spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
> > > > spapr_iommu_get(uint64_t liobn, uint64_t ioba, uint64_t ret, uint64_t tce) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" ret=%"PRId64" tce=0x%"PRIx64
> > > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > > index 60553d32c4..7bd47575d7 100644
> > > > --- a/include/hw/ppc/spapr.h
> > > > +++ b/include/hw/ppc/spapr.h
> > > > @@ -198,6 +198,7 @@ struct SpaprMachineState {
> > > > SpaprXive *xive;
> > > > SpaprIrq *irq;
> > > > qemu_irq *qirqs;
> > > > + char *tpm_device_file;
> > > >
> > > > bool cmd_line_caps[SPAPR_CAP_NUM];
> > > > SpaprCapabilities def, eff, mig;
> > > > @@ -490,8 +491,9 @@ struct SpaprMachineState {
> > > > #define H_INT_ESB 0x3C8
> > > > #define H_INT_SYNC 0x3CC
> > > > #define H_INT_RESET 0x3D0
> > > > +#define H_TPM_COMM 0xEF10
> > > >
> > > > -#define MAX_HCALL_OPCODE H_INT_RESET
> > > > +#define MAX_HCALL_OPCODE H_TPM_COMM
> > > >
> > > > /* The hcalls above are standardized in PAPR and implemented by pHyp
> > > > * as well.
> > > > @@ -864,6 +866,9 @@ int spapr_caps_post_migration(SpaprMachineState *spapr);
> > > >
> > > > void spapr_check_pagesize(SpaprMachineState *spapr, hwaddr pagesize,
> > > > Error **errp);
> > > > +
> > > > +void spapr_hcall_tpm_reset(void);
> > > > +
> > > > /*
> > > > * XIVE definitions
> > > > */
> > >
> >
>
> --
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall
2019-07-16 16:30 ` Michael Roth
@ 2019-07-17 1:29 ` David Gibson
2019-07-17 20:53 ` Michael Roth
0 siblings, 1 reply; 16+ messages in thread
From: David Gibson @ 2019-07-17 1:29 UTC (permalink / raw)
To: Michael Roth; +Cc: qemu-ppc, linuxram, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 15682 bytes --]
On Tue, Jul 16, 2019 at 11:30:01AM -0500, Michael Roth wrote:
> Quoting David Gibson (2019-07-14 21:25:24)
> > On Fri, Jul 12, 2019 at 09:34:46AM -0500, Michael Roth wrote:
> > > Quoting David Gibson (2019-07-12 01:46:19)
> > > > On Thu, Jul 11, 2019 at 08:19:34PM -0500, Michael Roth wrote:
> > > > > This implements the H_TPM_COMM hypercall, which is used by an
> > > > > Ultravisor to pass TPM commands directly to the host's TPM device, or
> > > > > a TPM Resource Manager associated with the device.
> > > > >
> > > > > This also introduces a new pseries machine option which is used to
> > > > > configure what TPM device to pass commands to, for example:
> > > > >
> > > > > -machine pseries,...,tpm-device-file=/dev/tmprm0
> > > >
> > > > Bolting this into yet another machine parameter seems kind of ugly.
> > > > Wouldn't it make more sense to treat this as an virtual device (say
> > > > "spapr-vtpm"). Adding that device would enable the hcall, and would
> > > > have properties for the back end host device.
> > >
> > > That does sound nicer.
> > >
> > > Originally I had SpaprMachineClass implement the TYPE_TPM_IF interface so
> > > we could define a TPM backend via -tpmdev passthrough,path=..., but after
> > > some discussion with the TPM maintainer it didn't quite work for the main
> > > use-case of passing through a TPM Resource Manager since it isn't suitable
> > > for full vTPM front-ends (since multiple guests can interfere with each
> > > other's operations when running the full gamut of TPM functionality).
> > >
> > > I hadn't consider a stand-alone -device implementation though. It's not
> > > a proper VIO or PCI device so there's no proper bus to attach it to. I
> > > guess we would just make it a direct child of SpaprMachineState (sort
> > > of like SpaprDrcClass), then we could define it via something like
> > > -object spapr-tpm-proxy,path=....
> >
> > It should be -device not -object, but otherwise that looks ok.
>
> Ok, for some reason I thought -device needed either a specific bus or
> needed to be a SysBusDevice to attach to main-system-bus, but maybe that
> was just for qdev-managed reset handling. I've re-worked the series to
> allow configuration via:
>
> -device spapr-tpm-proxy,host_path=/dev/tpmrmX
That looks good.
> > How does the TPM appear in the device tree?
>
> Nothing in the guest, on the host it appears as:
Hrm. That seems unwise. I mean, I guess its being treated as a
hypervisor facility rather than a device per se, but what if we ever
need to advertise more metadata about it.
> ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57
> ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/link-id
> ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/linux,sml-size
> ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/label
> ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/compatible
> ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/status
> ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/reg
> ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/phandle
> ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/linux,sml-base
> ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/name
>
> >
> > > I'll go ahead and give that a shot, assuming it seems reasonable to you.
> > >
> > > >
> > > > > By default, no tpm-device-file is defined and hcalls will return
> > > > > H_RESOURCE.
> > > >
> > > > Wouldn't H_FUNCTION make more sense?
> > >
> > > Yes, for this case it probably would.
> > >
> > > Thanks for the suggestions!
> > >
> > > >
> > > > >
> > > > > The full specification for this hypercall can be found in
> > > > > docs/specs/ppc-spapr-uv-hcalls.txt
> > > > >
> > > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com
> > > > > ---
> > > > > hw/ppc/Makefile.objs | 1 +
> > > > > hw/ppc/spapr.c | 27 ++++++++
> > > > > hw/ppc/spapr_hcall_tpm.c | 135 +++++++++++++++++++++++++++++++++++++++
> > > > > hw/ppc/trace-events | 4 ++
> > > > > include/hw/ppc/spapr.h | 7 +-
> > > > > 5 files changed, 173 insertions(+), 1 deletion(-)
> > > > > create mode 100644 hw/ppc/spapr_hcall_tpm.c
> > > > >
> > > > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > > > index 9da93af905..5aa120cae6 100644
> > > > > --- a/hw/ppc/Makefile.objs
> > > > > +++ b/hw/ppc/Makefile.objs
> > > > > @@ -5,6 +5,7 @@ obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
> > > > > obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > > > > obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o
> > > > > obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
> > > > > +obj-$(CONFIG_PSERIES) += spapr_hcall_tpm.o
> > > > > obj-$(CONFIG_SPAPR_RNG) += spapr_rng.o
> > > > > # IBM PowerNV
> > > > > obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o pnv_occ.o pnv_bmc.o
> > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > index 821f0d4a49..eb3421673b 100644
> > > > > --- a/hw/ppc/spapr.c
> > > > > +++ b/hw/ppc/spapr.c
> > > > > @@ -1776,6 +1776,10 @@ static void spapr_machine_reset(MachineState *machine)
> > > > > */
> > > > > object_child_foreach_recursive(object_get_root(), spapr_reset_drcs, NULL);
> > > > >
> > > > > + if (spapr->tpm_device_file) {
> > > > > + spapr_hcall_tpm_reset();
> > > > > + }
> > > > > +
> > > > > spapr_clear_pending_events(spapr);
> > > > >
> > > > > /*
> > > > > @@ -3340,6 +3344,21 @@ static void spapr_set_host_serial(Object *obj, const char *value, Error **errp)
> > > > > spapr->host_serial = g_strdup(value);
> > > > > }
> > > > >
> > > > > +static char *spapr_get_tpm_device_file(Object *obj, Error **errp)
> > > > > +{
> > > > > + SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > > > +
> > > > > + return g_strdup(spapr->tpm_device_file);
> > > > > +}
> > > > > +
> > > > > +static void spapr_set_tpm_device_file(Object *obj, const char *value, Error **errp)
> > > > > +{
> > > > > + SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > > > +
> > > > > + g_free(spapr->tpm_device_file);
> > > > > + spapr->tpm_device_file = g_strdup(value);
> > > > > +}
> > > > > +
> > > > > static void spapr_instance_init(Object *obj)
> > > > > {
> > > > > SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > > > @@ -3396,6 +3415,14 @@ static void spapr_instance_init(Object *obj)
> > > > > &error_abort);
> > > > > object_property_set_description(obj, "host-serial",
> > > > > "Host serial number to advertise in guest device tree", &error_abort);
> > > > > + object_property_add_str(obj, "tpm-device-file",
> > > > > + spapr_get_tpm_device_file,
> > > > > + spapr_set_tpm_device_file, &error_abort);
> > > > > + object_property_set_description(obj, "tpm-device-file",
> > > > > + "Specifies the path to the TPM character device file to use"
> > > > > + " for TPM communication via hypercalls (usually a TPM"
> > > > > + " resource manager)",
> > > > > + &error_abort);
> > > > > }
> > > > >
> > > > > static void spapr_machine_finalizefn(Object *obj)
> > > > > diff --git a/hw/ppc/spapr_hcall_tpm.c b/hw/ppc/spapr_hcall_tpm.c
> > > > > new file mode 100644
> > > > > index 0000000000..75e2b6d594
> > > > > --- /dev/null
> > > > > +++ b/hw/ppc/spapr_hcall_tpm.c
> > > > > @@ -0,0 +1,135 @@
> > > > > +/*
> > > > > + * SPAPR TPM Hypercall
> > > > > + *
> > > > > + * Copyright IBM Corp. 2019
> > > > > + *
> > > > > + * Authors:
> > > > > + * Michael Roth <mdroth@linux.vnet.ibm.com>
> > > > > + *
> > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > + * See the COPYING file in the top-level directory.
> > > > > + */
> > > > > +
> > > > > +#include "qemu/osdep.h"
> > > > > +#include "qemu-common.h"
> > > > > +#include "qapi/error.h"
> > > > > +#include "qemu/error-report.h"
> > > > > +#include "cpu.h"
> > > > > +#include "hw/ppc/spapr.h"
> > > > > +#include "trace.h"
> > > > > +
> > > > > +#define TPM_SPAPR_BUFSIZE 4096
> > > > > +
> > > > > +enum {
> > > > > + TPM_COMM_OP_EXECUTE = 1,
> > > > > + TPM_COMM_OP_CLOSE_SESSION = 2,
> > > > > +};
> > > > > +
> > > > > +static int tpm_devfd = -1;
> > > >
> > > > A global? Really? You can do better.
> > > >
> > > > > +
> > > > > +static ssize_t tpm_execute(SpaprMachineState *spapr, target_ulong *args)
> > > > > +{
> > > > > + uint64_t data_in = ppc64_phys_to_real(args[1]);
> > > > > + target_ulong data_in_size = args[2];
> > > > > + uint64_t data_out = ppc64_phys_to_real(args[3]);
> > > > > + target_ulong data_out_size = args[4];
> > > > > + uint8_t buf_in[TPM_SPAPR_BUFSIZE];
> > > > > + uint8_t buf_out[TPM_SPAPR_BUFSIZE];
> > > > > + ssize_t ret;
> > > > > +
> > > > > + trace_spapr_tpm_execute(data_in, data_in_size, data_out, data_out_size);
> > > > > +
> > > > > + if (data_in_size > TPM_SPAPR_BUFSIZE) {
> > > > > + error_report("invalid TPM input buffer size: " TARGET_FMT_lu "\n",
> > > > > + data_in_size);
> > > > > + return H_P3;
> > > > > + }
> > > > > +
> > > > > + if (data_out_size < TPM_SPAPR_BUFSIZE) {
> > > > > + error_report("invalid TPM output buffer size: " TARGET_FMT_lu "\n",
> > > > > + data_out_size);
> > > > > + return H_P5;
> > > > > + }
> > > > > +
> > > > > + if (tpm_devfd == -1) {
> > > > > + tpm_devfd = open(spapr->tpm_device_file, O_RDWR);
> > > > > + if (tpm_devfd == -1) {
> > > > > + error_report("failed to open TPM device %s: %d",
> > > > > + spapr->tpm_device_file, errno);
> > > > > + return H_RESOURCE;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + cpu_physical_memory_read(data_in, buf_in, data_in_size);
> > > > > +
> > > > > + do {
> > > > > + ret = write(tpm_devfd, buf_in, data_in_size);
> > > > > + if (ret > 0) {
> > > > > + data_in_size -= ret;
> > > > > + }
> > > > > + } while ((ret >= 0 && data_in_size > 0) || (ret == -1 && errno == EINTR));
> > > > > +
> > > > > + if (ret == -1) {
> > > > > + error_report("failed to write to TPM device %s: %d",
> > > > > + spapr->tpm_device_file, errno);
> > > > > + return H_RESOURCE;
> > > > > + }
> > > > > +
> > > > > + do {
> > > > > + ret = read(tpm_devfd, buf_out, data_out_size);
> > > > > + } while (ret == 0 || (ret == -1 && errno == EINTR));
> > > > > +
> > > > > + if (ret == -1) {
> > > > > + error_report("failed to read from TPM device %s: %d",
> > > > > + spapr->tpm_device_file, errno);
> > > > > + return H_RESOURCE;
> > > > > + }
> > > > > +
> > > > > + cpu_physical_memory_write(data_out, buf_out, ret);
> > > > > + args[0] = ret;
> > > > > +
> > > > > + return H_SUCCESS;
> > > > > +}
> > > > > +
> > > > > +static target_ulong h_tpm_comm(PowerPCCPU *cpu,
> > > > > + SpaprMachineState *spapr,
> > > > > + target_ulong opcode,
> > > > > + target_ulong *args)
> > > > > +{
> > > > > + target_ulong op = args[0];
> > > > > +
> > > > > + trace_spapr_h_tpm_comm(spapr->tpm_device_file ?: "null", op);
> > > > > +
> > > > > + if (!spapr->tpm_device_file) {
> > > > > + error_report("TPM device not specified");
> > > > > + return H_RESOURCE;
> > > > > + }
> > > > > +
> > > > > + switch (op) {
> > > > > + case TPM_COMM_OP_EXECUTE:
> > > > > + return tpm_execute(spapr, args);
> > > > > + case TPM_COMM_OP_CLOSE_SESSION:
> > > > > + if (tpm_devfd != -1) {
> > > > > + close(tpm_devfd);
> > > > > + tpm_devfd = -1;
> > > > > + }
> > > > > + return H_SUCCESS;
> > > > > + default:
> > > > > + return H_PARAMETER;
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +void spapr_hcall_tpm_reset(void)
> > > > > +{
> > > > > + if (tpm_devfd != -1) {
> > > > > + close(tpm_devfd);
> > > > > + tpm_devfd = -1;
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +static void hypercall_register_types(void)
> > > > > +{
> > > > > + spapr_register_hypercall(H_TPM_COMM, h_tpm_comm);
> > > > > +}
> > > > > +
> > > > > +type_init(hypercall_register_types)
> > > > > diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> > > > > index f76448f532..96dad767a1 100644
> > > > > --- a/hw/ppc/trace-events
> > > > > +++ b/hw/ppc/trace-events
> > > > > @@ -25,6 +25,10 @@ spapr_update_dt(unsigned cb) "New blob %u bytes"
> > > > > spapr_update_dt_failed_size(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > > > > spapr_update_dt_failed_check(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > > > >
> > > > > +# spapr_hcall_tpm.c
> > > > > +spapr_h_tpm_comm(const char *device_path, uint64_t operation) "tpm_device_path=%s operation=0x%"PRIu64
> > > > > +spapr_tpm_execute(uint64_t data_in, uint64_t data_in_sz, uint64_t data_out, uint64_t data_out_sz) "data_in=0x%"PRIx64", data_in_sz=%"PRIu64", data_out=0x%"PRIx64", data_out_sz=%"PRIu64
> > > > > +
> > > > > # spapr_iommu.c
> > > > > spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
> > > > > spapr_iommu_get(uint64_t liobn, uint64_t ioba, uint64_t ret, uint64_t tce) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" ret=%"PRId64" tce=0x%"PRIx64
> > > > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > > > index 60553d32c4..7bd47575d7 100644
> > > > > --- a/include/hw/ppc/spapr.h
> > > > > +++ b/include/hw/ppc/spapr.h
> > > > > @@ -198,6 +198,7 @@ struct SpaprMachineState {
> > > > > SpaprXive *xive;
> > > > > SpaprIrq *irq;
> > > > > qemu_irq *qirqs;
> > > > > + char *tpm_device_file;
> > > > >
> > > > > bool cmd_line_caps[SPAPR_CAP_NUM];
> > > > > SpaprCapabilities def, eff, mig;
> > > > > @@ -490,8 +491,9 @@ struct SpaprMachineState {
> > > > > #define H_INT_ESB 0x3C8
> > > > > #define H_INT_SYNC 0x3CC
> > > > > #define H_INT_RESET 0x3D0
> > > > > +#define H_TPM_COMM 0xEF10
> > > > >
> > > > > -#define MAX_HCALL_OPCODE H_INT_RESET
> > > > > +#define MAX_HCALL_OPCODE H_TPM_COMM
> > > > >
> > > > > /* The hcalls above are standardized in PAPR and implemented by pHyp
> > > > > * as well.
> > > > > @@ -864,6 +866,9 @@ int spapr_caps_post_migration(SpaprMachineState *spapr);
> > > > >
> > > > > void spapr_check_pagesize(SpaprMachineState *spapr, hwaddr pagesize,
> > > > > Error **errp);
> > > > > +
> > > > > +void spapr_hcall_tpm_reset(void);
> > > > > +
> > > > > /*
> > > > > * XIVE definitions
> > > > > */
> > > >
> > >
> >
>
--
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] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall
2019-07-17 1:29 ` David Gibson
@ 2019-07-17 20:53 ` Michael Roth
2019-07-18 1:53 ` David Gibson
0 siblings, 1 reply; 16+ messages in thread
From: Michael Roth @ 2019-07-17 20:53 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, linuxram, qemu-devel
Quoting David Gibson (2019-07-16 20:29:12)
> On Tue, Jul 16, 2019 at 11:30:01AM -0500, Michael Roth wrote:
> > Quoting David Gibson (2019-07-14 21:25:24)
> > > On Fri, Jul 12, 2019 at 09:34:46AM -0500, Michael Roth wrote:
> > > > Quoting David Gibson (2019-07-12 01:46:19)
> > > > > On Thu, Jul 11, 2019 at 08:19:34PM -0500, Michael Roth wrote:
> > > > > > This implements the H_TPM_COMM hypercall, which is used by an
> > > > > > Ultravisor to pass TPM commands directly to the host's TPM device, or
> > > > > > a TPM Resource Manager associated with the device.
> > > > > >
> > > > > > This also introduces a new pseries machine option which is used to
> > > > > > configure what TPM device to pass commands to, for example:
> > > > > >
> > > > > > -machine pseries,...,tpm-device-file=/dev/tmprm0
> > > > >
> > > > > Bolting this into yet another machine parameter seems kind of ugly.
> > > > > Wouldn't it make more sense to treat this as an virtual device (say
> > > > > "spapr-vtpm"). Adding that device would enable the hcall, and would
> > > > > have properties for the back end host device.
> > > >
> > > > That does sound nicer.
> > > >
> > > > Originally I had SpaprMachineClass implement the TYPE_TPM_IF interface so
> > > > we could define a TPM backend via -tpmdev passthrough,path=..., but after
> > > > some discussion with the TPM maintainer it didn't quite work for the main
> > > > use-case of passing through a TPM Resource Manager since it isn't suitable
> > > > for full vTPM front-ends (since multiple guests can interfere with each
> > > > other's operations when running the full gamut of TPM functionality).
> > > >
> > > > I hadn't consider a stand-alone -device implementation though. It's not
> > > > a proper VIO or PCI device so there's no proper bus to attach it to. I
> > > > guess we would just make it a direct child of SpaprMachineState (sort
> > > > of like SpaprDrcClass), then we could define it via something like
> > > > -object spapr-tpm-proxy,path=....
> > >
> > > It should be -device not -object, but otherwise that looks ok.
> >
> > Ok, for some reason I thought -device needed either a specific bus or
> > needed to be a SysBusDevice to attach to main-system-bus, but maybe that
> > was just for qdev-managed reset handling. I've re-worked the series to
> > allow configuration via:
> >
> > -device spapr-tpm-proxy,host_path=/dev/tpmrmX
>
> That looks good.
>
> > > How does the TPM appear in the device tree?
> >
> > Nothing in the guest, on the host it appears as:
>
> Hrm. That seems unwise. I mean, I guess its being treated as a
> hypervisor facility rather than a device per se, but what if we ever
> need to advertise more metadata about it.
It's a little bit awkward using a device tree in this case since it's
generally the ultravisor that will be making this hcall on behalf of
a guest requesting switch-over to SVM mode. The TPM device itself has
a GetCapabilities command that seems like it would cover most of the
metadata we might need though:
https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-3-Commands-01.38.pdf
(page 340)
and if we need to add a layer of metadata on top of that there's also
the option of introducing support for an additional operation in
H_TPM_COMM itself, e.g. TPM_COMM_OP_GET_CAPABILITIES. Unsupported or
invalid operations have a unique H_PARAMETER return code so callers
should be able to reliably probe for it in the future if they need
more information.
>
> > ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57
> > ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/link-id
> > ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/linux,sml-size
> > ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/label
> > ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/compatible
> > ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/status
> > ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/reg
> > ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/phandle
> > ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/linux,sml-base
> > ./xscom@603fc00000000/i2cm@a2000/i2c-bus@0/tpm@57/name
> >
> > >
> > > > I'll go ahead and give that a shot, assuming it seems reasonable to you.
> > > >
> > > > >
> > > > > > By default, no tpm-device-file is defined and hcalls will return
> > > > > > H_RESOURCE.
> > > > >
> > > > > Wouldn't H_FUNCTION make more sense?
> > > >
> > > > Yes, for this case it probably would.
> > > >
> > > > Thanks for the suggestions!
> > > >
> > > > >
> > > > > >
> > > > > > The full specification for this hypercall can be found in
> > > > > > docs/specs/ppc-spapr-uv-hcalls.txt
> > > > > >
> > > > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com
> > > > > > ---
> > > > > > hw/ppc/Makefile.objs | 1 +
> > > > > > hw/ppc/spapr.c | 27 ++++++++
> > > > > > hw/ppc/spapr_hcall_tpm.c | 135 +++++++++++++++++++++++++++++++++++++++
> > > > > > hw/ppc/trace-events | 4 ++
> > > > > > include/hw/ppc/spapr.h | 7 +-
> > > > > > 5 files changed, 173 insertions(+), 1 deletion(-)
> > > > > > create mode 100644 hw/ppc/spapr_hcall_tpm.c
> > > > > >
> > > > > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > > > > index 9da93af905..5aa120cae6 100644
> > > > > > --- a/hw/ppc/Makefile.objs
> > > > > > +++ b/hw/ppc/Makefile.objs
> > > > > > @@ -5,6 +5,7 @@ obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
> > > > > > obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > > > > > obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o
> > > > > > obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o spapr_irq.o
> > > > > > +obj-$(CONFIG_PSERIES) += spapr_hcall_tpm.o
> > > > > > obj-$(CONFIG_SPAPR_RNG) += spapr_rng.o
> > > > > > # IBM PowerNV
> > > > > > obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o pnv_core.o pnv_lpc.o pnv_psi.o pnv_occ.o pnv_bmc.o
> > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > index 821f0d4a49..eb3421673b 100644
> > > > > > --- a/hw/ppc/spapr.c
> > > > > > +++ b/hw/ppc/spapr.c
> > > > > > @@ -1776,6 +1776,10 @@ static void spapr_machine_reset(MachineState *machine)
> > > > > > */
> > > > > > object_child_foreach_recursive(object_get_root(), spapr_reset_drcs, NULL);
> > > > > >
> > > > > > + if (spapr->tpm_device_file) {
> > > > > > + spapr_hcall_tpm_reset();
> > > > > > + }
> > > > > > +
> > > > > > spapr_clear_pending_events(spapr);
> > > > > >
> > > > > > /*
> > > > > > @@ -3340,6 +3344,21 @@ static void spapr_set_host_serial(Object *obj, const char *value, Error **errp)
> > > > > > spapr->host_serial = g_strdup(value);
> > > > > > }
> > > > > >
> > > > > > +static char *spapr_get_tpm_device_file(Object *obj, Error **errp)
> > > > > > +{
> > > > > > + SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > > > > +
> > > > > > + return g_strdup(spapr->tpm_device_file);
> > > > > > +}
> > > > > > +
> > > > > > +static void spapr_set_tpm_device_file(Object *obj, const char *value, Error **errp)
> > > > > > +{
> > > > > > + SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > > > > +
> > > > > > + g_free(spapr->tpm_device_file);
> > > > > > + spapr->tpm_device_file = g_strdup(value);
> > > > > > +}
> > > > > > +
> > > > > > static void spapr_instance_init(Object *obj)
> > > > > > {
> > > > > > SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> > > > > > @@ -3396,6 +3415,14 @@ static void spapr_instance_init(Object *obj)
> > > > > > &error_abort);
> > > > > > object_property_set_description(obj, "host-serial",
> > > > > > "Host serial number to advertise in guest device tree", &error_abort);
> > > > > > + object_property_add_str(obj, "tpm-device-file",
> > > > > > + spapr_get_tpm_device_file,
> > > > > > + spapr_set_tpm_device_file, &error_abort);
> > > > > > + object_property_set_description(obj, "tpm-device-file",
> > > > > > + "Specifies the path to the TPM character device file to use"
> > > > > > + " for TPM communication via hypercalls (usually a TPM"
> > > > > > + " resource manager)",
> > > > > > + &error_abort);
> > > > > > }
> > > > > >
> > > > > > static void spapr_machine_finalizefn(Object *obj)
> > > > > > diff --git a/hw/ppc/spapr_hcall_tpm.c b/hw/ppc/spapr_hcall_tpm.c
> > > > > > new file mode 100644
> > > > > > index 0000000000..75e2b6d594
> > > > > > --- /dev/null
> > > > > > +++ b/hw/ppc/spapr_hcall_tpm.c
> > > > > > @@ -0,0 +1,135 @@
> > > > > > +/*
> > > > > > + * SPAPR TPM Hypercall
> > > > > > + *
> > > > > > + * Copyright IBM Corp. 2019
> > > > > > + *
> > > > > > + * Authors:
> > > > > > + * Michael Roth <mdroth@linux.vnet.ibm.com>
> > > > > > + *
> > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > + * See the COPYING file in the top-level directory.
> > > > > > + */
> > > > > > +
> > > > > > +#include "qemu/osdep.h"
> > > > > > +#include "qemu-common.h"
> > > > > > +#include "qapi/error.h"
> > > > > > +#include "qemu/error-report.h"
> > > > > > +#include "cpu.h"
> > > > > > +#include "hw/ppc/spapr.h"
> > > > > > +#include "trace.h"
> > > > > > +
> > > > > > +#define TPM_SPAPR_BUFSIZE 4096
> > > > > > +
> > > > > > +enum {
> > > > > > + TPM_COMM_OP_EXECUTE = 1,
> > > > > > + TPM_COMM_OP_CLOSE_SESSION = 2,
> > > > > > +};
> > > > > > +
> > > > > > +static int tpm_devfd = -1;
> > > > >
> > > > > A global? Really? You can do better.
> > > > >
> > > > > > +
> > > > > > +static ssize_t tpm_execute(SpaprMachineState *spapr, target_ulong *args)
> > > > > > +{
> > > > > > + uint64_t data_in = ppc64_phys_to_real(args[1]);
> > > > > > + target_ulong data_in_size = args[2];
> > > > > > + uint64_t data_out = ppc64_phys_to_real(args[3]);
> > > > > > + target_ulong data_out_size = args[4];
> > > > > > + uint8_t buf_in[TPM_SPAPR_BUFSIZE];
> > > > > > + uint8_t buf_out[TPM_SPAPR_BUFSIZE];
> > > > > > + ssize_t ret;
> > > > > > +
> > > > > > + trace_spapr_tpm_execute(data_in, data_in_size, data_out, data_out_size);
> > > > > > +
> > > > > > + if (data_in_size > TPM_SPAPR_BUFSIZE) {
> > > > > > + error_report("invalid TPM input buffer size: " TARGET_FMT_lu "\n",
> > > > > > + data_in_size);
> > > > > > + return H_P3;
> > > > > > + }
> > > > > > +
> > > > > > + if (data_out_size < TPM_SPAPR_BUFSIZE) {
> > > > > > + error_report("invalid TPM output buffer size: " TARGET_FMT_lu "\n",
> > > > > > + data_out_size);
> > > > > > + return H_P5;
> > > > > > + }
> > > > > > +
> > > > > > + if (tpm_devfd == -1) {
> > > > > > + tpm_devfd = open(spapr->tpm_device_file, O_RDWR);
> > > > > > + if (tpm_devfd == -1) {
> > > > > > + error_report("failed to open TPM device %s: %d",
> > > > > > + spapr->tpm_device_file, errno);
> > > > > > + return H_RESOURCE;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + cpu_physical_memory_read(data_in, buf_in, data_in_size);
> > > > > > +
> > > > > > + do {
> > > > > > + ret = write(tpm_devfd, buf_in, data_in_size);
> > > > > > + if (ret > 0) {
> > > > > > + data_in_size -= ret;
> > > > > > + }
> > > > > > + } while ((ret >= 0 && data_in_size > 0) || (ret == -1 && errno == EINTR));
> > > > > > +
> > > > > > + if (ret == -1) {
> > > > > > + error_report("failed to write to TPM device %s: %d",
> > > > > > + spapr->tpm_device_file, errno);
> > > > > > + return H_RESOURCE;
> > > > > > + }
> > > > > > +
> > > > > > + do {
> > > > > > + ret = read(tpm_devfd, buf_out, data_out_size);
> > > > > > + } while (ret == 0 || (ret == -1 && errno == EINTR));
> > > > > > +
> > > > > > + if (ret == -1) {
> > > > > > + error_report("failed to read from TPM device %s: %d",
> > > > > > + spapr->tpm_device_file, errno);
> > > > > > + return H_RESOURCE;
> > > > > > + }
> > > > > > +
> > > > > > + cpu_physical_memory_write(data_out, buf_out, ret);
> > > > > > + args[0] = ret;
> > > > > > +
> > > > > > + return H_SUCCESS;
> > > > > > +}
> > > > > > +
> > > > > > +static target_ulong h_tpm_comm(PowerPCCPU *cpu,
> > > > > > + SpaprMachineState *spapr,
> > > > > > + target_ulong opcode,
> > > > > > + target_ulong *args)
> > > > > > +{
> > > > > > + target_ulong op = args[0];
> > > > > > +
> > > > > > + trace_spapr_h_tpm_comm(spapr->tpm_device_file ?: "null", op);
> > > > > > +
> > > > > > + if (!spapr->tpm_device_file) {
> > > > > > + error_report("TPM device not specified");
> > > > > > + return H_RESOURCE;
> > > > > > + }
> > > > > > +
> > > > > > + switch (op) {
> > > > > > + case TPM_COMM_OP_EXECUTE:
> > > > > > + return tpm_execute(spapr, args);
> > > > > > + case TPM_COMM_OP_CLOSE_SESSION:
> > > > > > + if (tpm_devfd != -1) {
> > > > > > + close(tpm_devfd);
> > > > > > + tpm_devfd = -1;
> > > > > > + }
> > > > > > + return H_SUCCESS;
> > > > > > + default:
> > > > > > + return H_PARAMETER;
> > > > > > + }
> > > > > > +}
> > > > > > +
> > > > > > +void spapr_hcall_tpm_reset(void)
> > > > > > +{
> > > > > > + if (tpm_devfd != -1) {
> > > > > > + close(tpm_devfd);
> > > > > > + tpm_devfd = -1;
> > > > > > + }
> > > > > > +}
> > > > > > +
> > > > > > +static void hypercall_register_types(void)
> > > > > > +{
> > > > > > + spapr_register_hypercall(H_TPM_COMM, h_tpm_comm);
> > > > > > +}
> > > > > > +
> > > > > > +type_init(hypercall_register_types)
> > > > > > diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> > > > > > index f76448f532..96dad767a1 100644
> > > > > > --- a/hw/ppc/trace-events
> > > > > > +++ b/hw/ppc/trace-events
> > > > > > @@ -25,6 +25,10 @@ spapr_update_dt(unsigned cb) "New blob %u bytes"
> > > > > > spapr_update_dt_failed_size(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > > > > > spapr_update_dt_failed_check(unsigned cbold, unsigned cbnew, unsigned magic) "Old blob %u bytes, new blob %u bytes, magic 0x%x"
> > > > > >
> > > > > > +# spapr_hcall_tpm.c
> > > > > > +spapr_h_tpm_comm(const char *device_path, uint64_t operation) "tpm_device_path=%s operation=0x%"PRIu64
> > > > > > +spapr_tpm_execute(uint64_t data_in, uint64_t data_in_sz, uint64_t data_out, uint64_t data_out_sz) "data_in=0x%"PRIx64", data_in_sz=%"PRIu64", data_out=0x%"PRIx64", data_out_sz=%"PRIu64
> > > > > > +
> > > > > > # spapr_iommu.c
> > > > > > spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
> > > > > > spapr_iommu_get(uint64_t liobn, uint64_t ioba, uint64_t ret, uint64_t tce) "liobn=0x%"PRIx64" ioba=0x%"PRIx64" ret=%"PRId64" tce=0x%"PRIx64
> > > > > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > > > > index 60553d32c4..7bd47575d7 100644
> > > > > > --- a/include/hw/ppc/spapr.h
> > > > > > +++ b/include/hw/ppc/spapr.h
> > > > > > @@ -198,6 +198,7 @@ struct SpaprMachineState {
> > > > > > SpaprXive *xive;
> > > > > > SpaprIrq *irq;
> > > > > > qemu_irq *qirqs;
> > > > > > + char *tpm_device_file;
> > > > > >
> > > > > > bool cmd_line_caps[SPAPR_CAP_NUM];
> > > > > > SpaprCapabilities def, eff, mig;
> > > > > > @@ -490,8 +491,9 @@ struct SpaprMachineState {
> > > > > > #define H_INT_ESB 0x3C8
> > > > > > #define H_INT_SYNC 0x3CC
> > > > > > #define H_INT_RESET 0x3D0
> > > > > > +#define H_TPM_COMM 0xEF10
> > > > > >
> > > > > > -#define MAX_HCALL_OPCODE H_INT_RESET
> > > > > > +#define MAX_HCALL_OPCODE H_TPM_COMM
> > > > > >
> > > > > > /* The hcalls above are standardized in PAPR and implemented by pHyp
> > > > > > * as well.
> > > > > > @@ -864,6 +866,9 @@ int spapr_caps_post_migration(SpaprMachineState *spapr);
> > > > > >
> > > > > > void spapr_check_pagesize(SpaprMachineState *spapr, hwaddr pagesize,
> > > > > > Error **errp);
> > > > > > +
> > > > > > +void spapr_hcall_tpm_reset(void);
> > > > > > +
> > > > > > /*
> > > > > > * XIVE definitions
> > > > > > */
> > > > >
> > > >
> > >
> >
>
> --
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall
2019-07-17 20:53 ` Michael Roth
@ 2019-07-18 1:53 ` David Gibson
0 siblings, 0 replies; 16+ messages in thread
From: David Gibson @ 2019-07-18 1:53 UTC (permalink / raw)
To: Michael Roth; +Cc: qemu-ppc, linuxram, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3966 bytes --]
On Wed, Jul 17, 2019 at 03:53:47PM -0500, Michael Roth wrote:
> Quoting David Gibson (2019-07-16 20:29:12)
> > On Tue, Jul 16, 2019 at 11:30:01AM -0500, Michael Roth wrote:
> > > Quoting David Gibson (2019-07-14 21:25:24)
> > > > On Fri, Jul 12, 2019 at 09:34:46AM -0500, Michael Roth wrote:
> > > > > Quoting David Gibson (2019-07-12 01:46:19)
> > > > > > On Thu, Jul 11, 2019 at 08:19:34PM -0500, Michael Roth wrote:
> > > > > > > This implements the H_TPM_COMM hypercall, which is used by an
> > > > > > > Ultravisor to pass TPM commands directly to the host's TPM device, or
> > > > > > > a TPM Resource Manager associated with the device.
> > > > > > >
> > > > > > > This also introduces a new pseries machine option which is used to
> > > > > > > configure what TPM device to pass commands to, for example:
> > > > > > >
> > > > > > > -machine pseries,...,tpm-device-file=/dev/tmprm0
> > > > > >
> > > > > > Bolting this into yet another machine parameter seems kind of ugly.
> > > > > > Wouldn't it make more sense to treat this as an virtual device (say
> > > > > > "spapr-vtpm"). Adding that device would enable the hcall, and would
> > > > > > have properties for the back end host device.
> > > > >
> > > > > That does sound nicer.
> > > > >
> > > > > Originally I had SpaprMachineClass implement the TYPE_TPM_IF interface so
> > > > > we could define a TPM backend via -tpmdev passthrough,path=..., but after
> > > > > some discussion with the TPM maintainer it didn't quite work for the main
> > > > > use-case of passing through a TPM Resource Manager since it isn't suitable
> > > > > for full vTPM front-ends (since multiple guests can interfere with each
> > > > > other's operations when running the full gamut of TPM functionality).
> > > > >
> > > > > I hadn't consider a stand-alone -device implementation though. It's not
> > > > > a proper VIO or PCI device so there's no proper bus to attach it to. I
> > > > > guess we would just make it a direct child of SpaprMachineState (sort
> > > > > of like SpaprDrcClass), then we could define it via something like
> > > > > -object spapr-tpm-proxy,path=....
> > > >
> > > > It should be -device not -object, but otherwise that looks ok.
> > >
> > > Ok, for some reason I thought -device needed either a specific bus or
> > > needed to be a SysBusDevice to attach to main-system-bus, but maybe that
> > > was just for qdev-managed reset handling. I've re-worked the series to
> > > allow configuration via:
> > >
> > > -device spapr-tpm-proxy,host_path=/dev/tpmrmX
> >
> > That looks good.
> >
> > > > How does the TPM appear in the device tree?
> > >
> > > Nothing in the guest, on the host it appears as:
> >
> > Hrm. That seems unwise. I mean, I guess its being treated as a
> > hypervisor facility rather than a device per se, but what if we ever
> > need to advertise more metadata about it.
>
> It's a little bit awkward using a device tree in this case since it's
> generally the ultravisor that will be making this hcall on behalf of
> a guest requesting switch-over to SVM mode. The TPM device itself has
> a GetCapabilities command that seems like it would cover most of the
> metadata we might need though:
>
> https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-3-Commands-01.38.pdf
> (page 340)
>
> and if we need to add a layer of metadata on top of that there's also
> the option of introducing support for an additional operation in
> H_TPM_COMM itself, e.g. TPM_COMM_OP_GET_CAPABILITIES. Unsupported or
> invalid operations have a unique H_PARAMETER return code so callers
> should be able to reliably probe for it in the future if they need
> more information.
Yeah, ok.
--
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] 16+ messages in thread
end of thread, other threads:[~2019-07-18 3:50 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-12 1:19 [Qemu-devel] [RFC PATCH 0/2] spapr: Implement H_TPM_COMM for accessing host TPM device Michael Roth
2019-07-12 1:19 ` [Qemu-devel] [PATCH 1/2] docs/specs: initial spec summary for Ultravisor-related hcalls Michael Roth
2019-07-12 6:40 ` David Gibson
2019-07-12 15:13 ` Michael Roth
2019-07-15 2:25 ` David Gibson
2019-07-16 16:25 ` Michael Roth
2019-07-12 1:19 ` [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall Michael Roth
2019-07-12 6:46 ` David Gibson
2019-07-12 14:34 ` Michael Roth
2019-07-15 2:25 ` David Gibson
2019-07-16 16:30 ` Michael Roth
2019-07-17 1:29 ` David Gibson
2019-07-17 20:53 ` Michael Roth
2019-07-18 1:53 ` David Gibson
2019-07-12 6:40 ` [Qemu-devel] [RFC PATCH 0/2] spapr: Implement H_TPM_COMM for accessing host TPM device David Gibson
2019-07-12 15:33 ` no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.