[RFC v2 0/7] hw/tpm: CRB chunking capability to handle PQC

[RFC v2 0/7] hw/tpm: CRB chunking capability to handle PQC

[Arun Menon]

The move to Post Quantum Cryptography (PQC) changes how we manage
memory buffers. Unlike classic crypto algorithms like RSA or ECC which
used small keys and signatures, PQC algorithms require larger buffers.

The new version of TCG TPM v185 (currently under review [1]) supports
sending data/commands in chunks for the CRB (Command Response Buffer)
interface. This is in line with the initiative to support PQC algorithms.

This series implements the logic to send and receive data from the
linux guest to the TPM backend in chunks, thereby allowing the
guest to send larger data buffers. We introduce 2 new control registers
called nextChunk and crbRspRetry that will control the START. We also
add the CRB Interface Identifier called CapCRBChunk that is set to 1
indicating that the device supports chunking. The default maximum
chunk/buffer size is 3968 (4096 - 128) bytes.

During a send operation, the guest driver places data in the CRB buffer
and signals nextChunk for each segment until the final chunk is reached.
Upon receiving the START signal, QEMU appends the final chunk to its
internal buffer and dispatches the complete command to the TPM backend.

For responses, the backend's output is buffered. The guest consumes the
first chunk once the START bit is cleared. Subsequent chunks are
retrieved by the guest toggling the nextChunk bit, which advances the
internal buffer offset and populates the CRB data window.

For this to work, the linux guest tpm driver will also have to
a) probe if CRB chunking is supported
b) send data in chunks if the command length exceeds the chunk size.
c) receive data in chunks by sending a nextChunk signal and accumulate.

The included test demonstrates functional correctness for standard
buffer sizes. However, validation of PQC-sized payloads was performed
via manual buffer-size overrides.

[1] https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p07_rc1_121225.pdf

v2
--
- Add the VM migration support.
- Increase the TIS TPM interface max buffer size to 8192.

Arun Menon (7):
  hw/tpm: Add TPM CRB chunking fields
  hw/tpm: Refactor CRB_CTRL_START register access
  hw/tpm: Add internal buffer state for chunking
  hw/tpm: Implement TPM CRB chunking logic
  test/qtest: Add test for tpm crb chunking
  hw/tpm: Add support for VM migration with TPM CRB chunking
  hw/tpm: Increase TPM TIS max buffer size to 8192

 hw/core/machine.c                |   1 +
 hw/tpm/tpm_crb.c                 | 291 ++++++++++++++++++++++++++++---
 hw/tpm/tpm_tis.h                 |   2 +-
 include/hw/acpi/tpm.h            |   5 +-
 tests/qtest/tpm-crb-swtpm-test.c |  10 ++
 tests/qtest/tpm-util.c           | 106 +++++++++--
 tests/qtest/tpm-util.h           |   5 +
 7 files changed, 382 insertions(+), 38 deletions(-)

-- 
2.53.0

[RFC v2 1/7] hw/tpm: Add TPM CRB chunking fields

[Arun Menon]

- Add new fields to the CRB Interface Identifier and the CRB
  Control Start registers.
- CRB_CTRL_START now has 2 new settings, that can be toggled using the
  nextChunk and crbRspRetry bits.
- CapCRBChunk bit (10) was Reserved1 previously. The field is reused in
  this revision of the specification.
- Refer to section 6.4.2.2 of [1]

[1] https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p07_rc1_121225.pdf

Signed-off-by: Arun Menon <armenon@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
 hw/tpm/tpm_crb.c      | 3 +++
 include/hw/acpi/tpm.h | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 8723536f93..0a1c7ecdc6 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -59,6 +59,7 @@ DECLARE_INSTANCE_CHECKER(CRBState, CRB,
 #define CRB_INTF_CAP_FIFO_NOT_SUPPORTED 0b0
 #define CRB_INTF_CAP_CRB_SUPPORTED 0b1
 #define CRB_INTF_IF_SELECTOR_CRB 0b1
+#define CRB_INTF_CAP_CRB_CHUNK 0b1
 
 #define CRB_CTRL_CMD_SIZE (TPM_CRB_ADDR_SIZE - A_CRB_DATA_BUFFER)
 
@@ -262,6 +263,8 @@ static void tpm_crb_reset(void *dev)
                      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,
+                     CapCRBChunk, CRB_INTF_CAP_CRB_CHUNK);
     ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
                      RID, 0b0000);
     ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID2,
diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
index d2bf6637c5..03d452d2b5 100644
--- a/include/hw/acpi/tpm.h
+++ b/include/hw/acpi/tpm.h
@@ -149,7 +149,7 @@ REG32(CRB_INTF_ID, 0x30)
   FIELD(CRB_INTF_ID, InterfaceVersion, 4, 4)
   FIELD(CRB_INTF_ID, CapLocality, 8, 1)
   FIELD(CRB_INTF_ID, CapCRBIdleBypass, 9, 1)
-  FIELD(CRB_INTF_ID, Reserved1, 10, 1)
+  FIELD(CRB_INTF_ID, CapCRBChunk, 10, 1)
   FIELD(CRB_INTF_ID, CapDataXferSizeSupport, 11, 2)
   FIELD(CRB_INTF_ID, CapFIFO, 13, 1)
   FIELD(CRB_INTF_ID, CapCRB, 14, 1)
@@ -168,6 +168,9 @@ REG32(CRB_CTRL_STS, 0x44)
   FIELD(CRB_CTRL_STS, tpmIdle, 1, 1)
 REG32(CRB_CTRL_CANCEL, 0x48)
 REG32(CRB_CTRL_START, 0x4C)
+  FIELD(CRB_CTRL_START, invoke, 0, 1)
+  FIELD(CRB_CTRL_START, crbRspRetry, 1, 1)
+  FIELD(CRB_CTRL_START, nextChunk, 2, 1)
 REG32(CRB_INT_ENABLED, 0x50)
 REG32(CRB_INT_STS, 0x54)
 REG32(CRB_CTRL_CMD_SIZE, 0x58)
-- 
2.53.0

[RFC v2 2/7] hw/tpm: Refactor CRB_CTRL_START register access

[Arun Menon]

Replace manual bitwise operations with ARRAY_FIELD_DP32 macros
No functional changes.

Signed-off-by: Arun Menon <armenon@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
 hw/tpm/tpm_crb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 0a1c7ecdc6..bc55908786 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -145,7 +145,7 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
             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;
