From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44170) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UIKQp-0007vc-0A for qemu-devel@nongnu.org; Wed, 20 Mar 2013 10:56:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UIKQm-0000xs-4w for qemu-devel@nongnu.org; Wed, 20 Mar 2013 10:55:58 -0400 Received: from mx4-phx2.redhat.com ([209.132.183.25]:38245) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UIKQl-0000xX-NN for qemu-devel@nongnu.org; Wed, 20 Mar 2013 10:55:56 -0400 Date: Wed, 20 Mar 2013 10:55:29 -0400 (EDT) From: Paolo Bonzini Message-ID: <652790236.11019049.1363791329233.JavaMail.root@redhat.com> In-Reply-To: <5149BE73.9010509@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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: Corey Bryant Cc: qemu-devel@nongnu.org, Stefan Berger ----- Messaggio originale ----- > Da: "Corey Bryant" > A: "Paolo Bonzini" > Cc: qemu-devel@nongnu.org, "Stefan Berger" > Inviato: Mercoled=C3=AC, 20 marzo 2013 14:49:39 > Oggetto: Re: [Qemu-devel] [PATCH 04/35] tpm: reorganize headers and split= hardware part >=20 >=20 >=20 > On 03/18/2013 01:34 PM, Paolo Bonzini wrote: > > The TPM subsystem does not have a good front-end/back-end > > separation. >=20 > 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).=20 Paolo >=20 > > However, we can at least try to split the user interface (tpm.c) > > from > > the implementation (hw/tpm). >=20 > Here's a general break-down of front-end/back-end/general code >=20 > tpm_tis.c =3D TIS front-end (the only front-end at this time) > tpm_passthrough.c =3D Passthrough backend (the only back-end at this > time) > tpm.c =3D general code > tpm_backend.c =3D backend thread pool functions (could use a better > file > name?) >=20 > > > > 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 =3D> hw/tpm}/Makefile.objs | 3 +- > > {tpm =3D> hw/tpm}/tpm_backend.c | 14 ++++++++++ > > {tpm =3D> hw/tpm}/tpm_int.h | 55 > > ++---------------------------------- > > {tpm =3D> hw/tpm}/tpm_passthrough.c | 0 > > {tpm =3D> hw/tpm}/tpm_tis.c | 0 > > {tpm =3D> hw/tpm}/tpm_tis.h | 5 ---- > > {tpm =3D> include/tpm}/tpm_backend.h | 57 > > ++++++++++++++++++++++++++++++++++++++ > > tpm/tpm.c =3D> tpm.c | 18 ++---------- > > 12 files changed, 80 insertions(+), 79 deletions(-) > > rename {tpm =3D> hw/tpm}/Makefile.objs (61%) > > rename {tpm =3D> hw/tpm}/tpm_backend.c (82%) > > rename {tpm =3D> hw/tpm}/tpm_int.h (49%) > > rename {tpm =3D> hw/tpm}/tpm_passthrough.c (100%) > > rename {tpm =3D> hw/tpm}/tpm_tis.c (100%) > > rename {tpm =3D> hw/tpm}/tpm_tis.h (94%) > > rename {tpm =3D> include/tpm}/tpm_backend.h (50%) > > rename tpm/tpm.c =3D> 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 +=3D bt-host.o bt-vhci.o > > > > common-obj-y +=3D dma-helpers.o > > common-obj-y +=3D vl.o > > -common-obj-y +=3D tpm/ > > +common-obj-y +=3D tpm.o > > > > common-obj-$(CONFIG_SLIRP) +=3D 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=3Dy > > CONFIG_APPLESMC=3Dy > > CONFIG_I8259=3Dy > > CONFIG_PFLASH_CFI01=3Dy > > -CONFIG_TPM_TIS=3D$(CONFIG_TPM) > > +CONFIG_TPM_TIS=3Dy > > 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=3Dy > > CONFIG_APPLESMC=3Dy > > CONFIG_I8259=3Dy > > CONFIG_PFLASH_CFI01=3Dy > > -CONFIG_TPM_TIS=3D$(CONFIG_TPM) > > +CONFIG_TPM_TIS=3Dy > > 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) +=3D scsi/ > > devices-dirs-$(CONFIG_SOFTMMU) +=3D sd/ > > devices-dirs-$(CONFIG_SOFTMMU) +=3D ssi/ > > devices-dirs-$(CONFIG_SOFTMMU) +=3D timer/ > > +devices-dirs-$(CONFIG_TPM) +=3D tpm/ > > devices-dirs-$(CONFIG_SOFTMMU) +=3D usb/ > > devices-dirs-$(CONFIG_SOFTMMU) +=3D virtio/ > > devices-dirs-$(CONFIG_SOFTMMU) +=3D 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 =3D tpm.o > > -common-obj-$(CONFIG_TPM) +=3D tpm_backend.o > > +common-obj-y +=3D tpm_backend.o > > common-obj-$(CONFIG_TPM_TIS) +=3D tpm_tis.o > > common-obj-$(CONFIG_TPM_PASSTHROUGH) +=3D 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 >=3D sizeof(struct tpm_resp_hdr)) { > > + struct tpm_resp_hdr *resp =3D (struct tpm_resp_hdr *)out; > > + > > + resp->tag =3D cpu_to_be16(TPM_TAG_RSP_COMMAND); > > + resp->len =3D cpu_to_be32(sizeof(struct tpm_resp_hdr)); > > + resp->errcode =3D cpu_to_be32(TPM_FAIL); > > + } > > +} >=20 > 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. >=20 > -- > Regards, > Corey Bryant >=20 > > 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 =3D 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 >=3D sizeof(struct tpm_resp_hdr)) { > > - struct tpm_resp_hdr *resp =3D (struct tpm_resp_hdr *)out; > > - > > - resp->tag =3D cpu_to_be16(TPM_TAG_RSP_COMMAND); > > - resp->len =3D cpu_to_be32(sizeof(struct tpm_resp_hdr)); > > - resp->errcode =3D 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; > > >=20 >=20 >=20 >=20