On Thu, 13 Jul 2023 at 15:18, Stefan Berger <stef...@linux.ibm.com> wrote:
>
>
>
> On 7/12/23 23:51, Joelle van Dyne wrote:
> > On Apple Silicon, when Windows performs a LDP on the CRB MMIO space,
> > the exception is not decoded by hardware and we cannot trap the MMIO
> > read. This led to the idea from @agraf to use the same mapping type as
> > ROM devices: namely that reads should be seen as memory type and
> > writes should trap as MMIO.
These are hardware registers, right? Windows shouldn't
really be doing LDP to those if it expects to be able to run
in a VM...
> > Once that was done, the second memory mapping of the command buffer
> > region was redundent and was removed.
> >
> > A note about the removal of the read trap for `CRB_LOC_STATE`:
> > The only usage was to return the most up-to-date value for
> > `tpmEstablished`. However, `tpmEstablished` is only set when a
> > TPM2_HashStart operation is called which only exists for locality 4.
> > Indeed, the comment for the write handler of `CRB_LOC_CTRL` makes the
> > same argument for why it is not calling the backend to reset the
> > `tpmEstablished` bit. As this bit is unused, we do not need to worry
> > about updating it for reads.
> >
> > Signed-off-by: Joelle van Dyne <j...@getutm.app>
> > ---
> > hw/tpm/tpm_crb.h | 2 -
> > hw/tpm/tpm_crb.c | 3 -
> > hw/tpm/tpm_crb_common.c | 124 ++++++++++++++++++++--------------------
> > 3 files changed, 63 insertions(+), 66 deletions(-)
> >
> > diff --git a/hw/tpm/tpm_crb.h b/hw/tpm/tpm_crb.h
> > index da3a0cf256..7cdd37335f 100644
> > --- a/hw/tpm/tpm_crb.h
> > +++ b/hw/tpm/tpm_crb.h
> > @@ -26,9 +26,7 @@
> > typedef struct TPMCRBState {
> > TPMBackend *tpmbe;
> > TPMBackendCmd cmd;
> > - uint32_t regs[TPM_CRB_R_MAX];
> > MemoryRegion mmio;
> > - MemoryRegion cmdmem;
> >
> > size_t be_buffer_size;
> >
> > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> > index 598c3e0161..07c6868d8d 100644
> > --- a/hw/tpm/tpm_crb.c
> > +++ b/hw/tpm/tpm_crb.c
> > @@ -68,7 +68,6 @@ static const VMStateDescription vmstate_tpm_crb_none = {
> > .name = "tpm-crb",
> > .pre_save = tpm_crb_none_pre_save,
> > .fields = (VMStateField[]) {
> > - VMSTATE_UINT32_ARRAY(state.regs, CRBState, TPM_CRB_R_MAX),
>
> This has to stay here otherwise we cannot restart VMs from saved state once
> QEMU is upgraded.
>
> 2023-07-13T14:15:43.997718Z qemu-system-x86_64: Unknown ramblock
> "tpm-crb-cmd", cannot accept migration
> 2023-07-13T14:15:43.997813Z qemu-system-x86_64: error while loading state for
> instance 0x0 of device 'ram'
> 2023-07-13T14:15:43.997841Z qemu-system-x86_64: load of migration failed:
> Invalid argument
More generally, for migration compatibility in the other
direction you need to use memory_region_init_rom_device_nomigrate()
and make sure you keep migrating the data via this, not
via the MemoryRegion.
I'm not a super-fan of hacking around the fact that LDP
to hardware registers isn't supported in specific device
models, though...
thanks
-- PMM
