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

[Don Slutz <dslutz@xxxxxxxxxxx>]

On 09/11/14 07:23, George Dunlap wrote:
> On Mon, Sep 8, 2014 at 2:15 PM, Don Slutz <dslutz@xxxxxxxxxxx> wrote:
> > If non-zero then
> >    Force use of VMware's VGA in QEMU.
> >
> > Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>
> 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@xxxxxxxxxxxxx>
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@xxxxxxxxxx>, Don Slutz <dslutz@xxxxxxxxxxx>
CC: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>, Tim Deegan <tim@xxxxxxx>, Kevin
 Tian <kevin.tian@xxxxxxxxx>, Keir Fraser <keir@xxxxxxx>, Jun Nakajima
        <jun.nakajima@xxxxxxxxx>, Stefano Stabellini
        <stefano.stabellini@xxxxxxxxxxxxx>, Ian Jackson 
<ian.jackson@xxxxxxxxxxxxx>,
        Eddie Dong <eddie.dong@xxxxxxxxx>, <xen-devel@xxxxxxxxxxxxx>, "Aravind
 Gopalakrishnan" <Aravind.Gopalakrishnan@xxxxxxx>, Jan Beulich
        <jbeulich@xxxxxxxx>, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>, 
"Suravee
 Suthikulpanit" <suravee.suthikulpanit@xxxxxxx>
Subject: Re: [Xen-devel] [PATCH v2 1/3] Add vmware_hw to xl.cfg
References: <1409585629-25840-1-git-send-email-dslutz@xxxxxxxxxxx>
 <1409585629-25840-2-git-send-email-dslutz@xxxxxxxxxxx>
 <1410182256.3680.16.camel@xxxxxxxxxxxxxxxxxxxxxx>
 <540E00AB.1000501@xxxxxxxxxxxxx>
 <1410255568.8217.65.camel@xxxxxxxxxxxxxxxxxxxxxx>
 <540F32A0.2070609@xxxxxxxxxxxxx>
 <1410341443.8217.260.camel@xxxxxxxxxxxxxxxxxxxxxx>
In-Reply-To: <1410341443.8217.260.camel@xxxxxxxxxxxxxxxxxxxxxx>




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

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