+            ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, invoke, 1);
             s->cmd = (TPMBackendCmd) {
                 .in = mem,
                 .in_len = MIN(tpm_cmd_get_size(mem), s->be_buffer_size),
@@ -194,7 +194,7 @@ static void tpm_crb_request_completed(TPMIf *ti, int ret)
 {
     CRBState *s = CRB(ti);
 
-    s->regs[R_CRB_CTRL_START] &= ~CRB_START_INVOKE;
+    ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, invoke, 0);
     if (ret != 0) {
         ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
                          tpmSts, 1); /* fatal error */
-- 
2.53.0

[RFC v2 3/7] hw/tpm: Add internal buffer state for chunking

[Arun Menon]

- Introduce GByteArray buffers to hold the command request and response
  data during chunked TPM CRB transactions.
- Add helper function to clean them.

Signed-off-by: Arun Menon <armenon@redhat.com>
---
 hw/tpm/tpm_crb.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index bc55908786..5ea1a4a970 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -38,10 +38,13 @@ struct CRBState {
     TPMBackend *tpmbe;
     TPMBackendCmd cmd;
     uint32_t regs[TPM_CRB_R_MAX];
+    size_t be_buffer_size;
     MemoryRegion mmio;
     MemoryRegion cmdmem;
 
-    size_t be_buffer_size;
+    GByteArray *command_buffer;
+    GByteArray *response_buffer;
+    size_t response_offset;
 
     bool ppi_enabled;
     TPMPPI ppi;
@@ -85,6 +88,13 @@ enum crb_cancel {
 
 #define TPM_CRB_NO_LOCALITY 0xff
 
+static void tpm_crb_clear_internal_buffers(CRBState *s)
+{
+    g_byte_array_set_size(s->response_buffer, 0);
+    g_byte_array_set_size(s->command_buffer, 0);
+    s->response_offset = 0;
+}
+
 static uint64_t tpm_crb_mmio_read(void *opaque, hwaddr addr,
                                   unsigned size)
 {
@@ -134,9 +144,11 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
         }
         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);
+        if (val == CRB_CANCEL_INVOKE) {
+            if (s->regs[R_CRB_CTRL_START] & CRB_START_INVOKE) {
+                tpm_backend_cancel_cmd(s->tpmbe);
+            }
+            tpm_crb_clear_internal_buffers(s);
         }
         break;
     case A_CRB_CTRL_START:
@@ -240,6 +252,7 @@ static void tpm_crb_reset(void *dev)
         tpm_ppi_reset(&s->ppi);
     }
     tpm_backend_reset(s->tpmbe);
+    tpm_crb_clear_internal_buffers(s);
 
     memset(s->regs, 0, sizeof(s->regs));
 
@@ -306,6 +319,9 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(get_system_memory(),
         TPM_CRB_ADDR_BASE + sizeof(s->regs), &s->cmdmem);
 
+    s->command_buffer = g_byte_array_new();
+    s->response_buffer = g_byte_array_new();
+
     if (s->ppi_enabled) {
         tpm_ppi_init(&s->ppi, get_system_memory(),
                      TPM_PPI_ADDR_BASE, OBJECT(s));
-- 
2.53.0

[RFC v2 4/7] hw/tpm: Implement TPM CRB chunking logic

[Arun Menon]

- Add logic to populate internal TPM command request and response
  buffers and to toggle the control registers after each operation.
- The chunk size is limited to CRB_CTRL_CMD_SIZE which is
  (TPM_CRB_ADDR_SIZE - A_CRB_DATA_BUFFER). This comes out as 3968 bytes
  (4096 - 128 or 0x1000 - 0x80), because 128 bytes are reserved for
  control and status registers. In other words, only 3968 bytes are
  available for the TPM data.
- With this feature, guests can send commands larger than 3968 bytes.
- Refer section 6.5.3.9 of [1] for implementation details.

[1] https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p07_rc1_121225.pdf

Signed-off-by: Arun Menon <armenon@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
 hw/tpm/tpm_crb.c | 148 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 132 insertions(+), 16 deletions(-)

diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 5ea1a4a970..e61c04aee0 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -17,6 +17,7 @@
 #include "qemu/osdep.h"
 
 #include "qemu/module.h"
+#include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "system/address-spaces.h"
 #include "hw/core/qdev-properties.h"
@@ -65,6 +66,7 @@ DECLARE_INSTANCE_CHECKER(CRBState, CRB,
 #define CRB_INTF_CAP_CRB_CHUNK 0b1
 
 #define CRB_CTRL_CMD_SIZE (TPM_CRB_ADDR_SIZE - A_CRB_DATA_BUFFER)
+#define TPM_HEADER_SIZE 10
 
 enum crb_loc_ctrl {
     CRB_LOC_CTRL_REQUEST_ACCESS = BIT(0),
@@ -80,6 +82,8 @@ enum crb_ctrl_req {
 
 enum crb_start {
     CRB_START_INVOKE = BIT(0),
+    CRB_START_RESP_RETRY = BIT(1),
+    CRB_START_NEXT_CHUNK = BIT(2),
 };
 
 enum crb_cancel {
@@ -122,6 +126,68 @@ static uint8_t tpm_crb_get_active_locty(CRBState *s)
     return ARRAY_FIELD_EX32(s->regs, CRB_LOC_STATE, activeLocality);
 }
 
+static bool tpm_crb_append_command_request(CRBState *s)
+{
+    /*
+     * The linux guest writes the TPM command to the MMIO region in chunks.
+     * This function appends a chunk from the MMIO region to internal
+     * command_buffer.
+     */
+    void *mem = memory_region_get_ram_ptr(&s->cmdmem);
+    uint32_t to_copy = 0;
+    uint32_t total_request_size = 0;
+
+    /*
+     * The initial call extracts the total TPM command size
+     * from its header. For the subsequent calls, the data already
+     * appended in the command_buffer is used to calculate the total
+     * size, as its header stays the same.
+     */
+    if (s->command_buffer->len == 0) {
+        total_request_size = tpm_cmd_get_size(mem);
+        if (total_request_size < TPM_HEADER_SIZE) {
+            ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS, tpmSts, 1);
+            ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, invoke, 0);
+            ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, nextChunk, 0);
+            tpm_crb_clear_internal_buffers(s);
+            error_report("Command size '%d' less than TPM header size '%d'",
+                         total_request_size, TPM_HEADER_SIZE);
+            return false;
+        }
+    } else {
+        total_request_size = tpm_cmd_get_size(s->command_buffer->data);
+    }
+    total_request_size = MIN(total_request_size, s->be_buffer_size);
+
+    if (total_request_size > s->command_buffer->len) {
+        uint32_t remaining = total_request_size - s->command_buffer->len;
+        to_copy = MIN(remaining, CRB_CTRL_CMD_SIZE);
+        g_byte_array_append(s->command_buffer, (guint8 *)mem, to_copy);
+    }
+    return true;
+}
+
+static void tpm_crb_fill_command_response(CRBState *s)
+{
+    /*
+     * Response from the tpm backend will be stored in the internal
+     * response_buffer. This function will serve that accumulated response
+     * to the linux guest in chunks by writing it back to MMIO region.
+     */
+    void *mem = memory_region_get_ram_ptr(&s->cmdmem);
+    uint32_t remaining = s->response_buffer->len - s->response_offset;
+    uint32_t to_copy = MIN(CRB_CTRL_CMD_SIZE, remaining);
+
+    memcpy(mem, s->response_buffer->data + s->response_offset, to_copy);
+
+    if (to_copy < CRB_CTRL_CMD_SIZE) {
+        memset((guint8 *)mem + to_copy, 0, CRB_CTRL_CMD_SIZE - to_copy);
+    }
+
+    s->response_offset += to_copy;
+    memory_region_set_dirty(&s->cmdmem, 0, CRB_CTRL_CMD_SIZE);
+}
+
 static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
                                uint64_t val, unsigned size)
 {
@@ -152,20 +218,48 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
         }
         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);
