Re: [Xen-devel] [PATCH for-4.5 v6 02/16] tools: Add vmware_hw support

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Sat, 2014-09-20 at 14:07 -0400, Don Slutz wrote:
> This is used to set HVM_PARAM_VMWARE_HW. It is set to the VMware
> virtual hardware version.
> 
> Currently 0, 3-4, 6-11 are good values.  However the code only
> checks for == 0 or != 0.
> 
> If non-zero then
>   default VGA to VMware's VGA.
> 
> Also now allows vga=vmware
> 
> Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>
> ---
> v5:
>       Anything looking for Xen according to the Xen cpuid instructions...
>         Adjusted doc to new wording.
> 
>  docs/man/xl.cfg.pod.5               | 21 +++++++++++++++++++--
>  docs/misc/hypervisor-cpuid.markdown | 28 ++++++++++++++++++++++++++++
>  tools/libxc/xc_domain_restore.c     | 14 ++++++++++++++
>  tools/libxc/xc_domain_save.c        | 11 +++++++++++
>  tools/libxc/xg_save_restore.h       |  2 ++
>  tools/libxl/libxl.h                 | 10 ++++++++++
>  tools/libxl/libxl_create.c          |  4 +++-
>  tools/libxl/libxl_dm.c              | 10 +++++++++-
>  tools/libxl/libxl_dom.c             |  2 ++
>  tools/libxl/libxl_types.idl         |  2 ++
>  tools/libxl/xl_cmdimpl.c            | 11 ++++++++++-
>  11 files changed, 110 insertions(+), 5 deletions(-)
>  create mode 100644 docs/misc/hypervisor-cpuid.markdown
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 517ae2f..367b401 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1147,6 +1147,23 @@ some other Operating Systems and in some circumstance 
> can prevent
>  Xen's own paravirtualisation interfaces for HVM guests from being
>  used.
>  
> +=item B<vmware_hw=NUMBER>
> +
> +Turns on or off the exposure of VMware cpuid.  The number is the
> +VMware's hardware version number, where 0 is off.  If not zero it
> +changes the default VGA to VMware's VGA.

"is the VMware's" => "is VMware's".

> @@ -1185,8 +1202,8 @@ This option is deprecated, use vga="stdvga" instead.
>  
>  =item B<vga="STRING">
>  
> -Selects the emulated video card (none|stdvga|cirrus).
> -The default is cirrus.
> +Selects the emulated video card (none|stdvga|cirrus|vmware).
> +The default is cirrus (or vmware if B<vmware_hw> is not zero).

"The default is cirrus unless B<vmware_hw> is non-zero in which case it
is vmware." ?

>  
>  =item B<vnc=BOOLEAN>
>  
> diff --git a/docs/misc/hypervisor-cpuid.markdown 
> b/docs/misc/hypervisor-cpuid.markdown
> new file mode 100644
> index 0000000..901a4e1
> --- /dev/null
> +++ b/docs/misc/hypervisor-cpuid.markdown
> @@ -0,0 +1,28 @@
> +Hypervisor Cpuid
> +================
> +
> +The support of hypervisor cpuid leaves has not been agreed to.

by....

"the general hypervisor community" perhaps?

Perhaps a better way of putting this would be "There is no agreed
standard for the use of hypervisor cpuid leaves" or some such.

> +Other then the range 0x40000000 to 0x400000ff can be used by
> +hypervisors.

s/then/than/ I think. 

> +
> +MicroSoft Hyper-V (AKA viridian) currently must be at 0x40000000.
> +
> +VMware currently must be at 0x40000000.
> +
> +KVM currently must be at 0x40000000 (from Seabios).
> +
> +Xen can be found at the first otherwise unused 0x100 aligned
> +offset between 0x40000000 and 0x40010000.

I think you should add " leaves" after each hypervisor name.

> @@ -378,6 +379,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                         ("timeoffset",       string),
>                                         ("hpet",             libxl_defbool),
>                                         ("vpt_align",        libxl_defbool),
> +                                       ("vmware_hw",        UInt(64, 
> init_val = 0)),

There is no need for an explicitly 0 init_val, it's the default default.

>                                         ("timer_mode",       
> libxl_timer_mode),
>                                         ("nested_hvm",       libxl_defbool),
>                                         ("smbios_firmware",  string),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 698b3bc..2119bd6 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1038,6 +1038,8 @@ static void parse_config_data(const char *config_source,
>          xlu_cfg_get_defbool(config, "hpet", &b_info->u.hvm.hpet, 0);
>          xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align, 
> 0);
>  
> +        if (!xlu_cfg_get_long(config, "vmware_hw",  &l, 1))
> +            b_info->u.hvm.vmware_hw = l;
>          if (!xlu_cfg_get_long(config, "timer_mode", &l, 1)) {
>              const char *s = libxl_timer_mode_to_string(l);
>              fprintf(stderr, "WARNING: specifying \"timer_mode\" as an 
> integer is deprecated. "
> @@ -1676,13 +1678,20 @@ skip_vfb:
>                  b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
>              } else if (!strcmp(buf, "none")) {
>                  b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_NONE;
> +            } else if (!strcmp(buf, "vmware")) {
> +                b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_VMWARE;
>              } else {
>                  fprintf(stderr, "Unknown vga \"%s\" specified\n", buf);
>                  exit(1);
>              }
>          } else if (!xlu_cfg_get_long(config, "stdvga", &l, 0))
>              b_info->u.hvm.vga.kind = l ? LIBXL_VGA_INTERFACE_TYPE_STD :
> -                                         LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
> +                b_info->u.hvm.vmware_hw ? LIBXL_VGA_INTERFACE_TYPE_VMWARE :
> +                                          LIBXL_VGA_INTERFACE_TYPE_CIRRUS;

I don't think this is a good idea. stdvga = 1 in the config file should
still mean stdvga, not conditionally vmware. Likewise stdvga = 0 should
always be cirrus.

Someone who wants to force vmware should use vga=vmware and not specify
stdvga at all.

(NB: stdvga is deprecated synonym, the man page advises using vga=
already)

> +        else
> +            b_info->u.hvm.vga.kind =
> +                b_info->u.hvm.vmware_hw ? LIBXL_VGA_INTERFACE_TYPE_VMWARE :
> +                                          LIBXL_VGA_INTERFACE_TYPE_CIRRUS;

This else clause shouldn't be here, update
libxl__domain_build_info_setdefault instead where it currently says:
        if (!b_info->u.hvm.vga.kind)
            b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;

note that this code should only set vga.kind if it is currently zero
(which indicates to libxl "pick a default")

>  
>          xlu_cfg_replace_string (config, "keymap", &b_info->u.hvm.keymap, 0);
>          xlu_cfg_get_defbool (config, "spice", &b_info->u.hvm.spice.enable, 
> 0);



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