Re: [PATCH v4 01/16] xen: Add support for VMware cpuid leaves

Re: [PATCH v4 01/16] xen: Add support for VMware cpuid leaves

[Eric Shelton]

[-- Attachment #1.1: Type: text/plain, Size: 2162 bytes --]

On 15.09.14 at 08:42, Jan Beulich wrote:
>>>> On 12.09.14 at 19:46, <dslutz@verizon.com> wrote:
>> On 09/12/14 05:49, Jan Beulich wrote:
>>>>>> On 11.09.14 at 21:49, <andrew.cooper3@citrix.com> wrote:
>>>>> +    case 0x10:
>>>>> +        /* (Virtual) TSC frequency in kHz. */
>>>>> +        *eax =  d->arch.tsc_khz;
>>>>> +        /* (Virtual) Bus (local apic timer) frequency in kHz. */
>>>>> +        *ebx = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
>>>> At least 1 pair of brackets please, especially as the placement of
>>>> brackets affects the result of this particular calculation.
>>> Or simply eliminate one of the divisions using
>>> "1000000ull / APIC_BUS_CYCLE_NS".
>>
>> I am totally confused.  I am happy to go with Jan's version.
>>
>> The confusion is that I get the same answer all the ways I try.
>
> Hmm - this ...
>
>>          ebx1 = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
>>          ebx2 = (1000000000ull / APIC_BUS_CYCLE_NS) / 1000ull;
>>          ebx3 = 1000000000ull / (APIC_BUS_CYCLE_NS * 1000ull);
>
> ... clearly indicates the contrary: You converted to mutiplication
> here, when the respective possibility of putting parentheses here
> would have been
>
>          ebx3 = 1000000000ull / (APIC_BUS_CYCLE_NS / 1000ull);
>
> which I'm sure you agree won't produce the same result. But yes,
> the language implies parentheses this way
>
>          ebx2 = (1000000000ull / APIC_BUS_CYCLE_NS) / 1000ull;
>
> so the original expression without them was correct, just
> slightly ambiguous.
>
> Jan

A different approach for the bus frequency, taken in the patch I just
posted, is to provide the actual bus frequency, rather than a hardcoded
value.  Specifically, the value bus_freq in apic.c.

Additionally, I propose Xen provides the 0x40000010 timing info leaf for
all domains, not just those operating under a VMware compatability mode.
 This is the approach taken in my proposed patch.  OS X will happily use
the 0x40000010 leaf even if the hypervisor vendor ID does not indicate
VMware.  Use of this leaf ended up being the only way I managed to find to
get OS X to configure its timers correctly on a Haswell system.

Eric

[-- Attachment #1.2: Type: text/html, Size: 8022 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v4 01/16] xen: Add support for VMware cpuid leaves

[Don Slutz]

On 09/17/14 00:30, Eric Shelton wrote:
> On 15.09.14 at 08:42, Jan Beulich wrote:
> >>>> On 12.09.14 at 19:46, <dslutz@verizon.com 
> <mailto:dslutz@verizon.com>> wrote:
> >> On 09/12/14 05:49, Jan Beulich wrote:
> >>>>>> On 11.09.14 at 21:49, <andrew.cooper3@citrix.com 
> <mailto:andrew.cooper3@citrix.com>> wrote:
> >>>>> +    case 0x10:
> >>>>> +        /* (Virtual) TSC frequency in kHz. */
> >>>>> +        *eax =  d->arch.tsc_khz;
> >>>>> +        /* (Virtual) Bus (local apic timer) frequency in kHz. */
> >>>>> +        *ebx = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
> >>>> At least 1 pair of brackets please, especially as the placement of
> >>>> brackets affects the result of this particular calculation.
> >>> Or simply eliminate one of the divisions using
> >>> "1000000ull / APIC_BUS_CYCLE_NS".
> >>
> >> I am totally confused.  I am happy to go with Jan's version.
> >>
> >> The confusion is that I get the same answer all the ways I try.
> >
> > Hmm - this ...
> >
> >>        ebx1 = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
> >>        ebx2 = (1000000000ull / APIC_BUS_CYCLE_NS) / 1000ull;
> >>        ebx3 = 1000000000ull / (APIC_BUS_CYCLE_NS * 1000ull);
> >
> > ... clearly indicates the contrary: You converted to mutiplication
> >here, when the respective possibility of putting parentheses here
> >would have been
> >
> > ebx3 = 1000000000ull / (APIC_BUS_CYCLE_NS / 1000ull);
> >
> >which I'm sure you agree won't produce the same result. But yes,
> >the language implies parentheses this way
> >
> > ebx2 = (1000000000ull / APIC_BUS_CYCLE_NS) / 1000ull;
> >
> >so the original expression without them was correct, just
> >slightly ambiguous.
> >
> >Jan
>
> A different approach for the bus frequency, taken in the patch I just 
> posted, is to provide the actual bus frequency, rather than a 
> hardcoded value.  Specifically, the value bus_freq in apic.c.
>

This is not the correct bus frequency for the vlapic code.

I have a server where the host:
...
(XEN) ..... host bus clock speed is 200.0108 MHz.

But the guest sees:
...
..... host bus clock speed is 99.0999 MHz.

(do to a bug in Linux, this is not displayed correctly, the real value 
is 99.999 MHz
because "CONFIG_HZ=1000" not 100)

I also see:
..... host bus clock speed is 100.0000 MHz.


So reporting the 200.0108 to the guest would be wrong.

(And on a different server I have:

(XEN) ..... host bus clock speed is 100.0010 MHz.

for the host; which is closer but still not right).

    -Don Slutz

> Additionally, I propose Xen provides the 0x40000010 timing info leaf 
> for all domains, not just those operating under a VMware compatability 
> mode.  This is the approach taken in my proposed patch.  OS X will 
> happily use the 0x40000010 leaf even if the hypervisor vendor ID does 
> not indicate VMware.  Use of this leaf ended up being the only way I 
> managed to find to get OS X to configure its timers correctly on a 
> Haswell system.
>
> Eric
>

Re: [PATCH v4 01/16] xen: Add support for VMware cpuid leaves

[Eric Shelton]

On Mon, Sep 22, 2014 at 2:21 PM, Don Slutz <dslutz@verizon.com> wrote:
> On 09/17/14 00:30, Eric Shelton wrote:
>>
>> On 15.09.14 at 08:42, Jan Beulich wrote:
>> >>>> On 12.09.14 at 19:46, <dslutz@verizon.com
>> >>>> <mailto:dslutz@verizon.com>> wrote:
>> >> On 09/12/14 05:49, Jan Beulich wrote:
>> >>>>>> On 11.09.14 at 21:49, <andrew.cooper3@citrix.com
>> >>>>>> <mailto:andrew.cooper3@citrix.com>> wrote:
>> >>>>> +    case 0x10:
>> >>>>> +        /* (Virtual) TSC frequency in kHz. */
>> >>>>> +        *eax =  d->arch.tsc_khz;
>> >>>>> +        /* (Virtual) Bus (local apic timer) frequency in kHz. */
>> >>>>> +        *ebx = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
>> >>>> At least 1 pair of brackets please, especially as the placement of
>> >>>> brackets affects the result of this particular calculation.
>> >>> Or simply eliminate one of the divisions using
>> >>> "1000000ull / APIC_BUS_CYCLE_NS".
>> >>
>> >> I am totally confused.  I am happy to go with Jan's version.
>> >>
>> >> The confusion is that I get the same answer all the ways I try.
>> >
>> > Hmm - this ...
>> >
>> >>        ebx1 = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
>> >>        ebx2 = (1000000000ull / APIC_BUS_CYCLE_NS) / 1000ull;
>> >>        ebx3 = 1000000000ull / (APIC_BUS_CYCLE_NS * 1000ull);
>> >
>> > ... clearly indicates the contrary: You converted to mutiplication
>> >here, when the respective possibility of putting parentheses here
>> >would have been
>> >
>> > ebx3 = 1000000000ull / (APIC_BUS_CYCLE_NS / 1000ull);
>> >
>> >which I'm sure you agree won't produce the same result. But yes,
>> >the language implies parentheses this way
>> >
>> > ebx2 = (1000000000ull / APIC_BUS_CYCLE_NS) / 1000ull;
>> >
>> >so the original expression without them was correct, just
>> >slightly ambiguous.
>> >
>> >Jan
>>
>> A different approach for the bus frequency, taken in the patch I just
>> posted, is to provide the actual bus frequency, rather than a hardcoded
>> value.  Specifically, the value bus_freq in apic.c.
>>
>
> This is not the correct bus frequency for the vlapic code.
>
> I have a server where the host:
> ...
> (XEN) ..... host bus clock speed is 200.0108 MHz.
>
> But the guest sees:
> ...
> ..... host bus clock speed is 99.0999 MHz.
>
> (do to a bug in Linux, this is not displayed correctly, the real value is
> 99.999 MHz
> because "CONFIG_HZ=1000" not 100)
>
> I also see:
> ..... host bus clock speed is 100.0000 MHz.
>
>
> So reporting the 200.0108 to the guest would be wrong.
>
> (And on a different server I have:
>
> (XEN) ..... host bus clock speed is 100.0010 MHz.
>
> for the host; which is closer but still not right).
>
>    -Don Slutz

Thank you.  I see I overlooked this, and should have simply used the
vlapic frequency, which is essentially what was already done in the
veridian code.

- Eric

[PATCH v4 00/16] Xen VMware tools support

[Don Slutz]

Changes v3 to v4:
  Ian Campbell:
    Report on both viridian and vmware_hw set.
    Added LIBXL_VGA_INTERFACE_TYPE_VMWARE (vga=vmware).

  Andrew Cooper:
    Add doc for hypervisor-cpuid.

  Boris Ostrovsky:
    Changing regs->error_code may not be a good idea.
      Dropped this.
    
  Jan Beulich & Boris Ostrovsky:
    Only enable vmwxit for GP when vmware_port is set.
      Done.


Changes v2 to v3:

  Add optional unit test tools.
  Re-worked split of changes.

  Jan Beulich:
    for #0:
      I don't think you should be adding a new fine in hvm/ _and_ a new
      subdirectory.
        Moved all files to hvm/vmware that contain code.
    for old #1 (now #1 & #2):
      Is there really a point in enabling both Viridian and VMware extensions?
        I still think so.
      hvmloader change: This needs an explanation
        Dropped as not need now.
      Can you make vmware_hw similar to Viridian, returning success when
      setting the value to what it already is.
        Done.
      You don't seem to be using sub_idx: ...
        Dropped.
      Extra changes...
        Dropped.
    for old #2 (now #3):
      ... these guards have the (theoretical at this point) risk of clashing
      ... the patch is obviously incomplete without this header...
        Did not fix any of these issues.  I will stick with this needs
        to be a 2nd patch that changes the include files to better fit
        in Xen coding.  For now these files are in a sub directory
        which is not part of the normal include search.
        Moved the includeCheck.h file into this patch.
    for old #3 (now #4, #5, #6, #7, #8, #9, #10, #11)
      As I think was said on v1 already - this should be split into smaller
      pieces ...
        Done.
      All this would very likely better go into a separate function placed in
      vmport.c.
        Moved most of the code into vmport.c or vmport_rpc.c.
      In any event I'm rather uncomfortable about vmware_port getting
      enabled unconditionally, ...
        Added vmware_port (done in new patches #4, #5) as an xl.cfg
        option.
      You'll have to go through and fix coding style issues.
        I think I have found all these, but since they do not stand out
        for me, let me know of any left.
      "MAKE_INSTR(IN," name is ambiguous.
        Added all 4 opcodes for in and out that can access this port: INB_DX,
        INL_DX, OUTB_DX, OUTL_DX.
      A VMX-specific function shouldn't be named this way...
        Added new common routine vmport_gp_check() that is called from
        both vmx.c and svm.c which is where all the logic about checking
        for IN ans OUT is done.
        Also fixed naming and added static.
      Ah, here we go (as to using HVM_DBG_LOG()): Isn't this _way_ too
      fine grained?
        I have reduced the number of bits used.  Partialy by switching
        some to xentrace (new patch #6 and #7).
      Right, and zero is an indication that it wasn't found. Also I just
      noticed there's a gdprintk() in that event, which for all other ...
        Made the gdprintk() optional.

End of v3 changes.

This is a small part of the changes needed to allow running Linux
and windows (and others) guests that were built on VMware and run
run them unchanged on Xen.

This small part is the start of Xen support of VMware backdoor I/O
port which is how VMware tools (a standard addition installed on a
guest) communicates to the hypervisor.

I picked this subset to start with because it only has changes in
Xen.

Some of this code is already in QEMU and so KVM has some of this
already.  QEMU supported backdoor commands include VMware mouse
support.  A later patch set exists that links these changes, new
code and Xen changes to QEMU to provide VMware mouse support under
Xen.  The important part is that VMware mouse is an absolute
position mouse and so network delays do not effect usage of the
virtual mouse.

For example from the guest:

[root@C63-min-tools ~]# vmtoolsd --cmd "info-get guestinfo.joejoel"
No value found
[root@C63-min-tools ~]# vmtoolsd --cmd "info-set guestinfo.joejoel short"

[root@C63-min-tools ~]# vmtoolsd --cmd "info-get guestinfo.joejoel"
short
[root@C63-min-tools ~]# vmtoolsd --cmd "info-set guestinfo.joejoel long222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000joel"

[root@C63-min-tools ~]# vmtoolsd --cmd "info-get guestinfo.key1"
data1
[root@C63-min-tools ~]# vmtoolsd --cmd "info-get guestinfo.key2"
No value found
[root@C63-min-tools ~]# vmtoolsd --cmd "info-get guestinfo.key2"
data2
[root@C63-min-tools ~]# 


Most of this code has been reverse engineered by looking at
source code for Linux and open VMware tools.

http://open-vm-tools.sourceforge.net


changes RFC to v2:

Jan Beulich:
  Add xen/arch/x86/hvm/vmware.c for cpuid_vmware_leaves
  Fewer patches

Andrew Cooper:
  use the proper constant for apic_khz
  Follow 839b966e3f587bbb1a0d954230fb3904330dccb6 style changes.
  Changed HVM_PARAM_VMWARE_HW to write once (make is_vmware_domain()
    more static).
  Dropped vmport status stuff.
  Added checks for xzalloc() having failed.
  You should include backdoor_def.h ...
     Every thing I tried did not work better.  So I did not
     change VMPORT_PORT and BDOOR_PORT being the same value.
     I did not try and adjust VMware's include file backdoor_def.h
     to working in other xen source files.
  Switching to s_time_t is not valid. get_sec() is defined:
    unsigned long get_sec(void);
  and so my uses of it should be using unsigned long.  However
  since that is not a fixed width type, I used the uint64_t
  data type which is almost the same, but does allow the 32 bit
  build of libxc, libxl to do the correct thing.


Konrad Rzeszutek Wilk:
  Please don't include the address. It should be, etc
      about the Vmware provided include files.
    I went with no changes to these files.  Even if the files should
    be changed to match xen coding style, etc I still feel that the
    original ones should be added via a patch, and then adjusted in a
    2nd patch.
  Can you use XenBus?
    I would say no.  XenBus (and XenStore) is about domain to domain
    communication.  This is about VMware's hyper-call and providing
    access to VMware's guest info very low speed access.

Olaf Hering:
   Dropped changing of bios-strings.  Still needs some documentation
   about this may be needed to do in a tool stack or set of commands.


Boris Ostrovsky:
  Use svm_nextrip_insn_length()
    Looks like __get_instruction_length() does this, so switched to
    __get_instruction_length().
 
RFC:

See

http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=displayKC&externalId=1009458

for info on detecting VMware.

Linux does not follow this exactly.  It checks for CPUID 1st.  If
that fails, it checks for SMBIOS containing "VMware" (not VMware- or
VMW).

So this patch set provides:

        SMBIOS -- Add string VMware-
        CPUID -- Add VMware's CPUID (Note: currently HyperV (viridian support) breaks this check.)
        Add the magic VMware port
            Allow VMware tools poweroff and reboot
            Enable access to VMware's guest info
            Provide the VMware tools build number


Don Slutz (16):
  xen: Add support for VMware cpuid leaves
  tools: Add vmware_hw support
  vmware: Add VMware provided include files.
  xen: Add is_vmware_port_enabled
  tools: Add vmware_port support
  xen: Convert vmware_port to xentrace usage
  tools: Convert vmware_port to xentrace usage
  xen: Add limited support of VMware's hyper-call rpc
  tools: Add limited support of VMware's hyper-call rpc
  Add VMware tool's triggers
  Add live migration of VMware's hyper-call RPC
  Add dump of HVM_SAVE_CODE(VMPORT) to xen-hvmctx.
  Add xen-hvm-param
  Add xen-vmware-guestinfo
  Add xen-list-vmware-guestinfo
  Add xen-hvm-send-trigger

 .gitignore                                   |    4 +
 docs/man/xl.cfg.pod.5                        |   28 +-
 docs/misc/hypervisor-cpuid.markdown          |   29 +
 tools/libxc/xc_domain.c                      |  115 ++
 tools/libxc/xc_domain_restore.c              |   14 +
 tools/libxc/xc_domain_save.c                 |   11 +
 tools/libxc/xenctrl.h                        |   24 +
 tools/libxc/xg_save_restore.h                |    2 +
 tools/libxl/libxl.h                          |   15 +
 tools/libxl/libxl_create.c                   |    6 +-
 tools/libxl/libxl_dm.c                       |   10 +-
 tools/libxl/libxl_dom.c                      |    2 +
 tools/libxl/libxl_types.idl                  |    3 +
 tools/libxl/xl_cmdimpl.c                     |   12 +-
 tools/misc/Makefile                          |   16 +-
 tools/misc/xen-hvm-param.c                   |  164 +++
 tools/misc/xen-hvm-send-trigger.c            |  103 ++
 tools/misc/xen-hvmctx.c                      |  229 ++++
 tools/misc/xen-list-vmware-guestinfo.c       |   88 ++
 tools/misc/xen-vmware-guestinfo.c            |   97 ++
 tools/xentrace/formats                       |   13 +
 xen/arch/x86/domain.c                        |    2 +
 xen/arch/x86/domctl.c                        |   34 +
 xen/arch/x86/hvm/Makefile                    |    3 +-
 xen/arch/x86/hvm/hvm.c                       |   79 ++
 xen/arch/x86/hvm/svm/emulate.c               |   26 +-
 xen/arch/x86/hvm/svm/svm.c                   |   53 +-
 xen/arch/x86/hvm/svm/vmcb.c                  |    2 +
 xen/arch/x86/hvm/vmware/Makefile             |    3 +
 xen/arch/x86/hvm/vmware/backdoor_def.h       |  167 +++
 xen/arch/x86/hvm/vmware/cpuid.c              |   69 ++
 xen/arch/x86/hvm/vmware/guest_msg_def.h      |   87 ++
 xen/arch/x86/hvm/vmware/includeCheck.h       |   17 +
 xen/arch/x86/hvm/vmware/vmport.c             |  324 ++++++
 xen/arch/x86/hvm/vmware/vmport_rpc.c         | 1581 ++++++++++++++++++++++++++
 xen/arch/x86/hvm/vmx/vmcs.c                  |    1 +
 xen/arch/x86/hvm/vmx/vmx.c                   |   72 ++
 xen/arch/x86/hvm/vmx/vvmx.c                  |   14 +
 xen/arch/x86/traps.c                         |    8 +-
 xen/common/domctl.c                          |    3 +
 xen/include/asm-x86/hvm/domain.h             |    7 +
 xen/include/asm-x86/hvm/hvm.h                |    3 +
 xen/include/asm-x86/hvm/io.h                 |    2 +-
 xen/include/asm-x86/hvm/svm/emulate.h        |   13 +-
 xen/include/asm-x86/hvm/trace.h              |   45 +
 xen/include/asm-x86/hvm/vmport.h             |   72 ++
 xen/include/asm-x86/hvm/vmware.h             |   31 +
 xen/include/public/arch-x86/hvm/save.h       |   39 +-
 xen/include/public/arch-x86/hvm/vmporttype.h |  118 ++
 xen/include/public/domctl.h                  |    6 +
 xen/include/public/hvm/hvm_op.h              |   18 +
 xen/include/public/hvm/params.h              |    8 +-
 xen/include/public/trace.h                   |   12 +
 xen/include/xen/sched.h                      |    3 +
 54 files changed, 3884 insertions(+), 23 deletions(-)
 create mode 100644 docs/misc/hypervisor-cpuid.markdown
 create mode 100644 tools/misc/xen-hvm-param.c
 create mode 100644 tools/misc/xen-hvm-send-trigger.c
 create mode 100644 tools/misc/xen-list-vmware-guestinfo.c
 create mode 100644 tools/misc/xen-vmware-guestinfo.c
 create mode 100644 xen/arch/x86/hvm/vmware/Makefile
 create mode 100644 xen/arch/x86/hvm/vmware/backdoor_def.h
 create mode 100644 xen/arch/x86/hvm/vmware/cpuid.c
 create mode 100644 xen/arch/x86/hvm/vmware/guest_msg_def.h
 create mode 100644 xen/arch/x86/hvm/vmware/includeCheck.h
 create mode 100644 xen/arch/x86/hvm/vmware/vmport.c
 create mode 100644 xen/arch/x86/hvm/vmware/vmport_rpc.c
 create mode 100644 xen/include/asm-x86/hvm/vmport.h
 create mode 100644 xen/include/asm-x86/hvm/vmware.h
 create mode 100644 xen/include/public/arch-x86/hvm/vmporttype.h

-- 
1.8.4

[PATCH v4 01/16] xen: Add support for VMware cpuid leaves

[Don Slutz]

This is done by adding 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
  Return VMware's cpuid leaves.

The support of hypervisor cpuid leaves has not been agreed to.

MicroSoft Hyper-V (AKA viridian) currently must be at 0x40000000.

VMware currently must be at 0x40000000.

KVM currently must be at 0x40000000 (from Seabios).

Seabios will find xen at 0x40000000, 0x40000100, 0x40000200 ..
0x40010000.

http://download.microsoft.com/download/F/B/0/FB0D01A3-8E3A-4F5F-AA59-08C8026D3B8A/requirements-for-implementing-microsoft-hypervisor-interface.docx

http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=displayKC&externalId=1009458

http://lwn.net/Articles/301888/
  Attempted to get this cleaned up.

So based on this, I picked the order:

Xen at 0x40000000 or
Viridian or VMware at 0x40000000 and Xen at 0x40000100

If both Viridian and VMware selected, report an error.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 xen/arch/x86/hvm/Makefile        |  3 +-
 xen/arch/x86/hvm/hvm.c           | 32 +++++++++++++++++++
 xen/arch/x86/hvm/vmware/Makefile |  1 +
 xen/arch/x86/hvm/vmware/cpuid.c  | 69 ++++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/traps.c             |  8 +++--
 xen/include/asm-x86/hvm/hvm.h    |  3 ++
 xen/include/asm-x86/hvm/vmware.h | 31 ++++++++++++++++++
 xen/include/public/hvm/params.h  |  5 ++-
 8 files changed, 148 insertions(+), 4 deletions(-)
 create mode 100644 xen/arch/x86/hvm/vmware/Makefile
 create mode 100644 xen/arch/x86/hvm/vmware/cpuid.c
 create mode 100644 xen/include/asm-x86/hvm/vmware.h

diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index eea5555..77598a6 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -1,5 +1,6 @@
 subdir-y += svm
 subdir-y += vmx
+subdir-y += vmware
 
 obj-y += asid.o
 obj-y += emulate.o
@@ -22,4 +23,4 @@ obj-y += vlapic.o
 obj-y += vmsi.o
 obj-y += vpic.o
 obj-y += vpt.o
-obj-y += vpmu.o
\ No newline at end of file
+obj-y += vpmu.o
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8d905d3..03a1a19 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -57,6 +57,7 @@
 #include <asm/hvm/cacheattr.h>
 #include <asm/hvm/trace.h>
 #include <asm/hvm/nestedhvm.h>
+#include <asm/hvm/vmware.h>
 #include <asm/mtrr.h>
 #include <asm/apic.h>
 #include <public/sched.h>
@@ -4228,6 +4229,9 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
     if ( cpuid_viridian_leaves(input, eax, ebx, ecx, edx) )
         return;
 
+    if ( cpuid_vmware_leaves(input, eax, ebx, ecx, edx) )
+        return;
+
     if ( cpuid_hypervisor_leaves(input, count, eax, ebx, ecx, edx) )
         return;
 
@@ -5555,6 +5559,11 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
                     rc = -EINVAL;
                 break;
             case HVM_PARAM_VIRIDIAN:
+                if ( d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW] )
+                {
+                    rc = -EXDEV;
+                    break;
+                }
                 if ( a.value > 1 )
                     rc = -EINVAL;
                 break;
@@ -5692,6 +5701,29 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
 
                 break;
             }
+            case HVM_PARAM_VMWARE_HW:
+                /*
+                 * This should only ever be set non-zero one time by
+                 * the tools and is read only by the guest.
+                 */
+                if ( d == current->domain )
+                {
+                    rc = -EPERM;
+                    break;
+                }
+                if ( d->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN] )
+                {
+                    rc = -EXDEV;
+                    break;
+                }
+                if ( d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW] &&
+                     d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW] !=
+                     a.value )
+                {
+                    rc = -EEXIST;
+                    break;
+                }
+                break;
             }
 
             if ( rc == 0 ) 
