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

[Don Slutz <dslutz@verizon.com>]

On 09/22/14 09:34, Ian Campbell wrote:
> 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@verizon.com>
>> ---
>> 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".

Will do.

>> @@ -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." ?

Sure.

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

Ok.

>> +Other then the range 0x40000000 to 0x400000ff can be used by
>> +hypervisors.
> s/then/than/ I think.

I am not sure, so I will change it.

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

Sure.

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

Will switch to uint64.

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

I think you are miss reading this.  For "stdvga = 1", l === 1, so it
is not conditionally vmware.  However for "stdvga = 0" it is conditionally
vmware.  I will drop the "stdvga = 0" conditionally vmware.

And add to the xl.cfg.pod.5 the additional statement that
the deprecated "stdvga=0" prevents the usage of vmware by default.


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

This should work.  If VGA is specified, stdvga is ignored.

> (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")

Will do.

    -Don Slutz

>>   
>>           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);
>