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

[Laszlo Ersek]

On 06/27/18 17:05, Igor Mammedov wrote:
> 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.

Stability of reserved memory areas is only guaranteed across S3 resume.
Through a normal reboot, all DRAM is considered invalidated.

Thanks
Laszlo