Re: [PATCH] tpm: Dynamically allocate tpm-tis buffer

[Stefan Berger <stefanb@linux.ibm.com>]

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.