On 4/11/22 10:47, Anthony PERARD wrote:
> From: Anthony PERARD <anthony.perard@citrix.com>
>
> At the moment, there doesn't seems to be any way to know that QEMU
> made modification to the command buffer. This is potentially an issue
> on Xen while migrating a guest, as modification to the buffer after
> the migration as started could be ignored and not transfered to the
> destination. >
> Mark the memory region of the command buffer as dirty once a request
> is completed.
Not sure about the CRB...
The TIS at least carries the buffer as part of the state and stores it
when the interface is saved:
https://github.com/qemu/qemu/blob/v6.2.0/hw/tpm/tpm_tis_isa.c#L56
https://github.com/qemu/qemu/blob/v6.2.0/hw/tpm/tpm_tis_sysbus.c#L56
With the CRB the memory is registered using these functions.
memory_region_init_io(&s->mmio, OBJECT(s), &tpm_crb_memory_ops, s,
"tpm-crb-mmio", sizeof(s->regs));
memory_region_init_ram(&s->cmdmem, OBJECT(s),
"tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
memory_region_add_subregion(get_system_memory(),
TPM_CRB_ADDR_BASE, &s->mmio);
memory_region_add_subregion(get_system_memory(),
TPM_CRB_ADDR_BASE + sizeof(s->regs), &s->cmdmem);
The state of the registers is saved using this here:
static const VMStateDescription vmstate_tpm_crb = {
.name = "tpm-crb",
.pre_save = tpm_crb_pre_save,
.fields = (VMStateField[]) {
VMSTATE_UINT32_ARRAY(regs, CRBState, TPM_CRB_R_MAX),
VMSTATE_END_OF_LIST(),
}
};
The buffer with the commands is not part of this. So likely it needs to
be marked dirty but then I am not sure whether that is going to work if
the response to a command on the CRB is only received when
tpm_crb_pre_save() is called.. Maybe this buffer would have to be save
explicitly in a .save function or also as part of vmstate_tpm_crb... ?
The memory regions are migrated and the guest modification should be tracked.
However, when migrating the CRB device, CPUs should be paused, but the backend could indeed write some reply in the cmdmem memory.
I think the migration logic handles the case where RAM was already migrated but marked dirty again during a device migration. It would be nice if David or Juan could confirm.
If that's the case, the patch as is looks good to me.
thanks
Stefan
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>
> I have only read code to find out whether the tpm-crb device was fine
> with regards to migration, and I don't think there's anything that
> could mark the memory region as dirty once a request is completed.
>
> There is one call to memory_region_get_ram_ptr(), but nothing seems to
> be done with the pointer is regards to ram migration. Am I wrong?
>
> Thanks.
> ---
> hw/tpm/tpm_crb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index aa9c00aad3..67db594c48 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -197,6 +197,7 @@ static void tpm_crb_request_completed(TPMIf *ti, int ret)
> ARRAY_FIELD_DP32(s->regs, CRB_CTRL_STS,
> tpmSts, 1); /* fatal error */
> }
> + memory_region_set_dirty(&s->cmdmem, 0, CRB_CTRL_CMD_SIZE);
> }
>
> static enum TPMVersion tpm_crb_get_version(TPMIf *ti)