Hi
On Wed, Jan 10, 2018 at 7:35 PM, Stefan Berger
<stef...@linux.vnet.ibm.com> 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:
> Signed-off-by: Stefan Berger <stef...@linux.vnet.ibm.com>
> ---
> 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 <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.
> + *
> + */
> +
> +#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
> +
> +#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
> + }
> +
> + 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?)
> + 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?
> + .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 <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"
> +
> +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).
> }
>
> 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
>
>
--
Marc-André Lureau
