From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50656) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UIJOy-0005jd-09 for qemu-devel@nongnu.org; Wed, 20 Mar 2013 09:50:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UIJOr-0001C8-PC for qemu-devel@nongnu.org; Wed, 20 Mar 2013 09:49:59 -0400 Received: from e8.ny.us.ibm.com ([32.97.182.138]:59811) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UIJOr-0001Bp-KK for qemu-devel@nongnu.org; Wed, 20 Mar 2013 09:49:53 -0400 Received: from /spool/local by e8.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 20 Mar 2013 09:49:51 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id D2D8C6E805D for ; Wed, 20 Mar 2013 09:49:45 -0400 (EDT) Received: from d03av05.boulder.ibm.com (d03av05.boulder.ibm.com [9.17.195.85]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r2KDnkPG144788 for ; Wed, 20 Mar 2013 09:49:46 -0400 Received: from d03av05.boulder.ibm.com (loopback [127.0.0.1]) by d03av05.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r2KDniGs011671 for ; Wed, 20 Mar 2013 07:49:44 -0600 Message-ID: <5149BE73.9010509@linux.vnet.ibm.com> Date: Wed, 20 Mar 2013 09:49:39 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1363628125-5310-1-git-send-email-pbonzini@redhat.com> <1363628125-5310-5-git-send-email-pbonzini@redhat.com> In-Reply-To: <1363628125-5310-5-git-send-email-pbonzini@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 04/35] tpm: reorganize headers and split hardware part List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, Stefan Berger 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 > --- > 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; >