diff --git a/xen/arch/x86/hvm/vmware/Makefile b/xen/arch/x86/hvm/vmware/Makefile
new file mode 100644
index 0000000..3fb2e0b
--- /dev/null
+++ b/xen/arch/x86/hvm/vmware/Makefile
@@ -0,0 +1 @@
+obj-y += cpuid.o
diff --git a/xen/arch/x86/hvm/vmware/cpuid.c b/xen/arch/x86/hvm/vmware/cpuid.c
new file mode 100644
index 0000000..730ab8f
--- /dev/null
+++ b/xen/arch/x86/hvm/vmware/cpuid.c
@@ -0,0 +1,69 @@
+/*
+ * arch/x86/hvm/vmware/cpuid.c
+ *
+ * Copyright (C) 2012 Verizon Corporation
+ *
+ * This file is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License Version 2 (GPLv2)
+ * as published by the Free Software Foundation.
+ *
+ * This file is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details. <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/sched.h>
+
+#include <asm/hvm/hvm.h>
+#include <asm/hvm/vmware.h>
+
+int cpuid_vmware_leaves(uint32_t idx, uint32_t *eax, uint32_t *ebx,
+                        uint32_t *ecx, uint32_t *edx)
+{
+    struct domain *d = current->domain;
+
+    if ( !is_vmware_domain(d) )
+        return 0;
+
+    switch ( idx - 0x40000000 )
+    {
+    case 0x0:
+        *eax = 0x40000010;  /* Largest leaf */
+        *ebx = 0x61774d56;  /* "VMwa" */
+        *ecx = 0x4d566572;  /* "reVM" */
+        *edx = 0x65726177;  /* "ware" */
+        break;
+
+    case 0x1 ... 0xf:
+        *eax = 0;          /* Reserved */
+        *ebx = 0;          /* Reserved */
+        *ecx = 0;          /* Reserved */
+        *edx = 0;          /* Reserved */
+        break;
+
+    case 0x10:
+        /* (Virtual) TSC frequency in kHz. */
+        *eax =  d->arch.tsc_khz;
+        /* (Virtual) Bus (local apic timer) frequency in kHz. */
+        *ebx = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
+        *ecx = 0;          /* Reserved */
+        *edx = 0;          /* Reserved */
+        break;
+
+    default:
+        return 0;
+    }
+
+    return 1;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 10fc2ca..f353f42 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -685,8 +685,12 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
                uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
 {
     struct domain *d = current->domain;
-    /* Optionally shift out of the way of Viridian architectural leaves. */
-    uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
+    /*
+     * Optionally shift out of the way of Viridian or VMware
+     * architectural leaves.
+     */
+    uint32_t base = is_viridian_domain(d) | is_vmware_domain(d) ?
+        0x40000100 : 0x40000000;
     uint32_t limit, dummy;
 
     idx -= base;
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 1123857..546210a 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -347,6 +347,9 @@ static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
 #define is_viridian_domain(_d)                                             \
  (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN]))
 