-
-            ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, invoke, 1);
-            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);
+        if (tpm_crb_get_active_locty(s) != locty) {
+            break;
+        }
+        if (val & CRB_START_INVOKE) {
+            if (!(s->regs[R_CRB_CTRL_START] & CRB_START_INVOKE)) {
+                if (!tpm_crb_append_command_request(s)) {
+                    break;
+                }
+                ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, invoke, 1);
+                g_byte_array_set_size(s->response_buffer, s->be_buffer_size);
+                s->cmd = (TPMBackendCmd) {
+                    .in = s->command_buffer->data,
+                    .in_len = s->command_buffer->len,
+                    .out = s->response_buffer->data,
+                    .out_len = s->response_buffer->len,
+                };
+                tpm_backend_deliver_request(s->tpmbe, &s->cmd);
+            }
+        } else if (val & CRB_START_NEXT_CHUNK) {
+            /*
+             * nextChunk is used both while sending and receiving data.
+             * To distinguish between the two, response_buffer is checked
+             * If it does not have data, then that means we have not yet
+             * sent the command to the tpm backend, and therefore call
+             * tpm_crb_append_command_request()
+             */
+            if (s->response_buffer->len > 0 &&
+                s->response_offset < s->response_buffer->len) {
+                    tpm_crb_fill_command_response(s);
+            } else {
+                if (!tpm_crb_append_command_request(s)) {
+                    break;
+                }
+            }
+            ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, nextChunk, 0);
+        } else if (val & CRB_START_RESP_RETRY) {
+            if (s->response_buffer->len > 0) {
+                s->response_offset = 0;
+                tpm_crb_fill_command_response(s);
+            }
+            ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, crbRspRetry, 0);
+            ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, nextChunk, 0);
         }
         break;
     case A_CRB_LOC_CTRL:
@@ -205,13 +299,36 @@ static const MemoryRegionOps tpm_crb_memory_ops = {
 static void tpm_crb_request_completed(TPMIf *ti, int ret)
 {
     CRBState *s = CRB(ti);
+    void *mem = memory_region_get_ram_ptr(&s->cmdmem);
 
     ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, invoke, 0);
     if (ret != 0) {
         ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
                          tpmSts, 1); /* fatal error */
+        tpm_crb_clear_internal_buffers(s);
+    } else {
+        uint32_t actual_resp_size = tpm_cmd_get_size(s->response_buffer->data);
+        uint32_t total_resp_size = MIN(actual_resp_size, s->be_buffer_size);
+        g_byte_array_set_size(s->response_buffer, total_resp_size);
+        s->response_offset = 0;
+
+        /*
+         * Send the first chunk. Subsequent chunks will be sent using
+         * tpm_crb_fill_command_response()
+         */
+        uint32_t to_copy = MIN(CRB_CTRL_CMD_SIZE, s->response_buffer->len);
+        memcpy(mem, s->response_buffer->data, to_copy);
+
+        if (to_copy < CRB_CTRL_CMD_SIZE) {
+            memset((guint8 *)mem + to_copy, 0, CRB_CTRL_CMD_SIZE - to_copy);
+        }
+        s->response_offset += to_copy;
     }
     memory_region_set_dirty(&s->cmdmem, 0, CRB_CTRL_CMD_SIZE);
+    ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, invoke, 0);
+    ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, nextChunk, 0);
+    ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, crbRspRetry, 0);
+    g_byte_array_set_size(s->command_buffer, 0);
 }
 
 static enum TPMVersion tpm_crb_get_version(TPMIf *ti)
@@ -288,8 +405,7 @@ static void tpm_crb_reset(void *dev)
     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);
+    s->be_buffer_size = tpm_backend_get_buffer_size(s->tpmbe);
 
     if (tpm_backend_startup_tpm(s->tpmbe, s->be_buffer_size) < 0) {
         exit(1);
-- 
2.53.0

[RFC v2 5/7] test/qtest: Add test for tpm crb chunking

[Arun Menon]

- New test case added to the swtpm test. Data is written and read from
  the buffer in chunks.
- The chunk size is dynamically calculated by reading the
  CRB_CTRL_CMD_SIZE address. This can be changed manually to test.
- Add a helper function tpm_wait_till_bit_clear()

Signed-off-by: Arun Menon <armenon@redhat.com>
---
 tests/qtest/tpm-crb-swtpm-test.c |  10 +++
 tests/qtest/tpm-util.c           | 106 ++++++++++++++++++++++++++-----
 tests/qtest/tpm-util.h           |   5 ++
 3 files changed, 106 insertions(+), 15 deletions(-)

diff --git a/tests/qtest/tpm-crb-swtpm-test.c b/tests/qtest/tpm-crb-swtpm-test.c
index ffeb1c396b..050c7b0c1f 100644
--- a/tests/qtest/tpm-crb-swtpm-test.c
+++ b/tests/qtest/tpm-crb-swtpm-test.c
@@ -33,6 +33,14 @@ static void tpm_crb_swtpm_test(const void *data)
                         "tpm-crb", NULL);
 }
 
