Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping

[Stefan Berger <stefanb@linux.ibm.com>]

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


    Stefan