+#define is_vmware_domain(_d)                                             \
+ (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW]))
+
 void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
                                uint32_t *eax, uint32_t *ebx,
                                uint32_t *ecx, uint32_t *edx);
diff --git a/xen/include/asm-x86/hvm/vmware.h b/xen/include/asm-x86/hvm/vmware.h
new file mode 100644
index 0000000..f254106
--- /dev/null
+++ b/xen/include/asm-x86/hvm/vmware.h
@@ -0,0 +1,31 @@
+/*
+ * asm-x86/hvm/vmware.h
+ *
+ * Copyright (C) 2012 Verizon Corporation
+ *
+ * This file is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License Version 2 (GPLv2)
+ * as published by the Free Software Foundation.
+ *
+ * This file is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details. <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef ASM_X86_HVM_VMWARE_H__
+#define ASM_X86_HVM_VMWARE_H__
+
+int cpuid_vmware_leaves(uint32_t idx, uint32_t *eax, uint32_t *ebx,
+                        uint32_t *ecx, uint32_t *edx);
+
+#endif /* ASM_X86_HVM_VMWARE_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 614ff5f..dee6d68 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -151,6 +151,9 @@
 /* Location of the VM Generation ID in guest physical address space. */
 #define HVM_PARAM_VM_GENERATION_ID_ADDR 34
 
