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 <[email protected]> > > > > 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 <[email protected]> > > --- > > Dependencies: > > This patch depends on the following patch currently in the mailing list: > > https://lore.kernel.org/qemu-devel/[email protected]/ > > 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: <[email protected]> > > > > 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/[email protected]/ 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
