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

[marcandre.lureau@redhat.com]

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

Hi

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

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

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

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

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

(response_buffer_len ..)

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

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

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

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

Suggesting g_clear_pointer(&ptr, g_free)

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

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

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

Or you could simply without condition:

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

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

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

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

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

Could be version 0, I think.

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