-#define HVM_NR_PARAMS          35
+/* Params for VMware */
+#define HVM_PARAM_VMWARE_HW                 35
+
+#define HVM_NR_PARAMS          36
 
 #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
-- 
1.8.4

Re: [PATCH v4 01/16] xen: Add support for VMware cpuid leaves

[Andrew Cooper]

On 11/09/2014 19:36, Don Slutz wrote:
> This is done by adding 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
>   Return VMware's cpuid leaves.
>
> The support of hypervisor cpuid leaves has not been agreed to.
>
> MicroSoft Hyper-V (AKA viridian) currently must be at 0x40000000.
>
> VMware currently must be at 0x40000000.
>
> KVM currently must be at 0x40000000 (from Seabios).
>
> Seabios will find xen at 0x40000000, 0x40000100, 0x40000200 ..
> 0x40010000.
>
> http://download.microsoft.com/download/F/B/0/FB0D01A3-8E3A-4F5F-AA59-08C8026D3B8A/requirements-for-implementing-microsoft-hypervisor-interface.docx
>
> http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=displayKC&externalId=1009458
>
> http://lwn.net/Articles/301888/
>   Attempted to get this cleaned up.
>
> So based on this, I picked the order:
>
> Xen at 0x40000000 or
> Viridian or VMware at 0x40000000 and Xen at 0x40000100
>
> If both Viridian and VMware selected, report an error.
>
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
>  xen/arch/x86/hvm/Makefile        |  3 +-
>  xen/arch/x86/hvm/hvm.c           | 32 +++++++++++++++++++
>  xen/arch/x86/hvm/vmware/Makefile |  1 +
>  xen/arch/x86/hvm/vmware/cpuid.c  | 69 ++++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/traps.c             |  8 +++--
>  xen/include/asm-x86/hvm/hvm.h    |  3 ++
>  xen/include/asm-x86/hvm/vmware.h | 31 ++++++++++++++++++
>  xen/include/public/hvm/params.h  |  5 ++-
>  8 files changed, 148 insertions(+), 4 deletions(-)
>  create mode 100644 xen/arch/x86/hvm/vmware/Makefile
>  create mode 100644 xen/arch/x86/hvm/vmware/cpuid.c
>  create mode 100644 xen/include/asm-x86/hvm/vmware.h
>
> diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
> index eea5555..77598a6 100644
> --- a/xen/arch/x86/hvm/Makefile
> +++ b/xen/arch/x86/hvm/Makefile
> @@ -1,5 +1,6 @@
>  subdir-y += svm
>  subdir-y += vmx
> +subdir-y += vmware
>  
>  obj-y += asid.o
>  obj-y += emulate.o
> @@ -22,4 +23,4 @@ obj-y += vlapic.o
>  obj-y += vmsi.o
>  obj-y += vpic.o
>  obj-y += vpt.o
> -obj-y += vpmu.o
> \ No newline at end of file
> +obj-y += vpmu.o

