Re: [PATCH v3 06/10] hw/tpm: Add support for VM migration with TPM CRB chunking

[Arun Menon <armenon@redhat.com>]

On Tue, Apr 07, 2026 at 03:00:27PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Apr 6, 2026 at 6:18 PM Arun Menon <armenon@redhat.com> wrote:
> >
> > From: Arun Menon <armenon@redhat.com>
> >
> > - Add subsection in VMState for TPM CRB with the newly introduced
> >   command and response buffers, along with a needed callback, so that
> >   newer QEMU only sends the buffers if it is necessary.
> > - Add hw_compat blocker because the feature is only supported for
> >   machine type 11.1 and higher.
> > - If the VM has no pending chunked TPM commands in the internal buffers
> >   during a VM migration, or if the machine type does not support newly
> >   introduced buffers, then the needed callback will return false, as it
> >   checks the hw_compat blocker and thus the subsection will not be sent
> >   to the destination host.
> >
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >  hw/core/machine.c |  4 +++-
> >  hw/tpm/tpm_crb.c  | 31 +++++++++++++++++++++++++++++++
> >  2 files changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 1abc8ae737..fb290c6c53 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -38,7 +38,9 @@
> >  #include "hw/acpi/generic_event_device.h"
> >  #include "qemu/audio.h"
> >
> > -GlobalProperty hw_compat_11_0[] = {};
> > +GlobalProperty hw_compat_11_0[] = {
> > +    { "tpm-crb", "migrate-buffers", "off"},
> 
> If this is not intended to be user visible, we should use x- prefix

Yes, I will add x- prefix.

> 
> The problem is that the previous code changes expose CapChunk to the
> guest unconditionally.
> 
> If running with <=11.0 we should not enable chunk transfer by default
> - or we need to prevent/block migration... otherwise we may lose
> VM/device state, expose a differente device etc.
> 
> What about having a "cap-chunk" property? Enable it by default with >=11.1.
> 
> If enabled when <=11.0, then also disable migration.

This is really good insight. Similar to migrate-buffers, I need to make
sure that the CapCRBChunk bit is set only when we are running the newer
machine type.

I shall introduce a new entry in hw_compat array called x-cap-chunk.
That means, we should not unconditionally set the CapCRBChunk bit to 1.
We should set the bit based on cap_chunk property.

However, I am a bit confused on the order of precedence of this variable.
From what I understand, the device initialization happens first ->
followed by the compat properties

So,
"DEFINE_PROP_BOOL("migrate-buffers", CRBState, migrate_buffers, true)"
gets executed first setting it to true by default and then, the compat
property is applied,
"GlobalProperty hw_compat_11_0[] = {
    { "tpm-crb", "migrate-buffers", "off"},
}"
setting it to false.

When is the user provided command line argument parsed?
For example launching qemu using:
    -machine pc-11.0 
    -device tpm-crb,tpmdev=tpm0,cap-chunk=on

Is it set in the end? In the above example, the user explicitly set
cap-chunk to true whereas the migrate-buffers default value is false.

Therefore we end up with the following scenario.
Since chunking is supported, the guest writes a large command in the
buffer. The user then decides to migrate the VM. QEMU sees that
migrate-buffer is off, so it does not migrate the buffers. When the VM
resumes on the destination host, it will find the buffers empty.

Therefore, we need to stop migration if cap-chunk is enabled but
migrate-buffers is disabled. Please guide.

> 
> > +};
> >  const size_t hw_compat_11_0_len = G_N_ELEMENTS(hw_compat_11_0);
> >
> >  GlobalProperty hw_compat_10_2[] = {
> > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> > index b9f295db7a..81471dd9f8 100644
> > --- a/hw/tpm/tpm_crb.c
> > +++ b/hw/tpm/tpm_crb.c
> > @@ -49,6 +49,8 @@ struct CRBState {
> >
> >      bool ppi_enabled;
> >      TPMPPI ppi;
> > +
> > +    bool migrate_buffers;
> >  };
> >  typedef struct CRBState CRBState;
> >
> > @@ -345,18 +347,47 @@ static int tpm_crb_pre_save(void *opaque)
> >      return 0;
> >  }
> >
> > +static bool tpm_crb_chunk_needed(void *opaque)
> > +{
> > +    CRBState *s = opaque;
> > +
> > +    if (!s->migrate_buffers) {
> > +        return false;
> > +    }
> > +
> > +    return ((s->command_buffer && s->command_buffer->len > 0) ||
> > +            (s->response_buffer && s->response_buffer->len > 0));
> > +}
> > +
> > +static const VMStateDescription vmstate_tpm_crb_chunk = {
> > +    .name = "tpm-crb/chunk",
> > +    .version_id = 0,
> > +    .needed = tpm_crb_chunk_needed,
> > +    .fields = (const VMStateField[]) {
> > +        VMSTATE_GBYTEARRAY(command_buffer, CRBState, 0),
> > +        VMSTATE_GBYTEARRAY(response_buffer, CRBState, 0),
> > +        VMSTATE_UINT32(response_offset, CRBState),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  static const VMStateDescription vmstate_tpm_crb = {
> >      .name = "tpm-crb",
> >      .pre_save = tpm_crb_pre_save,
> >      .fields = (const VMStateField[]) {
> >          VMSTATE_UINT32_ARRAY(regs, CRBState, TPM_CRB_R_MAX),
> >          VMSTATE_END_OF_LIST(),
> > +    },
> > +    .subsections = (const VMStateDescription * const []) {
> > +        &vmstate_tpm_crb_chunk,
> > +        NULL,
> >      }
> >  };
> >
> >  static const Property tpm_crb_properties[] = {
> >      DEFINE_PROP_TPMBE("tpmdev", CRBState, tpmbe),
> >      DEFINE_PROP_BOOL("ppi", CRBState, ppi_enabled, true),
> > +    DEFINE_PROP_BOOL("migrate-buffers", CRBState, migrate_buffers, true),
> >  };
> >
> >  static void tpm_crb_reset(void *dev)
> > --
> > 2.53.0
> >
>

Regards,
Arun Menon