Re: [PATCH v9 07/13] tools: Add vmware_port support

[Don Slutz <dslutz@verizon.com>]

On 03/03/15 09:23, Ian Campbell wrote:
> On Mon, 2015-02-16 at 18:05 -0500, Don Slutz wrote:
> 

I do not see that I ever replied to this :(

>> > +=item B<vmware_port=BOOLEAN>
>> > +
>> > +Turns on or off the exposure of VMware port.  This is known as
>> > +vmport in QEMU.  Also called VMware Backdoor I/O Port.  Not all
>> > +defined VMware backdoor commands are implemented.  All of the
>> > +ones that Linux kernel uses are defined.
>> > +
>> > +if vmware_port is not specified in the config file, let vmware_hwver != 0
>> > +be the default value.  This means that only vmware_hwver = 7 needs to
>> > +be specified to enable both features.
> "If..." at the start of the sentence.
> 
> But I think a clearer wording, which avoids users having to know C
> syntax would be:
> 
>         Defaults to enabled if vmware_hwver is non-zero (i.e. enabled)
>         otherwise defaults to disabled.

Will use this.

> 
> 
> I think the thing about setting hwver to 7 should be in the hwver space,
> as in words to the effect that setting that option enabled vmware_port
> support by default.
> 
> Also, why is 7 special? The patch which added vmware_hwver didn't seem
> to suggest that vmware_hwver = 7 was what was needed.
> 

7 is special because that is when VMware started providing CPUID leaves.


>> > +Note: both vmware_port and nestedhvm cannot be specified at the
>> > +same time.
> Drop the "both" here (and in the commit message).
> 

Will do.

>> > +
>> >  =back
>> >  
>> >  =head3 Emulated VGA Graphics Device
>> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>> > index 0c27e5c..792b569 100644
>> > --- a/tools/libxl/libxl.h
>> > +++ b/tools/libxl/libxl.h
>> > @@ -173,6 +173,11 @@
>> >  #define LIBXL_HAVE_BUILDINFO_HVM_VMWARE_HWVER 1
>> >  
>> >  /*
>> > + * libxl_domain_create_info has the vmware_port field.
>> > + */
>> > +#define LIBXL_HAVE_CREATEINFO_VMWARE_PORT 1
> I think this can be part of the umbrella HAVE I asked you to add
> earlier. This probably means the define should be added alongside the
> final such interface addition in this series.
> 

Will switch to only 1 in this patch.

>> > +
>> > +/*
>> >   * libxl ABI compatibility
>> >   *
>> >   * The only guarantee which libxl makes regarding ABI compatibility
>> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> > index 8c910c4..439164a 100644
>> > --- a/tools/libxl/libxl_create.c
>> > +++ b/tools/libxl/libxl_create.c
>> > @@ -26,7 +26,8 @@
>> >  #include <xen/hvm/e820.h>
>> >  
>> >  int libxl__domain_create_info_setdefault(libxl__gc *gc,
>> > -                                         libxl_domain_create_info *c_info)
>> > +                                         libxl_domain_create_info *c_info,
>> > +                                         bool vmware_port_default)
> The need to pass this makes me think that vmware_hwver should probably
> be in create info not build info, since the soonest it is needed is
> create time. Unless that changes based on the discussion about hwo such
> things should be dpone in the other thread of course.
> 

Moved both to c_info.

> (the split is stupid and annoying, but we are stuck with it)
> 
>> > @@ -876,6 +881,13 @@ static void initiate_domain_create(libxl__egc *egc,
>> >      ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
>> >      if (ret) goto error_out;
>> >  
>> > +    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
>> > +        libxl_defbool_val(d_config->b_info.u.hvm.nested_hvm) &&
>> > +        libxl_defbool_val(d_config->c_info.vmware_port)) {
>> > +        LOG(ERROR, "Both vmware_port and nestedhvm can not be enabled\n");
> "vmware_port and nestedhvm cannot be enabled simultaneously"
> 
>> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> > index b05fa73..c27f9a4 100644
>> > --- a/tools/libxl/libxl_dm.c
>> > +++ b/tools/libxl/libxl_dm.c
>> > @@ -1061,7 +1061,9 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
>> >      dm_config->c_info.run_hotplug_scripts =
>> >          guest_config->c_info.run_hotplug_scripts;
>> >  
>> > -    ret = libxl__domain_create_info_setdefault(gc, &dm_config->c_info);
>> > +    ret = libxl__domain_create_info_setdefault(gc, &dm_config->c_info,
>> > +                       dm_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
>> > +                       dm_config->b_info.u.hvm.vmware_hwver);
> Isn't this enabling vmware support for the stbudom itself (not the
> target domain)? I think this should just be false here.
> 

Yes, will drop.

   -Don Slutz


> Ian.
> 
>