This hunk is unrelated, but is perhaps something better fixed.  A
passing note in the commit message perhaps?

> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 8d905d3..03a1a19 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -57,6 +57,7 @@
>  #include <asm/hvm/cacheattr.h>
>  #include <asm/hvm/trace.h>
>  #include <asm/hvm/nestedhvm.h>
> +#include <asm/hvm/vmware.h>
>  #include <asm/mtrr.h>
>  #include <asm/apic.h>
>  #include <public/sched.h>
> @@ -4228,6 +4229,9 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
>      if ( cpuid_viridian_leaves(input, eax, ebx, ecx, edx) )
>          return;
>  
> +    if ( cpuid_vmware_leaves(input, eax, ebx, ecx, edx) )
> +        return;
> +
>      if ( cpuid_hypervisor_leaves(input, count, eax, ebx, ecx, edx) )
>          return;
>  
> @@ -5555,6 +5559,11 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>                      rc = -EINVAL;
>                  break;
>              case HVM_PARAM_VIRIDIAN:
> +                if ( d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW] )
> +                {
> +                    rc = -EXDEV;
> +                    break;
> +                }
>                  if ( a.value > 1 )
>                      rc = -EINVAL;
>                  break;
> @@ -5692,6 +5701,29 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>                  break;
>              }
> +            case HVM_PARAM_VMWARE_HW:
> +                /*
> +                 * This should only ever be set non-zero one time by
> +                 * the tools and is read only by the guest.
> +                 */
> +                if ( d == current->domain )
> +                {
> +                    rc = -EPERM;
> +                    break;
> +                }
> +                if ( d->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN] )
> +                {
> +                    rc = -EXDEV;
> +                    break;
> +                }
> +                if ( d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW] &&
> +                     d->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW] !=
> +                     a.value )
> +                {
> +                    rc = -EEXIST;
> +                    break;
> +                }
> +                break;

Eugh - its code like this which is why I am certain that things such as
VIRIDIAN, VMWARE (and NESTED_VIRT for that matter) should be domain
creation flags rather than hvm params, and completely immutable for the
entire lifetime of the domain.  After all, these params are only being
used as glorified, set-once booleans.

I even have an in-progress patch to turn VIRIDIAN into a create flag
(which has now been complicated by VIRIDIAN turning into a bitmask), but
it appear that doing this necessitates breaking the Libxl API.  For
reasons which elude me, the Libxl API exposes the internal
implementation details between "domain creation information" and "domain
build information" in such a way that I couldn't find a compatible way
of moving information from from the latter to the former.  (i.e. you
can't duplicate the same field in the create info and still have a user
of an old libxl API version still have their domain creation work when
they only specify the value in the build info)

I am not sure how "fixing things correctly in Xen" fairs against "libxl
taking pain and possibly an API breakage because it previously exposed
internal details which it shouldn't have done", but I would prefer that
we didn't make the problem any harder to fix than it already is.

As a result, I am formally suggesting that this would be better done by
adding a domain creation flag (although not being a maintainer, I
realise my views in this matter don't strictly count for much).

>              }
>  
>              if ( rc == 0 ) 
> diff --git a/xen/arch/x86/hvm/vmware/Makefile b/xen/arch/x86/hvm/vmware/Makefile
> new file mode 100644
> index 0000000..3fb2e0b
> --- /dev/null
> +++ b/xen/arch/x86/hvm/vmware/Makefile
> @@ -0,0 +1 @@
> +obj-y += cpuid.o
> diff --git a/xen/arch/x86/hvm/vmware/cpuid.c b/xen/arch/x86/hvm/vmware/cpuid.c
> new file mode 100644
> index 0000000..730ab8f
> --- /dev/null
> +++ b/xen/arch/x86/hvm/vmware/cpuid.c
> @@ -0,0 +1,69 @@
> +/*
> + * arch/x86/hvm/vmware/cpuid.c
> + *
> + * Copyright (C) 2012 Verizon Corporation
> + *
> + * This file is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License Version 2 (GPLv2)
> + * as published by the Free Software Foundation.
> + *
> + * This file is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details. <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/sched.h>
> +
> +#include <asm/hvm/hvm.h>
> +#include <asm/hvm/vmware.h>
> +
> +int cpuid_vmware_leaves(uint32_t idx, uint32_t *eax, uint32_t *ebx,
> +                        uint32_t *ecx, uint32_t *edx)
> +{
> +    struct domain *d = current->domain;
> +
> +    if ( !is_vmware_domain(d) )
> +        return 0;
> +
> +    switch ( idx - 0x40000000 )
> +    {
> +    case 0x0:

Do these leaves have semantic names from a spec, which could live in an
appropriate header file?

> +        *eax = 0x40000010;  /* Largest leaf */
> +        *ebx = 0x61774d56;  /* "VMwa" */
> +        *ecx = 0x4d566572;  /* "reVM" */
> +        *edx = 0x65726177;  /* "ware" */
> +        break;
> +
> +    case 0x1 ... 0xf:
> +        *eax = 0;          /* Reserved */
> +        *ebx = 0;          /* Reserved */
> +        *ecx = 0;          /* Reserved */
> +        *edx = 0;          /* Reserved */
> +        break;
> +
> +    case 0x10:
> +        /* (Virtual) TSC frequency in kHz. */
> +        *eax =  d->arch.tsc_khz;
> +        /* (Virtual) Bus (local apic timer) frequency in kHz. */
> +        *ebx = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;

