* [PATCH 00/11] tpm: introduce TPM CRB SysBus device
@ 2023-07-13 3:51 Joelle van Dyne
2023-07-13 3:51 ` [PATCH 01/11] tpm_crb: refactor common code Joelle van Dyne
` (11 more replies)
0 siblings, 12 replies; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-13 3:51 UTC (permalink / raw)
To: qemu-devel
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 caused page faults. @agraf suggested that we do this to avoid having to
do AARCH64 decoding in the HVF 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.
Joelle van Dyne (11):
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: use the ISA bus
tpm_crb: move ACPI table building to device interface
hw/arm/virt: add plug handler for TPM on SysBus
hw/loongarch/virt: add plug handler for TPM on SysBus
tpm_tis_sysbus: fix crash when PPI is enabled
tpm_tis_sysbus: move DSDT AML generation to device
tpm_crb_sysbus: introduce TPM CRB SysBus device
docs/specs/tpm.rst | 2 +
hw/tpm/tpm_crb.h | 74 +++++++++
hw/tpm/tpm_ppi.h | 10 +-
include/hw/acpi/aml-build.h | 1 +
include/hw/acpi/tpm.h | 3 +-
include/sysemu/tpm.h | 3 +
hw/acpi/aml-build.c | 7 +-
hw/arm/virt-acpi-build.c | 38 +----
hw/arm/virt.c | 38 +++++
hw/core/sysbus-fdt.c | 1 +
hw/i386/acpi-build.c | 23 ---
hw/loongarch/acpi-build.c | 38 +----
hw/loongarch/virt.c | 38 +++++
hw/riscv/virt.c | 1 +
hw/tpm/tpm_crb.c | 307 ++++++++----------------------------
hw/tpm/tpm_crb_common.c | 224 ++++++++++++++++++++++++++
hw/tpm/tpm_crb_sysbus.c | 178 +++++++++++++++++++++
hw/tpm/tpm_ppi.c | 5 +-
hw/tpm/tpm_tis_isa.c | 5 +-
hw/tpm/tpm_tis_sysbus.c | 43 +++++
tests/qtest/tpm-crb-test.c | 2 +-
tests/qtest/tpm-util.c | 2 +-
hw/arm/Kconfig | 1 +
hw/riscv/Kconfig | 1 +
hw/tpm/Kconfig | 7 +-
hw/tpm/meson.build | 3 +
hw/tpm/trace-events | 2 +-
27 files changed, 703 insertions(+), 354 deletions(-)
create mode 100644 hw/tpm/tpm_crb.h
create mode 100644 hw/tpm/tpm_crb_common.c
create mode 100644 hw/tpm/tpm_crb_sysbus.c
--
2.39.2 (Apple Git-143)
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 01/11] tpm_crb: refactor common code
2023-07-13 3:51 [PATCH 00/11] tpm: introduce TPM CRB SysBus device Joelle van Dyne
@ 2023-07-13 3:51 ` Joelle van Dyne
2023-07-13 13:22 ` Stefan Berger
2023-07-13 3:51 ` [PATCH 02/11] tpm_crb: CTRL_RSP_ADDR is 64-bits wide Joelle van Dyne
` (10 subsequent siblings)
11 siblings, 1 reply; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-13 3:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Joelle van Dyne, Stefan Berger
In preparation for the SysBus variant, we move common code styled
after the TPM TIS devices.
To maintain compatibility, we do not rename the existing tpm-crb
device.
Signed-off-by: Joelle van Dyne <j@getutm.app>
---
docs/specs/tpm.rst | 1 +
hw/tpm/tpm_crb.h | 76 +++++++++++
hw/tpm/tpm_crb.c | 270 ++++++----------------------------------
hw/tpm/tpm_crb_common.c | 218 ++++++++++++++++++++++++++++++++
hw/tpm/meson.build | 1 +
hw/tpm/trace-events | 2 +-
6 files changed, 333 insertions(+), 235 deletions(-)
create mode 100644 hw/tpm/tpm_crb.h
create mode 100644 hw/tpm/tpm_crb_common.c
diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst
index efe124a148..2bc29c9804 100644
--- a/docs/specs/tpm.rst
+++ b/docs/specs/tpm.rst
@@ -45,6 +45,7 @@ operating system.
QEMU files related to TPM CRB interface:
- ``hw/tpm/tpm_crb.c``
+ - ``hw/tpm/tpm_crb_common.c``
SPAPR interface
---------------
diff --git a/hw/tpm/tpm_crb.h b/hw/tpm/tpm_crb.h
new file mode 100644
index 0000000000..da3a0cf256
--- /dev/null
+++ b/hw/tpm/tpm_crb.h
@@ -0,0 +1,76 @@
+/*
+ * tpm_crb.h - QEMU's TPM CRB interface emulator
+ *
+ * Copyright (c) 2018 Red Hat, Inc.
+ *
+ * Authors:
+ * Marc-André Lureau <marcandre.lureau@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * tpm_crb is a device for TPM 2.0 Command Response Buffer (CRB) Interface
+ * as defined in TCG PC Client Platform TPM Profile (PTP) Specification
+ * Family “2.0” Level 00 Revision 01.03 v22
+ */
+#ifndef TPM_TPM_CRB_H
+#define TPM_TPM_CRB_H
+
+#include "exec/memory.h"
+#include "hw/acpi/tpm.h"
+#include "sysemu/tpm_backend.h"
+#include "tpm_ppi.h"
+
+#define CRB_CTRL_CMD_SIZE (TPM_CRB_ADDR_SIZE - A_CRB_DATA_BUFFER)
+
+typedef struct TPMCRBState {
+ TPMBackend *tpmbe;
+ TPMBackendCmd cmd;
+ uint32_t regs[TPM_CRB_R_MAX];
+ MemoryRegion mmio;
+ MemoryRegion cmdmem;
+
+ size_t be_buffer_size;
+
+ bool ppi_enabled;
+ TPMPPI ppi;
+} TPMCRBState;
+
+#define CRB_INTF_TYPE_CRB_ACTIVE 0b1
+#define CRB_INTF_VERSION_CRB 0b1
+#define CRB_INTF_CAP_LOCALITY_0_ONLY 0b0
+#define CRB_INTF_CAP_IDLE_FAST 0b0
+#define CRB_INTF_CAP_XFER_SIZE_64 0b11
+#define CRB_INTF_CAP_FIFO_NOT_SUPPORTED 0b0
+#define CRB_INTF_CAP_CRB_SUPPORTED 0b1
+#define CRB_INTF_IF_SELECTOR_CRB 0b1
+
+enum crb_loc_ctrl {
+ CRB_LOC_CTRL_REQUEST_ACCESS = BIT(0),
+ CRB_LOC_CTRL_RELINQUISH = BIT(1),
+ CRB_LOC_CTRL_SEIZE = BIT(2),
+ CRB_LOC_CTRL_RESET_ESTABLISHMENT_BIT = BIT(3),
+};
+
+enum crb_ctrl_req {
+ CRB_CTRL_REQ_CMD_READY = BIT(0),
+ CRB_CTRL_REQ_GO_IDLE = BIT(1),
+};
+
+enum crb_start {
+ CRB_START_INVOKE = BIT(0),
+};
+
+enum crb_cancel {
+ CRB_CANCEL_INVOKE = BIT(0),
+};
+
+#define TPM_CRB_NO_LOCALITY 0xff
+
+void tpm_crb_request_completed(TPMCRBState *s, int ret);
+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);
+
+#endif /* TPM_TPM_CRB_H */
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index ea930da545..3ef4977fb5 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -31,257 +31,62 @@
#include "tpm_ppi.h"
#include "trace.h"
#include "qom/object.h"
+#include "tpm_crb.h"
struct CRBState {
DeviceState parent_obj;
- TPMBackend *tpmbe;
- TPMBackendCmd cmd;
- uint32_t regs[TPM_CRB_R_MAX];
- MemoryRegion mmio;
- MemoryRegion cmdmem;
-
- size_t be_buffer_size;
-
- bool ppi_enabled;
- TPMPPI ppi;
+ TPMCRBState state;
};
typedef struct CRBState CRBState;
DECLARE_INSTANCE_CHECKER(CRBState, CRB,
TYPE_TPM_CRB)
-#define CRB_INTF_TYPE_CRB_ACTIVE 0b1
-#define CRB_INTF_VERSION_CRB 0b1
-#define CRB_INTF_CAP_LOCALITY_0_ONLY 0b0
-#define CRB_INTF_CAP_IDLE_FAST 0b0
-#define CRB_INTF_CAP_XFER_SIZE_64 0b11
-#define CRB_INTF_CAP_FIFO_NOT_SUPPORTED 0b0
-#define CRB_INTF_CAP_CRB_SUPPORTED 0b1
-#define CRB_INTF_IF_SELECTOR_CRB 0b1
-
-#define CRB_CTRL_CMD_SIZE (TPM_CRB_ADDR_SIZE - A_CRB_DATA_BUFFER)
-
-enum crb_loc_ctrl {
- CRB_LOC_CTRL_REQUEST_ACCESS = BIT(0),
- CRB_LOC_CTRL_RELINQUISH = BIT(1),
- CRB_LOC_CTRL_SEIZE = BIT(2),
- CRB_LOC_CTRL_RESET_ESTABLISHMENT_BIT = BIT(3),
-};
-
-enum crb_ctrl_req {
- CRB_CTRL_REQ_CMD_READY = BIT(0),
- CRB_CTRL_REQ_GO_IDLE = BIT(1),
-};
-
-enum crb_start {
- CRB_START_INVOKE = BIT(0),
-};
-
-enum crb_cancel {
- CRB_CANCEL_INVOKE = BIT(0),
-};
-
-#define TPM_CRB_NO_LOCALITY 0xff
-
-static uint64_t tpm_crb_mmio_read(void *opaque, hwaddr addr,
- unsigned size)
-{
- CRBState *s = CRB(opaque);
- void *regs = (void *)&s->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(CRBState *s)
-{
- if (!ARRAY_FIELD_EX32(s->regs, CRB_LOC_STATE, locAssigned)) {
- return TPM_CRB_NO_LOCALITY;
- }
- return ARRAY_FIELD_EX32(s->regs, CRB_LOC_STATE, activeLocality);
-}
-
-static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
- uint64_t val, unsigned size)
-{
- CRBState *s = CRB(opaque);
- uint8_t locty = addr >> 12;
-
- trace_tpm_crb_mmio_write(addr, size, val);
-
- switch (addr) {
- case A_CRB_CTRL_REQ:
- switch (val) {
- case CRB_CTRL_REQ_CMD_READY:
- ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
- tpmIdle, 0);
- break;
- case CRB_CTRL_REQ_GO_IDLE:
- ARRAY_FIELD_DP32(s->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) {
- 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(&s->cmdmem);
-
- s->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),
- .out = mem,
- .out_len = s->be_buffer_size,
- };
-
- tpm_backend_deliver_request(s->tpmbe, &s->cmd);
- }
- break;
- case A_CRB_LOC_CTRL:
- switch (val) {
- case CRB_LOC_CTRL_RESET_ESTABLISHMENT_BIT:
- /* not loc 3 or 4 */
- break;
- case CRB_LOC_CTRL_RELINQUISH:
- ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
- locAssigned, 0);
- ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
- Granted, 0);
- break;
- case CRB_LOC_CTRL_REQUEST_ACCESS:
- ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
- Granted, 1);
- ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
- beenSeized, 0);
- ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
- locAssigned, 1);
- break;
- }
- break;
- }
-}
-
-static 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,
- },
-};
-
-static void tpm_crb_request_completed(TPMIf *ti, int ret)
+static void tpm_crb_none_request_completed(TPMIf *ti, int ret)
{
CRBState *s = CRB(ti);
- s->regs[R_CRB_CTRL_START] &= ~CRB_START_INVOKE;
- if (ret != 0) {
- ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
- tpmSts, 1); /* fatal error */
- }
- memory_region_set_dirty(&s->cmdmem, 0, CRB_CTRL_CMD_SIZE);
+ tpm_crb_request_completed(&s->state, ret);
}
-static enum TPMVersion tpm_crb_get_version(TPMIf *ti)
+static enum TPMVersion tpm_crb_none_get_version(TPMIf *ti)
{
CRBState *s = CRB(ti);
- return tpm_backend_get_tpm_version(s->tpmbe);
+ return tpm_crb_get_version(&s->state);
}
-static int tpm_crb_pre_save(void *opaque)
+static int tpm_crb_none_pre_save(void *opaque)
{
CRBState *s = opaque;
- tpm_backend_finish_sync(s->tpmbe);
-
- return 0;
+ return tpm_crb_pre_save(&s->state);
}
-static const VMStateDescription vmstate_tpm_crb = {
+static const VMStateDescription vmstate_tpm_crb_none = {
.name = "tpm-crb",
- .pre_save = tpm_crb_pre_save,
+ .pre_save = tpm_crb_none_pre_save,
.fields = (VMStateField[]) {
- VMSTATE_UINT32_ARRAY(regs, CRBState, TPM_CRB_R_MAX),
+ VMSTATE_UINT32_ARRAY(state.regs, CRBState, TPM_CRB_R_MAX),
VMSTATE_END_OF_LIST(),
}
};
-static Property tpm_crb_properties[] = {
- DEFINE_PROP_TPMBE("tpmdev", CRBState, tpmbe),
- DEFINE_PROP_BOOL("ppi", CRBState, ppi_enabled, true),
+static Property tpm_crb_none_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_reset(void *dev)
+static void tpm_crb_none_reset(void *dev)
{
CRBState *s = CRB(dev);
- if (s->ppi_enabled) {
- tpm_ppi_reset(&s->ppi);
- }
- tpm_backend_reset(s->tpmbe);
-
- memset(s->regs, 0, sizeof(s->regs));
-
- ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
- tpmRegValidSts, 1);
- ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
- tpmIdle, 1);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
- InterfaceType, CRB_INTF_TYPE_CRB_ACTIVE);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
- InterfaceVersion, CRB_INTF_VERSION_CRB);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
- CapLocality, CRB_INTF_CAP_LOCALITY_0_ONLY);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
- CapCRBIdleBypass, CRB_INTF_CAP_IDLE_FAST);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
- CapDataXferSizeSupport, CRB_INTF_CAP_XFER_SIZE_64);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
- CapFIFO, CRB_INTF_CAP_FIFO_NOT_SUPPORTED);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
- CapCRB, CRB_INTF_CAP_CRB_SUPPORTED);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
- InterfaceSelector, CRB_INTF_IF_SELECTOR_CRB);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
- RID, 0b0000);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID2,
- VID, PCI_VENDOR_ID_IBM);
-
- s->regs[R_CRB_CTRL_CMD_SIZE] = CRB_CTRL_CMD_SIZE;
- s->regs[R_CRB_CTRL_CMD_LADDR] = TPM_CRB_ADDR_BASE + A_CRB_DATA_BUFFER;
- s->regs[R_CRB_CTRL_RSP_SIZE] = CRB_CTRL_CMD_SIZE;
- s->regs[R_CRB_CTRL_RSP_ADDR] = TPM_CRB_ADDR_BASE + A_CRB_DATA_BUFFER;
-
- s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->tpmbe),
- CRB_CTRL_CMD_SIZE);
-
- if (tpm_backend_startup_tpm(s->tpmbe, s->be_buffer_size) < 0) {
- exit(1);
- }
+ return tpm_crb_reset(&s->state, TPM_CRB_ADDR_BASE);
}
-static void tpm_crb_realize(DeviceState *dev, Error **errp)
+static void tpm_crb_none_realize(DeviceState *dev, Error **errp)
{
CRBState *s = CRB(dev);
@@ -289,64 +94,61 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
error_setg(errp, "at most one TPM device is permitted");
return;
}
- if (!s->tpmbe) {
+ if (!s->state.tpmbe) {
error_setg(errp, "'tpmdev' property is required");
return;
}
- memory_region_init_io(&s->mmio, OBJECT(s), &tpm_crb_memory_ops, s,
- "tpm-crb-mmio", sizeof(s->regs));
- memory_region_init_ram(&s->cmdmem, OBJECT(s),
- "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
+ tpm_crb_init_memory(OBJECT(s), &s->state, errp);
memory_region_add_subregion(get_system_memory(),
- TPM_CRB_ADDR_BASE, &s->mmio);
+ TPM_CRB_ADDR_BASE, &s->state.mmio);
memory_region_add_subregion(get_system_memory(),
- TPM_CRB_ADDR_BASE + sizeof(s->regs), &s->cmdmem);
+ TPM_CRB_ADDR_BASE + sizeof(s->state.regs), &s->state.cmdmem);
- if (s->ppi_enabled) {
- tpm_ppi_init(&s->ppi, get_system_memory(),
+ if (s->state.ppi_enabled) {
+ tpm_ppi_init(&s->state.ppi, get_system_memory(),
TPM_PPI_ADDR_BASE, OBJECT(s));
}
if (xen_enabled()) {
- tpm_crb_reset(dev);
+ tpm_crb_none_reset(dev);
} else {
- qemu_register_reset(tpm_crb_reset, dev);
+ qemu_register_reset(tpm_crb_none_reset, dev);
}
}
-static void tpm_crb_class_init(ObjectClass *klass, void *data)
+static void tpm_crb_none_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
TPMIfClass *tc = TPM_IF_CLASS(klass);
- dc->realize = tpm_crb_realize;
- device_class_set_props(dc, tpm_crb_properties);
- dc->vmsd = &vmstate_tpm_crb;
+ dc->realize = tpm_crb_none_realize;
+ device_class_set_props(dc, tpm_crb_none_properties);
+ dc->vmsd = &vmstate_tpm_crb_none;
dc->user_creatable = true;
tc->model = TPM_MODEL_TPM_CRB;
- tc->get_version = tpm_crb_get_version;
- tc->request_completed = tpm_crb_request_completed;
+ tc->get_version = tpm_crb_none_get_version;
+ tc->request_completed = tpm_crb_none_request_completed;
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
}
-static const TypeInfo tpm_crb_info = {
+static const TypeInfo tpm_crb_none_info = {
.name = TYPE_TPM_CRB,
/* could be TYPE_SYS_BUS_DEVICE (or LPC etc) */
.parent = TYPE_DEVICE,
.instance_size = sizeof(CRBState),
- .class_init = tpm_crb_class_init,
+ .class_init = tpm_crb_none_class_init,
.interfaces = (InterfaceInfo[]) {
{ TYPE_TPM_IF },
{ }
}
};
-static void tpm_crb_register(void)
+static void tpm_crb_none_register(void)
{
- type_register_static(&tpm_crb_info);
+ type_register_static(&tpm_crb_none_info);
}
-type_init(tpm_crb_register)
+type_init(tpm_crb_none_register)
diff --git a/hw/tpm/tpm_crb_common.c b/hw/tpm/tpm_crb_common.c
new file mode 100644
index 0000000000..4c173affb6
--- /dev/null
+++ b/hw/tpm/tpm_crb_common.c
@@ -0,0 +1,218 @@
+/*
+ * tpm_crb.c - QEMU's TPM CRB interface emulator
+ *
+ * Copyright (c) 2018 Red Hat, Inc.
+ *
+ * Authors:
+ * Marc-André Lureau <marcandre.lureau@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * tpm_crb is a device for TPM 2.0 Command Response Buffer (CRB) Interface
+ * as defined in TCG PC Client Platform TPM Profile (PTP) Specification
+ * Family “2.0” Level 00 Revision 01.03 v22
+ */
+
+#include "qemu/osdep.h"
+
+#include "qemu/module.h"
+#include "qapi/error.h"
+#include "exec/address-spaces.h"
+#include "hw/qdev-properties.h"
+#include "hw/pci/pci_ids.h"
+#include "hw/acpi/tpm.h"
+#include "migration/vmstate.h"
+#include "sysemu/tpm_backend.h"
+#include "sysemu/tpm_util.h"
+#include "sysemu/reset.h"
+#include "sysemu/xen.h"
+#include "tpm_prop.h"
+#include "tpm_ppi.h"
+#include "trace.h"
+#include "qom/object.h"
+#include "tpm_crb.h"
+
+static uint64_t tpm_crb_mmio_read(void *opaque, hwaddr addr,
+ unsigned size)
+{
+ TPMCRBState *s = opaque;
+ void *regs = (void *)&s->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)) {
+ return TPM_CRB_NO_LOCALITY;
+ }
+ return ARRAY_FIELD_EX32(s->regs, CRB_LOC_STATE, activeLocality);
+}
+
+static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
+ uint64_t val, unsigned size)
+{
+ TPMCRBState *s = opaque;
+ uint8_t locty = addr >> 12;
+
+ trace_tpm_crb_mmio_write(addr, size, val);
+
+ switch (addr) {
+ case A_CRB_CTRL_REQ:
+ switch (val) {
+ case CRB_CTRL_REQ_CMD_READY:
+ ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
+ tpmIdle, 0);
+ break;
+ case CRB_CTRL_REQ_GO_IDLE:
+ ARRAY_FIELD_DP32(s->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) {
+ 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(&s->cmdmem);
+
+ s->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),
+ .out = mem,
+ .out_len = s->be_buffer_size,
+ };
+
+ tpm_backend_deliver_request(s->tpmbe, &s->cmd);
+ }
+ break;
+ case A_CRB_LOC_CTRL:
+ switch (val) {
+ case CRB_LOC_CTRL_RESET_ESTABLISHMENT_BIT:
+ /* not loc 3 or 4 */
+ break;
+ case CRB_LOC_CTRL_RELINQUISH:
+ ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
+ locAssigned, 0);
+ ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
+ Granted, 0);
+ break;
+ case CRB_LOC_CTRL_REQUEST_ACCESS:
+ ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
+ Granted, 1);
+ ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
+ beenSeized, 0);
+ ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
+ locAssigned, 1);
+ break;
+ }
+ break;
+ }
+}
+
+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,
+ },
+};
+
+void tpm_crb_request_completed(TPMCRBState *s, int ret)
+{
+ s->regs[R_CRB_CTRL_START] &= ~CRB_START_INVOKE;
+ if (ret != 0) {
+ ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
+ tpmSts, 1); /* fatal error */
+ }
+ memory_region_set_dirty(&s->cmdmem, 0, CRB_CTRL_CMD_SIZE);
+}
+
+enum TPMVersion tpm_crb_get_version(TPMCRBState *s)
+{
+ return tpm_backend_get_tpm_version(s->tpmbe);
+}
+
+int tpm_crb_pre_save(TPMCRBState *s)
+{
+ tpm_backend_finish_sync(s->tpmbe);
+
+ return 0;
+}
+
+void tpm_crb_reset(TPMCRBState *s, uint64_t baseaddr)
+{
+ if (s->ppi_enabled) {
+ tpm_ppi_reset(&s->ppi);
+ }
+ tpm_backend_reset(s->tpmbe);
+
+ memset(s->regs, 0, sizeof(s->regs));
+
+ ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
+ tpmRegValidSts, 1);
+ ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
+ tpmIdle, 1);
+ ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ InterfaceType, CRB_INTF_TYPE_CRB_ACTIVE);
+ ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ InterfaceVersion, CRB_INTF_VERSION_CRB);
+ ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ CapLocality, CRB_INTF_CAP_LOCALITY_0_ONLY);
+ ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ CapCRBIdleBypass, CRB_INTF_CAP_IDLE_FAST);
+ ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ CapDataXferSizeSupport, CRB_INTF_CAP_XFER_SIZE_64);
+ ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ CapFIFO, CRB_INTF_CAP_FIFO_NOT_SUPPORTED);
+ ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ CapCRB, CRB_INTF_CAP_CRB_SUPPORTED);
+ ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ InterfaceSelector, CRB_INTF_IF_SELECTOR_CRB);
+ ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ RID, 0b0000);
+ ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID2,
+ VID, PCI_VENDOR_ID_IBM);
+
+ baseaddr += A_CRB_DATA_BUFFER;
+ s->regs[R_CRB_CTRL_CMD_SIZE] = CRB_CTRL_CMD_SIZE;
+ s->regs[R_CRB_CTRL_CMD_LADDR] = (uint32_t)baseaddr;
+ s->regs[R_CRB_CTRL_CMD_HADDR] = (uint32_t)(baseaddr >> 32);
+ s->regs[R_CRB_CTRL_RSP_SIZE] = CRB_CTRL_CMD_SIZE;
+ s->regs[R_CRB_CTRL_RSP_ADDR] = (uint32_t)baseaddr;
+
+ s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->tpmbe),
+ CRB_CTRL_CMD_SIZE);
+
+ if (tpm_backend_startup_tpm(s->tpmbe, s->be_buffer_size) < 0) {
+ exit(1);
+ }
+}
+
+void tpm_crb_init_memory(Object *obj, TPMCRBState *s, Error **errp)
+{
+ memory_region_init_io(&s->mmio, obj, &tpm_crb_memory_ops, s,
+ "tpm-crb-mmio", sizeof(s->regs));
+ memory_region_init_ram(&s->cmdmem, obj,
+ "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
+}
diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
index 6968e60b3f..cb8204d5bc 100644
--- a/hw/tpm/meson.build
+++ b/hw/tpm/meson.build
@@ -3,6 +3,7 @@ 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_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'))
system_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_ppi.c'))
system_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_ppi.c'))
diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
index fa882dfefe..3ab1bdb97b 100644
--- a/hw/tpm/trace-events
+++ b/hw/tpm/trace-events
@@ -1,6 +1,6 @@
# See docs/devel/tracing.rst for syntax documentation.
-# tpm_crb.c
+# tpm_crb_common.c
tpm_crb_mmio_read(uint64_t addr, unsigned size, uint32_t val) "CRB read 0x%016" PRIx64 " len:%u val: 0x%" PRIx32
tpm_crb_mmio_write(uint64_t addr, unsigned size, uint32_t val) "CRB write 0x%016" PRIx64 " len:%u val: 0x%" PRIx32
--
2.39.2 (Apple Git-143)
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 02/11] tpm_crb: CTRL_RSP_ADDR is 64-bits wide
2023-07-13 3:51 [PATCH 00/11] tpm: introduce TPM CRB SysBus device Joelle van Dyne
2023-07-13 3:51 ` [PATCH 01/11] tpm_crb: refactor common code Joelle van Dyne
@ 2023-07-13 3:51 ` Joelle van Dyne
2023-07-13 15:31 ` Stefan Berger
2023-07-13 3:51 ` [PATCH 03/11] tpm_ppi: refactor memory space initialization Joelle van Dyne
` (9 subsequent siblings)
11 siblings, 1 reply; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-13 3:51 UTC (permalink / raw)
To: qemu-devel
Cc: Joelle van Dyne, Stefan Berger, Michael S. Tsirkin, Igor Mammedov,
Ani Sinha, Thomas Huth, Laurent Vivier, Paolo Bonzini
The register is actually 64-bits but in order to make this more clear
than the specification, we define two 32-bit registers:
CTRL_RSP_LADDR and CTRL_RSP_HADDR to match the CTRL_CMD_* naming. This
deviates from the specs but is way more clear.
Previously, the only CRB device uses a fixed system address so this
was not an issue. However, once we support SysBus CRB device, the
address can be anywhere in 64-bit space.
Signed-off-by: Joelle van Dyne <j@getutm.app>
---
include/hw/acpi/tpm.h | 3 ++-
hw/tpm/tpm_crb_common.c | 3 ++-
tests/qtest/tpm-crb-test.c | 2 +-
tests/qtest/tpm-util.c | 2 +-
4 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
index 579c45f5ba..f60bfe2789 100644
--- a/include/hw/acpi/tpm.h
+++ b/include/hw/acpi/tpm.h
@@ -174,7 +174,8 @@ REG32(CRB_CTRL_CMD_SIZE, 0x58)
REG32(CRB_CTRL_CMD_LADDR, 0x5C)
REG32(CRB_CTRL_CMD_HADDR, 0x60)
REG32(CRB_CTRL_RSP_SIZE, 0x64)
-REG32(CRB_CTRL_RSP_ADDR, 0x68)
+REG32(CRB_CTRL_RSP_LADDR, 0x68)
+REG32(CRB_CTRL_RSP_HADDR, 0x6C)
REG32(CRB_DATA_BUFFER, 0x80)
#define TPM_CRB_ADDR_BASE 0xFED40000
diff --git a/hw/tpm/tpm_crb_common.c b/hw/tpm/tpm_crb_common.c
index 4c173affb6..228e2d0faf 100644
--- a/hw/tpm/tpm_crb_common.c
+++ b/hw/tpm/tpm_crb_common.c
@@ -199,7 +199,8 @@ void tpm_crb_reset(TPMCRBState *s, uint64_t baseaddr)
s->regs[R_CRB_CTRL_CMD_LADDR] = (uint32_t)baseaddr;
s->regs[R_CRB_CTRL_CMD_HADDR] = (uint32_t)(baseaddr >> 32);
s->regs[R_CRB_CTRL_RSP_SIZE] = CRB_CTRL_CMD_SIZE;
- s->regs[R_CRB_CTRL_RSP_ADDR] = (uint32_t)baseaddr;
+ s->regs[R_CRB_CTRL_RSP_LADDR] = (uint32_t)baseaddr;
+ s->regs[R_CRB_CTRL_RSP_HADDR] = (uint32_t)(baseaddr >> 32);
s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->tpmbe),
CRB_CTRL_CMD_SIZE);
diff --git a/tests/qtest/tpm-crb-test.c b/tests/qtest/tpm-crb-test.c
index 396ae3f91c..9d30fe8293 100644
--- a/tests/qtest/tpm-crb-test.c
+++ b/tests/qtest/tpm-crb-test.c
@@ -28,7 +28,7 @@ static void tpm_crb_test(const void *data)
uint32_t csize = readl(TPM_CRB_ADDR_BASE + A_CRB_CTRL_CMD_SIZE);
uint64_t caddr = readq(TPM_CRB_ADDR_BASE + A_CRB_CTRL_CMD_LADDR);
uint32_t rsize = readl(TPM_CRB_ADDR_BASE + A_CRB_CTRL_RSP_SIZE);
- uint64_t raddr = readq(TPM_CRB_ADDR_BASE + A_CRB_CTRL_RSP_ADDR);
+ uint64_t raddr = readq(TPM_CRB_ADDR_BASE + A_CRB_CTRL_RSP_LADDR);
uint8_t locstate = readb(TPM_CRB_ADDR_BASE + A_CRB_LOC_STATE);
uint32_t locctrl = readl(TPM_CRB_ADDR_BASE + A_CRB_LOC_CTRL);
uint32_t locsts = readl(TPM_CRB_ADDR_BASE + A_CRB_LOC_STS);
diff --git a/tests/qtest/tpm-util.c b/tests/qtest/tpm-util.c
index 1c0319e6e7..dd02057fc0 100644
--- a/tests/qtest/tpm-util.c
+++ b/tests/qtest/tpm-util.c
@@ -25,7 +25,7 @@ void tpm_util_crb_transfer(QTestState *s,
unsigned char *rsp, size_t rsp_size)
{
uint64_t caddr = qtest_readq(s, TPM_CRB_ADDR_BASE + A_CRB_CTRL_CMD_LADDR);
- uint64_t raddr = qtest_readq(s, TPM_CRB_ADDR_BASE + A_CRB_CTRL_RSP_ADDR);
+ uint64_t raddr = qtest_readq(s, TPM_CRB_ADDR_BASE + A_CRB_CTRL_RSP_LADDR);
qtest_writeb(s, TPM_CRB_ADDR_BASE + A_CRB_LOC_CTRL, 1);
--
2.39.2 (Apple Git-143)
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 03/11] tpm_ppi: refactor memory space initialization
2023-07-13 3:51 [PATCH 00/11] tpm: introduce TPM CRB SysBus device Joelle van Dyne
2023-07-13 3:51 ` [PATCH 01/11] tpm_crb: refactor common code Joelle van Dyne
2023-07-13 3:51 ` [PATCH 02/11] tpm_crb: CTRL_RSP_ADDR is 64-bits wide Joelle van Dyne
@ 2023-07-13 3:51 ` Joelle van Dyne
2023-07-13 16:00 ` Stefan Berger
2023-07-13 3:51 ` [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping Joelle van Dyne
` (8 subsequent siblings)
11 siblings, 1 reply; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-13 3:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Joelle van Dyne, Stefan Berger
Instead of calling `memory_region_add_subregion` directly, we defer to
the caller to do it. This allows us to re-use the code for a SysBus
device.
Signed-off-by: Joelle van Dyne <j@getutm.app>
---
hw/tpm/tpm_ppi.h | 10 +++-------
hw/tpm/tpm_crb.c | 4 ++--
hw/tpm/tpm_crb_common.c | 3 +++
hw/tpm/tpm_ppi.c | 5 +----
hw/tpm/tpm_tis_isa.c | 5 +++--
5 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
index bf5d4a300f..30863c6438 100644
--- a/hw/tpm/tpm_ppi.h
+++ b/hw/tpm/tpm_ppi.h
@@ -20,17 +20,13 @@ typedef struct TPMPPI {
} TPMPPI;
/**
- * tpm_ppi_init:
+ * tpm_ppi_init_memory:
* @tpmppi: a TPMPPI
- * @m: the address-space / MemoryRegion to use
- * @addr: the address of the PPI region
* @obj: the owner object
*
- * Register the TPM PPI memory region at @addr on the given address
- * space for the object @obj.
+ * Creates the TPM PPI memory region.
**/
-void tpm_ppi_init(TPMPPI *tpmppi, MemoryRegion *m,
- hwaddr addr, Object *obj);
+void tpm_ppi_init_memory(TPMPPI *tpmppi, Object *obj);
/**
* tpm_ppi_reset:
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 3ef4977fb5..598c3e0161 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -107,8 +107,8 @@ static void tpm_crb_none_realize(DeviceState *dev, Error **errp)
TPM_CRB_ADDR_BASE + sizeof(s->state.regs), &s->state.cmdmem);
if (s->state.ppi_enabled) {
- tpm_ppi_init(&s->state.ppi, get_system_memory(),
- TPM_PPI_ADDR_BASE, OBJECT(s));
+ memory_region_add_subregion(get_system_memory(),
+ TPM_PPI_ADDR_BASE, &s->state.ppi.ram);
}
if (xen_enabled()) {
diff --git a/hw/tpm/tpm_crb_common.c b/hw/tpm/tpm_crb_common.c
index 228e2d0faf..e56e910670 100644
--- a/hw/tpm/tpm_crb_common.c
+++ b/hw/tpm/tpm_crb_common.c
@@ -216,4 +216,7 @@ void tpm_crb_init_memory(Object *obj, TPMCRBState *s, Error **errp)
"tpm-crb-mmio", sizeof(s->regs));
memory_region_init_ram(&s->cmdmem, obj,
"tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
+ if (s->ppi_enabled) {
+ tpm_ppi_init_memory(&s->ppi, obj);
+ }
}
diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
index 7f74e26ec6..40cab59afa 100644
--- a/hw/tpm/tpm_ppi.c
+++ b/hw/tpm/tpm_ppi.c
@@ -44,14 +44,11 @@ void tpm_ppi_reset(TPMPPI *tpmppi)
}
}
-void tpm_ppi_init(TPMPPI *tpmppi, MemoryRegion *m,
- hwaddr addr, Object *obj)
+void tpm_ppi_init_memory(TPMPPI *tpmppi, Object *obj)
{
tpmppi->buf = qemu_memalign(qemu_real_host_page_size(),
HOST_PAGE_ALIGN(TPM_PPI_ADDR_SIZE));
memory_region_init_ram_device_ptr(&tpmppi->ram, obj, "tpm-ppi",
TPM_PPI_ADDR_SIZE, tpmppi->buf);
vmstate_register_ram(&tpmppi->ram, DEVICE(obj));
-
- memory_region_add_subregion(m, addr, &tpmppi->ram);
}
diff --git a/hw/tpm/tpm_tis_isa.c b/hw/tpm/tpm_tis_isa.c
index 91e3792248..7cd7415f30 100644
--- a/hw/tpm/tpm_tis_isa.c
+++ b/hw/tpm/tpm_tis_isa.c
@@ -134,8 +134,9 @@ static void tpm_tis_isa_realizefn(DeviceState *dev, Error **errp)
TPM_TIS_ADDR_BASE, &s->mmio);
if (s->ppi_enabled) {
- tpm_ppi_init(&s->ppi, isa_address_space(ISA_DEVICE(dev)),
- TPM_PPI_ADDR_BASE, OBJECT(dev));
+ tpm_ppi_init_memory(&s->ppi, OBJECT(dev));
+ memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
+ TPM_PPI_ADDR_BASE, &s->ppi.ram);
}
}
--
2.39.2 (Apple Git-143)
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
2023-07-13 3:51 [PATCH 00/11] tpm: introduce TPM CRB SysBus device Joelle van Dyne
` (2 preceding siblings ...)
2023-07-13 3:51 ` [PATCH 03/11] tpm_ppi: refactor memory space initialization Joelle van Dyne
@ 2023-07-13 3:51 ` Joelle van Dyne
2023-07-13 14:17 ` Stefan Berger
2023-07-13 3:51 ` [PATCH 05/11] tpm_crb: use the ISA bus Joelle van Dyne
` (7 subsequent siblings)
11 siblings, 1 reply; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-13 3:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Joelle van Dyne, Stefan Berger
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 set when a
TPM2_HashStart operation is called which only exists for 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. As this bit is unused, we do not need to worry
about updating it for reads.
Signed-off-by: Joelle van Dyne <j@getutm.app>
---
hw/tpm/tpm_crb.h | 2 -
hw/tpm/tpm_crb.c | 3 -
hw/tpm/tpm_crb_common.c | 124 ++++++++++++++++++++--------------------
3 files changed, 63 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),
VMSTATE_END_OF_LIST(),
}
};
@@ -103,8 +102,6 @@ static void tpm_crb_none_realize(DeviceState *dev, Error **errp)
memory_region_add_subregion(get_system_memory(),
TPM_CRB_ADDR_BASE, &s->state.mmio);
- memory_region_add_subregion(get_system_memory(),
- TPM_CRB_ADDR_BASE + sizeof(s->state.regs), &s->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 e56e910670..f3e40095e3 100644
--- a/hw/tpm/tpm_crb_common.c
+++ b/hw/tpm/tpm_crb_common.c
@@ -33,31 +33,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 *)&s->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,
@@ -65,35 +46,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(&s->mmio);
+ mem = ®s[R_CRB_DATA_BUFFER];
+ assert(regs);
+
+ if (addr >= A_CRB_DATA_BUFFER) {
+ assert(addr + size <= TPM_CRB_ADDR_SIZE);
+ assert(size <= sizeof(val));
+ memcpy(mem + addr - A_CRB_DATA_BUFFER, &val, size);
+ memory_region_set_dirty(&s->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(&s->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),
@@ -110,26 +103,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;
case CRB_LOC_CTRL_REQUEST_ACCESS:
- ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
+ ARRAY_FIELD_DP32(regs, CRB_LOC_STS,
Granted, 1);
- ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
+ ARRAY_FIELD_DP32(regs, CRB_LOC_STS,
beenSeized, 0);
- ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
+ ARRAY_FIELD_DP32(regs, CRB_LOC_STATE,
locAssigned, 1);
break;
}
break;
}
+
+ memory_region_set_dirty(&s->mmio, 0, A_CRB_DATA_BUFFER);
}
const MemoryRegionOps tpm_crb_memory_ops = {
- .read = tpm_crb_mmio_read,
.write = tpm_crb_mmio_write,
.endianness = DEVICE_LITTLE_ENDIAN,
.valid = {
@@ -140,12 +134,16 @@ const MemoryRegionOps tpm_crb_memory_ops = {
void tpm_crb_request_completed(TPMCRBState *s, int ret)
{
- s->regs[R_CRB_CTRL_START] &= ~CRB_START_INVOKE;
+ uint32_t *regs = memory_region_get_ram_ptr(&s->mmio);
+
+ assert(regs);
+ regs[R_CRB_CTRL_START] &= ~CRB_START_INVOKE;
if (ret != 0) {
- ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
+ ARRAY_FIELD_DP32(regs, CRB_CTRL_STS,
tpmSts, 1); /* fatal error */
}
- memory_region_set_dirty(&s->cmdmem, 0, CRB_CTRL_CMD_SIZE);
+
+ memory_region_set_dirty(&s->mmio, 0, TPM_CRB_ADDR_SIZE);
}
enum TPMVersion tpm_crb_get_version(TPMCRBState *s)
@@ -162,45 +160,48 @@ int tpm_crb_pre_save(TPMCRBState *s)
void tpm_crb_reset(TPMCRBState *s, uint64_t baseaddr)
{
+ uint32_t *regs = memory_region_get_ram_ptr(&s->mmio);
+
+ assert(regs);
if (s->ppi_enabled) {
tpm_ppi_reset(&s->ppi);
}
tpm_backend_reset(s->tpmbe);
- memset(s->regs, 0, sizeof(s->regs));
+ memset(regs, 0, TPM_CRB_ADDR_SIZE);
- ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
+ ARRAY_FIELD_DP32(regs, CRB_LOC_STATE,
tpmRegValidSts, 1);
- ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
+ ARRAY_FIELD_DP32(regs, CRB_CTRL_STS,
tpmIdle, 1);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
InterfaceType, CRB_INTF_TYPE_CRB_ACTIVE);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
InterfaceVersion, CRB_INTF_VERSION_CRB);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
CapLocality, CRB_INTF_CAP_LOCALITY_0_ONLY);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
CapCRBIdleBypass, CRB_INTF_CAP_IDLE_FAST);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
CapDataXferSizeSupport, CRB_INTF_CAP_XFER_SIZE_64);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
CapFIFO, CRB_INTF_CAP_FIFO_NOT_SUPPORTED);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
CapCRB, CRB_INTF_CAP_CRB_SUPPORTED);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
InterfaceSelector, CRB_INTF_IF_SELECTOR_CRB);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
+ ARRAY_FIELD_DP32(regs, CRB_INTF_ID,
RID, 0b0000);
- ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID2,
+ ARRAY_FIELD_DP32(regs, CRB_INTF_ID2,
VID, PCI_VENDOR_ID_IBM);
baseaddr += A_CRB_DATA_BUFFER;
- s->regs[R_CRB_CTRL_CMD_SIZE] = CRB_CTRL_CMD_SIZE;
- s->regs[R_CRB_CTRL_CMD_LADDR] = (uint32_t)baseaddr;
- s->regs[R_CRB_CTRL_CMD_HADDR] = (uint32_t)(baseaddr >> 32);
- s->regs[R_CRB_CTRL_RSP_SIZE] = CRB_CTRL_CMD_SIZE;
- s->regs[R_CRB_CTRL_RSP_LADDR] = (uint32_t)baseaddr;
- s->regs[R_CRB_CTRL_RSP_HADDR] = (uint32_t)(baseaddr >> 32);
+ regs[R_CRB_CTRL_CMD_SIZE] = CRB_CTRL_CMD_SIZE;
+ regs[R_CRB_CTRL_CMD_LADDR] = (uint32_t)baseaddr;
+ regs[R_CRB_CTRL_CMD_HADDR] = (uint32_t)(baseaddr >> 32);
+ regs[R_CRB_CTRL_RSP_SIZE] = CRB_CTRL_CMD_SIZE;
+ regs[R_CRB_CTRL_RSP_LADDR] = (uint32_t)baseaddr;
+ regs[R_CRB_CTRL_RSP_HADDR] = (uint32_t)(baseaddr >> 32);
s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->tpmbe),
CRB_CTRL_CMD_SIZE);
@@ -208,14 +209,15 @@ void tpm_crb_reset(TPMCRBState *s, uint64_t baseaddr)
if (tpm_backend_startup_tpm(s->tpmbe, s->be_buffer_size) < 0) {
exit(1);
}
+
+ memory_region_rom_device_set_romd(&s->mmio, true);
+ memory_region_set_dirty(&s->mmio, 0, TPM_CRB_ADDR_SIZE);
}
void tpm_crb_init_memory(Object *obj, TPMCRBState *s, Error **errp)
{
- memory_region_init_io(&s->mmio, obj, &tpm_crb_memory_ops, s,
- "tpm-crb-mmio", sizeof(s->regs));
- memory_region_init_ram(&s->cmdmem, obj,
- "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
+ memory_region_init_rom_device(&s->mmio, obj, &tpm_crb_memory_ops, s,
+ "tpm-crb-mmio", TPM_CRB_ADDR_SIZE, errp);
if (s->ppi_enabled) {
tpm_ppi_init_memory(&s->ppi, obj);
}
--
2.39.2 (Apple Git-143)
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 05/11] tpm_crb: use the ISA bus
2023-07-13 3:51 [PATCH 00/11] tpm: introduce TPM CRB SysBus device Joelle van Dyne
` (3 preceding siblings ...)
2023-07-13 3:51 ` [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping Joelle van Dyne
@ 2023-07-13 3:51 ` Joelle van Dyne
2023-07-13 18:35 ` Stefan Berger
2023-07-13 3:51 ` [PATCH 06/11] tpm_crb: move ACPI table building to device interface Joelle van Dyne
` (6 subsequent siblings)
11 siblings, 1 reply; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-13 3:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Joelle van Dyne, Paolo Bonzini, Stefan Berger
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 <j@getutm.app>
---
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(&s->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(&s->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(&s->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(&s->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), &s->state, errp);
- memory_region_add_subregion(get_system_memory(),
+ memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
TPM_CRB_ADDR_BASE, &s->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, &s->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 = &vmstate_tpm_crb_none;
+ dc->realize = tpm_crb_isa_realize;
+ device_class_set_props(dc, tpm_crb_isa_properties);
+ dc->vmsd = &vmstate_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(&tpm_crb_none_info);
+ type_register_static(&tpm_crb_isa_info);
}
-type_init(tpm_crb_none_register)
+type_init(tpm_crb_isa_register)
diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
index a46663288c..1fd73fe617 100644
--- a/hw/tpm/Kconfig
+++ b/hw/tpm/Kconfig
@@ -22,7 +22,7 @@ config TPM_TIS
config TPM_CRB
bool
- depends on TPM && PC
+ depends on TPM && ISA_BUS
select TPM_BACKEND
config TPM_SPAPR
--
2.39.2 (Apple Git-143)
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 06/11] tpm_crb: move ACPI table building to device interface
2023-07-13 3:51 [PATCH 00/11] tpm: introduce TPM CRB SysBus device Joelle van Dyne
` (4 preceding siblings ...)
2023-07-13 3:51 ` [PATCH 05/11] tpm_crb: use the ISA bus Joelle van Dyne
@ 2023-07-13 3:51 ` Joelle van Dyne
2023-07-13 16:08 ` Stefan Berger
2023-07-13 3:51 ` [PATCH 07/11] hw/arm/virt: add plug handler for TPM on SysBus Joelle van Dyne
` (5 subsequent siblings)
11 siblings, 1 reply; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-13 3:51 UTC (permalink / raw)
To: qemu-devel
Cc: Joelle van Dyne, Michael S. Tsirkin, Igor Mammedov, Ani Sinha,
Paolo Bonzini, Richard Henderson, Eduardo Habkost,
Marcel Apfelbaum, Stefan Berger
This logic is similar to TPM TIS ISA device.
Signed-off-by: Joelle van Dyne <j@getutm.app>
---
hw/i386/acpi-build.c | 23 -----------------------
hw/tpm/tpm_crb.c | 28 ++++++++++++++++++++++++++++
2 files changed, 28 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..14feb9857f 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"
@@ -116,10 +118,34 @@ 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");
+ if (tpm_crb_isa_get_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(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 +154,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 +166,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 },
{ }
}
};
--
2.39.2 (Apple Git-143)
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 07/11] hw/arm/virt: add plug handler for TPM on SysBus
2023-07-13 3:51 [PATCH 00/11] tpm: introduce TPM CRB SysBus device Joelle van Dyne
` (5 preceding siblings ...)
2023-07-13 3:51 ` [PATCH 06/11] tpm_crb: move ACPI table building to device interface Joelle van Dyne
@ 2023-07-13 3:51 ` Joelle van Dyne
2023-07-13 13:13 ` Stefan Berger
2023-07-13 15:31 ` Peter Maydell
2023-07-13 3:51 ` [PATCH 08/11] hw/loongarch/virt: " Joelle van Dyne
` (4 subsequent siblings)
11 siblings, 2 replies; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-13 3:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Joelle van Dyne, Peter Maydell, open list:Virt
TPM needs to know its own base address in order to generate its DSDT
device entry.
Signed-off-by: Joelle van Dyne <j@getutm.app>
---
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, &error_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;
+ SysBusDevice *sbdev = SYS_BUS_DEVICE(tpmif);
+ MemoryRegion *sbdev_mr;
+ hwaddr tpm_base;
+ uint64_t tpm_size;
+
+ if (!sbdev || !object_dynamic_cast(OBJECT(sbdev), TYPE_SYS_BUS_DEVICE)) {
+ 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);
+ tpm_size = memory_region_size(sbdev_mr);
+
+ if (object_property_find(OBJECT(sbdev), "baseaddr")) {
+ object_property_set_uint(OBJECT(sbdev), "baseaddr", tpm_base, NULL);
+ }
+ if (object_property_find(OBJECT(sbdev), "size")) {
+ object_property_set_uint(OBJECT(sbdev), "size", tpm_size, NULL);
+ }
+}
+#endif
+
static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
@@ -2803,6 +2834,12 @@ 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)) {
+ virt_tpm_plug(vms, TPM_IF(dev));
+ }
+#endif
}
static void virt_dimm_unplug_request(HotplugHandler *hotplug_dev,
--
2.39.2 (Apple Git-143)
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 08/11] hw/loongarch/virt: add plug handler for TPM on SysBus
2023-07-13 3:51 [PATCH 00/11] tpm: introduce TPM CRB SysBus device Joelle van Dyne
` (6 preceding siblings ...)
2023-07-13 3:51 ` [PATCH 07/11] hw/arm/virt: add plug handler for TPM on SysBus Joelle van Dyne
@ 2023-07-13 3:51 ` Joelle van Dyne
2023-07-13 3:51 ` [PATCH 09/11] tpm_tis_sysbus: fix crash when PPI is enabled Joelle van Dyne
` (3 subsequent siblings)
11 siblings, 0 replies; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-13 3:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Joelle van Dyne, Xiaojuan Yang, Song Gao
TPM needs to know its own base address in order to generate its DSDT
device entry.
Signed-off-by: Joelle van Dyne <j@getutm.app>
---
hw/loongarch/virt.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index e19b042ce8..9c536c52bc 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -1040,6 +1040,37 @@ static void virt_mem_plug(HotplugHandler *hotplug_dev,
dev, &error_abort);
}
+#ifdef CONFIG_TPM
+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;
+ SysBusDevice *sbdev = SYS_BUS_DEVICE(tpmif);
+ MemoryRegion *sbdev_mr;
+ hwaddr tpm_base;
+ uint64_t tpm_size;
+
+ if (!sbdev || !object_dynamic_cast(OBJECT(sbdev), TYPE_SYS_BUS_DEVICE)) {
+ 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);
+ tpm_size = memory_region_size(sbdev_mr);
+
+ if (object_property_find(OBJECT(sbdev), "baseaddr")) {
+ object_property_set_uint(OBJECT(sbdev), "baseaddr", tpm_base, NULL);
+ }
+ if (object_property_find(OBJECT(sbdev), "size")) {
+ object_property_set_uint(OBJECT(sbdev), "size", tpm_size, NULL);
+ }
+}
+#endif
+
static void loongarch_machine_device_plug_cb(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
@@ -1054,6 +1085,12 @@ 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)) {
+ virt_tpm_plug(lams, TPM_IF(dev));
+ }
+#endif
}
static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
--
2.39.2 (Apple Git-143)
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 09/11] tpm_tis_sysbus: fix crash when PPI is enabled
2023-07-13 3:51 [PATCH 00/11] tpm: introduce TPM CRB SysBus device Joelle van Dyne
` (7 preceding siblings ...)
2023-07-13 3:51 ` [PATCH 08/11] hw/loongarch/virt: " Joelle van Dyne
@ 2023-07-13 3:51 ` Joelle van Dyne
2023-07-13 16:49 ` Stefan Berger
2023-07-13 3:51 ` [PATCH 10/11] tpm_tis_sysbus: move DSDT AML generation to device Joelle van Dyne
` (2 subsequent siblings)
11 siblings, 1 reply; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-13 3:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Joelle van Dyne, Stefan Berger
If 'ppi' property is set, then `tpm_ppi_reset` is called on reset
which SEGFAULTs because `tpmppi->buf` is not allocated.
Signed-off-by: Joelle van Dyne <j@getutm.app>
---
hw/tpm/tpm_tis_sysbus.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
index 45e63efd63..1014d5d993 100644
--- a/hw/tpm/tpm_tis_sysbus.c
+++ b/hw/tpm/tpm_tis_sysbus.c
@@ -124,6 +124,10 @@ static void tpm_tis_sysbus_realizefn(DeviceState *dev, Error **errp)
error_setg(errp, "'tpmdev' property is required");
return;
}
+
+ if (s->ppi_enabled) {
+ sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->ppi.ram);
+ }
}
static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data)
--
2.39.2 (Apple Git-143)
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 10/11] tpm_tis_sysbus: move DSDT AML generation to device
2023-07-13 3:51 [PATCH 00/11] tpm: introduce TPM CRB SysBus device Joelle van Dyne
` (8 preceding siblings ...)
2023-07-13 3:51 ` [PATCH 09/11] tpm_tis_sysbus: fix crash when PPI is enabled Joelle van Dyne
@ 2023-07-13 3:51 ` Joelle van Dyne
2023-07-13 3:51 ` [PATCH 11/11] tpm_crb_sysbus: introduce TPM CRB SysBus device Joelle van Dyne
2023-07-13 13:07 ` [PATCH 00/11] tpm: " Stefan Berger
11 siblings, 0 replies; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-13 3:51 UTC (permalink / raw)
To: qemu-devel
Cc: Joelle van Dyne, Michael S. Tsirkin, Igor Mammedov, Ani Sinha,
Peter Maydell, Shannon Zhao, Xiaojuan Yang, Song Gao,
Stefan Berger, open list:Virt
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
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 <j@getutm.app>
---
hw/arm/virt-acpi-build.c | 38 ++------------------------------------
hw/loongarch/acpi-build.c | 38 ++------------------------------------
hw/tpm/tpm_tis_sysbus.c | 39 +++++++++++++++++++++++++++++++++++++++
3 files changed, 43 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
-
/* 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 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 1014d5d993..a00f1a0105 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)
@@ -94,6 +97,8 @@ 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_UINT64("baseaddr", TPMStateSysBus, baseaddr, TPM_TIS_ADDR_BASE),
+ DEFINE_PROP_UINT64("size", TPMStateSysBus, size, TPM_TIS_ADDR_SIZE),
DEFINE_PROP_END_OF_LIST(),
};
@@ -130,10 +135,42 @@ 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(1)));
+ 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));
+ /**
+ * FIXME: PPI needs to also get a dynamic address.
+ */
+ /* tpm_build_ppi_acpi(ti, dev); */
+ 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 = &vmstate_tpm_tis_sysbus;
@@ -144,6 +181,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 = {
@@ -154,6 +192,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 },
{ }
}
};
--
2.39.2 (Apple Git-143)
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 11/11] tpm_crb_sysbus: introduce TPM CRB SysBus device
2023-07-13 3:51 [PATCH 00/11] tpm: introduce TPM CRB SysBus device Joelle van Dyne
` (9 preceding siblings ...)
2023-07-13 3:51 ` [PATCH 10/11] tpm_tis_sysbus: move DSDT AML generation to device Joelle van Dyne
@ 2023-07-13 3:51 ` Joelle van Dyne
2023-07-13 13:07 ` [PATCH 00/11] tpm: " Stefan Berger
11 siblings, 0 replies; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-13 3:51 UTC (permalink / raw)
To: qemu-devel
Cc: Joelle van Dyne, Stefan Berger, Michael S. Tsirkin, Igor Mammedov,
Ani Sinha, Paolo Bonzini, Peter Maydell, Xiaojuan Yang, Song Gao,
Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li,
Daniel Henrique Barboza, Liu Zhiwei, open list:ARM TCG CPUs,
open list:RISC-V TCG CPUs
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.
Signed-off-by: Joelle van Dyne <j@getutm.app>
---
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 | 178 ++++++++++++++++++++++++++++++++++++
hw/arm/Kconfig | 1 +
hw/riscv/Kconfig | 1 +
hw/tpm/Kconfig | 5 +
hw/tpm/meson.build | 2 +
13 files changed, 202 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/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;
+ start_method = TPM2_START_METHOD_CRB;
} else {
g_assert_not_reached();
}
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 432148ef47..88e8b16103 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2977,6 +2977,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_PLATFORM);
#ifdef CONFIG_TPM
machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
+ machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_CRB_SYSBUS);
#endif
mc->block_default_type = IF_VIRTIO;
mc->no_cdrom = 1;
diff --git a/hw/core/sysbus-fdt.c b/hw/core/sysbus-fdt.c
index eebcd28f9a..9c783f88eb 100644
--- a/hw/core/sysbus-fdt.c
+++ b/hw/core/sysbus-fdt.c
@@ -493,6 +493,7 @@ static const BindingEntry bindings[] = {
#endif
#ifdef CONFIG_TPM
TYPE_BINDING(TYPE_TPM_TIS_SYSBUS, add_tpm_tis_fdt_node),
+ TYPE_BINDING(TYPE_TPM_CRB_SYSBUS, no_fdt_node),
#endif
TYPE_BINDING(TYPE_RAMFB_DEVICE, no_fdt_node),
TYPE_BINDING("", NULL), /* last element */
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 9c536c52bc..eb59fb04ee 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -1194,6 +1194,7 @@ static void loongarch_class_init(ObjectClass *oc, void *data)
machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
#ifdef CONFIG_TPM
machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
+ machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_CRB_SYSBUS);
#endif
}
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index d90286dc46..5d639a870a 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1681,6 +1681,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
#ifdef CONFIG_TPM
machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
+ machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_CRB_SYSBUS);
#endif
if (tcg_enabled()) {
diff --git a/hw/tpm/tpm_crb_sysbus.c b/hw/tpm/tpm_crb_sysbus.c
new file mode 100644
index 0000000000..1289afcc7e
--- /dev/null
+++ b/hw/tpm/tpm_crb_sysbus.c
@@ -0,0 +1,178 @@
+/*
+ * tpm_crb_sysbus.c - QEMU's TPM CRB interface emulator
+ *
+ * Copyright (c) 2018 Red Hat, Inc.
+ *
+ * Authors:
+ * Marc-André Lureau <marcandre.lureau@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * tpm_crb is a device for TPM 2.0 Command Response Buffer (CRB) Interface
+ * as defined in TCG PC Client Platform TPM Profile (PTP) Specification
+ * Family “2.0” Level 00 Revision 01.03 v22
+ */
+
+#include "qemu/osdep.h"
+#include "hw/acpi/acpi_aml_interface.h"
+#include "hw/acpi/tpm.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+#include "tpm_prop.h"
+#include "hw/pci/pci_ids.h"
+#include "hw/sysbus.h"
+#include "qapi/visitor.h"
+#include "qom/object.h"
+#include "sysemu/tpm_util.h"
+#include "trace.h"
+#include "tpm_crb.h"
+
+struct TPMCRBStateSysBus {
+ /*< private >*/
+ SysBusDevice parent_obj;
+
+ /*< public >*/
+ TPMCRBState state;
+ uint64_t baseaddr;
+ uint64_t size;
+};
+
+OBJECT_DECLARE_SIMPLE_TYPE(TPMCRBStateSysBus, TPM_CRB_SYSBUS)
+
+static void tpm_crb_sysbus_request_completed(TPMIf *ti, int ret)
+{
+ TPMCRBStateSysBus *s = TPM_CRB_SYSBUS(ti);
+
+ return tpm_crb_request_completed(&s->state, ret);
+}
+
+static enum TPMVersion tpm_crb_sysbus_get_tpm_version(TPMIf *ti)
+{
+ TPMCRBStateSysBus *s = TPM_CRB_SYSBUS(ti);
+
+ return tpm_crb_get_version(&s->state);
+}
+
+static int tpm_crb_sysbus_pre_save(void *opaque)
+{
+ TPMCRBStateSysBus *s = opaque;
+
+ return tpm_crb_pre_save(&s->state);
+}
+
+static const VMStateDescription vmstate_tpm_crb_sysbus = {
+ .name = "tpm-crb-sysbus",
+ .pre_save = tpm_crb_sysbus_pre_save,
+ .fields = (VMStateField[]) {
+ VMSTATE_END_OF_LIST(),
+ }
+};
+
+static Property tpm_crb_sysbus_properties[] = {
+ DEFINE_PROP_TPMBE("tpmdev", TPMCRBStateSysBus, state.tpmbe),
+ DEFINE_PROP_BOOL("ppi", TPMCRBStateSysBus, state.ppi_enabled, false),
+ DEFINE_PROP_UINT64("baseaddr", TPMCRBStateSysBus,
+ baseaddr, TPM_CRB_ADDR_BASE),
+ DEFINE_PROP_UINT64("size", TPMCRBStateSysBus, size, TPM_CRB_ADDR_SIZE),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void tpm_crb_sysbus_initfn(Object *obj)
+{
+ TPMCRBStateSysBus *s = TPM_CRB_SYSBUS(obj);
+
+ tpm_crb_init_memory(obj, &s->state, NULL);
+
+ sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->state.mmio);
+}
+
+static void tpm_crb_sysbus_reset(DeviceState *dev)
+{
+ TPMCRBStateSysBus *s = TPM_CRB_SYSBUS(dev);
+
+ return tpm_crb_reset(&s->state, s->baseaddr);
+}
+
+static void tpm_crb_sysbus_realizefn(DeviceState *dev, Error **errp)
+{
+ TPMCRBStateSysBus *s = TPM_CRB_SYSBUS(dev);
+
+ if (!tpm_find()) {
+ error_setg(errp, "at most one TPM device is permitted");
+ return;
+ }
+
+ if (!s->state.tpmbe) {
+ error_setg(errp, "'tpmdev' property is required");
+ return;
+ }
+
+ if (s->state.ppi_enabled) {
+ sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->state.ppi.ram);
+ }
+}
+
+static void build_tpm_crb_sysbus_aml(AcpiDevAmlIf *adev, Aml *scope)
+{
+ Aml *dev, *crs;
+ TPMCRBStateSysBus *s = TPM_CRB_SYSBUS(adev);
+ TPMIf *ti = TPM_IF(s);
+
+ dev = aml_device("TPM");
+ if (tpm_crb_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(1)));
+ aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
+ crs = aml_resource_template();
+ aml_append(crs, aml_memory32_fixed(s->baseaddr, s->size,
+ AML_READ_WRITE));
+ aml_append(dev, aml_name_decl("_CRS", crs));
+ /**
+ * FIXME: PPI needs to also get a dynamic address.
+ */
+ /* tpm_build_ppi_acpi(ti, dev); */
+ aml_append(scope, dev);
+}
+
+static void tpm_crb_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_crb_sysbus_properties);
+ dc->vmsd = &vmstate_tpm_crb_sysbus;
+ tc->model = TPM_MODEL_TPM_CRB;
+ dc->realize = tpm_crb_sysbus_realizefn;
+ dc->user_creatable = true;
+ dc->reset = tpm_crb_sysbus_reset;
+ tc->request_completed = tpm_crb_sysbus_request_completed;
+ tc->get_version = tpm_crb_sysbus_get_tpm_version;
+ set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+ adevc->build_dev_aml = build_tpm_crb_sysbus_aml;
+}
+
+static const TypeInfo tpm_crb_sysbus_info = {
+ .name = TYPE_TPM_CRB_SYSBUS,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(TPMCRBStateSysBus),
+ .instance_init = tpm_crb_sysbus_initfn,
+ .class_init = tpm_crb_sysbus_class_init,
+ .interfaces = (InterfaceInfo[]) {
+ { TYPE_TPM_IF },
+ { TYPE_ACPI_DEV_AML_IF },
+ { }
+ }
+};
+
+static void tpm_crb_sysbus_register(void)
+{
+ type_register_static(&tpm_crb_sysbus_info);
+}
+
+type_init(tpm_crb_sysbus_register)
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 7e68348440..efe1beaa7b 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -5,6 +5,7 @@ config ARM_VIRT
imply VFIO_AMD_XGBE
imply VFIO_PLATFORM
imply VFIO_XGMAC
+ imply TPM_CRB_SYSBUS
imply TPM_TIS_SYSBUS
imply TPM_TIS_I2C
imply NVDIMM
diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
index b6a5eb4452..d824cb58f9 100644
--- a/hw/riscv/Kconfig
+++ b/hw/riscv/Kconfig
@@ -29,6 +29,7 @@ config RISCV_VIRT
imply PCI_DEVICES
imply VIRTIO_VGA
imply TEST_DEVICES
+ imply TPM_CRB_SYSBUS
imply TPM_TIS_SYSBUS
select RISCV_NUMA
select GOLDFISH_RTC
diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
index 1fd73fe617..3f294a20ba 100644
--- a/hw/tpm/Kconfig
+++ b/hw/tpm/Kconfig
@@ -25,6 +25,11 @@ config TPM_CRB
depends on TPM && ISA_BUS
select TPM_BACKEND
+config TPM_CRB_SYSBUS
+ bool
+ depends on TPM
+ select TPM_BACKEND
+
config TPM_SPAPR
bool
default y
diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
index cb8204d5bc..d96de92c16 100644
--- a/hw/tpm/meson.build
+++ b/hw/tpm/meson.build
@@ -4,6 +4,8 @@ system_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: files('tpm_tis_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'))
+system_ss.add(when: 'CONFIG_TPM_CRB_SYSBUS', if_true: files('tpm_crb_sysbus.c'))
+system_ss.add(when: 'CONFIG_TPM_CRB_SYSBUS', if_true: files('tpm_crb_common.c'))
system_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_ppi.c'))
system_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_ppi.c'))
--
2.39.2 (Apple Git-143)
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 00/11] tpm: introduce TPM CRB SysBus device
2023-07-13 3:51 [PATCH 00/11] tpm: introduce TPM CRB SysBus device Joelle van Dyne
` (10 preceding siblings ...)
2023-07-13 3:51 ` [PATCH 11/11] tpm_crb_sysbus: introduce TPM CRB SysBus device Joelle van Dyne
@ 2023-07-13 13:07 ` Stefan Berger
2023-07-13 17:35 ` Joelle van Dyne
11 siblings, 1 reply; 41+ messages in thread
From: Stefan Berger @ 2023-07-13 13:07 UTC (permalink / raw)
To: Joelle van Dyne, qemu-devel
On 7/12/23 23:51, 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.
Thanks a lot for this work. The last sentence seems to hint at the current issue
with TPM CRB on ARM64 and seems to be the only way forward there. You may want
to reformulate it a bit because it's not clear how the 'however' related to
CRB relates to TIS.
>
> 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
Please reorder bugs to the beginning of the series or submit in an extra patch set
so we can backport them. Ideal would be description(s) for how to trigger the bug(s).
> 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
s/redundent/redundant
> 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 caused page faults. @agraf suggested that we do this to avoid having to
Afaik, LDP is an ARM assembly instruction that loads two 32bit or 64bit registers from
consecutive addresses. May be worth mentioning for those wondering about it...
> do AARCH64 decoding in the HVF fault handler.
What is HVF?
Regards,
Stefan
>
> 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.
>
> Joelle van Dyne (11):
> 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: use the ISA bus
> tpm_crb: move ACPI table building to device interface
> hw/arm/virt: add plug handler for TPM on SysBus
> hw/loongarch/virt: add plug handler for TPM on SysBus
> tpm_tis_sysbus: fix crash when PPI is enabled
> tpm_tis_sysbus: move DSDT AML generation to device
> tpm_crb_sysbus: introduce TPM CRB SysBus device
>
> docs/specs/tpm.rst | 2 +
> hw/tpm/tpm_crb.h | 74 +++++++++
> hw/tpm/tpm_ppi.h | 10 +-
> include/hw/acpi/aml-build.h | 1 +
> include/hw/acpi/tpm.h | 3 +-
> include/sysemu/tpm.h | 3 +
> hw/acpi/aml-build.c | 7 +-
> hw/arm/virt-acpi-build.c | 38 +----
> hw/arm/virt.c | 38 +++++
> hw/core/sysbus-fdt.c | 1 +
> hw/i386/acpi-build.c | 23 ---
> hw/loongarch/acpi-build.c | 38 +----
> hw/loongarch/virt.c | 38 +++++
> hw/riscv/virt.c | 1 +
> hw/tpm/tpm_crb.c | 307 ++++++++----------------------------
> hw/tpm/tpm_crb_common.c | 224 ++++++++++++++++++++++++++
> hw/tpm/tpm_crb_sysbus.c | 178 +++++++++++++++++++++
> hw/tpm/tpm_ppi.c | 5 +-
> hw/tpm/tpm_tis_isa.c | 5 +-
> hw/tpm/tpm_tis_sysbus.c | 43 +++++
> tests/qtest/tpm-crb-test.c | 2 +-
> tests/qtest/tpm-util.c | 2 +-
> hw/arm/Kconfig | 1 +
> hw/riscv/Kconfig | 1 +
> hw/tpm/Kconfig | 7 +-
> hw/tpm/meson.build | 3 +
> hw/tpm/trace-events | 2 +-
> 27 files changed, 703 insertions(+), 354 deletions(-)
> create mode 100644 hw/tpm/tpm_crb.h
> create mode 100644 hw/tpm/tpm_crb_common.c
> create mode 100644 hw/tpm/tpm_crb_sysbus.c
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 07/11] hw/arm/virt: add plug handler for TPM on SysBus
2023-07-13 3:51 ` [PATCH 07/11] hw/arm/virt: add plug handler for TPM on SysBus Joelle van Dyne
@ 2023-07-13 13:13 ` Stefan Berger
2023-07-13 15:31 ` Peter Maydell
1 sibling, 0 replies; 41+ messages in thread
From: Stefan Berger @ 2023-07-13 13:13 UTC (permalink / raw)
To: Joelle van Dyne, qemu-devel; +Cc: Peter Maydell, open list:Virt
On 7/12/23 23:51, Joelle van Dyne wrote:
> TPM needs to know its own base address in order to generate its DSDT
> device entry.
This and the loongarch patch seem to have largely identical virt_tpm_plug functions.
Could they be consolidated in hw/tpm/virt.c ?
Stefan
>
> Signed-off-by: Joelle van Dyne <j@getutm.app>
> ---
> 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, &error_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;
> + SysBusDevice *sbdev = SYS_BUS_DEVICE(tpmif);
> + MemoryRegion *sbdev_mr;
> + hwaddr tpm_base;
> + uint64_t tpm_size;
> +
> + if (!sbdev || !object_dynamic_cast(OBJECT(sbdev), TYPE_SYS_BUS_DEVICE)) {
> + 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);
> + tpm_size = memory_region_size(sbdev_mr);
> +
> + if (object_property_find(OBJECT(sbdev), "baseaddr")) {
> + object_property_set_uint(OBJECT(sbdev), "baseaddr", tpm_base, NULL);
> + }
> + if (object_property_find(OBJECT(sbdev), "size")) {
> + object_property_set_uint(OBJECT(sbdev), "size", tpm_size, NULL);
> + }
> +}
> +#endif
> +
> static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> @@ -2803,6 +2834,12 @@ 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)) {
> + virt_tpm_plug(vms, TPM_IF(dev));
> + }
> +#endif
> }
>
> static void virt_dimm_unplug_request(HotplugHandler *hotplug_dev,
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 01/11] tpm_crb: refactor common code
2023-07-13 3:51 ` [PATCH 01/11] tpm_crb: refactor common code Joelle van Dyne
@ 2023-07-13 13:22 ` Stefan Berger
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Berger @ 2023-07-13 13:22 UTC (permalink / raw)
To: Joelle van Dyne, qemu-devel; +Cc: Stefan Berger
On 7/12/23 23:51, Joelle van Dyne wrote:
> In preparation for the SysBus variant, we move common code styled
> after the TPM TIS devices.
>
> To maintain compatibility, we do not rename the existing tpm-crb
> device.
>
> Signed-off-by: Joelle van Dyne <j@getutm.app>
> ---
> docs/specs/tpm.rst | 1 +
> hw/tpm/tpm_crb.h | 76 +++++++++++
> hw/tpm/tpm_crb.c | 270 ++++++----------------------------------
> hw/tpm/tpm_crb_common.c | 218 ++++++++++++++++++++++++++++++++
> hw/tpm/meson.build | 1 +
> hw/tpm/trace-events | 2 +-
> 6 files changed, 333 insertions(+), 235 deletions(-)
> create mode 100644 hw/tpm/tpm_crb.h
> create mode 100644 hw/tpm/tpm_crb_common.c
>
> diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst
> index efe124a148..2bc29c9804 100644
> --- a/docs/specs/tpm.rst
> +++ b/docs/specs/tpm.rst
> @@ -45,6 +45,7 @@ operating system.
>
> QEMU files related to TPM CRB interface:
> - ``hw/tpm/tpm_crb.c``
> + - ``hw/tpm/tpm_crb_common.c``
If you could add the command line to use for Windows on AARCH64 to this document in 11/11
that would be helpful because what is there right now ony works for Linux iirc.
Regarding this patch here:
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>
> SPAPR interface
> ---------------
> diff --git a/hw/tpm/tpm_crb.h b/hw/tpm/tpm_crb.h
> new file mode 100644
> index 0000000000..da3a0cf256
> --- /dev/null
> +++ b/hw/tpm/tpm_crb.h
> @@ -0,0 +1,76 @@
> +/*
> + * tpm_crb.h - QEMU's TPM CRB interface emulator
> + *
> + * Copyright (c) 2018 Red Hat, Inc.
> + *
> + * Authors:
> + * Marc-André Lureau <marcandre.lureau@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + * tpm_crb is a device for TPM 2.0 Command Response Buffer (CRB) Interface
> + * as defined in TCG PC Client Platform TPM Profile (PTP) Specification
> + * Family “2.0” Level 00 Revision 01.03 v22
> + */
> +#ifndef TPM_TPM_CRB_H
> +#define TPM_TPM_CRB_H
> +
> +#include "exec/memory.h"
> +#include "hw/acpi/tpm.h"
> +#include "sysemu/tpm_backend.h"
> +#include "tpm_ppi.h"
> +
> +#define CRB_CTRL_CMD_SIZE (TPM_CRB_ADDR_SIZE - A_CRB_DATA_BUFFER)
> +
> +typedef struct TPMCRBState {
> + TPMBackend *tpmbe;
> + TPMBackendCmd cmd;
> + uint32_t regs[TPM_CRB_R_MAX];
> + MemoryRegion mmio;
> + MemoryRegion cmdmem;
> +
> + size_t be_buffer_size;
> +
> + bool ppi_enabled;
> + TPMPPI ppi;
> +} TPMCRBState;
> +
> +#define CRB_INTF_TYPE_CRB_ACTIVE 0b1
> +#define CRB_INTF_VERSION_CRB 0b1
> +#define CRB_INTF_CAP_LOCALITY_0_ONLY 0b0
> +#define CRB_INTF_CAP_IDLE_FAST 0b0
> +#define CRB_INTF_CAP_XFER_SIZE_64 0b11
> +#define CRB_INTF_CAP_FIFO_NOT_SUPPORTED 0b0
> +#define CRB_INTF_CAP_CRB_SUPPORTED 0b1
> +#define CRB_INTF_IF_SELECTOR_CRB 0b1
> +
> +enum crb_loc_ctrl {
> + CRB_LOC_CTRL_REQUEST_ACCESS = BIT(0),
> + CRB_LOC_CTRL_RELINQUISH = BIT(1),
> + CRB_LOC_CTRL_SEIZE = BIT(2),
> + CRB_LOC_CTRL_RESET_ESTABLISHMENT_BIT = BIT(3),
> +};
> +
> +enum crb_ctrl_req {
> + CRB_CTRL_REQ_CMD_READY = BIT(0),
> + CRB_CTRL_REQ_GO_IDLE = BIT(1),
> +};
> +
> +enum crb_start {
> + CRB_START_INVOKE = BIT(0),
> +};
> +
> +enum crb_cancel {
> + CRB_CANCEL_INVOKE = BIT(0),
> +};
> +
> +#define TPM_CRB_NO_LOCALITY 0xff
> +
> +void tpm_crb_request_completed(TPMCRBState *s, int ret);
> +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);
> +
> +#endif /* TPM_TPM_CRB_H */
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index ea930da545..3ef4977fb5 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -31,257 +31,62 @@
> #include "tpm_ppi.h"
> #include "trace.h"
> #include "qom/object.h"
> +#include "tpm_crb.h"
>
> struct CRBState {
> DeviceState parent_obj;
>
> - TPMBackend *tpmbe;
> - TPMBackendCmd cmd;
> - uint32_t regs[TPM_CRB_R_MAX];
> - MemoryRegion mmio;
> - MemoryRegion cmdmem;
> -
> - size_t be_buffer_size;
> -
> - bool ppi_enabled;
> - TPMPPI ppi;
> + TPMCRBState state;
> };
> typedef struct CRBState CRBState;
>
> DECLARE_INSTANCE_CHECKER(CRBState, CRB,
> TYPE_TPM_CRB)
>
> -#define CRB_INTF_TYPE_CRB_ACTIVE 0b1
> -#define CRB_INTF_VERSION_CRB 0b1
> -#define CRB_INTF_CAP_LOCALITY_0_ONLY 0b0
> -#define CRB_INTF_CAP_IDLE_FAST 0b0
> -#define CRB_INTF_CAP_XFER_SIZE_64 0b11
> -#define CRB_INTF_CAP_FIFO_NOT_SUPPORTED 0b0
> -#define CRB_INTF_CAP_CRB_SUPPORTED 0b1
> -#define CRB_INTF_IF_SELECTOR_CRB 0b1
> -
> -#define CRB_CTRL_CMD_SIZE (TPM_CRB_ADDR_SIZE - A_CRB_DATA_BUFFER)
> -
> -enum crb_loc_ctrl {
> - CRB_LOC_CTRL_REQUEST_ACCESS = BIT(0),
> - CRB_LOC_CTRL_RELINQUISH = BIT(1),
> - CRB_LOC_CTRL_SEIZE = BIT(2),
> - CRB_LOC_CTRL_RESET_ESTABLISHMENT_BIT = BIT(3),
> -};
> -
> -enum crb_ctrl_req {
> - CRB_CTRL_REQ_CMD_READY = BIT(0),
> - CRB_CTRL_REQ_GO_IDLE = BIT(1),
> -};
> -
> -enum crb_start {
> - CRB_START_INVOKE = BIT(0),
> -};
> -
> -enum crb_cancel {
> - CRB_CANCEL_INVOKE = BIT(0),
> -};
> -
> -#define TPM_CRB_NO_LOCALITY 0xff
> -
> -static uint64_t tpm_crb_mmio_read(void *opaque, hwaddr addr,
> - unsigned size)
> -{
> - CRBState *s = CRB(opaque);
> - void *regs = (void *)&s->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(CRBState *s)
> -{
> - if (!ARRAY_FIELD_EX32(s->regs, CRB_LOC_STATE, locAssigned)) {
> - return TPM_CRB_NO_LOCALITY;
> - }
> - return ARRAY_FIELD_EX32(s->regs, CRB_LOC_STATE, activeLocality);
> -}
> -
> -static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
> - uint64_t val, unsigned size)
> -{
> - CRBState *s = CRB(opaque);
> - uint8_t locty = addr >> 12;
> -
> - trace_tpm_crb_mmio_write(addr, size, val);
> -
> - switch (addr) {
> - case A_CRB_CTRL_REQ:
> - switch (val) {
> - case CRB_CTRL_REQ_CMD_READY:
> - ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
> - tpmIdle, 0);
> - break;
> - case CRB_CTRL_REQ_GO_IDLE:
> - ARRAY_FIELD_DP32(s->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) {
> - 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(&s->cmdmem);
> -
> - s->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),
> - .out = mem,
> - .out_len = s->be_buffer_size,
> - };
> -
> - tpm_backend_deliver_request(s->tpmbe, &s->cmd);
> - }
> - break;
> - case A_CRB_LOC_CTRL:
> - switch (val) {
> - case CRB_LOC_CTRL_RESET_ESTABLISHMENT_BIT:
> - /* not loc 3 or 4 */
> - break;
> - case CRB_LOC_CTRL_RELINQUISH:
> - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
> - locAssigned, 0);
> - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
> - Granted, 0);
> - break;
> - case CRB_LOC_CTRL_REQUEST_ACCESS:
> - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
> - Granted, 1);
> - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
> - beenSeized, 0);
> - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
> - locAssigned, 1);
> - break;
> - }
> - break;
> - }
> -}
> -
> -static 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,
> - },
> -};
> -
> -static void tpm_crb_request_completed(TPMIf *ti, int ret)
> +static void tpm_crb_none_request_completed(TPMIf *ti, int ret)
> {
> CRBState *s = CRB(ti);
>
> - s->regs[R_CRB_CTRL_START] &= ~CRB_START_INVOKE;
> - if (ret != 0) {
> - ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
> - tpmSts, 1); /* fatal error */
> - }
> - memory_region_set_dirty(&s->cmdmem, 0, CRB_CTRL_CMD_SIZE);
> + tpm_crb_request_completed(&s->state, ret);
> }
>
> -static enum TPMVersion tpm_crb_get_version(TPMIf *ti)
> +static enum TPMVersion tpm_crb_none_get_version(TPMIf *ti)
> {
> CRBState *s = CRB(ti);
>
> - return tpm_backend_get_tpm_version(s->tpmbe);
> + return tpm_crb_get_version(&s->state);
> }
>
> -static int tpm_crb_pre_save(void *opaque)
> +static int tpm_crb_none_pre_save(void *opaque)
> {
> CRBState *s = opaque;
>
> - tpm_backend_finish_sync(s->tpmbe);
> -
> - return 0;
> + return tpm_crb_pre_save(&s->state);
> }
>
> -static const VMStateDescription vmstate_tpm_crb = {
> +static const VMStateDescription vmstate_tpm_crb_none = {
> .name = "tpm-crb",
> - .pre_save = tpm_crb_pre_save,
> + .pre_save = tpm_crb_none_pre_save,
> .fields = (VMStateField[]) {
> - VMSTATE_UINT32_ARRAY(regs, CRBState, TPM_CRB_R_MAX),
> + VMSTATE_UINT32_ARRAY(state.regs, CRBState, TPM_CRB_R_MAX),
> VMSTATE_END_OF_LIST(),
> }
> };
>
> -static Property tpm_crb_properties[] = {
> - DEFINE_PROP_TPMBE("tpmdev", CRBState, tpmbe),
> - DEFINE_PROP_BOOL("ppi", CRBState, ppi_enabled, true),
> +static Property tpm_crb_none_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_reset(void *dev)
> +static void tpm_crb_none_reset(void *dev)
> {
> CRBState *s = CRB(dev);
>
> - if (s->ppi_enabled) {
> - tpm_ppi_reset(&s->ppi);
> - }
> - tpm_backend_reset(s->tpmbe);
> -
> - memset(s->regs, 0, sizeof(s->regs));
> -
> - ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
> - tpmRegValidSts, 1);
> - ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
> - tpmIdle, 1);
> - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> - InterfaceType, CRB_INTF_TYPE_CRB_ACTIVE);
> - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> - InterfaceVersion, CRB_INTF_VERSION_CRB);
> - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> - CapLocality, CRB_INTF_CAP_LOCALITY_0_ONLY);
> - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> - CapCRBIdleBypass, CRB_INTF_CAP_IDLE_FAST);
> - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> - CapDataXferSizeSupport, CRB_INTF_CAP_XFER_SIZE_64);
> - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> - CapFIFO, CRB_INTF_CAP_FIFO_NOT_SUPPORTED);
> - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> - CapCRB, CRB_INTF_CAP_CRB_SUPPORTED);
> - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> - InterfaceSelector, CRB_INTF_IF_SELECTOR_CRB);
> - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> - RID, 0b0000);
> - ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID2,
> - VID, PCI_VENDOR_ID_IBM);
> -
> - s->regs[R_CRB_CTRL_CMD_SIZE] = CRB_CTRL_CMD_SIZE;
> - s->regs[R_CRB_CTRL_CMD_LADDR] = TPM_CRB_ADDR_BASE + A_CRB_DATA_BUFFER;
> - s->regs[R_CRB_CTRL_RSP_SIZE] = CRB_CTRL_CMD_SIZE;
> - s->regs[R_CRB_CTRL_RSP_ADDR] = TPM_CRB_ADDR_BASE + A_CRB_DATA_BUFFER;
> -
> - s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->tpmbe),
> - CRB_CTRL_CMD_SIZE);
> -
> - if (tpm_backend_startup_tpm(s->tpmbe, s->be_buffer_size) < 0) {
> - exit(1);
> - }
> + return tpm_crb_reset(&s->state, TPM_CRB_ADDR_BASE);
> }
>
> -static void tpm_crb_realize(DeviceState *dev, Error **errp)
> +static void tpm_crb_none_realize(DeviceState *dev, Error **errp)
> {
> CRBState *s = CRB(dev);
>
> @@ -289,64 +94,61 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
> error_setg(errp, "at most one TPM device is permitted");
> return;
> }
> - if (!s->tpmbe) {
> + if (!s->state.tpmbe) {
> error_setg(errp, "'tpmdev' property is required");
> return;
> }
>
> - memory_region_init_io(&s->mmio, OBJECT(s), &tpm_crb_memory_ops, s,
> - "tpm-crb-mmio", sizeof(s->regs));
> - memory_region_init_ram(&s->cmdmem, OBJECT(s),
> - "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
> + tpm_crb_init_memory(OBJECT(s), &s->state, errp);
>
> memory_region_add_subregion(get_system_memory(),
> - TPM_CRB_ADDR_BASE, &s->mmio);
> + TPM_CRB_ADDR_BASE, &s->state.mmio);
> memory_region_add_subregion(get_system_memory(),
> - TPM_CRB_ADDR_BASE + sizeof(s->regs), &s->cmdmem);
> + TPM_CRB_ADDR_BASE + sizeof(s->state.regs), &s->state.cmdmem);
>
> - if (s->ppi_enabled) {
> - tpm_ppi_init(&s->ppi, get_system_memory(),
> + if (s->state.ppi_enabled) {
> + tpm_ppi_init(&s->state.ppi, get_system_memory(),
> TPM_PPI_ADDR_BASE, OBJECT(s));
> }
>
> if (xen_enabled()) {
> - tpm_crb_reset(dev);
> + tpm_crb_none_reset(dev);
> } else {
> - qemu_register_reset(tpm_crb_reset, dev);
> + qemu_register_reset(tpm_crb_none_reset, dev);
> }
> }
>
> -static void tpm_crb_class_init(ObjectClass *klass, void *data)
> +static void tpm_crb_none_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> TPMIfClass *tc = TPM_IF_CLASS(klass);
>
> - dc->realize = tpm_crb_realize;
> - device_class_set_props(dc, tpm_crb_properties);
> - dc->vmsd = &vmstate_tpm_crb;
> + dc->realize = tpm_crb_none_realize;
> + device_class_set_props(dc, tpm_crb_none_properties);
> + dc->vmsd = &vmstate_tpm_crb_none;
> dc->user_creatable = true;
> tc->model = TPM_MODEL_TPM_CRB;
> - tc->get_version = tpm_crb_get_version;
> - tc->request_completed = tpm_crb_request_completed;
> + tc->get_version = tpm_crb_none_get_version;
> + tc->request_completed = tpm_crb_none_request_completed;
>
> set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> }
>
> -static const TypeInfo tpm_crb_info = {
> +static const TypeInfo tpm_crb_none_info = {
> .name = TYPE_TPM_CRB,
> /* could be TYPE_SYS_BUS_DEVICE (or LPC etc) */
> .parent = TYPE_DEVICE,
> .instance_size = sizeof(CRBState),
> - .class_init = tpm_crb_class_init,
> + .class_init = tpm_crb_none_class_init,
> .interfaces = (InterfaceInfo[]) {
> { TYPE_TPM_IF },
> { }
> }
> };
>
> -static void tpm_crb_register(void)
> +static void tpm_crb_none_register(void)
> {
> - type_register_static(&tpm_crb_info);
> + type_register_static(&tpm_crb_none_info);
> }
>
> -type_init(tpm_crb_register)
> +type_init(tpm_crb_none_register)
> diff --git a/hw/tpm/tpm_crb_common.c b/hw/tpm/tpm_crb_common.c
> new file mode 100644
> index 0000000000..4c173affb6
> --- /dev/null
> +++ b/hw/tpm/tpm_crb_common.c
> @@ -0,0 +1,218 @@
> +/*
> + * tpm_crb.c - QEMU's TPM CRB interface emulator
> + *
> + * Copyright (c) 2018 Red Hat, Inc.
> + *
> + * Authors:
> + * Marc-André Lureau <marcandre.lureau@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + * tpm_crb is a device for TPM 2.0 Command Response Buffer (CRB) Interface
> + * as defined in TCG PC Client Platform TPM Profile (PTP) Specification
> + * Family “2.0” Level 00 Revision 01.03 v22
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "qemu/module.h"
> +#include "qapi/error.h"
> +#include "exec/address-spaces.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/pci/pci_ids.h"
> +#include "hw/acpi/tpm.h"
> +#include "migration/vmstate.h"
> +#include "sysemu/tpm_backend.h"
> +#include "sysemu/tpm_util.h"
> +#include "sysemu/reset.h"
> +#include "sysemu/xen.h"
> +#include "tpm_prop.h"
> +#include "tpm_ppi.h"
> +#include "trace.h"
> +#include "qom/object.h"
> +#include "tpm_crb.h"
> +
> +static uint64_t tpm_crb_mmio_read(void *opaque, hwaddr addr,
> + unsigned size)
> +{
> + TPMCRBState *s = opaque;
> + void *regs = (void *)&s->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)) {
> + return TPM_CRB_NO_LOCALITY;
> + }
> + return ARRAY_FIELD_EX32(s->regs, CRB_LOC_STATE, activeLocality);
> +}
> +
> +static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
> + uint64_t val, unsigned size)
> +{
> + TPMCRBState *s = opaque;
> + uint8_t locty = addr >> 12;
> +
> + trace_tpm_crb_mmio_write(addr, size, val);
> +
> + switch (addr) {
> + case A_CRB_CTRL_REQ:
> + switch (val) {
> + case CRB_CTRL_REQ_CMD_READY:
> + ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
> + tpmIdle, 0);
> + break;
> + case CRB_CTRL_REQ_GO_IDLE:
> + ARRAY_FIELD_DP32(s->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) {
> + 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(&s->cmdmem);
> +
> + s->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),
> + .out = mem,
> + .out_len = s->be_buffer_size,
> + };
> +
> + tpm_backend_deliver_request(s->tpmbe, &s->cmd);
> + }
> + break;
> + case A_CRB_LOC_CTRL:
> + switch (val) {
> + case CRB_LOC_CTRL_RESET_ESTABLISHMENT_BIT:
> + /* not loc 3 or 4 */
> + break;
> + case CRB_LOC_CTRL_RELINQUISH:
> + ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
> + locAssigned, 0);
> + ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
> + Granted, 0);
> + break;
> + case CRB_LOC_CTRL_REQUEST_ACCESS:
> + ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
> + Granted, 1);
> + ARRAY_FIELD_DP32(s->regs, CRB_LOC_STS,
> + beenSeized, 0);
> + ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
> + locAssigned, 1);
> + break;
> + }
> + break;
> + }
> +}
> +
> +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,
> + },
> +};
> +
> +void tpm_crb_request_completed(TPMCRBState *s, int ret)
> +{
> + s->regs[R_CRB_CTRL_START] &= ~CRB_START_INVOKE;
> + if (ret != 0) {
> + ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
> + tpmSts, 1); /* fatal error */
> + }
> + memory_region_set_dirty(&s->cmdmem, 0, CRB_CTRL_CMD_SIZE);
> +}
> +
> +enum TPMVersion tpm_crb_get_version(TPMCRBState *s)
> +{
> + return tpm_backend_get_tpm_version(s->tpmbe);
> +}
> +
> +int tpm_crb_pre_save(TPMCRBState *s)
> +{
> + tpm_backend_finish_sync(s->tpmbe);
> +
> + return 0;
> +}
> +
> +void tpm_crb_reset(TPMCRBState *s, uint64_t baseaddr)
> +{
> + if (s->ppi_enabled) {
> + tpm_ppi_reset(&s->ppi);
> + }
> + tpm_backend_reset(s->tpmbe);
> +
> + memset(s->regs, 0, sizeof(s->regs));
> +
> + ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
> + tpmRegValidSts, 1);
> + ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
> + tpmIdle, 1);
> + ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> + InterfaceType, CRB_INTF_TYPE_CRB_ACTIVE);
> + ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> + InterfaceVersion, CRB_INTF_VERSION_CRB);
> + ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> + CapLocality, CRB_INTF_CAP_LOCALITY_0_ONLY);
> + ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> + CapCRBIdleBypass, CRB_INTF_CAP_IDLE_FAST);
> + ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> + CapDataXferSizeSupport, CRB_INTF_CAP_XFER_SIZE_64);
> + ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> + CapFIFO, CRB_INTF_CAP_FIFO_NOT_SUPPORTED);
> + ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> + CapCRB, CRB_INTF_CAP_CRB_SUPPORTED);
> + ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> + InterfaceSelector, CRB_INTF_IF_SELECTOR_CRB);
> + ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> + RID, 0b0000);
> + ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID2,
> + VID, PCI_VENDOR_ID_IBM);
> +
> + baseaddr += A_CRB_DATA_BUFFER;
> + s->regs[R_CRB_CTRL_CMD_SIZE] = CRB_CTRL_CMD_SIZE;
> + s->regs[R_CRB_CTRL_CMD_LADDR] = (uint32_t)baseaddr;
> + s->regs[R_CRB_CTRL_CMD_HADDR] = (uint32_t)(baseaddr >> 32);
> + s->regs[R_CRB_CTRL_RSP_SIZE] = CRB_CTRL_CMD_SIZE;
> + s->regs[R_CRB_CTRL_RSP_ADDR] = (uint32_t)baseaddr;
> +
> + s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->tpmbe),
> + CRB_CTRL_CMD_SIZE);
> +
> + if (tpm_backend_startup_tpm(s->tpmbe, s->be_buffer_size) < 0) {
> + exit(1);
> + }
> +}
> +
> +void tpm_crb_init_memory(Object *obj, TPMCRBState *s, Error **errp)
> +{
> + memory_region_init_io(&s->mmio, obj, &tpm_crb_memory_ops, s,
> + "tpm-crb-mmio", sizeof(s->regs));
> + memory_region_init_ram(&s->cmdmem, obj,
> + "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
> +}
> diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
> index 6968e60b3f..cb8204d5bc 100644
> --- a/hw/tpm/meson.build
> +++ b/hw/tpm/meson.build
> @@ -3,6 +3,7 @@ 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_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'))
> system_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_ppi.c'))
> system_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_ppi.c'))
>
> diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> index fa882dfefe..3ab1bdb97b 100644
> --- a/hw/tpm/trace-events
> +++ b/hw/tpm/trace-events
> @@ -1,6 +1,6 @@
> # See docs/devel/tracing.rst for syntax documentation.
>
> -# tpm_crb.c
> +# tpm_crb_common.c
> tpm_crb_mmio_read(uint64_t addr, unsigned size, uint32_t val) "CRB read 0x%016" PRIx64 " len:%u val: 0x%" PRIx32
> tpm_crb_mmio_write(uint64_t addr, unsigned size, uint32_t val) "CRB write 0x%016" PRIx64 " len:%u val: 0x%" PRIx32
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
2023-07-13 3:51 ` [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping Joelle van Dyne
@ 2023-07-13 14:17 ` Stefan Berger
2023-07-13 14:50 ` Peter Maydell
0 siblings, 1 reply; 41+ messages in thread
From: Stefan Berger @ 2023-07-13 14:17 UTC (permalink / raw)
To: Joelle van Dyne, qemu-devel; +Cc: Stefan Berger
On 7/12/23 23:51, 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 set when a
> TPM2_HashStart operation is called which only exists for 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. As this bit is unused, we do not need to worry
> about updating it for reads.
>
> Signed-off-by: Joelle van Dyne <j@getutm.app>
> ---
> hw/tpm/tpm_crb.h | 2 -
> hw/tpm/tpm_crb.c | 3 -
> hw/tpm/tpm_crb_common.c | 124 ++++++++++++++++++++--------------------
> 3 files changed, 63 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),
This has to stay here otherwise we cannot restart VMs from saved state once QEMU is upgraded.
2023-07-13T14:15:43.997718Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration
2023-07-13T14:15:43.997813Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram'
2023-07-13T14:15:43.997841Z qemu-system-x86_64: load of migration failed: Invalid argument
Stefan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
2023-07-13 14:17 ` Stefan Berger
@ 2023-07-13 14:50 ` Peter Maydell
2023-07-13 15:28 ` Stefan Berger
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2023-07-13 14:50 UTC (permalink / raw)
To: Stefan Berger; +Cc: Joelle van Dyne, qemu-devel, Stefan Berger
On Thu, 13 Jul 2023 at 15:18, Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 7/12/23 23:51, 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.
These are hardware registers, right? Windows shouldn't
really be doing LDP to those if it expects to be able to run
in a VM...
> > 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 set when a
> > TPM2_HashStart operation is called which only exists for 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. As this bit is unused, we do not need to worry
> > about updating it for reads.
> >
> > Signed-off-by: Joelle van Dyne <j@getutm.app>
> > ---
> > hw/tpm/tpm_crb.h | 2 -
> > hw/tpm/tpm_crb.c | 3 -
> > hw/tpm/tpm_crb_common.c | 124 ++++++++++++++++++++--------------------
> > 3 files changed, 63 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),
>
> This has to stay here otherwise we cannot restart VMs from saved state once QEMU is upgraded.
>
> 2023-07-13T14:15:43.997718Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration
> 2023-07-13T14:15:43.997813Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram'
> 2023-07-13T14:15:43.997841Z qemu-system-x86_64: load of migration failed: Invalid argument
More generally, for migration compatibility in the other
direction you need to use memory_region_init_rom_device_nomigrate()
and make sure you keep migrating the data via this, not
via the MemoryRegion.
I'm not a super-fan of hacking around the fact that LDP
to hardware registers isn't supported in specific device
models, though...
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
2023-07-13 14:50 ` Peter Maydell
@ 2023-07-13 15:28 ` Stefan Berger
2023-07-13 15:34 ` Peter Maydell
0 siblings, 1 reply; 41+ messages in thread
From: Stefan Berger @ 2023-07-13 15:28 UTC (permalink / raw)
To: Peter Maydell; +Cc: Joelle van Dyne, qemu-devel, Stefan Berger
On 7/13/23 10:50, Peter Maydell wrote:
> On Thu, 13 Jul 2023 at 15:18, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 7/12/23 23:51, 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.
>>> +++ 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),
>>
>> This has to stay here otherwise we cannot restart VMs from saved state once QEMU is upgraded.
>>
>> 2023-07-13T14:15:43.997718Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration
>> 2023-07-13T14:15:43.997813Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram'
>> 2023-07-13T14:15:43.997841Z qemu-system-x86_64: load of migration failed: Invalid argument
>
> More generally, for migration compatibility in the other
> direction you need to use memory_region_init_rom_device_nomigrate()
> and make sure you keep migrating the data via this, not
> via the MemoryRegion.
>
> I'm not a super-fan of hacking around the fact that LDP
> to hardware registers isn't supported in specific device
> models, though...
What does this mean for this effort here?
Stefan
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 02/11] tpm_crb: CTRL_RSP_ADDR is 64-bits wide
2023-07-13 3:51 ` [PATCH 02/11] tpm_crb: CTRL_RSP_ADDR is 64-bits wide Joelle van Dyne
@ 2023-07-13 15:31 ` Stefan Berger
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Berger @ 2023-07-13 15:31 UTC (permalink / raw)
To: Joelle van Dyne, qemu-devel
Cc: Stefan Berger, Michael S. Tsirkin, Igor Mammedov, Ani Sinha,
Thomas Huth, Laurent Vivier, Paolo Bonzini
On 7/12/23 23:51, Joelle van Dyne wrote:
> The register is actually 64-bits but in order to make this more clear
> than the specification, we define two 32-bit registers:
> CTRL_RSP_LADDR and CTRL_RSP_HADDR to match the CTRL_CMD_* naming. This
> deviates from the specs but is way more clear.
>
> Previously, the only CRB device uses a fixed system address so this
> was not an issue. However, once we support SysBus CRB device, the
> address can be anywhere in 64-bit space.
>
> Signed-off-by: Joelle van Dyne <j@getutm.app>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
> include/hw/acpi/tpm.h | 3 ++-
> hw/tpm/tpm_crb_common.c | 3 ++-
> tests/qtest/tpm-crb-test.c | 2 +-
> tests/qtest/tpm-util.c | 2 +-
> 4 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> index 579c45f5ba..f60bfe2789 100644
> --- a/include/hw/acpi/tpm.h
> +++ b/include/hw/acpi/tpm.h
> @@ -174,7 +174,8 @@ REG32(CRB_CTRL_CMD_SIZE, 0x58)
> REG32(CRB_CTRL_CMD_LADDR, 0x5C)
> REG32(CRB_CTRL_CMD_HADDR, 0x60)
> REG32(CRB_CTRL_RSP_SIZE, 0x64)
> -REG32(CRB_CTRL_RSP_ADDR, 0x68)
> +REG32(CRB_CTRL_RSP_LADDR, 0x68)
> +REG32(CRB_CTRL_RSP_HADDR, 0x6C)
> REG32(CRB_DATA_BUFFER, 0x80)
>
> #define TPM_CRB_ADDR_BASE 0xFED40000
> diff --git a/hw/tpm/tpm_crb_common.c b/hw/tpm/tpm_crb_common.c
> index 4c173affb6..228e2d0faf 100644
> --- a/hw/tpm/tpm_crb_common.c
> +++ b/hw/tpm/tpm_crb_common.c
> @@ -199,7 +199,8 @@ void tpm_crb_reset(TPMCRBState *s, uint64_t baseaddr)
> s->regs[R_CRB_CTRL_CMD_LADDR] = (uint32_t)baseaddr;
> s->regs[R_CRB_CTRL_CMD_HADDR] = (uint32_t)(baseaddr >> 32);
> s->regs[R_CRB_CTRL_RSP_SIZE] = CRB_CTRL_CMD_SIZE;
> - s->regs[R_CRB_CTRL_RSP_ADDR] = (uint32_t)baseaddr;
> + s->regs[R_CRB_CTRL_RSP_LADDR] = (uint32_t)baseaddr;
> + s->regs[R_CRB_CTRL_RSP_HADDR] = (uint32_t)(baseaddr >> 32);
>
> s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->tpmbe),
> CRB_CTRL_CMD_SIZE);
> diff --git a/tests/qtest/tpm-crb-test.c b/tests/qtest/tpm-crb-test.c
> index 396ae3f91c..9d30fe8293 100644
> --- a/tests/qtest/tpm-crb-test.c
> +++ b/tests/qtest/tpm-crb-test.c
> @@ -28,7 +28,7 @@ static void tpm_crb_test(const void *data)
> uint32_t csize = readl(TPM_CRB_ADDR_BASE + A_CRB_CTRL_CMD_SIZE);
> uint64_t caddr = readq(TPM_CRB_ADDR_BASE + A_CRB_CTRL_CMD_LADDR);
> uint32_t rsize = readl(TPM_CRB_ADDR_BASE + A_CRB_CTRL_RSP_SIZE);
> - uint64_t raddr = readq(TPM_CRB_ADDR_BASE + A_CRB_CTRL_RSP_ADDR);
> + uint64_t raddr = readq(TPM_CRB_ADDR_BASE + A_CRB_CTRL_RSP_LADDR);
> uint8_t locstate = readb(TPM_CRB_ADDR_BASE + A_CRB_LOC_STATE);
> uint32_t locctrl = readl(TPM_CRB_ADDR_BASE + A_CRB_LOC_CTRL);
> uint32_t locsts = readl(TPM_CRB_ADDR_BASE + A_CRB_LOC_STS);
> diff --git a/tests/qtest/tpm-util.c b/tests/qtest/tpm-util.c
> index 1c0319e6e7..dd02057fc0 100644
> --- a/tests/qtest/tpm-util.c
> +++ b/tests/qtest/tpm-util.c
> @@ -25,7 +25,7 @@ void tpm_util_crb_transfer(QTestState *s,
> unsigned char *rsp, size_t rsp_size)
> {
> uint64_t caddr = qtest_readq(s, TPM_CRB_ADDR_BASE + A_CRB_CTRL_CMD_LADDR);
> - uint64_t raddr = qtest_readq(s, TPM_CRB_ADDR_BASE + A_CRB_CTRL_RSP_ADDR);
> + uint64_t raddr = qtest_readq(s, TPM_CRB_ADDR_BASE + A_CRB_CTRL_RSP_LADDR);
>
> qtest_writeb(s, TPM_CRB_ADDR_BASE + A_CRB_LOC_CTRL, 1);
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 07/11] hw/arm/virt: add plug handler for TPM on SysBus
2023-07-13 3:51 ` [PATCH 07/11] hw/arm/virt: add plug handler for TPM on SysBus Joelle van Dyne
2023-07-13 13:13 ` Stefan Berger
@ 2023-07-13 15:31 ` Peter Maydell
2023-07-13 18:07 ` Joelle van Dyne
1 sibling, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2023-07-13 15:31 UTC (permalink / raw)
To: Joelle van Dyne; +Cc: qemu-devel, open list:Virt
On Thu, 13 Jul 2023 at 04:52, Joelle van Dyne <j@getutm.app> wrote:
>
> TPM needs to know its own base address in order to generate its DSDT
> device entry.
>
> Signed-off-by: Joelle van Dyne <j@getutm.app>
> ---
> 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, &error_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;
> + SysBusDevice *sbdev = SYS_BUS_DEVICE(tpmif);
> + MemoryRegion *sbdev_mr;
> + hwaddr tpm_base;
> + uint64_t tpm_size;
> +
> + if (!sbdev || !object_dynamic_cast(OBJECT(sbdev), TYPE_SYS_BUS_DEVICE)) {
> + 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);
> + tpm_size = memory_region_size(sbdev_mr);
> +
> + if (object_property_find(OBJECT(sbdev), "baseaddr")) {
> + object_property_set_uint(OBJECT(sbdev), "baseaddr", tpm_base, NULL);
> + }
> + if (object_property_find(OBJECT(sbdev), "size")) {
> + object_property_set_uint(OBJECT(sbdev), "size", tpm_size, NULL);
> + }
> +}
> +#endif
I do not like the "platform bus" at all -- it is a nasty hack.
If the virt board needs a memory mapped TPM device it should probably
just create one, the same way we create our other memory mapped
devices. But...
How are TPM devices typically set up/visible to the guest on
real Arm server hardware ? Should this be a sysbus device at all?
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
2023-07-13 15:28 ` Stefan Berger
@ 2023-07-13 15:34 ` Peter Maydell
2023-07-13 15:46 ` Stefan Berger
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2023-07-13 15:34 UTC (permalink / raw)
To: Stefan Berger; +Cc: Joelle van Dyne, qemu-devel, Stefan Berger
On Thu, 13 Jul 2023 at 16:28, Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 7/13/23 10:50, Peter Maydell wrote:
> > On Thu, 13 Jul 2023 at 15:18, Stefan Berger <stefanb@linux.ibm.com> wrote:
> >>
> >>
> >>
> >> On 7/12/23 23:51, 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.
>
> >>> +++ 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),
> >>
> >> This has to stay here otherwise we cannot restart VMs from saved state once QEMU is upgraded.
> >>
> >> 2023-07-13T14:15:43.997718Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration
> >> 2023-07-13T14:15:43.997813Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram'
> >> 2023-07-13T14:15:43.997841Z qemu-system-x86_64: load of migration failed: Invalid argument
> >
> > More generally, for migration compatibility in the other
> > direction you need to use memory_region_init_rom_device_nomigrate()
> > and make sure you keep migrating the data via this, not
> > via the MemoryRegion.
> >
> > I'm not a super-fan of hacking around the fact that LDP
> > to hardware registers isn't supported in specific device
> > models, though...
>
> What does this mean for this effort here?
Usually we say "fix the guest to not try to access hardware
registers with silly load/store instruction types". The other
option would be "put in a large amount of effort to support
emulating those instructions in QEMU userspace when KVM/HVF/etc
trap and punt them to us". For the last decade or so we have
taken the first of these approaches :-)
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
2023-07-13 15:34 ` Peter Maydell
@ 2023-07-13 15:46 ` Stefan Berger
2023-07-13 15:55 ` Peter Maydell
0 siblings, 1 reply; 41+ messages in thread
From: Stefan Berger @ 2023-07-13 15:46 UTC (permalink / raw)
To: Peter Maydell; +Cc: Joelle van Dyne, qemu-devel, Stefan Berger
On 7/13/23 11:34, Peter Maydell wrote:
> On Thu, 13 Jul 2023 at 16:28, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 7/13/23 10:50, Peter Maydell wrote:
>>> On Thu, 13 Jul 2023 at 15:18, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>
>>>>
>>>>
>>>> On 7/12/23 23:51, 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.
>>
>>>>> +++ 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),
>>>>
>>>> This has to stay here otherwise we cannot restart VMs from saved state once QEMU is upgraded.
>>>>
>>>> 2023-07-13T14:15:43.997718Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration
>>>> 2023-07-13T14:15:43.997813Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram'
>>>> 2023-07-13T14:15:43.997841Z qemu-system-x86_64: load of migration failed: Invalid argument
>>>
>>> More generally, for migration compatibility in the other
>>> direction you need to use memory_region_init_rom_device_nomigrate()
>>> and make sure you keep migrating the data via this, not
>>> via the MemoryRegion.
>>>
>>> I'm not a super-fan of hacking around the fact that LDP
>>> to hardware registers isn't supported in specific device
>>> models, though...
>>
>> What does this mean for this effort here?
>
> Usually we say "fix the guest to not try to access hardware
> registers with silly load/store instruction types". The other
> option would be "put in a large amount of effort to support
> emulating those instructions in QEMU userspace when KVM/HVF/etc
> trap and punt them to us". For the last decade or so we have
> taken the first of these approaches :-)
Is Microsoft likely to react to use telling them "fix the guest"?
Stefan
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
2023-07-13 15:46 ` Stefan Berger
@ 2023-07-13 15:55 ` Peter Maydell
2023-07-13 16:53 ` Stefan Berger
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2023-07-13 15:55 UTC (permalink / raw)
To: Stefan Berger; +Cc: Joelle van Dyne, qemu-devel, Stefan Berger
On Thu, 13 Jul 2023 at 16:46, Stefan Berger <stefanb@linux.ibm.com> wrote:
> On 7/13/23 11:34, Peter Maydell wrote:
> > On Thu, 13 Jul 2023 at 16:28, Stefan Berger <stefanb@linux.ibm.com> wrote:
> >> On 7/13/23 10:50, Peter Maydell wrote:
> >>> I'm not a super-fan of hacking around the fact that LDP
> >>> to hardware registers isn't supported in specific device
> >>> models, though...
> >>
> >> What does this mean for this effort here?
> >
> > Usually we say "fix the guest to not try to access hardware
> > registers with silly load/store instruction types". The other
> > option would be "put in a large amount of effort to support
> > emulating those instructions in QEMU userspace when KVM/HVF/etc
> > trap and punt them to us". For the last decade or so we have
> > taken the first of these approaches :-)
>
> Is Microsoft likely to react to use telling them "fix the guest"?
They have on occasion in the past, yes.
The other outstanding question here is if this TPM device
should be a sysbus one at all (i.e. not i2c), which might
render this part moot.
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 03/11] tpm_ppi: refactor memory space initialization
2023-07-13 3:51 ` [PATCH 03/11] tpm_ppi: refactor memory space initialization Joelle van Dyne
@ 2023-07-13 16:00 ` Stefan Berger
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Berger @ 2023-07-13 16:00 UTC (permalink / raw)
To: Joelle van Dyne, qemu-devel; +Cc: Stefan Berger
On 7/12/23 23:51, Joelle van Dyne wrote:
> Instead of calling `memory_region_add_subregion` directly, we defer to
> the caller to do it. This allows us to re-use the code for a SysBus
> device.
>
> Signed-off-by: Joelle van Dyne <j@getutm.app>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
> hw/tpm/tpm_ppi.h | 10 +++-------
> hw/tpm/tpm_crb.c | 4 ++--
> hw/tpm/tpm_crb_common.c | 3 +++
> hw/tpm/tpm_ppi.c | 5 +----
> hw/tpm/tpm_tis_isa.c | 5 +++--
> 5 files changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> index bf5d4a300f..30863c6438 100644
> --- a/hw/tpm/tpm_ppi.h
> +++ b/hw/tpm/tpm_ppi.h
> @@ -20,17 +20,13 @@ typedef struct TPMPPI {
> } TPMPPI;
>
> /**
> - * tpm_ppi_init:
> + * tpm_ppi_init_memory:
> * @tpmppi: a TPMPPI
> - * @m: the address-space / MemoryRegion to use
> - * @addr: the address of the PPI region
> * @obj: the owner object
> *
> - * Register the TPM PPI memory region at @addr on the given address
> - * space for the object @obj.
> + * Creates the TPM PPI memory region.
> **/
> -void tpm_ppi_init(TPMPPI *tpmppi, MemoryRegion *m,
> - hwaddr addr, Object *obj);
> +void tpm_ppi_init_memory(TPMPPI *tpmppi, Object *obj);
>
> /**
> * tpm_ppi_reset:
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index 3ef4977fb5..598c3e0161 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -107,8 +107,8 @@ static void tpm_crb_none_realize(DeviceState *dev, Error **errp)
> TPM_CRB_ADDR_BASE + sizeof(s->state.regs), &s->state.cmdmem);
>
> if (s->state.ppi_enabled) {
> - tpm_ppi_init(&s->state.ppi, get_system_memory(),
> - TPM_PPI_ADDR_BASE, OBJECT(s));
> + memory_region_add_subregion(get_system_memory(),
> + TPM_PPI_ADDR_BASE, &s->state.ppi.ram);
> }
>
> if (xen_enabled()) {
> diff --git a/hw/tpm/tpm_crb_common.c b/hw/tpm/tpm_crb_common.c
> index 228e2d0faf..e56e910670 100644
> --- a/hw/tpm/tpm_crb_common.c
> +++ b/hw/tpm/tpm_crb_common.c
> @@ -216,4 +216,7 @@ void tpm_crb_init_memory(Object *obj, TPMCRBState *s, Error **errp)
> "tpm-crb-mmio", sizeof(s->regs));
> memory_region_init_ram(&s->cmdmem, obj,
> "tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
> + if (s->ppi_enabled) {
> + tpm_ppi_init_memory(&s->ppi, obj);
> + }
> }
> diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
> index 7f74e26ec6..40cab59afa 100644
> --- a/hw/tpm/tpm_ppi.c
> +++ b/hw/tpm/tpm_ppi.c
> @@ -44,14 +44,11 @@ void tpm_ppi_reset(TPMPPI *tpmppi)
> }
> }
>
> -void tpm_ppi_init(TPMPPI *tpmppi, MemoryRegion *m,
> - hwaddr addr, Object *obj)
> +void tpm_ppi_init_memory(TPMPPI *tpmppi, Object *obj)
> {
> tpmppi->buf = qemu_memalign(qemu_real_host_page_size(),
> HOST_PAGE_ALIGN(TPM_PPI_ADDR_SIZE));
> memory_region_init_ram_device_ptr(&tpmppi->ram, obj, "tpm-ppi",
> TPM_PPI_ADDR_SIZE, tpmppi->buf);
> vmstate_register_ram(&tpmppi->ram, DEVICE(obj));
> -
> - memory_region_add_subregion(m, addr, &tpmppi->ram);
> }
> diff --git a/hw/tpm/tpm_tis_isa.c b/hw/tpm/tpm_tis_isa.c
> index 91e3792248..7cd7415f30 100644
> --- a/hw/tpm/tpm_tis_isa.c
> +++ b/hw/tpm/tpm_tis_isa.c
> @@ -134,8 +134,9 @@ static void tpm_tis_isa_realizefn(DeviceState *dev, Error **errp)
> TPM_TIS_ADDR_BASE, &s->mmio);
>
> if (s->ppi_enabled) {
> - tpm_ppi_init(&s->ppi, isa_address_space(ISA_DEVICE(dev)),
> - TPM_PPI_ADDR_BASE, OBJECT(dev));
> + tpm_ppi_init_memory(&s->ppi, OBJECT(dev));
> + memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
> + TPM_PPI_ADDR_BASE, &s->ppi.ram);
> }
> }
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 06/11] tpm_crb: move ACPI table building to device interface
2023-07-13 3:51 ` [PATCH 06/11] tpm_crb: move ACPI table building to device interface Joelle van Dyne
@ 2023-07-13 16:08 ` Stefan Berger
2023-07-13 18:10 ` Joelle van Dyne
0 siblings, 1 reply; 41+ messages in thread
From: Stefan Berger @ 2023-07-13 16:08 UTC (permalink / raw)
To: Joelle van Dyne, qemu-devel
Cc: Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Marcel Apfelbaum,
Stefan Berger
On 7/12/23 23:51, Joelle van Dyne wrote:
> This logic is similar to TPM TIS ISA device.
>
> Signed-off-by: Joelle van Dyne <j@getutm.app>
> ---
> hw/i386/acpi-build.c | 23 -----------------------
> hw/tpm/tpm_crb.c | 28 ++++++++++++++++++++++++++++
> 2 files changed, 28 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..14feb9857f 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"
> @@ -116,10 +118,34 @@ 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");
> + if (tpm_crb_isa_get_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")));
> + }
CRB only exists for TPM 2.0 and that's why we didn't have a different case here before.
CRB only has MSFT0101: https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm_crb.c#L820
TIS has PNP0C31: https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm_tis.c
You should remove the check for TPM_VERSION_2_0.
Stefan
> + 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 +154,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 +166,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 },
> { }
> }
> };
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 09/11] tpm_tis_sysbus: fix crash when PPI is enabled
2023-07-13 3:51 ` [PATCH 09/11] tpm_tis_sysbus: fix crash when PPI is enabled Joelle van Dyne
@ 2023-07-13 16:49 ` Stefan Berger
2023-07-13 18:15 ` Joelle van Dyne
0 siblings, 1 reply; 41+ messages in thread
From: Stefan Berger @ 2023-07-13 16:49 UTC (permalink / raw)
To: Joelle van Dyne, qemu-devel; +Cc: Stefan Berger
On 7/12/23 23:51, Joelle van Dyne wrote:
> If 'ppi' property is set, then `tpm_ppi_reset` is called on reset
> which SEGFAULTs because `tpmppi->buf` is not allocated.
>
> Signed-off-by: Joelle van Dyne <j@getutm.app>
> ---
> hw/tpm/tpm_tis_sysbus.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
> index 45e63efd63..1014d5d993 100644
> --- a/hw/tpm/tpm_tis_sysbus.c
> +++ b/hw/tpm/tpm_tis_sysbus.c
> @@ -124,6 +124,10 @@ static void tpm_tis_sysbus_realizefn(DeviceState *dev, Error **errp)
> error_setg(errp, "'tpmdev' property is required");
> return;
> }
> +
> + if (s->ppi_enabled) {
> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->ppi.ram);
> + }
> }
>
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)
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
2023-07-13 15:55 ` Peter Maydell
@ 2023-07-13 16:53 ` Stefan Berger
2023-07-13 17:07 ` Peter Maydell
0 siblings, 1 reply; 41+ messages in thread
From: Stefan Berger @ 2023-07-13 16:53 UTC (permalink / raw)
To: Peter Maydell; +Cc: Joelle van Dyne, qemu-devel, Stefan Berger
On 7/13/23 11:55, Peter Maydell wrote:
> On Thu, 13 Jul 2023 at 16:46, Stefan Berger <stefanb@linux.ibm.com> wrote:
>> On 7/13/23 11:34, Peter Maydell wrote:
>>> On Thu, 13 Jul 2023 at 16:28, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>> On 7/13/23 10:50, Peter Maydell wrote:
>>>>> I'm not a super-fan of hacking around the fact that LDP
>>>>> to hardware registers isn't supported in specific device
>>>>> models, though...
>>>>
>>>> What does this mean for this effort here?
>>>
>>> Usually we say "fix the guest to not try to access hardware
>>> registers with silly load/store instruction types". The other
>>> option would be "put in a large amount of effort to support
>>> emulating those instructions in QEMU userspace when KVM/HVF/etc
>>> trap and punt them to us". For the last decade or so we have
>>> taken the first of these approaches :-)
>>
>> Is Microsoft likely to react to use telling them "fix the guest"?
>
> They have on occasion in the past, yes.
>
> The other outstanding question here is if this TPM device
> should be a sysbus one at all (i.e. not i2c), which might
> render this part moot.
Does the aarch64 virt VM support an i2c bus? Would it support the aspeed i2c bus? Does Windows then accept this i2c bus? Maybe the faster answer comes via this device that Joelle presumably has working on AARCH64 Windows.
Stefan
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
2023-07-13 16:53 ` Stefan Berger
@ 2023-07-13 17:07 ` Peter Maydell
2023-07-13 17:16 ` Stefan Berger
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2023-07-13 17:07 UTC (permalink / raw)
To: Stefan Berger; +Cc: Joelle van Dyne, qemu-devel, Stefan Berger
On Thu, 13 Jul 2023 at 17:54, Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 7/13/23 11:55, Peter Maydell wrote:
> > On Thu, 13 Jul 2023 at 16:46, Stefan Berger <stefanb@linux.ibm.com> wrote:
> >> On 7/13/23 11:34, Peter Maydell wrote:
> >>> On Thu, 13 Jul 2023 at 16:28, Stefan Berger <stefanb@linux.ibm.com> wrote:
> >>>> On 7/13/23 10:50, Peter Maydell wrote:
> >>>>> I'm not a super-fan of hacking around the fact that LDP
> >>>>> to hardware registers isn't supported in specific device
> >>>>> models, though...
> >>>>
> >>>> What does this mean for this effort here?
> >>>
> >>> Usually we say "fix the guest to not try to access hardware
> >>> registers with silly load/store instruction types". The other
> >>> option would be "put in a large amount of effort to support
> >>> emulating those instructions in QEMU userspace when KVM/HVF/etc
> >>> trap and punt them to us". For the last decade or so we have
> >>> taken the first of these approaches :-)
> >>
> >> Is Microsoft likely to react to use telling them "fix the guest"?
> >
> > They have on occasion in the past, yes.
> >
> > The other outstanding question here is if this TPM device
> > should be a sysbus one at all (i.e. not i2c), which might
> > render this part moot.
>
> Does the aarch64 virt VM support an i2c bus? Would it support the aspeed i2c bus? Does Windows then accept this i2c bus? Maybe the faster answer comes via this device that Joelle presumably has working on AARCH64 Windows.
The aim is not "get Windows booting as fast as possible", though.
It's to end up with a QEMU virt board that (a) is maintainable
(b) is reasonably congruent with what real hardware does
(c) works in a way that will also work with what other
guest OSes are expecting.
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.
thanks
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
2023-07-13 17:07 ` Peter Maydell
@ 2023-07-13 17:16 ` Stefan Berger
2023-07-13 17:18 ` Peter Maydell
0 siblings, 1 reply; 41+ messages in thread
From: Stefan Berger @ 2023-07-13 17:16 UTC (permalink / raw)
To: Peter Maydell; +Cc: Joelle van Dyne, qemu-devel, Stefan Berger
On 7/13/23 13:07, Peter Maydell wrote:
> On Thu, 13 Jul 2023 at 17:54, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 7/13/23 11:55, Peter Maydell wrote:
>>> On Thu, 13 Jul 2023 at 16:46, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>> On 7/13/23 11:34, Peter Maydell wrote:
>>>>> On Thu, 13 Jul 2023 at 16:28, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>>> On 7/13/23 10:50, Peter Maydell wrote:
>>>>>>> I'm not a super-fan of hacking around the fact that LDP
>>>>>>> to hardware registers isn't supported in specific device
>>>>>>> models, though...
>>>>>>
>>>>>> What does this mean for this effort here?
>>>>>
>>>>> Usually we say "fix the guest to not try to access hardware
>>>>> registers with silly load/store instruction types". The other
>>>>> option would be "put in a large amount of effort to support
>>>>> emulating those instructions in QEMU userspace when KVM/HVF/etc
>>>>> trap and punt them to us". For the last decade or so we have
>>>>> taken the first of these approaches :-)
>>>>
>>>> Is Microsoft likely to react to use telling them "fix the guest"?
>>>
>>> They have on occasion in the past, yes.
>>>
>>> The other outstanding question here is if this TPM device
>>> should be a sysbus one at all (i.e. not i2c), which might
>>> render this part moot.
>>
>> Does the aarch64 virt VM support an i2c bus? Would it support the aspeed i2c bus? Does Windows then accept this i2c bus? Maybe the faster answer comes via this device that Joelle presumably has working on AARCH64 Windows.
>
> The aim is not "get Windows booting as fast as possible", though.
> It's to end up with a QEMU virt board that (a) is maintainable
> (b) is reasonably congruent with what real hardware does
> (c) works in a way that will also work with what other
> guest OSes are expecting.
>
> 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.
>
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'?
dev = qdev_new("arm-gicv2m");
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vms->memmap[VIRT_GIC_V2M].base);
qdev_prop_set_uint32(dev, "base-spi", irq);
qdev_prop_set_uint32(dev, "num-spi", NUM_GICV2M_SPIS);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
Stefan
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
2023-07-13 17:16 ` Stefan Berger
@ 2023-07-13 17:18 ` Peter Maydell
2023-07-13 18:43 ` Stefan Berger
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2023-07-13 17:18 UTC (permalink / raw)
To: Stefan Berger; +Cc: Joelle van Dyne, qemu-devel, Stefan Berger
On Thu, 13 Jul 2023 at 18:16, Stefan Berger <stefanb@linux.ibm.com> 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
"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).
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 00/11] tpm: introduce TPM CRB SysBus device
2023-07-13 13:07 ` [PATCH 00/11] tpm: " Stefan Berger
@ 2023-07-13 17:35 ` Joelle van Dyne
0 siblings, 0 replies; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-13 17:35 UTC (permalink / raw)
To: Stefan Berger; +Cc: Joelle van Dyne, qemu-devel
On Thu, Jul 13, 2023 at 6:07 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 7/12/23 23:51, 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.
>
> Thanks a lot for this work. The last sentence seems to hint at the current issue
> with TPM CRB on ARM64 and seems to be the only way forward there. You may want
> to reformulate it a bit because it's not clear how the 'however' related to
> CRB relates to TIS.
>
> >
> > 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
>
> Please reorder bugs to the beginning of the series or submit in an extra patch set
> so we can backport them. Ideal would be description(s) for how to trigger the bug(s).
>
> > 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
> s/redundent/redundant
>
>
> > 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 caused page faults. @agraf suggested that we do this to avoid having to
>
> Afaik, LDP is an ARM assembly instruction that loads two 32bit or 64bit registers from
> consecutive addresses. May be worth mentioning for those wondering about it...
>
> > do AARCH64 decoding in the HVF fault handler.
>
> What is HVF?
Sorry, HVF is the QEMU backend for Apple's Hypervisor.framework which
runs on macOS including on Apple Silicon.
>
> Regards,
> Stefan
> >
> > 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.
> >
> > Joelle van Dyne (11):
> > 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: use the ISA bus
> > tpm_crb: move ACPI table building to device interface
> > hw/arm/virt: add plug handler for TPM on SysBus
> > hw/loongarch/virt: add plug handler for TPM on SysBus
> > tpm_tis_sysbus: fix crash when PPI is enabled
> > tpm_tis_sysbus: move DSDT AML generation to device
> > tpm_crb_sysbus: introduce TPM CRB SysBus device
> >
> > docs/specs/tpm.rst | 2 +
> > hw/tpm/tpm_crb.h | 74 +++++++++
> > hw/tpm/tpm_ppi.h | 10 +-
> > include/hw/acpi/aml-build.h | 1 +
> > include/hw/acpi/tpm.h | 3 +-
> > include/sysemu/tpm.h | 3 +
> > hw/acpi/aml-build.c | 7 +-
> > hw/arm/virt-acpi-build.c | 38 +----
> > hw/arm/virt.c | 38 +++++
> > hw/core/sysbus-fdt.c | 1 +
> > hw/i386/acpi-build.c | 23 ---
> > hw/loongarch/acpi-build.c | 38 +----
> > hw/loongarch/virt.c | 38 +++++
> > hw/riscv/virt.c | 1 +
> > hw/tpm/tpm_crb.c | 307 ++++++++----------------------------
> > hw/tpm/tpm_crb_common.c | 224 ++++++++++++++++++++++++++
> > hw/tpm/tpm_crb_sysbus.c | 178 +++++++++++++++++++++
> > hw/tpm/tpm_ppi.c | 5 +-
> > hw/tpm/tpm_tis_isa.c | 5 +-
> > hw/tpm/tpm_tis_sysbus.c | 43 +++++
> > tests/qtest/tpm-crb-test.c | 2 +-
> > tests/qtest/tpm-util.c | 2 +-
> > hw/arm/Kconfig | 1 +
> > hw/riscv/Kconfig | 1 +
> > hw/tpm/Kconfig | 7 +-
> > hw/tpm/meson.build | 3 +
> > hw/tpm/trace-events | 2 +-
> > 27 files changed, 703 insertions(+), 354 deletions(-)
> > create mode 100644 hw/tpm/tpm_crb.h
> > create mode 100644 hw/tpm/tpm_crb_common.c
> > create mode 100644 hw/tpm/tpm_crb_sysbus.c
> >
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 07/11] hw/arm/virt: add plug handler for TPM on SysBus
2023-07-13 15:31 ` Peter Maydell
@ 2023-07-13 18:07 ` Joelle van Dyne
0 siblings, 0 replies; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-13 18:07 UTC (permalink / raw)
To: Peter Maydell; +Cc: Joelle van Dyne, qemu-devel, open list:Virt, Alexander Graf
On Thu, Jul 13, 2023 at 8:31 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 13 Jul 2023 at 04:52, Joelle van Dyne <j@getutm.app> wrote:
> >
> > TPM needs to know its own base address in order to generate its DSDT
> > device entry.
> >
> > Signed-off-by: Joelle van Dyne <j@getutm.app>
> > ---
> > 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, &error_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;
> > + SysBusDevice *sbdev = SYS_BUS_DEVICE(tpmif);
> > + MemoryRegion *sbdev_mr;
> > + hwaddr tpm_base;
> > + uint64_t tpm_size;
> > +
> > + if (!sbdev || !object_dynamic_cast(OBJECT(sbdev), TYPE_SYS_BUS_DEVICE)) {
> > + 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);
> > + tpm_size = memory_region_size(sbdev_mr);
> > +
> > + if (object_property_find(OBJECT(sbdev), "baseaddr")) {
> > + object_property_set_uint(OBJECT(sbdev), "baseaddr", tpm_base, NULL);
> > + }
> > + if (object_property_find(OBJECT(sbdev), "size")) {
> > + object_property_set_uint(OBJECT(sbdev), "size", tpm_size, NULL);
> > + }
> > +}
> > +#endif
>
> I do not like the "platform bus" at all -- it is a nasty hack.
> If the virt board needs a memory mapped TPM device it should probably
> just create one, the same way we create our other memory mapped
> devices. But...
>
> How are TPM devices typically set up/visible to the guest on
> real Arm server hardware ? Should this be a sysbus device at all?
+Alexander Graf who may answer this better.
My understanding is that we need to do this for the device to know its
own address which it needs to return in a register. On ISA devices, it
is always mapped to the same physical address so there's no issues but
for Virt machines, device addresses are dynamically allocated by the
PlatformBusDevice so only at this late stage can we tell the device
what its own address is.
>
> thanks
> -- PMM
Also to Stefan's question on consolidating code: that is ideal but
currently, it seems like much platform setup code is duplicated
amongst the various architecture's Virt machines. There would have to
be a larger effort in de-duplicating a lot of that code. Indeed, we
try to do this already with some of the ACPI stuff in the other
patches. For this specifically, we would need to know the platform
bus' base address which is done differently in ARM64's Virt and in
Loongarch's Virt. All we did was delete some existing duplicated code
and replace it with a different duplicated code :)
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 06/11] tpm_crb: move ACPI table building to device interface
2023-07-13 16:08 ` Stefan Berger
@ 2023-07-13 18:10 ` Joelle van Dyne
2023-07-13 18:30 ` Stefan Berger
0 siblings, 1 reply; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-13 18:10 UTC (permalink / raw)
To: Stefan Berger
Cc: Joelle van Dyne, qemu-devel, Michael S. Tsirkin, Igor Mammedov,
Ani Sinha, Paolo Bonzini, Richard Henderson, Eduardo Habkost,
Marcel Apfelbaum, Stefan Berger
In that case, do you think we should have a check in "realize" to make
sure the backend is 2.0?
On Thu, Jul 13, 2023 at 9:08 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 7/12/23 23:51, Joelle van Dyne wrote:
> > This logic is similar to TPM TIS ISA device.
> >
> > Signed-off-by: Joelle van Dyne <j@getutm.app>
> > ---
> > hw/i386/acpi-build.c | 23 -----------------------
> > hw/tpm/tpm_crb.c | 28 ++++++++++++++++++++++++++++
> > 2 files changed, 28 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..14feb9857f 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"
> > @@ -116,10 +118,34 @@ 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");
> > + if (tpm_crb_isa_get_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")));
> > + }
>
> CRB only exists for TPM 2.0 and that's why we didn't have a different case here before.
>
> CRB only has MSFT0101: https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm_crb.c#L820
> TIS has PNP0C31: https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm_tis.c
>
> You should remove the check for TPM_VERSION_2_0.
>
> Stefan
> > + 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 +154,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 +166,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 },
> > { }
> > }
> > };
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 09/11] tpm_tis_sysbus: fix crash when PPI is enabled
2023-07-13 16:49 ` Stefan Berger
@ 2023-07-13 18:15 ` Joelle van Dyne
2023-07-13 18:31 ` Stefan Berger
0 siblings, 1 reply; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-13 18:15 UTC (permalink / raw)
To: Stefan Berger; +Cc: Joelle van Dyne, qemu-devel, Stefan Berger
On Thu, Jul 13, 2023 at 9:49 AM Stefan Berger <stefanb@linux.ibm.com> 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.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 06/11] tpm_crb: move ACPI table building to device interface
2023-07-13 18:10 ` Joelle van Dyne
@ 2023-07-13 18:30 ` Stefan Berger
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Berger @ 2023-07-13 18:30 UTC (permalink / raw)
To: Joelle van Dyne
Cc: qemu-devel, Michael S. Tsirkin, Igor Mammedov, Ani Sinha,
Paolo Bonzini, Richard Henderson, Eduardo Habkost,
Marcel Apfelbaum, Stefan Berger
On 7/13/23 14:10, Joelle van Dyne wrote:
> In that case, do you think we should have a check in "realize" to make
> sure the backend is 2.0?
>
Maybe. I think at the moment it would simply not work (with existing drivers) without terminating QEMU on it due to the misconfiguration. On libvirt level we intercept this case and notify the user that the combination doesn't work. Leaving it like this would be an option...
Stefan
> On Thu, Jul 13, 2023 at 9:08 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 7/12/23 23:51, Joelle van Dyne wrote:
>>> This logic is similar to TPM TIS ISA device.
>>>
>>> Signed-off-by: Joelle van Dyne <j@getutm.app>
>>> ---
>>> hw/i386/acpi-build.c | 23 -----------------------
>>> hw/tpm/tpm_crb.c | 28 ++++++++++++++++++++++++++++
>>> 2 files changed, 28 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..14feb9857f 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"
>>> @@ -116,10 +118,34 @@ 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");
>>> + if (tpm_crb_isa_get_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")));
>>> + }
>>
>> CRB only exists for TPM 2.0 and that's why we didn't have a different case here before.
>>
>> CRB only has MSFT0101: https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm_crb.c#L820
>> TIS has PNP0C31: https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm_tis.c
>>
>> You should remove the check for TPM_VERSION_2_0.
>>
>> Stefan
>>> + 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 +154,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 +166,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 },
>>> { }
>>> }
>>> };
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 09/11] tpm_tis_sysbus: fix crash when PPI is enabled
2023-07-13 18:15 ` Joelle van Dyne
@ 2023-07-13 18:31 ` Stefan Berger
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Berger @ 2023-07-13 18:31 UTC (permalink / raw)
To: Joelle van Dyne; +Cc: qemu-devel, Stefan Berger
On 7/13/23 14:15, Joelle van Dyne wrote:
> On Thu, Jul 13, 2023 at 9:49 AM Stefan Berger <stefanb@linux.ibm.com> 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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 05/11] tpm_crb: use the ISA bus
2023-07-13 3:51 ` [PATCH 05/11] tpm_crb: use the ISA bus Joelle van Dyne
@ 2023-07-13 18:35 ` Stefan Berger
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Berger @ 2023-07-13 18:35 UTC (permalink / raw)
To: Joelle van Dyne, qemu-devel; +Cc: Paolo Bonzini, Stefan Berger
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 <j@getutm.app>
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(&s->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(&s->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(&s->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(&s->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), &s->state, errp);
>
> - memory_region_add_subregion(get_system_memory(),
> + memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
> TPM_CRB_ADDR_BASE, &s->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, &s->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 = &vmstate_tpm_crb_none;
> + dc->realize = tpm_crb_isa_realize;
> + device_class_set_props(dc, tpm_crb_isa_properties);
> + dc->vmsd = &vmstate_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(&tpm_crb_none_info);
> + type_register_static(&tpm_crb_isa_info);
> }
>
> -type_init(tpm_crb_none_register)
> +type_init(tpm_crb_isa_register)
> diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
> index a46663288c..1fd73fe617 100644
> --- a/hw/tpm/Kconfig
> +++ b/hw/tpm/Kconfig
> @@ -22,7 +22,7 @@ config TPM_TIS
>
> config TPM_CRB
> bool
> - depends on TPM && PC
> + depends on TPM && ISA_BUS
> select TPM_BACKEND
>
> config TPM_SPAPR
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
2023-07-13 17:18 ` Peter Maydell
@ 2023-07-13 18:43 ` Stefan Berger
2023-07-14 10:05 ` Peter Maydell
0 siblings, 1 reply; 41+ messages in thread
From: Stefan Berger @ 2023-07-13 18:43 UTC (permalink / raw)
To: Peter Maydell; +Cc: Joelle van Dyne, qemu-devel, Stefan Berger
On 7/13/23 13:18, Peter Maydell wrote:
> On Thu, 13 Jul 2023 at 18:16, Stefan Berger <stefanb@linux.ibm.com> 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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
2023-07-13 18:43 ` Stefan Berger
@ 2023-07-14 10:05 ` Peter Maydell
2023-07-14 11:56 ` Stefan Berger
0 siblings, 1 reply; 41+ messages in thread
From: Peter Maydell @ 2023-07-14 10:05 UTC (permalink / raw)
To: Stefan Berger; +Cc: Joelle van Dyne, qemu-devel, Stefan Berger
On Thu, 13 Jul 2023 at 19:43, Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 7/13/23 13:18, Peter Maydell wrote:
> > On Thu, 13 Jul 2023 at 18:16, Stefan Berger <stefanb@linux.ibm.com> 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.
-- PMM
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
2023-07-14 10:05 ` Peter Maydell
@ 2023-07-14 11:56 ` Stefan Berger
2023-07-14 17:38 ` Joelle van Dyne
0 siblings, 1 reply; 41+ messages in thread
From: Stefan Berger @ 2023-07-14 11:56 UTC (permalink / raw)
To: Peter Maydell; +Cc: Joelle van Dyne, qemu-devel, Stefan Berger
On 7/14/23 06:05, Peter Maydell wrote:
> On Thu, 13 Jul 2023 at 19:43, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 7/13/23 13:18, Peter Maydell wrote:
>>> On Thu, 13 Jul 2023 at 18:16, Stefan Berger <stefanb@linux.ibm.com> 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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
2023-07-14 11:56 ` Stefan Berger
@ 2023-07-14 17:38 ` Joelle van Dyne
0 siblings, 0 replies; 41+ messages in thread
From: Joelle van Dyne @ 2023-07-14 17:38 UTC (permalink / raw)
To: Stefan Berger; +Cc: Peter Maydell, Joelle van Dyne, qemu-devel, Stefan Berger
On Fri, Jul 14, 2023 at 4:57 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 7/14/23 06:05, Peter Maydell wrote:
> > On Thu, 13 Jul 2023 at 19:43, Stefan Berger <stefanb@linux.ibm.com> wrote:
> >>
> >>
> >>
> >> On 7/13/23 13:18, Peter Maydell wrote:
> >>> On Thu, 13 Jul 2023 at 18:16, Stefan Berger <stefanb@linux.ibm.com> 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
FWIW the Windows 11 tpm.sys driver does not support the I2C interface.
The driver only recognizes ACPI devices and the case for Start Method
= 12 (FIFO Interface over I2C bus) goes to an error handler.
^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2023-07-14 17:39 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-13 3:51 [PATCH 00/11] tpm: introduce TPM CRB SysBus device Joelle van Dyne
2023-07-13 3:51 ` [PATCH 01/11] tpm_crb: refactor common code Joelle van Dyne
2023-07-13 13:22 ` Stefan Berger
2023-07-13 3:51 ` [PATCH 02/11] tpm_crb: CTRL_RSP_ADDR is 64-bits wide Joelle van Dyne
2023-07-13 15:31 ` Stefan Berger
2023-07-13 3:51 ` [PATCH 03/11] tpm_ppi: refactor memory space initialization Joelle van Dyne
2023-07-13 16:00 ` Stefan Berger
2023-07-13 3:51 ` [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping Joelle van Dyne
2023-07-13 14:17 ` Stefan Berger
2023-07-13 14:50 ` Peter Maydell
2023-07-13 15:28 ` Stefan Berger
2023-07-13 15:34 ` Peter Maydell
2023-07-13 15:46 ` Stefan Berger
2023-07-13 15:55 ` Peter Maydell
2023-07-13 16:53 ` Stefan Berger
2023-07-13 17:07 ` Peter Maydell
2023-07-13 17:16 ` Stefan Berger
2023-07-13 17:18 ` Peter Maydell
2023-07-13 18:43 ` Stefan Berger
2023-07-14 10:05 ` Peter Maydell
2023-07-14 11:56 ` Stefan Berger
2023-07-14 17:38 ` Joelle van Dyne
2023-07-13 3:51 ` [PATCH 05/11] tpm_crb: use the ISA bus Joelle van Dyne
2023-07-13 18:35 ` Stefan Berger
2023-07-13 3:51 ` [PATCH 06/11] tpm_crb: move ACPI table building to device interface Joelle van Dyne
2023-07-13 16:08 ` Stefan Berger
2023-07-13 18:10 ` Joelle van Dyne
2023-07-13 18:30 ` Stefan Berger
2023-07-13 3:51 ` [PATCH 07/11] hw/arm/virt: add plug handler for TPM on SysBus Joelle van Dyne
2023-07-13 13:13 ` Stefan Berger
2023-07-13 15:31 ` Peter Maydell
2023-07-13 18:07 ` Joelle van Dyne
2023-07-13 3:51 ` [PATCH 08/11] hw/loongarch/virt: " Joelle van Dyne
2023-07-13 3:51 ` [PATCH 09/11] tpm_tis_sysbus: fix crash when PPI is enabled Joelle van Dyne
2023-07-13 16:49 ` Stefan Berger
2023-07-13 18:15 ` Joelle van Dyne
2023-07-13 18:31 ` Stefan Berger
2023-07-13 3:51 ` [PATCH 10/11] tpm_tis_sysbus: move DSDT AML generation to device Joelle van Dyne
2023-07-13 3:51 ` [PATCH 11/11] tpm_crb_sysbus: introduce TPM CRB SysBus device Joelle van Dyne
2023-07-13 13:07 ` [PATCH 00/11] tpm: " Stefan Berger
2023-07-13 17:35 ` Joelle van Dyne
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).