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 > > > > --- > > > > > 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 > > > > > + * > > > > > + * 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