At least 1 pair of brackets please, especially as the placement of
brackets affects the result of this particular calculation.

> +        *ecx = 0;          /* Reserved */
> +        *edx = 0;          /* Reserved */
> +        break;
> +
> +    default:
> +        return 0;
> +    }
> +
> +    return 1;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 10fc2ca..f353f42 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -685,8 +685,12 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
>                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
>  {
>      struct domain *d = current->domain;
> -    /* Optionally shift out of the way of Viridian architectural leaves. */
> -    uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
> +    /*
> +     * Optionally shift out of the way of Viridian or VMware
> +     * architectural leaves.
> +     */
> +    uint32_t base = is_viridian_domain(d) | is_vmware_domain(d) ?
> +        0x40000100 : 0x40000000;

Again, brackets please for binary operators.  (We have had one recent
slipup because of the precedence of | and ?:)

Furthermore, I think you mean (is_viridian_domain(d) ||
is_vmware_domain(d)) ? 0x40000100 : 0x40000000; which allows for short
circuiting of is_vmware_domain().

>      uint32_t limit, dummy;
>  
>      idx -= base;
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 1123857..546210a 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -347,6 +347,9 @@ static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
>  #define is_viridian_domain(_d)                                             \
>   (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN]))
>  
> +#define is_vmware_domain(_d)                                             \
> + (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW]))
> +
>  void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
>                                 uint32_t *eax, uint32_t *ebx,
>                                 uint32_t *ecx, uint32_t *edx);
> diff --git a/xen/include/asm-x86/hvm/vmware.h b/xen/include/asm-x86/hvm/vmware.h
> new file mode 100644
> index 0000000..f254106
> --- /dev/null
> +++ b/xen/include/asm-x86/hvm/vmware.h
> @@ -0,0 +1,31 @@
> +/*
> + * asm-x86/hvm/vmware.h
> + *
> + * Copyright (C) 2012 Verizon Corporation
> + *
> + * This file is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License Version 2 (GPLv2)
> + * as published by the Free Software Foundation.
> + *
> + * This file is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details. <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef ASM_X86_HVM_VMWARE_H__
> +#define ASM_X86_HVM_VMWARE_H__

#include <xen/types.h> (IIRC) please, or uint32_t could cause a
compilation failure given specific orderings of #include's in
translation units.

~Andrew

> +
> +int cpuid_vmware_leaves(uint32_t idx, uint32_t *eax, uint32_t *ebx,
> +                        uint32_t *ecx, uint32_t *edx);
> +
> +#endif /* ASM_X86_HVM_VMWARE_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
> index 614ff5f..dee6d68 100644
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -151,6 +151,9 @@
>  /* Location of the VM Generation ID in guest physical address space. */
>  #define HVM_PARAM_VM_GENERATION_ID_ADDR 34
>  
> -#define HVM_NR_PARAMS          35
> +/* Params for VMware */
> +#define HVM_PARAM_VMWARE_HW                 35
> +
> +#define HVM_NR_PARAMS          36
>  
>  #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */

Re: [PATCH v4 01/16] xen: Add support for VMware cpuid leaves

[Jan Beulich]

>>> On 11.09.14 at 21:49, <andrew.cooper3@citrix.com> wrote:
> I am not sure how "fixing things correctly in Xen" fairs against "libxl
> taking pain and possibly an API breakage because it previously exposed
> internal details which it shouldn't have done", but I would prefer that
> we didn't make the problem any harder to fix than it already is.
> 
> As a result, I am formally suggesting that this would be better done by
> adding a domain creation flag (although not being a maintainer, I
> realise my views in this matter don't strictly count for much).

The main reservation I have against this is that this, in the long run,
may result in a proliferation of VM creation flags. The slightly abused
HVM params at least make adding support of another kind of foreign
VMM emulation a pretty straightforward and isolated change.

>> +    case 0x10:
>> +        /* (Virtual) TSC frequency in kHz. */
>> +        *eax =  d->arch.tsc_khz;
>> +        /* (Virtual) Bus (local apic timer) frequency in kHz. */
>> +        *ebx = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
> 
> At least 1 pair of brackets please, especially as the placement of
> brackets affects the result of this particular calculation.

Or simply eliminate one of the divisions using
"1000000ull / APIC_BUS_CYCLE_NS".

>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -685,8 +685,12 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
>>                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
>>  {
>>      struct domain *d = current->domain;
>> -    /* Optionally shift out of the way of Viridian architectural leaves. */
>> -    uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
>> +    /*
>> +     * Optionally shift out of the way of Viridian or VMware
>> +     * architectural leaves.
>> +     */
>> +    uint32_t base = is_viridian_domain(d) | is_vmware_domain(d) ?
>> +        0x40000100 : 0x40000000;
> 
> Again, brackets please for binary operators.  (We have had one recent
> slipup because of the precedence of | and ?:)

I don't think parentheses are strictly required here - we're not very
consistent with when to disambiguate operator precedence by using
them when not strictly needed anyway, and I think people touching
hypervisor code can be expected to know the language specification
well enough.

Jan

Re: [PATCH v4 01/16] xen: Add support for VMware cpuid leaves

[Don Slutz]

On 09/12/14 05:49, Jan Beulich wrote:
>>>> On 11.09.14 at 21:49, <andrew.cooper3@citrix.com> wrote:
>>>>


>>> +    case 0x10:
>>> +        /* (Virtual) TSC frequency in kHz. */
>>> +        *eax =  d->arch.tsc_khz;
>>> +        /* (Virtual) Bus (local apic timer) frequency in kHz. */
>>> +        *ebx = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
>> At least 1 pair of brackets please, especially as the placement of
>> brackets affects the result of this particular calculation.
> Or simply eliminate one of the divisions using
> "1000000ull / APIC_BUS_CYCLE_NS".

I am totally confused.  I am happy to go with Jan's version.

The confusion is that I get the same answer all the ways I try. This 
includes
when APIC_BUS_CYCLE_NS is other values then 10.


Some facts:
1) 1000000000 takes only 30bits, I am asking it to be 64.
2) I am only making it smaller in all cases.

I have tested this on CentOS release 6.5 (Final) both 32 and 64 and
it all says they are all the same.

Here is my testing:

build1:~/tmp/zz-div>p /etc/redhat-release
CentOS release 6.5 (Final)
build1:~/tmp/zz-div>uname -a
Linux build1 2.6.32-431.el6.i686 #1 SMP Fri Nov 22 00:26:36 UTC 2013 
i686 i686 i386 GNU/Linux
build1:~/tmp/zz-div>cat zz-div.c
#include <stdio.h>
#include <stdint.h>

