[PATCH] tpm_crb: mark command buffer as dirty on request completion

[PATCH] tpm_crb: mark command buffer as dirty on request completion

[Anthony PERARD via]

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.

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)
-- 
Anthony PERARD

Re: [PATCH] tpm_crb: mark command buffer as dirty on request completion

[Stefan Berger]

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


   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)

Re: [PATCH] tpm_crb: mark command buffer as dirty on request completion

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3701 bytes --]

Hi

On Mon, Apr 11, 2022 at 8:32 PM Stefan Berger <stefanb@linux.ibm.com> wrote:

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

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 5194 bytes --]

Re: [PATCH] tpm_crb: mark command buffer as dirty on request completion

[Anthony PERARD via]

On Mon, Apr 11, 2022 at 12:31:01PM -0400, Stefan Berger wrote:
> On 4/11/22 10:47, Anthony PERARD wrote:
> > From: Anthony PERARD <anthony.perard@citrix.com>
> 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... ?

I did some research on migration of a guest with Xen toolstack, and I
think there is one last round of sending memory marked as dirty after we
call "xen-save-devices-state" (with the guest suspended), that's when
tpm_crb_pre_save() is called. Hopefully, when QEMU is in charge of
migration, it does the same thing and there would not be a need to save
the buffer in vmstate of this device.

Cheers,

-- 
Anthony PERARD

Re: [PATCH] tpm_crb: mark command buffer as dirty on request completion

[Stefan Berger]

On 4/11/22 10:47, Anthony PERARD via 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.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.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)