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

[David Gibson]

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 <address@hidden
> > > > > ---
> > > > >  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      <address@hidden>
> > > > > + *
> > > > > + * 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

signature.asc
Description: PGP signature