int main(void)
{
     uint32_t ebx1, ebx2, ebx3, ebx4, ebx5;
     uint32_t start = 1000000000ull;
     uint32_t APIC_BUS_CYCLE_NS;

     printf("start=0x%x, %d\n\n", start, start);
     for ( APIC_BUS_CYCLE_NS = 1; APIC_BUS_CYCLE_NS < 10000; 
APIC_BUS_CYCLE_NS++ )
     {
         ebx1 = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
         ebx2 = (1000000000ull / APIC_BUS_CYCLE_NS) / 1000ull;
         ebx3 = 1000000000ull / (APIC_BUS_CYCLE_NS * 1000ull);
         ebx4 = 1000000000ull / 1000ull / APIC_BUS_CYCLE_NS;
         ebx5 = 1000000ull / APIC_BUS_CYCLE_NS;

         if ( ebx1 != ebx2 ||
              ebx1 != ebx3 ||
              ebx1 != ebx4 ||
              ebx1 != ebx5 )
         {
             printf("APIC_BUS_CYCLE_NS=%d:\n", APIC_BUS_CYCLE_NS);
             printf(" ebx1=0x%x, %d\n", ebx1, ebx1);
             printf(" ebx2=0x%x, %d\n", ebx2, ebx2);
             printf(" ebx3=0x%x, %d\n", ebx3, ebx3);
             printf(" ebx4=0x%x, %d\n", ebx4, ebx4);
             printf(" ebx5=0x%x, %d\n", ebx5, ebx5);
         }
     }

     return 0;
}

/*
  * Local variables:
  * mode: C
  * c-file-style: "BSD"
  * c-basic-offset: 4
  * indent-tabs-mode: nil
  * End:
  */
build1:~/tmp/zz-div>gcc -g -o zz-div zz-div.c
build1:~/tmp/zz-div>./zz-div
start=0x3b9aca00, 1000000000

build1:~/tmp/zz-div>


    -Don Slutz




>
>
> Jan
>

Re: [PATCH v4 01/16] xen: Add support for VMware cpuid leaves

[Jan Beulich]

>>> On 12.09.14 at 19:46, <dslutz@verizon.com> wrote:
> On 09/12/14 05:49, Jan Beulich wrote:
>>>>> On 11.09.14 at 21:49, <andrew.cooper3@citrix.com> wrote:
>>>> +    case 0x10:
>>>> +        /* (Virtual) TSC frequency in kHz. */
>>>> +        *eax =  d->arch.tsc_khz;
>>>> +        /* (Virtual) Bus (local apic timer) frequency in kHz. */
>>>> +        *ebx = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
>>> At least 1 pair of brackets please, especially as the placement of
>>> brackets affects the result of this particular calculation.
>> Or simply eliminate one of the divisions using
>> "1000000ull / APIC_BUS_CYCLE_NS".
> 
> I am totally confused.  I am happy to go with Jan's version.
> 
> The confusion is that I get the same answer all the ways I try.

Hmm - this ...

>          ebx1 = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
>          ebx2 = (1000000000ull / APIC_BUS_CYCLE_NS) / 1000ull;
>          ebx3 = 1000000000ull / (APIC_BUS_CYCLE_NS * 1000ull);

... clearly indicates the contrary: You converted to mutiplication
here, when the respective possibility of putting parentheses here
would have been

         ebx3 = 1000000000ull / (APIC_BUS_CYCLE_NS / 1000ull);

which I'm sure you agree won't produce the same result. But yes,
the language implies parentheses this way

         ebx2 = (1000000000ull / APIC_BUS_CYCLE_NS) / 1000ull;

so the original expression without them was correct, just
slightly ambiguous.

Jan

Re: [PATCH v4 01/16] xen: Add support for VMware cpuid leaves

[Don Slutz]

On 09/15/14 03:42, Jan Beulich wrote:
>>>> On 12.09.14 at 19:46, <dslutz@verizon.com> wrote:
>> On 09/12/14 05:49, Jan Beulich wrote:
>>>>>> On 11.09.14 at 21:49, <andrew.cooper3@citrix.com> wrote:
>>>>> +    case 0x10:
>>>>> +        /* (Virtual) TSC frequency in kHz. */
>>>>> +        *eax =  d->arch.tsc_khz;
>>>>> +        /* (Virtual) Bus (local apic timer) frequency in kHz. */
>>>>> +        *ebx = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
>>>> At least 1 pair of brackets please, especially as the placement of
>>>> brackets affects the result of this particular calculation.
>>> Or simply eliminate one of the divisions using
>>> "1000000ull / APIC_BUS_CYCLE_NS".
>> I am totally confused.  I am happy to go with Jan's version.
>>
>> The confusion is that I get the same answer all the ways I try.
> Hmm - this ...
>
>>           ebx1 = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
>>           ebx2 = (1000000000ull / APIC_BUS_CYCLE_NS) / 1000ull;
>>           ebx3 = 1000000000ull / (APIC_BUS_CYCLE_NS * 1000ull);
> ... clearly indicates the contrary: You converted to mutiplication
> here, when the respective possibility of putting parentheses here
> would have been
>
>           ebx3 = 1000000000ull / (APIC_BUS_CYCLE_NS / 1000ull);
>
> which I'm sure you agree won't produce the same result. But yes,
> the language implies parentheses this way

Yes, I agree.  Since that is not mathematical correct either.  This has 
the same issue
as "a - b -c" is the same as "(a - b) - c" but not "a - (b - c)". 
However the "order" can be
changed.  "a / b / c" and "a / c / b" is the same.

>           ebx2 = (1000000000ull / APIC_BUS_CYCLE_NS) / 1000ull;
>
> so the original expression without them was correct, just
> slightly ambiguous.

Ok, I see that preventing mistakes here can only help.
     -Don Slutz

> Jan
>
>

Re: [PATCH v4 01/16] xen: Add support for VMware cpuid leaves

[Don Slutz]

On 09/11/14 15:49, Andrew Cooper wrote:
> On 11/09/2014 19:36, Don Slutz wrote:
>> This is done by adding 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
>>    Return VMware's cpuid leaves.
>>
>> The support of hypervisor cpuid leaves has not been agreed to.
>>
>> MicroSoft Hyper-V (AKA viridian) currently must be at 0x40000000.
>>
>> VMware currently must be at 0x40000000.
>>
>> KVM currently must be at 0x40000000 (from Seabios).
>>
>> Seabios will find xen at 0x40000000, 0x40000100, 0x40000200 ..
>> 0x40010000.
>>
>> http://download.microsoft.com/download/F/B/0/FB0D01A3-8E3A-4F5F-AA59-08C8026D3B8A/requirements-for-implementing-microsoft-hypervisor-interface.docx
>>
>> http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=displayKC&externalId=1009458
>>
>> http://lwn.net/Articles/301888/
>>    Attempted to get this cleaned up.
>>
>> So based on this, I picked the order:
>>
>> Xen at 0x40000000 or
>> Viridian or VMware at 0x40000000 and Xen at 0x40000100
>>
>> If both Viridian and VMware selected, report an error.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> ---
>>   xen/arch/x86/hvm/Makefile        |  3 +-
>>   xen/arch/x86/hvm/hvm.c           | 32 +++++++++++++++++++
>>   xen/arch/x86/hvm/vmware/Makefile |  1 +
>>   xen/arch/x86/hvm/vmware/cpuid.c  | 69 ++++++++++++++++++++++++++++++++++++++++
>>   xen/arch/x86/traps.c             |  8 +++--
>>   xen/include/asm-x86/hvm/hvm.h    |  3 ++
>>   xen/include/asm-x86/hvm/vmware.h | 31 ++++++++++++++++++
>>   xen/include/public/hvm/params.h  |  5 ++-
>>   8 files changed, 148 insertions(+), 4 deletions(-)
>>   create mode 100644 xen/arch/x86/hvm/vmware/Makefile
>>   create mode 100644 xen/arch/x86/hvm/vmware/cpuid.c
>>   create mode 100644 xen/include/asm-x86/hvm/vmware.h
>>
>> diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
>> index eea5555..77598a6 100644
>> --- a/xen/arch/x86/hvm/Makefile
>> +++ b/xen/arch/x86/hvm/Makefile
>> @@ -1,5 +1,6 @@
>>   subdir-y += svm
>>   subdir-y += vmx
>> +subdir-y += vmware
>>   
>>   obj-y += asid.o
>>   obj-y += emulate.o
>> @@ -22,4 +23,4 @@ obj-y += vlapic.o
>>   obj-y += vmsi.o
>>   obj-y += vpic.o
>>   obj-y += vpt.o
>> -obj-y += vpmu.o
>> \ No newline at end of file
>> +obj-y += vpmu.o
> This hunk is unrelated, but is perhaps something better fixed.  A
> passing note in the commit message perhaps?
>

