Re: [PATCH v10 2/2] tpm: add backend for mssim
On 5/1/24 12:52, James Bottomley wrote: On Wed, 2024-05-01 at 12:31 -0400, Stefan Berger wrote: On 5/1/24 12:21, James Bottomley wrote: On Tue, 2024-04-30 at 17:12 -0400, Stefan Berger wrote: On 4/30/24 15:08, James Bottomley wrote: [...] +The mssim backend supports snapshotting and migration by not resetting I don't thing snapshotting is supported because snapshooting would require you to be able to set the state of the vTPM from the snapshot you started. I would remove the claim. I thought we established last time that it can definitely do both (and I've tested it because you asked me to). Snapshotting and migration are essentially the same thing, with snapshotting being easier because it can be done on the same host meaning the same command line parameters. If you migrate to a different host you need the socket to point back to the host serving the vTPM. To do this easily you simply keep the vTPM running while the VM is undergoing snapshot and migration. If you're thinking of and extended down time for the snapshot, then it's up to the vTPM implementation to store the state (or simply keep it running for an extended time doing nothing). Which part of the code injects the state into the vTPM so that it resumes with the state of the TPM (PCRs, NVRAM indices, keys, sessions etc.) from when the snapshot was taken? We've had this conversation before too: https://lore.kernel.org/qemu-devel/f928986fd4095b1f27c83ede96f3b0dd65ad965e.ca...@linux.ibm.com/T/#u But the synopsis is nothing does. The design is to be entirely independent of vTPM implementation: it will actually work with any TPM obeying the simulator IP protocol (MS reference, ibmswtpm2 or even your swtpm) but the price of this is that the user has to preserve the vTPM state, by whatever means they deem appropriate, independently of the VM snapshot image. Unless your backend can retrieve the state upon snapshot save and inject state into the vTPM upon snapshot resume, 'snapshotting' is not working (correctly). James
Re: [PATCH v10 2/2] tpm: add backend for mssim
On 5/1/24 12:21, James Bottomley wrote: On Tue, 2024-04-30 at 17:12 -0400, Stefan Berger wrote: On 4/30/24 15:08, James Bottomley wrote: [...] +The mssim backend supports snapshotting and migration by not resetting I don't thing snapshotting is supported because snapshooting would require you to be able to set the state of the vTPM from the snapshot you started. I would remove the claim. I thought we established last time that it can definitely do both (and I've tested it because you asked me to). Snapshotting and migration are essentially the same thing, with snapshotting being easier because it can be done on the same host meaning the same command line parameters. If you migrate to a different host you need the socket to point back to the host serving the vTPM. To do this easily you simply keep the vTPM running while the VM is undergoing snapshot and migration. If you're thinking of and extended down time for the snapshot, then it's up to the vTPM implementation to store the state (or simply keep it running for an extended time doing nothing). Which part of the code injects the state into the vTPM so that it resumes with the state of the TPM (PCRs, NVRAM indices, keys, sessions etc.) from when the snapshot was taken? Rest LGTM. Thanks! Regards, James
Re: [PATCH v10 0/2] tpm: add mssim backend
On 4/30/24 15:08, James Bottomley wrote: The requested feedback was to convert the tpmdev handler to being json based, which requires rethreading all the backends. The good news is this reduced quite a bit of code (especially as I converted it to error_fatal handling as well, which removes the return status threading). The bad news is I can't test any of the conversions. swtpm still isn't building on opensuse and, apparently, passthrough It does build and packages are available: - https://app.travis-ci.com/github/stefanberger/swtpm-distro-compile/jobs/621150390 - https://software.opensuse.org/package/swtpm doesn't like my native TPM because it doesn't allow cancellation. v3 pulls out more unneeded code in the visitor conversion, makes migration work on external state preservation of the simulator and adds documentation v4 puts back the wrapper options (but doesn't add any for mssim since it post dates the necessity) v5 rebases to the latest master branch and adjusts for removed use_FOO ptrs v5 updates help to exit zero; does some checkpatch tidying v7 merge review feedback and add acks. v8 adds better error handling, more code tidies and adds command socket disconnection/reconnection (instead of trying to keep the socket open the whole time). This adds overhead, but makes debugging guest kernel TPM issues much easier. v9 Fix merge conflict with optarg->optstr conversion v10 Fix more merge conflicts and update API versions James --- James Bottomley (2): tpm: convert tpmdev options processing to new visitor format tpm: add backend for mssim MAINTAINERS| 6 + backends/tpm/Kconfig | 5 + backends/tpm/meson.build | 1 + backends/tpm/tpm_emulator.c| 25 ++- backends/tpm/tpm_mssim.c | 319 + backends/tpm/tpm_mssim.h | 44 + backends/tpm/tpm_passthrough.c | 23 +-- docs/specs/tpm.rst | 39 include/sysemu/tpm.h | 5 +- include/sysemu/tpm_backend.h | 2 +- qapi/tpm.json | 50 +- system/tpm-hmp-cmds.c | 9 + system/tpm.c | 91 -- system/vl.c| 19 +- 14 files changed, 530 insertions(+), 108 deletions(-) create mode 100644 backends/tpm/tpm_mssim.c create mode 100644 backends/tpm/tpm_mssim.h
Re: [PATCH v10 2/2] tpm: add backend for mssim
On 4/30/24 15:08, James Bottomley wrote:
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
Acked-by: Markus Armbruster
---
v2: convert to SocketAddr json and use qio_channel_socket_connect_sync()
v3: gate control power off by migration state keep control socket disconnected
to test outside influence and add docs.
v7: TPMmssim -> TPMMssim; doc and json fixes
Make command socket open each time (makes OS debugging easier)
---
MAINTAINERS | 6 +
backends/tpm/Kconfig | 5 +
backends/tpm/meson.build | 1 +
backends/tpm/tpm_mssim.c | 319 +++
backends/tpm/tpm_mssim.h | 44 ++
docs/specs/tpm.rst | 39 +
qapi/tpm.json| 31 +++-
system/tpm-hmp-cmds.c| 9 ++
8 files changed, 450 insertions(+), 4 deletions(-)
create mode 100644 backends/tpm/tpm_mssim.c
create mode 100644 backends/tpm/tpm_mssim.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 302b6fd00c..6bd7e82d1b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3386,10 +3386,16 @@ F: include/hw/acpi/tpm.h
F: include/sysemu/tpm*
F: qapi/tpm.json
F: backends/tpm/
+X: backends/tpm/tpm_mssim.*
F: tests/qtest/*tpm*
F: docs/specs/tpm.rst
T: git https://github.com/stefanberger/qemu-tpm.git tpm-next
+MSSIM TPM Backend
+M: James Bottomley
+S: Maintained
+F: backends/tpm/tpm_mssim.*
+
Checkpatch
S: Odd Fixes
F: scripts/checkpatch.pl
diff --git a/backends/tpm/Kconfig b/backends/tpm/Kconfig
index 5d91eb89c2..d6d6fa53e9 100644
--- a/backends/tpm/Kconfig
+++ b/backends/tpm/Kconfig
@@ -12,3 +12,8 @@ config TPM_EMULATOR
bool
default y
depends on TPM_BACKEND
+
+config TPM_MSSIM
+bool
+default y
+depends on TPM_BACKEND
diff --git a/backends/tpm/meson.build b/backends/tpm/meson.build
index 0bfa6c422b..c6f7c24cb1 100644
--- a/backends/tpm/meson.build
+++ b/backends/tpm/meson.build
@@ -3,4 +3,5 @@ if have_tpm
system_ss.add(files('tpm_util.c'))
system_ss.add(when: 'CONFIG_TPM_PASSTHROUGH', if_true:
files('tpm_passthrough.c'))
system_ss.add(when: 'CONFIG_TPM_EMULATOR', if_true: files('tpm_emulator.c'))
+ system_ss.add(when: 'CONFIG_TPM_MSSIM', if_true: files('tpm_mssim.c'))
endif
diff --git a/backends/tpm/tpm_mssim.c b/backends/tpm/tpm_mssim.c
new file mode 100644
index 00..962ad340c3
--- /dev/null
+++ b/backends/tpm/tpm_mssim.c
@@ -0,0 +1,319 @@
+/*
+ * Emulator TPM driver which connects over the mssim protocol
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * Copyright (c) 2022
+ * Author: James Bottomley
+ */
+
+#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, retc;
+Error *local_err = NULL;
+
+ret = qio_channel_socket_connect_sync(t->ctrl_qc, t->opts.control,
Re: [PATCH 12/12] backends/tpm: Use qemu_hexdump_line() to avoid sprintf()
On 4/10/24 12:06, Philippe Mathieu-Daudé wrote:
sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.
Use qemu_hexdump_line() to avoid sprintf() calls, silencing:
backends/tpm/tpm_util.c:357:14: warning: 'sprintf' is deprecated:
This function is provided for compatibility reasons only.
Due to security concerns inherent in the design of sprintf(3),
it is highly recommended that you use snprintf(3) instead.
[-Wdeprecated-declarations]
p += sprintf(p, "%.2X ", buffer[i]);
^ >
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Stefan Berger
---
backends/tpm/tpm_util.c | 24
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
index 1856589c3b..0747af2d1c 100644
--- a/backends/tpm/tpm_util.c
+++ b/backends/tpm/tpm_util.c
@@ -21,6 +21,7 @@
#include "qemu/osdep.h"
#include "qemu/error-report.h"
+#include "qemu/cutils.h"
#include "qapi/error.h"
#include "qapi/visitor.h"
#include "tpm_int.h"
@@ -336,27 +337,18 @@ void tpm_sized_buffer_reset(TPMSizedBuffer *tsb)
void tpm_util_show_buffer(const unsigned char *buffer,
size_t buffer_size, const char *string)
{
-size_t len, i;
-char *line_buffer, *p;
+size_t len;
+char *line, *lineup;
if (!trace_event_get_state_backends(TRACE_TPM_UTIL_SHOW_BUFFER)) {
return;
}
len = MIN(tpm_cmd_get_size(buffer), buffer_size);
-/*
- * allocate enough room for 3 chars per buffer entry plus a
- * newline after every 16 chars and a final null terminator.
- */
-line_buffer = g_malloc(len * 3 + (len / 16) + 1);
+line = qemu_hexdump_line(buffer, 0, len, false);
+lineup = g_ascii_strup(line, -1);
+trace_tpm_util_show_buffer(string, len, lineup);
-for (i = 0, p = line_buffer; i < len; i++) {
-if (i && !(i % 16)) {
-p += sprintf(p, "\n");
-}
-p += sprintf(p, "%.2X ", buffer[i]);
-}
-trace_tpm_util_show_buffer(string, len, line_buffer);
-
-g_free(line_buffer);
+g_free(line);
+g_free(lineup);
}
Re: [PATCH v1 3/5] hw/ppc: SPI controller model - sequencer and shifter
On 2/7/24 11:08, Chalapathi V wrote:
In this commit SPI shift engine and sequencer logic is implemented.
Shift engine performs serialization and de-serialization according to the
control by the sequencer and according to the setup defined in the
configuration registers. Sequencer implements the main control logic and
FSM to handle data transmit and data receive control of the shift engine.
Signed-off-by: Chalapathi V
---
include/hw/ppc/pnv_spi_controller.h | 58 ++
hw/ppc/pnv_spi_controller.c | 1274 ++-
2 files changed, 1331 insertions(+), 1 deletion(-)
diff --git a/include/hw/ppc/pnv_spi_controller.h
b/include/hw/ppc/pnv_spi_controller.h
index 8afaabdd1b..8160c35f5c 100644
--- a/include/hw/ppc/pnv_spi_controller.h
+++ b/include/hw/ppc/pnv_spi_controller.h
@@ -8,6 +8,14 @@
* This model Supports a connection to a single SPI responder.
* Introduced for P10 to provide access to SPI seeproms, TPM, flash device
* and an ADC controller.
+ *
+ * All SPI function control is mapped into the SPI register space to enable
+ * full control by firmware.
+ *
+ * SPI Controller has sequencer and shift engine. The SPI shift engine
+ * performs serialization and de-serialization according to the control by
+ * the sequencer and according to the setup defined in the configuration
+ * registers and the SPI sequencer implements the main control logic.
*/
#ifndef PPC_PNV_SPI_CONTROLLER_H
@@ -20,6 +28,7 @@
#define SPI_CONTROLLER_REG_SIZE 8
typedef struct SpiBus SpiBus;
+typedef struct xfer_buffer xfer_buffer;
typedef struct PnvSpiController {
DeviceState parent;
@@ -28,6 +37,39 @@ typedef struct PnvSpiController {
MemoryRegionxscom_spic_regs;
/* SPI controller object number */
uint32_tspic_num;
+uint8_t responder_select;
+/* To verify if shift_n1 happens prior to shift_n2 */
+boolshift_n1_done;
+/*
+ * Internal flags for the first and last indicators for the SPI
+ * interface methods
+ */
+uint8_t first;
+uint8_t last;
Do these two correspond to the first and liast here?
xfer_buffer *seeprom_spi_request(PnvSpiResponder *resp, int first, int
last, int bits, xfer_buffer *payload);
If so I think the data types in the prototype should be set to uint8_t
as well and also bits should probably be an unsigned int or uint8_t?
+/* Loop counter for branch operation opcode Ex/Fx */
+uint8_t loop_counter_1;
+uint8_t loop_counter_2;
+/* N1/N2_bits specifies the size of the N1/N2 segment of a frame in bits.*/
+uint8_t N1_bits;
+uint8_t N2_bits;
+/* Number of bytes in a payload for the N1/N2 frame segment.*/
+uint8_t N1_bytes;
+uint8_t N2_bytes;
+/* Number of N1/N2 bytes marked for transmit */
+uint8_t N1_tx;
+uint8_t N2_tx;
+/* Number of N1/N2 bytes marked for receive */
+uint8_t N1_rx;
+uint8_t N2_rx;
+/*
+ * Setting this attribute to true will cause the engine to reverse the
+ * bit order of each byte it appends to a payload before sending the
+ * payload to a device. There may be cases where an end device expects
+ * a reversed order, like in the case of the Nuvoton TPM device. The
+ * order of bytes in the payload is not reversed, only the order of the
+ * 8 bits in each payload byte.
+ */
+boolreverse_bits;
/* SPI Controller registers */
uint64_terror_reg;
@@ -40,4 +82,20 @@ typedef struct PnvSpiController {
uint8_t sequencer_operation_reg[SPI_CONTROLLER_REG_SIZE];
uint64_tstatus_reg;
} PnvSpiController;
+
+void log_all_N_counts(PnvSpiController *spi_controller);
+void spi_response(PnvSpiController *spi_controller, int bits,
+xfer_buffer *rsp_payload);
+void operation_sequencer(PnvSpiController *spi_controller);
+bool operation_shiftn1(PnvSpiController *spi_controller, uint8_t opcode,
+ xfer_buffer **payload, bool send_n1_alone);
+bool operation_shiftn2(PnvSpiController *spi_controller, uint8_t opcode,
+ xfer_buffer **payload);
+bool does_rdr_match(PnvSpiController *spi_controller);
+uint8_t get_from_offset(PnvSpiController *spi_controller, uint8_t offset);
+void shift_byte_in(PnvSpiController *spi_controller, uint8_t byte);
+void calculate_N1(PnvSpiController *spi_controller, uint8_t opcode);
+void calculate_N2(PnvSpiController *spi_controller, uint8_t opcode);
+void do_reset(PnvSpiController *spi_controller);
+uint8_t reverse_bits8(uint8_t x);
#endif /* PPC_PNV_SPI_CONTROLLER_H */
diff --git a/hw/ppc/pnv_spi_controller.c b/hw/ppc/pnv_spi_controller.c
index 0f2bc25e82..ef48af5d03 100644
--- a/hw/ppc/pnv_spi_controller.c
+++ b/hw/ppc/pnv_spi_controller.c
@@ -9,7 +9,6 @@
#include "qemu/osdep.h"
#include "qemu/log.h"
#include
Re: [PATCH v1 1/5] hw/ppc: SPI responder model
On 2/7/24 11:08, Chalapathi V wrote:
Serial pheripheral interface provides full-duplex synchronous serial
communication between single controller and multiple responder devices.
One SPI Controller is implemented and supported on a SPI Bus, there is
no support for multiple controllers on the SPI bus.
The current implemetation assumes that single responder is connected to
bus, hence chip_select is not modelled.
Signed-off-by: Chalapathi V
---
include/hw/ppc/pnv_spi_responder.h | 109 +++
hw/ppc/pnv_spi_responder.c | 166 +
hw/ppc/meson.build | 1 +
3 files changed, 276 insertions(+)
create mode 100644 include/hw/ppc/pnv_spi_responder.h
create mode 100644 hw/ppc/pnv_spi_responder.c
diff --git a/include/hw/ppc/pnv_spi_responder.h
b/include/hw/ppc/pnv_spi_responder.h
new file mode 100644
index 00..1cf7279aad
--- /dev/null
+++ b/include/hw/ppc/pnv_spi_responder.h
@@ -0,0 +1,109 @@
+/*
+ * QEMU SPI Responder.
+ *
+ * Copyright (c) 2024, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * SPI provides full-duplex synchronous serial communication between single
+ * controller and multiple responder devices. One SPI Controller is
+ * implemented and supported on a SPI Bus, there is no support for multiple
an SPI Bus
+ * controllers on the SPI bus.
+ *
+ * The current implementation assumes that single responder is connected to >
+ * bus, hence chip_select is not modelled.
to the bus, hence chip_select is not modeled.
+ */
+
+#ifndef PPC_PNV_SPI_RESPONDER_H
+#define PPC_PNV_SPI_RESPONDER_H
+
+#include "hw/qdev-core.h"
+#include "qom/object.h"
+#include "qemu/log.h"
+
+#define TYPE_PNV_SPI_RESPONDER "spi-responder"
+OBJECT_DECLARE_TYPE(PnvSpiResponder, PnvSpiResponderClass,
+PNV_SPI_RESPONDER)
+
+typedef struct xfer_buffer xfer_buffer;
+
+struct PnvSpiResponderClass {
+DeviceClass parent_class;
+
+/*
+ * These methods are from controller to responder and implemented
+ * by all responders.
+ * Connect_controller/disconnect_controller methods are called by
+ * controller to initiate/stop the SPI transfer.
+ */
+void (*connect_controller)(PnvSpiResponder *responder, const char *port);
+void (*disconnect_controller)(PnvSpiResponder *responder);
+/*
+ * SPI transfer consists of a number of consecutive calls to the request
+ * method.
+ * The parameter first is true on first call of the transfer and last is on
+ * the final call of the transfer. Parameter bits and payload defines the
+ * number of bits and data payload sent by controller.
+ * Responder returns the response payload.
+ */
+xfer_buffer *(*request)(PnvSpiResponder *responder, int first, int last,
+int bits, xfer_buffer *payload);
+};
+
+struct PnvSpiResponder {
+DeviceState parent_obj;
+};
+
+#define TYPE_SPI_BUS "spi-bus"
+OBJECT_DECLARE_SIMPLE_TYPE(SpiBus, SPI_BUS)
+
+struct SpiBus {
+BusState parent_obj;
+};
+
+/*
+ * spi_realize_and_unref: realize and unref an SPI responder
+ * @dev: SPI responder to realize
+ * @bus: SPI bus to put it on
+ * @errp: error pointer
+ */
+bool spi_realize_and_unref(DeviceState *dev, SpiBus *bus, Error **errp);
+
+/*
+ * spi_create_responder: create a SPI responder.
a -> an
+ * @bus: SPI bus to put it on
+ * @name: name of the responder object.
+ * call spi_realize_and_unref() after creating the responder.
+ */
+
+PnvSpiResponder *spi_create_responder(SpiBus *bus, const char *name);
+
+/* xfer_buffer */
+typedef struct xfer_buffer {
+
+uint32_tlen;
+uint8_t*data;
+
+} xfer_buffer;
+
+/*
+ * xfer_buffer_read_ptr: Increment the payload length and return the pointer
+ * to the data at offset
+ */
+uint8_t *xfer_buffer_write_ptr(xfer_buffer *payload, uint32_t offset,
+uint32_t length);
+/* xfer_buffer_read_ptr: Return the pointer to the data at offset */
+void xfer_buffer_read_ptr(xfer_buffer *payload, uint8_t **read_buf,
+uint32_t offset, uint32_t length);
+/* xfer_buffer_new: Allocate memory and return the pointer */
+xfer_buffer *xfer_buffer_new(void);
+/* xfer_buffer_free: free the payload */
+void xfer_buffer_free(xfer_buffer *payload);
+
+/* Controller interface */
+SpiBus *spi_create_bus(DeviceState *parent, const char *name);
+xfer_buffer *spi_request(SpiBus *bus, int first, int last, int bits,
+xfer_buffer *payload);
+bool spi_connect_controller(SpiBus *bus, const char *port);
+bool spi_disconnect_controller(SpiBus *bus);
+#endif /* PPC_PNV_SPI_SEEPROM_H */
diff --git a/hw/ppc/pnv_spi_responder.c b/hw/ppc/pnv_spi_responder.c
new file mode 100644
index 00..c3bc659b1b
--- /dev/null
+++ b/hw/ppc/pnv_spi_responder.c
@@ -0,0 +1,166 @@
+/*
+ * QEMU PowerPC SPI Responder
+ *
+ * Copyright (c) 2024, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include
Re: [PATCH v1 2/5] hw/ppc: SPI controller model - registers implementation
On 3/7/24 13:54, Stefan Berger wrote:
On 2/7/24 11:08, Chalapathi V wrote:
SPI controller device model supports a connection to a single SPI
responder.
This provide access to SPI seeproms, TPM, flash device and an ADC
controller.
All SPI function control is mapped into the SPI register space to
enable full
control by firmware. In this commit SPI configuration component is
modelled
which contains all SPI configuration and status registers as well as
the hold
registers for data to be sent or having been received.
Signed-off-by: Chalapathi V
+ case SEQUENCER_OPERATION_REG:
+ for (int i = 0; i < SPI_CONTROLLER_REG_SIZE; i++) {
+ sc->sequencer_operation_reg[i] =
+ (val & PPC_BITMASK(i * 8 , i * 8 + 7)) >> (63 - (i *
8 + 7));
To me it would be more obvious if you used a mask here like this:
mask = PPC_BIT_MASK(0, 7);
mask = (0xff << 56);
for (...) {
sc->sequencer_operation_reg[i] = (val & mask) >> (56 - i * 8);
mask >>= 8;
}
Actually simpler and even this masking is not necessary:
for (...) {
sc->sequencer_operation_reg[i] = (val >> (56 - i * 8)) & 0xff;
}
Re: [PATCH v1 4/5] hw/ppc: SPI SEEPROM model
On 2/7/24 11:08, Chalapathi V wrote:
This commit implements a Serial EEPROM utilizing the Serial Peripheral
Interface (SPI) compatible bus.
Currently implemented SEEPROM is Microchip's 25CSM04 which provides 4 Mbits
of Serial EEPROM utilizing the Serial Peripheral Interface (SPI) compatible
bus. The device is organized as 524288 bytes of 8 bits each (512Kbyte) and
is optimized for use in consumer and industrial applications where reliable
and dependable nonvolatile memory storage is essential.
Signed-off-by: Chalapathi V
---
include/hw/ppc/pnv_spi_seeprom.h | 70 +++
hw/ppc/pnv_spi_seeprom.c | 989 +++
hw/ppc/meson.build | 1 +
3 files changed, 1060 insertions(+)
create mode 100644 include/hw/ppc/pnv_spi_seeprom.h
create mode 100644 hw/ppc/pnv_spi_seeprom.c
diff --git a/include/hw/ppc/pnv_spi_seeprom.h b/include/hw/ppc/pnv_spi_seeprom.h
new file mode 100644
index 00..9739e411b5
--- /dev/null
+++ b/include/hw/ppc/pnv_spi_seeprom.h
@@ -0,0 +1,70 @@
+/*
+ * QEMU PowerPC SPI SEEPROM model
+ *
+ * Copyright (c) 2024, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This model implements a Serial EEPROM utilizing the Serial Peripheral
+ * Interface (SPI) compatible bus.
+ * Currently supported variants: 25CSM04.
+ * The Microchip Technology Inc. 25CSM04 provides 4 Mbits of Serial EEPROM
+ * utilizing the Serial Peripheral Interface (SPI) compatible bus. The device
+ * is organized as 524288 bytes of 8 bits each (512Kbyte) and is optimized
+ * for use in consumer and industrial applications where reliable and
+ * dependable nonvolatile memory storage is essential
+ */
+
+#ifndef PPC_PNV_SPI_SEEPROM_H
+#define PPC_PNV_SPI_SEEPROM_H
+
+#include "hw/ppc/pnv_spi_responder.h"
+#include "qom/object.h"
+
+#define TYPE_PNV_SPI_SEEPROM "pnv-spi-seeprom"
+
+OBJECT_DECLARE_SIMPLE_TYPE(PnvSpiSeeprom, PNV_SPI_SEEPROM)
+
+typedef struct xfer_buffer xfer_buffer;
+
+typedef struct PnvSpiSeeprom {
+PnvSpiResponder resp;
+
+char*file; /* SEEPROM image file */
+uint8_t opcode; /* SEEPROM Opcode */
+uint32_taddr; /* SEEPROM Command Address */
+uint8_t rd_state; /* READ State Machine state variable */
+boollocked; /* Security Register Locked */
+boolcontroller_connected; /* Flag for master connection */
+/*
+ * Device registers
+ * The 25CSM04 contains four types of registers that modulate device
+ * operation and/or report on the current status of the device. These
+ * registers are:
+ * STATUS register
+ * Security register
+ * Memory Partition registers (eight total)
+ * Identification register
+ */
+uint8_t status0;
+uint8_t status1;
+/*
+ * The Security register is split into
+ * 1. user-programmable lockable ID page section.
+ * 2. The read-only section contains a preprogrammed, globally unique,
+ *128-bit serial number.
+ */
+uint8_t uplid[256];
+uint8_t dsn[16];
+uint8_t mpr[8];
+uint8_t idr[5];
+} PnvSpiSeeprom;
+
+xfer_buffer *seeprom_spi_request(PnvSpiResponder *resp, int first, int last,
+int bits, xfer_buffer *payload);
+void seeprom_connect_controller(PnvSpiResponder *resp, const char *port);
+void seeprom_disconnect_controller(PnvSpiResponder *resp);
+bool compute_addr(PnvSpiSeeprom *spi_resp, xfer_buffer *req_payload,
+ xfer_buffer *rsp_payload, int bits, uint32_t *data_offset);
+bool validate_addr(PnvSpiSeeprom *spi_resp);
+#endif /* PPC_PNV_SPI_SEEPROM_H */
diff --git a/hw/ppc/pnv_spi_seeprom.c b/hw/ppc/pnv_spi_seeprom.c
new file mode 100644
index 00..ae46610045
--- /dev/null
+++ b/hw/ppc/pnv_spi_seeprom.c
@@ -0,0 +1,989 @@
+/*
+ * QEMU PowerPC SPI SEEPROM model
+ *
+ * Copyright (c) 2024, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/ppc/pnv_spi_seeprom.h"
+#include
+
+#define SPI_DEBUG(x)
+
+/*
+ * 2-byte STATUS register which is a combination of six nonvolatile bits of
+ * EEPROM and five volatile latches.
+ *
+ * status 0:
+ * bit 7 WPEN: Write-Protect Enable bit
+ * 1 = Write-Protect pin is enabled, 0 = Write-Protect pin is ignored
+ *
+ * bit 3-2 BP<1:0>: Block Protection bits
+ * 00 = No array write protection
+ * 01 = Upper quarter memory array protection
+ * 10 = Upper half memory array protection
+ * 11 = Entire memory array protection
+ *
+ * bit 1 WEL: Write Enable Latch bit
+ * 1 = WREN has been executed and device is enabled for writing
+ * 0 = Device is not write-enabled
+ *
+ * bit 0 RDY/BSY: Ready/Busy Status Latch bit
+ * 1 = Device is busy with an internal write cycle
+ * 0 = Device is ready for a new sequence
+ */
+#define STATUS0_WPEN0x7
+#define STATUS0_BP 0x2
+#define STATUS0_WEL 0x1
+#define STATUS0_BUSY
Re: [PATCH v1 2/5] hw/ppc: SPI controller model - registers implementation
On 3/7/24 13:54, Stefan Berger wrote: On 2/7/24 11:08, Chalapathi V wrote: +#define COUNTER_CONFIG_REG_SHIFT_COUNT_N1 PPC_BITMASK(0 , 7) No space before the ',' ==> PPC_BITMASK(0, 7)
Re: [PATCH v1 2/5] hw/ppc: SPI controller model - registers implementation
On 2/7/24 11:08, Chalapathi V wrote:
SPI controller device model supports a connection to a single SPI responder.
This provide access to SPI seeproms, TPM, flash device and an ADC controller.
All SPI function control is mapped into the SPI register space to enable full
control by firmware. In this commit SPI configuration component is modelled
which contains all SPI configuration and status registers as well as the hold
registers for data to be sent or having been received.
Signed-off-by: Chalapathi V
---
include/hw/ppc/pnv_spi_controller.h | 43
include/hw/ppc/pnv_xscom.h | 3 +
hw/ppc/pnv_spi_controller.c | 337
hw/ppc/meson.build | 1 +
4 files changed, 384 insertions(+)
create mode 100644 include/hw/ppc/pnv_spi_controller.h
create mode 100644 hw/ppc/pnv_spi_controller.c
diff --git a/include/hw/ppc/pnv_spi_controller.h
b/include/hw/ppc/pnv_spi_controller.h
new file mode 100644
index 00..8afaabdd1b
--- /dev/null
+++ b/include/hw/ppc/pnv_spi_controller.h
@@ -0,0 +1,43 @@
+/*
+ * QEMU PowerPC SPI Controller model
+ *
+ * Copyright (c) 2024, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This model Supports a connection to a single SPI responder.
+ * Introduced for P10 to provide access to SPI seeproms, TPM, flash device
+ * and an ADC controller.
+ */
+
+#ifndef PPC_PNV_SPI_CONTROLLER_H
+#define PPC_PNV_SPI_CONTROLLER_H
+
+#define TYPE_PNV_SPI_CONTROLLER "pnv-spi-controller"
+#define PNV_SPICONTROLLER(obj) \
+OBJECT_CHECK(PnvSpiController, (obj), TYPE_PNV_SPI_CONTROLLER)
+
+#define SPI_CONTROLLER_REG_SIZE 8
+
+typedef struct SpiBus SpiBus;
+
+typedef struct PnvSpiController {
+DeviceState parent;
+
+SpiBus*spi_bus;
+MemoryRegionxscom_spic_regs;
+/* SPI controller object number */
+uint32_tspic_num;
+
+/* SPI Controller registers */
+uint64_terror_reg;
+uint64_tcounter_config_reg;
+uint64_tconfig_reg1;
+uint64_tclock_config_reset_control;
+uint64_tmemory_mapping_reg;
+uint64_ttransmit_data_reg;
+uint64_treceive_data_reg;
+uint8_t sequencer_operation_reg[SPI_CONTROLLER_REG_SIZE];
+uint64_tstatus_reg;
+} PnvSpiController;
+#endif /* PPC_PNV_SPI_CONTROLLER_H */
diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
index f5becbab41..0575bf0985 100644
--- a/include/hw/ppc/pnv_xscom.h
+++ b/include/hw/ppc/pnv_xscom.h
@@ -176,6 +176,9 @@ struct PnvXScomInterfaceClass {
#define PNV10_XSCOM_PEC_PCI_BASE 0x8010800 /* index goes upwards ... */
#define PNV10_XSCOM_PEC_PCI_SIZE 0x200
+#define PNV10_XSCOM_PIB_SPIC_BASE 0xc
+#define PNV10_XSCOM_PIB_SPIC_SIZE 0x20
+
void pnv_xscom_init(PnvChip *chip, uint64_t size, hwaddr addr);
int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset,
uint64_t xscom_base, uint64_t xscom_size,
diff --git a/hw/ppc/pnv_spi_controller.c b/hw/ppc/pnv_spi_controller.c
new file mode 100644
index 00..0f2bc25e82
--- /dev/null
+++ b/hw/ppc/pnv_spi_controller.c
@@ -0,0 +1,337 @@
+/*
+ * QEMU PowerPC SPI Controller model
+ *
+ * Copyright (c) 2024, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/qdev-properties.h"
+#include "hw/ppc/pnv.h"
+#include "hw/ppc/pnv_xscom.h"
+#include "hw/ppc/pnv_spi_controller.h"
+#include "hw/ppc/pnv_spi_responder.h"
+#include "hw/ppc/fdt.h"
+#include
+#include
+
+#define SPI_DEBUG(x)
+
+/* Error Register */
+#define ERROR_REG 0x00
+
+/* counter_config_reg */
+#define COUNTER_CONFIG_REG 0x01
+#define COUNTER_CONFIG_REG_SHIFT_COUNT_N1 PPC_BITMASK(0 , 7)
+#define COUNTER_CONFIG_REG_SHIFT_COUNT_N2 PPC_BITMASK(8 , 15)
+#define COUNTER_CONFIG_REG_COUNT_COMPARE1 PPC_BITMASK(24 , 31)
+#define COUNTER_CONFIG_REG_COUNT_COMPARE2 PPC_BITMASK(32 , 39)
+#define COUNTER_CONFIG_REG_N1_COUNT_CONTROL PPC_BITMASK(48 , 51)
+#define COUNTER_CONFIG_REG_N2_COUNT_CONTROL PPC_BITMASK(52 , 55)
+
+/* config_reg */
+#define CONFIG_REG1 0x02
+
+/* clock_config_reset_control_ecc_enable_reg */
+#define CLOCK_CONFIG_REG0x03
+#define CLOCK_CONFIG_RESET_CONTROL_HARD_RESET 0x0084;
+#define CLOCK_CONFIG_REG_RESET_CONTROL PPC_BITMASK(24 , 27)
+#define CLOCK_CONFIG_REG_ECC_CONTROLPPC_BITMASK(28 , 30)
+
+/* memory_mapping_reg */
+#define MEMORY_MAPPING_REG 0x04
+#define MEMORY_MAPPING_REG_MMSPISM_BASE_ADDRPPC_BITMASK(0 , 15)
+#define MEMORY_MAPPING_REG_MMSPISM_ADDR_MASKPPC_BITMASK(16 , 31)
+#define MEMORY_MAPPING_REG_RDR_MATCH_VALPPC_BITMASK(32 , 47)
+#define MEMORY_MAPPING_REG_RDR_MATCH_MASK PPC_BITMASK(48 , 63)
+
+/* transmit_data_reg */
+#define
Re: [PATCH v2 4/4] Exclude TPM ioctls definitions for the GNU/Hurd
On 1/22/24 15:46, Peter Maydell wrote:
On Mon, 22 Jan 2024 at 19:30, Stefan Berger wrote:
On 1/22/24 12:16, Peter Maydell wrote:
On Thu, 18 Jan 2024 at 16:04, Manolo de Medici wrote:
The Hurd currently doesn't have any TPM driver, compilation fails
for missing symbols unless these are left undefined.
Signed-off-by: Manolo de Medici
---
backends/tpm/tpm_ioctl.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/backends/tpm/tpm_ioctl.h b/backends/tpm/tpm_ioctl.h
index 1933ab6855..c721bf8847 100644
--- a/backends/tpm/tpm_ioctl.h
+++ b/backends/tpm/tpm_ioctl.h
@@ -274,7 +274,7 @@ typedef struct ptm_lockstorage ptm_lockstorage;
#define PTM_CAP_SEND_COMMAND_HEADER (1 << 15)
#define PTM_CAP_LOCK_STORAGE (1 << 16)
-#ifndef _WIN32
+#if !defined(_WIN32) && !defined(__GNU__)
enum {
PTM_GET_CAPABILITY = _IOR('P', 0, ptm_cap),
PTM_INIT = _IOWR('P', 1, ptm_init),
--
2.43.0
This looks plausible as a change, but looking at the history
of the file in git it seems like this is a file we import
from a third-party swtpm project.
Stefan: should we get this change made in the swtpm project
too? Or have we diverged from that copy of the header?
The diffs are minimal at the moment:
$ diff swtpm/include/swtpm/tpm_ioctl.h qemu/backends/tpm/tpm_ioctl.h
15,16d14
< #include
< #include
Since we already handle _WIN32 we can just take this case for __GNU__.
OK, so how should we handle the mechanics of it -- just take
this commit in QEMU and then you'll sort it out in swtpm?
Yes.
Or do we need to change swtpm first and then sync?
No.
Regarding the patch:
Reviewed-by: Stefan Berger
thanks
-- PMM
Re: [PATCH v2 4/4] Exclude TPM ioctls definitions for the GNU/Hurd
On 1/22/24 12:16, Peter Maydell wrote:
On Thu, 18 Jan 2024 at 16:04, Manolo de Medici wrote:
The Hurd currently doesn't have any TPM driver, compilation fails
for missing symbols unless these are left undefined.
Signed-off-by: Manolo de Medici
---
backends/tpm/tpm_ioctl.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/backends/tpm/tpm_ioctl.h b/backends/tpm/tpm_ioctl.h
index 1933ab6855..c721bf8847 100644
--- a/backends/tpm/tpm_ioctl.h
+++ b/backends/tpm/tpm_ioctl.h
@@ -274,7 +274,7 @@ typedef struct ptm_lockstorage ptm_lockstorage;
#define PTM_CAP_SEND_COMMAND_HEADER (1 << 15)
#define PTM_CAP_LOCK_STORAGE (1 << 16)
-#ifndef _WIN32
+#if !defined(_WIN32) && !defined(__GNU__)
enum {
PTM_GET_CAPABILITY = _IOR('P', 0, ptm_cap),
PTM_INIT = _IOWR('P', 1, ptm_init),
--
2.43.0
This looks plausible as a change, but looking at the history
of the file in git it seems like this is a file we import
from a third-party swtpm project.
Stefan: should we get this change made in the swtpm project
too? Or have we diverged from that copy of the header?
The diffs are minimal at the moment:
$ diff swtpm/include/swtpm/tpm_ioctl.h qemu/backends/tpm/tpm_ioctl.h
15,16d14
< #include
< #include
Since we already handle _WIN32 we can just take this case for __GNU__.
Stefan
If the latter, then the simple thing woud be to delete
this enum entirely, because as far as I can see we don't
use any of the values in QEMU, so we can avoid the
portability problem that way.
thanks
-- PMM
Re: [PATCH v5 12/14] tests: acpi: implement TPM CRB tests for ARM virt
On 11/24/23 21:39, Joelle van Dyne wrote:
On Fri, Nov 24, 2023 at 8:26 AM Stefan Berger wrote:
On 11/24/23 11:21, Joelle van Dyne wrote:
On Fri, Nov 24, 2023 at 8:17 AM Stefan Berger wrote:
On 11/23/23 19:56, Joelle van Dyne wrote:
On Tue, Nov 14, 2023 at 4:12 PM Stefan Berger wrote:
On 11/14/23 16:05, Stefan Berger wrote:
On 11/14/23 13:03, Stefan Berger wrote:
On 11/14/23 04:36, Marc-André Lureau wrote:
Hi
On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne wrote:
Signed-off-by: Joelle van Dyne
Reviewed-by: Stefan Berger
nit: you also added tests for x86, could be a different patch?
For arm, the test fails until next patch with:
# starting QEMU: exec ./qemu-system-aarch64 -qtest
unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev
socket,path=/tmp/qtest-991279.qmp,id=char0 -mon
chardev=char0,mode=control -display none -audio none -machine virt
-accel tcg -nodefaults -nographic -drive
if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on
-drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on
-cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2
-cpu cortex-a57 -chardev
socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock
-tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev
-accel qtest
Warning! zero length expected file
'tests/data/acpi/virt/TPM2.crb-device.tpm2'
Warning! zero length expected file
'tests/data/acpi/virt/DSDT.crb-device.tpm2'
acpi-test: Warning! binary file mismatch. Actual
[aml:/tmp/aml-GO4ME2], Expected
[aml:tests/data/acpi/virt/TPM2.crb-device.tpm2].
See source file tests/qtest/bios-tables-test.c for instructions on how
to update expected files.
acpi-test: Warning! binary file mismatch. Actual
[aml:/tmp/aml-6N4ME2], Expected
[aml:tests/data/acpi/virt/DSDT.crb-device.tpm2].
See source file tests/qtest/bios-tables-test.c for instructions on how
to update expected files.
to see ASL diff between mismatched files install IASL, rebuild QEMU
from scratch and re-run tests with V=1 environment variable set**
ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion
failed: (all_tables_match)
not ok /aarch64/acpi/virt/tpm2-crb -
ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion
failed: (all_tables_match)
Bail out!
qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM:
Resource temporarily unavailable
Unexpected error in qio_channel_socket_writev() at
../io/channel-socket.c:622:
/home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable
to write to socket: Bad file descriptor
Travis testing on s390x I see the following failures for this patchset
(search for 'ERROR'):
https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267230363
Summary of Failures:
134/320 qemu:qtest+qtest-aarch64 / qtest-aarch64/tpm-crb-device-test
ERROR 0.70s killed by signal 6 SIGABRT
219/320 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-test
ERROR 0.88s killed by signal 6 SIGABRT
Summary of Failures:
271/537 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-test
ERROR 0.59s killed by signal 6 SIGABRT
My guess is it's an endianess issue on big endian machines due to
reading from the ROM device where we lost the .endianess:
+const MemoryRegionOps tpm_crb_memory_ops = {
+.read = tpm_crb_mmio_read,
+.write = tpm_crb_mmio_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+.valid = {
+.min_access_size = 1,
+.max_access_size = 4,
+},
+};
I think we need a 2nd set of registers to support the endianess
conversion. It's not exactly nice, though. Basically the saved_regs
could be used for this directly, even though I did not do that but
introduced n_regs:
https://github.com/stefanberger/qemu-tpm/commit/90f6b21c0dd93dbb13d9e80a628f5b631fd07d91
This patch allows the tests on s390x to run farther but the execution of
the command doesn't seem to work maybe due to command data that were
also written in wrong endianess. I don't know. I would have to get
access to a big endian / s390 machine to be able to fix it.
The latest version now passes on Travis s390x:
https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267245220
Are the tests failing on S390X due to the added code or are they
failing because previously it was untested? I don't think the original
code took account of endianness and that should be fixed, but feels
like it should be in a separate patch set?
They are failing because something like the topmost one or two patches
as in this branch here are missing for a big endian host:
https://github.com/stefanberger/qemu-tpm/tree/joelle.v5%2B2nd_registers
Right, but is this issue new due to the patchset? i.e. if just the
Yes, it is due to this patchset. The reason is that CRB switched to a
ROMD interface where the fact that the MMIO registers are little endian
got lost for existing x86_64 support.
tests were added without the other patches, would
Re: [PATCH v5 12/14] tests: acpi: implement TPM CRB tests for ARM virt
On 11/24/23 11:21, Joelle van Dyne wrote:
On Fri, Nov 24, 2023 at 8:17 AM Stefan Berger wrote:
On 11/23/23 19:56, Joelle van Dyne wrote:
On Tue, Nov 14, 2023 at 4:12 PM Stefan Berger wrote:
On 11/14/23 16:05, Stefan Berger wrote:
On 11/14/23 13:03, Stefan Berger wrote:
On 11/14/23 04:36, Marc-André Lureau wrote:
Hi
On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne wrote:
Signed-off-by: Joelle van Dyne
Reviewed-by: Stefan Berger
nit: you also added tests for x86, could be a different patch?
For arm, the test fails until next patch with:
# starting QEMU: exec ./qemu-system-aarch64 -qtest
unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev
socket,path=/tmp/qtest-991279.qmp,id=char0 -mon
chardev=char0,mode=control -display none -audio none -machine virt
-accel tcg -nodefaults -nographic -drive
if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on
-drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on
-cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2
-cpu cortex-a57 -chardev
socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock
-tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev
-accel qtest
Warning! zero length expected file
'tests/data/acpi/virt/TPM2.crb-device.tpm2'
Warning! zero length expected file
'tests/data/acpi/virt/DSDT.crb-device.tpm2'
acpi-test: Warning! binary file mismatch. Actual
[aml:/tmp/aml-GO4ME2], Expected
[aml:tests/data/acpi/virt/TPM2.crb-device.tpm2].
See source file tests/qtest/bios-tables-test.c for instructions on how
to update expected files.
acpi-test: Warning! binary file mismatch. Actual
[aml:/tmp/aml-6N4ME2], Expected
[aml:tests/data/acpi/virt/DSDT.crb-device.tpm2].
See source file tests/qtest/bios-tables-test.c for instructions on how
to update expected files.
to see ASL diff between mismatched files install IASL, rebuild QEMU
from scratch and re-run tests with V=1 environment variable set**
ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion
failed: (all_tables_match)
not ok /aarch64/acpi/virt/tpm2-crb -
ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion
failed: (all_tables_match)
Bail out!
qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM:
Resource temporarily unavailable
Unexpected error in qio_channel_socket_writev() at
../io/channel-socket.c:622:
/home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable
to write to socket: Bad file descriptor
Travis testing on s390x I see the following failures for this patchset
(search for 'ERROR'):
https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267230363
Summary of Failures:
134/320 qemu:qtest+qtest-aarch64 / qtest-aarch64/tpm-crb-device-test
ERROR 0.70s killed by signal 6 SIGABRT
219/320 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-test
ERROR 0.88s killed by signal 6 SIGABRT
Summary of Failures:
271/537 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-test
ERROR 0.59s killed by signal 6 SIGABRT
My guess is it's an endianess issue on big endian machines due to
reading from the ROM device where we lost the .endianess:
+const MemoryRegionOps tpm_crb_memory_ops = {
+.read = tpm_crb_mmio_read,
+.write = tpm_crb_mmio_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+.valid = {
+.min_access_size = 1,
+.max_access_size = 4,
+},
+};
I think we need a 2nd set of registers to support the endianess
conversion. It's not exactly nice, though. Basically the saved_regs
could be used for this directly, even though I did not do that but
introduced n_regs:
https://github.com/stefanberger/qemu-tpm/commit/90f6b21c0dd93dbb13d9e80a628f5b631fd07d91
This patch allows the tests on s390x to run farther but the execution of
the command doesn't seem to work maybe due to command data that were
also written in wrong endianess. I don't know. I would have to get
access to a big endian / s390 machine to be able to fix it.
The latest version now passes on Travis s390x:
https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267245220
Are the tests failing on S390X due to the added code or are they
failing because previously it was untested? I don't think the original
code took account of endianness and that should be fixed, but feels
like it should be in a separate patch set?
They are failing because something like the topmost one or two patches
as in this branch here are missing for a big endian host:
https://github.com/stefanberger/qemu-tpm/tree/joelle.v5%2B2nd_registers
Right, but is this issue new due to the patchset? i.e. if just the
Yes, it is due to this patchset. The reason is that CRB switched to a
ROMD interface where the fact that the MMIO registers are little endian
got lost for existing x86_64 support.
tests were added without the other patches, would they still fail? If
so, I think the fixes should be in another patchset while we disable
them
Re: [PATCH v5 12/14] tests: acpi: implement TPM CRB tests for ARM virt
On 11/23/23 19:56, Joelle van Dyne wrote:
On Tue, Nov 14, 2023 at 4:12 PM Stefan Berger wrote:
On 11/14/23 16:05, Stefan Berger wrote:
On 11/14/23 13:03, Stefan Berger wrote:
On 11/14/23 04:36, Marc-André Lureau wrote:
Hi
On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne wrote:
Signed-off-by: Joelle van Dyne
Reviewed-by: Stefan Berger
nit: you also added tests for x86, could be a different patch?
For arm, the test fails until next patch with:
# starting QEMU: exec ./qemu-system-aarch64 -qtest
unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev
socket,path=/tmp/qtest-991279.qmp,id=char0 -mon
chardev=char0,mode=control -display none -audio none -machine virt
-accel tcg -nodefaults -nographic -drive
if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on
-drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on
-cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2
-cpu cortex-a57 -chardev
socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock
-tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev
-accel qtest
Warning! zero length expected file
'tests/data/acpi/virt/TPM2.crb-device.tpm2'
Warning! zero length expected file
'tests/data/acpi/virt/DSDT.crb-device.tpm2'
acpi-test: Warning! binary file mismatch. Actual
[aml:/tmp/aml-GO4ME2], Expected
[aml:tests/data/acpi/virt/TPM2.crb-device.tpm2].
See source file tests/qtest/bios-tables-test.c for instructions on how
to update expected files.
acpi-test: Warning! binary file mismatch. Actual
[aml:/tmp/aml-6N4ME2], Expected
[aml:tests/data/acpi/virt/DSDT.crb-device.tpm2].
See source file tests/qtest/bios-tables-test.c for instructions on how
to update expected files.
to see ASL diff between mismatched files install IASL, rebuild QEMU
from scratch and re-run tests with V=1 environment variable set**
ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion
failed: (all_tables_match)
not ok /aarch64/acpi/virt/tpm2-crb -
ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion
failed: (all_tables_match)
Bail out!
qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM:
Resource temporarily unavailable
Unexpected error in qio_channel_socket_writev() at
../io/channel-socket.c:622:
/home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable
to write to socket: Bad file descriptor
Travis testing on s390x I see the following failures for this patchset
(search for 'ERROR'):
https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267230363
Summary of Failures:
134/320 qemu:qtest+qtest-aarch64 / qtest-aarch64/tpm-crb-device-test
ERROR 0.70s killed by signal 6 SIGABRT
219/320 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-test
ERROR 0.88s killed by signal 6 SIGABRT
Summary of Failures:
271/537 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-test
ERROR 0.59s killed by signal 6 SIGABRT
My guess is it's an endianess issue on big endian machines due to
reading from the ROM device where we lost the .endianess:
+const MemoryRegionOps tpm_crb_memory_ops = {
+.read = tpm_crb_mmio_read,
+.write = tpm_crb_mmio_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+.valid = {
+.min_access_size = 1,
+.max_access_size = 4,
+},
+};
I think we need a 2nd set of registers to support the endianess
conversion. It's not exactly nice, though. Basically the saved_regs
could be used for this directly, even though I did not do that but
introduced n_regs:
https://github.com/stefanberger/qemu-tpm/commit/90f6b21c0dd93dbb13d9e80a628f5b631fd07d91
This patch allows the tests on s390x to run farther but the execution of
the command doesn't seem to work maybe due to command data that were
also written in wrong endianess. I don't know. I would have to get
access to a big endian / s390 machine to be able to fix it.
The latest version now passes on Travis s390x:
https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267245220
Are the tests failing on S390X due to the added code or are they
failing because previously it was untested? I don't think the original
code took account of endianness and that should be fixed, but feels
like it should be in a separate patch set?
They are failing because something like the topmost one or two patches
as in this branch here are missing for a big endian host:
https://github.com/stefanberger/qemu-tpm/tree/joelle.v5%2B2nd_registers
Re: [PATCH v5 00/14] tpm: introduce TPM CRB SysBus device
On 11/20/23 03:29, Marc-André Lureau wrote: Hi On Tue, Nov 14, 2023 at 11:25 PM Joelle van Dyne wrote: On Tue, Nov 14, 2023 at 1:38 AM Marc-André Lureau wrote: Hi On Tue, Nov 14, 2023 at 6:10 AM Joelle van Dyne wrote: The impetus for this patch set is to get TPM 2.0 working on Windows 11 ARM64. Windows' tpm.sys does not seem to work on a TPM TIS device (as verified with VMWare's implementation). However, the current TPM CRB device uses a fixed system bus address that is reserved for RAM in ARM64 Virt machines. In the process of adding the TPM CRB SysBus device, we also went ahead and cleaned up some of the existing TPM hardware code and fixed some bugs. We used the TPM TIS devices as a template for the TPM CRB devices and refactored out common code. We moved the ACPI DSDT generation to the device in order to handle dynamic base address requirements as well as reduce redundent code in different machine ACPI generation. We also changed the tpm_crb device to use the ISA bus instead of depending on the default system bus as the device only was built for the PC configuration. Another change is that the TPM CRB registers are now mapped in the same way that the pflash ROM devices are mapped. It is a memory region whose writes are trapped as MMIO accesses. This was needed because Apple Silicon does not decode LDP (AARCH64 load pair of registers) caused page faults. @agraf suggested that we do this to avoid having to do AARCH64 decoding in the HVF backend's fault handler. Unfortunately, it seems like the LDP fault still happens on HVF but the issue seems to be in the HVF backend which needs to be fixed in a separate patch. One last thing that's needed to get Windows 11 to recognize the TPM 2.0 device is for the OVMF firmware to setup the TPM device. Currently, OVMF for ARM64 Virt only recognizes the TPM TIS device through a FDT entry. A workaround is to falsely identify the TPM CRB device as a TPM TIS device in the FDT node but this causes issues for Linux. A proper fix would involve adding an ACPI device driver in OVMF. This has been tested on ARM64 with `tpm-crb-device` and on x86_64 with `tpm-crb`. Additional testing should be performed on other architectures (RISCV and Loongarch for example) as well as migration cases. v5: - Fixed a typo in "tpm_crb: use a single read-as-mem/write-as-mmio mapping" - Fixed ACPI tables not being created for pc CRB device v4: - Fixed broken test blobs v3: - Support backwards and forwards migration of existing tpm-crb device - Dropped patch which moved tpm-crb to ISA bus due to migration concerns - Unified `tpm_sysbus_plug` handler for ARM and Loongarch - Added ACPI table tests for tpm-crb-device - Refactored TPM CRB tests to run on tpm-crb-device for ARM Virt v2: - Fixed an issue where VMstate restore from an older version failed due to name collision of the memory block. - In the ACPI table generation for CRB devices, the check for TPM 2.0 backend is moved to the device realize as CRB does not support TPM 1.0. It will error in that case. - Dropped the patch to fix crash when PPI is enabled on TIS SysBus device since a separate patch submitted by Stefan Berger disables such an option. - Fixed an issue where we default tpmEstablished=0 when it should be 1. - In TPM CRB SysBus's ACPI entry, we accidently changed _UID from 0 to 1. This shouldn't be an issue but we changed it back just in case. - Added a patch to migrate saved VMstate from an older version with the regs saved separately instead of as a RAM block. Joelle van Dyne (14): tpm_crb: refactor common code tpm_crb: CTRL_RSP_ADDR is 64-bits wide tpm_ppi: refactor memory space initialization tpm_crb: use a single read-as-mem/write-as-mmio mapping tpm_crb: move ACPI table building to device interface tpm-sysbus: add plug handler for TPM on SysBus hw/arm/virt: connect TPM to platform bus hw/loongarch/virt: connect TPM to platform bus tpm_tis_sysbus: move DSDT AML generation to device tests: acpi: prepare for TPM CRB tests tpm_crb_sysbus: introduce TPM CRB SysBus device tests: acpi: implement TPM CRB tests for ARM virt tests: acpi: updated expected blobs for TPM CRB tests: add TPM-CRB sysbus tests for aarch64 The series looks good to me. Have you checked there are no regressions with Windows HLK? I don't have any experience with Windows HLK. Is there any guide on running it with QEMU? Preferably with a prebuilt image? Or maybe someone in the mailing list can run it? I haven't done it for a while. (it wasn't that hard, just a bit tedious: install a Controller on a Windows Server VM, and set up a target VM) There is also AutoHCK, but I never managed to use it. (https://github.com/HCK-CI/AutoHCK) For the current results, they are summarized here: https://github.com/stefanberger/libtpms/wiki/Testing-of-libtpms-Functionality Stefan, do you have a working setup? I'll try to make one if it can help. Once we have the fixes for
Re: [PATCH v5 12/14] tests: acpi: implement TPM CRB tests for ARM virt
On 11/14/23 16:05, Stefan Berger wrote:
On 11/14/23 13:03, Stefan Berger wrote:
On 11/14/23 04:36, Marc-André Lureau wrote:
Hi
On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne wrote:
Signed-off-by: Joelle van Dyne
Reviewed-by: Stefan Berger
nit: you also added tests for x86, could be a different patch?
For arm, the test fails until next patch with:
# starting QEMU: exec ./qemu-system-aarch64 -qtest
unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev
socket,path=/tmp/qtest-991279.qmp,id=char0 -mon
chardev=char0,mode=control -display none -audio none -machine virt
-accel tcg -nodefaults -nographic -drive
if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on
-drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on
-cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2
-cpu cortex-a57 -chardev
socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock
-tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev
-accel qtest
Warning! zero length expected file
'tests/data/acpi/virt/TPM2.crb-device.tpm2'
Warning! zero length expected file
'tests/data/acpi/virt/DSDT.crb-device.tpm2'
acpi-test: Warning! binary file mismatch. Actual
[aml:/tmp/aml-GO4ME2], Expected
[aml:tests/data/acpi/virt/TPM2.crb-device.tpm2].
See source file tests/qtest/bios-tables-test.c for instructions on how
to update expected files.
acpi-test: Warning! binary file mismatch. Actual
[aml:/tmp/aml-6N4ME2], Expected
[aml:tests/data/acpi/virt/DSDT.crb-device.tpm2].
See source file tests/qtest/bios-tables-test.c for instructions on how
to update expected files.
to see ASL diff between mismatched files install IASL, rebuild QEMU
from scratch and re-run tests with V=1 environment variable set**
ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion
failed: (all_tables_match)
not ok /aarch64/acpi/virt/tpm2-crb -
ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion
failed: (all_tables_match)
Bail out!
qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM:
Resource temporarily unavailable
Unexpected error in qio_channel_socket_writev() at
../io/channel-socket.c:622:
/home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable
to write to socket: Bad file descriptor
Travis testing on s390x I see the following failures for this patchset
(search for 'ERROR'):
https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267230363
Summary of Failures:
134/320 qemu:qtest+qtest-aarch64 / qtest-aarch64/tpm-crb-device-test
ERROR 0.70s killed by signal 6 SIGABRT
219/320 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-test
ERROR 0.88s killed by signal 6 SIGABRT
Summary of Failures:
271/537 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-test
ERROR 0.59s killed by signal 6 SIGABRT
My guess is it's an endianess issue on big endian machines due to
reading from the ROM device where we lost the .endianess:
+const MemoryRegionOps tpm_crb_memory_ops = {
+ .read = tpm_crb_mmio_read,
+ .write = tpm_crb_mmio_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .valid = {
+ .min_access_size = 1,
+ .max_access_size = 4,
+ },
+};
I think we need a 2nd set of registers to support the endianess
conversion. It's not exactly nice, though. Basically the saved_regs
could be used for this directly, even though I did not do that but
introduced n_regs:
https://github.com/stefanberger/qemu-tpm/commit/90f6b21c0dd93dbb13d9e80a628f5b631fd07d91
This patch allows the tests on s390x to run farther but the execution of
the command doesn't seem to work maybe due to command data that were
also written in wrong endianess. I don't know. I would have to get
access to a big endian / s390 machine to be able to fix it.
The latest version now passes on Travis s390x:
https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267245220
Re: [PATCH v5 12/14] tests: acpi: implement TPM CRB tests for ARM virt
On 11/14/23 13:03, Stefan Berger wrote:
On 11/14/23 04:36, Marc-André Lureau wrote:
Hi
On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne wrote:
Signed-off-by: Joelle van Dyne
Reviewed-by: Stefan Berger
nit: you also added tests for x86, could be a different patch?
For arm, the test fails until next patch with:
# starting QEMU: exec ./qemu-system-aarch64 -qtest
unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev
socket,path=/tmp/qtest-991279.qmp,id=char0 -mon
chardev=char0,mode=control -display none -audio none -machine virt
-accel tcg -nodefaults -nographic -drive
if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on
-drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on
-cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2
-cpu cortex-a57 -chardev
socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock
-tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev
-accel qtest
Warning! zero length expected file
'tests/data/acpi/virt/TPM2.crb-device.tpm2'
Warning! zero length expected file
'tests/data/acpi/virt/DSDT.crb-device.tpm2'
acpi-test: Warning! binary file mismatch. Actual
[aml:/tmp/aml-GO4ME2], Expected
[aml:tests/data/acpi/virt/TPM2.crb-device.tpm2].
See source file tests/qtest/bios-tables-test.c for instructions on how
to update expected files.
acpi-test: Warning! binary file mismatch. Actual
[aml:/tmp/aml-6N4ME2], Expected
[aml:tests/data/acpi/virt/DSDT.crb-device.tpm2].
See source file tests/qtest/bios-tables-test.c for instructions on how
to update expected files.
to see ASL diff between mismatched files install IASL, rebuild QEMU
from scratch and re-run tests with V=1 environment variable set**
ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion
failed: (all_tables_match)
not ok /aarch64/acpi/virt/tpm2-crb -
ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion
failed: (all_tables_match)
Bail out!
qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM:
Resource temporarily unavailable
Unexpected error in qio_channel_socket_writev() at
../io/channel-socket.c:622:
/home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable
to write to socket: Bad file descriptor
Travis testing on s390x I see the following failures for this patchset
(search for 'ERROR'):
https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267230363
Summary of Failures:
134/320 qemu:qtest+qtest-aarch64 / qtest-aarch64/tpm-crb-device-test
ERROR 0.70s killed by signal 6 SIGABRT
219/320 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-test
ERROR 0.88s killed by signal 6 SIGABRT
Summary of Failures:
271/537 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-test
ERROR 0.59s killed by signal 6 SIGABRT
My guess is it's an endianess issue on big endian machines due to
reading from the ROM device where we lost the .endianess:
+const MemoryRegionOps tpm_crb_memory_ops = {
+ .read = tpm_crb_mmio_read,
+ .write = tpm_crb_mmio_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .valid = {
+ .min_access_size = 1,
+ .max_access_size = 4,
+ },
+};
I think we need a 2nd set of registers to support the endianess
conversion. It's not exactly nice, though. Basically the saved_regs
could be used for this directly, even though I did not do that but
introduced n_regs:
https://github.com/stefanberger/qemu-tpm/commit/90f6b21c0dd93dbb13d9e80a628f5b631fd07d91
This patch allows the tests on s390x to run farther but the execution of
the command doesn't seem to work maybe due to command data that were
also written in wrong endianess. I don't know. I would have to get
access to a big endian / s390 machine to be able to fix it.
Re: [PATCH v5 12/14] tests: acpi: implement TPM CRB tests for ARM virt
On 11/14/23 04:36, Marc-André Lureau wrote:
Hi
On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne wrote:
Signed-off-by: Joelle van Dyne
Reviewed-by: Stefan Berger
nit: you also added tests for x86, could be a different patch?
For arm, the test fails until next patch with:
# starting QEMU: exec ./qemu-system-aarch64 -qtest
unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev
socket,path=/tmp/qtest-991279.qmp,id=char0 -mon
chardev=char0,mode=control -display none -audio none -machine virt
-accel tcg -nodefaults -nographic -drive
if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on
-drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on
-cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2
-cpu cortex-a57 -chardev
socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock
-tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev
-accel qtest
Warning! zero length expected file 'tests/data/acpi/virt/TPM2.crb-device.tpm2'
Warning! zero length expected file 'tests/data/acpi/virt/DSDT.crb-device.tpm2'
acpi-test: Warning! binary file mismatch. Actual
[aml:/tmp/aml-GO4ME2], Expected
[aml:tests/data/acpi/virt/TPM2.crb-device.tpm2].
See source file tests/qtest/bios-tables-test.c for instructions on how
to update expected files.
acpi-test: Warning! binary file mismatch. Actual
[aml:/tmp/aml-6N4ME2], Expected
[aml:tests/data/acpi/virt/DSDT.crb-device.tpm2].
See source file tests/qtest/bios-tables-test.c for instructions on how
to update expected files.
to see ASL diff between mismatched files install IASL, rebuild QEMU
from scratch and re-run tests with V=1 environment variable set**
ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion
failed: (all_tables_match)
not ok /aarch64/acpi/virt/tpm2-crb -
ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion
failed: (all_tables_match)
Bail out!
qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM:
Resource temporarily unavailable
Unexpected error in qio_channel_socket_writev() at ../io/channel-socket.c:622:
/home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable
to write to socket: Bad file descriptor
Travis testing on s390x I see the following failures for this patchset
(search for 'ERROR'):
https://app.travis-ci.com/github/stefanberger/qemu-tpm/builds/267230363
Summary of Failures:
134/320 qemu:qtest+qtest-aarch64 / qtest-aarch64/tpm-crb-device-test
ERROR 0.70s killed by signal 6 SIGABRT
219/320 qemu:qtest+qtest-x86_64 / qtest-x86_64/tpm-crb-test
ERROR 0.88s killed by signal 6 SIGABRT
Summary of Failures:
271/537 qemu:qtest+qtest-i386 / qtest-i386/tpm-crb-test
ERROR 0.59s killed by signal 6 SIGABRT
My guess is it's an endianess issue on big endian machines due to
reading from the ROM device where we lost the .endianess:
+const MemoryRegionOps tpm_crb_memory_ops = {
+.read = tpm_crb_mmio_read,
+.write = tpm_crb_mmio_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+.valid = {
+.min_access_size = 1,
+.max_access_size = 4,
+},
+};
Re: [PATCH v5 05/14] tpm_crb: move ACPI table building to device interface
On 11/14/23 11:37, Stefan Berger wrote:
On 11/13/23 21:09, Joelle van Dyne wrote:
This logic is similar to TPM TIS ISA device. Since TPM CRB can only
support TPM 2.0 backends, we check for this in realize.
Signed-off-by: Joelle van Dyne
---
hw/tpm/tpm_crb.h | 2 ++
hw/i386/acpi-build.c | 16 +---
hw/tpm/tpm_crb.c | 16
hw/tpm/tpm_crb_common.c | 19 +++
4 files changed, 38 insertions(+), 15 deletions(-)
diff --git a/hw/tpm/tpm_crb.h b/hw/tpm/tpm_crb.h
index 36863e1664..e6a86e3fd1 100644
--- a/hw/tpm/tpm_crb.h
+++ b/hw/tpm/tpm_crb.h
@@ -73,5 +73,7 @@ void tpm_crb_init_memory(Object *obj, TPMCRBState
*s, Error **errp);
void tpm_crb_mem_save(TPMCRBState *s, uint32_t *saved_regs, void
*saved_cmdmem);
void tpm_crb_mem_load(TPMCRBState *s, const uint32_t *saved_regs,
const void *saved_cmdmem);
+void tpm_crb_build_aml(TPMIf *ti, Aml *scope, uint32_t baseaddr,
uint32_t size,
+ bool build_ppi);
#endif /* TPM_TPM_CRB_H */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 80db183b78..7491cee2af 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1792,21 +1792,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
#ifdef CONFIG_TPM
if (TPM_IS_CRB(tpm)) {
- dev = aml_device("TPM");
- aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
- aml_append(dev, aml_name_decl("_STR",
- aml_string("TPM 2.0 Device")));
- crs = aml_resource_template();
- aml_append(crs, aml_memory32_fixed(TPM_CRB_ADDR_BASE,
- TPM_CRB_ADDR_SIZE,
AML_READ_WRITE));
- aml_append(dev, aml_name_decl("_CRS", crs));
-
- aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
- aml_append(dev, aml_name_decl("_UID", aml_int(1)));
-
- tpm_build_ppi_acpi(tpm, dev);
-
- aml_append(sb_scope, dev);
+ call_dev_aml_func(DEVICE(tpm), scope);
For an x86_64 VM we have to call it directly otherwise the ACPI table
won't be there.
tpm_crb_build_aml(tpm, sb_scope, TPM_CRB_ADDR_BASE,
TPM_CRB_ADDR_SIZE, true);
I looks like a good place for the moved code would be in hw/acpi/tpm.c
Re: [PATCH v5 05/14] tpm_crb: move ACPI table building to device interface
On 11/13/23 21:09, Joelle van Dyne wrote:
This logic is similar to TPM TIS ISA device. Since TPM CRB can only
support TPM 2.0 backends, we check for this in realize.
Signed-off-by: Joelle van Dyne
---
hw/tpm/tpm_crb.h| 2 ++
hw/i386/acpi-build.c| 16 +---
hw/tpm/tpm_crb.c| 16
hw/tpm/tpm_crb_common.c | 19 +++
4 files changed, 38 insertions(+), 15 deletions(-)
diff --git a/hw/tpm/tpm_crb.h b/hw/tpm/tpm_crb.h
index 36863e1664..e6a86e3fd1 100644
--- a/hw/tpm/tpm_crb.h
+++ b/hw/tpm/tpm_crb.h
@@ -73,5 +73,7 @@ void tpm_crb_init_memory(Object *obj, TPMCRBState *s, Error
**errp);
void tpm_crb_mem_save(TPMCRBState *s, uint32_t *saved_regs, void
*saved_cmdmem);
void tpm_crb_mem_load(TPMCRBState *s, const uint32_t *saved_regs,
const void *saved_cmdmem);
+void tpm_crb_build_aml(TPMIf *ti, Aml *scope, uint32_t baseaddr, uint32_t size,
+ bool build_ppi);
#endif /* TPM_TPM_CRB_H */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 80db183b78..7491cee2af 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1792,21 +1792,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
#ifdef CONFIG_TPM
if (TPM_IS_CRB(tpm)) {
-dev = aml_device("TPM");
-aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
-aml_append(dev, aml_name_decl("_STR",
- aml_string("TPM 2.0 Device")));
-crs = aml_resource_template();
-aml_append(crs, aml_memory32_fixed(TPM_CRB_ADDR_BASE,
- TPM_CRB_ADDR_SIZE, AML_READ_WRITE));
-aml_append(dev, aml_name_decl("_CRS", crs));
-
-aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
-aml_append(dev, aml_name_decl("_UID", aml_int(1)));
-
-tpm_build_ppi_acpi(tpm, dev);
-
-aml_append(sb_scope, dev);
+call_dev_aml_func(DEVICE(tpm), scope);
For an x86_64 VM we have to call it directly otherwise the ACPI table
won't be there.
tpm_crb_build_aml(tpm, sb_scope, TPM_CRB_ADDR_BASE,
TPM_CRB_ADDR_SIZE, true);
Re: [PATCH v5 04/14] tpm_crb: use a single read-as-mem/write-as-mmio mapping
On 11/13/23 21:09, Joelle van Dyne wrote:
On Apple Silicon, when Windows performs a LDP on the CRB MMIO space,
the exception is not decoded by hardware and we cannot trap the MMIO
read. This led to the idea from @agraf to use the same mapping type as
ROM devices: namely that reads should be seen as memory type and
writes should trap as MMIO.
Once that was done, the second memory mapping of the command buffer
region was redundent and was removed.
A note about the removal of the read trap for `CRB_LOC_STATE`:
The only usage was to return the most up-to-date value for
`tpmEstablished`. However, `tpmEstablished` is only cleared when a
TPM2_HashStart operation is called which only exists for locality 4.
We do not handle locality 4. Indeed, the comment for the write handler
of `CRB_LOC_CTRL` makes the same argument for why it is not calling
the backend to reset the `tpmEstablished` bit (to 1).
As this bit is unused, we do not need to worry about updating it for
reads.
In order to maintain migration compatibility with older versions of
QEMU, we store a copy of the register data and command data which is
used only during save/restore.
Signed-off-by: Joelle van Dyne
Reviewed-by: Stefan Berger
---
hw/tpm/tpm_crb.h| 5 +-
hw/tpm/tpm_crb.c| 30 -
hw/tpm/tpm_crb_common.c | 145 +++-
3 files changed, 114 insertions(+), 66 deletions(-)
diff --git a/hw/tpm/tpm_crb.h b/hw/tpm/tpm_crb.h
index da3a0cf256..36863e1664 100644
--- a/hw/tpm/tpm_crb.h
+++ b/hw/tpm/tpm_crb.h
@@ -26,9 +26,7 @@
typedef struct TPMCRBState {
TPMBackend *tpmbe;
TPMBackendCmd cmd;
-uint32_t regs[TPM_CRB_R_MAX];
MemoryRegion mmio;
-MemoryRegion cmdmem;
size_t be_buffer_size;
@@ -72,5 +70,8 @@ enum TPMVersion tpm_crb_get_version(TPMCRBState *s);
int tpm_crb_pre_save(TPMCRBState *s);
void tpm_crb_reset(TPMCRBState *s, uint64_t baseaddr);
void tpm_crb_init_memory(Object *obj, TPMCRBState *s, Error **errp);
+void tpm_crb_mem_save(TPMCRBState *s, uint32_t *saved_regs, void
*saved_cmdmem);
+void tpm_crb_mem_load(TPMCRBState *s, const uint32_t *saved_regs,
+ const void *saved_cmdmem);
#endif /* TPM_TPM_CRB_H */
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 598c3e0161..99c64dd72a 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -37,6 +37,10 @@ struct CRBState {
DeviceState parent_obj;
TPMCRBState state;
+
+/* These states are only for migration */
+uint32_t saved_regs[TPM_CRB_R_MAX];
+MemoryRegion saved_cmdmem;
};
typedef struct CRBState CRBState;
@@ -57,18 +61,36 @@ static enum TPMVersion tpm_crb_none_get_version(TPMIf *ti)
return tpm_crb_get_version(>state);
}
+/**
+ * For migrating to an older version of QEMU
+ */
static int tpm_crb_none_pre_save(void *opaque)
{
CRBState *s = opaque;
+void *saved_cmdmem = memory_region_get_ram_ptr(>saved_cmdmem);
+tpm_crb_mem_save(>state, s->saved_regs, saved_cmdmem);
return tpm_crb_pre_save(>state);
}
+/**
+ * For migrating from an older version of QEMU
+ */
+static int tpm_crb_none_post_load(void *opaque, int version_id)
+{
+CRBState *s = opaque;
+void *saved_cmdmem = memory_region_get_ram_ptr(>saved_cmdmem);
+
+tpm_crb_mem_load(>state, s->saved_regs, saved_cmdmem);
+return 0;
+}
+
static const VMStateDescription vmstate_tpm_crb_none = {
.name = "tpm-crb",
.pre_save = tpm_crb_none_pre_save,
+.post_load = tpm_crb_none_post_load,
.fields = (VMStateField[]) {
-VMSTATE_UINT32_ARRAY(state.regs, CRBState, TPM_CRB_R_MAX),
+VMSTATE_UINT32_ARRAY(saved_regs, CRBState, TPM_CRB_R_MAX),
VMSTATE_END_OF_LIST(),
}
};
@@ -101,10 +123,12 @@ static void tpm_crb_none_realize(DeviceState *dev, Error
**errp)
tpm_crb_init_memory(OBJECT(s), >state, errp);
+/* only used for migration */
+memory_region_init_ram(>saved_cmdmem, OBJECT(s),
+"tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
+
memory_region_add_subregion(get_system_memory(),
TPM_CRB_ADDR_BASE, >state.mmio);
-memory_region_add_subregion(get_system_memory(),
-TPM_CRB_ADDR_BASE + sizeof(s->state.regs), >state.cmdmem);
if (s->state.ppi_enabled) {
memory_region_add_subregion(get_system_memory(),
diff --git a/hw/tpm/tpm_crb_common.c b/hw/tpm/tpm_crb_common.c
index bee0b71fee..f96a8cf299 100644
--- a/hw/tpm/tpm_crb_common.c
+++ b/hw/tpm/tpm_crb_common.c
@@ -31,31 +31,12 @@
#include "qom/object.h"
#include "tpm_crb.h"
-static uint64_t tpm_crb_mmio_read(void *opaque, hwaddr addr,
- unsigned size)
+static uint8_t tpm_crb_get_active_locty(TPMCRBState *s, uint32_t *regs)
{
-TPMCRBState *s = opaque;
-void *regs = (void *)>regs + (addr & ~3);
-unsigned offset = addr & 3;
-uint32_t val = *(ui
Re: [PATCH v5 12/14] tests: acpi: implement TPM CRB tests for ARM virt
On 11/14/23 04:36, Marc-André Lureau wrote: Hi On Tue, Nov 14, 2023 at 6:12 AM Joelle van Dyne wrote: Signed-off-by: Joelle van Dyne Reviewed-by: Stefan Berger nit: you also added tests for x86, could be a different patch? For arm, the test fails until next patch with: # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-991279.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-991279.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine virt -accel tcg -nodefaults -nographic -drive if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 -cpu cortex-a57 -chardev socket,id=chr,path=/tmp/qemu-test_acpi_virt_tcg_crb-device.KZ3GE2/sock -tpmdev emulator,id=dev,chardev=chr -device tpm-crb-device,tpmdev=dev -accel qtest Warning! zero length expected file 'tests/data/acpi/virt/TPM2.crb-device.tpm2' Warning! zero length expected file 'tests/data/acpi/virt/DSDT.crb-device.tpm2' acpi-test: Warning! binary file mismatch. Actual [aml:/tmp/aml-GO4ME2], Expected [aml:tests/data/acpi/virt/TPM2.crb-device.tpm2]. See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files. acpi-test: Warning! binary file mismatch. Actual [aml:/tmp/aml-6N4ME2], Expected [aml:tests/data/acpi/virt/DSDT.crb-device.tpm2]. See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files. to see ASL diff between mismatched files install IASL, rebuild QEMU from scratch and re-run tests with V=1 environment variable set** ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion failed: (all_tables_match) not ok /aarch64/acpi/virt/tpm2-crb - ERROR:../tests/qtest/bios-tables-test.c:538:test_acpi_asl: assertion failed: (all_tables_match) Bail out! qemu-system-aarch64: tpm-emulator: Could not cleanly shutdown the TPM: Resource temporarily unavailable Unexpected error in qio_channel_socket_writev() at ../io/channel-socket.c:622: /home/elmarco/src/qemu/buildall/tests/qtest/bios-tables-test: Unable to write to socket: Bad file descriptor I had seen errors like these as well. However, the first error appears when applying the series after getting it using 'b4 am' -- it complains about patch 13. I always take patch 13 & 14 from my email client. Both of these here pass: QTEST_QEMU_BINARY=./qemu-system-aarch64 ./tests/qtest/bios-tables-test QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/bios-tables-test
Re: [PATCH v4 05/14] tpm_crb: move ACPI table building to device interface
On 11/2/23 22:37, Joelle van Dyne wrote:
On Thu, Nov 2, 2023 at 11:50 AM Stefan Berger wrote:
On 10/31/23 00:00, Joelle van Dyne wrote:
This logic is similar to TPM TIS ISA device. Since TPM CRB can only
support TPM 2.0 backends, we check for this in realize.
The problem on x86_64 is that the creation of the ACPI doesn't seem to
get invoked. The device then ends up not working under Linux. The
problem seems to be
.parent = TYPE_DEVICE
When I change this to TYPE_ISA_DEVICE it starts generating the ACPI
table. I am not sure what other side effects this may have, though.
Ah sorry, this is probably a side effect of the patch I dropped where
the bus was moved back from ISA to SysBus. The patch was dropped
because people complained that the side effects of a new device
appearing on the ISA bus during migration is unknown. That means we
will probably have to add some logic to call the ACPI build methods on
non-ISA devices.
If you moved tpm_crb_build_aml() into a common file like tpm_acpi.c as
tpm_build_aml() it could be called from multiple drivers. For the x86_64
driver I would just call this function from its old location then.
Stefan
Stefan
Signed-off-by: Joelle van Dyne
Reviewed-by: Stefan Berger
---
hw/tpm/tpm_crb.h| 2 ++
hw/i386/acpi-build.c| 23 ---
hw/tpm/tpm_crb.c| 16
hw/tpm/tpm_crb_common.c | 19 +++
4 files changed, 37 insertions(+), 23 deletions(-)
diff --git a/hw/tpm/tpm_crb.h b/hw/tpm/tpm_crb.h
index 36863e1664..e6a86e3fd1 100644
--- a/hw/tpm/tpm_crb.h
+++ b/hw/tpm/tpm_crb.h
@@ -73,5 +73,7 @@ void tpm_crb_init_memory(Object *obj, TPMCRBState *s, Error
**errp);
void tpm_crb_mem_save(TPMCRBState *s, uint32_t *saved_regs, void
*saved_cmdmem);
void tpm_crb_mem_load(TPMCRBState *s, const uint32_t *saved_regs,
const void *saved_cmdmem);
+void tpm_crb_build_aml(TPMIf *ti, Aml *scope, uint32_t baseaddr, uint32_t size,
+ bool build_ppi);
#endif /* TPM_TPM_CRB_H */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 80db183b78..ce3f7b2d91 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1441,9 +1441,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
uint32_t nr_mem = machine->ram_slots;
int root_bus_limit = 0xFF;
PCIBus *bus = NULL;
-#ifdef CONFIG_TPM
-TPMIf *tpm = tpm_find();
-#endif
bool cxl_present = false;
int i;
VMBusBridge *vmbus_bridge = vmbus_bridge_find();
@@ -1790,26 +1787,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
}
}
-#ifdef CONFIG_TPM
-if (TPM_IS_CRB(tpm)) {
-dev = aml_device("TPM");
-aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
-aml_append(dev, aml_name_decl("_STR",
- aml_string("TPM 2.0 Device")));
-crs = aml_resource_template();
-aml_append(crs, aml_memory32_fixed(TPM_CRB_ADDR_BASE,
- TPM_CRB_ADDR_SIZE, AML_READ_WRITE));
-aml_append(dev, aml_name_decl("_CRS", crs));
-
-aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
-aml_append(dev, aml_name_decl("_UID", aml_int(1)));
-
-tpm_build_ppi_acpi(tpm, dev);
-
-aml_append(sb_scope, dev);
-}
-#endif
-
if (pcms->sgx_epc.size != 0) {
uint64_t epc_base = pcms->sgx_epc.base;
uint64_t epc_size = pcms->sgx_epc.size;
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 99c64dd72a..8d57295b15 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -19,6 +19,8 @@
#include "qemu/module.h"
#include "qapi/error.h"
#include "exec/address-spaces.h"
+#include "hw/acpi/acpi_aml_interface.h"
+#include "hw/acpi/tpm.h"
#include "hw/qdev-properties.h"
#include "hw/pci/pci_ids.h"
#include "hw/acpi/tpm.h"
@@ -121,6 +123,11 @@ static void tpm_crb_none_realize(DeviceState *dev, Error
**errp)
return;
}
+if (tpm_crb_none_get_version(TPM_IF(s)) != TPM_VERSION_2_0) {
+error_setg(errp, "TPM CRB only supports TPM 2.0 backends");
+return;
+}
+
tpm_crb_init_memory(OBJECT(s), >state, errp);
/* only used for migration */
@@ -142,10 +149,17 @@ static void tpm_crb_none_realize(DeviceState *dev, Error
**errp)
}
}
+static void build_tpm_crb_none_aml(AcpiDevAmlIf *adev, Aml *scope)
+{
+tpm_crb_build_aml(TPM_IF(adev), scope, TPM_CRB_ADDR_BASE,
TPM_CRB_ADDR_SIZE,
+ true);
+}
+
static void tpm_crb_none_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
TPMIfClass *tc = TPM_IF_CLASS(klass);
+AcpiDevAmlIfClass *adevc = ACPI
Re: [PATCH v4 05/14] tpm_crb: move ACPI table building to device interface
On 10/31/23 00:00, Joelle van Dyne wrote:
This logic is similar to TPM TIS ISA device. Since TPM CRB can only
support TPM 2.0 backends, we check for this in realize.
The problem on x86_64 is that the creation of the ACPI doesn't seem to
get invoked. The device then ends up not working under Linux. The
problem seems to be
.parent = TYPE_DEVICE
When I change this to TYPE_ISA_DEVICE it starts generating the ACPI
table. I am not sure what other side effects this may have, though.
Stefan
Signed-off-by: Joelle van Dyne
Reviewed-by: Stefan Berger
---
hw/tpm/tpm_crb.h| 2 ++
hw/i386/acpi-build.c| 23 ---
hw/tpm/tpm_crb.c| 16
hw/tpm/tpm_crb_common.c | 19 +++
4 files changed, 37 insertions(+), 23 deletions(-)
diff --git a/hw/tpm/tpm_crb.h b/hw/tpm/tpm_crb.h
index 36863e1664..e6a86e3fd1 100644
--- a/hw/tpm/tpm_crb.h
+++ b/hw/tpm/tpm_crb.h
@@ -73,5 +73,7 @@ void tpm_crb_init_memory(Object *obj, TPMCRBState *s, Error
**errp);
void tpm_crb_mem_save(TPMCRBState *s, uint32_t *saved_regs, void
*saved_cmdmem);
void tpm_crb_mem_load(TPMCRBState *s, const uint32_t *saved_regs,
const void *saved_cmdmem);
+void tpm_crb_build_aml(TPMIf *ti, Aml *scope, uint32_t baseaddr, uint32_t size,
+ bool build_ppi);
#endif /* TPM_TPM_CRB_H */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 80db183b78..ce3f7b2d91 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1441,9 +1441,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
uint32_t nr_mem = machine->ram_slots;
int root_bus_limit = 0xFF;
PCIBus *bus = NULL;
-#ifdef CONFIG_TPM
-TPMIf *tpm = tpm_find();
-#endif
bool cxl_present = false;
int i;
VMBusBridge *vmbus_bridge = vmbus_bridge_find();
@@ -1790,26 +1787,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
}
}
-#ifdef CONFIG_TPM
-if (TPM_IS_CRB(tpm)) {
-dev = aml_device("TPM");
-aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
-aml_append(dev, aml_name_decl("_STR",
- aml_string("TPM 2.0 Device")));
-crs = aml_resource_template();
-aml_append(crs, aml_memory32_fixed(TPM_CRB_ADDR_BASE,
- TPM_CRB_ADDR_SIZE, AML_READ_WRITE));
-aml_append(dev, aml_name_decl("_CRS", crs));
-
-aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
-aml_append(dev, aml_name_decl("_UID", aml_int(1)));
-
-tpm_build_ppi_acpi(tpm, dev);
-
-aml_append(sb_scope, dev);
-}
-#endif
-
if (pcms->sgx_epc.size != 0) {
uint64_t epc_base = pcms->sgx_epc.base;
uint64_t epc_size = pcms->sgx_epc.size;
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 99c64dd72a..8d57295b15 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -19,6 +19,8 @@
#include "qemu/module.h"
#include "qapi/error.h"
#include "exec/address-spaces.h"
+#include "hw/acpi/acpi_aml_interface.h"
+#include "hw/acpi/tpm.h"
#include "hw/qdev-properties.h"
#include "hw/pci/pci_ids.h"
#include "hw/acpi/tpm.h"
@@ -121,6 +123,11 @@ static void tpm_crb_none_realize(DeviceState *dev, Error
**errp)
return;
}
+if (tpm_crb_none_get_version(TPM_IF(s)) != TPM_VERSION_2_0) {
+error_setg(errp, "TPM CRB only supports TPM 2.0 backends");
+return;
+}
+
tpm_crb_init_memory(OBJECT(s), >state, errp);
/* only used for migration */
@@ -142,10 +149,17 @@ static void tpm_crb_none_realize(DeviceState *dev, Error
**errp)
}
}
+static void build_tpm_crb_none_aml(AcpiDevAmlIf *adev, Aml *scope)
+{
+tpm_crb_build_aml(TPM_IF(adev), scope, TPM_CRB_ADDR_BASE,
TPM_CRB_ADDR_SIZE,
+ true);
+}
+
static void tpm_crb_none_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
TPMIfClass *tc = TPM_IF_CLASS(klass);
+AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass);
dc->realize = tpm_crb_none_realize;
device_class_set_props(dc, tpm_crb_none_properties);
@@ -154,6 +168,7 @@ static void tpm_crb_none_class_init(ObjectClass *klass,
void *data)
tc->model = TPM_MODEL_TPM_CRB;
tc->get_version = tpm_crb_none_get_version;
tc->request_completed = tpm_crb_none_request_completed;
+adevc->build_dev_aml = build_tpm_crb_none_aml;
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
}
@@ -166,6 +181,7 @@ static const TypeInfo tpm_crb_none_info = {
.class_init = tpm_crb_none_class_init,
.interfaces = (InterfaceInfo[]) {
{ TYPE_TPM_IF },
+{ TYPE_ACPI_DEV_AML_IF },
Re: [PATCH v4 04/14] tpm_crb: use a single read-as-mem/write-as-mmio mapping
On 10/31/23 00:00, Joelle van Dyne wrote:
On Apple Silicon, when Windows performs a LDP on the CRB MMIO space,
the exception is not decoded by hardware and we cannot trap the MMIO
read. This led to the idea from @agraf to use the same mapping type as
ROM devices: namely that reads should be seen as memory type and
writes should trap as MMIO.
Once that was done, the second memory mapping of the command buffer
region was redundent and was removed.
A note about the removal of the read trap for `CRB_LOC_STATE`:
The only usage was to return the most up-to-date value for
`tpmEstablished`. However, `tpmEstablished` is only cleared when a
TPM2_HashStart operation is called which only exists for locality 4.
We do not handle locality 4. Indeed, the comment for the write handler
of `CRB_LOC_CTRL` makes the same argument for why it is not calling
the backend to reset the `tpmEstablished` bit (to 1).
As this bit is unused, we do not need to worry about updating it for
reads.
In order to maintain migration compatibility with older versions of
QEMU, we store a copy of the register data and command data which is
used only during save/restore.
Signed-off-by: Joelle van Dyne
---
diff --git a/hw/tpm/tpm_crb_common.c b/hw/tpm/tpm_crb_common.c
index bee0b71fee..605e8576e9 100644
--- a/hw/tpm/tpm_crb_common.c
+++ b/hw/tpm/tpm_crb_common.c
@@ -31,31 +31,12 @@
#include "qom/object.h"
#include "tpm_crb.h"
-static uint64_t tpm_crb_mmio_read(void *opaque, hwaddr addr,
- unsigned size)
+static uint8_t tpm_crb_get_active_locty(TPMCRBState *s, uint32_t *regs)
{
-TPMCRBState *s = opaque;
-void *regs = (void *)>regs + (addr & ~3);
-unsigned offset = addr & 3;
-uint32_t val = *(uint32_t *)regs >> (8 * offset);
-
-switch (addr) {
-case A_CRB_LOC_STATE:
-val |= !tpm_backend_get_tpm_established_flag(s->tpmbe);
-break;
-}
-
-trace_tpm_crb_mmio_read(addr, size, val);
-
-return val;
-}
-
-static uint8_t tpm_crb_get_active_locty(TPMCRBState *s)
-{
-if (!ARRAY_FIELD_EX32(s->regs, CRB_LOC_STATE, locAssigned)) {
+if (!ARRAY_FIELD_EX32(regs, CRB_LOC_STATE, locAssigned)) {
return TPM_CRB_NO_LOCALITY;
}
-return ARRAY_FIELD_EX32(s->regs, CRB_LOC_STATE, activeLocality);
+return ARRAY_FIELD_EX32(regs, CRB_LOC_STATE, activeLocality);
}
static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
@@ -63,35 +44,47 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
{
TPMCRBState *s = opaque;
uint8_t locty = addr >> 12;
+uint32_t *regs;
+void *mem;
trace_tpm_crb_mmio_write(addr, size, val);
+regs = memory_region_get_ram_ptr(>mmio);
+mem = [R_CRB_DATA_BUFFER];
+assert(regs);
+
+if (addr >= A_CRB_DATA_BUFFER) {
Can you write here /* receive TPM command bytes */ ?
+assert(addr + size <= TPM_CRB_ADDR_SIZE);
+assert(size <= sizeof(val));
+memcpy(mem + addr - A_CRB_DATA_BUFFER, , size);
+memory_region_set_dirty(>mmio, addr, size);
+return;
+}
switch (addr) {
case A_CRB_CTRL_REQ:
switch (val) {
case CRB_CTRL_REQ_CMD_READY:
-ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
+ARRAY_FIELD_DP32(regs, CRB_CTRL_STS,
tpmIdle, 0);
break;
case CRB_CTRL_REQ_GO_IDLE:
-ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
+ARRAY_FIELD_DP32(regs, CRB_CTRL_STS,
tpmIdle, 1);
break;
}
break;
case A_CRB_CTRL_CANCEL:
if (val == CRB_CANCEL_INVOKE &&
-s->regs[R_CRB_CTRL_START] & CRB_START_INVOKE) {
+regs[R_CRB_CTRL_START] & CRB_START_INVOKE) {
tpm_backend_cancel_cmd(s->tpmbe);
}
break;
case A_CRB_CTRL_START:
if (val == CRB_START_INVOKE &&
-!(s->regs[R_CRB_CTRL_START] & CRB_START_INVOKE) &&
-tpm_crb_get_active_locty(s) == locty) {
-void *mem = memory_region_get_ram_ptr(>cmdmem);
+!(regs[R_CRB_CTRL_START] & CRB_START_INVOKE) &&
+tpm_crb_get_active_locty(s, regs) == locty) {
-s->regs[R_CRB_CTRL_START] |= CRB_START_INVOKE;
+regs[R_CRB_CTRL_START] |= CRB_START_INVOKE;
s->cmd = (TPMBackendCmd) {
.in = mem,
.in_len = MIN(tpm_cmd_get_size(mem), s->be_buffer_size),
@@ -108,26 +101,27 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
/* not loc 3 or 4 */
break;
case CRB_LOC_CTRL_RELINQUISH:
-ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
+ARRAY_FIELD_DP32(regs, CRB_LOC_STATE,
locAssigned, 0);
-ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
+ARRAY_FIELD_DP32(regs, CRB_LOC_STS,
Granted, 0);
break;
Re: [PATCH v4 11/14] tpm_crb_sysbus: introduce TPM CRB SysBus device
On 10/31/23 00:00, Joelle van Dyne wrote:
This SysBus variant of the CRB interface supports dynamically locating
the MMIO interface so that Virt machines can use it. This interface
is currently the only one supported by QEMU that works on Windows 11
ARM64 as 'tpm-tis-device' does not work with current Windows drivers.
We largely follow that device as a template.
To try out this device with Windows 11 before OVMF is updated, you
will need to modify `sysbus-fdt.c` and change the added line from:
```c
TYPE_BINDING(TYPE_TPM_CRB_SYSBUS, no_fdt_node),
```
to
```c
TYPE_BINDING(TYPE_TPM_CRB_SYSBUS, add_tpm_tis_fdt_node),
```
This change was not included because it can confuse Linux (although
from testing, it seems like Linux is able to properly ignore the
device from the TPM TIS driver and recognize it from the ACPI device
in the TPM CRB driver). A proper fix would require OVMF to recognize
the ACPI device and not depend on the FDT node for recognizing TPM.
The command line to try out this device with SWTPM is:
```
$ qemu-system-aarch64 \
-chardev socket,id=chrtpm0,path=tpm.sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm0 \
-device tpm-crb-device,tpmdev=tpm0
```
along with SWTPM:
```
$ swtpm \
--ctrl type=unixio,path=tpm.sock,terminate \
--tpmstate backend-uri=file://tpm.data \
--tpm2
```
Signed-off-by: Joelle van Dyne
I wonder whether we will get someone to test this on loongarch and/or riscv.
Reviewed-by: Stefan Berger
---
docs/specs/tpm.rst | 1 +
include/sysemu/tpm.h| 3 +
hw/acpi/aml-build.c | 7 +-
hw/arm/virt.c | 1 +
hw/core/sysbus-fdt.c| 1 +
hw/loongarch/virt.c | 1 +
hw/riscv/virt.c | 1 +
hw/tpm/tpm_crb_sysbus.c | 162
hw/arm/Kconfig | 1 +
hw/loongarch/Kconfig| 1 +
hw/riscv/Kconfig| 1 +
hw/tpm/Kconfig | 5 ++
hw/tpm/meson.build | 3 +
13 files changed, 187 insertions(+), 1 deletion(-)
create mode 100644 hw/tpm/tpm_crb_sysbus.c
diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst
index 2bc29c9804..95aeb49220 100644
--- a/docs/specs/tpm.rst
+++ b/docs/specs/tpm.rst
@@ -46,6 +46,7 @@ operating system.
QEMU files related to TPM CRB interface:
- ``hw/tpm/tpm_crb.c``
- ``hw/tpm/tpm_crb_common.c``
+ - ``hw/tpm/tpm_crb_sysbus.c``
SPAPR interface
---
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index ffd300e607..bab30fa546 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -49,6 +49,7 @@ struct TPMIfClass {
#define TYPE_TPM_TIS_ISA"tpm-tis"
#define TYPE_TPM_TIS_SYSBUS "tpm-tis-device"
#define TYPE_TPM_CRB"tpm-crb"
+#define TYPE_TPM_CRB_SYSBUS "tpm-crb-device"
#define TYPE_TPM_SPAPR "tpm-spapr"
#define TYPE_TPM_TIS_I2C"tpm-tis-i2c"
@@ -58,6 +59,8 @@ struct TPMIfClass {
object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
#define TPM_IS_CRB(chr) \
object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
+#define TPM_IS_CRB_SYSBUS(chr) \
+object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB_SYSBUS)
#define TPM_IS_SPAPR(chr) \
object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)
#define TPM_IS_TIS_I2C(chr) \
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index af66bde0f5..acc654382e 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -31,6 +31,7 @@
#include "hw/pci/pci_bus.h"
#include "hw/pci/pci_bridge.h"
#include "qemu/cutils.h"
+#include "qom/object.h"
static GArray *build_alloc_array(void)
{
@@ -2218,7 +2219,7 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker,
GArray *tcpalog,
{
uint8_t start_method_params[12] = {};
unsigned log_addr_offset;
-uint64_t control_area_start_address;
+uint64_t baseaddr, control_area_start_address;
TPMIf *tpmif = tpm_find();
uint32_t start_method;
AcpiTable table = { .sig = "TPM2", .rev = 4,
@@ -2236,6 +2237,10 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker,
GArray *tcpalog,
} else if (TPM_IS_CRB(tpmif)) {
control_area_start_address = TPM_CRB_ADDR_CTRL;
start_method = TPM2_START_METHOD_CRB;
+} else if (TPM_IS_CRB_SYSBUS(tpmif)) {
+baseaddr = object_property_get_uint(OBJECT(tpmif), "x-baseaddr", NULL);
+control_area_start_address = baseaddr + A_CRB_CTRL_REQ;
+start_method = TPM2_START_METHOD_CRB;
} else {
g_assert_not_reached();
}
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f1a161b0ea..22e147f71a 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2950,6 +2950,7 @@ static void virt_machine_class_init(ObjectClass *oc, void
*data)
machine_cla
Re: [PATCH v4 06/14] tpm-sysbus: add plug handler for TPM on SysBus
On 10/31/23 00:00, Joelle van Dyne wrote:
TPM needs to know its own base address in order to generate its DSDT
device entry.
Signed-off-by: Joelle van Dyne
Reviewed-by: Stefan Berger
---
include/sysemu/tpm.h | 4
hw/tpm/tpm-sysbus.c | 47
hw/tpm/meson.build | 1 +
3 files changed, 52 insertions(+)
create mode 100644 hw/tpm/tpm-sysbus.c
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index 1ee568b3b6..ffd300e607 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -12,6 +12,8 @@
#ifndef QEMU_TPM_H
#define QEMU_TPM_H
+#include "qemu/osdep.h"
+#include "exec/hwaddr.h"
#include "qapi/qapi-types-tpm.h"
#include "qom/object.h"
@@ -78,6 +80,8 @@ static inline TPMVersion tpm_get_version(TPMIf *ti)
return TPM_IF_GET_CLASS(ti)->get_version(ti);
}
+void tpm_sysbus_plug(TPMIf *tpmif, Object *pbus, hwaddr pbus_base);
+
#else /* CONFIG_TPM */
#define tpm_init() (0)
diff --git a/hw/tpm/tpm-sysbus.c b/hw/tpm/tpm-sysbus.c
new file mode 100644
index 00..732ce34c73
--- /dev/null
+++ b/hw/tpm/tpm-sysbus.c
@@ -0,0 +1,47 @@
+/*
+ * tpm-sysbus.c - Support functions for SysBus TPM devices
+ *
+ * Copyright (c) 2023 QEMU contributors
+ *
+ * Authors:
+ * Joelle van Dyne
+ *
+ * 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 "sysemu/tpm.h"
+#include "hw/platform-bus.h"
+#include "hw/sysbus.h"
+#include "qapi/error.h"
+
+/**
+ * Called from a machine's pre_plug handler to set the device's physical addr.
+ */
+void tpm_sysbus_plug(TPMIf *tpmif, Object *pbus, hwaddr pbus_base)
+{
+PlatformBusDevice *pbusdev = PLATFORM_BUS_DEVICE(pbus);
+SysBusDevice *sbdev = SYS_BUS_DEVICE(tpmif);
+MemoryRegion *sbdev_mr;
+hwaddr tpm_base;
+uint64_t tpm_size;
+
+/* exit early if TPM is not a sysbus device */
+if (!object_dynamic_cast(OBJECT(tpmif), TYPE_SYS_BUS_DEVICE)) {
+return;
+}
+
+assert(object_dynamic_cast(pbus, TYPE_PLATFORM_BUS_DEVICE));
+
+tpm_base = platform_bus_get_mmio_addr(pbusdev, sbdev, 0);
+assert(tpm_base != -1);
+
+tpm_base += pbus_base;
+
+sbdev_mr = sysbus_mmio_get_region(sbdev, 0);
+tpm_size = memory_region_size(sbdev_mr);
+
+object_property_set_uint(OBJECT(sbdev), "x-baseaddr",
+ tpm_base, _abort);
+object_property_set_uint(OBJECT(sbdev), "x-size",
+ tpm_size, _abort);
+}
diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
index cb8204d5bc..3060ac05e8 100644
--- a/hw/tpm/meson.build
+++ b/hw/tpm/meson.build
@@ -1,6 +1,7 @@
system_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_tis_common.c'))
system_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: files('tpm_tis_isa.c'))
system_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true:
files('tpm_tis_sysbus.c'))
+system_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: files('tpm-sysbus.c'))
system_ss.add(when: 'CONFIG_TPM_TIS_I2C', if_true: files('tpm_tis_i2c.c'))
system_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
system_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb_common.c'))
Re: [PATCH v4 14/14] tests: add TPM-CRB sysbus tests for aarch64
On 10/31/23 00:00, Joelle van Dyne wrote:
- Factor out common test code from tpm-crb-test.c -> tpm-tests.c
- Store device addr in `tpm_device_base_addr` (unify with TIS tests)
- Add new tests for aarch64
Signed-off-by: Joelle van Dyne
Reviewed-by: Stefan Berger
---
tests/qtest/tpm-tests.h | 2 +
tests/qtest/tpm-util.h | 4 +-
tests/qtest/bios-tables-test.c | 4 +-
tests/qtest/tpm-crb-device-swtpm-test.c | 72 ++
tests/qtest/tpm-crb-device-test.c | 71 ++
tests/qtest/tpm-crb-swtpm-test.c| 2 +
tests/qtest/tpm-crb-test.c | 121 +---
tests/qtest/tpm-tests.c | 121
tests/qtest/tpm-tis-device-swtpm-test.c | 2 +-
tests/qtest/tpm-tis-device-test.c | 2 +-
tests/qtest/tpm-tis-i2c-test.c | 3 +
tests/qtest/tpm-tis-swtpm-test.c| 2 +-
tests/qtest/tpm-tis-test.c | 2 +-
tests/qtest/tpm-util.c | 16 ++--
tests/qtest/meson.build | 4 +
15 files changed, 295 insertions(+), 133 deletions(-)
create mode 100644 tests/qtest/tpm-crb-device-swtpm-test.c
create mode 100644 tests/qtest/tpm-crb-device-test.c
diff --git a/tests/qtest/tpm-tests.h b/tests/qtest/tpm-tests.h
index 07ba60d26e..c1bfb2f914 100644
--- a/tests/qtest/tpm-tests.h
+++ b/tests/qtest/tpm-tests.h
@@ -24,4 +24,6 @@ void tpm_test_swtpm_migration_test(const char *src_tpm_path,
const char *ifmodel,
const char *machine_options);
+void tpm_test_crb(const void *data);
+
#endif /* TESTS_TPM_TESTS_H */
diff --git a/tests/qtest/tpm-util.h b/tests/qtest/tpm-util.h
index 0cb28dd6e5..c99380684e 100644
--- a/tests/qtest/tpm-util.h
+++ b/tests/qtest/tpm-util.h
@@ -15,10 +15,10 @@
#include "io/channel-socket.h"
-extern uint64_t tpm_tis_base_addr;
+extern uint64_t tpm_device_base_addr;
#define TIS_REG(LOCTY, REG) \
-(tpm_tis_base_addr + ((LOCTY) << 12) + REG)
+(tpm_device_base_addr + ((LOCTY) << 12) + REG)
typedef void (tx_func)(QTestState *s,
const unsigned char *req, size_t req_size,
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index c63bad0205..dea2a18158 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -1343,7 +1343,7 @@ static void test_acpi_piix4_tcg_numamem(void)
free_test_data();
}
-uint64_t tpm_tis_base_addr;
+uint64_t tpm_device_base_addr;
static test_data tcg_tpm_test_data(const char *machine)
{
@@ -1379,7 +1379,7 @@ static void test_acpi_tcg_tpm(const char *machine, const
char *tpm_if,
const char *suffix = tpm_version == TPM_VERSION_2_0 ? "tpm2" : "tpm12";
char *args, *variant = g_strdup_printf(".%s.%s", tpm_if, suffix);
-tpm_tis_base_addr = base;
+tpm_device_base_addr = base;
module_call_init(MODULE_INIT_QOM);
diff --git a/tests/qtest/tpm-crb-device-swtpm-test.c
b/tests/qtest/tpm-crb-device-swtpm-test.c
new file mode 100644
index 00..332add5ca6
--- /dev/null
+++ b/tests/qtest/tpm-crb-device-swtpm-test.c
@@ -0,0 +1,72 @@
+/*
+ * QTest testcase for TPM CRB talking to external swtpm and swtpm migration
+ *
+ * Copyright (c) 2018 IBM Corporation
+ * with parts borrowed from migration-test.c that is:
+ * Copyright (c) 2016-2018 Red Hat, Inc. and/or its affiliates
+ *
+ * Authors:
+ * Stefan Berger
+ *
+ * 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 "libqtest.h"
+#include "qemu/module.h"
+#include "tpm-tests.h"
+#include "hw/acpi/tpm.h"
+
+uint64_t tpm_device_base_addr = 0xc00;
+#define MACHINE_OPTIONS "-machine virt,gic-version=max -accel tcg"
+
+typedef struct TestState {
+char *src_tpm_path;
+char *dst_tpm_path;
+char *uri;
+} TestState;
+
+static void tpm_crb_swtpm_test(const void *data)
+{
+const TestState *ts = data;
+
+tpm_test_swtpm_test(ts->src_tpm_path, tpm_util_crb_transfer,
+"tpm-crb-device", MACHINE_OPTIONS);
+}
+
+static void tpm_crb_swtpm_migration_test(const void *data)
+{
+const TestState *ts = data;
+
+tpm_test_swtpm_migration_test(ts->src_tpm_path, ts->dst_tpm_path, ts->uri,
+ tpm_util_crb_transfer, "tpm-crb-device",
+ MACHINE_OPTIONS);
+}
+
+int main(int argc, char **argv)
+{
+int ret;
+TestState ts = { 0 };
+
+ts.src_tpm_path = g_dir_make_tmp("qemu-tpm-crb-swtpm-test.XX", NULL);
+ts.dst_tpm_path = g_dir_make_tmp("qemu-tpm-crb-swtpm-test.XX", NULL);
+ts.uri = g_strdup_printf("unix:%
Re: [PATCH v4 13/14] tests: acpi: updated expected blobs for TPM CRB
On 10/31/23 00:00, Joelle van Dyne wrote:
Signed-off-by: Joelle van Dyne
Tested-by: Stefan Berger
---
tests/qtest/bios-tables-test-allowed-diff.h | 4
tests/data/acpi/q35/DSDT.crb.tpm2 | Bin 0 -> 8355 bytes
tests/data/acpi/q35/TPM2.crb.tpm2 | Bin 0 -> 76 bytes
tests/data/acpi/virt/DSDT.crb-device.tpm2 | Bin 0 -> 5276 bytes
tests/data/acpi/virt/TPM2.crb-device.tpm2 | Bin 0 -> 76 bytes
5 files changed, 4 deletions(-)
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h
b/tests/qtest/bios-tables-test-allowed-diff.h
index c2d1924c2f..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,5 +1 @@
/* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/q35/DSDT.crb.tpm2",
-"tests/data/acpi/q35/TPM2.crb.tpm2",
-"tests/data/acpi/virt/DSDT.crb.tpm2",
-"tests/data/acpi/virt/TPM2.crb.tpm2",
diff --git a/tests/data/acpi/q35/DSDT.crb.tpm2
b/tests/data/acpi/q35/DSDT.crb.tpm2
index
e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..fb89ae0ac6d4346e33156e9e4d3718698a0a1a8e
100644
GIT binary patch
literal 8355
zcmb7JOKcm*8J^`sS}m8-l3H7S#U`8>eWXYzJ1?|oP;!?qOOz=tWv8G4E+waxod8)R
zF%Ty(AS*!P_|$}T&?B8HKyU4-*Ba=hz4_JvJ@wE_u0;`_qJIC(jyR
z|M_Nj=UdMBf#3Umi815V>Lsrkl?YJV~mdJ*J)+0vi^==Z48WDDr5BTC~XclT0V1EX9t%8FLUoL=J{8a$7|Wqc45(S`t5`0mW9UwnDx
z{mR3i|KnHp-m)?PoX4+;-wP3ag&&31>2U0PF}iNtCOSX2JYM`_#7~Phht5PHwLGvz
z6Qx?-d&^zP*8HHIAHOoX!J`}kkI_Fg!iy?@uC#YS-
zrTUv~WpJ%1@T%q7MVzRvwYx^{k)ToFRo6D!rB2I#qtrL5tKJH8@o#Z>=UiuU)T
zZ9+u1jO^nXCjd(3^l0?srP<%;MljIp6xo$K)N^k)t=pcpkO!Uqsv2HV7CxINlr
zqfHxQv(Ii1jp6O#EyJ39I>6&|+UO?|M4RF=n4OkaXRbZKuMuri#S%~yOl!FkU<(jlNIwB^beO&;Npl_0M3hZoCl~3
ziHZCio8nAh*I0HosF6cTDsyZD_r=#g~be#xQodr#2LDN~#bs|)C
z7B!tkO=nTpiBQ$KsOenPbS~;T5vn>}O{c5rbakBwRh>sPokujCM|7PCRh>sQokumD
zM|GVDRh_UgF=z2vX-U($r0Ybe>O7|DJf`V9rt3tg>O9Vwm3SsR
zevCb#F;8gB6FL*2$~>ttPioAQIuoJFJf${Mq@ssF`vFwO=K`)$^IEADxgmc6rq#`0~J_lpbC@>R6w5?C_?l`8mPd=5!FOi>6pZnBSr>_
z5Iy2p7^uL;QLK?O$v_2EhN?~_b28vL+A{i(`sS^e$&14RiI>`0?KQW
zfg+STVW0xbnJ`cVN(L_XQ0jz%3M^;BKouw%sDN@N87M+|Jz<~%%b74x1xf}g
zpqxnticso=feI{V!ax-$8K{7ACK)I~sS^e$&14RiI>`0?L_Wpa`W-7^uKf}LawZulLa7r5DzKah
z16818paRO7WS|J8P8g`bawZH^fs%m=C})y^B9uB|paRR8Fi-_b1}dPONd}5g>V$y`
zEN8+%6(|{~fN~}oC_Az;Y%GRDqI#3Mglifg+ST
zVW0xbnJ`cVN(L_XQ0jz%3M^;BKouw%sDN@N87M-j69y`}VUmF=OcK^?eeK12m6?d_<
zj{pDTxsR-!ZMJ94?O8eZrPjLForCRm%Y}I>_t^}a<4Xy**ga~qviNRAA8lI;jE<0~
zTkh|!#_awW!I5bG}{N(Y6b*5YULY%UFlVwi&>a>HxeJ4!S7Ce9g-&<9;uZ#e
zD`2c%0eSC#5jUcHL`snx6Q^y=0A
zZkx1=wHT~I#oDdZA>|dr0>%4qDQNDga`FdQwkt{!Ri1H1ke1n&7
zB+54qDBp<7H6%l<>Y}^13d0xaZ+z{XZRzJ
zA9}9ibjioqD(LC(zA%wav`tMn@mv=5ba;uFNGIB+rki-q7WH&^vzSOH+NP$Pcy3%h
z9bPvk(uuaI=_a0oBYHZ#gG{6oZBx@t+}B6-ba*kDNGIB+rkl7=m-KXaTbW2F+Vpfz
z+Z4B9w`6y(g5bLpfY&`$@Xvls$wAsJ@o85ys!qRAYy?VG&^FA-|#xCEAc~m7dzp*X5f9Ho3R9MOD)Yc5IwH6p&
zw|&{b%6pl<>IO@DUfaj!AFQ~1S0QW1s5|*u7Yb`Tk)QE@g!d1R8fDVaH#%h+!
z)D5w%l64DSul~!_*cxrKPdrGy?lxzzZBUu(KYR7Xj4G4_(7J!J8O0*n2^l3%kc7xu
zzIL4Kd4LSlTdQ3uruHMY6@{6NuM#Qc~
zMi{Z-SF84KMxk+k3r%6Pl`P2xCmV55#!L5;t+*^(UytWTLu(*7yA3rxSa&+CJ
zt-I96A-g$5uO7TQet81M?+jeNEh`;O3=B?!cXNxj-D(-J??wqX*%n=LXxr*9PZu|l
z3;nsdIenPhbKa$(XCE-k)9;pv{209G`joMWtW>gwo+j-P81RGuqX`
zeQoa1-Hj)pUFj8amdUViL9fH^JT?}4ITFLRuitP_;^DzGFsPN!v-pXp2Z`<}r+q{`
zS$sc;Pa+p{)}Qa@SqgI$KKt}#G>pggW7{y%ZrIp?VeC7cer!L9^VmLO>_2>SkDsTv
z>HU3ro2E~SY1@7#cDDW~@OyFQ;p_LF$5vsSO|+3Z+7^RQ?L#r9pIt8l
zonp_G?>ts8-HEA;+LbvBlXS0Q<;1kf=djXDX~w`FWPkT!rqk?n`COPtfpHcQS7~x{+9&8L_IGnZxjZlj6~7BLKMu;Ti2zs3VDRu@*=N|@
z#KC!aaDfh>+zska!DnbbZ*|vGR%qFdm*GYFcYgX}n#vH8>{tMuj3mv1td
z*?NtR>-5#2ucq1GeQBlYqcdVlJPl7IO|f|$vyL>3kcG^^?RJe_!|?zpBr*FKs+w
zE#Rd_VVPF;ENvb4ch9eOddo6*2IB<>!{00g>sa}Q@j?27v}vB*;hE2Sm)cJ_S)iwL
z9;Y9tnR(XXoO9it_oO#D)FJ2PsUsFK!#v9j>drz?uf*e?Vi-zlsKyOxG!Wk
HVR!t0z{JvO
literal 0
HcmV?d1
diff --git a/tests/data/acpi/q35/TPM2.crb.tpm2
b/tests/data/acpi/q35/TPM2.crb.tpm2
index
e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..9d89c5becc0fc4558a0556daf46e152181f2eb51
100644
GIT binary patch
literal 76
zcmWFu@HO|?Wja`Jcf2v%^42yj*a0!E-1hz+7az=7e)KM>6hB2WNG#ec9c0JCKY
A0RR91
literal 0
HcmV?d1
diff --git a/tests/data/acpi/virt/DSDT.crb-device.tpm2
b/tests/data/acpi/virt/DSDT.crb-device.tpm2
index
e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..1b3a8ef4cb3ab093402f4bdb2a349b3d0d015ad5
100644
GIT binary patch
literal 5276
zcmZvg%WoT16o>D`lh__VVmr?JVHZlpGaB1Xla{u`9y^IkoEVSWAf=KkRYjC+Dp6G`
z6;jBeh3;r1RxE-P3H}TuR_xfZV9kbqfF0&{=guVOsAr^%=gi#m@$qf?%?
zrAB^f?0Q>%x$$Y`T^iQu8F~^Y@MZ
z8Dg384@p$$Wyp1!mgX@-7}qI7uG5Ufm?MlElp)t?R$?p=#!bqQ>vUXVED^>>
zlp)t?PGXb^W1TYOI?YRrHwdFn8FHN#B*sO;Xi$b+rxOz65@C3RQI+eoC^6n9j3#Bs
zbvh|A-X)9{Wyp0pB{AM7j19_=>vURTEEC2iWyp0pBQadU*rE)%PG==Xl`z_rA=l}g
z#JEftwWPqgrkH1nd8W80Lh6}jo@wTZmXCTOq@Ee(nPHw8?un3k
Re: [PATCH v3 13/14] tests: acpi: updated expected blobs for TPM CRB
On 10/30/23 18:42, Stefan Berger wrote:
On 10/29/23 02:03, Joelle van Dyne wrote:
Signed-off-by: Joelle van Dyne
I see this error here with the test cases:
| 364/377 ERROR:../tests/qtest/bios-tables-test.c:535:test_acpi_asl:
assertion failed: (all_tables_match) ERROR
364/377 qemu:qtest+qtest-x86_64 /
qtest-x86_64/bios-tables-test ERROR 34.83s killed
by signal 6 SIGABRT
>>> QTEST_QEMU_BINARY=./qemu-system-x86_64 MALLOC_PERTURB_=200
QTEST_QEMU_IMG=./qemu-img
G_TEST_DBUS_DAEMON=/home/stefanb/qemu-tpm/tests/dbus-vmstate-daemon.sh
QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon
PYTHON=/home/stefanb/qemu-tpm/build/pyvenv/bin/python3
/home/stefanb/qemu-tpm/build/tests/qtest/bios-tables-test --tap -k
--- 8<
---
$ diff tests/data/acpi/virt/TPM2.crb-device.dsl /tmp/aml-98C6D2.dsl
6c6
< * Disassembly of tests/data/acpi/virt/TPM2.crb-device.tpm2, Mon Oct
30 18:30:00 2023
---
> * Disassembly of /tmp/aml-98C6D2, Mon Oct 30 18:29:29 2023
16c16
< [009h 0009 1] Checksum : BA
---
> [009h 0009 1] Checksum : C2
30c30
< [044h 0068 8] Log Address : 43D1
---
> [044h 0068 8] Log Address : 43C9
The diff is in the address of the TPM log ... Not good. I don't know
how we could have it ignore the address or not build the TPM2 table
with an address for a log. It would be good to have test cases.
Stefan
The log that the TPM2 ACPI table points to is needed for a BIOS, UEFI
does not need it (but we don't typically know whether BIOS or UEFI will
run). So we could introduce a property no-acpi-log and have the test
cases set this to 'on' and get a NULL pointer for the 'Log Address'. You
could use the following in a patch:
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index acc654382e..2b2de34201 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2224,6 +2224,7 @@ void build_tpm2(GArray *table_data, BIOSLinker
*linker, GArray *tcpalog,
uint32_t start_method;
AcpiTable table = { .sig = "TPM2", .rev = 4,
.oem_id = oem_id, .oem_table_id = oem_table_id };
+ bool acpi_log = true;
acpi_table_begin(, table_data);
@@ -2238,6 +2239,7 @@ void build_tpm2(GArray *table_data, BIOSLinker
*linker, GArray *tcpalog,
control_area_start_address = TPM_CRB_ADDR_CTRL;
start_method = TPM2_START_METHOD_CRB;
} else if (TPM_IS_CRB_SYSBUS(tpmif)) {
+ acpi_log = !object_property_get_bool(OBJECT(tpmif),
"no-acpi-log", NULL);
baseaddr = object_property_get_uint(OBJECT(tpmif),
"x-baseaddr", NULL);
control_area_start_address = baseaddr + A_CRB_CTRL_REQ;
start_method = TPM2_START_METHOD_CRB;
@@ -2253,20 +2255,25 @@ void build_tpm2(GArray *table_data, BIOSLinker
*linker, GArray *tcpalog,
g_array_append_vals(table_data, _method_params,
ARRAY_SIZE(start_method_params));
- /* Log Area Minimum Length */
- build_append_int_noprefix(table_data, TPM_LOG_AREA_MINIMUM_SIZE, 4);
+ if (acpi_log) {
+ /* Log Area Minimum Length */
+ build_append_int_noprefix(table_data,
TPM_LOG_AREA_MINIMUM_SIZE, 4);
- acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
- bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE, tcpalog, 1,
- false);
+ acpi_data_push(tcpalog, TPM_LOG_AREA_MINIMUM_SIZE);
+ bios_linker_loader_alloc(linker, ACPI_BUILD_TPMLOG_FILE,
tcpalog, 1,
+ false);
- log_addr_offset = table_data->len;
+ log_addr_offset = table_data->len;
- /* Log Area Start Address to be filled by Guest linker */
- build_append_int_noprefix(table_data, 0, 8);
- bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
- log_addr_offset, 8,
- ACPI_BUILD_TPMLOG_FILE, 0);
+ /* Log Area Start Address to be filled by Guest linker */
+ build_append_int_noprefix(table_data, 0, 8);
+ bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
+ log_addr_offset, 8,
+ ACPI_BUILD_TPMLOG_FILE, 0);
+ } else {
+ build_append_int_noprefix(table_data, 0, 4);
+ build_append_int_noprefix(table_data, 0, 8);
+ }
acpi_table_end(linker, );
}
#endif
diff --git a/hw/tpm/tpm_crb_sysbus.c b/hw/tpm/tpm_crb_sysbus.c
index c10a8b5639..aeeaba512b 100644
--- a/hw/tpm/tpm_crb_sysbus.c
+++ b/hw/tpm/tpm_crb_sysbus.c
@@ -35,6 +35,7 @@ struct TPMCRBStateSysBus {
TPMCRBState state;
uint64_t baseaddr;
uint64_t size;
+ bool no_acpi_log;
};
OBJECT_DECLARE_SIMPLE_TYPE(TP
Re: [PATCH v3 08/14] hw/loongarch/virt: connect TPM to platform bus
On 10/29/23 02:03, Joelle van Dyne wrote:
Signed-off-by: Joelle van Dyne
Reviewed-by: Stefan Berger
---
hw/loongarch/virt.c | 7 +++
hw/loongarch/Kconfig | 1 +
2 files changed, 8 insertions(+)
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 4b7dc67a2d..feed0f8bbf 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -1004,6 +1004,13 @@ static void
loongarch_machine_device_plug_cb(HotplugHandler *hotplug_dev,
} else if (memhp_type_supported(dev)) {
virt_mem_plug(hotplug_dev, dev, errp);
}
+
+#ifdef CONFIG_TPM
+if (object_dynamic_cast(OBJECT(dev), TYPE_TPM_IF)) {
+tpm_sysbus_plug(TPM_IF(dev), OBJECT(lams->platform_bus_dev),
+VIRT_PLATFORM_BUS_BASEADDRESS);
+}
+#endif
}
static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
diff --git a/hw/loongarch/Kconfig b/hw/loongarch/Kconfig
index 5727efed6d..25da190ffc 100644
--- a/hw/loongarch/Kconfig
+++ b/hw/loongarch/Kconfig
@@ -5,6 +5,7 @@ config LOONGARCH_VIRT
imply VIRTIO_VGA
imply PCI_DEVICES
imply NVDIMM
+imply TPM_TIS_SYSBUS
select SERIAL
select VIRTIO_PCI
select PLATFORM_BUS
Re: [PATCH v3 07/14] hw/arm/virt: connect TPM to platform bus
On 10/29/23 02:03, Joelle van Dyne wrote:
Signed-off-by: Joelle van Dyne
Reviewed-by: Stefan Berger
---
hw/arm/virt.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 529f1c089c..f1a161b0ea 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2806,6 +2806,13 @@ static void virt_machine_device_plug_cb(HotplugHandler
*hotplug_dev,
vms->virtio_iommu_bdf = pci_get_bdf(pdev);
create_virtio_iommu_dt_bindings(vms);
}
+
+#ifdef CONFIG_TPM
+if (object_dynamic_cast(OBJECT(dev), TYPE_TPM_IF)) {
+tpm_sysbus_plug(TPM_IF(dev), OBJECT(vms->platform_bus_dev),
+vms->memmap[VIRT_PLATFORM_BUS].base);
+}
+#endif
}
static void virt_dimm_unplug_request(HotplugHandler *hotplug_dev,
Re: [PATCH v3 13/14] tests: acpi: updated expected blobs for TPM CRB
On 10/29/23 02:03, Joelle van Dyne wrote: Signed-off-by: Joelle van Dyne I see this error here with the test cases: | 364/377 ERROR:../tests/qtest/bios-tables-test.c:535:test_acpi_asl: assertion failed: (all_tables_match) ERROR 364/377 qemu:qtest+qtest-x86_64 / qtest-x86_64/bios-tables-test ERROR 34.83s killed by signal 6 SIGABRT >>> QTEST_QEMU_BINARY=./qemu-system-x86_64 MALLOC_PERTURB_=200 QTEST_QEMU_IMG=./qemu-img G_TEST_DBUS_DAEMON=/home/stefanb/qemu-tpm/tests/dbus-vmstate-daemon.sh QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon PYTHON=/home/stefanb/qemu-tpm/build/pyvenv/bin/python3 /home/stefanb/qemu-tpm/build/tests/qtest/bios-tables-test --tap -k --- 8< --- $ diff tests/data/acpi/virt/TPM2.crb-device.dsl /tmp/aml-98C6D2.dsl 6c6 < * Disassembly of tests/data/acpi/virt/TPM2.crb-device.tpm2, Mon Oct 30 18:30:00 2023 --- > * Disassembly of /tmp/aml-98C6D2, Mon Oct 30 18:29:29 2023 16c16 < [009h 0009 1] Checksum : BA --- > [009h 0009 1] Checksum : C2 30c30 < [044h 0068 8] Log Address : 43D1 --- > [044h 0068 8] Log Address : 43C9 The diff is in the address of the TPM log ... Not good. I don't know how we could have it ignore the address or not build the TPM2 table with an address for a log. It would be good to have test cases. Stefan
Re: [PATCH v3 12/14] tests: acpi: implement TPM CRB tests for ARM virt
On 10/29/23 02:03, Joelle van Dyne wrote:
Signed-off-by: Joelle van Dyne
Reviewed-by: Stefan Berger
---
tests/qtest/bios-tables-test.c | 43 --
1 file changed, 41 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 9f4bc15aab..c63bad0205 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -1345,6 +1345,28 @@ static void test_acpi_piix4_tcg_numamem(void)
uint64_t tpm_tis_base_addr;
+static test_data tcg_tpm_test_data(const char *machine)
+{
+if (g_strcmp0(machine, "virt") == 0) {
+test_data data = {
+.machine = "virt",
+.tcg_only = true,
+.uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
+.uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
+.cd =
+
"tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2",
+.ram_start = 0x4000ULL,
+.scan_len = 128ULL * 1024 * 1024,
+};
+return data;
+} else {
+test_data data = {
+.machine = machine,
+};
+return data;
+}
+}
+
static void test_acpi_tcg_tpm(const char *machine, const char *tpm_if,
uint64_t base, enum TPMVersion tpm_version)
{
@@ -1352,7 +1374,7 @@ static void test_acpi_tcg_tpm(const char *machine, const
char *tpm_if,
machine, tpm_if);
char *tmp_path = g_dir_make_tmp(tmp_dir_name, NULL);
TPMTestState test;
-test_data data = {};
+test_data data = tcg_tpm_test_data(machine);
GThread *thread;
const char *suffix = tpm_version == TPM_VERSION_2_0 ? "tpm2" : "tpm12";
char *args, *variant = g_strdup_printf(".%s.%s", tpm_if, suffix);
@@ -1372,13 +1394,14 @@ static void test_acpi_tcg_tpm(const char *machine,
const char *tpm_if,
thread = g_thread_new(NULL, tpm_emu_ctrl_thread, );
tpm_emu_test_wait_cond();
-data.machine = machine;
data.variant = variant;
args = g_strdup_printf(
+" %s"
" -chardev socket,id=chr,path=%s"
" -tpmdev emulator,id=dev,chardev=chr"
" -device tpm-%s,tpmdev=dev",
+g_strcmp0(machine, "virt") == 0 ? "-cpu cortex-a57" : "",
test.addr->u.q_unix.path, tpm_if);
test_acpi_one(args, );
@@ -1404,6 +1427,16 @@ static void test_acpi_q35_tcg_tpm12_tis(void)
test_acpi_tcg_tpm("q35", "tis", 0xFED4, TPM_VERSION_1_2);
}
+static void test_acpi_q35_tcg_tpm2_crb(void)
+{
+test_acpi_tcg_tpm("q35", "crb", 0xFED4, TPM_VERSION_2_0);
+}
+
+static void test_acpi_virt_tcg_tpm2_crb(void)
+{
+test_acpi_tcg_tpm("virt", "crb-device", 0xFED4, TPM_VERSION_2_0);
+}
+
static void test_acpi_tcg_dimm_pxm(const char *machine)
{
test_data data = {};
@@ -2110,6 +2143,9 @@ int main(int argc, char *argv[])
qtest_add_func("acpi/q35/tpm12-tis",
test_acpi_q35_tcg_tpm12_tis);
}
+if (tpm_model_is_available("-machine q35", "tpm-crb")) {
+qtest_add_func("acpi/q35/tpm2-crb",
test_acpi_q35_tcg_tpm2_crb);
+}
qtest_add_func("acpi/q35/bridge", test_acpi_q35_tcg_bridge);
qtest_add_func("acpi/q35/no-acpi-hotplug",
test_acpi_q35_tcg_no_acpi_hotplug);
@@ -2191,6 +2227,9 @@ int main(int argc, char *argv[])
qtest_add_func("acpi/virt/viot", test_acpi_virt_viot);
}
}
+if (tpm_model_is_available("-machine virt", "tpm-crb")) {
+qtest_add_func("acpi/virt/tpm2-crb", test_acpi_virt_tcg_tpm2_crb);
+}
}
ret = g_test_run();
boot_sector_cleanup(disk);
Re: [PATCH v3 10/14] tests: acpi: prepare for TPM CRB tests
On 10/29/23 02:03, Joelle van Dyne wrote: Signed-off-by: Joelle van Dyne Reviewed-by: Stefan Berger --- tests/qtest/bios-tables-test-allowed-diff.h | 4 tests/data/acpi/q35/DSDT.crb.tpm2 | 0 tests/data/acpi/q35/TPM2.crb.tpm2 | 0 tests/data/acpi/virt/DSDT.crb-device.tpm2 | 0 tests/data/acpi/virt/TPM2.crb-device.tpm2 | 0 5 files changed, 4 insertions(+) create mode 100644 tests/data/acpi/q35/DSDT.crb.tpm2 create mode 100644 tests/data/acpi/q35/TPM2.crb.tpm2 create mode 100644 tests/data/acpi/virt/DSDT.crb-device.tpm2 create mode 100644 tests/data/acpi/virt/TPM2.crb-device.tpm2 diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index dfb8523c8b..c2d1924c2f 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1 +1,5 @@ /* List of comma-separated changed AML files to ignore */ +"tests/data/acpi/q35/DSDT.crb.tpm2", +"tests/data/acpi/q35/TPM2.crb.tpm2", +"tests/data/acpi/virt/DSDT.crb.tpm2", +"tests/data/acpi/virt/TPM2.crb.tpm2", diff --git a/tests/data/acpi/q35/DSDT.crb.tpm2 b/tests/data/acpi/q35/DSDT.crb.tpm2 new file mode 100644 index 00..e69de29bb2 diff --git a/tests/data/acpi/q35/TPM2.crb.tpm2 b/tests/data/acpi/q35/TPM2.crb.tpm2 new file mode 100644 index 00..e69de29bb2 diff --git a/tests/data/acpi/virt/DSDT.crb-device.tpm2 b/tests/data/acpi/virt/DSDT.crb-device.tpm2 new file mode 100644 index 00..e69de29bb2 diff --git a/tests/data/acpi/virt/TPM2.crb-device.tpm2 b/tests/data/acpi/virt/TPM2.crb-device.tpm2 new file mode 100644 index 00..e69de29bb2
Re: [PATCH v3 11/14] tpm_crb_sysbus: introduce TPM CRB SysBus device
On 10/30/23 17:08, Stefan Berger wrote: On 10/29/23 02:03, Joelle van Dyne wrote: diff --git a/hw/tpm/tpm_crb_sysbus.c b/hw/tpm/tpm_crb_sysbus.c new file mode 100644 index 00..c10a8b5639 --- /dev/null +++ b/hw/tpm/tpm_crb_sysbus.c @@ -0,0 +1,161 @@ +/* + * tpm_crb_sysbus.c - QEMU's TPM CRB interface emulator + * + * Copyright (c) 2018 Red Hat, Inc. + * + * Authors: + * Marc-André Lureau You can add your name here...
Re: [PATCH v3 11/14] tpm_crb_sysbus: introduce TPM CRB SysBus device
On 10/29/23 02:03, Joelle van Dyne wrote:
This SysBus variant of the CRB interface supports dynamically locating
the MMIO interface so that Virt machines can use it. This interface
is currently the only one supported by QEMU that works on Windows 11
ARM64 as 'tpm-tis-device' does not work with current Windows drivers.
We largely follow that device as a template.
To try out this device with Windows 11 before OVMF is updated, you
will need to modify `sysbud-fdt.c` and change the added line from:
typo: sysbus-fdt.c
```c
TYPE_BINDING(TYPE_TPM_CRB_SYSBUS, no_fdt_node),
```
to
```c
TYPE_BINDING(TYPE_TPM_CRB_SYSBUS, add_tpm_tis_fdt_node),
Would the proper fix at some point be a call to a function that has
'crb' in its name (add_tpm_crb_fdt_node) rather than 'tis'?
```
This change was not included because it can confuse Linux (although
from testing, it seems like Linux is able to properly ignore the
device from the TPM TIS driver and recognize it from the ACPI device
in the TPM CRB driver). A proper fix would require OVMF to recognize
the ACPI device and not depend on the FDT node for recognizing TPM.
What does a fix in OVMF have to do with Linux possibly getting confused?
The command line to try out this device with SWTPM is:
```
$ qemu-system-aarch64 \
-chardev socket,id=chrtpm0,path=tpm.sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm0 \
-device tpm-crb-device,tpmdev=tpm0
```
along with SWTPM:
```
$ swtpm \
--ctrl type=unixio,path=tpm.sock,terminate \
--tpmstate backend-uri=file://tpm.data \
--tpm2
```
Signed-off-by: Joelle van Dyne
---
docs/specs/tpm.rst | 1 +
include/sysemu/tpm.h| 3 +
hw/acpi/aml-build.c | 7 +-
hw/arm/virt.c | 1 +
hw/core/sysbus-fdt.c| 1 +
hw/loongarch/virt.c | 1 +
hw/riscv/virt.c | 1 +
hw/tpm/tpm_crb_sysbus.c | 161
hw/arm/Kconfig | 1 +
hw/loongarch/Kconfig| 1 +
hw/riscv/Kconfig| 1 +
hw/tpm/Kconfig | 5 ++
hw/tpm/meson.build | 3 +
13 files changed, 186 insertions(+), 1 deletion(-)
create mode 100644 hw/tpm/tpm_crb_sysbus.c
diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst
index 2bc29c9804..95aeb49220 100644
--- a/docs/specs/tpm.rst
+++ b/docs/specs/tpm.rst
@@ -46,6 +46,7 @@ operating system.
QEMU files related to TPM CRB interface:
- ``hw/tpm/tpm_crb.c``
- ``hw/tpm/tpm_crb_common.c``
+ - ``hw/tpm/tpm_crb_sysbus.c``
SPAPR interface
---
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index ffd300e607..bab30fa546 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -49,6 +49,7 @@ struct TPMIfClass {
#define TYPE_TPM_TIS_ISA"tpm-tis"
#define TYPE_TPM_TIS_SYSBUS "tpm-tis-device"
#define TYPE_TPM_CRB"tpm-crb"
+#define TYPE_TPM_CRB_SYSBUS "tpm-crb-device"
#define TYPE_TPM_SPAPR "tpm-spapr"
#define TYPE_TPM_TIS_I2C"tpm-tis-i2c"
@@ -58,6 +59,8 @@ struct TPMIfClass {
object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
#define TPM_IS_CRB(chr) \
object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
+#define TPM_IS_CRB_SYSBUS(chr) \
+object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB_SYSBUS)
#define TPM_IS_SPAPR(chr) \
object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)
#define TPM_IS_TIS_I2C(chr) \
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index af66bde0f5..acc654382e 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -31,6 +31,7 @@
#include "hw/pci/pci_bus.h"
#include "hw/pci/pci_bridge.h"
#include "qemu/cutils.h"
+#include "qom/object.h"
static GArray *build_alloc_array(void)
{
@@ -2218,7 +2219,7 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker,
GArray *tcpalog,
{
uint8_t start_method_params[12] = {};
unsigned log_addr_offset;
-uint64_t control_area_start_address;
+uint64_t baseaddr, control_area_start_address;
TPMIf *tpmif = tpm_find();
uint32_t start_method;
AcpiTable table = { .sig = "TPM2", .rev = 4,
@@ -2236,6 +2237,10 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker,
GArray *tcpalog,
} else if (TPM_IS_CRB(tpmif)) {
control_area_start_address = TPM_CRB_ADDR_CTRL;
start_method = TPM2_START_METHOD_CRB;
+} else if (TPM_IS_CRB_SYSBUS(tpmif)) {
+baseaddr = object_property_get_uint(OBJECT(tpmif), "x-baseaddr", NULL);
+control_area_start_address = baseaddr + A_CRB_CTRL_REQ;
+start_method = TPM2_START_METHOD_CRB;
} else {
g_assert_not_reached();
}
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f1a161b0ea..22e147f71a 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2950,6 +2950,7 @@ static void virt_machine_class_init(ObjectClass *oc, void
*data)
Re: [PATCH v3 09/14] tpm_tis_sysbus: move DSDT AML generation to device
eState *machine)
@@ -379,7 +345,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
MachineState *machine)
build_la_ged_aml(dsdt, machine);
build_flash_aml(dsdt, lams);
#ifdef CONFIG_TPM
-acpi_dsdt_add_tpm(dsdt, lams);
+call_dev_aml_func(DEVICE(tpm_find()), dsdt);
#endif
/* System State Package */
scope = aml_scope("\\");
diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
index 2fc550f119..462b0e1571 100644
--- a/hw/tpm/tpm_tis_sysbus.c
+++ b/hw/tpm/tpm_tis_sysbus.c
@@ -30,6 +30,7 @@
#include "hw/sysbus.h"
#include "tpm_tis.h"
#include "qom/object.h"
+#include "hw/acpi/acpi_aml_interface.h"
struct TPMStateSysBus {
/*< private >*/
@@ -37,6 +38,8 @@ struct TPMStateSysBus {
/*< public >*/
TPMState state; /* not a QOM object */
+uint64_t baseaddr;
+uint64_t size;
};
OBJECT_DECLARE_SIMPLE_TYPE(TPMStateSysBus, TPM_TIS_SYSBUS)
@@ -93,6 +96,10 @@ static void tpm_tis_sysbus_reset(DeviceState *dev)
static Property tpm_tis_sysbus_properties[] = {
DEFINE_PROP_UINT32("irq", TPMStateSysBus, state.irq_num, TPM_TIS_IRQ),
DEFINE_PROP_TPMBE("tpmdev", TPMStateSysBus, state.be_driver),
+DEFINE_PROP_UINT64("x-baseaddr", TPMStateSysBus, baseaddr,
+ TPM_TIS_ADDR_BASE),
+DEFINE_PROP_UINT64("x-size", TPMStateSysBus, size,
+ TPM_TIS_ADDR_SIZE),
DEFINE_PROP_END_OF_LIST(),
};
@@ -125,10 +132,38 @@ static void tpm_tis_sysbus_realizefn(DeviceState *dev,
Error **errp)
}
}
+static void build_tpm_tis_sysbus_aml(AcpiDevAmlIf *adev, Aml *scope)
+{
+Aml *dev, *crs;
+TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(adev);
+TPMIf *ti = TPM_IF(sbdev);
+
+dev = aml_device("TPM");
+if (tpm_tis_sysbus_get_tpm_version(ti) == TPM_VERSION_2_0) {
+aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
+aml_append(dev, aml_name_decl("_STR", aml_string("TPM 2.0 Device")));
+} else {
+aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31")));
+}
+aml_append(dev, aml_name_decl("_UID", aml_int(0)));
+aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
+crs = aml_resource_template();
+aml_append(crs, aml_memory32_fixed(sbdev->baseaddr, sbdev->size,
+ AML_READ_WRITE));
+/*
+ * FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs,
+ * fix default TPM_TIS_IRQ value there to use some unused IRQ
+ */
+/* aml_append(crs, aml_irq_no_flags(sbdev->state.irq_num)); */
+aml_append(dev, aml_name_decl("_CRS", crs));
+aml_append(scope, dev);
+}
+
static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
TPMIfClass *tc = TPM_IF_CLASS(klass);
+AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass);
device_class_set_props(dc, tpm_tis_sysbus_properties);
dc->vmsd = _tpm_tis_sysbus;
@@ -139,6 +174,7 @@ static void tpm_tis_sysbus_class_init(ObjectClass *klass,
void *data)
tc->request_completed = tpm_tis_sysbus_request_completed;
tc->get_version = tpm_tis_sysbus_get_tpm_version;
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+adevc->build_dev_aml = build_tpm_tis_sysbus_aml;
}
static const TypeInfo tpm_tis_sysbus_info = {
@@ -149,6 +185,7 @@ static const TypeInfo tpm_tis_sysbus_info = {
.class_init = tpm_tis_sysbus_class_init,
.interfaces = (InterfaceInfo[]) {
{ TYPE_TPM_IF },
+{ TYPE_ACPI_DEV_AML_IF },
{ }
}
};
Reviewed-by: Stefan Berger
Re: [PATCH v3 06/14] tpm-sysbus: add plug handler for TPM on SysBus
On 10/30/23 12:55, Joelle van Dyne wrote: I was debating what to add. I couldn't find a project-wide template for what the header should be and I couldn't copy/paste from where I copied the code from (virt.c) because it names a specific author that I'm not sure wrote this code... Any advice? I would follow the files in hw/tpm/*.c and use those as templates with - name of file and short description - Copyright - Author(s) - 2 sentences about the license - Maybe a longer description. On Mon, Oct 30, 2023 at 9:52 AM Stefan Berger wrote: On 10/29/23 02:03, Joelle van Dyne wrote: TPM needs to know its own base address in order to generate its DSDT device entry. Signed-off-by: Joelle van Dyne --- include/sysemu/tpm.h | 4 hw/tpm/tpm-sysbus.c | 33 + hw/tpm/meson.build | 1 + 3 files changed, 38 insertions(+) create mode 100644 hw/tpm/tpm-sysbus.c diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h index 1ee568b3b6..ffd300e607 100644 --- a/include/sysemu/tpm.h +++ b/include/sysemu/tpm.h @@ -12,6 +12,8 @@ #ifndef QEMU_TPM_H #define QEMU_TPM_H +#include "qemu/osdep.h" +#include "exec/hwaddr.h" #include "qapi/qapi-types-tpm.h" #include "qom/object.h" @@ -78,6 +80,8 @@ static inline TPMVersion tpm_get_version(TPMIf *ti) return TPM_IF_GET_CLASS(ti)->get_version(ti); } +void tpm_sysbus_plug(TPMIf *tpmif, Object *pbus, hwaddr pbus_base); + #else /* CONFIG_TPM */ #define tpm_init() (0) diff --git a/hw/tpm/tpm-sysbus.c b/hw/tpm/tpm-sysbus.c new file mode 100644 index 00..ef0592b837 --- /dev/null +++ b/hw/tpm/tpm-sysbus.c @@ -0,0 +1,33 @@ A header in this new file would be good. Otherwise LGTM. Stefan +#include "sysemu/tpm.h" +#include "hw/platform-bus.h" +#include "hw/sysbus.h" +#include "qapi/error.h"
Re: [PATCH v3 06/14] tpm-sysbus: add plug handler for TPM on SysBus
On 10/29/23 02:03, Joelle van Dyne wrote: TPM needs to know its own base address in order to generate its DSDT device entry. Signed-off-by: Joelle van Dyne --- include/sysemu/tpm.h | 4 hw/tpm/tpm-sysbus.c | 33 + hw/tpm/meson.build | 1 + 3 files changed, 38 insertions(+) create mode 100644 hw/tpm/tpm-sysbus.c diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h index 1ee568b3b6..ffd300e607 100644 --- a/include/sysemu/tpm.h +++ b/include/sysemu/tpm.h @@ -12,6 +12,8 @@ #ifndef QEMU_TPM_H #define QEMU_TPM_H +#include "qemu/osdep.h" +#include "exec/hwaddr.h" #include "qapi/qapi-types-tpm.h" #include "qom/object.h" @@ -78,6 +80,8 @@ static inline TPMVersion tpm_get_version(TPMIf *ti) return TPM_IF_GET_CLASS(ti)->get_version(ti); } +void tpm_sysbus_plug(TPMIf *tpmif, Object *pbus, hwaddr pbus_base); + #else /* CONFIG_TPM */ #define tpm_init() (0) diff --git a/hw/tpm/tpm-sysbus.c b/hw/tpm/tpm-sysbus.c new file mode 100644 index 00..ef0592b837 --- /dev/null +++ b/hw/tpm/tpm-sysbus.c @@ -0,0 +1,33 @@ A header in this new file would be good. Otherwise LGTM. Stefan +#include "sysemu/tpm.h" +#include "hw/platform-bus.h" +#include "hw/sysbus.h" +#include "qapi/error.h"
Re: [PATCH v2 10/13] migration: Improve example and documentation of vmstate_register()
On 10/20/23 05:07, Juan Quintela wrote: Signed-off-by: Juan Quintela Reviewed-by: Stefan Berger --- docs/devel/migration.rst | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst index c3e1400c0c..bfd8710c95 100644 --- a/docs/devel/migration.rst +++ b/docs/devel/migration.rst @@ -165,13 +165,17 @@ An example (from hw/input/pckbd.c) } }; -We are declaring the state with name "pckbd". -The ``version_id`` is 3, and the fields are 4 uint8_t in a KBDState structure. -We registered this with: +We are declaring the state with name "pckbd". The ``version_id`` is +3, and there are 4 uint8_t fields in the KBDState structure. We +registered this ``VMSTATEDescription`` with one of the following +functions. The first one will generate a device ``instance_id`` +different for each registration. Use the second one if you already +have an id that is different for each instance of the device: .. code:: c -vmstate_register(NULL, 0, _kbd, s); +vmstate_register_any(NULL, _kbd, s); +vmstate_register(NULL, instance_id, _kbd, s); For devices that are ``qdev`` based, we can register the device in the class init function:
Re: [PATCH 10/13] migration: Improve example and documentation of vmstate_register()
On 10/19/23 15:08, Juan Quintela wrote: Signed-off-by: Juan Quintela --- docs/devel/migration.rst | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst index c3e1400c0c..a9fde75862 100644 --- a/docs/devel/migration.rst +++ b/docs/devel/migration.rst @@ -165,13 +165,17 @@ An example (from hw/input/pckbd.c) } }; -We are declaring the state with name "pckbd". -The ``version_id`` is 3, and the fields are 4 uint8_t in a KBDState structure. -We registered this with: +We are declaring the state with name "pckbd". The ``version_id`` is +3, and the fields are 4 uint8_t in a KBDState structure. We and there are 4 uint8_t fields in the KBDState structure. +registered this with one of those. The first one will generate a I am not sure what this means 'We registered this with one of those'. What is 'one of those'? Maybe you mean: We register the KBDState with one of the following functions. +device ``instance_id`` different for each registration. Use the +second one if you already have an id different for each instance of +the device: ... have an id that is is different for each ... .. code:: c -vmstate_register(NULL, 0, _kbd, s); +vmstate_register_any(NULL, _kbd, s); +vmstate_register(NULL, instance_id, _kbd, s); For devices that are ``qdev`` based, we can register the device in the class init function:
Re: [PATCH 13/13] migration: Use vmstate_register_any() for vmware_vga
On 10/19/23 15:08, Juan Quintela wrote: I have no idea if we can have more than one vmware_vga device, so play it safe. Signed-off-by: Juan Quintela Reviewed-by: Stefan Berger --- hw/display/vmware_vga.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c index 09591fbd39..7490d43881 100644 --- a/hw/display/vmware_vga.c +++ b/hw/display/vmware_vga.c @@ -1264,7 +1264,7 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s, vga_common_init(>vga, OBJECT(dev), _fatal); vga_init(>vga, OBJECT(dev), address_space, io, true); -vmstate_register(NULL, 0, _vga_common, >vga); +vmstate_register_any(NULL, _vga_common, >vga); And the first one registered with 'any' will again have instance_id = 0 assigned. So there's no side effect to be expected with any of these device, I suppose. s->new_depth = 32; }
Re: [PATCH 11/13] migration: Use vmstate_register_any() for audio
On 10/19/23 15:08, Juan Quintela wrote:
We can have more than one audio card.
void audio_init_audiodevs(void)
{
AudiodevListEntry *e;
QSIMPLEQ_FOREACH(e, , next) {
audio_init(e->dev, _fatal);
}
}
Signed-off-by: Juan Quintela
Reviewed-by: Stefan Berger
---
audio/audio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/audio/audio.c b/audio/audio.c
index e9815d6812..f91e05b72c 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1781,7 +1781,7 @@ static AudioState *audio_init(Audiodev *dev, Error **errp)
QTAILQ_INSERT_TAIL(_states, s, list);
QLIST_INIT (>card_head);
-vmstate_register (NULL, 0, _audio, s);
+vmstate_register_any(NULL, _audio, s);
return s;
out:
Re: [PATCH 12/13] migration: Use vmstate_register_any() for eeprom93xx
On 10/19/23 15:08, Juan Quintela wrote:
We can have more than one eeprom93xx.
For instance:
e100_nic_realize() -> eeprom93xx_new()
Signed-off-by: Juan Quintela
Reviewed-by: Stefan Berger
---
hw/nvram/eeprom93xx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/nvram/eeprom93xx.c b/hw/nvram/eeprom93xx.c
index 1081e2cc0d..57d63638d7 100644
--- a/hw/nvram/eeprom93xx.c
+++ b/hw/nvram/eeprom93xx.c
@@ -321,7 +321,7 @@ eeprom_t *eeprom93xx_new(DeviceState *dev, uint16_t nwords)
/* Output DO is tristate, read results in 1. */
eeprom->eedo = 1;
logout("eeprom = 0x%p, nwords = %u\n", eeprom, nwords);
-vmstate_register(VMSTATE_IF(dev), 0, _eeprom, eeprom);
+vmstate_register_any(VMSTATE_IF(dev), _eeprom, eeprom);
return eeprom;
}
Re: [PATCH 05/13] migration: Use VMSTATE_INSTANCE_ID_ANY for slirp
On 10/19/23 15:08, Juan Quintela wrote:
Each user network conection create a new slirp instance. We register
more than one slirp instance for number 0.
qemu-system-x86_64: -netdev user,id=hs1: savevm_state_handler_insert: Detected
duplicate SaveStateEntry: id=slirp, instance_id=0x0
Broken pipe
../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried
to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)
Signed-off-by: Juan Quintela
Reviewed-by: Stefan Berger
---
net/slirp.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/slirp.c b/net/slirp.c
index c33b3e02e7..25b49c4526 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -46,6 +46,7 @@
#include "qapi/qmp/qdict.h"
#include "util.h"
#include "migration/register.h"
+#include "migration/vmstate.h"
#include "migration/qemu-file-types.h"
static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
@@ -659,8 +660,8 @@ static int net_slirp_init(NetClientState *peer, const char
*model,
* specific version?
*/
g_assert(slirp_state_version() == 4);
-register_savevm_live("slirp", 0, slirp_state_version(),
- _slirp_state, s->slirp);
+register_savevm_live("slirp", VMSTATE_INSTANCE_ID_ANY,
+ slirp_state_version(), _slirp_state, s->slirp);
s->poll_notifier.notify = net_slirp_poll_notify;
main_loop_poll_add_notifier(>poll_notifier);
Re: [PATCH 06/13] migration: Use VMSTATE_INSTANCE_ID_ANY for s390 devices
On 10/19/23 15:08, Juan Quintela wrote:
Just with make check I can see that we can have more than one of this
devices, so use ANY.
ok 5 /s390x/device/introspect/abstract-interfaces
...
Broken pipe
../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried
to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)
Signed-off-by: Juan Quintela
Reviewed-by: Stefan Berger
---
hw/s390x/s390-skeys.c| 3 ++-
hw/s390x/s390-stattrib.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 5024faf411..ef089e1967 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -22,6 +22,7 @@
#include "sysemu/kvm.h"
#include "migration/qemu-file-types.h"
#include "migration/register.h"
+#include "migration/vmstate.h"
#define S390_SKEYS_BUFFER_SIZE (128 * KiB) /* Room for 128k storage keys */
#define S390_SKEYS_SAVE_FLAG_EOS 0x01
@@ -457,7 +458,7 @@ static inline void s390_skeys_set_migration_enabled(Object
*obj, bool value,
ss->migration_enabled = value;
if (ss->migration_enabled) {
-register_savevm_live(TYPE_S390_SKEYS, 0, 1,
+register_savevm_live(TYPE_S390_SKEYS, VMSTATE_INSTANCE_ID_ANY, 1,
_s390_storage_keys, ss);
} else {
unregister_savevm(VMSTATE_IF(ss), TYPE_S390_SKEYS, ss);
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index 220e845d12..055d382c3c 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -13,6 +13,7 @@
#include "qemu/units.h"
#include "migration/qemu-file.h"
#include "migration/register.h"
+#include "migration/vmstate.h"
#include "hw/s390x/storage-attributes.h"
#include "qemu/error-report.h"
#include "exec/ram_addr.h"
@@ -380,7 +381,7 @@ static void s390_stattrib_instance_init(Object *obj)
{
S390StAttribState *sas = S390_STATTRIB(obj);
-register_savevm_live(TYPE_S390_STATTRIB, 0, 0,
+register_savevm_live(TYPE_S390_STATTRIB, VMSTATE_INSTANCE_ID_ANY, 0,
_s390_stattrib_handlers, sas);
object_property_add_bool(obj, "migration-enabled",
Re: [PATCH 04/13] migration: Use vmstate_register_any() for ipmi-bt*
On 10/19/23 15:08, Juan Quintela wrote:
Otherwise device-introspection-test fails.
$ ./tests/qtest/device-introspect-test
...
Broken pipe
../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried
to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)
Signed-off-by: Juan Quintela
Reviewed-by: Stefan Berger
---
hw/ipmi/ipmi_bmc_extern.c | 2 +-
hw/ipmi/ipmi_bmc_sim.c| 2 +-
hw/ipmi/isa_ipmi_bt.c | 2 +-
hw/ipmi/isa_ipmi_kcs.c| 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
index e232d35ba2..324a2c8835 100644
--- a/hw/ipmi/ipmi_bmc_extern.c
+++ b/hw/ipmi/ipmi_bmc_extern.c
@@ -504,7 +504,7 @@ static void ipmi_bmc_extern_init(Object *obj)
IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe);
-vmstate_register(NULL, 0, _ipmi_bmc_extern, ibe);
+vmstate_register_any(NULL, _ipmi_bmc_extern, ibe);
}
static void ipmi_bmc_extern_finalize(Object *obj)
diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 905e091094..404db5d5bc 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -2188,7 +2188,7 @@ static void ipmi_sim_realize(DeviceState *dev, Error
**errp)
ibs->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ipmi_timeout, ibs);
-vmstate_register(NULL, 0, _ipmi_sim, ibs);
+vmstate_register_any(NULL, _ipmi_sim, ibs);
}
static Property ipmi_sim_properties[] = {
diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index a83e7243d6..afb76b548a 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -125,7 +125,7 @@ static void isa_ipmi_bt_init(Object *obj)
ipmi_bmc_find_and_link(obj, (Object **) >bt.bmc);
-vmstate_register(NULL, 0, _ISAIPMIBTDevice, iib);
+vmstate_register_any(NULL, _ISAIPMIBTDevice, iib);
}
static void *isa_ipmi_bt_get_backend_data(IPMIInterface *ii)
diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
index b2ed70b9da..5ab63b2fcf 100644
--- a/hw/ipmi/isa_ipmi_kcs.c
+++ b/hw/ipmi/isa_ipmi_kcs.c
@@ -132,7 +132,7 @@ static void isa_ipmi_kcs_init(Object *obj)
* IPMI device, so receive it, but transmit a different
* version.
*/
-vmstate_register(NULL, 0, _ISAIPMIKCSDevice, iik);
+vmstate_register_any(NULL, _ISAIPMIKCSDevice, iik);
}
static void *isa_ipmi_kcs_get_backend_data(IPMIInterface *ii)
Re: [PATCH 03/13] migration: Use vmstate_register_any() for isa-ide
Re: [PATCH 02/13] migration: Use vmstate_register_any()
On 10/19/23 15:08, Juan Quintela wrote:
This are the easiest cases, where we were already using
VMSTATE_INSTANCE_ID_ANY.
Signed-off-by: Juan Quintela
Reviewed-by: Stefan Berger
---
backends/dbus-vmstate.c | 3 +--
backends/tpm/tpm_emulator.c | 3 +--
hw/i2c/core.c | 2 +-
hw/input/adb.c | 2 +-
hw/input/ads7846.c | 2 +-
hw/input/stellaris_input.c | 3 +--
hw/net/eepro100.c | 3 +--
hw/pci/pci.c| 2 +-
hw/ppc/spapr_nvdimm.c | 3 +--
hw/timer/arm_timer.c| 2 +-
hw/virtio/virtio-mem.c | 4 ++--
11 files changed, 12 insertions(+), 17 deletions(-)
diff --git a/backends/dbus-vmstate.c b/backends/dbus-vmstate.c
index 57369ec0f2..a9d8cb0acd 100644
--- a/backends/dbus-vmstate.c
+++ b/backends/dbus-vmstate.c
@@ -426,8 +426,7 @@ dbus_vmstate_complete(UserCreatable *uc, Error **errp)
return;
}
-if (vmstate_register(VMSTATE_IF(self), VMSTATE_INSTANCE_ID_ANY,
- _vmstate, self) < 0) {
+if (vmstate_register_any(VMSTATE_IF(self), _vmstate, self) < 0) {
error_setg(errp, "Failed to register vmstate");
}
}
diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index 402a2d6312..8920b75251 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -978,8 +978,7 @@ static void tpm_emulator_inst_init(Object *obj)
qemu_add_vm_change_state_handler(tpm_emulator_vm_state_change,
tpm_emu);
-vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
- _tpm_emulator, obj);
+vmstate_register_any(NULL, _tpm_emulator, obj);
}
/*
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index bed594fe59..879a1d45cb 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -64,7 +64,7 @@ I2CBus *i2c_init_bus(DeviceState *parent, const char *name)
bus = I2C_BUS(qbus_new(TYPE_I2C_BUS, parent, name));
QLIST_INIT(>current_devs);
QSIMPLEQ_INIT(>pending_masters);
-vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, _i2c_bus, bus);
+vmstate_register_any(NULL, _i2c_bus, bus);
return bus;
}
diff --git a/hw/input/adb.c b/hw/input/adb.c
index 214ae6f42b..8aed0da2cd 100644
--- a/hw/input/adb.c
+++ b/hw/input/adb.c
@@ -247,7 +247,7 @@ static void adb_bus_realize(BusState *qbus, Error **errp)
adb_bus->autopoll_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, adb_autopoll,
adb_bus);
-vmstate_register(NULL, -1, _adb_bus, adb_bus);
+vmstate_register_any(NULL, _adb_bus, adb_bus);
}
static void adb_bus_unrealize(BusState *qbus)
diff --git a/hw/input/ads7846.c b/hw/input/ads7846.c
index dc0998ac79..91116c6bdb 100644
--- a/hw/input/ads7846.c
+++ b/hw/input/ads7846.c
@@ -158,7 +158,7 @@ static void ads7846_realize(SSIPeripheral *d, Error **errp)
ads7846_int_update(s);
-vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, _ads7846, s);
+vmstate_register_any(NULL, _ads7846, s);
}
static void ads7846_class_init(ObjectClass *klass, void *data)
diff --git a/hw/input/stellaris_input.c b/hw/input/stellaris_input.c
index e6ee5e11f1..a58721c8cd 100644
--- a/hw/input/stellaris_input.c
+++ b/hw/input/stellaris_input.c
@@ -88,6 +88,5 @@ void stellaris_gamepad_init(int n, qemu_irq *irq, const int
*keycode)
}
s->num_buttons = n;
qemu_add_kbd_event_handler(stellaris_gamepad_put_key, s);
-vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
- _stellaris_gamepad, s);
+vmstate_register_any(NULL, _stellaris_gamepad, s);
}
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index dc07984ae9..94ce9e18ff 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -1883,8 +1883,7 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error
**errp)
s->vmstate = g_memdup(_eepro100, sizeof(vmstate_eepro100));
s->vmstate->name = qemu_get_queue(s->nic)->model;
-vmstate_register(VMSTATE_IF(_dev->qdev), VMSTATE_INSTANCE_ID_ANY,
- s->vmstate, s);
+vmstate_register_any(VMSTATE_IF(_dev->qdev), s->vmstate, s);
}
static void eepro100_instance_init(Object *obj)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b0d21bf43a..294c3c38ea 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -147,7 +147,7 @@ static void pci_bus_realize(BusState *qbus, Error **errp)
bus->machine_done.notify = pcibus_machine_done;
qemu_add_machine_init_done_notifier(>machine_done);
-vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, _pcibus, bus);
+vmstate_register_any(NULL, _pcibus, bus);
}
static void pcie_bus_realize(BusState *qbus, Error **errp)
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index b2f009c816..ad7afe7544 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -876,8 +876,7 @@ static void spapr_nvdimm_realize(NVDIMMDevice *dimm, E
Re: [PATCH 01/13] migration: Create vmstate_register_any()
On 10/19/23 15:08, Juan Quintela wrote:
We have lots of cases where we are using an instance_id==0 when we
should be using VMSTATE_INSTANCE_ID_ANY (-1). Basically everything
that can have more than one needs to have a proper instance_id or -1
and the system will take one for it.
vmstate_register_any(): We register with -1.
Signed-off-by: Juan Quintela
Reviewed-by: Stefan Berger
---
include/migration/vmstate.h | 17 +
1 file changed, 17 insertions(+)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1a31fb7293..9ca7e9cc48 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1230,6 +1230,23 @@ static inline int vmstate_register(VMStateIf *obj, int
instance_id,
opaque, -1, 0, NULL);
}
+/**
+ * vmstate_register_any() - legacy function to register state
+ * serialisation description and let the function choose the id
+ *
+ * New code shouldn't be using this function as QOM-ified devices have
+ * dc->vmsd to store the serialisation description.
+ *
+ * Returns: 0 on success, -1 on failure
+ */
+static inline int vmstate_register_any(VMStateIf *obj,
+ const VMStateDescription *vmsd,
+ void *opaque)
+{
+return vmstate_register_with_alias_id(obj, VMSTATE_INSTANCE_ID_ANY, vmsd,
+ opaque, -1, 0, NULL);
+}
+
void vmstate_unregister(VMStateIf *obj, const VMStateDescription *vmsd,
void *opaque);
Re: [PATCH 15/21] qapi: Inline QERR_MISSING_PARAMETER definition (constant parameter)
On 10/4/23 13:31, Philippe Mathieu-Daudé wrote:
Address the comment added in commit 4629ed1e98
("qerror: Finally unused, clean up"), from 2015:
/*
* These macros will go away, please don't use
* in new code, and do not add new ones!
*/
Mechanical transformation using the following
coccinelle semantic patches:
@match@
expression errp;
constant param;
@@
error_setg(errp, QERR_MISSING_PARAMETER, param);
@script:python strformat depends on match@
param << match.param;
fixedfmt; // new var
@@
if param[0] == '"': # Format skipping '"',
fixedfmt = f'"Parameter \'{param[1:-1]}\' is missing"'
else: # or use definition.
fixedfmt = f'"Parameter " {param} " is missing"'
coccinelle.fixedfmt = cocci.make_ident(fixedfmt)
@replace@
expression match.errp;
constant match.param;
identifier strformat.fixedfmt;
@@
-error_setg(errp, QERR_MISSING_PARAMETER, param);
+error_setg(errp, fixedfmt);
and:
@match@
constant param;
@@
error_report(QERR_MISSING_PARAMETER, param);
@script:python strformat depends on match@
param << match.param;
fixedfmt; // new var
@@
fixedfmt = f'"Parameter \'{param[1:-1]}\' is missing"'
coccinelle.fixedfmt = cocci.make_ident(fixedfmt)
@replace@
constant match.param;
identifier strformat.fixedfmt;
@@
-error_report(QERR_MISSING_PARAMETER, param);
+error_report(fixedfmt);
Signed-off-by: Philippe Mathieu-Daud
Reviewed-by: Stefan Berger
---
backends/dbus-vmstate.c| 2 +-
block/gluster.c| 21 +++--
block/monitor/block-hmp-cmds.c | 6 +++---
dump/dump.c| 4 ++--
hw/usb/redirect.c | 2 +-
softmmu/qdev-monitor.c | 2 +-
softmmu/tpm.c | 4 ++--
softmmu/vl.c | 4 ++--
ui/input-barrier.c | 2 +-
ui/ui-qmp-cmds.c | 2 +-
10 files changed, 25 insertions(+), 24 deletions(-)
diff --git a/backends/dbus-vmstate.c b/backends/dbus-vmstate.c
index 57369ec0f2..e781ded17c 100644
--- a/backends/dbus-vmstate.c
+++ b/backends/dbus-vmstate.c
@@ -413,7 +413,7 @@ dbus_vmstate_complete(UserCreatable *uc, Error **errp)
}
if (!self->dbus_addr) {
-error_setg(errp, QERR_MISSING_PARAMETER, "addr");
+error_setg(errp, "Parameter 'addr' is missing");
return;
}
diff --git a/block/gluster.c b/block/gluster.c
index ad5fadbe79..8d97d698c3 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -530,20 +530,20 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster
*gconf,
num_servers = qdict_array_entries(options, GLUSTER_OPT_SERVER_PATTERN);
if (num_servers < 1) {
-error_setg(_err, QERR_MISSING_PARAMETER, "server");
+error_setg(_err, "Parameter 'server' is missing");
goto out;
}
ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME);
if (!ptr) {
-error_setg(_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_VOLUME);
+error_setg(_err, "Parameter " GLUSTER_OPT_VOLUME " is missing");
goto out;
}
gconf->volume = g_strdup(ptr);
ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH);
if (!ptr) {
-error_setg(_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_PATH);
+error_setg(_err, "Parameter " GLUSTER_OPT_PATH " is missing");
goto out;
}
gconf->path = g_strdup(ptr);
@@ -562,7 +562,8 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster
*gconf,
ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
if (!ptr) {
-error_setg(_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
+error_setg(_err,
+ "Parameter " GLUSTER_OPT_TYPE " is missing");
error_append_hint(_err, GERR_INDEX_HINT, i);
goto out;
@@ -592,16 +593,16 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster
*gconf,
ptr = qemu_opt_get(opts, GLUSTER_OPT_HOST);
if (!ptr) {
-error_setg(_err, QERR_MISSING_PARAMETER,
- GLUSTER_OPT_HOST);
+error_setg(_err,
+ "Parameter " GLUSTER_OPT_HOST " is missing");
error_append_hint(_err, GERR_INDEX_HINT, i);
goto out;
}
gsconf->u.inet.host = g_strdup(ptr);
ptr = qemu_opt_get(opts, GLUSTER_OPT_PORT);
if (!ptr) {
-error_setg(_err, QERR_MISSING_PARAMETER,
- GLUSTER_OPT_PORT);
+error_setg(_err,
+
Re: [PATCH 12/21] qapi: Inline and remove QERR_INVALID_PARAMETER_VALUE definition
On 10/4/23 13:31, Philippe Mathieu-Daudé wrote:
Address the comment added in commit 4629ed1e98
("qerror: Finally unused, clean up"), from 2015:
/*
* These macros will go away, please don't use
* in new code, and do not add new ones!
*/
Manually modify the error_report() call in softmmu/tpm.c,
then use sed to mechanically transform the rest. Finally
remove the definition in include/qapi/qmp/qerror.h.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Stefan Berger
---
include/qapi/qmp/qerror.h | 3 ---
qapi/opts-visitor.c | 4 ++--
qapi/qapi-visit-core.c| 4 ++--
softmmu/tpm.c | 3 +--
4 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index b723830eff..ac727d1c2d 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -17,9 +17,6 @@
* add new ones!
*/
-#define QERR_INVALID_PARAMETER_VALUE \
-"Parameter '%s' expects %s"
-
#define QERR_IO_ERROR \
"An IO error has occurred"
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 0393704a73..844db583f4 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -441,7 +441,7 @@ opts_type_int64(Visitor *v, const char *name, int64_t *obj,
Error **errp)
}
}
}
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
+error_setg(errp, "Parameter '%s' expects %s", opt->name,
(ov->list_mode == LM_NONE) ? "an int64 value" :
"an int64 value or range");
return false;
@@ -494,7 +494,7 @@ opts_type_uint64(Visitor *v, const char *name, uint64_t
*obj, Error **errp)
}
}
}
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
+error_setg(errp, "Parameter '%s' expects %s", opt->name,
(ov->list_mode == LM_NONE) ? "a uint64 value" :
"a uint64 value or range");
return false;
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 6c13510a2b..01793d6e74 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -194,7 +194,7 @@ static bool visit_type_uintN(Visitor *v, uint64_t *obj,
const char *name,
}
if (value > max) {
assert(v->type == VISITOR_INPUT);
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+error_setg(errp, "Parameter '%s' expects %s",
name ? name : "null", type);
return false;
}
@@ -262,7 +262,7 @@ static bool visit_type_intN(Visitor *v, int64_t *obj, const
char *name,
}
if (value < min || value > max) {
assert(v->type == VISITOR_INPUT);
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+error_setg(errp, "Parameter '%s' expects %s",
name ? name : "null", type);
return false;
}
diff --git a/softmmu/tpm.c b/softmmu/tpm.c
index 578563f05a..8437c4efc3 100644
--- a/softmmu/tpm.c
+++ b/softmmu/tpm.c
@@ -120,8 +120,7 @@ static int tpm_init_tpmdev(void *dummy, QemuOpts *opts,
Error **errp)
i = qapi_enum_parse(_lookup, value, -1, NULL);
be = i >= 0 ? tpm_be_find_by_type(i) : NULL;
if (be == NULL) {
-error_report(QERR_INVALID_PARAMETER_VALUE,
- "type", "a TPM backend type");
+error_report("Parameter 'type' expects a TPM backend type");
tpm_display_backend_drivers();
return 1;
}
Re: [PATCH v3 15/16] sysemu/tpm: Clean up global variable shadowing
On 10/4/23 08:00, Philippe Mathieu-Daudé wrote:
Fix:
softmmu/tpm.c:178:59: error: declaration shadows a variable in the global
scope [-Werror,-Wshadow]
int tpm_config_parse(QemuOptsList *opts_list, const char *optarg)
^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/getopt.h:77:14:
note: previous declaration is here
extern char *optarg;/* getopt(3) external variables */
^
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Stefan Berger
---
include/sysemu/tpm.h | 2 +-
softmmu/tpm.c| 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index 66e3b45f30..1ee568b3b6 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -17,7 +17,7 @@
#ifdef CONFIG_TPM
-int tpm_config_parse(QemuOptsList *opts_list, const char *optarg);
+int tpm_config_parse(QemuOptsList *opts_list, const char *optstr);
int tpm_init(void);
void tpm_cleanup(void);
diff --git a/softmmu/tpm.c b/softmmu/tpm.c
index 578563f05a..7164ea7ff1 100644
--- a/softmmu/tpm.c
+++ b/softmmu/tpm.c
@@ -175,15 +175,15 @@ int tpm_init(void)
* Parse the TPM configuration options.
* To display all available TPM backends the user may use '-tpmdev help'
*/
-int tpm_config_parse(QemuOptsList *opts_list, const char *optarg)
+int tpm_config_parse(QemuOptsList *opts_list, const char *optstr)
{
QemuOpts *opts;
-if (!strcmp(optarg, "help")) {
+if (!strcmp(optstr, "help")) {
tpm_display_backend_drivers();
return -1;
}
-opts = qemu_opts_parse_noisily(opts_list, optarg, true);
+opts = qemu_opts_parse_noisily(opts_list, optstr, true);
if (!opts) {
return -1;
}
Re: [PATCH v7 1/2] tpm: convert tpmdev options processing to new visitor format
On 9/27/23 12:49, James Bottomley wrote:
From: James Bottomley
Instead of processing the tpmdev options using the old qemu options,
convert to the new visitor format which also allows the passing of
json on the command line.
Signed-off-by: James Bottomley
---
v4: add TpmConfiOptions
v5: exit(0) for help
v7: adjust line lengths, free options
---
backends/tpm/tpm_emulator.c| 25 --
backends/tpm/tpm_passthrough.c | 23 +++--
include/sysemu/tpm.h | 4 +-
include/sysemu/tpm_backend.h | 2 +-
qapi/tpm.json | 19 +++
softmmu/tpm.c | 91 ++
softmmu/vl.c | 19 +--
7 files changed, 78 insertions(+), 105 deletions(-)
diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index 402a2d6312..833520e49a 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -583,33 +583,29 @@ err_exit:
return -1;
}
-static int tpm_emulator_handle_device_opts(TPMEmulator *tpm_emu, QemuOpts *opts)
+static int tpm_emulator_handle_device_opts(TPMEmulator *tpm_emu,
+ TpmCreateOptions *opts)
{
-const char *value;
Error *err = NULL;
Chardev *dev;
-value = qemu_opt_get(opts, "chardev");
-if (!value) {
-error_report("tpm-emulator: parameter 'chardev' is missing");
-goto err;
-}
+tpm_emu->options = QAPI_CLONE(TPMEmulatorOptions, >u.emulator);
+tpm_emu->data_ioc = NULL;
-dev = qemu_chr_find(value);
+dev = qemu_chr_find(opts->u.emulator.chardev);
if (!dev) {
-error_report("tpm-emulator: tpm chardev '%s' not found", value);
+error_report("tpm-emulator: tpm chardev '%s' not found",
+opts->u.emulator.chardev);
still, indentation.
--- a/qapi/tpm.json
+++ b/qapi/tpm.json
@@ -138,6 +138,25 @@
'emulator': 'TPMEmulatorOptionsWrapper' },
'if': 'CONFIG_TPM' }
+##
+# @TpmCreateOptions:
+#
+# A union referencing different TPM backend types' configuration options
+# without the wrapper to be usable by visitors.
+#
+# @type: - 'passthrough' The configuration options for the TPM passthrough type
+#- 'emulator' The configuration options for TPM emulator backend type
+#
+# Since: 7.2
+##'
Should probably be 8.2.
With the above fixed:
Tested-by: Stefan Berger
Reviewed-by: Stefan Berger
Re: [PATCH v6 1/2] tpm: convert tpmdev options processing to new visitor format
On 9/26/23 11:20, Stefan Berger wrote:
On 1/9/23 11:15, James Bottomley wrote:
From: James Bottomley
Instead of processing the tpmdev options using the old qemu options,
convert to the new visitor format which also allows the passing of
json on the command line.
Signed-off-by: James Bottomley
+ while (!QSIMPLEQ_EMPTY(_queue)) {
+ TpmCreateOptionsQueueEntry *tcoqe = QSIMPLEQ_FIRST(_queue);
- return 0;
+ QSIMPLEQ_REMOVE_HEAD(_queue, entry);
+ tpm_init_tpmdev(tcoqe->tco);
tcoqe->tco and the possible emulator or passthrough specific
allocations should probably be also free'ed here.
Use: qapi_free_TpmCreateOptions(tcoqe->tco);
Re: [PATCH v6 1/2] tpm: convert tpmdev options processing to new visitor format
On 1/9/23 11:15, James Bottomley wrote:
From: James Bottomley
Instead of processing the tpmdev options using the old qemu options,
convert to the new visitor format which also allows the passing of
json on the command line.
Signed-off-by: James Bottomley
$ ./scripts/checkpatch.pl
0001-tpm-convert-tpmdev-options-processing-to-new-visitor.patch
WARNING: line over 80 characters
#30: FILE: backends/tpm/tpm_emulator.c:586:
+static int tpm_emulator_handle_device_opts(TPMEmulator *tpm_emu,
TpmCreateOptions *opts)
WARNING: line over 80 characters
#101: FILE: backends/tpm/tpm_passthrough.c:255:
+tpm_passthrough_handle_device_opts(TPMPassthruState *tpm_pt,
TpmCreateOptions *opts)
total: 0 errors, 2 warnings, 340 lines checked
---
v4: add TpmConfiOptions
v5: exit(0) for help
---
backends/tpm/tpm_emulator.c| 24 -
backends/tpm/tpm_passthrough.c | 22 +++--
include/sysemu/tpm.h | 4 +-
include/sysemu/tpm_backend.h | 2 +-
qapi/tpm.json | 19 +++
softmmu/tpm.c | 90 ++
softmmu/vl.c | 19 +--
7 files changed, 75 insertions(+), 105 deletions(-)
diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index 49cc3d749d..cb6bf9d7c2 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -584,33 +584,28 @@ err_exit:
return -1;
}
-static int tpm_emulator_handle_device_opts(TPMEmulator *tpm_emu, QemuOpts *opts)
+static int tpm_emulator_handle_device_opts(TPMEmulator *tpm_emu,
TpmCreateOptions *opts)
{
-const char *value;
Error *err = NULL;
Chardev *dev;
-value = qemu_opt_get(opts, "chardev");
-if (!value) {
-error_report("tpm-emulator: parameter 'chardev' is missing");
-goto err;
-}
+tpm_emu->options = QAPI_CLONE(TPMEmulatorOptions, >u.emulator);
+tpm_emu->data_ioc = NULL;
-dev = qemu_chr_find(value);
+dev = qemu_chr_find(opts->u.emulator.chardev);
if (!dev) {
-error_report("tpm-emulator: tpm chardev '%s' not found", value);
+error_report("tpm-emulator: tpm chardev '%s' not found",
+opts->u.emulator.chardev);
indentation
goto err;
}
if (!qemu_chr_fe_init(_emu->ctrl_chr, dev, )) {
error_prepend(, "tpm-emulator: No valid chardev found at '%s':",
- value);
+ opts->u.emulator.chardev);
error_report_err(err);
goto err;
}
-tpm_emu->options->chardev = g_strdup(value);
-
if (tpm_emulator_prepare_data_fd(tpm_emu) < 0) {
goto err;
}
@@ -649,7 +644,7 @@ err:
return -1;
}
-static TPMBackend *tpm_emulator_create(QemuOpts *opts)
+static TPMBackend *tpm_emulator_create(TpmCreateOptions *opts)
{
TPMBackend *tb = TPM_BACKEND(object_new(TYPE_TPM_EMULATOR));
@@ -972,7 +967,6 @@ static void tpm_emulator_inst_init(Object *obj)
trace_tpm_emulator_inst_init();
-tpm_emu->options = g_new0(TPMEmulatorOptions, 1);
tpm_emu->cur_locty_number = ~0;
qemu_mutex_init(_emu->mutex);
tpm_emu->vmstate =
@@ -990,7 +984,7 @@ static void tpm_emulator_shutdown(TPMEmulator *tpm_emu)
{
ptm_res res;
-if (!tpm_emu->options->chardev) {
+if (!tpm_emu->data_ioc) {
/* was never properly initialized */
return;
}
diff --git a/backends/tpm/tpm_passthrough.c b/backends/tpm/tpm_passthrough.c
index 179697a3a9..3da467ef01 100644
--- a/backends/tpm/tpm_passthrough.c
+++ b/backends/tpm/tpm_passthrough.c
@@ -252,21 +252,12 @@ static int
tpm_passthrough_open_sysfs_cancel(TPMPassthruState *tpm_pt)
}
static int
-tpm_passthrough_handle_device_opts(TPMPassthruState *tpm_pt, QemuOpts *opts)
+tpm_passthrough_handle_device_opts(TPMPassthruState *tpm_pt, TpmCreateOptions
*opts)
{
-const char *value;
+tpm_pt->options = QAPI_CLONE(TPMPassthroughOptions, >u.passthrough);
-value = qemu_opt_get(opts, "cancel-path");
-if (value) {
-tpm_pt->options->cancel_path = g_strdup(value);
-}
-
-value = qemu_opt_get(opts, "path");
-if (value) {
-tpm_pt->options->path = g_strdup(value);
-}
-
-tpm_pt->tpm_dev = value ? value : TPM_PASSTHROUGH_DEFAULT_DEVICE;
+tpm_pt->tpm_dev = opts->u.passthrough.path ? opts->u.passthrough.path :
+TPM_PASSTHROUGH_DEFAULT_DEVICE;
tpm_pt->tpm_fd = qemu_open_old(tpm_pt->tpm_dev, O_RDWR);
if (tpm_pt->tpm_fd < 0) {
error_report("Cannot access TPM device using '%s': %s",
@@ -288,11 +279,11 @@ tpm_passthrough_handle_device_opts(TPMPassthruState
*tpm_pt, QemuOpts *opts)
return 0;
}
-static TPMBackend *tpm_passthrough_create(QemuOpts *opts)
+static TPMBackend *tpm_passthrough_create(TpmCreateOptions *tco)
{
Object *obj = object_new(TYPE_TPM_PASSTHROUGH);
-if
Re: [PATCH v6 2/2] tpm: add backend for mssim
On 9/22/23 09:02, Daniel P. Berrangé wrote: On Fri, Sep 22, 2023 at 08:41:19AM -0400, Stefan Berger wrote: On 9/22/23 02:00, Markus Armbruster wrote: Found this cleaning out old mail, sorry for missing it until now! I think we owe James a quick decision wether we're willing to take the feature. Stefan, thoughts? I thought we discusses it back then. Does it handle snapshotting and migration correctly? To quote the patch itself: +The mssim backend supports snapshotting and migration, but the state +of the Microsoft Simulator server must be preserved (or the server +kept running) outside of QEMU for restore to be successful. How does 'it' support snapshotting where the state of the TPM can be completely different depending on the snapshot? I know what it took to support this feature with swtpm/libtpms but I don't see the equivalent here in this backend driver nor in the TCG reference code that the underlying TPM 2 simulator is based upon. I do not want to stand in the way of it being merged but please understand that I will also neither maintain nor fix bugs related to it nor its related underlying simulator -- with James being the maintainer of it, this should be clear. I have reason why I am saying this and they come from dealing with the upstream TPM 2 reference code. Thanks, Stefan With regards, Daniel
Re: [PATCH v6 2/2] tpm: add backend for mssim
On 9/22/23 02:00, Markus Armbruster wrote:
Found this cleaning out old mail, sorry for missing it until now!
I think we owe James a quick decision wether we're willing to take the
feature. Stefan, thoughts?
I thought we discusses it back then. Does it handle snapshotting and
migration correctly?
Stefan
James Bottomley writes:
From: James Bottomley
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
[...]
diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst
index 535912a92b..1398735956 100644
--- a/docs/specs/tpm.rst
+++ b/docs/specs/tpm.rst
@@ -270,6 +270,38 @@ available as a module (assuming a TPM 2 is passed through):
/sys/devices/LNXSYSTEM:00/LNXSYBUS:00/MSFT0101:00/tpm/tpm0/pcr-sha256/9
...
+The QEMU TPM Microsoft Simulator Device
+---
+
+The TCG provides a reference implementation for TPM 2.0 written by
Suggest to copy the cover letter's nice introductory paragraph here:
The Microsoft Simulator (mssim) is the reference emulation platform
for the TCG TPM 2.0 specification.
It provides a reference implementation for TPM 2.0 written by
+Microsoft (See `ms-tpm-20-ref`_ on github). The reference implementation
+starts a network server and listens for TPM commands on port 2321 and
+TPM Platform control commands on port 2322, although these can be
+altered. The QEMU mssim TPM backend talks to this implementation. By
+default it connects to the default ports on localhost:
+
+.. code-block:: console
+
+ qemu-system-x86_64 \
+-tpmdev mssim,id=tpm0 \
+-device tpm-crb,tpmdev=tpm0
+
+
+Although it can also communicate with a remote host, which must be
+specified as a SocketAddress via json on the command line for each of
Is the "via JSON" part in "must be specified ... on the command line"
correct? I'd expect to be able to use dotted keys as well, like
-tpmdev
type=mssim,id=tpm0,command.type=inet,command.host=remote,command.port=2321',control.type=inet,control.host=remote,control.port=2322
Aside: I do recommend management applications stick to JSON.
+the command and control ports:
+
+.. code-block:: console
+
+ qemu-system-x86_64 \
+-tpmdev
"{'type':'mssim','id':'tpm0','command':{'type':'inet','host':'remote','port':'2321'},'control':{'type':'inet','host':'remote','port':'2322'}}"
\
+-device tpm-crb,tpmdev=tpm0
+
+
+The mssim backend supports snapshotting and migration, but the state
+of the Microsoft Simulator server must be preserved (or the server
+kept running) outside of QEMU for restore to be successful.
+
The QEMU TPM emulator device
@@ -526,3 +558,6 @@ the following:
.. _SWTPM protocol:
https://github.com/stefanberger/swtpm/blob/master/man/man3/swtpm_ioctls.pod
+
+.. _ms-tpm-20-ref:
+ https://github.com/microsoft/ms-tpm-20-ref
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index ed78a87ddd..12482368d0 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -731,6 +731,7 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
unsigned int c = 0;
TPMPassthroughOptions *tpo;
TPMEmulatorOptions *teo;
+TPMmssimOptions *tmo;
info_list = qmp_query_tpm();
if (err) {
@@ -764,6 +765,14 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
teo = ti->options->u.emulator.data;
monitor_printf(mon,
[PULL v3 0/1] Merge tpm 2023/09/12 v3
Hello! This PR contains a fix for the case where the TPM file descriptor is >= 1024 and the select() call cannot be used. It also avoids unnecessary errors due to EINTR being returned from the syscall. Regards, Stefan The following changes since commit 9ef497755afc252fb8e060c9ea6b0987abfd20b6: Merge tag 'pull-vfio-20230911' of https://github.com/legoater/qemu into staging (2023-09-11 09:13:08 -0400) are available in the Git repository at: https://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2023-09-12-3 for you to fetch changes up to 8e32ddff69b6b4547cc00592ad816484e160817a: tpm: fix crash when FD >= 1024 and unnecessary errors due to EINTR (2023-09-13 08:42:57 -0400) Marc-André Lureau (1): tpm: fix crash when FD >= 1024 and unnecessary errors due to EINTR backends/tpm/tpm_util.c | 11 ++- 1 file changed, 2 insertions(+), 9 deletions(-) -- 2.41.0
[PULL v3 1/1] tpm: fix crash when FD >= 1024 and unnecessary errors due to EINTR
From: Marc-André Lureau
Replace select() with poll() to fix a crash when QEMU has a large number
of FDs. Also use RETRY_ON_EINTR to avoid unnecessary errors due to EINTR.
Cc: qemu-sta...@nongnu.org
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2020133
Fixes: 56a3c24ffc ("tpm: Probe for connected TPM 1.2 or TPM 2")
Signed-off-by: Marc-André Lureau
Reviewed-by: Michael Tokarev
Reviewed-by: Stefan Berger
Signed-off-by: Stefan Berger
---
backends/tpm/tpm_util.c | 11 ++-
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
index a6e6d3e72f..1856589c3b 100644
--- a/backends/tpm/tpm_util.c
+++ b/backends/tpm/tpm_util.c
@@ -112,12 +112,8 @@ static int tpm_util_request(int fd,
void *response,
size_t responselen)
{
-fd_set readfds;
+GPollFD fds[1] = { {.fd = fd, .events = G_IO_IN } };
int n;
-struct timeval tv = {
-.tv_sec = 1,
-.tv_usec = 0,
-};
n = write(fd, request, requestlen);
if (n < 0) {
@@ -127,11 +123,8 @@ static int tpm_util_request(int fd,
return -EFAULT;
}
-FD_ZERO();
-FD_SET(fd, );
-
/* wait for a second */
-n = select(fd + 1, , NULL, NULL, );
+n = RETRY_ON_EINTR(g_poll(fds, 1, 1000));
if (n != 1) {
return -errno;
}
--
2.41.0
Re: [PULL v2 0/1] Merge tpm 2023/09/12 v2
On 9/13/23 08:27, Philippe Mathieu-Daudé wrote: Hi Stefan, On 13/9/23 13:54, Stefan Berger wrote: Hello! This PR contains a fix for the case where the TPM file descriptor is >= 1024 and the select() call cannot be used. It also avoids unnecessary errors due to EINTR being returned from the syscall. Regards, Stefan The following changes since commit 9ef497755afc252fb8e060c9ea6b0987abfd20b6: Merge tag 'pull-vfio-20230911' of https://github.com/legoater/qemu into staging (2023-09-11 09:13:08 -0400) are available in the Git repository at: https://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2023-09-12-2 for you to fetch changes up to 07160c57e47ce38bd256af3eae0481543fb52626: tpm: fix crash when FD >= 1024 and unnecessary errors due to EINTR (2023-09-13 07:46:59 -0400) Marc-Andr Lureau (1): tpm: fix crash when FD >= 1024 and unnecessary errors due to EINTR I recommend you the b4 tool, see: https://pypi.org/project/b4/ https://people.kernel.org/monsieuricon/introducing-b4-and-patch-attestation I had tried to use 'patches' but it doesn't seem to work anymore on the python level. So I ended up saving the email via Thunderbird and applied it. Maybe that's the mistake? Otherwise I know the b4 tool and typically am using it for Linux patches but I see now that there's lore.kernel.org for qemu-devel as well. Alright, v3 coming up soon. Stefan
[PULL v2 1/1] tpm: fix crash when FD >= 1024 and unnecessary errors due to EINTR
From: Marc-Andr Lureau
Replace select() with poll() to fix a crash when QEMU has a large number
of FDs. Also use RETRY_ON_EINTR to avoid unnecessary errors due to EINTR.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2020133
Cc: qemu-sta...@nongnu.org
Fixes: 56a3c24ffc ("tpm: Probe for connected TPM 1.2 or TPM 2")
Signed-off-by: Marc-Andr Lureau
Reviewed-by: Michael Tokarev
Reviewed-by: Stefan Berger
Signed-off-by: Stefan Berger
---
backends/tpm/tpm_util.c | 11 ++-
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
index a6e6d3e72f..1856589c3b 100644
--- a/backends/tpm/tpm_util.c
+++ b/backends/tpm/tpm_util.c
@@ -112,12 +112,8 @@ static int tpm_util_request(int fd,
void *response,
size_t responselen)
{
-fd_set readfds;
+GPollFD fds[1] = { {.fd = fd, .events = G_IO_IN } };
int n;
-struct timeval tv = {
-.tv_sec = 1,
-.tv_usec = 0,
-};
n = write(fd, request, requestlen);
if (n < 0) {
@@ -127,11 +123,8 @@ static int tpm_util_request(int fd,
return -EFAULT;
}
-FD_ZERO();
-FD_SET(fd, );
-
/* wait for a second */
-n = select(fd + 1, , NULL, NULL, );
+n = RETRY_ON_EINTR(g_poll(fds, 1, 1000));
if (n != 1) {
return -errno;
}
--
2.41.0
[PULL v2 0/1] Merge tpm 2023/09/12 v2
Hello! This PR contains a fix for the case where the TPM file descriptor is >= 1024 and the select() call cannot be used. It also avoids unnecessary errors due to EINTR being returned from the syscall. Regards, Stefan The following changes since commit 9ef497755afc252fb8e060c9ea6b0987abfd20b6: Merge tag 'pull-vfio-20230911' of https://github.com/legoater/qemu into staging (2023-09-11 09:13:08 -0400) are available in the Git repository at: https://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2023-09-12-2 for you to fetch changes up to 07160c57e47ce38bd256af3eae0481543fb52626: tpm: fix crash when FD >= 1024 and unnecessary errors due to EINTR (2023-09-13 07:46:59 -0400) Marc-Andr Lureau (1): tpm: fix crash when FD >= 1024 and unnecessary errors due to EINTR backends/tpm/tpm_util.c | 11 ++- 1 file changed, 2 insertions(+), 9 deletions(-) -- 2.41.0
[PULL v1 1/1] tpm: fix crash when FD >= 1024
From: Marc-Andr Lureau
Replace select() with poll() to fix a crash when QEMU has a large number
of FDs.
Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=2020133
Cc: qemu-sta...@nongnu.org
Fixes: 56a3c24ffc ("tpm: Probe for connected TPM 1.2 or TPM 2")
Signed-off-by: Marc-Andr Lureau
Reviewed-by: Michael Tokarev
Reviewed-by: Stefan Berger
Signed-off-by: Stefan Berger
---
backends/tpm/tpm_util.c | 11 ++-
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
index a6e6d3e72f..1856589c3b 100644
--- a/backends/tpm/tpm_util.c
+++ b/backends/tpm/tpm_util.c
@@ -112,12 +112,8 @@ static int tpm_util_request(int fd,
void *response,
size_t responselen)
{
-fd_set readfds;
+GPollFD fds[1] = { {.fd = fd, .events = G_IO_IN } };
int n;
-struct timeval tv = {
-.tv_sec = 1,
-.tv_usec = 0,
-};
n = write(fd, request, requestlen);
if (n < 0) {
@@ -127,11 +123,8 @@ static int tpm_util_request(int fd,
return -EFAULT;
}
-FD_ZERO();
-FD_SET(fd, );
-
/* wait for a second */
-n = select(fd + 1, , NULL, NULL, );
+n = RETRY_ON_EINTR(g_poll(fds, 1, 1000));
if (n != 1) {
return -errno;
}
--
2.41.0
[PULL v1 0/1] Merge tpm 2023/09/12 v1
Hello! This PR contains a fix for the case where the TPM file descriptor is >= 1024 and the select() call cannot be used. Regards, Stefan The following changes since commit 9ef497755afc252fb8e060c9ea6b0987abfd20b6: Merge tag 'pull-vfio-20230911' of https://github.com/legoater/qemu into staging (2023-09-11 09:13:08 -0400) are available in the Git repository at: https://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2023-09-12-1 for you to fetch changes up to 8557de964dfaae5c6eea09d488f85f4aa6cb3ce7: tpm: fix crash when FD >= 1024 (2023-09-12 17:30:12 -0400) Marc-Andr Lureau (1): tpm: fix crash when FD >= 1024 backends/tpm/tpm_util.c | 11 ++- 1 file changed, 2 insertions(+), 9 deletions(-) -- 2.41.0
Re: [PATCH v2] tpm: fix crash when FD >= 1024
On 9/11/23 09:25, marcandre.lur...@redhat.com wrote:
From: Marc-Andr Lureau
Replace select() with poll() to fix a crash when QEMU has a large number
of FDs.
Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=2020133
For backporting I think we should also add this tag here:
Fixes: ca64b08638 ("tpm: Move backend code under the 'backends/'
directory")
Though RETRY_ON_EINTR was only introduced in 8.0.0-rc0. What's the right
tag for backporting then?
Stefan
Signed-off-by: Marc-Andr Lureau
Reviewed-by: Michael Tokarev
Reviewed-by: Stefan Berger
---
backends/tpm/tpm_util.c | 11 ++-
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
index a6e6d3e72f..1856589c3b 100644
--- a/backends/tpm/tpm_util.c
+++ b/backends/tpm/tpm_util.c
@@ -112,12 +112,8 @@ static int tpm_util_request(int fd,
void *response,
size_t responselen)
{
-fd_set readfds;
+GPollFD fds[1] = { {.fd = fd, .events = G_IO_IN } };
int n;
-struct timeval tv = {
-.tv_sec = 1,
-.tv_usec = 0,
-};
n = write(fd, request, requestlen);
if (n < 0) {
@@ -127,11 +123,8 @@ static int tpm_util_request(int fd,
return -EFAULT;
}
-FD_ZERO();
-FD_SET(fd, );
-
/* wait for a second */
-n = select(fd + 1, , NULL, NULL, );
+n = RETRY_ON_EINTR(g_poll(fds, 1, 1000));
if (n != 1) {
return -errno;
}
Re: [PATCH 6/7] hw/tpm: spelling fixes
On 9/11/23 05:54, Michael Tokarev wrote: Stefan, can you take a quick look please? https://patchew.org/QEMU/20230909131258.354675-1-...@tls.msk.ru/20230909131258.354675-7-...@tls.msk.ru/ I lost subscription to the mailing list for some reason and missed the patch, looks good, though. Thanks. Reviewed-by: Stefan Berger Most of this is s/Familiy/Family/ in a few places, I guess it's some copy-paste error. 09.09.2023 16:12, Michael Tokarev: Signed-off-by: Michael Tokarev --- hw/tpm/tpm_tis.h | 2 +- hw/tpm/tpm_tis_common.c | 2 +- hw/tpm/tpm_tis_i2c.c | 4 ++-- hw/tpm/tpm_tis_isa.c | 2 +- hw/tpm/tpm_tis_sysbus.c | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) Thanks, /mjt
Re: [PATCH] tpm: fix crash when FD >= 1024
On 9/11/23 07:36, marcandre.lur...@redhat.com wrote:
From: Marc-Andr Lureau
Replace select() with poll() to fix a crash when QEMU has a large number
of FDs.
Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=2020133
The description there seems wrong. It's a limit of the POSIX API not the
vTPM device driver.
Signed-off-by: Marc-Andr Lureau
Reviewed-by: Stefan Berger
---
backends/tpm/tpm_util.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
index a6e6d3e72f..5f4c9f5b6f 100644
--- a/backends/tpm/tpm_util.c
+++ b/backends/tpm/tpm_util.c
@@ -112,12 +112,9 @@ static int tpm_util_request(int fd,
void *response,
size_t responselen)
{
-fd_set readfds;
+GPollFD fds[1] = { {.fd = fd, .events = G_IO_IN } };
int n;
-struct timeval tv = {
-.tv_sec = 1,
-.tv_usec = 0,
-};
+int timeout = 1000;
n = write(fd, request, requestlen);
if (n < 0) {
@@ -127,11 +124,8 @@ static int tpm_util_request(int fd,
return -EFAULT;
}
-FD_ZERO();
-FD_SET(fd, );
-
/* wait for a second */
-n = select(fd + 1, , NULL, NULL, );
+n = RETRY_ON_EINTR(g_poll(fds, 1, timeout));
if (n != 1) {
return -errno;
}
Re: [PATCH v2 08/11] hw/loongarch/virt: add plug handler for TPM on SysBus
On 7/20/23 13:57, Stefan Berger wrote:
On 7/14/23 03:09, Joelle van Dyne wrote:
TPM needs to know its own base address in order to generate its DSDT
device entry.
Signed-off-by: Joelle van Dyne
It would be great to also cover the crb-device with tests:
from tests/qtest/meson.build:
(config_all.has_key('CONFIG_TCG') and
config_all_devices.has_key('CONFIG_TPM_TIS_SYSBUS') ? \
['tpm-tis-device-test', 'tpm-tis-device-swtpm-test'] : []) +
\
It should be easy to make a copy of these two tis-device tests and rename them
to crb-device tests, adapt them, and run them at least on aarch64.
... actually make a copy of the files tests/qtest/tpm-crb-swtpm-test.c &
tests/qtest/tpm-crb-test.c.
Regards,
Stefan
Re: [PATCH v2 06/11] tpm_crb: move ACPI table building to device interface
On 7/31/23 23:02, Joelle van Dyne wrote:
On Mon, Jul 17, 2023 at 6:42 AM Igor Mammedov wrote:
On Fri, 14 Jul 2023 13:21:33 -0400
Stefan Berger wrote:
On 7/14/23 03:09, Joelle van Dyne wrote:
This logic is similar to TPM TIS ISA device. Since TPM CRB can only
support TPM 2.0 backends, we check for this in realize.
Signed-off-by: Joelle van Dyne
This patch changes the order of in which the ACPI table elements are created
but doesn't matter and also doesn't seem to upset ACPI test cases from what I
saw:
it seems we do have tests for TIS only (which I added when I was refactoring it
to TYPE_ACPI_DEV_AML_IF)
perhaps add a test for CRB before this patch a follow process described in
bios-tables-test.c
for updating expected blob
I read the file and looked at the commits for TIS tests but I'm not
sure I understand how it works. At what point do I specify that the
CRB device should be created for the test?
For me it would be a bit of trial an error as well. So here's my best guess:
Did you look at b193e5f9cccb322b0febd5a2aba486? You basically have to find out
which files
are going to change due to extending the tests, so doing something like in that
patch comes
after you found out which files are changing and iirc the tests are going to
complain about
those files. So I would try to first add CRB tests similar to the following to
tests/qtest/bios-tables-test.c.
in one patch, then run the test cases and they will tell you which files
changed, and then
add a patch similar to b193e5f9cccb322b0febd5a2aba486 before the test-enabling
patch.
if (tpm_model_is_available("-machine q35", "tpm-tis")) {
qtest_add_func("acpi/q35/tpm2-tis", test_acpi_q35_tcg_tpm2_tis);
qtest_add_func("acpi/q35/tpm12-tis",
test_acpi_q35_tcg_tpm12_tis);
}
I would try to something like the above to aarch64 here:
} else if (strcmp(arch, "aarch64") == 0) {
if (has_tcg && qtest_has_device("virtio-blk-pci")) {
qtest_add_func("acpi/virt", test_acpi_virt_tcg);
qtest_add_func("acpi/virt/acpihmatvirt",
test_acpi_virt_tcg_acpi_hmat);
Then you run the tests again then it should create those files with the ACPI
data and you copy them
to their destination (like in ca745d2277496464b54fd832c15c45d0227325bb) and
remove the changes from
tests/qtest/bios-tables-test-allowed-diff.h and that becomes your 3rd patch.
Once you run the tests
again with the 3rd patch there should be no more complaints about ACPI related
changes.
Since CRB ACPI tests are not enabled right now you can add these patches
somewhere in the middle of
the series or also at the end.
I hope this helps.
Stefan
Re: [PATCH v2 08/11] hw/loongarch/virt: add plug handler for TPM on SysBus
On 7/14/23 03:09, Joelle van Dyne wrote:
TPM needs to know its own base address in order to generate its DSDT
device entry.
Signed-off-by: Joelle van Dyne
It would be great to also cover the crb-device with tests:
from tests/qtest/meson.build:
(config_all.has_key('CONFIG_TCG') and
config_all_devices.has_key('CONFIG_TPM_TIS_SYSBUS') ?\
['tpm-tis-device-test', 'tpm-tis-device-swtpm-test'] : []) +
\
It should be easy to make a copy of these two tis-device tests and rename them
to crb-device tests, adapt them, and run them at least on aarch64.
Regards,
Stefan
Re: [PATCH v2 05/11] tpm_crb: use the ISA bus
On 7/17/23 09:46, Igor Mammedov wrote: On Fri, 14 Jul 2023 00:09:21 -0700 Joelle van Dyne wrote: Since this device is gated to only build for targets with the PC configuration, we should use the ISA bus like with TPM TIS. does it affect migration in any way? From guest pov it looks like there a new ISA device will appear and then if you do ping pong migration between old - new QEMU will really it work? If it will, then commit message here shall describe why it safe and why it works I would just leave the existing device as-is. This seems safest and we know thta it works. Stefan
Re: [PATCH v2 11/11] tpm_crb: support restoring older vmstate
On 7/14/23 15:44, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 12:12 PM Stefan Berger wrote: On 7/14/23 14:49, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 11:41 AM Stefan Berger wrote: On 7/14/23 14:22, Stefan Berger wrote: On 7/14/23 13:04, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 7:51 AM Stefan Berger wrote: On 7/14/23 10:05, Stefan Berger wrote: On 7/14/23 03:09, Joelle van Dyne wrote: When we moved to a single mapping and modified TPM CRB's VMState, it broke restoring of VMs that were saved on an older version. This change allows those VMs to gracefully migrate to the new memory mapping. Thanks. This has to be in 4/11 though. After applying the whole series and trying to resume state taken with current git master I cannot restore it but it leads to this error here. I would just leave it completely untouched in 4/11. 2023-07-14T14:46:34.547550Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration 2023-07-14T14:46:34.547799Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram' 2023-07-14T14:46:34.547835Z qemu-system-x86_64: load of migration failed: Invalid argument Stefan To be clear, you are asking to back out of 4/11? That patch changes how the registers are mapped so it's impossible to support the old style register mapping. This patch attempts to fix that with a Why can we not keep the old style register mapping as 'secondary mapping'? I think the first goal should be for existing TPM CRB device not to change anything, they keep their .read and .write behaivor as it. If you need different .read behavior for the sysbus device due to AARCH64 then it may want to use its own MemoryRegionOps. I am fairly sure that you could refactor the core of the existing tpm_crb_mmio_write() and have it work on s->regs and mmio regs. The former would be used by existing code, the latter for CRB sysbus calling into this new function from a wrapper. Stefan I agree that new QEMU should be able to read old QEMU state but vice versa is not always true. There's been many changes in the past that incremented the vmstate's version_id to indicate that the state format has changed. Also, we are not changing the .read behavior because in Unfortunately the CRB device is being used by x86 on some distros and the expectation is that this existing device can also downgrade to a previous version of QEMU I would say. I have read people migrating from RHEL 9.x even to RHEL 8.x and the expectation is that this works. But would the migration even work due to other parts of QEMU? The only way you can, say, migrate from QEMU 8.1.0 to 8.0.0 is if every single VMstate has its version_id unchanged. Does QEMU provide that guarantee? I'm fine with changing it but just want to make sure expectations are set correctly. Have you tested a downgrade and found that no other device impeded the process? No I have not done this. The best we can do is that CRB at least is not the reason that is causing such a failure and since we are introducing a new device it need not be the reason, either. Stefan
Re: [PATCH v2 11/11] tpm_crb: support restoring older vmstate
On 7/14/23 14:49, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 11:41 AM Stefan Berger wrote: On 7/14/23 14:22, Stefan Berger wrote: On 7/14/23 13:04, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 7:51 AM Stefan Berger wrote: On 7/14/23 10:05, Stefan Berger wrote: On 7/14/23 03:09, Joelle van Dyne wrote: When we moved to a single mapping and modified TPM CRB's VMState, it broke restoring of VMs that were saved on an older version. This change allows those VMs to gracefully migrate to the new memory mapping. Thanks. This has to be in 4/11 though. After applying the whole series and trying to resume state taken with current git master I cannot restore it but it leads to this error here. I would just leave it completely untouched in 4/11. 2023-07-14T14:46:34.547550Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration 2023-07-14T14:46:34.547799Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram' 2023-07-14T14:46:34.547835Z qemu-system-x86_64: load of migration failed: Invalid argument Stefan To be clear, you are asking to back out of 4/11? That patch changes how the registers are mapped so it's impossible to support the old style register mapping. This patch attempts to fix that with a Why can we not keep the old style register mapping as 'secondary mapping'? I think the first goal should be for existing TPM CRB device not to change anything, they keep their .read and .write behaivor as it. If you need different .read behavior for the sysbus device due to AARCH64 then it may want to use its own MemoryRegionOps. I am fairly sure that you could refactor the core of the existing tpm_crb_mmio_write() and have it work on s->regs and mmio regs. The former would be used by existing code, the latter for CRB sysbus calling into this new function from a wrapper. Stefan I agree that new QEMU should be able to read old QEMU state but vice versa is not always true. There's been many changes in the past that incremented the vmstate's version_id to indicate that the state format has changed. Also, we are not changing the .read behavior because in Unfortunately the CRB device is being used by x86 on some distros and the expectation is that this existing device can also downgrade to a previous version of QEMU I would say. I have read people migrating from RHEL 9.x even to RHEL 8.x and the expectation is that this works. Now you are introducing a new device and I think you can leave the existing device with its s->regs alone and have the new device with its mmio regs work a little different just to preserve the QEMU downgrade for x86. the old code, the only field that gets a dynamic update is tpmEstablished which we found is never changed. So effectively, .read Correct and that's why you don't need a .read in the new device. is just doing a memcpy of the `regs` state. This makes it possible to map the page as memory while retaining the same behavior as before. (We are changing the code but not the behavior). The issue with Windows's buggy tpm.sys driver is that fundamentally it cannot work with MemoryRegionOps. The way MMIO is implemented is that At least not with the .read part as it seems and you have to have the .write part to be able to react to cmd transfers etc. a hole is left in the guest memory space so when the device registers are accessed, the hypervisor traps it and sends it over to QEMU to handle. QEMU looks up the address, sees its a valid MMIO mapping, and calls into the MemoryRegionOps implementation. When tpm.sys does a LDP instruction access to the hole, the information for QEMU to determine if it's a valid access is not provided. Other hypervisors like Apple's VZ.framework and VMware will read the guest PC, manually decode the AArch64 instruction, determine the type of access, read the guest Rn registers, does a TLB lookup to determine the physical address, then emulate the MMIO. None of this capability currently exists in QEMU's ARM64 backend. That's why we decided the easier path is to tell QEMU that this mapping is RAM for read purposes and MMIO only for write purposes (thankfully Windows does not do a STP or we'd be hosed). Thanks, this confirms what I thought. Stefan
Re: [PATCH v2 11/11] tpm_crb: support restoring older vmstate
On 7/14/23 14:22, Stefan Berger wrote: On 7/14/23 13:04, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 7:51 AM Stefan Berger wrote: On 7/14/23 10:05, Stefan Berger wrote: On 7/14/23 03:09, Joelle van Dyne wrote: When we moved to a single mapping and modified TPM CRB's VMState, it broke restoring of VMs that were saved on an older version. This change allows those VMs to gracefully migrate to the new memory mapping. Thanks. This has to be in 4/11 though. After applying the whole series and trying to resume state taken with current git master I cannot restore it but it leads to this error here. I would just leave it completely untouched in 4/11. 2023-07-14T14:46:34.547550Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration 2023-07-14T14:46:34.547799Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram' 2023-07-14T14:46:34.547835Z qemu-system-x86_64: load of migration failed: Invalid argument Stefan To be clear, you are asking to back out of 4/11? That patch changes how the registers are mapped so it's impossible to support the old style register mapping. This patch attempts to fix that with a Why can we not keep the old style register mapping as 'secondary mapping'? I think the first goal should be for existing TPM CRB device not to change anything, they keep their .read and .write behaivor as it. If you need different .read behavior for the sysbus device due to AARCH64 then it may want to use its own MemoryRegionOps. I am fairly sure that you could refactor the core of the existing tpm_crb_mmio_write() and have it work on s->regs and mmio regs. The former would be used by existing code, the latter for CRB sysbus calling into this new function from a wrapper. Stefan
Re: [PATCH v2 11/11] tpm_crb: support restoring older vmstate
On 7/14/23 13:04, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 7:51 AM Stefan Berger wrote: On 7/14/23 10:05, Stefan Berger wrote: On 7/14/23 03:09, Joelle van Dyne wrote: When we moved to a single mapping and modified TPM CRB's VMState, it broke restoring of VMs that were saved on an older version. This change allows those VMs to gracefully migrate to the new memory mapping. Thanks. This has to be in 4/11 though. After applying the whole series and trying to resume state taken with current git master I cannot restore it but it leads to this error here. I would just leave it completely untouched in 4/11. 2023-07-14T14:46:34.547550Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration 2023-07-14T14:46:34.547799Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram' 2023-07-14T14:46:34.547835Z qemu-system-x86_64: load of migration failed: Invalid argument Stefan To be clear, you are asking to back out of 4/11? That patch changes how the registers are mapped so it's impossible to support the old style register mapping. This patch attempts to fix that with a Why can we not keep the old style register mapping as 'secondary mapping'? The expectation is that old VM state / CRB state can be used by the new QEMU and also new QEMU CRB state should be readable by old QEMU. So in a way the old 'secondary' mmaping has to hold the true value of the registers so that the latter works. migration path but I realized that I missed the "tpm-crb-cmd" ramblock. It can be added to v3 for this patch. Similar to the logic to have `legacy_regs` we will add a `legacy_cmdmem` memory region that is not registered with the system bus but only exists to migrate the data. Would that work? Also open to better ideas on migrating legacy saved state. If we back out of 4/11 (the split mapping), then the proposed way for working on Apple Silicon would not be available and we would have to go down the path of emulating AArch64 instruction in HVF backend (or decide not to support Apple Silicon).
Re: [PULL v1 1/1] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
On 7/14/23 13:58, Philippe Mathieu-Daudé wrote:
Hi Stefan,
On 14/7/23 17:41, Stefan Berger wrote:
The ppi command line option for the TIS device on sysbus never worked
and caused an immediate segfault. Remove support for it since it also
needs support in the firmware and needs testing inside the VM.
Reproducer with the ppi=on option passed:
qemu-system-aarch64 \
-machine virt,gic-version=3 \
-m 4G \
-nographic -no-acpi \
-chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm \
-device tpm-tis-device,tpmdev=tpm0,ppi=on
[...]
Segmentation fault (core dumped)
Signed-off-by: Stefan Berger
Reviewed-by: Eric Auger
Message-id: 20230713171955.149236-1-stef...@linux.ibm.com
---
hw/tpm/tpm_tis_sysbus.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
index 45e63efd63..6724b3d4f6 100644
--- a/hw/tpm/tpm_tis_sysbus.c
+++ b/hw/tpm/tpm_tis_sysbus.c
@@ -93,7 +93,6 @@ static void tpm_tis_sysbus_reset(DeviceState *dev)
static Property tpm_tis_sysbus_properties[] = {
DEFINE_PROP_UINT32("irq", TPMStateSysBus, state.irq_num, TPM_TIS_IRQ),
DEFINE_PROP_TPMBE("tpmdev", TPMStateSysBus, state.be_driver),
- DEFINE_PROP_BOOL("ppi", TPMStateSysBus, state.ppi_enabled, false),
Since properties are user-facing, shouldn't we deprecate their
removal? I'm not sure so I ask :) Otherwise we could register
the property with object_class_property_add_bool() and have
the setter display a warning. Anyhow I suppose now setting
"ppi" triggers some error, which is better than a abort.
ppi=on crashed it, now it doesn't crash it. On the next level, ppi=on may come
with the expectation that ppi is working on aarch64 and I am not sure about
this.
DEFINE_PROP_END_OF_LIST(),
};
Re: [PATCH v2 09/11] tpm_tis_sysbus: move DSDT AML generation to device
On 7/14/23 13:46, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 10:43 AM Stefan Berger wrote: On 7/14/23 13:39, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 10:37 AM Stefan Berger wrote: On 7/14/23 13:29, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger wrote: I don't know whether we would want multiple devices. tpm_find() usage is certainly not prepared for multiple devices. Sorry, "multiple TPM interfaces" here does not mean "at the same time". Will clarify the description. Good for the consolidation. Does moving the TIS to a different address help on aarch64? That was the first thing we tried and no it doesn't help. I would remove it if we don't have a known alternative address that makes it work. If we do, I think we should document it in tpm.rst. "It" is referring to tpm-tis-device? Note that it does work fine with Linux VMs. yes, tpm_tis_sysbus and I know it works with Liunux but I see this discussion here around Win 11 on aarch64. Why do we need to user another address than the standard address if for Win 11 on aarch64 it doesn't get it to work. The standard address won't work for Linux either. TPM TIS on standard address on ARM64 Virt machines = collision with DRAM, will not instantiate I thought that this was working with Linux on the aarch64 virt board as contributed by Eric Auger. https://github.com/qemu/qemu/commit/fcaa204194e15ba24cd53087dd616aabbc29e64f Also I had tested it to some extent: https://github.com/stefanberger/swtpm/issues/493#issuecomment-885221109 TPM TIS on SysBus with dynamically allocated address = works on Linux, cannot start on Windows Can the size really be an option? I don't see it useful and if one gave the wrong size it may break things. It was added for consistency (otherwise we have to determine the size by looking at the interface everywhere). Also, it is possible for the size to be larger than the constant. For example, Apple Silicon uses 16KiB page sizes and we may decide to force the device to be 16KiB aligned (not sure if this is needed yet while we still track down why the dual mapping was not working). In that case, we would need to inform the OS of the true region size to prevent any overlap issues. Both baseaddr and size should be provided only by the plug handler in the virt machine, otherwise things may break even if we get rid of size and have just an incorrect baseaddr.
Re: [PATCH v2 06/11] tpm_crb: move ACPI table building to device interface
On 7/14/23 03:09, Joelle van Dyne wrote:
This logic is similar to TPM TIS ISA device. Since TPM CRB can only
support TPM 2.0 backends, we check for this in realize.
Signed-off-by: Joelle van Dyne
This patch changes the order of in which the ACPI table elements are created
but doesn't matter and also doesn't seem to upset ACPI test cases from what I
saw:
Reviewed-by: Stefan Berger
---
hw/i386/acpi-build.c | 23 ---
hw/tpm/tpm_crb.c | 29 +
2 files changed, 29 insertions(+), 23 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9c74fa17ad..b767df39df 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1441,9 +1441,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
uint32_t nr_mem = machine->ram_slots;
int root_bus_limit = 0xFF;
PCIBus *bus = NULL;
-#ifdef CONFIG_TPM
-TPMIf *tpm = tpm_find();
-#endif
bool cxl_present = false;
int i;
VMBusBridge *vmbus_bridge = vmbus_bridge_find();
@@ -1793,26 +1790,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
}
}
-#ifdef CONFIG_TPM
-if (TPM_IS_CRB(tpm)) {
-dev = aml_device("TPM");
-aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
-aml_append(dev, aml_name_decl("_STR",
- aml_string("TPM 2.0 Device")));
-crs = aml_resource_template();
-aml_append(crs, aml_memory32_fixed(TPM_CRB_ADDR_BASE,
- TPM_CRB_ADDR_SIZE, AML_READ_WRITE));
-aml_append(dev, aml_name_decl("_CRS", crs));
-
-aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
-aml_append(dev, aml_name_decl("_UID", aml_int(1)));
-
-tpm_build_ppi_acpi(tpm, dev);
-
-aml_append(sb_scope, dev);
-}
-#endif
-
if (pcms->sgx_epc.size != 0) {
uint64_t epc_base = pcms->sgx_epc.base;
uint64_t epc_size = pcms->sgx_epc.size;
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 6144081d30..594696ffb8 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -19,6 +19,8 @@
#include "qemu/module.h"
#include "qapi/error.h"
#include "exec/address-spaces.h"
+#include "hw/acpi/acpi_aml_interface.h"
+#include "hw/acpi/tpm.h"
#include "hw/qdev-properties.h"
#include "hw/pci/pci_ids.h"
#include "hw/acpi/tpm.h"
@@ -99,6 +101,11 @@ static void tpm_crb_isa_realize(DeviceState *dev, Error
**errp)
return;
}
+if (tpm_crb_isa_get_version(TPM_IF(s)) != TPM_VERSION_2_0) {
+error_setg(errp, "TPM CRB only supports TPM 2.0 backends");
+return;
+}
+
tpm_crb_init_memory(OBJECT(s), >state, errp);
memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
@@ -116,10 +123,30 @@ static void tpm_crb_isa_realize(DeviceState *dev, Error
**errp)
}
}
+static void build_tpm_crb_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
+{
+Aml *dev, *crs;
+CRBState *s = CRB(adev);
+TPMIf *ti = TPM_IF(s);
+
+dev = aml_device("TPM");
+aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
+aml_append(dev, aml_name_decl("_STR", aml_string("TPM 2.0 Device")));
+aml_append(dev, aml_name_decl("_UID", aml_int(1)));
+aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
+crs = aml_resource_template();
+aml_append(crs, aml_memory32_fixed(TPM_CRB_ADDR_BASE, TPM_CRB_ADDR_SIZE,
+ AML_READ_WRITE));
+aml_append(dev, aml_name_decl("_CRS", crs));
+tpm_build_ppi_acpi(ti, dev);
+aml_append(scope, dev);
+}
+
static void tpm_crb_isa_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
TPMIfClass *tc = TPM_IF_CLASS(klass);
+AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass);
dc->realize = tpm_crb_isa_realize;
device_class_set_props(dc, tpm_crb_isa_properties);
@@ -128,6 +155,7 @@ static void tpm_crb_isa_class_init(ObjectClass *klass, void
*data)
tc->model = TPM_MODEL_TPM_CRB;
tc->get_version = tpm_crb_isa_get_version;
tc->request_completed = tpm_crb_isa_request_completed;
+adevc->build_dev_aml = build_tpm_crb_isa_aml;
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
}
@@ -139,6 +167,7 @@ static const TypeInfo tpm_crb_isa_info = {
.class_init = tpm_crb_isa_class_init,
.interfaces = (InterfaceInfo[]) {
{ TYPE_TPM_IF },
+{ TYPE_ACPI_DEV_AML_IF },
{ }
}
};
Re: [PATCH v2 10/11] tpm_crb_sysbus: introduce TPM CRB SysBus device
On 7/14/23 13:20, Joelle van Dyne wrote:
On Fri, Jul 14, 2023 at 7:27 AM Stefan Berger wrote:
On 7/14/23 03:09, Joelle van Dyne wrote:
This SysBus variant of the CRB interface supports dynamically locating
the MMIO interface so that Virt machines can use it. This interface
is currently the only one supported by QEMU that works on Windows 11
ARM64. We largely follow the TPM TIS SysBus device as a template.
To try out this device with Windows 11 before OVMF is updated, you
will need to modify `sysbud-fdt.c` and change the added line from:
```c
TYPE_BINDING(TYPE_TPM_CRB_SYSBUS, no_fdt_node),
```
to
```c
TYPE_BINDING(TYPE_TPM_CRB_SYSBUS, add_tpm_tis_fdt_node),
```
This change was not included because it can confuse Linux (although
from testing, it seems like Linux is able to properly ignore the
device from the TPM TIS driver and recognize it from the ACPI device
in the TPM CRB driver). A proper fix would require OVMF to recognize
the ACPI device and not depend on the FDT node for recognizing TPM.
The command line to try out this device with SWTPM is:
```
$ qemu-system-aarch64 \
-chardev socket,id=chrtpm0,path=tpm.sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm0 \
-device tpm-crb-device,tpmdev=tpm0
```
along with SWTPM:
```
$ swtpm \
--ctrl type=unixio,path=tpm.sock,terminate \
--tpmstate backend-uri=file://tpm.data \
--tpm2
```
Signed-off-by: Joelle van Dyne
---
docs/specs/tpm.rst | 1 +
include/hw/acpi/aml-build.h | 1 +
include/sysemu/tpm.h| 3 +
hw/acpi/aml-build.c | 7 +-
hw/arm/virt.c | 1 +
hw/core/sysbus-fdt.c| 1 +
hw/loongarch/virt.c | 1 +
hw/riscv/virt.c | 1 +
hw/tpm/tpm_crb_sysbus.c | 170
hw/arm/Kconfig | 1 +
hw/riscv/Kconfig| 1 +
hw/tpm/Kconfig | 5 ++
hw/tpm/meson.build | 2 +
13 files changed, 194 insertions(+), 1 deletion(-)
create mode 100644 hw/tpm/tpm_crb_sysbus.c
diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst
index 2bc29c9804..95aeb49220 100644
--- a/docs/specs/tpm.rst
+++ b/docs/specs/tpm.rst
@@ -46,6 +46,7 @@ operating system.
QEMU files related to TPM CRB interface:
- ``hw/tpm/tpm_crb.c``
- ``hw/tpm/tpm_crb_common.c``
+ - ``hw/tpm/tpm_crb_sysbus.c``
If you added the command line to use for Windows guests to this document
I think this would be helpful. And also mention that this must be used for
Windows
since the other ones don't work.
SPAPR interface
---
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index d1fb08514b..9660e16148 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -3,6 +3,7 @@
#include "hw/acpi/acpi-defs.h"
#include "hw/acpi/bios-linker-loader.h"
+#include "exec/hwaddr.h"
#define ACPI_BUILD_APPNAME6 "BOCHS "
#define ACPI_BUILD_APPNAME8 "BXPC"
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index 66e3b45f30..f79c8f3575 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -47,6 +47,7 @@ struct TPMIfClass {
#define TYPE_TPM_TIS_ISA"tpm-tis"
#define TYPE_TPM_TIS_SYSBUS "tpm-tis-device"
#define TYPE_TPM_CRB"tpm-crb"
+#define TYPE_TPM_CRB_SYSBUS "tpm-crb-device"
#define TYPE_TPM_SPAPR "tpm-spapr"
#define TYPE_TPM_TIS_I2C"tpm-tis-i2c"
@@ -56,6 +57,8 @@ struct TPMIfClass {
object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
#define TPM_IS_CRB(chr) \
object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
+#define TPM_IS_CRB_SYSBUS(chr) \
+object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB_SYSBUS)
#define TPM_IS_SPAPR(chr) \
object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)
#define TPM_IS_TIS_I2C(chr) \
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index ea331a20d1..f809137fc9 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -31,6 +31,7 @@
#include "hw/pci/pci_bus.h"
#include "hw/pci/pci_bridge.h"
#include "qemu/cutils.h"
+#include "qom/object.h"
static GArray *build_alloc_array(void)
{
@@ -2218,7 +2219,7 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker,
GArray *tcpalog,
{
uint8_t start_method_params[12] = {};
unsigned log_addr_offset;
-uint64_t control_area_start_address;
+uint64_t baseaddr, control_area_start_address;
TPMIf *tpmif = tpm_find();
uint32_t start_method;
AcpiTable table = { .sig = "TPM2", .rev = 4,
@@ -2236,6 +2237,10 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker,
GArray *tcpalog
Re: [PATCH v2 09/11] tpm_tis_sysbus: move DSDT AML generation to device
On 7/14/23 13:39, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 10:37 AM Stefan Berger wrote: On 7/14/23 13:29, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger wrote: I don't know whether we would want multiple devices. tpm_find() usage is certainly not prepared for multiple devices. Sorry, "multiple TPM interfaces" here does not mean "at the same time". Will clarify the description. Good for the consolidation. Does moving the TIS to a different address help on aarch64? That was the first thing we tried and no it doesn't help. I would remove it if we don't have a known alternative address that makes it work. If we do, I think we should document it in tpm.rst. "It" is referring to tpm-tis-device? Note that it does work fine with Linux VMs. yes, tpm_tis_sysbus and I know it works with Liunux but I see this discussion here around Win 11 on aarch64. Why do we need to user another address than the standard address if for Win 11 on aarch64 it doesn't get it to work. Can the size really be an option? I don't see it useful and if one gave the wrong size it may break things. It was added for consistency (otherwise we have to determine the size by looking at the interface everywhere). Also, it is possible for the size to be larger than the constant. For example, Apple Silicon uses 16KiB page sizes and we may decide to force the device to be 16KiB aligned (not sure if this is needed yet while we still track down why the dual mapping was not working). In that case, we would need to inform the OS of the true region size to prevent any overlap issues. Both baseaddr and size should be provided only by the plug handler in the virt machine, otherwise things may break even if we get rid of size and have just an incorrect baseaddr.
Re: [PATCH v2 09/11] tpm_tis_sysbus: move DSDT AML generation to device
On 7/14/23 13:29, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger wrote: I don't know whether we would want multiple devices. tpm_find() usage is certainly not prepared for multiple devices. Sorry, "multiple TPM interfaces" here does not mean "at the same time". Will clarify the description. Good for the consolidation. Does moving the TIS to a different address help on aarch64? That was the first thing we tried and no it doesn't help. I would remove it if we don't have a known alternative address that makes it work. If we do, I think we should document it in tpm.rst. Can the size really be an option? I don't see it useful and if one gave the wrong size it may break things. It was added for consistency (otherwise we have to determine the size by looking at the interface everywhere). Also, it is possible for the size to be larger than the constant. For example, Apple Silicon uses 16KiB page sizes and we may decide to force the device to be 16KiB aligned (not sure if this is needed yet while we still track down why the dual mapping was not working). In that case, we would need to inform the OS of the true region size to prevent any overlap issues. Both baseaddr and size should be provided only by the plug handler in the virt machine, otherwise things may break even if we get rid of size and have just an incorrect baseaddr.
Re: [PATCH v2 09/11] tpm_tis_sysbus: move DSDT AML generation to device
On 7/14/23 03:09, Joelle van Dyne wrote:
This reduces redundent code in different machine types with ACPI table
generation. Additionally, this will allow us to support multiple TPM
interfaces. Finally, this matches up with the TPM TIS ISA
I don't know whether we would want multiple devices. tpm_find() usage is
certainly not prepared for multiple devices.
implementation.
Ideally, we would be able to call `qbus_build_aml` and avoid any TPM
specific code in the ACPI table generation. However, currently we
still have to call `build_tpm2` anyways and it does not look like
most other ACPI devices support the `ACPI_DEV_AML_IF` interface.
Signed-off-by: Joelle van Dyne
---
hw/arm/virt-acpi-build.c | 38 ++
hw/loongarch/acpi-build.c | 38 ++
hw/tpm/tpm_tis_sysbus.c | 35 +++
3 files changed, 39 insertions(+), 72 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 6b674231c2..49b2f19440 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -35,6 +35,7 @@
#include "target/arm/cpu.h"
#include "hw/acpi/acpi-defs.h"
#include "hw/acpi/acpi.h"
+#include "hw/acpi/acpi_aml_interface.h"
#include "hw/nvram/fw_cfg.h"
#include "hw/acpi/bios-linker-loader.h"
#include "hw/acpi/aml-build.h"
@@ -208,41 +209,6 @@ static void acpi_dsdt_add_gpio(Aml *scope, const
MemMapEntry *gpio_memmap,
aml_append(scope, dev);
}
-#ifdef CONFIG_TPM
-static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
-{
-PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
-hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
-SysBusDevice *sbdev = SYS_BUS_DEVICE(tpm_find());
-MemoryRegion *sbdev_mr;
-hwaddr tpm_base;
-
-if (!sbdev) {
-return;
-}
-
-tpm_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
-assert(tpm_base != -1);
-
-tpm_base += pbus_base;
-
-sbdev_mr = sysbus_mmio_get_region(sbdev, 0);
-
-Aml *dev = aml_device("TPM0");
-aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
-aml_append(dev, aml_name_decl("_STR", aml_string("TPM 2.0 Device")));
-aml_append(dev, aml_name_decl("_UID", aml_int(0)));
-
-Aml *crs = aml_resource_template();
-aml_append(crs,
- aml_memory32_fixed(tpm_base,
- (uint32_t)memory_region_size(sbdev_mr),
- AML_READ_WRITE));
-aml_append(dev, aml_name_decl("_CRS", crs));
-aml_append(scope, dev);
-}
-#endif
-
#define ID_MAPPING_ENTRY_SIZE 20
#define SMMU_V3_ENTRY_SIZE 68
#define ROOT_COMPLEX_ENTRY_SIZE 36
@@ -891,7 +857,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
VirtMachineState *vms)
acpi_dsdt_add_power_button(scope);
#ifdef CONFIG_TPM
-acpi_dsdt_add_tpm(scope, vms);
+call_dev_aml_func(DEVICE(tpm_find()), scope);
#endif
aml_append(dsdt, scope);
diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c
index 0b62c3a2f7..4291e670c8 100644
--- a/hw/loongarch/acpi-build.c
+++ b/hw/loongarch/acpi-build.c
@@ -14,6 +14,7 @@
#include "target/loongarch/cpu.h"
#include "hw/acpi/acpi-defs.h"
#include "hw/acpi/acpi.h"
+#include "hw/acpi/acpi_aml_interface.h"
#include "hw/nvram/fw_cfg.h"
#include "hw/acpi/bios-linker-loader.h"
#include "migration/vmstate.h"
@@ -328,41 +329,6 @@ static void build_flash_aml(Aml *scope,
LoongArchMachineState *lams)
aml_append(scope, dev);
}
-#ifdef CONFIG_TPM
-static void acpi_dsdt_add_tpm(Aml *scope, LoongArchMachineState *vms)
-{
-PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
-hwaddr pbus_base = VIRT_PLATFORM_BUS_BASEADDRESS;
-SysBusDevice *sbdev = SYS_BUS_DEVICE(tpm_find());
-MemoryRegion *sbdev_mr;
-hwaddr tpm_base;
-
-if (!sbdev) {
-return;
-}
-
-tpm_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
-assert(tpm_base != -1);
-
-tpm_base += pbus_base;
-
-sbdev_mr = sysbus_mmio_get_region(sbdev, 0);
-
-Aml *dev = aml_device("TPM0");
-aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
-aml_append(dev, aml_name_decl("_STR", aml_string("TPM 2.0 Device")));
-aml_append(dev, aml_name_decl("_UID", aml_int(0)));
-
-Aml *crs = aml_resource_template();
-aml_append(crs,
- aml_memory32_fixed(tpm_base,
- (uint32_t)memory_region_size(sbdev_mr),
- AML_READ_WRITE));
-aml_append(dev, aml_name_decl("_CRS", crs));
-aml_append(scope, dev);
-}
-#endif
-
Good for the consolidation.
/* build DSDT */
static void
build_dsdt(GArray *table_data, BIOSLinker *linker, MachineState *machine)
@@ -379,7 +345,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
MachineState *machine)
build_la_ged_aml(dsdt, machine);
build_flash_aml(dsdt, lams);
#ifdef
[PULL v1 1/1] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
The ppi command line option for the TIS device on sysbus never worked
and caused an immediate segfault. Remove support for it since it also
needs support in the firmware and needs testing inside the VM.
Reproducer with the ppi=on option passed:
qemu-system-aarch64 \
-machine virt,gic-version=3 \
-m 4G \
-nographic -no-acpi \
-chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm \
-device tpm-tis-device,tpmdev=tpm0,ppi=on
[...]
Segmentation fault (core dumped)
Signed-off-by: Stefan Berger
Reviewed-by: Eric Auger
Message-id: 20230713171955.149236-1-stef...@linux.ibm.com
---
hw/tpm/tpm_tis_sysbus.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
index 45e63efd63..6724b3d4f6 100644
--- a/hw/tpm/tpm_tis_sysbus.c
+++ b/hw/tpm/tpm_tis_sysbus.c
@@ -93,7 +93,6 @@ static void tpm_tis_sysbus_reset(DeviceState *dev)
static Property tpm_tis_sysbus_properties[] = {
DEFINE_PROP_UINT32("irq", TPMStateSysBus, state.irq_num, TPM_TIS_IRQ),
DEFINE_PROP_TPMBE("tpmdev", TPMStateSysBus, state.be_driver),
-DEFINE_PROP_BOOL("ppi", TPMStateSysBus, state.ppi_enabled, false),
DEFINE_PROP_END_OF_LIST(),
};
--
2.41.0
[PULL v1 0/1] Merge tpm 2023/07/14 v1
Hello! This PR removes the 'ppi' boolean property from the tpm tis sysbus device. It could never be activated since it was leading to a segfault immediatley. Stefan The following changes since commit 3dd9e54703e6ae4f9ab3767f5cecc99edf08: Merge tag 'block-pull-request' of https://gitlab.com/stefanha/qemu into staging (2023-07-12 20:46:10 +0100) are available in the Git repository at: https://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2023-07-14-1 for you to fetch changes up to 4c46fe2ed492f35f411632c8b5a8442f322bc3f0: hw/tpm: TIS on sysbus: Remove unsupport ppi command line option (2023-07-14 11:31:54 -0400) Stefan Berger (1): hw/tpm: TIS on sysbus: Remove unsupport ppi command line option hw/tpm/tpm_tis_sysbus.c | 1 - 1 file changed, 1 deletion(-) -- 2.41.0
Re: [PATCH v2 11/11] tpm_crb: support restoring older vmstate
On 7/14/23 10:05, Stefan Berger wrote: On 7/14/23 03:09, Joelle van Dyne wrote: When we moved to a single mapping and modified TPM CRB's VMState, it broke restoring of VMs that were saved on an older version. This change allows those VMs to gracefully migrate to the new memory mapping. Thanks. This has to be in 4/11 though. After applying the whole series and trying to resume state taken with current git master I cannot restore it but it leads to this error here. I would just leave it completely untouched in 4/11. 2023-07-14T14:46:34.547550Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration 2023-07-14T14:46:34.547799Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram' 2023-07-14T14:46:34.547835Z qemu-system-x86_64: load of migration failed: Invalid argument Stefan
Re: [PATCH] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
On 7/14/23 09:51, Eric Auger wrote: Hi Stefan, On 7/14/23 13:51, Stefan Berger wrote: On 7/14/23 02:07, Joelle van Dyne wrote: On Thu, Jul 13, 2023 at 10:20 AM Stefan Berger wrote: The ppi command line option for the TIS device on sysbus never worked and caused an immediate segfault. Remove support for it since it also needs support in the firmware and needs testing inside the VM. Reproducer with the ppi=on option passed: qemu-system-aarch64 \ -machine virt,gic-version=3 \ -m 4G \ -nographic -no-acpi \ -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \ -tpmdev emulator,id=tpm0,chardev=chrtpm \ -device tpm-tis-device,tpmdev=tpm0,ppi=on [...] Segmentation fault (core dumped) Signed-off-by: Stefan Berger Do you need to add a VMSTATE_UNUSED_TEST in case a future QEMU version introduces a new field in the same position which will cause an issue when restoring from an older version? Hm, you got a point there. We will have to error-out in case someone sets ppi=on instead since the expectation that PPI would work is simply not there. v2 coming soon. as Joelle pointed it out ppi_enabled is not part of vmstate_tpm_tis_sysbus fields. And since it has never worked I suspect we cannot have any existing VM enabling it. So I don't get the issue with this 1st version? You are right. I repeated my test with restoring state of a VM taken before the removal of this field and it restored it. So that other patch is good and I am withdrawing this patch here. Stefan Thanks Eric Stefan
Re: [PATCH v2 10/11] tpm_crb_sysbus: introduce TPM CRB SysBus device
On 7/14/23 03:09, Joelle van Dyne wrote:
This SysBus variant of the CRB interface supports dynamically locating
the MMIO interface so that Virt machines can use it. This interface
is currently the only one supported by QEMU that works on Windows 11
ARM64. We largely follow the TPM TIS SysBus device as a template.
To try out this device with Windows 11 before OVMF is updated, you
will need to modify `sysbud-fdt.c` and change the added line from:
```c
TYPE_BINDING(TYPE_TPM_CRB_SYSBUS, no_fdt_node),
```
to
```c
TYPE_BINDING(TYPE_TPM_CRB_SYSBUS, add_tpm_tis_fdt_node),
```
This change was not included because it can confuse Linux (although
from testing, it seems like Linux is able to properly ignore the
device from the TPM TIS driver and recognize it from the ACPI device
in the TPM CRB driver). A proper fix would require OVMF to recognize
the ACPI device and not depend on the FDT node for recognizing TPM.
The command line to try out this device with SWTPM is:
```
$ qemu-system-aarch64 \
-chardev socket,id=chrtpm0,path=tpm.sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm0 \
-device tpm-crb-device,tpmdev=tpm0
```
along with SWTPM:
```
$ swtpm \
--ctrl type=unixio,path=tpm.sock,terminate \
--tpmstate backend-uri=file://tpm.data \
--tpm2
```
Signed-off-by: Joelle van Dyne
---
docs/specs/tpm.rst | 1 +
include/hw/acpi/aml-build.h | 1 +
include/sysemu/tpm.h| 3 +
hw/acpi/aml-build.c | 7 +-
hw/arm/virt.c | 1 +
hw/core/sysbus-fdt.c| 1 +
hw/loongarch/virt.c | 1 +
hw/riscv/virt.c | 1 +
hw/tpm/tpm_crb_sysbus.c | 170
hw/arm/Kconfig | 1 +
hw/riscv/Kconfig| 1 +
hw/tpm/Kconfig | 5 ++
hw/tpm/meson.build | 2 +
13 files changed, 194 insertions(+), 1 deletion(-)
create mode 100644 hw/tpm/tpm_crb_sysbus.c
diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst
index 2bc29c9804..95aeb49220 100644
--- a/docs/specs/tpm.rst
+++ b/docs/specs/tpm.rst
@@ -46,6 +46,7 @@ operating system.
QEMU files related to TPM CRB interface:
- ``hw/tpm/tpm_crb.c``
- ``hw/tpm/tpm_crb_common.c``
+ - ``hw/tpm/tpm_crb_sysbus.c``
If you added the command line to use for Windows guests to this document
I think this would be helpful. And also mention that this must be used for
Windows
since the other ones don't work.
SPAPR interface
---
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index d1fb08514b..9660e16148 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -3,6 +3,7 @@
#include "hw/acpi/acpi-defs.h"
#include "hw/acpi/bios-linker-loader.h"
+#include "exec/hwaddr.h"
#define ACPI_BUILD_APPNAME6 "BOCHS "
#define ACPI_BUILD_APPNAME8 "BXPC"
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index 66e3b45f30..f79c8f3575 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -47,6 +47,7 @@ struct TPMIfClass {
#define TYPE_TPM_TIS_ISA"tpm-tis"
#define TYPE_TPM_TIS_SYSBUS "tpm-tis-device"
#define TYPE_TPM_CRB"tpm-crb"
+#define TYPE_TPM_CRB_SYSBUS "tpm-crb-device"
#define TYPE_TPM_SPAPR "tpm-spapr"
#define TYPE_TPM_TIS_I2C"tpm-tis-i2c"
@@ -56,6 +57,8 @@ struct TPMIfClass {
object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
#define TPM_IS_CRB(chr) \
object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
+#define TPM_IS_CRB_SYSBUS(chr) \
+object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB_SYSBUS)
#define TPM_IS_SPAPR(chr) \
object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)
#define TPM_IS_TIS_I2C(chr) \
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index ea331a20d1..f809137fc9 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -31,6 +31,7 @@
#include "hw/pci/pci_bus.h"
#include "hw/pci/pci_bridge.h"
#include "qemu/cutils.h"
+#include "qom/object.h"
static GArray *build_alloc_array(void)
{
@@ -2218,7 +2219,7 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker,
GArray *tcpalog,
{
uint8_t start_method_params[12] = {};
unsigned log_addr_offset;
-uint64_t control_area_start_address;
+uint64_t baseaddr, control_area_start_address;
TPMIf *tpmif = tpm_find();
uint32_t start_method;
AcpiTable table = { .sig = "TPM2", .rev = 4,
@@ -2236,6 +2237,10 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker,
GArray *tcpalog,
} else if (TPM_IS_CRB(tpmif)) {
control_area_start_address = TPM_CRB_ADDR_CTRL;
start_method = TPM2_START_METHOD_CRB;
+} else if (TPM_IS_CRB_SYSBUS(tpmif)) {
+baseaddr = object_property_get_uint(OBJECT(tpmif), "baseaddr", NULL);
+control_area_start_address = baseaddr + A_CRB_CTRL_REQ;
Re: [PATCH v2 11/11] tpm_crb: support restoring older vmstate
On 7/14/23 03:09, Joelle van Dyne wrote: When we moved to a single mapping and modified TPM CRB's VMState, it broke restoring of VMs that were saved on an older version. This change allows those VMs to gracefully migrate to the new memory mapping. Thanks. This has to be in 4/11 though.
Re: [PATCH] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
On 7/14/23 02:07, Joelle van Dyne wrote: On Thu, Jul 13, 2023 at 10:20 AM Stefan Berger wrote: The ppi command line option for the TIS device on sysbus never worked and caused an immediate segfault. Remove support for it since it also needs support in the firmware and needs testing inside the VM. Reproducer with the ppi=on option passed: qemu-system-aarch64 \ -machine virt,gic-version=3 \ -m 4G \ -nographic -no-acpi \ -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \ -tpmdev emulator,id=tpm0,chardev=chrtpm \ -device tpm-tis-device,tpmdev=tpm0,ppi=on [...] Segmentation fault (core dumped) Signed-off-by: Stefan Berger Do you need to add a VMSTATE_UNUSED_TEST in case a future QEMU version introduces a new field in the same position which will cause an issue when restoring from an older version? Hm, you got a point there. We will have to error-out in case someone sets ppi=on instead since the expectation that PPI would work is simply not there. v2 coming soon. Stefan
Re: [PATCH v2 07/11] hw/arm/virt: add plug handler for TPM on SysBus
On 7/14/23 03:09, Joelle van Dyne wrote:
TPM needs to know its own base address in order to generate its DSDT
device entry.
Signed-off-by: Joelle van Dyne
---
hw/arm/virt.c | 37 +
1 file changed, 37 insertions(+)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7d9dbc2663..432148ef47 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2732,6 +2732,37 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
dev, _abort);
}
+#ifdef CONFIG_TPM
+static void virt_tpm_plug(VirtMachineState *vms, TPMIf *tpmif)
+{
+PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
+hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
+static void virt_tpm_plug(LoongArchMachineState *lams, TPMIf *tpmif)
+{
+PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(lams->platform_bus_dev);
+hwaddr pbus_base = VIRT_PLATFORM_BUS_BASEADDRESS;
These seem to be the differences between the loongarch and the arm virt
implementations.
Why not have a function with this signature that both archs can call?
static void virt_tpm_plug(PlatformBusDevice *pbus, hwaddr pbus_base, TPMIf
*tpmif)
Regards,
Stefan
Re: [PATCH v2 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
On 7/14/23 03:09, Joelle van Dyne wrote:
On Apple Silicon, when Windows performs a LDP on the CRB MMIO space,
the exception is not decoded by hardware and we cannot trap the MMIO
read. This led to the idea from @agraf to use the same mapping type as
ROM devices: namely that reads should be seen as memory type and
writes should trap as MMIO.
Once that was done, the second memory mapping of the command buffer
region was redundent and was removed.
A note about the removal of the read trap for `CRB_LOC_STATE`:
The only usage was to return the most up-to-date value for
`tpmEstablished`. However, `tpmEstablished` is only cleared when a
TPM2_HashStart operation is called which only exists for locality 4.
We do not handle locality 4. Indeed, the comment for the write handler
of `CRB_LOC_CTRL` makes the same argument for why it is not calling
the backend to reset the `tpmEstablished` bit (to 1).
As this bit is unused, we do not need to worry about updating it for
reads.
Signed-off-by: Joelle van Dyne
---
hw/tpm/tpm_crb.h| 2 -
hw/tpm/tpm_crb.c| 3 -
hw/tpm/tpm_crb_common.c | 126 +---
3 files changed, 65 insertions(+), 66 deletions(-)
diff --git a/hw/tpm/tpm_crb.h b/hw/tpm/tpm_crb.h
index da3a0cf256..7cdd37335f 100644
--- a/hw/tpm/tpm_crb.h
+++ b/hw/tpm/tpm_crb.h
@@ -26,9 +26,7 @@
typedef struct TPMCRBState {
TPMBackend *tpmbe;
TPMBackendCmd cmd;
-uint32_t regs[TPM_CRB_R_MAX];
MemoryRegion mmio;
-MemoryRegion cmdmem;
size_t be_buffer_size;
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 598c3e0161..07c6868d8d 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -68,7 +68,6 @@ static const VMStateDescription vmstate_tpm_crb_none = {
.name = "tpm-crb",
.pre_save = tpm_crb_none_pre_save,
.fields = (VMStateField[]) {
-VMSTATE_UINT32_ARRAY(state.regs, CRBState, TPM_CRB_R_MAX),
The same comment as stated on v1 still applies, this part has to stay since VM
state contains it:
2023-07-14T12:01:38.005199Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd",
cannot accept migration
2023-07-14T12:01:38.005318Z qemu-system-x86_64: error while loading state for
instance 0x0 of device 'ram'
2023-07-14T12:01:38.005350Z qemu-system-x86_64: load of migration failed:
Invalid argument
Stefan
Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
On 7/14/23 06:05, Peter Maydell wrote: On Thu, 13 Jul 2023 at 19:43, Stefan Berger wrote: On 7/13/23 13:18, Peter Maydell wrote: On Thu, 13 Jul 2023 at 18:16, Stefan Berger wrote: I guess the first point would be to decide whether to support an i2c bus on the virt board and then whether we can use the aspeed bus that we know that the tpm_tis_i2c device model works with but we don't know how Windows may react to it. It seems sysbus is already supported there so ... we may have a 'match'? You can use sysbus devices anywhere -- they're just 'anywhere' also includes aarch64 virt board I suppose. Yes. Literally any machine can have memory mapped devices. "this is a memory mapped device". The question is whether we should, or whether an i2c controller is more like what the real world uses (and if so, what i2c controller). I don't want to accept changes to the virt board that are hard to live with in future, because changing virt in non-backward compatible ways is painful. Once we have the CRB sysbus device we would keep it around forever and it seems to - not require any changes to the virt board (iiuc) since sysbus is already being used - works already with Windows and probably also Linux "Add a sysbus device to the virt board" is the kind of change I mean -- once you do that it's hard to take it out again, and if we decide in 6 months time that actually i2c would be the better option then we end up with two different ways to do the same thing and trying to deprecate the other one is a pain. At least CRB is a standard interface and from this perspective we are fine. I am not sure what would drive the introduction of the i2c bus in 6 months. I suppose one could then still use sysbus CRB device or the i2c device. The sysbus CRB device should still work then. Anyway, I think we should continue with this series. Stefan -- PMM
[PATCH] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
The ppi command line option for the TIS device on sysbus never worked
and caused an immediate segfault. Since it is part of the state of
a VM we cannot remove it but have to intercept ppi_enabled set to
true and display an error instead.
Reproducer with the ppi=on option passed:
qemu-system-aarch64 \
-machine virt,gic-version=3 \
-m 4G \
-nographic -no-acpi \
-chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm \
-device tpm-tis-device,tpmdev=tpm0,ppi=on
[...]
Segmentation fault (core dumped)
Signed-off-by: Stefan Berger
---
hw/tpm/tpm_tis_sysbus.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
index 45e63efd63..4319d31c88 100644
--- a/hw/tpm/tpm_tis_sysbus.c
+++ b/hw/tpm/tpm_tis_sysbus.c
@@ -124,6 +124,11 @@ static void tpm_tis_sysbus_realizefn(DeviceState *dev,
Error **errp)
error_setg(errp, "'tpmdev' property is required");
return;
}
+
+if (s->ppi_enabled) {
+error_setg(errp, "'ppi=on' is not supported by this device");
+return;
+}
}
static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data)
--
2.41.0
Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
On 7/13/23 13:18, Peter Maydell wrote: On Thu, 13 Jul 2023 at 18:16, Stefan Berger wrote: I guess the first point would be to decide whether to support an i2c bus on the virt board and then whether we can use the aspeed bus that we know that the tpm_tis_i2c device model works with but we don't know how Windows may react to it. It seems sysbus is already supported there so ... we may have a 'match'? You can use sysbus devices anywhere -- they're just 'anywhere' also includes aarch64 virt board I suppose. "this is a memory mapped device". The question is whether we should, or whether an i2c controller is more like what the real world uses (and if so, what i2c controller). I don't want to accept changes to the virt board that are hard to live with in future, because changing virt in non-backward compatible ways is painful. Once we have the CRB sysbus device we would keep it around forever and it seems to - not require any changes to the virt board (iiuc) since sysbus is already being used - works already with Windows and probably also Linux Stefan -- PMM
Re: [PATCH 05/11] tpm_crb: use the ISA bus
On 7/12/23 23:51, Joelle van Dyne wrote:
Since this device is gated to only build for targets with the PC
configuration, we should use the ISA bus like with TPM TIS.
Signed-off-by: Joelle van Dyne
I think this patch is good but I'd like to try it with resuming and old VM
snapshot and for this to work we need 04/11 to have the registers in the VM
state.
Stefan
---
hw/tpm/tpm_crb.c | 52
hw/tpm/Kconfig | 2 +-
2 files changed, 27 insertions(+), 27 deletions(-)
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 07c6868d8d..6144081d30 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -22,6 +22,7 @@
#include "hw/qdev-properties.h"
#include "hw/pci/pci_ids.h"
#include "hw/acpi/tpm.h"
+#include "hw/isa/isa.h"
#include "migration/vmstate.h"
#include "sysemu/tpm_backend.h"
#include "sysemu/tpm_util.h"
@@ -34,7 +35,7 @@
#include "tpm_crb.h"
struct CRBState {
-DeviceState parent_obj;
+ISADevice parent_obj;
TPMCRBState state;
};
@@ -43,49 +44,49 @@ typedef struct CRBState CRBState;
DECLARE_INSTANCE_CHECKER(CRBState, CRB,
TYPE_TPM_CRB)
-static void tpm_crb_none_request_completed(TPMIf *ti, int ret)
+static void tpm_crb_isa_request_completed(TPMIf *ti, int ret)
{
CRBState *s = CRB(ti);
tpm_crb_request_completed(>state, ret);
}
-static enum TPMVersion tpm_crb_none_get_version(TPMIf *ti)
+static enum TPMVersion tpm_crb_isa_get_version(TPMIf *ti)
{
CRBState *s = CRB(ti);
return tpm_crb_get_version(>state);
}
-static int tpm_crb_none_pre_save(void *opaque)
+static int tpm_crb_isa_pre_save(void *opaque)
{
CRBState *s = opaque;
return tpm_crb_pre_save(>state);
}
-static const VMStateDescription vmstate_tpm_crb_none = {
+static const VMStateDescription vmstate_tpm_crb_isa = {
.name = "tpm-crb",
-.pre_save = tpm_crb_none_pre_save,
+.pre_save = tpm_crb_isa_pre_save,
.fields = (VMStateField[]) {
VMSTATE_END_OF_LIST(),
}
};
-static Property tpm_crb_none_properties[] = {
+static Property tpm_crb_isa_properties[] = {
DEFINE_PROP_TPMBE("tpmdev", CRBState, state.tpmbe),
DEFINE_PROP_BOOL("ppi", CRBState, state.ppi_enabled, true),
DEFINE_PROP_END_OF_LIST(),
};
-static void tpm_crb_none_reset(void *dev)
+static void tpm_crb_isa_reset(void *dev)
{
CRBState *s = CRB(dev);
return tpm_crb_reset(>state, TPM_CRB_ADDR_BASE);
}
-static void tpm_crb_none_realize(DeviceState *dev, Error **errp)
+static void tpm_crb_isa_realize(DeviceState *dev, Error **errp)
{
CRBState *s = CRB(dev);
@@ -100,52 +101,51 @@ static void tpm_crb_none_realize(DeviceState *dev, Error
**errp)
tpm_crb_init_memory(OBJECT(s), >state, errp);
-memory_region_add_subregion(get_system_memory(),
+memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
TPM_CRB_ADDR_BASE, >state.mmio);
if (s->state.ppi_enabled) {
-memory_region_add_subregion(get_system_memory(),
+memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
TPM_PPI_ADDR_BASE, >state.ppi.ram);
}
if (xen_enabled()) {
-tpm_crb_none_reset(dev);
+tpm_crb_isa_reset(dev);
} else {
-qemu_register_reset(tpm_crb_none_reset, dev);
+qemu_register_reset(tpm_crb_isa_reset, dev);
}
}
-static void tpm_crb_none_class_init(ObjectClass *klass, void *data)
+static void tpm_crb_isa_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
TPMIfClass *tc = TPM_IF_CLASS(klass);
-dc->realize = tpm_crb_none_realize;
-device_class_set_props(dc, tpm_crb_none_properties);
-dc->vmsd = _tpm_crb_none;
+dc->realize = tpm_crb_isa_realize;
+device_class_set_props(dc, tpm_crb_isa_properties);
+dc->vmsd = _tpm_crb_isa;
dc->user_creatable = true;
tc->model = TPM_MODEL_TPM_CRB;
-tc->get_version = tpm_crb_none_get_version;
-tc->request_completed = tpm_crb_none_request_completed;
+tc->get_version = tpm_crb_isa_get_version;
+tc->request_completed = tpm_crb_isa_request_completed;
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
}
-static const TypeInfo tpm_crb_none_info = {
+static const TypeInfo tpm_crb_isa_info = {
.name = TYPE_TPM_CRB,
-/* could be TYPE_SYS_BUS_DEVICE (or LPC etc) */
-.parent = TYPE_DEVICE,
+.parent = TYPE_ISA_DEVICE,
.instance_size = sizeof(CRBState),
-.class_init = tpm_crb_none_class_init,
+.class_init = tpm_crb_isa_class_init,
.interfaces = (InterfaceInfo[]) {
{ TYPE_TPM_IF },
{ }
}
};
-static void tpm_crb_none_register(void)
+static void tpm_crb_isa_register(void)
{
-type_register_static(_crb_none_info);
+type_register_static(_crb_isa_info);
}
-type_init(tpm_crb_none_register)
+type_init(tpm_crb_isa_register)
diff --git
Re: [PATCH 09/11] tpm_tis_sysbus: fix crash when PPI is enabled
On 7/13/23 14:15, Joelle van Dyne wrote:
On Thu, Jul 13, 2023 at 9:49 AM Stefan Berger wrote:
The tpm-tis-device doesn't work for x86_64 but for aarch64.
We have this here in this file:
DEFINE_PROP_BOOL("ppi", TPMStateSysBus, state.ppi_enabled, false),
I don't know whether ppi would work on aarch64. It needs firmware support like
in edk2.
I think the best solution is to remove this DEFINE_PROP_BOOL() and if someone
wants
to enable it they would have to add firmware support and test it before
re-enabling it.
Stefan
static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data)
Yeah, I'm not sure if PPI works with AARCH64 since I didn't bother to
change it to not use hard coded addresses. However, isn't that "ppi"
overridable from the command line? If so, should we add a check in
"realize" to error if PPI=true? Otherwise, it will just crash.
Once the option is removed via my patch (cc'ed you), then you get this once you
pass ppi=on on the command line:
qemu-system-aarch64: -device tpm-tis-device,tpmdev=tpm0,ppi=on: Property
'tpm-tis-device.ppi' not found
This disables it for good.
Stefan
