Hi Dave, On 2/8/22 6:03 PM, Dr. David Alan Gilbert wrote: > * Eric Auger (eric.au...@redhat.com) wrote: >> Representing the CRB cmd/response buffer as a standard >> RAM region causes some trouble when the device is used >> with VFIO. Indeed VFIO attempts to DMA_MAP this region >> as usual RAM but this latter does not have a valid page >> size alignment causing such an error report: >> "vfio_listener_region_add received unaligned region". >> To allow VFIO to detect that failing dma mapping >> this region is not an issue, let's use a ram_device >> memory region type instead. > Don't the weird sizes and alignments also cause problems with the RAM > migration code? I tested CRB migration and it seems to work properly. > Why don't you just align this up to a convenient boundary? The spec (CG PC Client Platform TPM Profile (PTP) Specification Family “2.0” Level 00 Revision 01.03 v22, page 100) says that the command/response data "may be defined as large as 3968", which is (0x1000 - 0x80), 0x80 being the size of the control struct. so the size of the region logically is less than a 4kB page, hence our trouble.
We learnt in the past Windows driver has some stronger expectation wrt memory mapping. I don't know if those latter would work if we were to enlarge the window by some tricks. Besides https://trustedcomputinggroup.org/wp-content/uploads/Mobile-Command-Response-Buffer-Interface-v2-r12-Specification_FINAL2.pdf says " Including the control structure, the three memory areas comprise the entirety of the CRB. There are no constraints on how those three memory areas are provided. They can all be in system RAM, or all be in device memory, or any combination. " So device memory looks an option too. Adding Marc-Andre in the loop Thanks Eric > > Dave > >> Signed-off-by: Eric Auger <eric.au...@redhat.com> >> Tested-by: Stefan Berger <stef...@linux.ibm.com> >> Acked-by: Stefan Berger <stef...@linux.ibm.com> >> [PMD: Keep tpm_crb.c in meson's softmmu_ss] >> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org> >> >> --- >> >> v3 -> v4: >> - call vmstate_unregister_ram >> --- >> hw/tpm/tpm_crb.c | 23 +++++++++++++++++++++-- >> 1 file changed, 21 insertions(+), 2 deletions(-) >> >> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c >> index 58ebd1469c3..668f969b409 100644 >> --- a/hw/tpm/tpm_crb.c >> +++ b/hw/tpm/tpm_crb.c >> @@ -25,6 +25,7 @@ >> #include "sysemu/tpm_backend.h" >> #include "sysemu/tpm_util.h" >> #include "sysemu/reset.h" >> +#include "exec/cpu-common.h" >> #include "tpm_prop.h" >> #include "tpm_ppi.h" >> #include "trace.h" >> @@ -43,6 +44,7 @@ struct CRBState { >> >> bool ppi_enabled; >> TPMPPI ppi; >> + uint8_t *crb_cmd_buf; >> }; >> typedef struct CRBState CRBState; >> >> @@ -291,10 +293,14 @@ static void tpm_crb_realize(DeviceState *dev, Error >> **errp) >> return; >> } >> >> + s->crb_cmd_buf = qemu_memalign(qemu_real_host_page_size, >> + HOST_PAGE_ALIGN(CRB_CTRL_CMD_SIZE)); >> + >> 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_init_ram_device_ptr(&s->cmdmem, OBJECT(s), "tpm-crb-cmd", >> + CRB_CTRL_CMD_SIZE, s->crb_cmd_buf); >> + vmstate_register_ram(&s->cmdmem, dev); >> >> memory_region_add_subregion(get_system_memory(), >> TPM_CRB_ADDR_BASE, &s->mmio); >> @@ -309,12 +315,25 @@ static void tpm_crb_realize(DeviceState *dev, Error >> **errp) >> qemu_register_reset(tpm_crb_reset, dev); >> } >> >> +static void tpm_crb_unrealize(DeviceState *dev) >> +{ >> + CRBState *s = CRB(dev); >> + >> + vmstate_unregister_ram(&s->cmdmem, dev); >> + qemu_vfree(s->crb_cmd_buf); >> + >> + if (s->ppi_enabled) { >> + qemu_vfree(s->ppi.buf); >> + } >> +} >> + >> static void tpm_crb_class_init(ObjectClass *klass, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(klass); >> TPMIfClass *tc = TPM_IF_CLASS(klass); >> >> dc->realize = tpm_crb_realize; >> + dc->unrealize = tpm_crb_unrealize; >> device_class_set_props(dc, tpm_crb_properties); >> dc->vmsd = &vmstate_tpm_crb; >> dc->user_creatable = true; >> -- >> 2.26.3 >>
