Re: [PATCH v7 4/6] tpm_spapr: Support suspend and resume

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5134 bytes --]

On Thu, Dec 19, 2019 at 09:06:03AM -0500, Stefan Berger wrote:
> Extend the tpm_spapr frontend with VM suspend and resume support.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  hw/tpm/tpm_spapr.c  | 67 ++++++++++++++++++++++++++++++++++++++++++++-
>  hw/tpm/trace-events |  2 ++
>  2 files changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
> index ab184fbb82..cf5c7851e7 100644
> --- a/hw/tpm/tpm_spapr.c
> +++ b/hw/tpm/tpm_spapr.c
> @@ -76,6 +76,9 @@ typedef struct {
>  
>      unsigned char buffer[TPM_SPAPR_BUFFER_MAX];
>  
> +    uint32_t numbytes; /* number of bytes in suspend_buffer */
> +    unsigned char *suspend_buffer;
> +
>      TPMBackendCmd cmd;
>  
>      TPMBackend *be_driver;
> @@ -240,6 +243,13 @@ static void tpm_spapr_request_completed(TPMIf *ti, int ret)
>  
>      /* a max. of be_buffer_size bytes can be transported */
>      len = MIN(tpm_cmd_get_size(s->buffer), s->be_buffer_size);
> +
> +    if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
> +        /* defer delivery of response until .post_load */
> +        s->numbytes = len;
> +        return;
> +    }

I'm not understanding the basics of what's going on here.  IIUC, the
backend TPM can complete a request after we've entered migration
downtime.  But if that's the case, I can't see any guarantee that we
won't have already transmitted the TPM device state, so updating it
here might never reach the destination.  In that case we'd still lose
the request, so I'm not sure what we're accomplishing here.

>      rc = spapr_vio_dma_write(&s->vdev, be32_to_cpu(crq->data),
>                               s->buffer, len);
>  
> @@ -288,11 +298,13 @@ static void tpm_spapr_reset(SpaprVioDevice *dev)
>      SpaprTpmState *s = VIO_SPAPR_VTPM(dev);
>  
>      s->state = SPAPR_VTPM_STATE_NONE;
> +    s->numbytes = 0;
>  
>      s->be_tpm_version = tpm_backend_get_tpm_version(s->be_driver);
>  
>      s->be_buffer_size = MIN(tpm_backend_get_buffer_size(s->be_driver),
>                              TPM_SPAPR_BUFFER_MAX);
> +    s->suspend_buffer = g_realloc(s->suspend_buffer, s->be_buffer_size);
>  
>      tpm_backend_reset(s->be_driver);
>      tpm_spapr_do_startup_tpm(s, s->be_buffer_size);
> @@ -309,9 +321,62 @@ static enum TPMVersion tpm_spapr_get_version(TPMIf *ti)
>      return tpm_backend_get_tpm_version(s->be_driver);
>  }
>  
> +/* persistent state handling */
> +
> +static int tpm_spapr_pre_save(void *opaque)
> +{
> +    SpaprTpmState *s = opaque;
> +
> +    tpm_backend_finish_sync(s->be_driver);
> +
> +    if (s->numbytes) {
> +        memcpy(s->suspend_buffer, s->buffer, s->numbytes);
> +    }
> +
> +    trace_tpm_spapr_pre_save(s->numbytes);
> +    /*
> +     * we cannot deliver the results to the VM since DMA would touch VM memory
> +     */
> +
> +    return 0;
> +}
> +
> +static int tpm_spapr_post_load(void *opaque, int version_id)
> +{
> +    SpaprTpmState *s = opaque;
> +
> +    if (s->numbytes) {
> +        trace_tpm_spapr_post_load();
> +
> +        memcpy(s->buffer, s->suspend_buffer,
> +               MIN(s->numbytes, s->be_buffer_size));
> +        /* deliver the results to the VM via DMA */
> +        tpm_spapr_request_completed(TPM_IF(s), 0);
> +        s->numbytes = 0;
> +    }
> +
> +    return 0;
> +}
> +
>  static const VMStateDescription vmstate_spapr_vtpm = {
>      .name = "tpm-spapr",
> -    .unmigratable = 1,
> +    .version_id = 1,
> +    .minimum_version_id = 0,
> +    .minimum_version_id_old = 0,
> +    .pre_save = tpm_spapr_pre_save,
> +    .post_load = tpm_spapr_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_SPAPR_VIO(vdev, SpaprTpmState),
> +
> +        VMSTATE_UINT8(state, SpaprTpmState),
> +        VMSTATE_UINT32(numbytes, SpaprTpmState),
> +        VMSTATE_VBUFFER_ALLOC_UINT32(suspend_buffer,
> +                                     SpaprTpmState, 0, NULL,
> +                                     numbytes),
> +        /* remember DMA address */
> +        VMSTATE_UINT32(crq.data, SpaprTpmState),
> +        VMSTATE_END_OF_LIST(),
> +    }
>  };
>  
>  static Property tpm_spapr_properties[] = {
> diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> index 9143a8eaa3..5592cec7de 100644
> --- a/hw/tpm/trace-events
> +++ b/hw/tpm/trace-events
> @@ -67,3 +67,5 @@ tpm_spapr_do_crq_get_version(uint32_t version) "response: version %u"
>  tpm_spapr_do_crq_prepare_to_suspend(void) "response: preparing to suspend"
>  tpm_spapr_do_crq_unknown_msg_type(uint8_t type) "Unknown message type 0x%02x"
>  tpm_spapr_do_crq_unknown_crq(uint8_t raw1, uint8_t raw2) "unknown CRQ 0x%02x 0x%02x ..."
> +tpm_spapr_pre_save(uint32_t v) "Number of TPM response bytes to deliver after resume: %u"
> +tpm_spapr_post_load(void) "Delivering TPM response after resume"

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]