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

[Stefan Berger]

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() ?


   Stefan