* [PATCH] tpm: Dynamically allocate tpm-tis buffer @ 2026-04-27 20:01 Arun Menon 2026-04-27 20:49 ` Stefan Berger 0 siblings, 1 reply; 10+ messages in thread From: Arun Menon @ 2026-04-27 20:01 UTC (permalink / raw) To: qemu-devel; +Cc: Stefan Berger, Arun Menon From: Arun Menon <armenon@redhat.com> The TPM TIS buffer is currently a fixed-size static array. Change this to a dynamically allocated heap block. The buffer size is now determined at runtime by querying the TPM backend. To support VM migration, 1. Replace the static VMSTATE_BUFFER macro with pointer-based variant VMSTATE_BUFFER_POINTER_UNSAFE, explicitly mentioning the size. 2. Introduce ext_buffer and ext_size in the migration subsection to track allocation exceeding TPM_TIS_BUFFER_MAX. Allocate ext_buffer using VMSTATE_VBUFFER_ALLOC_UINT32 only to be freed later after it is appended to the main buffer. This allows us to migrate to a destination host without breaking backward compatibility. Old QEMU does not include a size field along with the buffer in the migration stream, and therefore the new QEMU is also forced to keep expecting exactly 4096 bytes. Implement a post_load hook that will validate the incoming data size from the migration stream, failing the migration if it exceeds the destination backend capacity. Add unrealize functions for the TIS interface types ISA, SysBus and I2C to ensure that the buffer is safely freed on device destruction. Signed-off-by: Arun Menon <armenon@redhat.com> --- Dependencies: This patch depends on the following patch currently in the mailing list: https://lore.kernel.org/qemu-devel/20260422103018.123608-10-armenon@redhat.com/ Depends-on: <20260422103018.123608-10-armenon@redhat.com> hw/tpm/tpm_tis.h | 6 ++++- hw/tpm/tpm_tis_common.c | 56 +++++++++++++++++++++++++++++++++++------ hw/tpm/tpm_tis_i2c.c | 28 +++++++++++++++++++-- hw/tpm/tpm_tis_isa.c | 31 +++++++++++++++++++++-- hw/tpm/tpm_tis_sysbus.c | 32 +++++++++++++++++++++-- 5 files changed, 138 insertions(+), 15 deletions(-) diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h index b2d9c0116c..c736ecedc1 100644 --- a/hw/tpm/tpm_tis.h +++ b/hw/tpm/tpm_tis.h @@ -56,7 +56,9 @@ typedef struct TPMLocality { typedef struct TPMState { MemoryRegion mmio; - unsigned char buffer[TPM_TIS_BUFFER_MAX]; + uint8_t *buffer; + uint8_t *ext_buffer; + uint32_t ext_size; uint16_t rw_offset; uint8_t active_locty; @@ -82,6 +84,8 @@ extern const VMStateDescription vmstate_locty; extern const MemoryRegionOps tpm_tis_memory_ops; int tpm_tis_pre_save(TPMState *s); +int tpm_tis_post_load(TPMState *s); +int tpm_tis_ext_buffer_post_load(TPMState *s); void tpm_tis_reset(TPMState *s, bool ppi_enabled); enum TPMVersion tpm_tis_get_tpm_version(TPMState *s); void tpm_tis_request_completed(TPMState *s, int ret); diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c index 43e68410f8..c9c4dd1190 100644 --- a/hw/tpm/tpm_tis_common.c +++ b/hw/tpm/tpm_tis_common.c @@ -270,7 +270,7 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty) uint16_t len; if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) { - len = MIN(tpm_cmd_get_size(&s->buffer), + len = MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size); ret = s->buffer[s->rw_offset++]; @@ -317,7 +317,7 @@ static void tpm_tis_dump_state(TPMState *s, hwaddr addr) "tpm_tis: result buffer : ", s->rw_offset); for (idx = 0; - idx < MIN(tpm_cmd_get_size(&s->buffer), s->be_buffer_size); + idx < MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size); idx++) { printf("%c%02x%s", s->rw_offset == idx ? '>' : ' ', @@ -383,7 +383,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr, if (s->active_locty == locty) { if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) { val = TPM_TIS_BURST_COUNT( - MIN(tpm_cmd_get_size(&s->buffer), + MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size) - s->rw_offset) | s->loc[locty].sts; } else { @@ -754,7 +754,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr, /* we have a packet length - see if we have all of it */ bool need_irq = !(s->loc[locty].sts & TPM_TIS_STS_VALID); - len = tpm_cmd_get_size(&s->buffer); + len = tpm_cmd_get_size(s->buffer); if (len > s->rw_offset) { tpm_tis_sts_set(&s->loc[locty], TPM_TIS_STS_EXPECT | TPM_TIS_STS_VALID); @@ -818,9 +818,10 @@ void tpm_tis_reset(TPMState *s, bool ppi_enabled) int c; s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver); - s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver), - TPM_TIS_BUFFER_MAX); + s->be_buffer_size = tpm_backend_get_buffer_size(s->be_driver); + s->buffer = g_realloc(s->buffer, MAX(s->be_buffer_size, + TPM_TIS_BUFFER_MAX)); if (ppi_enabled) { tpm_ppi_reset(&s->ppi); } @@ -873,6 +874,45 @@ int tpm_tis_pre_save(TPMState *s) */ tpm_backend_finish_sync(s->be_driver); + if (s->be_buffer_size > TPM_TIS_BUFFER_MAX) { + s->ext_size = s->be_buffer_size - TPM_TIS_BUFFER_MAX; + s->ext_buffer = s->buffer + TPM_TIS_BUFFER_MAX; + } else { + s->ext_size = 0; + s->ext_buffer = NULL; + } + return 0; +} + +int tpm_tis_post_load(TPMState *s) +{ + if (s->rw_offset > s->be_buffer_size) { + return -EINVAL; + } + return 0; +} + +int tpm_tis_ext_buffer_post_load(TPMState *s) +{ + /* + * Calculate the maximum extension buffer size allowed, by comparing + * the destination VM's backend capacity with TPM_TIS_BUFFER_MAX. + */ + uint32_t max_ext = s->be_buffer_size > TPM_TIS_BUFFER_MAX ? + s->be_buffer_size - TPM_TIS_BUFFER_MAX : 0; + + if (s->ext_size > max_ext) { + /* + * Source buffer size is greater than what the destination backend + * allows + */ + g_clear_pointer(&s->ext_buffer, g_free); + return -EINVAL; + } + if (s->ext_size > 0) { + memcpy(s->buffer + TPM_TIS_BUFFER_MAX, s->ext_buffer, s->ext_size); + g_clear_pointer(&s->ext_buffer, g_free); + } return 0; } @@ -901,7 +941,7 @@ bool tpm_tis_ext_buffer_migration_needed(struct TPMState *s) case TPM_TIS_STATE_READY: return false; case TPM_TIS_STATE_RECEPTION: - return s->rw_offset >= 4096; + return s->rw_offset >= TPM_TIS_BUFFER_MAX; case TPM_TIS_STATE_EXECUTION: /* * TPM is executing: we cannot know the size of TPM response. @@ -909,7 +949,7 @@ bool tpm_tis_ext_buffer_migration_needed(struct TPMState *s) */ return false; case TPM_TIS_STATE_COMPLETION: - return (tpm_cmd_get_size(&s->buffer) >= 4096); + return (tpm_cmd_get_size(s->buffer) >= TPM_TIS_BUFFER_MAX); } return false; } diff --git a/hw/tpm/tpm_tis_i2c.c b/hw/tpm/tpm_tis_i2c.c index f48938e3a1..41a5486497 100644 --- a/hw/tpm/tpm_tis_i2c.c +++ b/hw/tpm/tpm_tis_i2c.c @@ -103,6 +103,10 @@ static int tpm_tis_i2c_post_load(void *opaque, int version_id) { TPMStateI2C *i2cst = opaque; + if (tpm_tis_post_load(&i2cst->state) < 0) { + return -1; + } + if (i2cst->offset >= 1) { tpm_tis_i2c_to_tis_reg(i2cst, i2cst->data[0]); } @@ -117,13 +121,23 @@ static bool tpm_tis_ext_buffer_migration_needed_i2c(void *opaque) return tpm_tis_ext_buffer_migration_needed(&i2cst->state); } +static int tpm_tis_ext_buffer_post_load_i2c(void *opaque, int version_id) +{ + TPMStateI2C *i2cst = opaque; + + return tpm_tis_ext_buffer_post_load(&i2cst->state); +} + static const VMStateDescription vmstate_tpm_tis_ext_buffer_i2c = { .name = "tpm-tis/ext_buffer", .version_id = 0, .needed = tpm_tis_ext_buffer_migration_needed_i2c, .pre_save = tpm_tis_i2c_pre_save, + .post_load = tpm_tis_ext_buffer_post_load_i2c, .fields = (const VMStateField[]) { - VMSTATE_BUFFER_START_MIDDLE(state.buffer, TPMStateI2C, 4096), + VMSTATE_UINT32(state.ext_size, TPMStateI2C), + VMSTATE_VBUFFER_ALLOC_UINT32(state.ext_buffer, TPMStateI2C, 0, NULL, + state.ext_size), VMSTATE_END_OF_LIST() } }; @@ -134,7 +148,8 @@ static const VMStateDescription vmstate_tpm_tis_i2c = { .pre_save = tpm_tis_i2c_pre_save, .post_load = tpm_tis_i2c_post_load, .fields = (const VMStateField[]) { - VMSTATE_PARTIAL_BUFFER(state.buffer, TPMStateI2C, 4096), + VMSTATE_BUFFER_POINTER_UNSAFE(state.buffer, TPMStateI2C, 0, + TPM_TIS_BUFFER_MAX), VMSTATE_UINT16(state.rw_offset, TPMStateI2C), VMSTATE_UINT8(state.active_locty, TPMStateI2C), VMSTATE_UINT8(state.aborting_locty, TPMStateI2C), @@ -535,6 +550,14 @@ static void tpm_tis_i2c_realizefn(DeviceState *dev, Error **errp) } } +static void tpm_tis_i2c_unrealizefn(DeviceState *dev) +{ + TPMStateI2C *i2cst = TPM_TIS_I2C(dev); + TPMState *state = &i2cst->state; + + g_clear_pointer(&state->buffer, g_free); +} + static void tpm_tis_i2c_reset(DeviceState *dev) { TPMStateI2C *i2cst = TPM_TIS_I2C(dev); @@ -555,6 +578,7 @@ static void tpm_tis_i2c_class_init(ObjectClass *klass, const void *data) TPMIfClass *tc = TPM_IF_CLASS(klass); dc->realize = tpm_tis_i2c_realizefn; + dc->unrealize = tpm_tis_i2c_unrealizefn; device_class_set_legacy_reset(dc, tpm_tis_i2c_reset); dc->vmsd = &vmstate_tpm_tis_i2c; device_class_set_props(dc, tpm_tis_i2c_properties); diff --git a/hw/tpm/tpm_tis_isa.c b/hw/tpm/tpm_tis_isa.c index 4999de1c61..7fbdfb96e6 100644 --- a/hw/tpm/tpm_tis_isa.c +++ b/hw/tpm/tpm_tis_isa.c @@ -49,6 +49,12 @@ static int tpm_tis_pre_save_isa(void *opaque) return tpm_tis_pre_save(&isadev->state); } +static int tpm_tis_post_load_isa(void *opaque, int version_id) +{ + TPMStateISA *isadev = opaque; + return tpm_tis_post_load(&isadev->state); +} + static bool tpm_tis_ext_buffer_migration_needed_isa(void *opaque) { TPMStateISA *isadev = opaque; @@ -56,13 +62,23 @@ static bool tpm_tis_ext_buffer_migration_needed_isa(void *opaque) return tpm_tis_ext_buffer_migration_needed(&isadev->state); } +static int tpm_tis_ext_buffer_post_load_isa(void *opaque, int version_id) +{ + TPMStateISA *isadev = opaque; + + return tpm_tis_ext_buffer_post_load(&isadev->state); +} + static const VMStateDescription vmstate_tpm_tis_ext_buffer_isa = { .name = "tpm-tis/ext_buffer", .version_id = 0, .needed = tpm_tis_ext_buffer_migration_needed_isa, .pre_save = tpm_tis_pre_save_isa, + .post_load = tpm_tis_ext_buffer_post_load_isa, .fields = (const VMStateField[]) { - VMSTATE_BUFFER_START_MIDDLE(state.buffer, TPMStateISA, 4096), + VMSTATE_UINT32(state.ext_size, TPMStateISA), + VMSTATE_VBUFFER_ALLOC_UINT32(state.ext_buffer, TPMStateISA, 0, NULL, + state.ext_size), VMSTATE_END_OF_LIST() } }; @@ -71,8 +87,10 @@ static const VMStateDescription vmstate_tpm_tis_isa = { .name = "tpm-tis", .version_id = 0, .pre_save = tpm_tis_pre_save_isa, + .post_load = tpm_tis_post_load_isa, .fields = (const VMStateField[]) { - VMSTATE_PARTIAL_BUFFER(state.buffer, TPMStateISA, 4096), + VMSTATE_BUFFER_POINTER_UNSAFE(state.buffer, TPMStateISA, 0, + TPM_TIS_BUFFER_MAX), VMSTATE_UINT16(state.rw_offset, TPMStateISA), VMSTATE_UINT8(state.active_locty, TPMStateISA), VMSTATE_UINT8(state.aborting_locty, TPMStateISA), @@ -157,6 +175,14 @@ static void tpm_tis_isa_realizefn(DeviceState *dev, Error **errp) TPM_PPI_ADDR_BASE, OBJECT(dev)); } +static void tpm_tis_isa_unrealizefn(DeviceState *dev) +{ + TPMStateISA *isadev = TPM_TIS_ISA(dev); + TPMState *state = &isadev->state; + + g_clear_pointer(&state->buffer, g_free); +} + static void build_tpm_tis_isa_aml(AcpiDevAmlIf *adev, Aml *scope) { Aml *dev, *crs; @@ -196,6 +222,7 @@ static void tpm_tis_isa_class_init(ObjectClass *klass, const void *data) tc->model = TPM_MODEL_TPM_TIS; tc->ppi_enabled = true; dc->realize = tpm_tis_isa_realizefn; + dc->unrealize = tpm_tis_isa_unrealizefn; device_class_set_legacy_reset(dc, tpm_tis_isa_reset); tc->request_completed = tpm_tis_isa_request_completed; tc->get_version = tpm_tis_isa_get_tpm_version; diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c index c29f43bdce..ad8cfa85b1 100644 --- a/hw/tpm/tpm_tis_sysbus.c +++ b/hw/tpm/tpm_tis_sysbus.c @@ -49,6 +49,13 @@ static int tpm_tis_pre_save_sysbus(void *opaque) return tpm_tis_pre_save(&sbdev->state); } +static int tpm_tis_post_load_sysbus(void *opaque, int version_id) +{ + TPMStateSysBus *sbdev = opaque; + + return tpm_tis_post_load(&sbdev->state); +} + static bool tpm_tis_ext_buffer_migration_needed_sysbus(void *opaque) { TPMStateSysBus *sbdev = opaque; @@ -56,13 +63,23 @@ static bool tpm_tis_ext_buffer_migration_needed_sysbus(void *opaque) return tpm_tis_ext_buffer_migration_needed(&sbdev->state); } +static int tpm_tis_ext_buffer_post_load_sysbus(void *opaque, int version_id) +{ + TPMStateSysBus *sbdev = opaque; + + return tpm_tis_ext_buffer_post_load(&sbdev->state); +} + static const VMStateDescription vmstate_tpm_tis_ext_buffer_sysbus = { .name = "tpm-tis/ext_buffer", .version_id = 0, .needed = tpm_tis_ext_buffer_migration_needed_sysbus, .pre_save = tpm_tis_pre_save_sysbus, + .post_load = tpm_tis_ext_buffer_post_load_sysbus, .fields = (const VMStateField[]) { - VMSTATE_BUFFER_START_MIDDLE(state.buffer, TPMStateSysBus, 4096), + VMSTATE_UINT32(state.ext_size, TPMStateSysBus), + VMSTATE_VBUFFER_ALLOC_UINT32(state.ext_buffer, TPMStateSysBus, 0, + NULL, state.ext_size), VMSTATE_END_OF_LIST() } }; @@ -71,8 +88,10 @@ static const VMStateDescription vmstate_tpm_tis_sysbus = { .name = "tpm-tis", .version_id = 0, .pre_save = tpm_tis_pre_save_sysbus, + .post_load = tpm_tis_post_load_sysbus, .fields = (const VMStateField[]) { - VMSTATE_PARTIAL_BUFFER(state.buffer, TPMStateSysBus, 4096), + VMSTATE_BUFFER_POINTER_UNSAFE(state.buffer, TPMStateSysBus, 0, + TPM_TIS_BUFFER_MAX), VMSTATE_UINT16(state.rw_offset, TPMStateSysBus), VMSTATE_UINT8(state.active_locty, TPMStateSysBus), VMSTATE_UINT8(state.aborting_locty, TPMStateSysBus), @@ -156,6 +175,14 @@ static void tpm_tis_sysbus_realizefn(DeviceState *dev, Error **errp) vmstate_register_ram(&s->ppi.ram, dev); } +static void tpm_tis_sysbus_unrealizefn(DeviceState *dev) +{ + TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(dev); + TPMState *state = &sbdev->state; + + g_clear_pointer(&state->buffer, g_free); +} + static void tpm_tis_sysbus_class_init(ObjectClass *klass, const void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -166,6 +193,7 @@ static void tpm_tis_sysbus_class_init(ObjectClass *klass, const void *data) tc->model = TPM_MODEL_TPM_TIS; tc->ppi_enabled = true; dc->realize = tpm_tis_sysbus_realizefn; + dc->unrealize = tpm_tis_sysbus_unrealizefn; device_class_set_legacy_reset(dc, tpm_tis_sysbus_reset); tc->request_completed = tpm_tis_sysbus_request_completed; tc->get_version = tpm_tis_sysbus_get_tpm_version; -- 2.53.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm: Dynamically allocate tpm-tis buffer 2026-04-27 20:01 [PATCH] tpm: Dynamically allocate tpm-tis buffer Arun Menon @ 2026-04-27 20:49 ` Stefan Berger 2026-04-28 7:01 ` Arun Menon 0 siblings, 1 reply; 10+ messages in thread From: Stefan Berger @ 2026-04-27 20:49 UTC (permalink / raw) To: Arun Menon, qemu-devel; +Cc: Stefan Berger On 4/27/26 4:01 PM, Arun Menon wrote: > From: Arun Menon <armenon@redhat.com> > > The TPM TIS buffer is currently a fixed-size static array. Change this > to a dynamically allocated heap block. The buffer size is now determined > at runtime by querying the TPM backend. Do we really need this? I mean for the forseeable future 8kb should be sufficient. > > To support VM migration, > 1. Replace the static VMSTATE_BUFFER macro with pointer-based variant > VMSTATE_BUFFER_POINTER_UNSAFE, explicitly mentioning the size. > 2. Introduce ext_buffer and ext_size in the migration subsection to > track allocation exceeding TPM_TIS_BUFFER_MAX. Allocate ext_buffer > using VMSTATE_VBUFFER_ALLOC_UINT32 only to be freed later after it is > appended to the main buffer. > > This allows us to migrate to a destination host without breaking > backward compatibility. Old QEMU does not include a size field along > with the buffer in the migration stream, and therefore the > new QEMU is also forced to keep expecting exactly 4096 bytes. > > Implement a post_load hook that will validate the incoming data size > from the migration stream, failing the migration if it exceeds the > destination backend capacity. Add unrealize functions for the TIS interface > types ISA, SysBus and I2C to ensure that the buffer is safely freed on > device destruction. > > Signed-off-by: Arun Menon <armenon@redhat.com> > --- > Dependencies: > This patch depends on the following patch currently in the mailing list: > https://lore.kernel.org/qemu-devel/20260422103018.123608-10-armenon@redhat.com/ I had tried to apply your v5 series to master but could not. Do you have a git repo where you keep your patches? > > Depends-on: <20260422103018.123608-10-armenon@redhat.com> > > hw/tpm/tpm_tis.h | 6 ++++- > hw/tpm/tpm_tis_common.c | 56 +++++++++++++++++++++++++++++++++++------ > hw/tpm/tpm_tis_i2c.c | 28 +++++++++++++++++++-- > hw/tpm/tpm_tis_isa.c | 31 +++++++++++++++++++++-- > hw/tpm/tpm_tis_sysbus.c | 32 +++++++++++++++++++++-- > 5 files changed, 138 insertions(+), 15 deletions(-) > > diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h > index b2d9c0116c..c736ecedc1 100644 > --- a/hw/tpm/tpm_tis.h > +++ b/hw/tpm/tpm_tis.h > @@ -56,7 +56,9 @@ typedef struct TPMLocality { > typedef struct TPMState { > MemoryRegion mmio; > > - unsigned char buffer[TPM_TIS_BUFFER_MAX]; > + uint8_t *buffer; > + uint8_t *ext_buffer; > + uint32_t ext_size; > uint16_t rw_offset; > > uint8_t active_locty; > @@ -82,6 +84,8 @@ extern const VMStateDescription vmstate_locty; > extern const MemoryRegionOps tpm_tis_memory_ops; > > int tpm_tis_pre_save(TPMState *s); > +int tpm_tis_post_load(TPMState *s); > +int tpm_tis_ext_buffer_post_load(TPMState *s); > void tpm_tis_reset(TPMState *s, bool ppi_enabled); > enum TPMVersion tpm_tis_get_tpm_version(TPMState *s); > void tpm_tis_request_completed(TPMState *s, int ret); > diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c > index 43e68410f8..c9c4dd1190 100644 > --- a/hw/tpm/tpm_tis_common.c > +++ b/hw/tpm/tpm_tis_common.c > @@ -270,7 +270,7 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty) > uint16_t len; > > if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) { > - len = MIN(tpm_cmd_get_size(&s->buffer), > + len = MIN(tpm_cmd_get_size(s->buffer), > s->be_buffer_size); > > ret = s->buffer[s->rw_offset++]; > @@ -317,7 +317,7 @@ static void tpm_tis_dump_state(TPMState *s, hwaddr addr) > "tpm_tis: result buffer : ", > s->rw_offset); > for (idx = 0; > - idx < MIN(tpm_cmd_get_size(&s->buffer), s->be_buffer_size); > + idx < MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size); > idx++) { > printf("%c%02x%s", > s->rw_offset == idx ? '>' : ' ', > @@ -383,7 +383,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr, > if (s->active_locty == locty) { > if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) { > val = TPM_TIS_BURST_COUNT( > - MIN(tpm_cmd_get_size(&s->buffer), > + MIN(tpm_cmd_get_size(s->buffer), > s->be_buffer_size) > - s->rw_offset) | s->loc[locty].sts; > } else { > @@ -754,7 +754,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr, > /* we have a packet length - see if we have all of it */ > bool need_irq = !(s->loc[locty].sts & TPM_TIS_STS_VALID); > > - len = tpm_cmd_get_size(&s->buffer); > + len = tpm_cmd_get_size(s->buffer); > if (len > s->rw_offset) { > tpm_tis_sts_set(&s->loc[locty], > TPM_TIS_STS_EXPECT | TPM_TIS_STS_VALID); > @@ -818,9 +818,10 @@ void tpm_tis_reset(TPMState *s, bool ppi_enabled) > int c; > > s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver); > - s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver), > - TPM_TIS_BUFFER_MAX); > > + s->be_buffer_size = tpm_backend_get_buffer_size(s->be_driver); > + s->buffer = g_realloc(s->buffer, MAX(s->be_buffer_size, > + TPM_TIS_BUFFER_MAX)); With MAX() it can now be bigger than TPM_TIS_BUFFER_MAX if the backend says so -- hm... > if (ppi_enabled) { > tpm_ppi_reset(&s->ppi); > } > @@ -873,6 +874,45 @@ int tpm_tis_pre_save(TPMState *s) > */ > tpm_backend_finish_sync(s->be_driver); > > + if (s->be_buffer_size > TPM_TIS_BUFFER_MAX) { > + s->ext_size = s->be_buffer_size - TPM_TIS_BUFFER_MAX; > + s->ext_buffer = s->buffer + TPM_TIS_BUFFER_MAX; > + } else { > + s->ext_size = 0; > + s->ext_buffer = NULL; > + } > + return 0; > +} > + > +int tpm_tis_post_load(TPMState *s) > +{ > + if (s->rw_offset > s->be_buffer_size) { > + return -EINVAL; > + } > + return 0; > +} > + > +int tpm_tis_ext_buffer_post_load(TPMState *s) > +{ > + /* > + * Calculate the maximum extension buffer size allowed, by comparing > + * the destination VM's backend capacity with TPM_TIS_BUFFER_MAX. > + */ > + uint32_t max_ext = s->be_buffer_size > TPM_TIS_BUFFER_MAX ? > + s->be_buffer_size - TPM_TIS_BUFFER_MAX : 0; > + > + if (s->ext_size > max_ext) { > + /* > + * Source buffer size is greater than what the destination backend > + * allows > + */ > + g_clear_pointer(&s->ext_buffer, g_free); > + return -EINVAL; > + } > + if (s->ext_size > 0) { > + memcpy(s->buffer + TPM_TIS_BUFFER_MAX, s->ext_buffer, s->ext_size); > + g_clear_pointer(&s->ext_buffer, g_free); > + } > return 0; > } > > @@ -901,7 +941,7 @@ bool tpm_tis_ext_buffer_migration_needed(struct TPMState *s) > case TPM_TIS_STATE_READY: > return false; > case TPM_TIS_STATE_RECEPTION: > - return s->rw_offset >= 4096; > + return s->rw_offset >= TPM_TIS_BUFFER_MAX; This cannot be right. For PQC support we extended the 4096 byte buffer from 4096 bytes to 8192 byte but only want to store the 2nd 4096 bytes if necessary and to keep backwards compatibility. This function was called to determine whether more than 4096 bytes were either written to the buffer by the OS driver or received from the TPM as a response so that we now would have to send the additional 2nd 4096 bytes. But TPM_TIS_BUFFER_MAX is 8192. > case TPM_TIS_STATE_EXECUTION:> /* > * TPM is executing: we cannot know the size of TPM response. > @@ -909,7 +949,7 @@ bool tpm_tis_ext_buffer_migration_needed(struct TPMState *s) > */ > return false; > case TPM_TIS_STATE_COMPLETION: > - return (tpm_cmd_get_size(&s->buffer) >= 4096); > + return (tpm_cmd_get_size(s->buffer) >= TPM_TIS_BUFFER_MAX); Not good, either. > } > return false; > } > diff --git a/hw/tpm/tpm_tis_i2c.c b/hw/tpm/tpm_tis_i2c.c > index f48938e3a1..41a5486497 100644 > --- a/hw/tpm/tpm_tis_i2c.c > +++ b/hw/tpm/tpm_tis_i2c.c > @@ -103,6 +103,10 @@ static int tpm_tis_i2c_post_load(void *opaque, int version_id) > { > TPMStateI2C *i2cst = opaque; > > + if (tpm_tis_post_load(&i2cst->state) < 0) { > + return -1; > + } > + > if (i2cst->offset >= 1) { > tpm_tis_i2c_to_tis_reg(i2cst, i2cst->data[0]); > } > @@ -117,13 +121,23 @@ static bool tpm_tis_ext_buffer_migration_needed_i2c(void *opaque) > return tpm_tis_ext_buffer_migration_needed(&i2cst->state); > } > > +static int tpm_tis_ext_buffer_post_load_i2c(void *opaque, int version_id) > +{ > + TPMStateI2C *i2cst = opaque; > + > + return tpm_tis_ext_buffer_post_load(&i2cst->state); > +} > + > static const VMStateDescription vmstate_tpm_tis_ext_buffer_i2c = { > .name = "tpm-tis/ext_buffer", > .version_id = 0, > .needed = tpm_tis_ext_buffer_migration_needed_i2c, > .pre_save = tpm_tis_i2c_pre_save, > + .post_load = tpm_tis_ext_buffer_post_load_i2c, > .fields = (const VMStateField[]) { > - VMSTATE_BUFFER_START_MIDDLE(state.buffer, TPMStateI2C, 4096), Here we instructed the buffer to be written from 4096 bytes to 8192 bytes, so the 2nd part. I don't think your changes are changing it to anything equivalent. > + VMSTATE_UINT32(state.ext_size, TPMStateI2C), > + VMSTATE_VBUFFER_ALLOC_UINT32(state.ext_buffer, TPMStateI2C, 0, NULL, > + state.ext_size), > VMSTATE_END_OF_LIST() > } > }; > @@ -134,7 +148,8 @@ static const VMStateDescription vmstate_tpm_tis_i2c = { > .pre_save = tpm_tis_i2c_pre_save, > .post_load = tpm_tis_i2c_post_load, > .fields = (const VMStateField[]) { > - VMSTATE_PARTIAL_BUFFER(state.buffer, TPMStateI2C, 4096), This was supposed to store the first 4096 bytes of the buffer to keep backwards compatibility. And the 2nd 4096 bytes were only supposed to be written if found necessary. > + VMSTATE_BUFFER_POINTER_UNSAFE(state.buffer, TPMStateI2C, 0, > + TPM_TIS_BUFFER_MAX), > VMSTATE_UINT16(state.rw_offset, TPMStateI2C), > VMSTATE_UINT8(state.active_locty, TPMStateI2C), > VMSTATE_UINT8(state.aborting_locty, TPMStateI2C), > @@ -535,6 +550,14 @@ static void tpm_tis_i2c_realizefn(DeviceState *dev, Error **errp) > } > } > > +static void tpm_tis_i2c_unrealizefn(DeviceState *dev) > +{ > + TPMStateI2C *i2cst = TPM_TIS_I2C(dev); > + TPMState *state = &i2cst->state; > + > + g_clear_pointer(&state->buffer, g_free); > +} > + > static void tpm_tis_i2c_reset(DeviceState *dev) > { > TPMStateI2C *i2cst = TPM_TIS_I2C(dev); > @@ -555,6 +578,7 @@ static void tpm_tis_i2c_class_init(ObjectClass *klass, const void *data) > TPMIfClass *tc = TPM_IF_CLASS(klass); > > dc->realize = tpm_tis_i2c_realizefn; > + dc->unrealize = tpm_tis_i2c_unrealizefn; > device_class_set_legacy_reset(dc, tpm_tis_i2c_reset); > dc->vmsd = &vmstate_tpm_tis_i2c; > device_class_set_props(dc, tpm_tis_i2c_properties); > diff --git a/hw/tpm/tpm_tis_isa.c b/hw/tpm/tpm_tis_isa.c > index 4999de1c61..7fbdfb96e6 100644 > --- a/hw/tpm/tpm_tis_isa.c > +++ b/hw/tpm/tpm_tis_isa.c > @@ -49,6 +49,12 @@ static int tpm_tis_pre_save_isa(void *opaque) > return tpm_tis_pre_save(&isadev->state); > } > > +static int tpm_tis_post_load_isa(void *opaque, int version_id) > +{ > + TPMStateISA *isadev = opaque; > + return tpm_tis_post_load(&isadev->state); > +} > + > static bool tpm_tis_ext_buffer_migration_needed_isa(void *opaque) > { > TPMStateISA *isadev = opaque; > @@ -56,13 +62,23 @@ static bool tpm_tis_ext_buffer_migration_needed_isa(void *opaque) > return tpm_tis_ext_buffer_migration_needed(&isadev->state); > } > > +static int tpm_tis_ext_buffer_post_load_isa(void *opaque, int version_id) > +{ > + TPMStateISA *isadev = opaque; > + > + return tpm_tis_ext_buffer_post_load(&isadev->state); > +} > + > static const VMStateDescription vmstate_tpm_tis_ext_buffer_isa = { > .name = "tpm-tis/ext_buffer", > .version_id = 0, > .needed = tpm_tis_ext_buffer_migration_needed_isa, > .pre_save = tpm_tis_pre_save_isa, > + .post_load = tpm_tis_ext_buffer_post_load_isa, > .fields = (const VMStateField[]) { > - VMSTATE_BUFFER_START_MIDDLE(state.buffer, TPMStateISA, 4096), > + VMSTATE_UINT32(state.ext_size, TPMStateISA), > + VMSTATE_VBUFFER_ALLOC_UINT32(state.ext_buffer, TPMStateISA, 0, NULL, > + state.ext_size), > VMSTATE_END_OF_LIST() > } > }; > @@ -71,8 +87,10 @@ static const VMStateDescription vmstate_tpm_tis_isa = { > .name = "tpm-tis", > .version_id = 0, > .pre_save = tpm_tis_pre_save_isa, > + .post_load = tpm_tis_post_load_isa, > .fields = (const VMStateField[]) { > - VMSTATE_PARTIAL_BUFFER(state.buffer, TPMStateISA, 4096), > + VMSTATE_BUFFER_POINTER_UNSAFE(state.buffer, TPMStateISA, 0, > + TPM_TIS_BUFFER_MAX), > VMSTATE_UINT16(state.rw_offset, TPMStateISA), > VMSTATE_UINT8(state.active_locty, TPMStateISA), > VMSTATE_UINT8(state.aborting_locty, TPMStateISA), > @@ -157,6 +175,14 @@ static void tpm_tis_isa_realizefn(DeviceState *dev, Error **errp) > TPM_PPI_ADDR_BASE, OBJECT(dev)); > } > > +static void tpm_tis_isa_unrealizefn(DeviceState *dev) > +{ > + TPMStateISA *isadev = TPM_TIS_ISA(dev); > + TPMState *state = &isadev->state; > + > + g_clear_pointer(&state->buffer, g_free); > +} > + > static void build_tpm_tis_isa_aml(AcpiDevAmlIf *adev, Aml *scope) > { > Aml *dev, *crs; > @@ -196,6 +222,7 @@ static void tpm_tis_isa_class_init(ObjectClass *klass, const void *data) > tc->model = TPM_MODEL_TPM_TIS; > tc->ppi_enabled = true; > dc->realize = tpm_tis_isa_realizefn; > + dc->unrealize = tpm_tis_isa_unrealizefn; > device_class_set_legacy_reset(dc, tpm_tis_isa_reset); > tc->request_completed = tpm_tis_isa_request_completed; > tc->get_version = tpm_tis_isa_get_tpm_version; > diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c > index c29f43bdce..ad8cfa85b1 100644 > --- a/hw/tpm/tpm_tis_sysbus.c > +++ b/hw/tpm/tpm_tis_sysbus.c > @@ -49,6 +49,13 @@ static int tpm_tis_pre_save_sysbus(void *opaque) > return tpm_tis_pre_save(&sbdev->state); > } > > +static int tpm_tis_post_load_sysbus(void *opaque, int version_id) > +{ > + TPMStateSysBus *sbdev = opaque; > + > + return tpm_tis_post_load(&sbdev->state); > +} > + > static bool tpm_tis_ext_buffer_migration_needed_sysbus(void *opaque) > { > TPMStateSysBus *sbdev = opaque; > @@ -56,13 +63,23 @@ static bool tpm_tis_ext_buffer_migration_needed_sysbus(void *opaque) > return tpm_tis_ext_buffer_migration_needed(&sbdev->state); > } > > +static int tpm_tis_ext_buffer_post_load_sysbus(void *opaque, int version_id) > +{ > + TPMStateSysBus *sbdev = opaque; > + > + return tpm_tis_ext_buffer_post_load(&sbdev->state); > +} > + > static const VMStateDescription vmstate_tpm_tis_ext_buffer_sysbus = { > .name = "tpm-tis/ext_buffer", > .version_id = 0, > .needed = tpm_tis_ext_buffer_migration_needed_sysbus, > .pre_save = tpm_tis_pre_save_sysbus, > + .post_load = tpm_tis_ext_buffer_post_load_sysbus, > .fields = (const VMStateField[]) { > - VMSTATE_BUFFER_START_MIDDLE(state.buffer, TPMStateSysBus, 4096), > + VMSTATE_UINT32(state.ext_size, TPMStateSysBus), > + VMSTATE_VBUFFER_ALLOC_UINT32(state.ext_buffer, TPMStateSysBus, 0, > + NULL, state.ext_size), > VMSTATE_END_OF_LIST() > } > }; > @@ -71,8 +88,10 @@ static const VMStateDescription vmstate_tpm_tis_sysbus = { > .name = "tpm-tis", > .version_id = 0, > .pre_save = tpm_tis_pre_save_sysbus, > + .post_load = tpm_tis_post_load_sysbus, > .fields = (const VMStateField[]) { > - VMSTATE_PARTIAL_BUFFER(state.buffer, TPMStateSysBus, 4096), > + VMSTATE_BUFFER_POINTER_UNSAFE(state.buffer, TPMStateSysBus, 0, > + TPM_TIS_BUFFER_MAX), > VMSTATE_UINT16(state.rw_offset, TPMStateSysBus), > VMSTATE_UINT8(state.active_locty, TPMStateSysBus), > VMSTATE_UINT8(state.aborting_locty, TPMStateSysBus), > @@ -156,6 +175,14 @@ static void tpm_tis_sysbus_realizefn(DeviceState *dev, Error **errp) > vmstate_register_ram(&s->ppi.ram, dev); > } > > +static void tpm_tis_sysbus_unrealizefn(DeviceState *dev) > +{ > + TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(dev); > + TPMState *state = &sbdev->state; > + > + g_clear_pointer(&state->buffer, g_free); > +} > + > static void tpm_tis_sysbus_class_init(ObjectClass *klass, const void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -166,6 +193,7 @@ static void tpm_tis_sysbus_class_init(ObjectClass *klass, const void *data) > tc->model = TPM_MODEL_TPM_TIS; > tc->ppi_enabled = true; > dc->realize = tpm_tis_sysbus_realizefn; > + dc->unrealize = tpm_tis_sysbus_unrealizefn; > device_class_set_legacy_reset(dc, tpm_tis_sysbus_reset); > tc->request_completed = tpm_tis_sysbus_request_completed; > tc->get_version = tpm_tis_sysbus_get_tpm_version; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm: Dynamically allocate tpm-tis buffer 2026-04-27 20:49 ` Stefan Berger @ 2026-04-28 7:01 ` Arun Menon 2026-04-28 12:57 ` Stefan Berger 0 siblings, 1 reply; 10+ messages in thread From: Arun Menon @ 2026-04-28 7:01 UTC (permalink / raw) To: Stefan Berger; +Cc: qemu-devel, Stefan Berger Hi Stefan, Thank you for taking a look. On Mon, Apr 27, 2026 at 04:49:04PM -0400, Stefan Berger wrote: > > > On 4/27/26 4:01 PM, Arun Menon wrote: > > From: Arun Menon <armenon@redhat.com> > > > > The TPM TIS buffer is currently a fixed-size static array. Change this > > to a dynamically allocated heap block. The buffer size is now determined > > at runtime by querying the TPM backend. > > Do we really need this? I mean for the forseeable future 8kb should be > sufficient. You are right that 8kb should be sufficient. I implemented this to address the TODO mentioned here: https://github.com/qemu/qemu/commit/e5f62d87e3c03bda6006085cf6303736fb57f5c5 That is why this patch is deliberately posted outside the v5 series. It is more about future-proofing than a strict requirement. I thought it was worth addressing while the context was fresh. I am happy to leave it out if we prefer that. > > > > > To support VM migration, > > 1. Replace the static VMSTATE_BUFFER macro with pointer-based variant > > VMSTATE_BUFFER_POINTER_UNSAFE, explicitly mentioning the size. > > 2. Introduce ext_buffer and ext_size in the migration subsection to > > track allocation exceeding TPM_TIS_BUFFER_MAX. Allocate ext_buffer > > using VMSTATE_VBUFFER_ALLOC_UINT32 only to be freed later after it is > > appended to the main buffer. > > > > This allows us to migrate to a destination host without breaking > > backward compatibility. Old QEMU does not include a size field along > > with the buffer in the migration stream, and therefore the > > new QEMU is also forced to keep expecting exactly 4096 bytes. > > > > Implement a post_load hook that will validate the incoming data size > > from the migration stream, failing the migration if it exceeds the > > destination backend capacity. Add unrealize functions for the TIS interface > > types ISA, SysBus and I2C to ensure that the buffer is safely freed on > > device destruction. > > > > Signed-off-by: Arun Menon <armenon@redhat.com> > > --- > > Dependencies: > > This patch depends on the following patch currently in the mailing list: > > https://lore.kernel.org/qemu-devel/20260422103018.123608-10-armenon@redhat.com/ > > I had tried to apply your v5 series to master but could not. Do you have a > git repo where you keep your patches? Yes. Here is the repo: https://gitlab.com/armenon/qemu-dev/-/tree/pqc_tpm?ref_type=heads I have applied the GByteArray change first, followed by the 10 commits. > > > > > Depends-on: <20260422103018.123608-10-armenon@redhat.com> > > > > hw/tpm/tpm_tis.h | 6 ++++- > > hw/tpm/tpm_tis_common.c | 56 +++++++++++++++++++++++++++++++++++------ > > hw/tpm/tpm_tis_i2c.c | 28 +++++++++++++++++++-- > > hw/tpm/tpm_tis_isa.c | 31 +++++++++++++++++++++-- > > hw/tpm/tpm_tis_sysbus.c | 32 +++++++++++++++++++++-- > > 5 files changed, 138 insertions(+), 15 deletions(-) > > > > diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h > > index b2d9c0116c..c736ecedc1 100644 > > --- a/hw/tpm/tpm_tis.h > > +++ b/hw/tpm/tpm_tis.h > > @@ -56,7 +56,9 @@ typedef struct TPMLocality { > > typedef struct TPMState { > > MemoryRegion mmio; > > - unsigned char buffer[TPM_TIS_BUFFER_MAX]; > > + uint8_t *buffer; > > + uint8_t *ext_buffer; > > + uint32_t ext_size; > > uint16_t rw_offset; > > uint8_t active_locty; > > @@ -82,6 +84,8 @@ extern const VMStateDescription vmstate_locty; > > extern const MemoryRegionOps tpm_tis_memory_ops; > > int tpm_tis_pre_save(TPMState *s); > > +int tpm_tis_post_load(TPMState *s); > > +int tpm_tis_ext_buffer_post_load(TPMState *s); > > void tpm_tis_reset(TPMState *s, bool ppi_enabled); > > enum TPMVersion tpm_tis_get_tpm_version(TPMState *s); > > void tpm_tis_request_completed(TPMState *s, int ret); > > diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c > > index 43e68410f8..c9c4dd1190 100644 > > --- a/hw/tpm/tpm_tis_common.c > > +++ b/hw/tpm/tpm_tis_common.c > > @@ -270,7 +270,7 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty) > > uint16_t len; > > if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) { > > - len = MIN(tpm_cmd_get_size(&s->buffer), > > + len = MIN(tpm_cmd_get_size(s->buffer), > > s->be_buffer_size); > > ret = s->buffer[s->rw_offset++]; > > @@ -317,7 +317,7 @@ static void tpm_tis_dump_state(TPMState *s, hwaddr addr) > > "tpm_tis: result buffer : ", > > s->rw_offset); > > for (idx = 0; > > - idx < MIN(tpm_cmd_get_size(&s->buffer), s->be_buffer_size); > > + idx < MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size); > > idx++) { > > printf("%c%02x%s", > > s->rw_offset == idx ? '>' : ' ', > > @@ -383,7 +383,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr, > > if (s->active_locty == locty) { > > if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) { > > val = TPM_TIS_BURST_COUNT( > > - MIN(tpm_cmd_get_size(&s->buffer), > > + MIN(tpm_cmd_get_size(s->buffer), > > s->be_buffer_size) > > - s->rw_offset) | s->loc[locty].sts; > > } else { > > @@ -754,7 +754,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr, > > /* we have a packet length - see if we have all of it */ > > bool need_irq = !(s->loc[locty].sts & TPM_TIS_STS_VALID); > > - len = tpm_cmd_get_size(&s->buffer); > > + len = tpm_cmd_get_size(s->buffer); > > if (len > s->rw_offset) { > > tpm_tis_sts_set(&s->loc[locty], > > TPM_TIS_STS_EXPECT | TPM_TIS_STS_VALID); > > @@ -818,9 +818,10 @@ void tpm_tis_reset(TPMState *s, bool ppi_enabled) > > int c; > > s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver); > > - s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver), > > - TPM_TIS_BUFFER_MAX); > > + s->be_buffer_size = tpm_backend_get_buffer_size(s->be_driver); > > + s->buffer = g_realloc(s->buffer, MAX(s->be_buffer_size, > > + TPM_TIS_BUFFER_MAX)); > > With MAX() it can now be bigger than TPM_TIS_BUFFER_MAX if the backend says > so -- hm... TPM_TIS_BUFFER_MAX is still 4096. In this patch, I changed hardcoded 4096 value to TPM_TIS_BUFFER_MAX. So that the buffer can be allocated adequate space, the size passed to g_realloc is : MAX(backend_tpm_size, 4096). The additional part is sent using ext_buffer and ext_size. These are set in the pre_save hook. We did change the TPM_BUFSIZE in the kernel: https://lore.kernel.org/lkml/20260324181244.17741-5-armenon@redhat.com/ Please correct me if I am wrong. I might have missed something while applying patches on top of the tree. > > > if (ppi_enabled) { > > tpm_ppi_reset(&s->ppi); > > } > > @@ -873,6 +874,45 @@ int tpm_tis_pre_save(TPMState *s) > > */ > > tpm_backend_finish_sync(s->be_driver); > > + if (s->be_buffer_size > TPM_TIS_BUFFER_MAX) { > > + s->ext_size = s->be_buffer_size - TPM_TIS_BUFFER_MAX; > > + s->ext_buffer = s->buffer + TPM_TIS_BUFFER_MAX; > > + } else { > > + s->ext_size = 0; > > + s->ext_buffer = NULL; > > + } > > + return 0; > > +} > > + > > +int tpm_tis_post_load(TPMState *s) > > +{ > > + if (s->rw_offset > s->be_buffer_size) { > > + return -EINVAL; > > + } > > + return 0; > > +} > > + > > +int tpm_tis_ext_buffer_post_load(TPMState *s) > > +{ > > + /* > > + * Calculate the maximum extension buffer size allowed, by comparing > > + * the destination VM's backend capacity with TPM_TIS_BUFFER_MAX. > > + */ > > + uint32_t max_ext = s->be_buffer_size > TPM_TIS_BUFFER_MAX ? > > + s->be_buffer_size - TPM_TIS_BUFFER_MAX : 0; > > + > > + if (s->ext_size > max_ext) { > > + /* > > + * Source buffer size is greater than what the destination backend > > + * allows > > + */ > > + g_clear_pointer(&s->ext_buffer, g_free); > > + return -EINVAL; > > + } > > + if (s->ext_size > 0) { > > + memcpy(s->buffer + TPM_TIS_BUFFER_MAX, s->ext_buffer, s->ext_size); ... [1] > > + g_clear_pointer(&s->ext_buffer, g_free); > > + } > > return 0; > > } > > @@ -901,7 +941,7 @@ bool tpm_tis_ext_buffer_migration_needed(struct TPMState *s) > > case TPM_TIS_STATE_READY: > > return false; > > case TPM_TIS_STATE_RECEPTION: > > - return s->rw_offset >= 4096; > > + return s->rw_offset >= TPM_TIS_BUFFER_MAX; > > This cannot be right. > > For PQC support we extended the 4096 byte buffer from 4096 bytes to 8192 > byte but only want to store the 2nd 4096 bytes if necessary and to keep > backwards compatibility. > Because TPM_TIS_BUFFER_MAX is still 4096, the check s->rw_offset >= TPM_TIS_BUFFER_MAX correctly identifies when we have data exceeding the legacy limit. > This function was called to determine whether more than 4096 bytes were > either written to the buffer by the OS driver or received from the TPM as a > response so that we now would have to send the additional 2nd 4096 bytes. > But TPM_TIS_BUFFER_MAX is 8192. > > If TPM_TIS_BUFFER_MAX stayed 4096, then the comparisons should make sense. > > > case TPM_TIS_STATE_EXECUTION:> /* > > * TPM is executing: we cannot know the size of TPM response. > > @@ -909,7 +949,7 @@ bool tpm_tis_ext_buffer_migration_needed(struct TPMState *s) > > */ > > return false; > > case TPM_TIS_STATE_COMPLETION: > > - return (tpm_cmd_get_size(&s->buffer) >= 4096); > > + return (tpm_cmd_get_size(s->buffer) >= TPM_TIS_BUFFER_MAX); > > Not good, either. If TPM_TIS_BUFFER_MAX stayed 4096, then the comparisons should make sense. > > > } > > return false; > > } > > diff --git a/hw/tpm/tpm_tis_i2c.c b/hw/tpm/tpm_tis_i2c.c > > index f48938e3a1..41a5486497 100644 > > --- a/hw/tpm/tpm_tis_i2c.c > > +++ b/hw/tpm/tpm_tis_i2c.c > > @@ -103,6 +103,10 @@ static int tpm_tis_i2c_post_load(void *opaque, int version_id) > > { > > TPMStateI2C *i2cst = opaque; > > + if (tpm_tis_post_load(&i2cst->state) < 0) { > > + return -1; > > + } > > + > > if (i2cst->offset >= 1) { > > tpm_tis_i2c_to_tis_reg(i2cst, i2cst->data[0]); > > } > > @@ -117,13 +121,23 @@ static bool tpm_tis_ext_buffer_migration_needed_i2c(void *opaque) > > return tpm_tis_ext_buffer_migration_needed(&i2cst->state); > > } > > +static int tpm_tis_ext_buffer_post_load_i2c(void *opaque, int version_id) > > +{ > > + TPMStateI2C *i2cst = opaque; > > + > > + return tpm_tis_ext_buffer_post_load(&i2cst->state); > > +} > > + > > static const VMStateDescription vmstate_tpm_tis_ext_buffer_i2c = { > > .name = "tpm-tis/ext_buffer", > > .version_id = 0, > > .needed = tpm_tis_ext_buffer_migration_needed_i2c, > > .pre_save = tpm_tis_i2c_pre_save, > > + .post_load = tpm_tis_ext_buffer_post_load_i2c, > > .fields = (const VMStateField[]) { > > - VMSTATE_BUFFER_START_MIDDLE(state.buffer, TPMStateI2C, 4096), > > Here we instructed the buffer to be written from 4096 bytes to 8192 bytes, > so the 2nd part. I don't think your changes are changing it to anything > equivalent. If TPM_TIS_BUFFER_MAX stayed 4096, then VMSTATE_VBUFFER_ALLOC_UINT32() would create a new buffer of size ext_size (anything more than 4096) and allocate the additional buffer into it. Subsequently it will memcpy it into the original buffer and free it [1]. > > > + VMSTATE_UINT32(state.ext_size, TPMStateI2C), > > + VMSTATE_VBUFFER_ALLOC_UINT32(state.ext_buffer, TPMStateI2C, 0, NULL, > > + state.ext_size), > > VMSTATE_END_OF_LIST() > > } > > }; > > @@ -134,7 +148,8 @@ static const VMStateDescription vmstate_tpm_tis_i2c = { > > .pre_save = tpm_tis_i2c_pre_save, > > .post_load = tpm_tis_i2c_post_load, > > .fields = (const VMStateField[]) { > > - VMSTATE_PARTIAL_BUFFER(state.buffer, TPMStateI2C, 4096), > > This was supposed to store the first 4096 bytes of the buffer to keep > backwards compatibility. And the 2nd 4096 bytes were only supposed to be > written if found necessary. If TPM_TIS_BUFFER_MAX stayed 4096, then this would make sense. Only hardcoded 4096 is changed to TPM_TIS_BUFFER_MAX and the macro is changed because now the buffer is a pointer. > > > + VMSTATE_BUFFER_POINTER_UNSAFE(state.buffer, TPMStateI2C, 0, > > + TPM_TIS_BUFFER_MAX), > > VMSTATE_UINT16(state.rw_offset, TPMStateI2C), > > VMSTATE_UINT8(state.active_locty, TPMStateI2C), > > VMSTATE_UINT8(state.aborting_locty, TPMStateI2C), > > @@ -535,6 +550,14 @@ static void tpm_tis_i2c_realizefn(DeviceState *dev, Error **errp) > > } > > } > > +static void tpm_tis_i2c_unrealizefn(DeviceState *dev) > > +{ > > + TPMStateI2C *i2cst = TPM_TIS_I2C(dev); > > + TPMState *state = &i2cst->state; > > + > > + g_clear_pointer(&state->buffer, g_free); > > +} > > + > > static void tpm_tis_i2c_reset(DeviceState *dev) > > { > > TPMStateI2C *i2cst = TPM_TIS_I2C(dev); > > @@ -555,6 +578,7 @@ static void tpm_tis_i2c_class_init(ObjectClass *klass, const void *data) > > TPMIfClass *tc = TPM_IF_CLASS(klass); > > dc->realize = tpm_tis_i2c_realizefn; > > + dc->unrealize = tpm_tis_i2c_unrealizefn; > > device_class_set_legacy_reset(dc, tpm_tis_i2c_reset); > > dc->vmsd = &vmstate_tpm_tis_i2c; > > device_class_set_props(dc, tpm_tis_i2c_properties); > > diff --git a/hw/tpm/tpm_tis_isa.c b/hw/tpm/tpm_tis_isa.c > > index 4999de1c61..7fbdfb96e6 100644 > > --- a/hw/tpm/tpm_tis_isa.c > > +++ b/hw/tpm/tpm_tis_isa.c > > @@ -49,6 +49,12 @@ static int tpm_tis_pre_save_isa(void *opaque) > > return tpm_tis_pre_save(&isadev->state); > > } > > +static int tpm_tis_post_load_isa(void *opaque, int version_id) > > +{ > > + TPMStateISA *isadev = opaque; > > + return tpm_tis_post_load(&isadev->state); > > +} > > + > > static bool tpm_tis_ext_buffer_migration_needed_isa(void *opaque) > > { > > TPMStateISA *isadev = opaque; > > @@ -56,13 +62,23 @@ static bool tpm_tis_ext_buffer_migration_needed_isa(void *opaque) > > return tpm_tis_ext_buffer_migration_needed(&isadev->state); > > } > > +static int tpm_tis_ext_buffer_post_load_isa(void *opaque, int version_id) > > +{ > > + TPMStateISA *isadev = opaque; > > + > > + return tpm_tis_ext_buffer_post_load(&isadev->state); > > +} > > + > > static const VMStateDescription vmstate_tpm_tis_ext_buffer_isa = { > > .name = "tpm-tis/ext_buffer", > > .version_id = 0, > > .needed = tpm_tis_ext_buffer_migration_needed_isa, > > .pre_save = tpm_tis_pre_save_isa, > > + .post_load = tpm_tis_ext_buffer_post_load_isa, > > .fields = (const VMStateField[]) { > > - VMSTATE_BUFFER_START_MIDDLE(state.buffer, TPMStateISA, 4096), > > + VMSTATE_UINT32(state.ext_size, TPMStateISA), > > + VMSTATE_VBUFFER_ALLOC_UINT32(state.ext_buffer, TPMStateISA, 0, NULL, > > + state.ext_size), > > VMSTATE_END_OF_LIST() > > } > > }; > > @@ -71,8 +87,10 @@ static const VMStateDescription vmstate_tpm_tis_isa = { > > .name = "tpm-tis", > > .version_id = 0, > > .pre_save = tpm_tis_pre_save_isa, > > + .post_load = tpm_tis_post_load_isa, > > .fields = (const VMStateField[]) { > > - VMSTATE_PARTIAL_BUFFER(state.buffer, TPMStateISA, 4096), > > + VMSTATE_BUFFER_POINTER_UNSAFE(state.buffer, TPMStateISA, 0, > > + TPM_TIS_BUFFER_MAX), > > VMSTATE_UINT16(state.rw_offset, TPMStateISA), > > VMSTATE_UINT8(state.active_locty, TPMStateISA), > > VMSTATE_UINT8(state.aborting_locty, TPMStateISA), > > @@ -157,6 +175,14 @@ static void tpm_tis_isa_realizefn(DeviceState *dev, Error **errp) > > TPM_PPI_ADDR_BASE, OBJECT(dev)); > > } > > +static void tpm_tis_isa_unrealizefn(DeviceState *dev) > > +{ > > + TPMStateISA *isadev = TPM_TIS_ISA(dev); > > + TPMState *state = &isadev->state; > > + > > + g_clear_pointer(&state->buffer, g_free); > > +} > > + > > static void build_tpm_tis_isa_aml(AcpiDevAmlIf *adev, Aml *scope) > > { > > Aml *dev, *crs; > > @@ -196,6 +222,7 @@ static void tpm_tis_isa_class_init(ObjectClass *klass, const void *data) > > tc->model = TPM_MODEL_TPM_TIS; > > tc->ppi_enabled = true; > > dc->realize = tpm_tis_isa_realizefn; > > + dc->unrealize = tpm_tis_isa_unrealizefn; > > device_class_set_legacy_reset(dc, tpm_tis_isa_reset); > > tc->request_completed = tpm_tis_isa_request_completed; > > tc->get_version = tpm_tis_isa_get_tpm_version; > > diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c > > index c29f43bdce..ad8cfa85b1 100644 > > --- a/hw/tpm/tpm_tis_sysbus.c > > +++ b/hw/tpm/tpm_tis_sysbus.c > > @@ -49,6 +49,13 @@ static int tpm_tis_pre_save_sysbus(void *opaque) > > return tpm_tis_pre_save(&sbdev->state); > > } > > +static int tpm_tis_post_load_sysbus(void *opaque, int version_id) > > +{ > > + TPMStateSysBus *sbdev = opaque; > > + > > + return tpm_tis_post_load(&sbdev->state); > > +} > > + > > static bool tpm_tis_ext_buffer_migration_needed_sysbus(void *opaque) > > { > > TPMStateSysBus *sbdev = opaque; > > @@ -56,13 +63,23 @@ static bool tpm_tis_ext_buffer_migration_needed_sysbus(void *opaque) > > return tpm_tis_ext_buffer_migration_needed(&sbdev->state); > > } > > +static int tpm_tis_ext_buffer_post_load_sysbus(void *opaque, int version_id) > > +{ > > + TPMStateSysBus *sbdev = opaque; > > + > > + return tpm_tis_ext_buffer_post_load(&sbdev->state); > > +} > > + > > static const VMStateDescription vmstate_tpm_tis_ext_buffer_sysbus = { > > .name = "tpm-tis/ext_buffer", > > .version_id = 0, > > .needed = tpm_tis_ext_buffer_migration_needed_sysbus, > > .pre_save = tpm_tis_pre_save_sysbus, > > + .post_load = tpm_tis_ext_buffer_post_load_sysbus, > > .fields = (const VMStateField[]) { > > - VMSTATE_BUFFER_START_MIDDLE(state.buffer, TPMStateSysBus, 4096), > > + VMSTATE_UINT32(state.ext_size, TPMStateSysBus), > > + VMSTATE_VBUFFER_ALLOC_UINT32(state.ext_buffer, TPMStateSysBus, 0, > > + NULL, state.ext_size), > > VMSTATE_END_OF_LIST() > > } > > }; > > @@ -71,8 +88,10 @@ static const VMStateDescription vmstate_tpm_tis_sysbus = { > > .name = "tpm-tis", > > .version_id = 0, > > .pre_save = tpm_tis_pre_save_sysbus, > > + .post_load = tpm_tis_post_load_sysbus, > > .fields = (const VMStateField[]) { > > - VMSTATE_PARTIAL_BUFFER(state.buffer, TPMStateSysBus, 4096), > > + VMSTATE_BUFFER_POINTER_UNSAFE(state.buffer, TPMStateSysBus, 0, > > + TPM_TIS_BUFFER_MAX), > > VMSTATE_UINT16(state.rw_offset, TPMStateSysBus), > > VMSTATE_UINT8(state.active_locty, TPMStateSysBus), > > VMSTATE_UINT8(state.aborting_locty, TPMStateSysBus), > > @@ -156,6 +175,14 @@ static void tpm_tis_sysbus_realizefn(DeviceState *dev, Error **errp) > > vmstate_register_ram(&s->ppi.ram, dev); > > } > > +static void tpm_tis_sysbus_unrealizefn(DeviceState *dev) > > +{ > > + TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(dev); > > + TPMState *state = &sbdev->state; > > + > > + g_clear_pointer(&state->buffer, g_free); > > +} > > + > > static void tpm_tis_sysbus_class_init(ObjectClass *klass, const void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > @@ -166,6 +193,7 @@ static void tpm_tis_sysbus_class_init(ObjectClass *klass, const void *data) > > tc->model = TPM_MODEL_TPM_TIS; > > tc->ppi_enabled = true; > > dc->realize = tpm_tis_sysbus_realizefn; > > + dc->unrealize = tpm_tis_sysbus_unrealizefn; > > device_class_set_legacy_reset(dc, tpm_tis_sysbus_reset); > > tc->request_completed = tpm_tis_sysbus_request_completed; > > tc->get_version = tpm_tis_sysbus_get_tpm_version; > > Was TPM_TIS_BUFFER_MAX expected to be increased to 8192? Regards, Arun Menon ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm: Dynamically allocate tpm-tis buffer 2026-04-28 7:01 ` Arun Menon @ 2026-04-28 12:57 ` Stefan Berger 2026-04-29 18:15 ` Stefan Berger 0 siblings, 1 reply; 10+ messages in thread From: Stefan Berger @ 2026-04-28 12:57 UTC (permalink / raw) To: armenon; +Cc: qemu-devel, Stefan Berger On 4/28/26 3:01 AM, Arun Menon wrote: > Hi Stefan, > > Thank you for taking a look. > > On Mon, Apr 27, 2026 at 04:49:04PM -0400, Stefan Berger wrote: >> >> >> On 4/27/26 4:01 PM, Arun Menon wrote: >>> From: Arun Menon <armenon@redhat.com> >>> >>> The TPM TIS buffer is currently a fixed-size static array. Change this >>> to a dynamically allocated heap block. The buffer size is now determined >>> at runtime by querying the TPM backend. >> >> Do we really need this? I mean for the forseeable future 8kb should be >> sufficient. > > You are right that 8kb should be sufficient. > I implemented this to address the TODO mentioned here: > https://github.com/qemu/qemu/commit/e5f62d87e3c03bda6006085cf6303736fb57f5c5 > > That is why this patch is deliberately posted outside the v5 series. > It is more about future-proofing than a strict requirement. I thought it > was worth addressing while the context was fresh. I am happy to leave it > out if we prefer that. Actually, go ahead. It's better than hard coding the size. > >> >>> >>> To support VM migration, >>> 1. Replace the static VMSTATE_BUFFER macro with pointer-based variant >>> VMSTATE_BUFFER_POINTER_UNSAFE, explicitly mentioning the size. >>> 2. Introduce ext_buffer and ext_size in the migration subsection to >>> track allocation exceeding TPM_TIS_BUFFER_MAX. Allocate ext_buffer >>> using VMSTATE_VBUFFER_ALLOC_UINT32 only to be freed later after it is >>> appended to the main buffer. >>> >>> This allows us to migrate to a destination host without breaking >>> backward compatibility. Old QEMU does not include a size field along >>> with the buffer in the migration stream, and therefore the >>> new QEMU is also forced to keep expecting exactly 4096 bytes. >>> >>> Implement a post_load hook that will validate the incoming data size >>> from the migration stream, failing the migration if it exceeds the >>> destination backend capacity. Add unrealize functions for the TIS interface >>> types ISA, SysBus and I2C to ensure that the buffer is safely freed on >>> device destruction. >>> >>> Signed-off-by: Arun Menon <armenon@redhat.com> >>> --- >>> Dependencies: >>> This patch depends on the following patch currently in the mailing list: >>> https://lore.kernel.org/qemu-devel/20260422103018.123608-10-armenon@redhat.com/ >> >> I had tried to apply your v5 series to master but could not. Do you have a >> git repo where you keep your patches? > > Yes. Here is the repo: https://gitlab.com/armenon/qemu-dev/-/tree/pqc_tpm?ref_type=heads > I have applied the GByteArray change first, followed by the 10 commits. > >> >>> >>> Depends-on: <20260422103018.123608-10-armenon@redhat.com> >>> >>> hw/tpm/tpm_tis.h | 6 ++++- >>> hw/tpm/tpm_tis_common.c | 56 +++++++++++++++++++++++++++++++++++------ >>> hw/tpm/tpm_tis_i2c.c | 28 +++++++++++++++++++-- >>> hw/tpm/tpm_tis_isa.c | 31 +++++++++++++++++++++-- >>> hw/tpm/tpm_tis_sysbus.c | 32 +++++++++++++++++++++-- >>> 5 files changed, 138 insertions(+), 15 deletions(-) >>> >>> diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h >>> index b2d9c0116c..c736ecedc1 100644 >>> --- a/hw/tpm/tpm_tis.h >>> +++ b/hw/tpm/tpm_tis.h >>> @@ -56,7 +56,9 @@ typedef struct TPMLocality { >>> typedef struct TPMState { >>> MemoryRegion mmio; >>> - unsigned char buffer[TPM_TIS_BUFFER_MAX]; >>> + uint8_t *buffer; >>> + uint8_t *ext_buffer; >>> + uint32_t ext_size; >>> uint16_t rw_offset; >>> uint8_t active_locty; >>> @@ -82,6 +84,8 @@ extern const VMStateDescription vmstate_locty; >>> extern const MemoryRegionOps tpm_tis_memory_ops; >>> int tpm_tis_pre_save(TPMState *s); >>> +int tpm_tis_post_load(TPMState *s); >>> +int tpm_tis_ext_buffer_post_load(TPMState *s); >>> void tpm_tis_reset(TPMState *s, bool ppi_enabled); >>> enum TPMVersion tpm_tis_get_tpm_version(TPMState *s); >>> void tpm_tis_request_completed(TPMState *s, int ret); >>> diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c >>> index 43e68410f8..c9c4dd1190 100644 >>> --- a/hw/tpm/tpm_tis_common.c >>> +++ b/hw/tpm/tpm_tis_common.c >>> @@ -270,7 +270,7 @@ static uint32_t tpm_tis_data_read(TPMState *s, uint8_t locty) >>> uint16_t len; >>> if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) { >>> - len = MIN(tpm_cmd_get_size(&s->buffer), >>> + len = MIN(tpm_cmd_get_size(s->buffer), >>> s->be_buffer_size); >>> ret = s->buffer[s->rw_offset++]; >>> @@ -317,7 +317,7 @@ static void tpm_tis_dump_state(TPMState *s, hwaddr addr) >>> "tpm_tis: result buffer : ", >>> s->rw_offset); >>> for (idx = 0; >>> - idx < MIN(tpm_cmd_get_size(&s->buffer), s->be_buffer_size); >>> + idx < MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size); >>> idx++) { >>> printf("%c%02x%s", >>> s->rw_offset == idx ? '>' : ' ', >>> @@ -383,7 +383,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr, >>> if (s->active_locty == locty) { >>> if ((s->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) { >>> val = TPM_TIS_BURST_COUNT( >>> - MIN(tpm_cmd_get_size(&s->buffer), >>> + MIN(tpm_cmd_get_size(s->buffer), >>> s->be_buffer_size) >>> - s->rw_offset) | s->loc[locty].sts; >>> } else { >>> @@ -754,7 +754,7 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr, >>> /* we have a packet length - see if we have all of it */ >>> bool need_irq = !(s->loc[locty].sts & TPM_TIS_STS_VALID); >>> - len = tpm_cmd_get_size(&s->buffer); >>> + len = tpm_cmd_get_size(s->buffer); >>> if (len > s->rw_offset) { >>> tpm_tis_sts_set(&s->loc[locty], >>> TPM_TIS_STS_EXPECT | TPM_TIS_STS_VALID); >>> @@ -818,9 +818,10 @@ void tpm_tis_reset(TPMState *s, bool ppi_enabled) >>> int c; >>> s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver); >>> - s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver), >>> - TPM_TIS_BUFFER_MAX); >>> + s->be_buffer_size = tpm_backend_get_buffer_size(s->be_driver); >>> + s->buffer = g_realloc(s->buffer, MAX(s->be_buffer_size, >>> + TPM_TIS_BUFFER_MAX)); >> >> With MAX() it can now be bigger than TPM_TIS_BUFFER_MAX if the backend says >> so -- hm... > > TPM_TIS_BUFFER_MAX is still 4096. Oh, I had still based my patches on your TPM_TIS_BUFFER_MAX increase to 8192 bytes. Let me fixes this along with a few other things. I will let you know. I have another series that I will post now that can be applied to master. It's adding a test for TIS over I2C but I will need to extend that one also with the large transfer test case then. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm: Dynamically allocate tpm-tis buffer 2026-04-28 12:57 ` Stefan Berger @ 2026-04-29 18:15 ` Stefan Berger 2026-04-29 20:07 ` Arun Menon 0 siblings, 1 reply; 10+ messages in thread From: Stefan Berger @ 2026-04-29 18:15 UTC (permalink / raw) To: armenon; +Cc: qemu-devel, Stefan Berger On 4/28/26 8:57 AM, Stefan Berger wrote: > > > On 4/28/26 3:01 AM, Arun Menon wrote: >>> >>> With MAX() it can now be bigger than TPM_TIS_BUFFER_MAX if the >>> backend says >>> so -- hm... >> >> TPM_TIS_BUFFER_MAX is still 4096. > > Oh, I had still based my patches on your TPM_TIS_BUFFER_MAX increase to > 8192 bytes. Let me fixes this along with a few other things. I will let > you know. > > I have another series that I will post now that can be applied to > master. It's adding a test for TIS over I2C but I will need to extend > that one also with the large transfer test case then. > > So I have resolved this issue now along with a few other things. My branch is here: https://github.com/stefanberger/qemu-tpm/tree/work-tpm-for-11.1 - I have applied the i2c swtpm test case series first since it could be easily upstreamed first - Then your patch "migration/vmstate: Add VMState support for GByteArray" - Then the CRB chunk + TIS extended buffer support series. I modified my patches (last 4 in that series) to - increased MAX_TIS_BUFFER_SIZE to 8192 - added migration blockers dynamically for TIS whenever >4096 bytes are either in the request or response; remove them later on again when device goes into ready state for example - added large transfer test also for i2c Please pick up those patches for v6 posting, or otherwise you can split your v5 series up into CRB-only support for v6 and I post my (last 4) patches later on. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm: Dynamically allocate tpm-tis buffer 2026-04-29 18:15 ` Stefan Berger @ 2026-04-29 20:07 ` Arun Menon 2026-04-30 3:40 ` Stefan Berger 0 siblings, 1 reply; 10+ messages in thread From: Arun Menon @ 2026-04-29 20:07 UTC (permalink / raw) To: Stefan Berger; +Cc: qemu-devel, Stefan Berger Hi Stefan, On Wed, Apr 29, 2026 at 02:15:16PM -0400, Stefan Berger wrote: > > > On 4/28/26 8:57 AM, Stefan Berger wrote: > > > > > > On 4/28/26 3:01 AM, Arun Menon wrote: > > > > > > > > > With MAX() it can now be bigger than TPM_TIS_BUFFER_MAX if the > > > > backend says > > > > so -- hm... > > > > > > TPM_TIS_BUFFER_MAX is still 4096. > > > > Oh, I had still based my patches on your TPM_TIS_BUFFER_MAX increase to > > 8192 bytes. Let me fixes this along with a few other things. I will let > > you know. > > > > I have another series that I will post now that can be applied to > > master. It's adding a test for TIS over I2C but I will need to extend > > that one also with the large transfer test case then. > > > > > > So I have resolved this issue now along with a few other things. My branch > is here: https://github.com/stefanberger/qemu-tpm/tree/work-tpm-for-11.1 > > - I have applied the i2c swtpm test case series first since it could be > easily upstreamed first > - Then your patch "migration/vmstate: Add VMState support for GByteArray" > - Then the CRB chunk + TIS extended buffer support series. I modified my > patches (last 4 in that series) to > - increased MAX_TIS_BUFFER_SIZE to 8192 > - added migration blockers dynamically for TIS whenever >4096 bytes are > either in the request or response; remove them later on again when device > goes into ready state for example > - added large transfer test also for i2c > > Please pick up those patches for v6 posting, or otherwise you can split your > v5 series up into CRB-only support for v6 and I post my (last 4) patches > later on. > Thank you. I agree. I will post v6 as CRB-only support. The GByteArray patch is already accepted. The complex TIS changes can be posted separately later. Regarding the TIS buffer: I saw you implemented the migration blockers and 8192-byte increase in your branch. Should I discard my 'Dynamically allocate tpm-tis buffer' patch entirely, or do you want to try and adapt that logic into your TIS follow-up series later? Since your new migration blockers are based on the 4096/8192 threshold, it might be simpler to stick with the static approach for now, but let me know what you prefer. Regards, Arun Menon > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm: Dynamically allocate tpm-tis buffer 2026-04-29 20:07 ` Arun Menon @ 2026-04-30 3:40 ` Stefan Berger 2026-04-30 20:43 ` Stefan Berger 0 siblings, 1 reply; 10+ messages in thread From: Stefan Berger @ 2026-04-30 3:40 UTC (permalink / raw) To: Arun Menon; +Cc: qemu-devel, Stefan Berger On 4/29/26 4:07 PM, Arun Menon wrote: > Hi Stefan, > > On Wed, Apr 29, 2026 at 02:15:16PM -0400, Stefan Berger wrote: >> >> >> On 4/28/26 8:57 AM, Stefan Berger wrote: >>> >>> >>> On 4/28/26 3:01 AM, Arun Menon wrote: >> >>>>> >>>>> With MAX() it can now be bigger than TPM_TIS_BUFFER_MAX if the >>>>> backend says >>>>> so -- hm... >>>> >>>> TPM_TIS_BUFFER_MAX is still 4096. >>> >>> Oh, I had still based my patches on your TPM_TIS_BUFFER_MAX increase to >>> 8192 bytes. Let me fixes this along with a few other things. I will let >>> you know. >>> >>> I have another series that I will post now that can be applied to >>> master. It's adding a test for TIS over I2C but I will need to extend >>> that one also with the large transfer test case then. >>> >>> >> >> So I have resolved this issue now along with a few other things. My branch >> is here: https://github.com/stefanberger/qemu-tpm/tree/work-tpm-for-11.1 >> >> - I have applied the i2c swtpm test case series first since it could be >> easily upstreamed first >> - Then your patch "migration/vmstate: Add VMState support for GByteArray" >> - Then the CRB chunk + TIS extended buffer support series. I modified my >> patches (last 4 in that series) to >> - increased MAX_TIS_BUFFER_SIZE to 8192 >> - added migration blockers dynamically for TIS whenever >4096 bytes are >> either in the request or response; remove them later on again when device >> goes into ready state for example >> - added large transfer test also for i2c >> >> Please pick up those patches for v6 posting, or otherwise you can split your >> v5 series up into CRB-only support for v6 and I post my (last 4) patches >> later on. >> > > Thank you. I agree. I will post v6 as CRB-only support. The GByteArray > patch is already accepted. The complex TIS changes can be posted > separately later. > > Regarding the TIS buffer: I saw you implemented the migration blockers > and 8192-byte increase in your branch. Should I discard my It's quite dynamic with the blocker being set when 4097 bytes of a TPM command were received or when a TPM response with > 4096 bytes is received. The blocker is removed when the device goes into ready state. The intention is to allow a user to choose an old machine type with a PQC-enabled swtpm+libtpms and be able to migrate to an older version of QEMU unless the blocker was set. Correct me if I am wrong in my thinking... > 'Dynamically allocate tpm-tis buffer' patch entirely, or do you want to > try and adapt that logic into your TIS follow-up series later? > It may be good to have a dynamically allocate tpm-tis buffer for future proofing the TIS, though it would have to support saving a range of the bytes in this buffer -- like 1:1 replacement using GByteArray for the VMSTATE_PARTIAL_BUFFER and VMSTATE_BUFFER_START_MIDDLE that I am using now. > Since your new migration blockers are based on the 4096/8192 threshold, > it might be simpler to stick with the static approach for now, but let > me know what you prefer. It would still be good not to have to worry about buffer size increases anymore... > > > Regards, > Arun Menon > >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm: Dynamically allocate tpm-tis buffer 2026-04-30 3:40 ` Stefan Berger @ 2026-04-30 20:43 ` Stefan Berger 2026-05-04 18:10 ` Arun Menon 0 siblings, 1 reply; 10+ messages in thread From: Stefan Berger @ 2026-04-30 20:43 UTC (permalink / raw) To: Arun Menon; +Cc: qemu-devel, Stefan Berger On 4/29/26 11:40 PM, Stefan Berger wrote: > > > On 4/29/26 4:07 PM, Arun Menon wrote: >> Hi Stefan, >> >> On Wed, Apr 29, 2026 at 02:15:16PM -0400, Stefan Berger wrote: >>> >>> >>> On 4/28/26 8:57 AM, Stefan Berger wrote: >>>> >>>> >>>> On 4/28/26 3:01 AM, Arun Menon wrote: >>> >>>>>> >>>>>> With MAX() it can now be bigger than TPM_TIS_BUFFER_MAX if the >>>>>> backend says >>>>>> so -- hm... >>>>> >>>>> TPM_TIS_BUFFER_MAX is still 4096. >>>> >>>> Oh, I had still based my patches on your TPM_TIS_BUFFER_MAX increase to >>>> 8192 bytes. Let me fixes this along with a few other things. I will let >>>> you know. >>>> >>>> I have another series that I will post now that can be applied to >>>> master. It's adding a test for TIS over I2C but I will need to extend >>>> that one also with the large transfer test case then. >>>> >>>> >>> >>> So I have resolved this issue now along with a few other things. My >>> branch >>> is here: https://github.com/stefanberger/qemu-tpm/tree/work-tpm-for-11.1 >>> >>> - I have applied the i2c swtpm test case series first since it could be >>> easily upstreamed first >>> - Then your patch "migration/vmstate: Add VMState support for >>> GByteArray" >>> - Then the CRB chunk + TIS extended buffer support series. I modified my >>> patches (last 4 in that series) to >>> - increased MAX_TIS_BUFFER_SIZE to 8192 >>> - added migration blockers dynamically for TIS whenever >4096 >>> bytes are >>> either in the request or response; remove them later on again when >>> device >>> goes into ready state for example >>> - added large transfer test also for i2c >>> >>> Please pick up those patches for v6 posting, or otherwise you can >>> split your >>> v5 series up into CRB-only support for v6 and I post my (last 4) patches >>> later on. >>> >> >> Thank you. I agree. I will post v6 as CRB-only support. The GByteArray >> patch is already accepted. The complex TIS changes can be posted >> separately later. >> >> Regarding the TIS buffer: I saw you implemented the migration blockers >> and 8192-byte increase in your branch. Should I discard my > > It's quite dynamic with the blocker being set when 4097 bytes of a TPM > command were received or when a TPM response with > 4096 bytes is > received. The blocker is removed when the device goes into ready state. > The intention is to allow a user to choose an old machine type with a > PQC-enabled swtpm+libtpms and be able to migrate to an older version of > QEMU unless the blocker was set. Correct me if I am wrong in my thinking... Another solution would be to limit the 8kb buffer to 4kb for 11.0 and older machines. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm: Dynamically allocate tpm-tis buffer 2026-04-30 20:43 ` Stefan Berger @ 2026-05-04 18:10 ` Arun Menon 2026-05-04 20:35 ` Stefan Berger 0 siblings, 1 reply; 10+ messages in thread From: Arun Menon @ 2026-05-04 18:10 UTC (permalink / raw) To: Stefan Berger; +Cc: qemu-devel, Stefan Berger On Thu, Apr 30, 2026 at 04:43:27PM -0400, Stefan Berger wrote: > > > On 4/29/26 11:40 PM, Stefan Berger wrote: > > > > > > On 4/29/26 4:07 PM, Arun Menon wrote: > > > Hi Stefan, > > > > > > On Wed, Apr 29, 2026 at 02:15:16PM -0400, Stefan Berger wrote: > > > > > > > > > > > > On 4/28/26 8:57 AM, Stefan Berger wrote: > > > > > > > > > > > > > > > On 4/28/26 3:01 AM, Arun Menon wrote: > > > > > > > > > > > > > > > > > > With MAX() it can now be bigger than TPM_TIS_BUFFER_MAX if the > > > > > > > backend says > > > > > > > so -- hm... > > > > > > > > > > > > TPM_TIS_BUFFER_MAX is still 4096. > > > > > > > > > > Oh, I had still based my patches on your TPM_TIS_BUFFER_MAX increase to > > > > > 8192 bytes. Let me fixes this along with a few other things. I will let > > > > > you know. > > > > > > > > > > I have another series that I will post now that can be applied to > > > > > master. It's adding a test for TIS over I2C but I will need to extend > > > > > that one also with the large transfer test case then. > > > > > > > > > > > > > > > > > > So I have resolved this issue now along with a few other things. > > > > My branch > > > > is here: https://github.com/stefanberger/qemu-tpm/tree/work-tpm-for-11.1 > > > > > > > > - I have applied the i2c swtpm test case series first since it could be > > > > easily upstreamed first > > > > - Then your patch "migration/vmstate: Add VMState support for > > > > GByteArray" > > > > - Then the CRB chunk + TIS extended buffer support series. I modified my > > > > patches (last 4 in that series) to > > > > - increased MAX_TIS_BUFFER_SIZE to 8192 > > > > - added migration blockers dynamically for TIS whenever >4096 > > > > bytes are > > > > either in the request or response; remove them later on again > > > > when device > > > > goes into ready state for example > > > > - added large transfer test also for i2c > > > > > > > > Please pick up those patches for v6 posting, or otherwise you > > > > can split your > > > > v5 series up into CRB-only support for v6 and I post my (last 4) patches > > > > later on. > > > > > > > > > > Thank you. I agree. I will post v6 as CRB-only support. The GByteArray > > > patch is already accepted. The complex TIS changes can be posted > > > separately later. > > > > > > Regarding the TIS buffer: I saw you implemented the migration blockers > > > and 8192-byte increase in your branch. Should I discard my > > > > It's quite dynamic with the blocker being set when 4097 bytes of a TPM > > command were received or when a TPM response with > 4096 bytes is > > received. The blocker is removed when the device goes into ready state. > > The intention is to allow a user to choose an old machine type with a > > PQC-enabled swtpm+libtpms and be able to migrate to an older version of > > QEMU unless the blocker was set. Correct me if I am wrong in my > > thinking... > > Another solution would be to limit the 8kb buffer to 4kb for 11.0 and older > machines. Yes, I have limited this using the post_load hook for the extended part. +int tpm_tis_ext_buffer_post_load(TPMState *s) +{ + /* + * Calculate the maximum extension buffer size allowed, by comparing + * the destination VM's backend capacity with TPM_TIS_BUFFER_MAX. + */ + uint32_t max_ext = s->be_buffer_size > TPM_TIS_BUFFER_MAX ? + s->be_buffer_size - TPM_TIS_BUFFER_MAX : 0; + + if (s->ext_size > max_ext) { + /* + * Source buffer size is greater than what the destination backend + * allows + */ + g_clear_pointer(&s->ext_buffer, g_free); + return -EINVAL; + } + if (s->ext_size > 0) { + memcpy(s->buffer + TPM_TIS_BUFFER_MAX, s->ext_buffer, s->ext_size); + g_clear_pointer(&s->ext_buffer, g_free); + } We may use this patch in future if we want to change static to dynamic buffer array allocation. Regards, Arun Menon ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tpm: Dynamically allocate tpm-tis buffer 2026-05-04 18:10 ` Arun Menon @ 2026-05-04 20:35 ` Stefan Berger 0 siblings, 0 replies; 10+ messages in thread From: Stefan Berger @ 2026-05-04 20:35 UTC (permalink / raw) To: Arun Menon; +Cc: qemu-devel, Stefan Berger On 5/4/26 2:10 PM, Arun Menon wrote: > On Thu, Apr 30, 2026 at 04:43:27PM -0400, Stefan Berger wrote: >> >> >> On 4/29/26 11:40 PM, Stefan Berger wrote: >>> >>> >>> On 4/29/26 4:07 PM, Arun Menon wrote: >>>> Hi Stefan, >>>> >>>> On Wed, Apr 29, 2026 at 02:15:16PM -0400, Stefan Berger wrote: >>>>> >>>>> >>>>> On 4/28/26 8:57 AM, Stefan Berger wrote: >>>>>> >>>>>> >>>>>> On 4/28/26 3:01 AM, Arun Menon wrote: >>>>> >>>>>>>> >>>>>>>> With MAX() it can now be bigger than TPM_TIS_BUFFER_MAX if the >>>>>>>> backend says >>>>>>>> so -- hm... >>>>>>> >>>>>>> TPM_TIS_BUFFER_MAX is still 4096. >>>>>> >>>>>> Oh, I had still based my patches on your TPM_TIS_BUFFER_MAX increase to >>>>>> 8192 bytes. Let me fixes this along with a few other things. I will let >>>>>> you know. >>>>>> >>>>>> I have another series that I will post now that can be applied to >>>>>> master. It's adding a test for TIS over I2C but I will need to extend >>>>>> that one also with the large transfer test case then. >>>>>> >>>>>> >>>>> >>>>> So I have resolved this issue now along with a few other things. >>>>> My branch >>>>> is here: https://github.com/stefanberger/qemu-tpm/tree/work-tpm-for-11.1 >>>>> >>>>> - I have applied the i2c swtpm test case series first since it could be >>>>> easily upstreamed first >>>>> - Then your patch "migration/vmstate: Add VMState support for >>>>> GByteArray" >>>>> - Then the CRB chunk + TIS extended buffer support series. I modified my >>>>> patches (last 4 in that series) to >>>>> - increased MAX_TIS_BUFFER_SIZE to 8192 >>>>> - added migration blockers dynamically for TIS whenever >4096 >>>>> bytes are >>>>> either in the request or response; remove them later on again >>>>> when device >>>>> goes into ready state for example >>>>> - added large transfer test also for i2c >>>>> >>>>> Please pick up those patches for v6 posting, or otherwise you >>>>> can split your >>>>> v5 series up into CRB-only support for v6 and I post my (last 4) patches >>>>> later on. >>>>> >>>> >>>> Thank you. I agree. I will post v6 as CRB-only support. The GByteArray >>>> patch is already accepted. The complex TIS changes can be posted >>>> separately later. >>>> >>>> Regarding the TIS buffer: I saw you implemented the migration blockers >>>> and 8192-byte increase in your branch. Should I discard my >>> >>> It's quite dynamic with the blocker being set when 4097 bytes of a TPM >>> command were received or when a TPM response with > 4096 bytes is >>> received. The blocker is removed when the device goes into ready state. >>> The intention is to allow a user to choose an old machine type with a >>> PQC-enabled swtpm+libtpms and be able to migrate to an older version of >>> QEMU unless the blocker was set. Correct me if I am wrong in my >>> thinking... >> >> Another solution would be to limit the 8kb buffer to 4kb for 11.0 and older >> machines. > > Yes, I have limited this using the post_load hook for the extended part. > > +int tpm_tis_ext_buffer_post_load(TPMState *s) > +{ > + /* > + * Calculate the maximum extension buffer size allowed, by comparing > + * the destination VM's backend capacity with TPM_TIS_BUFFER_MAX. > + */ > + uint32_t max_ext = s->be_buffer_size > TPM_TIS_BUFFER_MAX ? > + s->be_buffer_size - TPM_TIS_BUFFER_MAX : 0; > + > + if (s->ext_size > max_ext) { > + /* > + * Source buffer size is greater than what the destination backend > + * allows > + */ > + g_clear_pointer(&s->ext_buffer, g_free); > + return -EINVAL; > + } > + if (s->ext_size > 0) { > + memcpy(s->buffer + TPM_TIS_BUFFER_MAX, s->ext_buffer, s->ext_size); > + g_clear_pointer(&s->ext_buffer, g_free); > + } > > We may use this patch in future if we want to change static to dynamic > buffer array allocation. I posted now what I have beyond your v6 CRB patches and we can see in which order we want to switch to dynamically allocated buffers. > > > Regards, > Arun Menon > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2026-05-04 20:35 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-04-27 20:01 [PATCH] tpm: Dynamically allocate tpm-tis buffer Arun Menon 2026-04-27 20:49 ` Stefan Berger 2026-04-28 7:01 ` Arun Menon 2026-04-28 12:57 ` Stefan Berger 2026-04-29 18:15 ` Stefan Berger 2026-04-29 20:07 ` Arun Menon 2026-04-30 3:40 ` Stefan Berger 2026-04-30 20:43 ` Stefan Berger 2026-05-04 18:10 ` Arun Menon 2026-05-04 20:35 ` Stefan Berger
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.