Sure.

[snip -- different thread]


> +#include <xen/sched.h>
> +
> +#include <asm/hvm/hvm.h>
> +#include <asm/hvm/vmware.h>
> +
> +int cpuid_vmware_leaves(uint32_t idx, uint32_t *eax, uint32_t *ebx,
> +                        uint32_t *ecx, uint32_t *edx)
> +{
> +    struct domain *d = current->domain;
> +
> +    if ( !is_vmware_domain(d) )
> +        return 0;
> +
> +    switch ( idx - 0x40000000 )
> +    {
> +    case 0x0:
> Do these leaves have semantic names from a spec, which could live in an
> appropriate header file?

Not sure.

http://lwn.net/Articles/301888/

Which is from Oct 2008 -- Looks like VMware went with this and no one
else.

I can only find list postings that refer the the statement:

"
VMware hardware version 7 defines some of these cpuid levels, below is a 
brief
description about those. These levels can be implemented by other 
hypervisors
too so that Linux has a standard way of communicating to any hypervisor.

Leaf 0x40000000, Hypervisor CPUID information
# EAX: The maximum input value for hypervisor CPUID info (0x40000010).
# EBX, ECX, EDX: Hypervisor vendor ID signature. E.g. "VMwareVMware"

Leaf 0x40000010, Timing information.
# EAX: (Virtual) TSC frequency in kHz.
# EBX: (Virtual) Bus (local apic timer) frequency in kHz.
# ECX, EDX: RESERVED
"

There are a few about 0x40000010 as HYPERVISOR_TIMING_LEAF
or  HYPERVISOR_VMWARE_TIMING_LEAF.

All from late Sep 2008 (https://lkml.org/lkml/2008/9/29/45)

But I also found that I should be checking for >= 7:

The hypervisor present bit and hypervisor information leaf are only 
defined for products based on VMware hardware version 7.

So I will be changing this.



>> +    case 0x10:
>> +        /* (Virtual) TSC frequency in kHz. */
>> +        *eax =  d->arch.tsc_khz;
>> +        /* (Virtual) Bus (local apic timer) frequency in kHz. */
>> +        *ebx = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
> At least 1 pair of brackets please, especially as the placement of
> brackets affects the result of this particular calculation.

See other thread.

>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index 10fc2ca..f353f42 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -685,8 +685,12 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
>>                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
>>   {
>>       struct domain *d = current->domain;
>> -    /* Optionally shift out of the way of Viridian architectural leaves. */
>> -    uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
>> +    /*
>> +     * Optionally shift out of the way of Viridian or VMware
>> +     * architectural leaves.
>> +     */
>> +    uint32_t base = is_viridian_domain(d) | is_vmware_domain(d) ?
>> +        0x40000100 : 0x40000000;
> Again, brackets please for binary operators.  (We have had one recent
> slipup because of the precedence of | and ?:)

I do not think they are needed, but can add them.  A separate
thread also talks about this.


> Furthermore, I think you mean (is_viridian_domain(d) ||
> is_vmware_domain(d)) ? 0x40000100 : 0x40000000; which allows for short
> circuiting of is_vmware_domain().


Boris Ostrovsky siad the same.  I will switch to "||".


>>       uint32_t limit, dummy;
>>   
>>       idx -= base;
>> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
>> index 1123857..546210a 100644
>> --- a/xen/include/asm-x86/hvm/hvm.h
>> +++ b/xen/include/asm-x86/hvm/hvm.h
>> @@ -347,6 +347,9 @@ static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
>>   #define is_viridian_domain(_d)                                             \
>>    (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN]))
>>   
>> +#define is_vmware_domain(_d)                                             \
>> + (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW]))
>> +
>>   void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
>>                                  uint32_t *eax, uint32_t *ebx,
>>                                  uint32_t *ecx, uint32_t *edx);
>> diff --git a/xen/include/asm-x86/hvm/vmware.h b/xen/include/asm-x86/hvm/vmware.h
>> new file mode 100644
>> index 0000000..f254106
>> --- /dev/null
>> +++ b/xen/include/asm-x86/hvm/vmware.h
>> @@ -0,0 +1,31 @@
>> +/*
>> + * asm-x86/hvm/vmware.h
>> + *
>> + * Copyright (C) 2012 Verizon Corporation
>> + *
>> + * This file is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License Version 2 (GPLv2)
>> + * as published by the Free Software Foundation.
>> + *
>> + * This file is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details. <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef ASM_X86_HVM_VMWARE_H__
>> +#define ASM_X86_HVM_VMWARE_H__
> #include <xen/types.h> (IIRC) please, or uint32_t could cause a
> compilation failure given specific orderings of #include's in
> translation units.

I am fine with adding this.

     -Don Slutz

> ~Andrew
>
>

Re: [PATCH v4 01/16] xen: Add support for VMware cpuid leaves

[Boris Ostrovsky]

On 09/11/2014 02:36 PM, Don Slutz wrote:
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 10fc2ca..f353f42 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -685,8 +685,12 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
>                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
>   {
>       struct domain *d = current->domain;
> -    /* Optionally shift out of the way of Viridian architectural leaves. */
> -    uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
> +    /*
> +     * Optionally shift out of the way of Viridian or VMware
> +     * architectural leaves.
> +     */
> +    uint32_t base = is_viridian_domain(d) | is_vmware_domain(d) ?

Given how is_viridian and is_vmware are defined I think '||' is more 
appropriate.

-boris

> +        0x40000100 : 0x40000000;
>       uint32_t limit, dummy;
>   
>       idx -= base;
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 1123857..546210a 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -347,6 +347,9 @@ static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
>   #define is_viridian_domain(_d)                                             \
>    (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN]))
>   
> +#define is_vmware_domain(_d)                                             \
> + (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VMWARE_HW]))
> +
>

Re: [PATCH v4 01/16] xen: Add support for VMware cpuid leaves

[Don Slutz]

On 09/12/14 08:37, Boris Ostrovsky wrote:
> On 09/11/2014 02:36 PM, Don Slutz wrote:
>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index 10fc2ca..f353f42 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -685,8 +685,12 @@ int cpuid_hypervisor_leaves( uint32_t idx, 
>> uint32_t sub_idx,
>>                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, 
>> uint32_t *edx)
>>   {
>>       struct domain *d = current->domain;
>> -    /* Optionally shift out of the way of Viridian architectural 
>> leaves. */
>> -    uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
>> +    /*
>> +     * Optionally shift out of the way of Viridian or VMware
>> +     * architectural leaves.
>> +     */
>> +    uint32_t base = is_viridian_domain(d) | is_vmware_domain(d) ?
>
> Given how is_viridian and is_vmware are defined I think '||' is more 
> appropriate.
>

I plan to convert to "||".
    -Don Slutz

> -boris
>
>>
>