Re: [Qemu-devel] [PATCH 04/35] tpm: reorganize headers and split hardware part

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 03/18/2013 01:34 PM, Paolo Bonzini wrote:
> The TPM subsystem does not have a good front-end/back-end separation.

I think it has very good front-end/back-end separation, but perhaps you 
mean the code could be moved around and better organized.

> However, we can at least try to split the user interface (tpm.c) from
> the implementation (hw/tpm).

Here's a general break-down of front-end/back-end/general code

tpm_tis.c = TIS front-end (the only front-end at this time)
tpm_passthrough.c = Passthrough backend (the only back-end at this time)
tpm.c = general code
tpm_backend.c = backend thread pool functions (could use a better file 
name?)

>
> The patches makes tpm.c not include tpm_int.h; instead it moves more
> stuff to tpm_backend.h.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   Makefile.objs                      |  2 +-
>   default-configs/i386-softmmu.mak   |  2 +-
>   default-configs/x86_64-softmmu.mak |  2 +-
>   hw/Makefile.objs                   |  1 +
>   {tpm => hw/tpm}/Makefile.objs      |  3 +-
>   {tpm => hw/tpm}/tpm_backend.c      | 14 ++++++++++
>   {tpm => hw/tpm}/tpm_int.h          | 55 ++----------------------------------
>   {tpm => hw/tpm}/tpm_passthrough.c  |  0
>   {tpm => hw/tpm}/tpm_tis.c          |  0
>   {tpm => hw/tpm}/tpm_tis.h          |  5 ----
>   {tpm => include/tpm}/tpm_backend.h | 57 ++++++++++++++++++++++++++++++++++++++
>   tpm/tpm.c => tpm.c                 | 18 ++----------
>   12 files changed, 80 insertions(+), 79 deletions(-)
>   rename {tpm => hw/tpm}/Makefile.objs (61%)
>   rename {tpm => hw/tpm}/tpm_backend.c (82%)
>   rename {tpm => hw/tpm}/tpm_int.h (49%)
>   rename {tpm => hw/tpm}/tpm_passthrough.c (100%)
>   rename {tpm => hw/tpm}/tpm_tis.c (100%)
>   rename {tpm => hw/tpm}/tpm_tis.h (94%)
>   rename {tpm => include/tpm}/tpm_backend.h (50%)
>   rename tpm/tpm.c => tpm.c (93%)
>
> diff --git a/Makefile.objs b/Makefile.objs
> index f99841c..ff3a6b3 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -74,7 +74,7 @@ common-obj-y += bt-host.o bt-vhci.o
>
>   common-obj-y += dma-helpers.o
>   common-obj-y += vl.o
> -common-obj-y += tpm/
> +common-obj-y += tpm.o
>
>   common-obj-$(CONFIG_SLIRP) += slirp/
>
> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
> index 7d8908f..f70594d 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -26,4 +26,4 @@ CONFIG_HPET=y
>   CONFIG_APPLESMC=y
>   CONFIG_I8259=y
>   CONFIG_PFLASH_CFI01=y
> -CONFIG_TPM_TIS=$(CONFIG_TPM)
> +CONFIG_TPM_TIS=y
> diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
> index e87e644..66c4855 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -26,4 +26,4 @@ CONFIG_HPET=y
>   CONFIG_APPLESMC=y
>   CONFIG_I8259=y
>   CONFIG_PFLASH_CFI01=y
> -CONFIG_TPM_TIS=$(CONFIG_TPM)
> +CONFIG_TPM_TIS=y
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 09fea2c..5626292 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -25,6 +25,7 @@ devices-dirs-$(CONFIG_SOFTMMU) += scsi/
>   devices-dirs-$(CONFIG_SOFTMMU) += sd/
>   devices-dirs-$(CONFIG_SOFTMMU) += ssi/
>   devices-dirs-$(CONFIG_SOFTMMU) += timer/
> +devices-dirs-$(CONFIG_TPM) += tpm/
>   devices-dirs-$(CONFIG_SOFTMMU) += usb/
>   devices-dirs-$(CONFIG_SOFTMMU) += virtio/
>   devices-dirs-$(CONFIG_SOFTMMU) += watchdog/
> diff --git a/tpm/Makefile.objs b/hw/tpm/Makefile.objs
> similarity index 61%
> rename from tpm/Makefile.objs
> rename to hw/tpm/Makefile.objs
> index 366e4a7..8bbed7a 100644
> --- a/tpm/Makefile.objs
> +++ b/hw/tpm/Makefile.objs
> @@ -1,4 +1,3 @@
> -common-obj-y = tpm.o
> -common-obj-$(CONFIG_TPM) += tpm_backend.o
> +common-obj-y += tpm_backend.o
>   common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
>   common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
> diff --git a/tpm/tpm_backend.c b/hw/tpm/tpm_backend.c
> similarity index 82%
> rename from tpm/tpm_backend.c
> rename to hw/tpm/tpm_backend.c
> index 4144ef7..31d833c 100644
> --- a/tpm/tpm_backend.c
> +++ b/hw/tpm/tpm_backend.c
> @@ -56,3 +56,17 @@ void tpm_backend_thread_tpm_reset(TPMBackendThread *tbt,
>                              NULL);
>       }
>   }
> +
> +/*
> + * Write an error message in the given output buffer.
> + */
> +void tpm_write_fatal_error_response(uint8_t *out, uint32_t out_len)
> +{
> +    if (out_len >= sizeof(struct tpm_resp_hdr)) {
> +        struct tpm_resp_hdr *resp = (struct tpm_resp_hdr *)out;
> +
> +        resp->tag = cpu_to_be16(TPM_TAG_RSP_COMMAND);
> +        resp->len = cpu_to_be32(sizeof(struct tpm_resp_hdr));
> +        resp->errcode = cpu_to_be32(TPM_FAIL);
> +    }
> +}