+static void tpm_crb_chunk_swtpm_test(const void *data)
+{
+    const TestState *ts = data;
+
+    tpm_test_swtpm_test(ts->src_tpm_path, tpm_util_crb_chunk_transfer,
+                        "tpm-crb", NULL);
+}
+
 static void tpm_crb_swtpm_migration_test(const void *data)
 {
     const TestState *ts = data;
@@ -54,6 +62,8 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_data_func("/tpm/crb-swtpm/test", &ts, tpm_crb_swtpm_test);
+    qtest_add_data_func("/tpm/crb-chunk-swtpm/test", &ts,
+                        tpm_crb_chunk_swtpm_test);
     qtest_add_data_func("/tpm/crb-swtpm-migration/test", &ts,
                         tpm_crb_swtpm_migration_test);
     ret = g_test_run();
diff --git a/tests/qtest/tpm-util.c b/tests/qtest/tpm-util.c
index 2cb2dd4796..dfc1fd70b7 100644
--- a/tests/qtest/tpm-util.c
+++ b/tests/qtest/tpm-util.c
@@ -14,16 +14,42 @@
 
 #include "qemu/osdep.h"
 #include <glib/gstdio.h>
+#include "system/memory.h"
 
 #include "hw/acpi/tpm.h"
 #include "libqtest.h"
 #include "tpm-util.h"
 #include "qobject/qdict.h"
 
+#define CRB_ADDR_START (TPM_CRB_ADDR_BASE + A_CRB_CTRL_START)
+#define CRB_CTRL_STS (TPM_CRB_ADDR_BASE + A_CRB_CTRL_STS)
+#define CRB_CTRL_CMD_SIZE (TPM_CRB_ADDR_BASE + A_CRB_CTRL_CMD_SIZE)
+
+#define BIT_START_INVOKE  (1 << 0)
+#define BIT_RETRY_RESPONSE (1 << 1)
+#define BIT_NEXT_CHUNK (1 << 2)
+
+void tpm_wait_till_bit_clear(QTestState *s, uint64_t addr, uint32_t mask)
+{
+    uint32_t sts;
+    uint64_t end_time = g_get_monotonic_time() + 5 * G_TIME_SPAN_SECOND;
+
+    while (true) {
+        sts = qtest_readl(s, addr);
+        if ((sts & mask) == 0) {
+            break;
+        }
+        if (g_get_monotonic_time() >= end_time) {
+            break;
+        }
+    }
+}
+
 void tpm_util_crb_transfer(QTestState *s,
                            const unsigned char *req, size_t req_size,
                            unsigned char *rsp, size_t rsp_size)
 {
+    uint32_t tpmSts;
     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);
 
@@ -31,24 +57,74 @@ void tpm_util_crb_transfer(QTestState *s,
 
     qtest_memwrite(s, caddr, req, req_size);
 
-    uint32_t sts, start = 1;
-    uint64_t end_time = g_get_monotonic_time() + 5 * G_TIME_SPAN_SECOND;
-    qtest_writel(s, TPM_CRB_ADDR_BASE + A_CRB_CTRL_START, start);
-    while (true) {
-        start = qtest_readl(s, TPM_CRB_ADDR_BASE + A_CRB_CTRL_START);
-        if ((start & 1) == 0) {
-            break;
+    qtest_writel(s, CRB_ADDR_START, BIT_START_INVOKE);
+    tpm_wait_till_bit_clear(s, CRB_ADDR_START, BIT_START_INVOKE);
+
+    tpmSts = qtest_readl(s, CRB_CTRL_STS);
+    g_assert_cmpint(tpmSts & 1, ==, 0);
+
+    qtest_memread(s, raddr, rsp, rsp_size);
+}
+
+void tpm_util_crb_chunk_transfer(QTestState *s,
+                                 const unsigned char *req, size_t req_size,
+                                 unsigned char *rsp, size_t rsp_size)
+{
+    uint32_t tpmSts;
+
+    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);
+    uint32_t crb_ctrl_cmd_size = qtest_readl(s, CRB_CTRL_CMD_SIZE);
+
+    size_t chunk_size = crb_ctrl_cmd_size;
+
+    qtest_writeb(s, TPM_CRB_ADDR_BASE + A_CRB_LOC_CTRL, 1);
+
+    for (size_t i = 0 ; i < req_size; i += chunk_size) {
+        bool last_chunk = false;
+        size_t current_chunk_size = chunk_size ;
+
+        if (i + chunk_size > req_size) {
+            last_chunk = true;
+            current_chunk_size = req_size - i;
         }
-        if (g_get_monotonic_time() >= end_time) {
-            break;
+
+        qtest_memwrite(s, caddr, req + i, current_chunk_size);
+
+        if (last_chunk) {
+            qtest_writel(s, CRB_ADDR_START , BIT_START_INVOKE);
+            tpm_wait_till_bit_clear(s, CRB_ADDR_START, BIT_START_INVOKE);
+        } else {
+            qtest_writel(s, CRB_ADDR_START , BIT_NEXT_CHUNK);
+            tpm_wait_till_bit_clear(s, CRB_ADDR_START, BIT_NEXT_CHUNK);
         }
-    };
-    start = qtest_readl(s, TPM_CRB_ADDR_BASE + A_CRB_CTRL_START);
-    g_assert_cmpint(start & 1, ==, 0);
-    sts = qtest_readl(s, TPM_CRB_ADDR_BASE + A_CRB_CTRL_STS);
-    g_assert_cmpint(sts & 1, ==, 0);
+    }
+    tpmSts = qtest_readl(s, CRB_CTRL_STS);
+    g_assert_cmpint(tpmSts & 1, ==, 0);
 
-    qtest_memread(s, raddr, rsp, rsp_size);
+    /*
+     * Read response in chunks
+     */
+
+    unsigned char header[10];
+    qtest_memread(s, raddr, header, sizeof(header));
+
+    uint32_t actual_response_size = ldl_be_p(&header[2]);
+
+    if (actual_response_size > rsp_size) {
+        actual_response_size = rsp_size;
+    }
+
+    for (size_t i = 0; i < actual_response_size; i += chunk_size) {
+        size_t to_read = i + chunk_size > actual_response_size
+                       ? actual_response_size - i
+                       : chunk_size;
+        if (i > 0) {
+            qtest_writel(s, CRB_ADDR_START, BIT_NEXT_CHUNK);
+            tpm_wait_till_bit_clear(s, CRB_ADDR_START, BIT_NEXT_CHUNK);
+        }
+        qtest_memread(s, raddr, rsp + i, to_read);
+    }
 }
 
 void tpm_util_startup(QTestState *s, tx_func *tx)
diff --git a/tests/qtest/tpm-util.h b/tests/qtest/tpm-util.h
index 0cb28dd6e5..681544e7d8 100644
--- a/tests/qtest/tpm-util.h
+++ b/tests/qtest/tpm-util.h
@@ -24,10 +24,15 @@ typedef void (tx_func)(QTestState *s,
                        const unsigned char *req, size_t req_size,
                        unsigned char *rsp, size_t rsp_size);
 
+void tpm_wait_till_bit_clear(QTestState *s, uint64_t addr, uint32_t mask);
 void tpm_util_crb_transfer(QTestState *s,
                            const unsigned char *req, size_t req_size,
                            unsigned char *rsp, size_t rsp_size);
 
+void tpm_util_crb_chunk_transfer(QTestState *s,
+                                 const unsigned char *req, size_t req_size,
+                                 unsigned char *rsp, size_t rsp_size);
+
 void tpm_util_startup(QTestState *s, tx_func *tx);
 void tpm_util_pcrextend(QTestState *s, tx_func *tx);
 void tpm_util_pcrread(QTestState *s, tx_func *tx,
-- 
2.53.0

[RFC v2 6/7] hw/tpm: Add support for VM migration with TPM CRB chunking

[Arun Menon]

- Add subsection in VMState for TPM CRB with the newly introduced
  command and response buffers, along with a needed callback, so that
  newer QEMU only sends the buffers if it is necessary.
- Add hw_compat blocker because the feature is only supported for
  machine type 11.0 and higher.
- If the VM has no pending chunked TPM commands in the internal buffers
  during a VM migration, or if the machine type does not support newly
  introduced buffers, then the needed callback will return false, as it
  checks the hw_compat blocker and thus the subsection will not be sent
  to the destination host.
- Since the original command and response buffers are of type GByteArray,
  they are serialized in pre-save hook before sending them to the
  destination host and then restored back into original buffer using
  the post-load hook.

Signed-off-by: Arun Menon <armenon@redhat.com>
---
 hw/core/machine.c |   1 +
 hw/tpm/tpm_crb.c  | 114 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 6cf0e2f404..fcd6043c99 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -40,6 +40,7 @@
 
 GlobalProperty hw_compat_10_2[] = {
     { "scsi-block", "migrate-pr", "off" },
+    { "tpm-crb", "migrate-buffers", "off"},
 };
 const size_t hw_compat_10_2_len = G_N_ELEMENTS(hw_compat_10_2);
 
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index e61c04aee0..9ce342fe8a 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -33,6 +33,17 @@
 #include "trace.h"
 #include "qom/object.h"
 
+/* command and response buffers; part of VM state when migrating */
+typedef struct TPMCRBMigState {
+    uint32_t cmd_size;
+    uint8_t *cmd_tmp;
+
+    uint32_t rsp_size;
+    uint8_t *rsp_tmp;
+
+    uint32_t rsp_offset;
+} TPMCRBMigState;
+
 struct CRBState {
     DeviceState parent_obj;
 
@@ -49,6 +60,9 @@ struct CRBState {
 
     bool ppi_enabled;
     TPMPPI ppi;
+
+    bool migrate_buffers;
+    TPMCRBMigState mig;
 };
 typedef struct CRBState CRBState;
 
@@ -347,18 +361,118 @@ static int tpm_crb_pre_save(void *opaque)
     return 0;
 }
 
+static bool tpm_crb_chunk_needed(void *opaque)
+{
+    CRBState *s = opaque;
+
+    if (!s->migrate_buffers) {
+        return false;
+    }
+
+    return ((s->command_buffer && s->command_buffer->len > 0) ||
+            (s->response_buffer && s->response_buffer->len > 0));
+}
+
+static int tpm_crb_chunk_pre_save(void *opaque)
+{
+    CRBState *s = opaque;
+
+    if (s->command_buffer) {
+        s->mig.cmd_size = s->command_buffer->len;
+        s->mig.cmd_tmp = s->command_buffer->data;
+    } else {
+        s->mig.cmd_tmp = NULL;
+        s->mig.cmd_size = 0;
+    }
+
+    if (s->response_buffer) {
+        s->mig.rsp_size = s->response_buffer->len;
+        s->mig.rsp_tmp = s->response_buffer->data;
+    } else {
+        s->mig.rsp_tmp = NULL;
+        s->mig.rsp_size = 0;
+    }
+    s->mig.rsp_offset = (uint32_t)s->response_offset;
+    return 0;
+}
+
+static bool tpm_crb_chunk_post_load(void *opaque, int version_id, Error **errp)
+{
+    CRBState *s = opaque;
+
+    if (s->mig.cmd_size > s->be_buffer_size ||
+        s->mig.rsp_size > s->be_buffer_size ||
+        s->mig.rsp_offset > s->mig.rsp_size) {
+        error_setg(errp,
+                   "tpm-crb-chunk: incoming buffer %u, exceeds limits %zu "
+                   "or offset %u exceeds size %u",
+                   s->mig.cmd_size, s->be_buffer_size,
+                   s->mig.rsp_offset, s->mig.rsp_size);
+        g_free(s->mig.cmd_tmp);
+        s->mig.cmd_tmp = NULL;
+        g_free(s->mig.rsp_tmp);
+        s->mig.rsp_tmp = NULL;
+        return false;
+    }
+
+    if (s->mig.cmd_tmp) {
+        if (s->command_buffer) {
+            g_byte_array_unref(s->command_buffer);
+        }
+        s->command_buffer = g_byte_array_new_take(s->mig.cmd_tmp,
+                                                  s->mig.cmd_size);
+        s->mig.cmd_tmp = NULL;
+    } else {
+        if (s->command_buffer) {
+            g_byte_array_set_size(s->command_buffer, 0);
+        }
+    }
+    if (s->mig.rsp_tmp) {
+        if (s->response_buffer) {
+            g_byte_array_unref(s->response_buffer);
+        }
+        s->response_buffer = g_byte_array_new_take(s->mig.rsp_tmp,
+                                                   s->mig.rsp_size);
+        s->mig.rsp_tmp = NULL;
+    }
+    return true;
+}
+
+static const VMStateDescription vmstate_tpm_crb_chunk = {
+    .name = "tpm-crb/chunk",
+    .version_id = 1,
+    .needed = tpm_crb_chunk_needed,
+    .pre_save = tpm_crb_chunk_pre_save,
+    .post_load_errp = tpm_crb_chunk_post_load,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT32(mig.cmd_size, CRBState),
+        VMSTATE_VBUFFER_ALLOC_UINT32(mig.cmd_tmp, CRBState, 0, NULL,
+                                     mig.cmd_size),
+        VMSTATE_UINT32(mig.rsp_size, CRBState),
+        VMSTATE_VBUFFER_ALLOC_UINT32(mig.rsp_tmp, CRBState, 0, NULL,
+                                     mig.rsp_size),
+        VMSTATE_UINT32(mig.rsp_offset, CRBState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_tpm_crb = {
     .name = "tpm-crb",
     .pre_save = tpm_crb_pre_save,
     .fields = (const VMStateField[]) {
         VMSTATE_UINT32_ARRAY(regs, CRBState, TPM_CRB_R_MAX),
         VMSTATE_END_OF_LIST(),
+    },
+    .subsections = (const VMStateDescription * const []) {
+        &vmstate_tpm_crb_chunk,
+        NULL,
     }
 };
 
 static const Property tpm_crb_properties[] = {
     DEFINE_PROP_TPMBE("tpmdev", CRBState, tpmbe),
     DEFINE_PROP_BOOL("ppi", CRBState, ppi_enabled, true),
+    DEFINE_PROP_BOOL("migrate-buffers", CRBState, migrate_buffers, true),
 };
 
 static void tpm_crb_reset(void *dev)
-- 
2.53.0

[RFC v2 7/7] hw/tpm: Increase TPM TIS max buffer size to 8192

[Arun Menon]

- Double the size from 4096 to 8192 so that we can have bigger buffer
  enabling support for PQC algorithms in the TPM TIS interface.
- v185 of TCG TPM rolls out PQC algorithm support. [1]

[1] section 46 https://members.trustedcomputinggroup.org/wg/TCG/document/previewpdf/45151

Signed-off-by: Arun Menon <armenon@redhat.com>
---
 hw/tpm/tpm_tis.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h
index 184632ff66..0df35d5a54 100644
--- a/hw/tpm/tpm_tis.h
+++ b/hw/tpm/tpm_tis.h
@@ -33,7 +33,7 @@
 
 #define TPM_TIS_IS_VALID_LOCTY(x)   ((x) < TPM_TIS_NUM_LOCALITIES)
 
-#define TPM_TIS_BUFFER_MAX          4096
+#define TPM_TIS_BUFFER_MAX          8192
 
 typedef enum {
     TPM_TIS_STATE_IDLE = 0,
-- 
2.53.0

Re: [RFC v2 7/7] hw/tpm: Increase TPM TIS max buffer size to 8192

[Stefan Berger]

On 3/19/26 9:53 AM, Arun Menon wrote:
> - Double the size from 4096 to 8192 so that we can have bigger buffer
>    enabling support for PQC algorithms in the TPM TIS interface.
> - v185 of TCG TPM rolls out PQC algorithm support. [1]
> 
> [1] section 46 https://members.trustedcomputinggroup.org/wg/TCG/document/previewpdf/45151
> 
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>   hw/tpm/tpm_tis.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h
> index 184632ff66..0df35d5a54 100644
> --- a/hw/tpm/tpm_tis.h
> +++ b/hw/tpm/tpm_tis.h
> @@ -33,7 +33,7 @@
>   
>   #define TPM_TIS_IS_VALID_LOCTY(x)   ((x) < TPM_TIS_NUM_LOCALITIES)
>   
> -#define TPM_TIS_BUFFER_MAX          4096
> +#define TPM_TIS_BUFFER_MAX          8192

Unfortunately TIS uses a fixed-size buffer that would now become bigger:

typedef struct TPMState {
     MemoryRegion mmio;

     unsigned char buffer[TPM_TIS_BUFFER_MAX];  <-- now 8192; before 4096


static const VMStateDescription vmstate_tpm_tis_isa = {
     .name = "tpm-tis",
     .version_id = 0,
     .pre_save  = tpm_tis_pre_save_isa,
     .fields = (const VMStateField[]) {
         VMSTATE_BUFFER(state.buffer, TPMStateISA),    <-- now 8192; 
before 4096
         VMSTATE_UINT16(state.rw_offset, TPMStateISA),

Problem would be if an older version of the TIS (with size 4096) then 
receives this 8192 buffer, we would (probably) get a buffer overflow.



>   
>   typedef enum {
>       TPM_TIS_STATE_IDLE = 0,

Re: [RFC v2 3/7] hw/tpm: Add internal buffer state for chunking

[marcandre.lureau]

On Thu, 19 Mar 2026 19:23:12 +0530, Arun Menon <armenon@redhat.com> wrote:

Hi

> - Introduce GByteArray buffers to hold the command request and response
>   data during chunked TPM CRB transactions.
> - Add helper function to clean them.
> 

I would introduce them where they are actually used, but ok

>
>
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index bc559087867..5ea1a4a9706 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -306,6 +319,9 @@ static void tpm_crb_realize(DeviceState *dev, Error **errp)
>      memory_region_add_subregion(get_system_memory(),
>          TPM_CRB_ADDR_BASE + sizeof(s->regs), &s->cmdmem);
>  
> +    s->command_buffer = g_byte_array_new();
> +    s->response_buffer = g_byte_array_new();

These buffers are allocated here but never freed. Please add an
unrealize or finalize handler that calls g_byte_array_unref() on
both buffers.

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [RFC v2 4/7] hw/tpm: Implement TPM CRB chunking logic

[marcandre.lureau]

On Thu, 19 Mar 2026 19:23:13 +0530, Arun Menon <armenon@redhat.com> wrote:

Hi,

>
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index 5ea1a4a9706..e61c04aee0b 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -80,6 +82,8 @@ enum crb_ctrl_req {
>  
>  enum crb_start {
>      CRB_START_INVOKE = BIT(0),
> +    CRB_START_RESP_RETRY = BIT(1),

Hmm, maybe we should rename that field, since it is "Start" in the spec (it changed?).

> +    CRB_START_NEXT_CHUNK = BIT(2),
>  };

And follow the spec naming here "RspRetry" -> RSP_RETRY

> @@ -122,6 +126,68 @@ static uint8_t tpm_crb_get_active_locty(CRBState *s)
> [ ... skip 23 lines ... ]
> +            ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS, tpmSts, 1);
> +            ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, invoke, 0);
> +            ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, nextChunk, 0);
> +            tpm_crb_clear_internal_buffers(s);
> +            error_report("Command size '%d' less than TPM header size '%d'",
> +                         total_request_size, TPM_HEADER_SIZE);

Use PRIu32 to avoid sign sign-mismatch

> [ ... skip 28 lines ... ]
> +    if (to_copy < CRB_CTRL_CMD_SIZE) {
> +        memset((guint8 *)mem + to_copy, 0, CRB_CTRL_CMD_SIZE - to_copy);
> +    }
> +
> +    s->response_offset += to_copy;
> +    memory_region_set_dirty(&s->cmdmem, 0, CRB_CTRL_CMD_SIZE);

Those lines are repeated below and could be factorized.

> @@ -152,20 +218,48 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
> [ ... skip 21 lines ... ]
> +            if (!(s->regs[R_CRB_CTRL_START] & CRB_START_INVOKE)) {
> +                if (!tpm_crb_append_command_request(s)) {
> +                    break;
> +                }
> +                ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, invoke, 1);
> +                g_byte_array_set_size(s->response_buffer, s->be_buffer_size);

This pre-allocates the response buffer before the backend has written
anything. Since response_buffer->len > 0 now, a subsequent nextChunk
from the guest would enter tpm_crb_fill_command_response() and serve
uninitialized data.

Before this patch we would serve guest input data, which wasn't great either.


We could add a separate flag to track response readiness instead. This
will need to be migrated too.

> +                s->cmd = (TPMBackendCmd) {
> +                    .in = s->command_buffer->data,
> +                    .in_len = s->command_buffer->len,
> +                    .out = s->response_buffer->data,
> +                    .out_len = s->response_buffer->len,
> +                };
> +                tpm_backend_deliver_request(s->tpmbe, &s->cmd);
> +            }
> +        } else if (val & CRB_START_NEXT_CHUNK) {
> +            /*
> +             * nextChunk is used both while sending and receiving data.
> +             * To distinguish between the two, response_buffer is checked

Missing .

> +             * If it does not have data, then that means we have not yet
> +             * sent the command to the tpm backend, and therefore call
> +             * tpm_crb_append_command_request()
> +             */
> +            if (s->response_buffer->len > 0 &&
> +                s->response_offset < s->response_buffer->len) {
> +                    tpm_crb_fill_command_response(s);

Indentation <4

> +            } else {
> +                if (!tpm_crb_append_command_request(s)) {
> +                    break;
> +                }
> +            }
> +            ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, nextChunk, 0);
> +        } else if (val & CRB_START_RESP_RETRY) {
> +            if (s->response_buffer->len > 0) {

I'd suggest adding a trace here.

> @@ -205,13 +299,36 @@ static const MemoryRegionOps tpm_crb_memory_ops = {
> [ ... skip 15 lines ... ]
> +
> +        /*
> +         * Send the first chunk. Subsequent chunks will be sent using
> +         * tpm_crb_fill_command_response()
> +         */
> +        uint32_t to_copy = MIN(CRB_CTRL_CMD_SIZE, s->response_buffer->len);

QEMU coding style: declaration not mixed with statements.

> +        memcpy(mem, s->response_buffer->data, to_copy);
> +
> +        if (to_copy < CRB_CTRL_CMD_SIZE) {
> +            memset((guint8 *)mem + to_copy, 0, CRB_CTRL_CMD_SIZE - to_copy);
> +        }
> +        s->response_offset += to_copy;
>      }
>      memory_region_set_dirty(&s->cmdmem, 0, CRB_CTRL_CMD_SIZE);

Consider factorizing

> +    ARRAY_FIELD_DP32(s->regs, CRB_CTRL_START, invoke, 0);

redundant clear

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [RFC v2 5/7] test/qtest: Add test for tpm crb chunking

[marcandre.lureau]

On Thu, 19 Mar 2026 19:23:14 +0530, Arun Menon <armenon@redhat.com> wrote:

Hi

>
> diff --git a/tests/qtest/tpm-util.c b/tests/qtest/tpm-util.c
> index 2cb2dd47961..dfc1fd70b7f 100644
> --- a/tests/qtest/tpm-util.c
> +++ b/tests/qtest/tpm-util.c
> @@ -14,16 +14,42 @@
>  
>  #include "qemu/osdep.h"
>  #include <glib/gstdio.h>
> +#include "system/memory.h"

Use qemu/bswap.h instead

>  
>  #include "hw/acpi/tpm.h"
>  #include "libqtest.h"
>  #include "tpm-util.h"
>  #include "qobject/qdict.h"
>  
> +#define CRB_ADDR_START (TPM_CRB_ADDR_BASE + A_CRB_CTRL_START)
> +#define CRB_CTRL_STS (TPM_CRB_ADDR_BASE + A_CRB_CTRL_STS)
> +#define CRB_CTRL_CMD_SIZE (TPM_CRB_ADDR_BASE + A_CRB_CTRL_CMD_SIZE)

Shadows tpm.h macro. Use CRB_ADDR_CTRL_STS ?

> +
> +#define BIT_START_INVOKE  (1 << 0)

Similar (although in tpm_crb.c unit)

> [ ... skip 11 lines ... ]
> +            break;
> +        }
> +        if (g_get_monotonic_time() >= end_time) {
> +            break;
> +        }
> +    }

Consider adding a g_assert() here

> @@ -31,24 +57,74 @@ void tpm_util_crb_transfer(QTestState *s,
> [ ... skip 30 lines ... ]
> +
> +    qtest_writeb(s, TPM_CRB_ADDR_BASE + A_CRB_LOC_CTRL, 1);
> +
> +    for (size_t i = 0 ; i < req_size; i += chunk_size) {
> +        bool last_chunk = false;
> +        size_t current_chunk_size = chunk_size ;

Trailing whitespace before the semicolon.

> +
> +        if (i + chunk_size > req_size) {
> +            last_chunk = true;
> +            current_chunk_size = req_size - i;
>          }
> -        if (g_get_monotonic_time() >= end_time) {
> -            break;
> +
> +        qtest_memwrite(s, caddr, req + i, current_chunk_size);
> +
> +        if (last_chunk) {
> +            qtest_writel(s, CRB_ADDR_START , BIT_START_INVOKE);

Trailing whitespace before the comma.

Style: should be tpm_sts per QEMU naming conventions.
Style: declarations should be at the top of the function.
Same: declaration mixed with statements.

Style: should be tpm_sts per QEMU naming conventions.
Style: declarations should be at the top of the function.
Same: declaration mixed with statements.

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [RFC v2 6/7] hw/tpm: Add support for VM migration with TPM CRB chunking

[marcandre.lureau]

On Thu, 19 Mar 2026 19:23:15 +0530, Arun Menon <armenon@redhat.com> wrote:

Hi

>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 6cf0e2f404e..fcd6043c992 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -40,6 +40,7 @@
>  
>  GlobalProperty hw_compat_10_2[] = {

Let's not forget to update this to 11.0 for 11.1 :)

>
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index e61c04aee0b..9ce342fe8ac 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -33,6 +33,17 @@
>  #include "trace.h"
>  #include "qom/object.h"
>  
> +/* command and response buffers; part of VM state when migrating */
> +typedef struct TPMCRBMigState {
> +    uint32_t cmd_size;
> +    uint8_t *cmd_tmp;

Could we name them after the name of the state fields?
command_buffer_len && command_buffer_data ?

> +
> +    uint32_t rsp_size;
> +    uint8_t *rsp_tmp;

(response_buffer_len ..)

Actually, you should not need an extra MigState at all. Check how
GByteArray are handled in in ui/vdagent.c for example. (I have a doubt
that it may eventually leak if loading for a running state, where data
!= NULL though, I would have to check - we may want to use realloc for
buffers)

> @@ -347,18 +361,118 @@ static int tpm_crb_pre_save(void *opaque)
> [ ... skip 30 lines ... ]
> +    } else {
> +        s->mig.rsp_tmp = NULL;
> +        s->mig.rsp_size = 0;
> +    }
> +    s->mig.rsp_offset = (uint32_t)s->response_offset;
> +    return 0;

If you get rid of MigState, the original response_offset field will
proably have to be uint32_t too.

> [ ... skip 14 lines ... ]
> +        g_free(s->mig.cmd_tmp);
> +        s->mig.cmd_tmp = NULL;
> +        g_free(s->mig.rsp_tmp);
> +        s->mig.rsp_tmp = NULL;
> +        return false;
> +    }

Suggesting g_clear_pointer(&ptr, g_free)

> +
> +    if (s->mig.cmd_tmp) {
> +        if (s->command_buffer) {
> +            g_byte_array_unref(s->command_buffer);
> +        }

Slightly neater:
g_clear_pointer(&s->command_buffer, g_byte_array_unref);

> +        s->command_buffer = g_byte_array_new_take(s->mig.cmd_tmp,
> +                                                  s->mig.cmd_size);
> +        s->mig.cmd_tmp = NULL;
> +    } else {
> +        if (s->command_buffer) {
> +            g_byte_array_set_size(s->command_buffer, 0);
> +        }
> +    }
> +    if (s->mig.rsp_tmp) {
> +        if (s->response_buffer) {
> +            g_byte_array_unref(s->response_buffer);
> +        }

Or you could simply without condition:

g_clear_pointer(&s->command_buffer, g_byte_array_unref);
s->command_buffer = g_byte_array_new_take(s->mig.cmd_tmp, s->mig.cmd_size);

This will also reset the size to 0 if cmd_tmp is NULL and cmd_size is 0.

> +        s->response_buffer = g_byte_array_new_take(s->mig.rsp_tmp,
> +                                                   s->mig.rsp_size);
> +        s->mig.rsp_tmp = NULL;
> +    }

Missing
    s->response_offset = s->mig.rsp_offset;

> +    return true;
> +}
> +
> +static const VMStateDescription vmstate_tpm_crb_chunk = {
> +    .name = "tpm-crb/chunk",
> +    .version_id = 1,
> +    .needed = tpm_crb_chunk_needed,
> +    .pre_save = tpm_crb_chunk_pre_save,
> +    .post_load_errp = tpm_crb_chunk_post_load,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_UINT32(mig.cmd_size, CRBState),

Could be version 0, I think.

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [RFC v2 5/7] test/qtest: Add test for tpm crb chunking

[Marc-André Lureau]

Hi

On Thu, Mar 26, 2026 at 3:27 PM <marcandre.lureau@redhat.com> wrote:
>
> On Thu, 19 Mar 2026 19:23:14 +0530, Arun Menon <armenon@redhat.com> wrote:
>
> Hi
>
> >
> > diff --git a/tests/qtest/tpm-util.c b/tests/qtest/tpm-util.c
> > index 2cb2dd47961..dfc1fd70b7f 100644
> > --- a/tests/qtest/tpm-util.c
> > +++ b/tests/qtest/tpm-util.c
> > @@ -14,16 +14,42 @@
> >
> >  #include "qemu/osdep.h"
> >  #include <glib/gstdio.h>
> > +#include "system/memory.h"
>
> Use qemu/bswap.h instead
>
> >
> >  #include "hw/acpi/tpm.h"
> >  #include "libqtest.h"
> >  #include "tpm-util.h"
> >  #include "qobject/qdict.h"
> >
> > +#define CRB_ADDR_START (TPM_CRB_ADDR_BASE + A_CRB_CTRL_START)
> > +#define CRB_CTRL_STS (TPM_CRB_ADDR_BASE + A_CRB_CTRL_STS)
> > +#define CRB_CTRL_CMD_SIZE (TPM_CRB_ADDR_BASE + A_CRB_CTRL_CMD_SIZE)
>
> Shadows tpm.h macro. Use CRB_ADDR_CTRL_STS ?
>
> > +
> > +#define BIT_START_INVOKE  (1 << 0)
>
> Similar (although in tpm_crb.c unit)
>
> > [ ... skip 11 lines ... ]
> > +            break;
> > +        }
> > +        if (g_get_monotonic_time() >= end_time) {
> > +            break;
> > +        }
> > +    }
>
> Consider adding a g_assert() here
>
> > @@ -31,24 +57,74 @@ void tpm_util_crb_transfer(QTestState *s,
> > [ ... skip 30 lines ... ]
> > +
> > +    qtest_writeb(s, TPM_CRB_ADDR_BASE + A_CRB_LOC_CTRL, 1);
> > +
> > +    for (size_t i = 0 ; i < req_size; i += chunk_size) {
> > +        bool last_chunk = false;
> > +        size_t current_chunk_size = chunk_size ;
>
> Trailing whitespace before the semicolon.
>
> > +
> > +        if (i + chunk_size > req_size) {
> > +            last_chunk = true;
> > +            current_chunk_size = req_size - i;
> >          }
> > -        if (g_get_monotonic_time() >= end_time) {
> > -            break;
> > +
> > +        qtest_memwrite(s, caddr, req + i, current_chunk_size);
> > +
> > +        if (last_chunk) {
> > +            qtest_writel(s, CRB_ADDR_START , BIT_START_INVOKE);
>
> Trailing whitespace before the comma.
>
> Style: should be tpm_sts per QEMU naming conventions.
> Style: declarations should be at the top of the function.
> Same: declaration mixed with statements.
>
> Style: should be tpm_sts per QEMU naming conventions.
> Style: declarations should be at the top of the function.
> Same: declaration mixed with statements.
>

This is the first time I used "b4 review tui", this is strange. I'll
check what happened :)

I'd recommend others to try it, especially those like me who lack a
good "all-in-terminal" workflow for reviews.