Re: [Qemu-devel] [PATCH 14/42] tpm: add TPMBackendCmd to hold the request state

[Marc-André Lureau]

Hi

----- Original Message -----
> On 10/09/2017 06:55 PM, Marc-André Lureau wrote:
> > This simplifies a bit locality handling, and argument passing, and
> > could pave the way to queuing requests (if that makes sense).
> 
> We won't queue requests. The TPM interfaces all send one request and
> expect the driver to wait until the response comes back.

Even on different localities? (I am not familiar enough with that part)

> 
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> 
> Reviewed-by: Stefan Berger <address@hidden>
> 
> > ---
> >   hw/tpm/tpm_int.h             |  1 +
> >   include/sysemu/tpm_backend.h | 16 +++++++++++++---
> >   backends/tpm.c               |  6 +++---
> >   hw/tpm/tpm_emulator.c        | 29 +++++++++++++++--------------
> >   hw/tpm/tpm_passthrough.c     | 24 +++++-------------------
> >   hw/tpm/tpm_tis.c             | 18 +++++++++++++-----
> >   6 files changed, 50 insertions(+), 44 deletions(-)
> >
> > diff --git a/hw/tpm/tpm_int.h b/hw/tpm/tpm_int.h
> > index f2f285b3cc..6d7b3dc850 100644
> > --- a/hw/tpm/tpm_int.h
> > +++ b/hw/tpm/tpm_int.h
> > @@ -26,6 +26,7 @@ struct TPMState {
> >   
> >       uint8_t     locty_number;
> >       TPMLocality *locty_data;
> > +    TPMBackendCmd cmd;
> >   
> >       char *backend;
> >       TPMBackend *be_driver;
> > diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
> > index 9c83a512e1..3bb90be3de 100644
> > --- a/include/sysemu/tpm_backend.h
> > +++ b/include/sysemu/tpm_backend.h
> > @@ -30,7 +30,16 @@
> >   typedef struct TPMBackendClass TPMBackendClass;
> >   typedef struct TPMBackend TPMBackend;
> >   
> > -typedef void (TPMRecvDataCB)(TPMState *, bool selftest_done);
> > +typedef void (TPMRecvDataCB)(TPMState *);
> > +
> > +typedef struct TPMBackendCmd {
> > +    uint8_t locty;
> > +    const uint8_t *in;
> > +    uint32_t in_len;
> > +    uint8_t *out;
> > +    uint32_t out_len;
> > +    bool selftest_done;
> > +} TPMBackendCmd;
> >   
> >   struct TPMBackend {
> >       Object parent;
> > @@ -76,7 +85,7 @@ struct TPMBackendClass {
> >   
> >       void (*opened)(TPMBackend *s, Error **errp);
> >   
> > -    void (*handle_request)(TPMBackend *s);
> > +    void (*handle_request)(TPMBackend *s, TPMBackendCmd *cmd);
> >   };
> >   
> >   /**
> > @@ -121,11 +130,12 @@ bool tpm_backend_had_startup_error(TPMBackend *s);
> >   /**
> >    * tpm_backend_deliver_request:
> >    * @s: the backend to send the request to
> > + * @cmd: the command to deliver
> >    *
> >    * Send a request to the backend. The backend will then send the request
> >    * to the TPM implementation.
> >    */
> > -void tpm_backend_deliver_request(TPMBackend *s);
> > +void tpm_backend_deliver_request(TPMBackend *s, TPMBackendCmd *cmd);
> >   
> >   /**
> >    * tpm_backend_reset:
> > diff --git a/backends/tpm.c b/backends/tpm.c
> > index 34e82085ec..dc7c831ff8 100644
> > --- a/backends/tpm.c
> > +++ b/backends/tpm.c
> > @@ -25,7 +25,7 @@ static void tpm_backend_worker_thread(gpointer data,
> > gpointer user_data)
> >       TPMBackendClass *k  = TPM_BACKEND_GET_CLASS(s);
> >   
> >       assert(k->handle_request != NULL);
> > -    k->handle_request(s);
> > +    k->handle_request(s, (TPMBackendCmd *)data);
> >   }
> >   
> >   static void tpm_backend_thread_end(TPMBackend *s)
> > @@ -76,9 +76,9 @@ bool tpm_backend_had_startup_error(TPMBackend *s)
> >       return s->had_startup_error;
> >   }
> >   
> > -void tpm_backend_deliver_request(TPMBackend *s)
> > +void tpm_backend_deliver_request(TPMBackend *s, TPMBackendCmd *cmd)
> >   {
> > -    g_thread_pool_push(s->thread_pool, NULL, NULL);
> > +    g_thread_pool_push(s->thread_pool, cmd, NULL);
> >   }
> >   
> >   void tpm_backend_reset(TPMBackend *s)
> > diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
> > index 4fe405353a..788ab9876d 100644
> > --- a/hw/tpm/tpm_emulator.c
> > +++ b/hw/tpm/tpm_emulator.c
> > @@ -172,28 +172,29 @@ static int tpm_emulator_set_locality(TPMEmulator
> > *tpm_emu, uint8_t locty_number)
> >       return 0;
> >   }
> >   
> > -static void tpm_emulator_handle_request(TPMBackend *tb)
> > +static void tpm_emulator_handle_request(TPMBackend *tb, TPMBackendCmd
> > *cmd)
> >   {
> >       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
> > -    TPMLocality *locty = NULL;
> > -    bool selftest_done = false;
> >       Error *err = NULL;
> >   
> >       DPRINTF("processing TPM command");
> >   
> > -    locty = tb->tpm_state->locty_data;
> > -    if (tpm_emulator_set_locality(tpm_emu,
> > -                                  tb->tpm_state->locty_number) < 0 ||
> > -        tpm_emulator_unix_tx_bufs(tpm_emu, locty->w_buffer.buffer,
> > -                                  locty->w_offset, locty->r_buffer.buffer,
> > -                                  locty->r_buffer.size, &selftest_done,
> > -                                  &err) < 0) {
> > -        tpm_util_write_fatal_error_response(locty->r_buffer.buffer,
> > -                                            locty->r_buffer.size);
> > -        error_report_err(err);
> > +    if (tpm_emulator_set_locality(tpm_emu, tb->tpm_state->locty_number) <
> > 0) {
> > +        goto error;
> > +    }
> > +
> > +    if (tpm_emulator_unix_tx_bufs(tpm_emu, cmd->in, cmd->in_len,
> > +                                  cmd->out, cmd->out_len,
> > +                                  &cmd->selftest_done, &err) < 0) {
> > +        goto error;
> >       }
> >   
> > -    tb->recv_data_callback(tb->tpm_state, selftest_done);
> > +    tb->recv_data_callback(tb->tpm_state);
> > +    return;
> > +
> > +error:
> > +    tpm_util_write_fatal_error_response(cmd->out, cmd->out_len);
> > +    error_report_err(err);
> >   }
> >   
> >   static int tpm_emulator_probe_caps(TPMEmulator *tpm_emu)
> > diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
> > index 0ae4596932..93d72b8e9e 100644
> > --- a/hw/tpm/tpm_passthrough.c
> > +++ b/hw/tpm/tpm_passthrough.c
> > @@ -137,30 +137,16 @@ err_exit:
> >       return ret;
> >   }
> >   
> > -static int tpm_passthrough_unix_transfer(TPMPassthruState *tpm_pt,
> > -                                         const TPMLocality *locty_data,
> > -                                         bool *selftest_done)
> > -{
> > -    return tpm_passthrough_unix_tx_bufs(tpm_pt,
> > -                                        locty_data->w_buffer.buffer,
> > -                                        locty_data->w_offset,
> > -                                        locty_data->r_buffer.buffer,
> > -                                        locty_data->r_buffer.size,
> > -                                        selftest_done);
> > -}
> > -
> > -static void tpm_passthrough_handle_request(TPMBackend *tb)
> > +static void tpm_passthrough_handle_request(TPMBackend *tb, TPMBackendCmd
> > *cmd)
> >   {
> >       TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
> > -    bool selftest_done = false;
> >   
> > -    DPRINTF("tpm_passthrough: processing command\n");
> > +    DPRINTF("tpm_passthrough: processing command %p\n", cmd);
> >   
> > -    tpm_passthrough_unix_transfer(tpm_pt,
> > -                                  tb->tpm_state->locty_data,
> > -                                  &selftest_done);
> > +    tpm_passthrough_unix_tx_bufs(tpm_pt, cmd->in, cmd->in_len,
> > +                                 cmd->out, cmd->out_len,
> > &cmd->selftest_done);
> >   
> > -    tb->recv_data_callback(tb->tpm_state, selftest_done);
> > +    tb->recv_data_callback(tb->tpm_state);
> >   }
> >   
> >   static void tpm_passthrough_reset(TPMBackend *tb)
> > diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> > index 345a4fbee5..ffed7bfaf9 100644
> > --- a/hw/tpm/tpm_tis.c
> > +++ b/hw/tpm/tpm_tis.c
> > @@ -215,7 +215,15 @@ static void tpm_tis_tpm_send(TPMState *s, uint8_t
> > locty)
> >        */
> >       tis->loc[locty].state = TPM_TIS_STATE_EXECUTION;
> >   
> > -    tpm_backend_deliver_request(s->be_driver);
> > +    s->cmd = (TPMBackendCmd) {
> > +        .locty = locty,
> > +        .in = s->locty_data->w_buffer.buffer,
> > +        .in_len = s->locty_data->w_offset,
> > +        .out = s->locty_data->r_buffer.buffer,
> > +        .out_len = s->locty_data->r_buffer.size
> > +    };
> > +
> > +    tpm_backend_deliver_request(s->be_driver, &s->cmd);
> >   }
> >   
> >   /* raise an interrupt if allowed */
> > @@ -352,7 +360,7 @@ static void tpm_tis_receive_bh(void *opaque)
> >   {
> >       TPMState *s = opaque;
> >       TPMTISEmuState *tis = &s->s.tis;
> > -    uint8_t locty = s->locty_number;
> > +    uint8_t locty = s->cmd.locty;
> >   
> >       tpm_tis_sts_set(&tis->loc[locty],
> >                       TPM_TIS_STS_VALID | TPM_TIS_STS_DATA_AVAILABLE);
> > @@ -371,11 +379,11 @@ static void tpm_tis_receive_bh(void *opaque)
> >   /*
> >    * Callback from the TPM to indicate that the response was received.
> >    */
> > -static void tpm_tis_receive_cb(TPMState *s,
> > -                               bool is_selftest_done)
> > +static void tpm_tis_receive_cb(TPMState *s)
> >   {
> >       TPMTISEmuState *tis = &s->s.tis;
> > -    uint8_t locty = s->locty_number;
> > +    bool is_selftest_done = s->cmd.selftest_done;
> > +    uint8_t locty = s->cmd.locty;
> >       uint8_t l;
> >   
> >       if (is_selftest_done) {
> 
> 
>