Re: [PATCH v3 02/16] tools part of add vmware_hw to xl.cfg

[Don Slutz <dslutz@verizon.com>]

On 09/11/14 07:23, George Dunlap wrote:
> On Mon, Sep 8, 2014 at 2:15 PM, Don Slutz <dslutz@verizon.com> wrote:
>> If non-zero then
>>    Force use of VMware's VGA in QEMU.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
> I think the title would be better "tools: Add vmware_hw support"

Ok.

>> ---
>>   docs/man/xl.cfg.pod.5           |  6 ++++++
>>   tools/libxc/xc_domain_restore.c | 14 ++++++++++++++
>>   tools/libxc/xc_domain_save.c    | 11 +++++++++++
>>   tools/libxc/xg_save_restore.h   |  2 ++
>>   tools/libxl/libxl_create.c      |  4 +++-
>>   tools/libxl/libxl_dm.c          | 33 +++++++++++++++++++++------------
>>   tools/libxl/libxl_dom.c         |  2 ++
>>   tools/libxl/libxl_types.idl     |  1 +
>>   tools/libxl/xl_cmdimpl.c        |  2 ++
>>   9 files changed, 62 insertions(+), 13 deletions(-)
>>
>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
>> index 517ae2f..7f7319a 100644
>> --- a/docs/man/xl.cfg.pod.5
>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -1147,6 +1147,12 @@ 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 on it also
>> +forces the use of VMware's VGA in QEMU.
> You should also say what values are supported.

Here is what I have in v4:


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

The hardware version number (vmware_hw) come from VMware config files.

=over 4

In a .vmx it is virtualHW.version

In a .ovf it is part of the value of vssd:VirtualSystemType.
For vssd:VirtualSystemType == vmx-07, vmware_hw = 7.

=back


There is a lot in the different thread:

Message-ID: <54108DFB.8030804@terremark.com>
Date: Wed, 10 Sep 2014 13:44:27 -0400
User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0
To: Ian Campbell <Ian.Campbell@citrix.com>, Don Slutz <dslutz@verizon.com>
CC: Andrew Cooper <Andrew.Cooper3@citrix.com>, Tim Deegan <tim@xen.org>, Kevin
  Tian <kevin.tian@intel.com>, Keir Fraser <keir@xen.org>, Jun Nakajima
	<jun.nakajima@intel.com>, Stefano Stabellini
	<stefano.stabellini@eu.citrix.com>, Ian Jackson <ian.jackson@eu.citrix.com>,
	Eddie Dong <eddie.dong@intel.com>, <xen-devel@lists.xen.org>, "Aravind
  Gopalakrishnan" <Aravind.Gopalakrishnan@amd.com>, Jan Beulich
	<jbeulich@suse.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, "Suravee
  Suthikulpanit" <suravee.suthikulpanit@amd.com>
Subject: Re: [Xen-devel] [PATCH v2 1/3] Add vmware_hw to xl.cfg
References: <1409585629-25840-1-git-send-email-dslutz@verizon.com>
  <1409585629-25840-2-git-send-email-dslutz@verizon.com>
  <1410182256.3680.16.camel@kazak.uk.xensource.com>
  <540E00AB.1000501@terremark.com>
  <1410255568.8217.65.camel@kazak.uk.xensource.com>
  <540F32A0.2070609@terremark.com>
  <1410341443.8217.260.camel@kazak.uk.xensource.com>
In-Reply-To: <1410341443.8217.260.camel@kazak.uk.xensource.com>




>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index 103cbca..c79274b 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -542,19 +542,28 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>>               }
>>           }
>>
>> -        switch (b_info->u.hvm.vga.kind) {
>> -        case LIBXL_VGA_INTERFACE_TYPE_STD:
>> -            flexarray_append_pair(dm_args, "-device",
>> -                GCSPRINTF("VGA,vgamem_mb=%d",
>> -                libxl__sizekb_to_mb(b_info->video_memkb)));
>> -            break;
>> -        case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
>> +        if (b_info->u.hvm.vmware_hw) {
>>               flexarray_append_pair(dm_args, "-device",
>> -                GCSPRINTF("cirrus-vga,vgamem_mb=%d",
>> -                libxl__sizekb_to_mb(b_info->video_memkb)));
>> -            break;
>> -        case LIBXL_VGA_INTERFACE_TYPE_NONE:
>> -            break;
>> +                                  GCSPRINTF("vmware-svga,vgamem_mb=%d",
>> +                                            libxl__sizekb_to_mb(
>> +                                                b_info->video_memkb)));
>> +        } else {
>> +            switch (b_info->u.hvm.vga.kind) {
>> +            case LIBXL_VGA_INTERFACE_TYPE_STD:
>> +                flexarray_append_pair(dm_args, "-device",
>> +                                      GCSPRINTF("VGA,vgamem_mb=%d",
>> +                                                libxl__sizekb_to_mb(
>> +                                                    b_info->video_memkb)));
>> +                break;
>> +            case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
>> +                flexarray_append_pair(dm_args, "-device",
>> +                                      GCSPRINTF("cirrus-vga,vgamem_mb=%d",
>> +                                                libxl__sizekb_to_mb(
>> +                                                    b_info->video_memkb)));
>> +                break;
>> +            case LIBXL_VGA_INTERFACE_TYPE_NONE:
>> +                break;
>> +            }
>>           }
> So if we set vmware_hw, then we:
> 1. always add a vmware-svga device, regargless of whether vga has been requested
> 2. Ignore the vga parameter and only add vmware-svga, even if someone
> may want something else?
>
> I think at the libxl level, we should add a VMWARE interface type.
> Then in xl_cmdimpl.c, we should:
> * Add a vga="vmware" type in xl.cfg
> * honor the vga= setting (perhaps with a warning if vmware_hw is true?)
> * if vga is not set, and vmware_hw is true, default to vga=VMWARE

In v2 and v3 yes.  In v4 no.  What you are suggesting I have already coded.

>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 8a38077..c30341a 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -1033,6 +1033,8 @@ static void parse_config_data(const char *config_source,
>>           xlu_cfg_get_defbool(config, "acpi_s4", &b_info->u.hvm.acpi_s4, 0);
>>           xlu_cfg_get_defbool(config, "nx", &b_info->u.hvm.nx, 0);
>>           xlu_cfg_get_defbool(config, "viridian", &b_info->u.hvm.viridian, 0);
>> +        if (!xlu_cfg_get_long(config, "vmware_hw",  &l, 1))
>> +            b_info->u.hvm.vmware_hw = l;
>>           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);
> This is a really minor thing, but is there a reason you put this in
> the middle of a bunch of other get_defbool()s, instead of putting it
> just after?  It makes the code just look a bit more ugly. :-)

Not a good one, I was just following where viridian was done.  Will move 
it down a
few lines.

    -Don Slutz

>   -George