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

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

On Mon, 2015-02-16 at 18:05 -0500, Don Slutz wrote:

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


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.

> +Note: both vmware_port and nestedhvm cannot be specified at the
> +same time.

Drop the "both" here (and in the commit message).

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

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

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

Ian.