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

[Arun Menon <armenon@redhat.com>]

On Thu, Mar 26, 2026 at 03:27:02PM +0400, marcandre.lureau@redhat.com wrote:
> 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 :)

Yes I shall base it on top of 20260331140347.653404-1-cohuck@redhat.com

> 
> >
> > 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 ?

This will be not needed.

> 
> > +
> > +    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)

I am thinking of adding a separate VMStateInfo for GByteArray. The .get
and .put functions will call actual GLib API to set the size of the
buffer or create a new one if the existing data == NULL.

const VMStateInfo vmstate_info_g_byte_array = {
    .name = "GByteArray",
    .get = get_g_byte_array,  // Creates a new GLib object iff data!=NULL
    .put = put_g_byte_array,
}


We will not require any pre-save and post load hooks in that case.
We can also use it in the ui/vdagent.c.

> 
> > @@ -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.

Yes, will change it

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

Regards,
Arun Menon