On 06/27/18 17:05, Igor Mammedov wrote:
> On Wed, 27 Jun 2018 10:36:52 -0400
> Stefan Berger <stef...@linux.vnet.ibm.com> wrote:
>
>> On 06/27/2018 10:19 AM, Igor Mammedov wrote:
>>> On Wed, 27 Jun 2018 08:53:28 -0400
>>> Stefan Berger <stef...@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.lur...@redhat.com> wrote:
>>>>>
>>>>>> From: Stefan Berger <stef...@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 <stef...@linux.vnet.ibm.com>
>>>>>> Signed-off-by: Marc-André Lureau <marcandre.lur...@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 <stef...@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
> 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
