| Message ID | 20220411144749.47185-1-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm_crb: mark command buffer as dirty on request completion | expand |
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)
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) > >
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,
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)
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)