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

[Marc-André Lureau]

On Wed, Jun 27, 2018 at 4:19 PM, Igor Mammedov <address@hidden> 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.
> 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.

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



-- 
Marc-André Lureau