[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 4/5] vTPM: add vTPM device for HVM virtual machine



On Tue, Dec 30, 2014 at 11:45:31PM -0500, Quan Xu wrote:
> Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> ---
>  tools/libxl/libxl.c          | 62 
> ++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_create.c   |  6 +++++
>  tools/libxl/libxl_dm.c       | 16 ++++++++++++
>  tools/libxl/libxl_internal.h |  3 +++
>  4 files changed, 87 insertions(+)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 18561fb..656d4b0 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2015,6 +2015,10 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t 
> domid,
>      flexarray_append(front, "handle");
>      flexarray_append(front, GCSPRINTF("%d", vtpm->devid));
>  
> +    /*for para virtual machine*/

Space after "/*" and before "*/", and capitalise "for".

Please also fix similar issues below.

> +    flexarray_append(front, "domain-type");
> +    flexarray_append(front, GCSPRINTF("%d", LIBXL_DOMAIN_TYPE_PV));
> +

What is this used for?

>      if (aodev->update_json) {
>          lock = libxl__lock_domain_userdata(gc, domid);
>          if (!lock) {
> @@ -2073,6 +2077,64 @@ out:
>      return;
>  }
>  
> +void libxl__device_hvm_vtpm_add(libxl__gc *gc, uint32_t domid,
> +                                libxl_device_vtpm *vtpm)
> +{
> +    flexarray_t *front;
> +    flexarray_t *back;
> +    libxl__device *device;
> +    unsigned int rc;
> +
> +    rc = libxl__device_vtpm_setdefault(gc, vtpm);
> +    if (rc) goto out;
> +
> +    front = flexarray_make(gc, 16, 1);
> +    back = flexarray_make(gc, 16, 1);
> +
> +    if (vtpm->devid == -1) {
> +        if ((vtpm->devid = libxl__device_nextid(gc, domid, "vtpm")) < 0) {
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +    }
> +
> +    GCNEW(device);
> +    rc = libxl__device_from_vtpm(gc, domid, vtpm, device);
> +    if ( rc != 0 ) goto out;

Coding style.

> +    flexarray_append(back, "frontend-id");
> +    flexarray_append(back, GCSPRINTF("%d", domid));
> +    flexarray_append(back, "online");
> +    flexarray_append(back, "1");
> +    flexarray_append(back, "state");
> +    flexarray_append(back, GCSPRINTF("%d", 1));

No need to use GCSPRINT.

> +    flexarray_append(back, "handle");
> +    flexarray_append(back, GCSPRINTF("%d", vtpm->devid));
> +
> +    flexarray_append(back, "uuid");
> +    flexarray_append(back, GCSPRINTF(LIBXL_UUID_FMT, 
> LIBXL_UUID_BYTES(vtpm->uuid)));

Line too long.

> +    flexarray_append(back, "resume");
> +    flexarray_append(back, "False");
> +
> +    flexarray_append(front, "backend-id");
> +    flexarray_append(front, GCSPRINTF("%d", vtpm->backend_domid));
> +    flexarray_append(front, "state");
> +    flexarray_append(front, GCSPRINTF("%d", 1));
> +    flexarray_append(front, "handle");
> +    flexarray_append(front, GCSPRINTF("%d", vtpm->devid));
> +
> +    flexarray_append(front, "domain-type");
> +    flexarray_append(front, GCSPRINTF("%d", LIBXL_DOMAIN_TYPE_HVM));
> +
> +    libxl__device_generic_add(gc, XBT_NULL, device,
> +                              libxl__xs_kvs_of_flexarray(gc, back, 
> back->count),
> +                              libxl__xs_kvs_of_flexarray(gc, front, 
> front->count),
> +                              NULL);
> +
> +    rc = 0;
> +out:
> +    return;
> +}
> +

There is one major issue with your implementation. You didn't handle
vtpm hot-plug and hot-unplug.

I think what you should do is to refactor libxl__device_vtpm_add to call
the right helpers (say libxl__device_vtpm_add_{pv,hvm}).

With this approach you don't need to worry about all the internal logic
of handling JSON template and locking etc. You might also be able to
nuke b_info->num_vtpms you added in previous patches.

Does this make sense?

>  libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t domid, 
> int *num)
>  {
>      GC_INIT(ctx);
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index c6f68fe..b2f61cb 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -901,6 +901,12 @@ static void initiate_domain_create(libxl__egc *egc,
>              d_config->nics[i].devid = ++last_devid;
>      }
>  
> +    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
> +        d_config->num_vtpms > 0) {
> +        ret = libxl__device_vtpm_setdefault(gc, d_config->vtpms);
> +        if (ret) goto error_out;
> +    }
> +
>      if (restore_fd >= 0) {
>          LOG(DEBUG, "restoring, not running bootloader");
>          domcreate_bootloader_done(egc, &dcs->bl, 0);
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 3e191c3..337ac64 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -414,6 +414,7 @@ static char ** 
> libxl__build_device_model_args_new(libxl__gc *gc,
>      const libxl_device_nic *nics = guest_config->nics;
>      const int num_disks = guest_config->num_disks;
>      const int num_nics = guest_config->num_nics;
> +    const int num_vtpms = guest_config->num_vtpms;
>      const libxl_vnc_info *vnc = libxl__dm_vnc(guest_config);
>      const libxl_sdl_info *sdl = dm_sdl(guest_config);
>      const char *keymap = dm_keymap(guest_config);
> @@ -747,6 +748,15 @@ static char ** 
> libxl__build_device_model_args_new(libxl__gc *gc,
>          abort();
>      }
>  
> +    /*add vTPM parameters for HVM virtual machine*/
> +    if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
> +        num_vtpms >0) {

"> 0".

> +        flexarray_vappend(dm_args, "-tpmdev",
> +                          "xenstubdoms,id=xenvtpm0", NULL);

Hardcoded id? I think we need to be more flexible on this.

Wei.

> +        flexarray_vappend(dm_args,"-device",
> +                          "tpm-tis,tpmdev=xenvtpm0", NULL);
> +    }
> +
>      ram_size = libxl__sizekb_to_mb(b_info->max_memkb - b_info->video_memkb);
>      flexarray_append(dm_args, "-m");
>      flexarray_append(dm_args, libxl__sprintf(gc, "%"PRId64, ram_size));
> @@ -1412,6 +1422,12 @@ retry_transaction:
>      spawn->failure_cb = device_model_startup_failed;
>      spawn->detached_cb = device_model_detached;
>  
> +    /* Plug vtpm devices*/
> +    if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
> +        guest_config->num_vtpms > 0){
> +        libxl__device_hvm_vtpm_add(gc, domid, guest_config->vtpms);
> +    }
> +
>      rc = libxl__spawn_spawn(egc, spawn);
>      if (rc < 0)
>          goto out_close;
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 4361421..946b8cf 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2388,6 +2388,9 @@ _hidden void libxl__device_vtpm_add(libxl__egc *egc, 
> uint32_t domid,
>                                     libxl_device_vtpm *vtpm,
>                                     libxl__ao_device *aodev);
>  
> +void libxl__device_hvm_vtpm_add(libxl__gc *gc, uint32_t domid,
> +                                libxl_device_vtpm *vtpm);
> +
>  /* Internal function to connect a vkb device */
>  _hidden int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
>                                    libxl_device_vkb *vkb);
> -- 
> 1.8.3.2

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.