From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH for-4.5 v6 02/16] tools: Add vmware_hw support Date: Mon, 22 Sep 2014 18:08:00 -0400 Message-ID: <54209DC0.1010609@terremark.com> References: <1411236447-7435-1-git-send-email-dslutz@verizon.com> <1411236447-7435-3-git-send-email-dslutz@verizon.com> <1411392841.18331.100.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1411392841.18331.100.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , Don Slutz Cc: Tim Deegan , Kevin Tian , Keir Fraser , Jun Nakajima , Stefano Stabellini , George Dunlap , Ian Jackson , Eddie Dong , xen-devel@lists.xen.org, Aravind Gopalakrishnan , Jan Beulich , Andrew Cooper , Boris Ostrovsky , Suravee Suthikulpanit List-Id: xen-devel@lists.xenproject.org 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 >> --- >> 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 >> + >> +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 >> >> -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 is not zero). > "The default is cirrus unless B is non-zero in which case it > is vmware." ? Sure. >> >> =item B >> >> 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); >