Daniel P. Berrangé <berrange@xxxxxxxxxx> writes: > On Wed, Sep 27, 2023 at 12:49:08PM -0400, James Bottomley wrote: >> From: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> >> >> The Microsoft Simulator (mssim) is the reference emulation platform >> for the TCG TPM 2.0 specification. >> >> https://github.com/Microsoft/ms-tpm-20-ref.git >> >> It exports a fairly simple network socket based protocol on two >> sockets, one for command (default 2321) and one for control (default >> 2322). This patch adds a simple backend that can speak the mssim >> protocol over the network. It also allows the two sockets to be >> specified on the command line. The benefits are twofold: firstly it >> gives us a backend that actually speaks a standard TPM emulation >> protocol instead of the linux specific TPM driver format of the >> current emulated TPM backend and secondly, using the microsoft >> protocol, the end point of the emulator can be anywhere on the >> network, facilitating the cloud use case where a central TPM service >> can be used over a control network. >> >> The implementation does basic control commands like power off/on, but >> doesn't implement cancellation or startup. The former because >> cancellation is pretty much useless on a fast operating TPM emulator >> and the latter because this emulator is designed to be used with OVMF >> which itself does TPM startup and I wanted to validate that. >> >> To run this, simply download an emulator based on the MS specification >> (package ibmswtpm2 on openSUSE) and run it, then add these two lines >> to the qemu command and it will use the emulator. >> >> -tpmdev mssim,id=tpm0 \ >> -device tpm-crb,tpmdev=tpm0 \ >> >> to use a remote emulator replace the first line with >> >> -tpmdev "{'type':'mssim','id':'tpm0','command':{'type':inet,'host':'remote','port':'2321'}}" >> >> tpm-tis also works as the backend. >> >> Signed-off-by: James Bottomley <jejb@xxxxxxxxxxxxx> >> Acked-by: Markus Armbruster <armbru@xxxxxxxxxx> [...] >> diff --git a/backends/tpm/tpm_mssim.c b/backends/tpm/tpm_mssim.c >> new file mode 100644 >> index 0000000000..b8a12dce04 >> --- /dev/null >> +++ b/backends/tpm/tpm_mssim.c >> @@ -0,0 +1,290 @@ >> +/* >> + * Emulator TPM driver which connects over the mssim protocol >> + * SPDX-License-Identifier: GPL-2.0-or-later >> + * >> + * Copyright (c) 2022 >> + * Author: James Bottomley <jejb@xxxxxxxxxxxxx> >> + */ >> + >> +#include "qemu/osdep.h" >> +#include "qemu/error-report.h" >> +#include "qemu/sockets.h" >> + >> +#include "qapi/clone-visitor.h" >> +#include "qapi/qapi-visit-tpm.h" >> + >> +#include "io/channel-socket.h" >> + >> +#include "sysemu/runstate.h" >> +#include "sysemu/tpm_backend.h" >> +#include "sysemu/tpm_util.h" >> + >> +#include "qom/object.h" >> + >> +#include "tpm_int.h" >> +#include "tpm_mssim.h" >> + >> +#define ERROR_PREFIX "TPM mssim Emulator: " >> + >> +#define TYPE_TPM_MSSIM "tpm-mssim" >> +OBJECT_DECLARE_SIMPLE_TYPE(TPMMssim, TPM_MSSIM) >> + >> +struct TPMMssim { >> + TPMBackend parent; >> + >> + TPMMssimOptions opts; >> + >> + QIOChannelSocket *cmd_qc, *ctrl_qc; >> +}; >> + >> +static int tpm_send_ctrl(TPMMssim *t, uint32_t cmd, Error **errp) >> +{ >> + int ret; >> + >> + qio_channel_socket_connect_sync(t->ctrl_qc, t->opts.control, errp); > > Need to assign to 'ret' and check for failure here, otherwise the > next call to write_all will overwrite the useful message in 'errp' > with a less helpful one. No, it'll crash :) An @errp argument must point to a null pointer. If it doesn't, setting an error will trip error_setv()'s assertion. > + cmd = htonl(cmd); > + ret = qio_channel_write_all(QIO_CHANNEL(t->ctrl_qc), > + (char *)&cmd, sizeof(cmd), errp); > + if (ret != 0) { > + goto out; > + } qapi/error.h's big comment advises: * Receive and accumulate multiple errors (first one wins): * Error *err = NULL, *local_err = NULL; * foo(arg, &err); * bar(arg, &local_err); * error_propagate(&err, local_err); * if (err) { * handle the error... * } * * Do *not* "optimize" this to * Error *err = NULL; * foo(arg, &err); * bar(arg, &err); // WRONG! * if (err) { * handle the error... * } * because this may pass a non-null err to bar(). * * Likewise, do *not* * Error *err = NULL; * if (cond1) { * error_setg(&err, ...); * } * if (cond2) { * error_setg(&err, ...); // WRONG! * } * because this may pass a non-null err to error_setg(). The quoted code is like the last example, except the error_setg() lurk within the functions called. [...]