I don't think moving this from tpm.c to tpm_backend.c helps anything. 
Maybe just renaming some of the files mentioned above might make the 
front-end vs back-end vs general code more intuitive.

-- 
Regards,
Corey Bryant

> diff --git a/tpm/tpm_int.h b/hw/tpm/tpm_int.h
> similarity index 49%
> rename from tpm/tpm_int.h
> rename to hw/tpm/tpm_int.h
> index f705643..d5f7bb8 100644
> --- a/tpm/tpm_int.h
> +++ b/hw/tpm/tpm_int.h
> @@ -15,27 +15,8 @@
>   #include "exec/memory.h"
>   #include "tpm/tpm_tis.h"
>
> -struct TPMDriverOps;
> -typedef struct TPMDriverOps TPMDriverOps;
> -
> -typedef struct TPMPassthruState TPMPassthruState;
> -
> -typedef struct TPMBackend {
> -    char *id;
> -    enum TpmModel fe_model;
> -    char *path;
> -    char *cancel_path;
> -    const TPMDriverOps *ops;
> -
> -    union {
> -        TPMPassthruState *tpm_pt;
> -    } s;
> -
> -    QLIST_ENTRY(TPMBackend) list;
> -} TPMBackend;
> -
>   /* overall state of the TPM interface */
> -typedef struct TPMState {
> +struct TPMState {
>       ISADevice busdev;
>       MemoryRegion mmio;
>
> @@ -48,38 +29,10 @@ typedef struct TPMState {
>
>       char *backend;
>       TPMBackend *be_driver;
> -} TPMState;
> +};
>
>   #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
>
> -typedef void (TPMRecvDataCB)(TPMState *, uint8_t locty);
> -
> -struct TPMDriverOps {
> -    enum TpmType type;
> -    /* get a descriptive text of the backend to display to the user */
> -    const char *(*desc)(void);
> -
> -    TPMBackend *(*create)(QemuOpts *opts, const char *id);
> -    void (*destroy)(TPMBackend *t);
> -
> -    /* initialize the backend */
> -    int (*init)(TPMBackend *t, TPMState *s, TPMRecvDataCB *datacb);
> -    /* start up the TPM on the backend */
> -    int (*startup_tpm)(TPMBackend *t);
> -    /* returns true if nothing will ever answer TPM requests */
> -    bool (*had_startup_error)(TPMBackend *t);
> -
> -    size_t (*realloc_buffer)(TPMSizedBuffer *sb);
> -
> -    void (*deliver_request)(TPMBackend *t);
> -
> -    void (*reset)(TPMBackend *t);
> -
> -    void (*cancel_cmd)(TPMBackend *t);
> -
> -    bool (*get_tpm_established_flag)(TPMBackend *t);
> -};
> -
>   struct tpm_req_hdr {
>       uint16_t tag;
>       uint32_t len;
> @@ -105,10 +58,6 @@ struct tpm_resp_hdr {
>   #define TPM_ORD_GetTicks          0xf1
>
>   TPMBackend *qemu_find_tpm(const char *id);
> -int tpm_register_model(enum TpmModel model);
> -int tpm_register_driver(const TPMDriverOps *tdo);
> -void tpm_display_backend_drivers(void);
> -const TPMDriverOps *tpm_get_backend_driver(const char *type);
>   void tpm_write_fatal_error_response(uint8_t *out, uint32_t out_len);
>
>   extern const TPMDriverOps tpm_passthrough_driver;
> diff --git a/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
> similarity index 100%
> rename from tpm/tpm_passthrough.c
> rename to hw/tpm/tpm_passthrough.c
> diff --git a/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> similarity index 100%
> rename from tpm/tpm_tis.c
> rename to hw/tpm/tpm_tis.c
> diff --git a/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h
> similarity index 94%
> rename from tpm/tpm_tis.h
> rename to hw/tpm/tpm_tis.h
> index 963682c..916152a 100644
> --- a/tpm/tpm_tis.h
> +++ b/hw/tpm/tpm_tis.h
> @@ -35,11 +35,6 @@
>   #define TYPE_TPM_TIS                "tpm-tis"
>
>
> -typedef struct TPMSizedBuffer {
> -    uint32_t size;
> -    uint8_t  *buffer;
> -} TPMSizedBuffer;
> -
>   typedef enum {
>       TPM_TIS_STATE_IDLE = 0,
>       TPM_TIS_STATE_READY,
> diff --git a/tpm/tpm_backend.h b/include/tpm/tpm_backend.h
> similarity index 50%
> rename from tpm/tpm_backend.h
> rename to include/tpm/tpm_backend.h
> index 05d94d0..f5390b4 100644
> --- a/tpm/tpm_backend.h
> +++ b/include/tpm/tpm_backend.h
> @@ -42,4 +42,61 @@ typedef enum TPMBackendCmd {
>       TPM_BACKEND_CMD_TPM_RESET,
>   } TPMBackendCmd;
>
> +struct TPMDriverOps;
> +typedef struct TPMDriverOps TPMDriverOps;
> +
> +typedef struct TPMState TPMState;
> +typedef struct TPMPassthruState TPMPassthruState;
> +
> +typedef struct TPMBackend {
> +    char *id;
> +    enum TpmModel fe_model;
> +    char *path;
> +    char *cancel_path;
> +    const TPMDriverOps *ops;
> +
> +    union {
> +        TPMPassthruState *tpm_pt;
> +    } s;
> +
> +    QLIST_ENTRY(TPMBackend) list;
> +} TPMBackend;
> +
> +typedef void (TPMRecvDataCB)(TPMState *, uint8_t locty);
> +
> +typedef struct TPMSizedBuffer {
> +    uint32_t size;
> +    uint8_t  *buffer;
> +} TPMSizedBuffer;
> +
> +struct TPMDriverOps {
> +    enum TpmType type;
> +    /* get a descriptive text of the backend to display to the user */
> +    const char *(*desc)(void);
> +
> +    TPMBackend *(*create)(QemuOpts *opts, const char *id);
> +    void (*destroy)(TPMBackend *t);
> +
> +    /* initialize the backend */
> +    int (*init)(TPMBackend *t, TPMState *s, TPMRecvDataCB *datacb);
> +    /* start up the TPM on the backend */
> +    int (*startup_tpm)(TPMBackend *t);
> +    /* returns true if nothing will ever answer TPM requests */
> +    bool (*had_startup_error)(TPMBackend *t);
> +
> +    size_t (*realloc_buffer)(TPMSizedBuffer *sb);
> +
> +    void (*deliver_request)(TPMBackend *t);
> +
> +    void (*reset)(TPMBackend *t);
> +
> +    void (*cancel_cmd)(TPMBackend *t);
> +
> +    bool (*get_tpm_established_flag)(TPMBackend *t);
> +};
> +
> +const TPMDriverOps *tpm_get_backend_driver(const char *type);
> +int tpm_register_model(enum TpmModel model);
> +int tpm_register_driver(const TPMDriverOps *tdo);
> +
>   #endif /* TPM_TPM_BACKEND_H */
> diff --git a/tpm/tpm.c b/tpm.c
> similarity index 93%
> rename from tpm/tpm.c
> rename to tpm.c
> index ffd2495..49ac4cc 100644
> --- a/tpm/tpm.c
> +++ b/tpm.c
> @@ -15,7 +15,7 @@
>
>   #include "monitor/monitor.h"
>   #include "qapi/qmp/qerror.h"
> -#include "tpm_int.h"
> +#include "tpm/tpm_backend.h"
>   #include "tpm/tpm.h"
>   #include "qemu/config-file.h"
>   #include "qmp-commands.h"
> @@ -61,20 +61,6 @@ static bool tpm_model_is_registered(enum TpmModel model)
>       return false;
>   }
>
> -/*
> - * Write an error message in the given output buffer.
> - */
> -void tpm_write_fatal_error_response(uint8_t *out, uint32_t out_len)
> -{
> -    if (out_len >= sizeof(struct tpm_resp_hdr)) {
> -        struct tpm_resp_hdr *resp = (struct tpm_resp_hdr *)out;
> -
> -        resp->tag = cpu_to_be16(TPM_TAG_RSP_COMMAND);
> -        resp->len = cpu_to_be32(sizeof(struct tpm_resp_hdr));
> -        resp->errcode = cpu_to_be32(TPM_FAIL);
> -    }
> -}
> -
>   const TPMDriverOps *tpm_get_backend_driver(const char *type)
>   {
>       int i;
> @@ -108,7 +94,7 @@ int tpm_register_driver(const TPMDriverOps *tdo)
>    * Walk the list of available TPM backend drivers and display them on the
>    * screen.
>    */
> -void tpm_display_backend_drivers(void)
> +static void tpm_display_backend_drivers(void)
>   {
>       int i;
>