[Qemu-devel] [RFC PATCH 0/2] spapr: Implement H_TPM_COMM for accessing host TPM device

[Qemu-devel] [RFC PATCH 0/2] spapr: Implement H_TPM_COMM for accessing host TPM device

[Michael Roth]

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(-)

[Qemu-devel] [PATCH 1/2] docs/specs: initial spec summary for Ultravisor-related hcalls

[Michael Roth]

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

Re: [Qemu-devel] [PATCH 1/2] docs/specs: initial spec summary for Ultravisor-related hcalls

[David Gibson]

[-- 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 --]

Re: [Qemu-devel] [PATCH 1/2] docs/specs: initial spec summary for Ultravisor-related hcalls

[Michael Roth]

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

Re: [Qemu-devel] [PATCH 1/2] docs/specs: initial spec summary for Ultravisor-related hcalls

[David Gibson]

[-- 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 --]

Re: [Qemu-devel] [PATCH 1/2] docs/specs: initial spec summary for Ultravisor-related hcalls

[Michael Roth]

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

[Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall

[Michael Roth]

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

Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall

[David Gibson]

[-- 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 --]

Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall

[Michael Roth]

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

Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall

[David Gibson]

[-- 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 --]

Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall

[Michael Roth]

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

Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall

[David Gibson]

[-- 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 --]

Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall

[Michael Roth]

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

Re: [Qemu-devel] [PATCH 2/2] spapr: initial implementation for H_TPM_COMM hcall

[David Gibson]

[-- 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 --]

Re: [Qemu-devel] [RFC PATCH 0/2] spapr: Implement H_TPM_COMM for accessing host TPM device

[David Gibson]

[-- 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 --]

Re: [Qemu-devel] [RFC PATCH 0/2] spapr: Implement H_TPM_COMM for accessing host TPM device

[no-reply]

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