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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

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"

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

> 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

> 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. :-)

 -George

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