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

["Xu, Quan" <quan.xu@xxxxxxxxx>]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@xxxxxxxxxx]
> Sent: Monday, January 05, 2015 9:14 PM
> To: Xu, Quan
> Cc: xen-devel@xxxxxxxxxxxxx; ian.jackson@xxxxxxxxxxxxx;
> stefano.stabellini@xxxxxxxxxxxxx; ian.campbell@xxxxxxxxxx; wei.liu2@xxxxxxxxxx
> Subject: Re: [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".

Thanks Wei. 
I will modify it in v3. 

> 
> 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?
The domain-type is the flag bit for Domain ID when bind event channel port.
It will be deleted in v3.


> 
> >      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.

I will modify it in v3.

> 
> > +    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.


Ditto

> 
> > +    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.
> 


Ditto


> > +    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.

Yes, host-plug/host-unplug is an issue. I have no more idea for this. Could you 
share 
Some host-plug/host-unplug QEMU device mode? Then I can implement it as the 
next plan. 

> 
> 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?


Yes, 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".

I will modify it in v3.

> 
> > +        flexarray_vappend(dm_args, "-tpmdev",
> > +                          "xenstubdoms,id=xenvtpm0", NULL);
> 
> Hardcoded id? I think we need to be more flexible on this.

It is not hardcoded id.
Just make sure id=xenvtpm0 is the same as tpmdev=xenvtpm0, in next 2 functions.
flexarray_vappend(dm_args, "-tpmdev",
               "xenstubdoms,id=xenvtpm0", NULL);
flexarray_vappend(dm_args,"-device",
                "tpm-tis,tpmdev=xenvtpm0", NULL);

I can change it to:

flexarray_vappend(dm_args, "-tpmdev",
                libxl__sprintf(gc, " xenstubdoms,id=xenvtpm%d", guest_domid, 
NULL);
flexarray_vappend(dm_args,"-device",
                libxl__sprintf(gc, " tpm-tis,tpmdev=xenvtpm%d",NULL);

thanks 
Quan 

> 
> 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