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

[Paolo Bonzini]

----- Messaggio originale -----
> Da: "Corey Bryant" <address@hidden>
> A: "Paolo Bonzini" <address@hidden>
> Cc: address@hidden, "Stefan Berger" <address@hidden>
> Inviato: Mercoledì, 20 marzo 2013 14:49:39
> Oggetto: Re: [Qemu-devel] [PATCH 04/35] tpm: reorganize headers and split 
> hardware part
> 
> 
> 
> 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.

I mean that the back-end has knowledge about the front-end.
The tpm_int.h header has a dependency on tpm_tis.h and on TPMState
(thus on ISADevice, which should be hidden to front-ends). 

Paolo

> 
> > 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 <address@hidden>
> > ---
> >   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;
> >
> 
> 
> 
>