Re: [PATCH v3 2/7] vTPM: limit libxl__add_vtpms() function to para virtual machine

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-03-13 at 15:41 +0000, Wei Liu wrote:
> On Tue, Mar 10, 2015 at 08:13:57AM -0400, Quan Xu wrote:
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> > ---
> >  tools/libxl/libxl_create.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > index b1ff5ae..66877b3 100644
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -1358,8 +1358,15 @@ static void domcreate_attach_vtpms(libxl__egc *egc,
> >         goto error_out;
> >     }
> >  
> > -    /* Plug vtpm devices */
> > -   if (d_config->num_vtpms > 0) {
> > +    /*
> > +     * Plug vtpm devices only for PV guest. The xenstore directory is very
> > +     * different for PV guest and HVM guest, but it is still call it for
> > +     * creating HVM guest, and xl should create xenstore directory before
> > +     * spawning QEMU. So try to make it only for PV guest.
> > +     */
> > +    if (d_config->num_vtpms > 0 &&
> > +        d_config->b_info.type == LIBXL_DOMAIN_TYPE_PV) {
> > +
> 
> I'm not convinced that you can / should do this. This is a common entry
> for both HVM and PV guest. If you end up removing this hunk in later
> patch you need to rearrange your series to avoiding adding this hunk in
> the first place.

Agreed, although I also couldn't really parse the comment to fully
understand what the intention was.

> 
> Wei.
> 
> >         /* Attach vtpms */
> >         libxl__multidev_begin(ao, &dcs->multidev);
> >         dcs->multidev.callback = domcreate_attach_pci;
> > -- 
> > 1.8.3.2
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel