Re: [Qemu-devel] [PATCH v5 2/4] tpm: implement virtual memory device for TPM PPI

[Igor Mammedov]

On Wed, 27 Jun 2018 08:53:28 -0400
Stefan Berger <address@hidden> wrote:

> On 06/27/2018 07:44 AM, Igor Mammedov wrote:
> > On Tue, 26 Jun 2018 14:23:41 +0200
> > Marc-André Lureau <address@hidden> wrote:
> >  
> >> From: Stefan Berger <address@hidden>
> >>
> >> Implement a virtual memory device for the TPM Physical Presence interface.
> >> The memory is located at 0xFED45000 and used by ACPI to send messages to 
> >> the
> >> firmware (BIOS) and by the firmware to provide parameters for each one of
> >> the supported codes.
> >>
> >> This device should be used by all TPM interfaces on x86 and can be added
> >> by calling tpm_ppi_init_io().
> >>
> >> Signed-off-by: Stefan Berger <address@hidden>
> >> Signed-off-by: Marc-André Lureau <address@hidden>
> >>
> >> ---
> >>
> >> v4 (Marc-André):
> >>   - pass TPM_PPI_ADDR_BASE as argument to tpm_ppi_init_io()
> >>   - only enable PPI if property is set
> >>
> >> v3 (Marc-André):
> >>   - merge CRB support
> >>   - use trace events instead of DEBUG printf
> >>   - headers inclusion simplification
> >>
> >> v2:
> >>   - moved to byte access since an infrequently used device;
> >>     this simplifies code
> >>   - increase size of device to 0x400
> >>   - move device to 0xfffef000 since SeaBIOS has some code at 0xffff0000:
> >>     'On the emulators, the bios at 0xf0000 is also at 0xffff0000'
> >> ---
> >>   hw/tpm/tpm_ppi.h      | 27 ++++++++++++++++++++
> >>   include/hw/acpi/tpm.h |  6 +++++
> >>   hw/tpm/tpm_crb.c      |  7 ++++++
> >>   hw/tpm/tpm_ppi.c      | 57 +++++++++++++++++++++++++++++++++++++++++++
> >>   hw/tpm/tpm_tis.c      |  7 ++++++
> >>   hw/tpm/Makefile.objs  |  2 +-
> >>   hw/tpm/trace-events   |  4 +++
> >>   7 files changed, 109 insertions(+), 1 deletion(-)
> >>   create mode 100644 hw/tpm/tpm_ppi.h
> >>   create mode 100644 hw/tpm/tpm_ppi.c
> >>
> >> diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> >> new file mode 100644
> >> index 0000000000..ac7ad47238
> >> --- /dev/null
> >> +++ b/hw/tpm/tpm_ppi.h
> >> @@ -0,0 +1,27 @@
> >> +/*
> >> + * TPM Physical Presence Interface
> >> + *
> >> + * Copyright (C) 2018 IBM Corporation
> >> + *
> >> + * Authors:
> >> + *  Stefan Berger    <address@hidden>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2 or 
> >> later.
> >> + * See the COPYING file in the top-level directory.
> >> + */
> >> +#ifndef TPM_TPM_PPI_H
> >> +#define TPM_TPM_PPI_H
> >> +
> >> +#include "hw/acpi/tpm.h"
> >> +#include "exec/address-spaces.h"
> >> +
> >> +typedef struct TPMPPI {
> >> +    MemoryRegion mmio;
> >> +
> >> +    uint8_t ram[TPM_PPI_ADDR_SIZE];
> >> +} TPMPPI;  
> > I probably miss something obvious here,
> > 1st:
> > commit message says that memory reion is supposed to be interface
> > between FW and OSPM (i.e. totally guest internal thingy).
> > So question is:
> >    why do we register memory region at all?  
> 
> One reason for the device itself was being able to debug the interaction 
> of the guest with ACPI though I had additional instrumentation for that 
> showing register contents.
> We need it to have some memory in the region where we place it. I 
> suppose a memory_region_init_ram() would provide migration support 
> automatically but cannot be used on memory where we have 
> MemoryRegionOps. So we could drop most parts of the device and only run 
> memory_region_init_ram() ?
if QEMU doesn't need to touch it ever, you could do even better.
Use bios_linker to make FW allocate a reserved portion if guest RAM and
patch TPM aml code with allocated address. Then you won't have to worry
about migration as reserved area is already migrated as part of RAM.

Huge benefit here is that QEMU doesn't dictate address so no chance
of conflicts and related compat hacks in case we have to move it. 

To do it easily, I'd suggest to extract TPM from DSDT into a dedicated
SSDT table (see vmgenid_build_acpi() as example /you don't need
bios_linker_loader_write_pointer() part, later we can use it if
it would be necessary for QEMU to know address/)
 
>     Stefan
>