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

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 27 Jun 2018 16:29:51 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> On Wed, Jun 27, 2018 at 4:19 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Wed, 27 Jun 2018 08:53:28 -0400
> > Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
> >  
> >> On 06/27/2018 07:44 AM, Igor Mammedov wrote:  
> >> > On Tue, 26 Jun 2018 14:23:41 +0200
> >> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >> >  
> >> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> >>
> >> >> 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 <stefanb@linux.vnet.ibm.com>
> >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >>
> >> >> ---
> >> >>
> >> >> 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    <stefanb@us.ibm.com>
> >> >> + *
> >> >> + * 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.  
> 
> The location needs to be fixed across reboots (or content moved
> somehow..) And timing-wise, I am not sure the ACPI tables are
> installed and processed by firmware before the TPM PPI operation must
> be handled.
true,
maybe add a note/reason to commit message why bios_linker couldn't be used,
to prevent the same question being raised again or wasting cycles on
'optimizing' code later with bios_linker when nobody recalls why it can't be used.

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