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

[Igor Mammedov]

On Wed, 27 Jun 2018 10:36:52 -0400
Stefan Berger <address@hidden> wrote:

> On 06/27/2018 10:19 AM, Igor Mammedov wrote:
> > 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.  
> 
> QEMU does indirectly touch it in 4/4 where we define the 
> OperationRegion()s and need to know where they are located in memory. We 
> could read the base address that is now TPM_PPI_ADDR_BASE from a hard 
> coded memory location
that's done for you by bios_linker_loader_add_pointer() when
ACPI tables are installed by FW.

> and pass it into OperationRegion(), but I doubt we 
> would want that.
> 
> +    aml_append(dev,
> +               aml_operation_region("TPP1", AML_SYSTEM_MEMORY,
> +                                    aml_int(TPM_PPI_ADDR_BASE), 0x100));
> 
> 
> +    aml_append(dev,
> +               aml_operation_region("TPP2", AML_SYSTEM_MEMORY,
> +                                    aml_int(TPM_PPI_ADDR_BASE + 0x100),
> +                                    0x5A));
that's possible, usually it works as dynamic memory region where region
lives within scope of a method.

but scratch it.
As Andre pointed out reserved memory should stay at the same place
across reboots and might be needed before ACPI tables are installed,
which probably is impossible.

CCing Laszlo just in case if I'm wrong.


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