Re: [Qemu-devel] [RFC PATCH 1/3] tpm: Implement virtual memory device for TPM PPI

[Stefan Berger]

On 01/12/2018 09:55 AM, Marc-André Lureau wrote:
> Hi
>
> On Wed, Jan 10, 2018 at 7:35 PM, Stefan Berger
> <address@hidden> wrote:
> > Implement a virtual memory device for the TPM physical
> > presence interface. The memory is located at 0xffff0000
> > and used by ACPI to send messages to the firmware (BIOS).
> >
> > This device should be used by all TPM interfaces on x86 and
> > can be added through by calling tpm_ppi_init_io().
> I haven't followed closely the current design discussion on seabios
> ML, so this is just a quick review:

I think what I should add is an ACPI table called QEMU that holds the 
address of this device so that we can move this device later on. SeaBIOS 
would find this table and use the address it finds there rather than the 
#define.

> > Signed-off-by: Stefan Berger <address@hidden>
> > ---
> >   hw/tpm/Makefile.objs  |  2 +-
> >   hw/tpm/tpm_ppi.c      | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >   hw/tpm/tpm_ppi.h      | 25 ++++++++++++++++
> >   hw/tpm/tpm_tis.c      |  5 ++++
> >   include/hw/acpi/tpm.h |  6 ++++
> >   5 files changed, 116 insertions(+), 1 deletion(-)
> >   create mode 100644 hw/tpm/tpm_ppi.c
> >   create mode 100644 hw/tpm/tpm_ppi.h
> >
> > diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
> > index 7a93b24..3eb0558 100644
> > --- a/hw/tpm/Makefile.objs
> > +++ b/hw/tpm/Makefile.objs
> > @@ -1,4 +1,4 @@
> > -common-obj-y += tpm_util.o
> > +common-obj-y += tpm_util.o tpm_ppi.o
> >   common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
> >   common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
> >   common-obj-$(CONFIG_TPM_EMULATOR) += tpm_emulator.o
> > diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
> > new file mode 100644
> > index 0000000..fa8c3c3
> > --- /dev/null
> > +++ b/hw/tpm/tpm_ppi.c
> > @@ -0,0 +1,79 @@
> > +/*
> > + * tpm_ppi.c - 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.
> > + *
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "exec/memory.h"
> > +#include "exec/address-spaces.h"
> > +
> > +#include "tpm_ppi.h"
> > +
> > +#define DEBUG_PPI 1
> to be switched to 0

Done.
> > +
> > +#define DPRINTF(fmt, ...) do { \
> > +    if (DEBUG_PPI) { \
> > +        printf(fmt, ## __VA_ARGS__); \
> > +    } \
> > +} while (0);
> > +
> > +static uint64_t tpm_ppi_mmio_read(void *opaque, hwaddr addr,
> > +                                  unsigned size)
> > +{
> > +    TPMPPI *s = opaque;
> > +    uint32_t val = 0;
> > +    int c;
> > +
> > +    for (c = size - 1; c >= 0; c--) {
> I would rather put the -1,
>
> > +        val <<= 8;
> > +        val |= s->ram[addr + c];
> ^ here, ymmv

you mean val |= s->ram[addr + c - 1]?

> > +    }
> > +
> > +    DPRINTF("tpm_ppi: read.%u(%08x) = %08x\n", size,
> > +            (unsigned int)addr, (unsigned int)val);
> > +
> > +    return val;
> > +}
> > +
> > +static void tpm_ppi_mmio_write(void *opaque, hwaddr addr,
> > +                               uint64_t val, unsigned size)
> > +{
> > +    TPMPPI *s = opaque;
> > +    int c;
> > +
> > +    DPRINTF("tpm_ppi: write.%u(%08x) = %08x\n", size,
> > +            (unsigned int)addr, (unsigned int)val);
> > +
> > +    for (c = 0; c <= size - 1; c++) {
> same
>
> (alternatively, why not cast the addr pointer?)

You mean write 16 bits and 32bits directly into the array? I have to see 
what happens when one writes beyond the end of the array...

> > +        s->ram[addr + c] = val;
> > +        val >>= 8;
> > +    }
> > +}
> > +
> > +static const MemoryRegionOps tpm_ppi_memory_ops = {
> > +    .read = tpm_ppi_mmio_read,
> > +    .write = tpm_ppi_mmio_write,
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> Shouldn't it be native endian?

Have to figure out what that means...

> > +    .valid = {
> > +        .min_access_size = 1,
> > +        .max_access_size = 4,
> > +    },
> > +};
> > +
> > +void tpm_ppi_init_io(TPMPPI *tpmppi, Object *obj)
> > +{
> > +    memory_region_init_io(&tpmppi->mmio, obj, &tpm_ppi_memory_ops,
> > +                          tpmppi, "tpm-ppi-mmio",
> > +                          TPM_PPI_ADDR_SIZE);
> > +
> > +    memory_region_add_subregion(get_system_memory(),
> > +                                TPM_PPI_ADDR_BASE, &tpmppi->mmio);
> > +}
> > diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> > new file mode 100644
> > index 0000000..8959912
> > --- /dev/null
> > +++ b/hw/tpm/tpm_ppi.h
> > @@ -0,0 +1,25 @@
> > +/*
> > + * 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"
> > +
> > +typedef struct TPMPPI {
> > +    MemoryRegion mmio;
> > +
> > +    uint8_t ram[TPM_PPI_ADDR_SIZE];
> > +} TPMPPI;
> > +
> > +void tpm_ppi_init_io(TPMPPI *tpmppi, Object *obj);
> > +
> > +#endif /* TPM_TPM_PPI_H */
> > diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> > index 561384c..f980972 100644
> > --- a/hw/tpm/tpm_tis.c
> > +++ b/hw/tpm/tpm_tis.c
> > @@ -31,6 +31,7 @@
> >   #include "sysemu/tpm_backend.h"
> >   #include "tpm_int.h"
> >   #include "tpm_util.h"
> > +#include "tpm_ppi.h"
> >
> >   #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
> >   #define TPM_TIS_LOCALITY_SHIFT      12
> > @@ -80,6 +81,8 @@ typedef struct TPMState {
> >       TPMVersion be_tpm_version;
> >
> >       size_t be_buffer_size;
> > +
> > +    TPMPPI ppi;
> >   } TPMState;
> >
> >   #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
> > @@ -1050,6 +1053,8 @@ static void tpm_tis_initfn(Object *obj)
> >       memory_region_init_io(&s->mmio, OBJECT(s), &tpm_tis_memory_ops,
> >                             s, "tpm-tis-mmio",
> >                             TPM_TIS_NUM_LOCALITIES << TPM_TIS_LOCALITY_SHIFT);
> > +
> > +    tpm_ppi_init_io(&s->ppi, obj);
> I suggest to pass the device isa address space there (instead of
> hooking the PPI only on system_memory).

I can do that. For my education, I wonder why, though.

> >   }
> >
> >   static void tpm_tis_class_init(ObjectClass *klass, void *data)
> > diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> > index 6d516c6..d9b7452 100644
> > --- a/include/hw/acpi/tpm.h
> > +++ b/include/hw/acpi/tpm.h
> > @@ -31,4 +31,10 @@
> >
> >   #define TPM2_START_METHOD_MMIO      6
> >
> > +/*
> > + * Physical Presence Interface
> > + */
> > +#define TPM_PPI_ADDR_SIZE           0x100
> > +#define TPM_PPI_ADDR_BASE           0xffff0000
> > +
> >   #endif /* HW_ACPI_TPM_H */
> > --
> > 2.5.5