Re: [Qemu-devel] [PATCH v5 3/6] Qemu-Xen-vTPM: Xen frontend driver infrastructure

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

On 04/10/2015 02:59 AM, Quan Xu wrote:
> This patch adds infrastructure for xen front drivers living in qemu,
> so drivers don't need to implement common stuff on their own.  It's
> mostly xenbus management stuff: some functions to access XenStore,
> setting up XenStore watches, callbacks on device discovery and state
> changes, and handle event channel between the virtual machines.
>
> Call xen_fe_register() function to register XenDevOps, and make sure,
>           [...]
>      3 = ""
>       [...]
>       device = "" (frontend device, the backend is running in QEMU/.etc)
>        vkbd = ""
>         [...]
>        vif = ""
>         [...]
>
>   ..
>
> (QEMU) xen_vtpmdev_ops is initialized with the following process:
>    xen_hvm_init()
>      [...]
>      -->xen_fe_register("vtpm", ...)
>        -->xenstore_fe_scan()
>          -->xen_fe_try_init(ops)
>            --> XenDevOps.init()
>          -->xen_fe_get_xendev()
>            --> XenDevOps.alloc()
>          -->xen_fe_check()
>            -->xen_fe_try_initialise()
>              --> XenDevOps.initialise()
>            -->xen_fe_try_connected()
>              --> XenDevOps.connected()
>          -->xs_watch()
>      [...]
>
> Signed-off-by: Quan Xu <quan.xu@intel.com>
>
> --Changes in v5:
>   -Comments enhancement.
>   -Pass the size of the buff for vtpm_recv()|vtpm_send().
> ---
>   hw/tpm/Makefile.objs         |   1 +
>   hw/tpm/xen_vtpm_frontend.c   | 321 +++++++++++++++++++++++++++++++++++++++++++
>   hw/xen/xen_frontend.c        |  20 +++
>   include/hw/xen/xen_backend.h |   7 +
>   include/hw/xen/xen_common.h  |   6 +
>   xen-hvm.c                    |   5 +
>   6 files changed, 360 insertions(+)
>   create mode 100644 hw/tpm/xen_vtpm_frontend.c
>
> diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
> index 99f5983..57919fa 100644
> --- a/hw/tpm/Makefile.objs
> +++ b/hw/tpm/Makefile.objs
> @@ -1,2 +1,3 @@
>   common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
>   common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
> +common-obj-$(CONFIG_TPM_XENSTUBDOMS) += xen_vtpm_frontend.o
> diff --git a/hw/tpm/xen_vtpm_frontend.c b/hw/tpm/xen_vtpm_frontend.c
> new file mode 100644
> index 0000000..3ca9501
> --- /dev/null
> +++ b/hw/tpm/xen_vtpm_frontend.c
> @@ -0,0 +1,321 @@
> +/*
> + * Connect to Xen vTPM stubdom domain
> + *
> + *  Copyright (c) 2015 Intel Corporation
> + *  Authors:
> + *    Quan Xu <quan.xu@intel.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdarg.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <signal.h>
> +#include <inttypes.h>
> +#include <time.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +#include <sys/ioctl.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/mman.h>
> +#include <sys/uio.h>
> +
> +#include "hw/hw.h"
> +#include "block/aio.h"
> +#include "hw/xen/xen_backend.h"
> +
> +#ifndef XS_STUBDOM_VTPM_ENABLE
> +#define XS_STUBDOM_VTPM_ENABLE    "1"
> +#endif
> +
> +#ifndef PAGE_SIZE
> +#define PAGE_SIZE      4096
> +#endif
> +
> +enum tpmif_state {
> +    /* No contents, vTPM idle, cancel complete */
> +    TPMIF_STATE_IDLE,
> +    /* Request ready or vTPM working */
> +    TPMIF_STATE_SUBMIT,
> +    /* Response ready or vTPM idle */
> +    TPMIF_STATE_FINISH,
> +    /* Cancel requested or vTPM working */
> +    TPMIF_STATE_CANCEL,
> +};
> +
> +static AioContext *vtpm_aio_ctx;
> +
> +enum status_bits {
> +    VTPM_STATUS_RUNNING  = 0x1,
> +    VTPM_STATUS_IDLE     = 0x2,
> +    VTPM_STATUS_RESULT   = 0x4,
> +    VTPM_STATUS_CANCELED = 0x8,
> +};
> +
> +struct tpmif_shared_page {
> +    /* Request and response length in bytes */
> +    uint32_t length;
> +    /* Enum tpmif_state */
> +    uint8_t  state;
> +    /* For the current request */
> +    uint8_t  locality;
> +    /* Should be zero */
> +    uint8_t  pad;
> +    /* Extra pages for long packets; may be zero */
> +    uint8_t  nr_extra_pages;
> +    /*
> +     * Grant IDs, the length is actually nr_extra_pages.
> +     * beyond the extra_pages entries is the actual requese
> +     * and response.
> +     */

requese->request


> +    uint32_t extra_pages[0];
> +};
> +
> +struct xen_vtpm_dev {
> +    struct XenDevice xendev;  /* must be first */
> +    struct           tpmif_shared_page *shr;
> +    xc_gntshr        *xen_xcs;
> +    int              ring_ref;
> +    int              bedomid;
> +    QEMUBH           *sr_bh;
> +};
> +
> +static uint8_t vtpm_status(struct xen_vtpm_dev *vtpmdev)
> +{
> +    switch (vtpmdev->shr->state) {
> +    case TPMIF_STATE_IDLE:
> +    case TPMIF_STATE_FINISH:
> +        return VTPM_STATUS_IDLE;
> +    case TPMIF_STATE_SUBMIT:
> +    case TPMIF_STATE_CANCEL:
> +        return VTPM_STATUS_RUNNING;
> +    default:
> +        return 0;
> +    }
> +}
> +
> +static bool vtpm_aio_wait(AioContext *ctx)
> +{
> +    return aio_poll(ctx, true);
> +}
> +
> +static void sr_bh_handler(void *opaque)
> +{
> +}
> +
> +int vtpm_recv(struct XenDevice *xendev, uint8_t* buf, uint32_t buf_size,
> +              size_t *count)
> +{
> +    struct xen_vtpm_dev *vtpmdev = container_of(xendev, struct xen_vtpm_dev,
> +                                                xendev);
> +    struct tpmif_shared_page *shr = vtpmdev->shr;
> +    unsigned int offset;
> +    size_t length = shr->length;
> +
> +    if (shr->state == TPMIF_STATE_IDLE) {
> +        return -ECANCELED;
> +    }
> +
> +    offset = sizeof(*shr) + sizeof(shr->extra_pages[0])*shr->nr_extra_pages;

offset now points to where the TPM response starts, right?

> +    if (offset > buf_size) {

You are comparing offset as if it was the size of the TPM response, but 
that's not what it is as far as I understand this.

I would have thought that shr->length indicates the size of the TPM 
response that starts at offset.
So then you should only have to ensure that shr->length <= buf_size and 
never copy more than buf_size bytes to buf.

Similar comments to vtpm_send.

> +        return -EIO;
> +    }
> +
> +    if (offset + length > buf_size) {
> +        length = buf_size - offset;
> +    }
> +
> +    if (length > *count) {
> +        length = *count;
> +    }
> +
> +    memcpy(buf, offset + (uint8_t *)shr, shr->length);

use length rather than shr->length otherwise length goes unused.


Rest looks good.

    Stefan