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

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

On 10/10/2017 12:16 PM, Marc-André Lureau wrote:
> 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)

Yes, I believe so.

>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>
>>> ---
>>>    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) {
>>
>>