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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Jan 08, 2015 at 09:44:11AM +0000, Xu, Quan wrote:
[...]
> 
> > > +    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. 
> 

The hotplug handling logic is generic to both PV and HVM. It's more
about maintaining some states in libxl. The locking pattern is
documented in libxl_internal.h. Search for json_lock in that file. BTW
that file also contains documents for some other libxl internal
machinery that you might find useful.

Generally speaking, I don't think you will need to touch hotplug
handling logic.  If you're aware of it you won't break things by chance.

I will use your specific case as example.

Search for libxl_device_vtpm_add in libxl.c you will see a macro
defining that function, which is called in device hotplug path. That
function initiates an asynchronous operation (AO), which is documented
in libxl_internal.h.

During domain creation, the code path is a bit different. Take a look at
libxl_create.c:domcreate_attach_vtpms. In the end it's still
libxl__device_vtpm_add that does the real work.

Again, you're not likely to change all these, but knowing them can help
you not break things by chance. And you can also see why I want you to
modify libxl__device_vtpm_add.

To play with hotplug and unplug, see `xl --help | grep vtpm'.

> > 
> > 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.
> 
> > 
[...]
> 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);
> 

Probably you should use that particular vtpm's devid? Given that in
theory you can have several vtpms for a guest if you hotplug more than
one.

Wei.

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