Re: [Qemu-devel] [PATCH v3 09/13] tpm: Introduce condition to notify waiters of completed command

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Introduce a lock and a condition to notify anyone waiting for the completion
> of the execution of a TPM command by the backend (thread). The backend
> uses the condition to signal anyone waiting for command completion.
> We need to place the condition in two locations: one is invoked by the
> backend thread, the other by the bottom half thread.
> We will use the signaling to wait for command completion before VM
> suspend.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  hw/tpm/tpm_tis.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 035c6ef..86e9a92 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -80,6 +80,9 @@ typedef struct TPMState {
>      TPMVersion be_tpm_version;
>
>      size_t be_buffer_size;
> +
> +    QemuMutex state_lock;
> +    QemuCond cmd_complete;

Looks like the cond is unused in the following patches.

>  } TPMState;
>
>  #define TPM(obj) OBJECT_CHECK(TPMState, (obj), TYPE_TPM_TIS)
> @@ -405,6 +408,8 @@ static void tpm_tis_request_completed(TPMIf *ti)
>          }
>      }
>
> +    qemu_mutex_lock(&s->state_lock);
> +

I get grumpy with multiple threads... Potentially, many threads will
want to use TPMState: main thread, backend thread (hopefully not with
the bh), migration thread (with the next patches...), and vcpu thread.
It's hard to review which part of TPMState needs mux and which
doesn't. Why do you lock only in this function? After checking
s->cmd.selftest_done & modifying s->loc[locty].sts...

>      tpm_tis_sts_set(&s->loc[locty],
>                      TPM_TIS_STS_VALID | TPM_TIS_STS_DATA_AVAILABLE);
>      s->loc[locty].state = TPM_TIS_STATE_COMPLETION;
> @@ -419,6 +424,10 @@ static void tpm_tis_request_completed(TPMIf *ti)
>
>      tpm_tis_raise_irq(s, locty,
>                        TPM_TIS_INT_DATA_AVAILABLE | TPM_TIS_INT_STS_VALID);
> +
> +    /* notify of completed command */
> +    qemu_cond_signal(&s->cmd_complete);
> +    qemu_mutex_unlock(&s->state_lock);
>  }
>
>  /*
> @@ -1046,6 +1055,9 @@ static void tpm_tis_initfn(Object *obj)
>      memory_region_init_io(&s->mmio, OBJECT(s), &tpm_tis_memory_ops,
>                            s, "tpm-tis-mmio",
>                            TPM_TIS_NUM_LOCALITIES << TPM_TIS_LOCALITY_SHIFT);
> +
> +    qemu_mutex_init(&s->state_lock);
> +    qemu_cond_init(&s->cmd_complete);
>  }
>
>  static void tpm_tis_class_init(ObjectClass *klass, void *data)
> --
> 2.5.5
>
>



-- 
Marc-André Lureau