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

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

On 06/27/2018 10:19 AM, Igor Mammedov 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.

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



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