Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Xen-devel] [XEN PATCH v14 0/8] Xen VMware tools support
@ 2020-08-19 16:51 Don Slutz
  2020-08-19 16:51 ` [Xen-devel] [XEN PATCH v14 1/8] tools: Add vga=vmware Don Slutz
  0 siblings, 1 reply; 16+ messages in thread
From: Don Slutz @ 2020-08-19 16:51 UTC (permalink / raw)
  To: xen-devel
  Cc: Aravind Gopalakrishnan, Boris Ostrovsky, Eddie Dong,
	Ian Campbell, Ian Jackson, Jan Beulich, Jun Nakajima,
	Keir Fraser, Kevin Tian, Stefano Stabellini,
	Suravee Suthikulpanit, Tim Deegan, Andrew Cooper,
	Konrad Rzeszutek Wilk, George Dunlap, Don Slutz

From: Don Slutz <don.slutz@gmail.com>

https://lists.xenproject.org/archives/html/xen-devel/2016-01/msg01105.html

Is the last message on the list.  I do not have this message to directly
reply to.  I plan to try and continue this as part of this version.

Changes v13 to v14:
  Rebased on staging (not a simple rebase, needed rework to function
  with changes).

  Since over 4 years have passed, moved all Acked-by, Reviewed-by
  to under the ---

  Jan Beulich (#2 xen: Add support for VMware cpuid)
    why this needs to be a 64-bit quantity?
      changed to 32 bit.

  Jan Beulich (#5 xen: Add vmware_port support)
    why you couldn't just use another XEN_X86_EMU_ ...
      Drop arch_flags and use XEN_X86_EMU_VMWARE_PORT which will not be
      added to "all".

  Paul Durrant (#7 Add IOREQ_TYPE_VMWARE_PORT)
    Reworked the "Verify the emulation request has been correctly re-issued"
    to have a pre if.
    Made a single statement.
    Code no longer exists.  But, there is a way to not have it.
    See HVM_IOREQSRV_DISABLE_VMPORT

  Jan Beulich (#7 Add IOREQ_TYPE_VMWARE_PORT)
    Was left off in the middle
      I think haveing the current state of the code posted will help.

    don't think having something prefixed vmware_ in the Xen public
    headers is a good idea.
      Also did not reach a clear resolution.
        Last statement:
          All of this may be true and fine, but none of this is - to me - a
          reason to introduce new unclean names into the Xen public
          interface. In no event do I see qemu dictating naming to us.
        My current answer:
          My take on this is that you want me to use a different name,
          is vmport_regs a name you can live with?

    Also throughout the series I didn't find any code addition to
    guarantee (perhaps at build time) that BDOOR_PORT doesn't
    collide with any other use ports 
      Added BUILD_BUG_ON for all that can be checked at build time,
      which I think answers this.

    Long line needs breaking up.
      Fixed.
    The parameter should no longer be named "buf".
      Changed to pt.
    Do you really need to set up a (dummy) range set for this
      Last version did not finsh on this.  Here is what I think now:
      hvm_select_ioreq_server() uses this "dummy" range to see if a given
      IOREQ Server should be picked.  XEN_DMOP_map_io_range_to_ioreq_server
      and XEN_DMOP_unmap_io_range_from_ioreq_server can be used to enable
      or disable vmport usage at any time.      

    Several comments on vmport_check_port
      All changed.
    Lower case please (and ...
      Changed to lower case in the enum.
    But accessing BDOOR_PORT + 1 with a 4-byte operation ought to
    be undefined (or properly split up). After all you don't know what is
    on BDOOR_PORT + 4.
      Changed to only allow BDOOR_PORT with a 4-byte operation.

  --Fixed bug in hvm_alloc_legacy_ioreq_gfn: the if is always false.

  (#8 Add xentrace to vmware_port)
    Added VMPORT_SEND because I wanted to see it during testing.


Changes v12 to v13:
  Rebased on staging (not a simple rebase, needed rework to function
  with changes).

  Ian Campbell (#1: tools: Add vga=vmware):
    Added Acked-by.

  Konrad Rzeszutek Wilk (#2: xen: Add support for VMware cpuid leaves):
    Adjust temporary comment to include which patch.
    Adjust copyright year to 2012-2015

  Ian Campbell (#2: xen: Add support for VMware cpuid leaves):
    Added Tools side only: Acked-by

  Konrad Rzeszutek Wilk (#3: tools: Add vmware_hwver support):
    Perhaps add 'Recommended value is 7' ?
      Ian Campbell: Assuming that is a good recommendation then that might
                    be a good enhancement for a future patch. Or
                    perhaps some sort of simple mapping from VMware
                    product versions/names to a suitable value to
                    use, unless that means having to think about
                    trademarks etc.
       So did no change here.

  Ian Campbell (#3: tools: Add vmware_hwver support):
    Added Acked-by.

  Ian Campbell (#6: tools: Add vmware_port support):
    Added Acked-by.

  Konrad Rzeszutek Wilk (#7: Add IOREQ_TYPE_VMWARE_PORT):
    I would have made this !vmport_check_port ...
       Changed to !is_vmware, and invert vmport_check_port's return value.
    That would mean you could also change the function to be 'is_port_vmport' or
    such.
       Did not change at this time.
    Comments on code that no longer exists do to re-work, just
    dropped.
    Use 1 one for "list_for_each_entry ( sv, ..."
      Done.
    Full stop missing in comments.
      Done.

  Andrew Cooper (#8: Add xentrace to vmware_port):
    Please do this by extending the existing infrastructure rather
    than special-casing 7 on the side.  (i.e. extend ND to take 7
    parameters, and introduce HVMTRACE_7D)
      Done.
    = { d1, d2, d3, d4, d5, d6, d7 } will be far shorter, linewise.
      Done.


Changes v11 to v12:
  Rebased on staging.

  Jan Beulich (xen: Add ring 3 vmware_port support)
    As there don't seem to be enough convincing arguments for this
    to be worthwhile, I'm going to drop this...
      Dropped the 2 ring3 patches.

  Ian Campbell (tools: Add vmware_hwver support):
    s/come/comes/
      Done
    I'm not sure this hunk has anything to do with this patch, nor
    what the semantic difference between the old and new text is
    supposed to be.
      Dropped comment change.

  Ian Campbell (multiple):
    In v11 this seems to have morphed into only
    LIBXL_HAVE_LIBXL_VGA_INTERFACE_TYPE_VMWARE being provided, which
    is clearly not an appropriate umbrella #define.

    "#define LIBXL_HAVE_CREATEINFO_VMWARE 1"
    Lets just have a single one of these indicating support for
    vmware, it should be added at the end of the series after all
    the baseline vmware functionality is in place. I think that
    means hwver, vga=vmware and this port stuff.

      Make (tools: Add vga=vmware) no longer independent.
      Change the #define to "LIBXL_HAVE_VMWARE"

   Jan Beulich (xen: Add vmware_port support):
     Surrounding code avoiding the use of "break" makes the result
     look rather inconsistent. Please move this up immediately after
     the XSM check, or drop the "break".
       Moved it up.

   George Dunlap (Add IOREQ_TYPE_VMWARE_PORT):
     Sorry for coming a bit late to this party.  On a high level I
     think this is good, but there doesn't seem to be anything in
     here in particular that is vmware-specific.  Would it make more
     sense to give this a more generic name, and have it include all
     of the general-purpose registers?
       Did not change at this time because QEMU would need changes 1st.

   George Dunlap (Add xentrace to vmware_port):
     Do you need to log edi as well? It looks like it's not used.
       I think it makes sense to indicate that edi can be used and
       may in the future.
     And do you need to log all the registers here?  It seems like
     port + regs->_ecx would be enough to tell you why it got
     ignored.
        Adjusted to log just port and eax (ecx was NOT tested for).


Changes v10 to v11:

  Andrew Cooper & Ian Campbell (#1 "tools: Add vga=vmware"):
    Nack. Qemu-trad is currently has remote code execution vulnerabilities.
      Dropped support for Qemu-trad.
    
     Also changed later patchs to not need this one.

  Andrew Cooper (#2 "xen: Add support for VMware cpuid leaves"):
    Needs re-base.
      Done
    Adjust /* Disallow if vmware_hwver */
      Done
    Newline after break;
      Done 2 places.
    Allowed Reviewed-by: Andrew Cooper, if these changes are done.
      Added Reviewed-by: Andrew Cooper.

   Julien Grall (#2 "xen: Add support for VMware cpuid leaves"):
    It would be worth to add an explicit vmware_hwver = 0 in the
    libxl__arch_domain_prepare_config.
      Done -- Note: Adds a tool change to this patch.

  (#3 "tools: Add vmware_hwver support"):
    Since Qemu-trad does not support vga=vmware,
    Dropped "If non-zero then default VGA to VMware's VGA"

  Andrew Cooper (#5 "xen: Add vmware_port support"):
    You will not be getting here for a non HVM domain...
      Dropped ASSERT(is_hvm_domain(currd))
    Newline after break;
      Done 6 places.
    Allowed Reviewed-by: Andrew Cooper, if these changes are done.
      Added Reviewed-by: Andrew Cooper.

  (#7 "tools: Add vmware_port support"):
    Since Qemu-trad does not support vga=vmware,
    Dropped "If non-zero then default VGA to VMware's VGA"

Changes v9 to v10:
  Split out LIBXL_VGA_INTERFACE_TYPE_VMWARE into it's own patch (#1)
  that can stand alone.  In the patch set because a later patch
  depends on it.

  Reworked to be based on:

    commit a7511905fae7ba592c5bf63cd77d8ff78087d689
    Author: Julien Grall <julien.grall@linaro.org>
    Date:   Wed Apr 1 17:21:41 2015 +0100

        xen: Extend DOMCTL createdomain to support arch configuration

  rebased onto:

    commit e13013dbf1d5997915548a3b5f1c39594d8c1d7b
    Author: Yang Hongyang <yanghy@cn.fujitsu.com>
    Date:   Thu May 14 16:55:18 2015 +0800

        libxc/restore: add checkpointed flag to the restore context


  Andrew Cooper (#2: "xen: Add support for VMware cpuid leaves"):
    Did not add "Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>"
    because of changes here to do things the new way.
  Reword comment message to reflect new way.

  Ian Campbell (#3 "tools: Add vmware_hwver support"):
    LIBXL_HAVE_LIBXL_VGA_INTERFACE_TYPE_VMWARE &
    LIBXL_HAVE_BUILDINFO_HVM_VMWARE_HWVER are arriving together
    a single umbrella could be used.
      Since I split the LIBXL_VGA_INTERFACE_TYPE_VMWARE into
      it's own patch, this is not longer true.
      But I did use 1 for the 2 c_info changes.
    Please use GCSPRINTF.
      Done.
  Remove vga=vmware from here.

  Ian Campbell (#3 "tools: Add vmware_hwver support"):
    For "Add IOREQ_TYPE_VMWARE_PORT"
      With those fixed the tools/* bits are:
        Acked-by: Ian Campbell <ian.campbell@citrix.com>  
    Did not add Acked-by to "tools: Add vmware_hwver support"
    because of the rework for using libxl_domain_create_info.

  Andrew Cooper (#4: "vmware: Add VMware provided include file."):
    Added "Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>"

  Andrew Cooper (#5 "xen: Add vmware_port support"):
    Probably better as EOPNOTSUPP, as it is a configuration problem.
      Done.
    vmport_ioport function looks as if it should be static.
      Done.
    Why is GETHZ the only one of these with a CPL check?
      Please see thread for detail.
    I would suggest putting vmport_register declaration in hvm.h ...
      Done.

  Jan Beulich (#5 "xen: Add vmware_port support"):
    As indicated before, I don't think this is a good use case for a
    domain creation flag.
      Switch to the new config way.
    struct domain *d => struct domain *currd
      Done
    Are you sure you don't want to zero the high halves of 64-bit ...
      Comment added.
   Then just have this handled into the default case.
      Reworked new_eax handling.
   is_hvm_domain(currd)
   And - why here rather than before the switch() or even right at the
   start of the function?
      Moved to start.
   With that, is it really correct that OUT updates the other registers
   just like IN? If so, this deserves a comment, so that readers won't
   think this is in error.
     All done in comment at start.

  Andrew Cooper (#6 "xen: Add ring 3 vmware_port support"):
    >> This looks horribly invasive.
    >>
    >> Why are emulation changes needed?  What is wrong with the normal
    >> handling with a registered ioport handler?
    > Because VMware made a bad way to provide a "hyper call".  They decided to
    > allow user access to this.  So when a #GP fault should have been
    > reported, they instead do the "hyper call".
    >
    Urgh - now I remember.

    Right.  In the case that vmport is active, we start intercepting #GP
    faults and emulating access.  That part of the patch looks ok.

    However, the rest is very invasive to the emulation infrastructure.
      Re-worked along this lines suggested.

  Jan Beulich (#6 "xen: Add ring 3 vmware_port support"):
    I hope that vmport_check will no longer be needed with the adjustments ...
    > Since this is not an architecture feature and I do not expect any real
    > CPUs to support this, I do not expect any other use.  But I am happy
    > to make it more generic.

    Let's see how this ends up looking - the hook is probably indeed
    bogus (from an architectural pov) no matter how you name it.
      Last e-mail on thread, so no change.

  Ian Campbell (#7 "tools: Add vmware_port support"):
    If..." at the start of the sentence ...
      Used Ian's reword.
    Also, why is 7 special?
      Attempted to better explain.

  Paul Durrant & Jan Beulich (#8 "Add IOREQ_TYPE_VMWARE_PORT"):
    Now that buf is no longer a bool, could ...
    These literals should become an enum
      Added an enum.
    I don't think the invalidate type is needed.
      Dropped.
    IOREQ_TYPE_VMWARE_PORT as 3 is a re-use.
      Switch to 9.
    Code handling "case X86EMUL_UNHANDLEABLE:" in emulate.c
    is unclear.
       Re-worked to a version that Jan likes better.
    Comment about "special' range of 1" is not clear.
       Re-worded comments.

  Ian Campbell (#9 "Add xentrace to vmware_port"):
    Acked-by
  Readded dropped traces.

  Jan Beulich & Andrew Cooper (#9 "Add xentrace to vmware_port"):
    Why is cmd in this patch?
      Because the trace points use it.

  Jan Beulich (#10 "test_x86_emulator.c: Add tests for #GP usage"):
    Need more comments and simpler error checking.
      Done.  
      Dropped un-needed new routines.

  Andrew Cooper:
    That is because you broke it adding a bool_t item.
      Has now been dropped.


Changes v8 to v9:
  Overview of changes:
    s/vmware_hw/vmware_hwver/i
    Switch to x86_emulator to handle #GP
    New patch: Move MAX_INST_LEN into x86_emulate.h
    Add QEMU usage, patch #8 "Add IOREQ_TYPE_VMWARE_PORT"
    Split patch "xen: Add vmware_port support" into 2. 1st has same
    name.  New one is "xen: Add ring 3 vmware_port support".
    Added 3 new patches about test_x86_emulator.

  
  Jan Beulich (#2: "xen: Add support for VMware cpuid leaves"):
    Change -EXDEV to EOPNOTSUPP.
      Done.
    adding another subdirectory: xen/arch/x86/hvm/vmware
    Much will depend on the discussion of the subsequent patches.
      TBD.
    So for versions < 7 there's effectively no CPUID support at all?
      Changed to check at entry.
    The comment /* Params for VMware */ seems wrong...
      Changed to /* emulated VMware Hardware Version */
    Also please use d, not _d in #define is_vmware_domain()
      Changed.  Line is now > 80 characters, so split into 2.

  Andrew Cooper (#3: "tools: Add vmware_hwver support"):
      I assumed that s/vmware_hw/vmware_hwver/ is not a big enough
      change to drop the Reviewed-by.  Did a minor edit to the
      commit message to add 7 to the list of values checked.

  Jan Beulich (#4: "vmware: Add VMware provided include file"):
    Either the description is wrong, or the patch is stale.
      stale commit message -- fixed.
    I'd say a file with a single comment line in it would suffice.
      Done.

  Jan Beulich (#5: "xen: Add vmware_port support"):
    Can you explain why a HVM param isn't suitable here?
      Issue with changing QEMU on the fly.
      Andrew Cooper: My recommendation is still to use a creation flag
        So no change.
    Please move SVM's identical definition into ...
      Did this as #1.  No longer needed, but since the patch was ready
      I have included it.
    --Lots of questions about code that no long is part of this patch. --
    With this, is handling other than 32-bit in/out really
    meaningful/correct?
      Added comment about this.
    Since you can't get here for PV, I can't see what you need this.
      Changed to an ASSERT.
    Why version 4?
      Added comment about this.
    -- Several questions about register changes.
      Re-coded to use new_eax and set *val to this.
      Change to generealy use reg->_e..
    These ei1/ei2 checks belong in the callers imo -
      Moved.
    the "port" function parameter isn't even checked
      Add check for exact match.
    If dropping the code is safe without also forbidding the
    combination of nested and VMware emulation.
      Added the forbidding the combination of nested and VMware.
      Mostly do to the cases of the nested virtual code is the one
      to handle VMware stuff if needed, not the root one.  Also I am
      having issues testing xen nested in xen and using hvm.

      

Changes v7 to v8:

  Jan Beulich:
    Coding changes to vmport_ioport. Things like:
-             regs->rax = (uint32_t)~0ul;
+             regs->_eax = ~0u;
      
  Andrew Cooper (#2: "tools: Add vmware_hwver support"):
    Other than these two comments, the rest of the patch looks ok, so...
      Added Reviewed-by after addressing the "Spurious whitepsace change".
      and the wording in the new docs/misc/hypervisor-cpuid.markdown.


Changes v6 to v7:
  summary of changes.

  George Dunlap:
    Any doc about this?
      Added reference to:
        https://sites.google.com/site/chitchatvmback/backdoor
      Last updated: Feb. 2008

  George Dunlap & Jan Beulich
    Too much logging and tracing.
      Dropped a lot of it.  This includes vmport_debug=

  Ian Campbell:
    Any reason RPC code cannot be done in QEMU?
      Not that I know of, so dropped all parts of RPC code.
    Default handling of hvm.vga.kind bad.
      Fixed.
    Default of vmware_port should be based on vmware_hw.
      Done. 

  Tim Deegan:
    CPL check of GETHZ needs to be fixed somewhere.
      Added check for CPL == 0 (assuming this is what VMware is
      checking.  Matches the testing.

  Ian Campbell, Andrew Cooper, George Dunlap, Boris Ostrovsky,
   & Jan Beulich
     Various minor fixes.
    
  Per patch notes:
    #1 "xen: Add support for VMware cpuid leaves":
      Prevent setting of HVM_PARAM_VIRIDIAN if HVM_PARAM_VMWARE_HW set.
    #4 "xen: Add vmware_port support":
      More on AMD in the commit message.
      Switch to only change 32bit part of registers, what VMware
        does.
    #6 "Add xentrace to vmware_port":
      Dropped some of the new traces.
      Added HVMTRACE_ND7.
    #7 "Add xen-hvm-param":
       Was a later patch.  Still optional.
       Fixed formatting.
       Adjust for drop of VMware RPC.

Comments on v3, v4, v5, v6:
  George Dunlap:
    Is there any reason not to merge 05/16 with 03/16?
      The reason I have is that v3 03/16 only contains new files. 2
      from VMware and 1 to allow use of the VMware files.  I added
      xen/arch/x86/hvm/vmware/includeCheck.h at the request of
      Konrad Wilk.

      This patch has many style issues and white space issues.  So I
      want it as a separate patch so as to be clear on what files do
      not meet the coding style.  And why and where they came from.

Changes v5 to v6:
  Boris Ostrovsky & Jan Beulich
    #4 "xen: Add vmware_port support":
    #6 "xen: Convert vmware_port to xentrace usage":
    There is an issue with reading instruction bytes more then once.
      Dropped the attempt to use svm_nextrip_insn_length via
      __get_instruction_length (added in v2).  Just always look
      at upto 15 bytes on AMD.

Changes v4 to v5:
  Re tagged the optional patches.

  Added debug=y build checking that vmx is defining
  VM_EXIT_INTR_ERROR_CODE.

  Boris Ostrovsky:
    #1 "xen: Add support for VMware cpuid leaves":
      Given how is_viridian and is_vmware are defined I think '||' is more
      appropriate.
        Fixed.
    #4 "xen: Add is_vmware_port_enabled":
      we should make sure that svm_vmexit_gp_intercept is not executed for
      any other guest.
        Added an ASSERT on is_vmware_port_enabled.
      magic integers?
        Added #define for them.
    #6 "xen: Convert vmware_port to xentrace usage":
      exitinfo1 is used twice.
        Fixed.
    #7 "tools: Convert vmware_port to xentrace usage":
      'bytes = 0x%(2)d' or 'bytes = %(2)d' ?
        Fixed.
    #8 "xen: Add limited support of VMware's hyper-call rpc":
      PV vs. HVM vs. PVH. So probably 'if(is_hvm_vcpu)'?
        I see no reason to exclude PVH.   Will change to has_hvm_container_vcpu
    #11 "Add live migration of VMware's hyper-call":
      You ASSERTed that vg->key_len is 1 so you may not need the 'if'.
        That is a ASSERT(sizeof, not just ASSERT -- not changed.
      Use real errno, not -1.
        Fixed.
      No ASSERT in vmport_load_domain_ctxt
        Added.

  Jan Beulich & Boris Ostrovsky:
    #8 "xen: Add limited support of VMware's hyper-call rpc":
      The names of all three functions are bogus.
        removed static support routines.
        Also changed in #1.

  Andrew Cooper:
    #2 "tools: Add vmware_hw support":
      Anything looking for Xen according to the Xen cpuid instructions...
        Adjusted doc to new wording.
    #4 "xen: Add is_vmware_port_enabled":
      I am fairly certain that you need some brackets here.
        Added brackets.

  Jan Beulich & Andrew Cooper:
    #1 "xen: Add support for VMware cpuid leaves":
      This hunk is unrelated, but is perhaps something better fixed.
        Added to commit message.
      include <xen/types.h> (IIRC) please.
        Done.
      At least 1 pair of brackets please, especially as the placement of
      brackets affects the result of this particular calculation.
        Switch to "1000000ull / APIC_BUS_CYCLE_NS"      


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 (8):
  tools: Add vga=vmware
  xen: Add support for VMware cpuid leaves
  tools: Add vmware_hwver support
  vmware: Add VMware provided include file.
  xen: Add vmware_port support
  tools: Add vmware_port support
  Add IOREQ_TYPE_VMWARE_PORT
  Add xentrace to vmware_port

 docs/man/xl.cfg.5.pod.in               |  36 +++-
 tools/libs/devicemodel/core.c          |   4 +-
 tools/libxc/include/xc_dom.h           |   2 +-
 tools/libxc/xc_dom_x86.c               |   3 +
 tools/libxl/libxl.h                    |   5 +
 tools/libxl/libxl_create.c             |  14 +-
 tools/libxl/libxl_dm.c                 |  11 ++
 tools/libxl/libxl_types.idl            |   3 +
 tools/libxl/libxl_x86.c                |   3 +
 tools/xentrace/formats                 |   6 +
 tools/xl/xl_parse.c                    |   6 +
 xen/arch/x86/cpuid.c                   |   4 +
 xen/arch/x86/domain.c                  |  20 +-
 xen/arch/x86/hvm/Makefile              |   1 +
 xen/arch/x86/hvm/dm.c                  |   3 +-
 xen/arch/x86/hvm/emulate.c             |  57 +++++-
 xen/arch/x86/hvm/hvm.c                 |  21 ++
 xen/arch/x86/hvm/ioreq.c               | 351 ++++++++++++++++++++++++++++-----
 xen/arch/x86/hvm/svm/svm.c             |   6 +-
 xen/arch/x86/hvm/vmware/Makefile       |   2 +
 xen/arch/x86/hvm/vmware/backdoor_def.h | 167 ++++++++++++++++
 xen/arch/x86/hvm/vmware/includeCheck.h |   1 +
 xen/arch/x86/hvm/vmware/vmport.c       | 194 ++++++++++++++++++
 xen/arch/x86/hvm/vmware/vmware.c       |  82 ++++++++
 xen/arch/x86/hvm/vmx/vmx.c             |   6 +-
 xen/arch/x86/traps.c                   |  21 +-
 xen/include/asm-x86/hvm/domain.h       |  10 +-
 xen/include/asm-x86/hvm/hvm.h          |  18 ++
 xen/include/asm-x86/hvm/trace.h        |  60 +++---
 xen/include/asm-x86/hvm/vmware.h       |  33 ++++
 xen/include/public/arch-x86/xen.h      |   5 +
 xen/include/public/hvm/dm_op.h         |  21 +-
 xen/include/public/hvm/ioreq.h         |  17 ++
 xen/include/public/hvm/params.h        |   2 +-
 xen/include/public/trace.h             |   4 +
 35 files changed, 1088 insertions(+), 111 deletions(-)
 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/includeCheck.h
 create mode 100644 xen/arch/x86/hvm/vmware/vmport.c
 create mode 100644 xen/arch/x86/hvm/vmware/vmware.c
 create mode 100644 xen/include/asm-x86/hvm/vmware.h

-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Xen-devel] [XEN PATCH v14 1/8] tools: Add vga=vmware
  2020-08-19 16:51 [Xen-devel] [XEN PATCH v14 0/8] Xen VMware tools support Don Slutz
@ 2020-08-19 16:51 ` Don Slutz
  2020-08-19 16:51   ` [Xen-devel] [XEN PATCH v14 2/8] xen: Add support for VMware cpuid leaves Don Slutz
  0 siblings, 1 reply; 16+ messages in thread
From: Don Slutz @ 2020-08-19 16:51 UTC (permalink / raw)
  To: xen-devel
  Cc: Aravind Gopalakrishnan, Boris Ostrovsky, Eddie Dong,
	Ian Campbell, Ian Jackson, Jan Beulich, Jun Nakajima,
	Keir Fraser, Kevin Tian, Stefano Stabellini,
	Suravee Suthikulpanit, Tim Deegan, Andrew Cooper,
	Konrad Rzeszutek Wilk, George Dunlap, Don Slutz, Don Slutz

From: Don Slutz <dslutz@verizon.com>

This allows use of QEMU's VMware emulated video card

NOTE: vga=vmware is not supported by device_model_version=qemu-xen-traditional

Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <don.slutz@gmail.com>
---
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

v14:
  Reworked to current code.
  Added below --- Reviewed-by: Konrad Rzeszutek 

v13:
  Added Acked-by: Ian Campbell

v12:
  Dropped LIBXL_HAVE_LIBXL_VGA_INTERFACE_TYPE_VMWARE
  This means that the later patch that defines LIBXL_HAVE_VMWARE
  is now also required.

v11:
  Dropped support for Qemu-trad.
  Also changed later patchs to not need this one.

v10: New at v10.

  Was part of "tools: Add vmware_hwver support"


 docs/man/xl.cfg.5.pod.in    | 4 +++-
 tools/libxl/libxl_dm.c      | 9 +++++++++
 tools/libxl/libxl_types.idl | 1 +
 tools/xl/xl_parse.c         | 2 ++
 4 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 0532739..a9eae96 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2396,7 +2396,7 @@ B<This option is deprecated, use vga="stdvga" instead>.
 =item B<vga="STRING">
 
 Selects the emulated video card.
-Options are: B<none>, B<stdvga>, B<cirrus> and B<qxl>.
+Options are: B<none>, B<stdvga>, B<cirrus>, B<vmware> and B<qxl>.
 The default is B<cirrus>.
 
 In general, QXL should work with the Spice remote display protocol
@@ -2404,6 +2404,8 @@ for acceleration, and a QXL driver is necessary in the guest in that case.
 QXL can also work with the VNC protocol, but it will be like a standard
 VGA card without acceleration.
 
+NOTE: B<vmware> is not supported on B<device_model_version = "qemu-xen-traditional">
+
 =item B<vnc=BOOLEAN>
 
 Allow access to the display via the VNC protocol.  This enables the
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index f2dc569..415c12e 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -808,6 +808,10 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
         case LIBXL_VGA_INTERFACE_TYPE_NONE:
             flexarray_append_pair(dm_args, "-vga", "none");
             break;
+        case LIBXL_VGA_INTERFACE_TYPE_VMWARE:
+            LOG(ERROR, "vga=vmware is not supported by "
+                "qemu-xen-traditional");
+            return ERROR_INVAL;
         case LIBXL_VGA_INTERFACE_TYPE_QXL:
             break;
         default:
@@ -1446,6 +1450,11 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                 GCSPRINTF("qxl-vga,vram_size_mb=%"PRIu64",ram_size_mb=%"PRIu64,
                 (b_info->video_memkb/2/1024), (b_info->video_memkb/2/1024) ) );
             break;
+        case LIBXL_VGA_INTERFACE_TYPE_VMWARE:
+            flexarray_append_pair(dm_args, "-device",
+                GCSPRINTF("vmware-svga,vgamem_mb=%d",
+                libxl__sizekb_to_mb(b_info->video_memkb)));
+            break;
         default:
             LOGD(ERROR, guest_domid, "Invalid emulated video card specified");
             return ERROR_INVAL;
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 9d3f05f..36350d2 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -220,6 +220,7 @@ libxl_vga_interface_type = Enumeration("vga_interface_type", [
     (2, "STD"),
     (3, "NONE"),
     (4, "QXL"),
+    (5, "VMWARE"),
     ], init_val = "LIBXL_VGA_INTERFACE_TYPE_UNKNOWN")
 
 libxl_vendor_device = Enumeration("vendor_device", [
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 61b4ef7..c74a9e3 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2590,6 +2590,8 @@ skip_usbdev:
                 b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_NONE;
             } else if (!strcmp(buf, "qxl")) {
                 b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_QXL;
+            } 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);
-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Xen-devel] [XEN PATCH v14 2/8] xen: Add support for VMware cpuid leaves
  2020-08-19 16:51 ` [Xen-devel] [XEN PATCH v14 1/8] tools: Add vga=vmware Don Slutz
@ 2020-08-19 16:51   ` Don Slutz
  2020-08-19 16:51     ` [Xen-devel] [XEN PATCH v14 3/8] tools: Add vmware_hwver support Don Slutz
  2020-09-30 14:24     ` [XEN PATCH v14 2/8] xen: Add support for VMware cpuid leaves Jan Beulich
  0 siblings, 2 replies; 16+ messages in thread
From: Don Slutz @ 2020-08-19 16:51 UTC (permalink / raw)
  To: xen-devel
  Cc: Aravind Gopalakrishnan, Boris Ostrovsky, Eddie Dong,
	Ian Campbell, Ian Jackson, Jan Beulich, Jun Nakajima,
	Keir Fraser, Kevin Tian, Stefano Stabellini,
	Suravee Suthikulpanit, Tim Deegan, Andrew Cooper,
	Konrad Rzeszutek Wilk, George Dunlap, Don Slutz, Don Slutz

From: Don Slutz <dslutz@verizon.com>

This is done by adding xen_arch_domainconfig 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 or >= 7.

If non-zero then
  Return VMware's cpuid leaves.  If >= 7 return data, else
  return 0.

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

Xen can be found at the first otherwise unused 0x100 aligned
offset between 0x40000000 and 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.

Since I need to change xen/arch/x86/hvm/Makefile; also add
a newline at end of file.

Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <don.slutz@gmail.com>
---
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tools side only: Acked-by: Ian Campbell <ian.campbell@citrix.com>

v14:
  Reworked to current code.
  Changed vmware_hwver to 32 bits

v13:
  Adjust temporary comment to include which patch.
  Adjust copyright year to 2012-2015

v12:
  No change

v11:
  Adjust /* Disallow if vmware_hwver */
  Newline after break;
  Added Reviewed-by: Andrew Cooper.
    It would be worth to add an explicit vmware_hwver = 0 in the
    libxl__arch_domain_prepare_config.
 Note: Adds a tool change to this patch.

v10:
    Did not add "Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>"
    because of changes here to do things the new way.
  Reword comment message to reflect new way.

v9:
    s/vmware_hw/vmware_hwver/i
    Change -EXDEV to EOPNOTSUPP.
      Done.
    adding another subdirectory: xen/arch/x86/hvm/vmware
    Much will depend on the discussion of the subsequent patches.
      TBD.
    So for versions < 7 there's effectively no CPUID support at all?
      Changed to check at entry.
    The comment /* Params for VMware */ seems wrong...
      Changed to /* emulated VMware Hardware Version */
    Also please use d, not _d in #define is_vmware_domain()
      Changed.  Line is now > 80 characters, so split into 2.

v7:
      Prevent setting of HVM_PARAM_VIRIDIAN if HVM_PARAM_VMWARE_HW set.
v5:
      Given how is_viridian and is_vmware are defined I think '||' is more
      appropriate.
        Fixed.
      The names of all three functions are bogus.
        removed static support routines.
      This hunk is unrelated, but is perhaps something better fixed.
        Added to commit message.
      include <xen/types.h> (IIRC) please.
        Done.
      At least 1 pair of brackets please, especially as the placement of
      brackets affects the result of this particular calculation.
        Switch to "1000000ull / APIC_BUS_CYCLE_NS"      

 tools/libxl/libxl_x86.c           |  2 +
 xen/arch/x86/cpuid.c              |  4 ++
 xen/arch/x86/domain.c             |  5 +++
 xen/arch/x86/hvm/Makefile         |  1 +
 xen/arch/x86/hvm/hvm.c            |  8 ++++
 xen/arch/x86/hvm/vmware/Makefile  |  1 +
 xen/arch/x86/hvm/vmware/vmware.c  | 82 +++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/traps.c              | 21 ++++++----
 xen/include/asm-x86/hvm/domain.h  |  3 ++
 xen/include/asm-x86/hvm/hvm.h     | 15 +++++++
 xen/include/asm-x86/hvm/vmware.h  | 33 ++++++++++++++++
 xen/include/public/arch-x86/xen.h |  1 +
 12 files changed, 169 insertions(+), 7 deletions(-)
 create mode 100644 xen/arch/x86/hvm/vmware/Makefile
 create mode 100644 xen/arch/x86/hvm/vmware/vmware.c
 create mode 100644 xen/include/asm-x86/hvm/vmware.h

diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index e57f632..3418a81 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -21,6 +21,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
         abort();
     }
 
+    /* Note: will be changed in next patch (tools: Add ...). */
+    config->arch.vmware_hwver = 0;
     return 0;
 }
 
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 63a03ef..6cd3670 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -8,6 +8,7 @@
 #include <asm/hvm/nestedhvm.h>
 #include <asm/hvm/svm/svm.h>
 #include <asm/hvm/viridian.h>
+#include <asm/hvm/vmware.h>
 #include <asm/hvm/vmx/vmcs.h>
 #include <asm/paging.h>
 #include <asm/processor.h>
@@ -791,6 +792,9 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
         if ( is_viridian_domain(d) )
             return cpuid_viridian_leaves(v, leaf, subleaf, res);
 
+        if ( has_vmware_cpuid(d) )
+            return cpuid_vmware_leaves(v, leaf, subleaf, res);
+
         /*
          * Fallthrough.
          *
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index d1ecc7b..a317907 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -597,6 +597,11 @@ int arch_domain_create(struct domain *d,
     }
     d->arch.emulation_flags = emflags;
 
+    if ( is_hvm_domain(d) )
+    {
+        d->arch.hvm.vmware_hwver = config->arch.vmware_hwver;
+    }
+
     HYPERVISOR_COMPAT_VIRT_START(d) =
         is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u;
 
diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 3464191..58de573 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -1,6 +1,7 @@
 obj-y += svm/
 obj-y += vmx/
 obj-y += viridian/
+obj-y += vmware/
 
 obj-y += asid.o
 obj-y += dm.o
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index a9d1685..e91169f 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -68,6 +68,7 @@
 #include <asm/hvm/viridian.h>
 #include <asm/hvm/vm_event.h>
 #include <asm/altp2m.h>
+#include <asm/hvm/vmware.h>
 #include <asm/mtrr.h>
 #include <asm/apic.h>
 #include <asm/vm_event.h>
@@ -4109,6 +4110,13 @@ static int hvm_allow_set_param(struct domain *d,
     {
     /* The following parameters should only be changed once. */
     case HVM_PARAM_VIRIDIAN:
+        /* Disallow if vmware_hwver is in use */
+        if ( d->arch.hvm.vmware_hwver )
+        {
+            rc = -EOPNOTSUPP;
+            break;
+        }
+        /* Fall through */
     case HVM_PARAM_IOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_PFN:
     case HVM_PARAM_IOREQ_SERVER_PFN:
diff --git a/xen/arch/x86/hvm/vmware/Makefile b/xen/arch/x86/hvm/vmware/Makefile
new file mode 100644
index 0000000..f864486
--- /dev/null
+++ b/xen/arch/x86/hvm/vmware/Makefile
@@ -0,0 +1 @@
+obj-y += vmware.o
diff --git a/xen/arch/x86/hvm/vmware/vmware.c b/xen/arch/x86/hvm/vmware/vmware.c
new file mode 100644
index 0000000..19e52c1
--- /dev/null
+++ b/xen/arch/x86/hvm/vmware/vmware.c
@@ -0,0 +1,82 @@
+/*
+ * arch/x86/hvm/vmware/cpuid.c
+ *
+ * Copyright (C) 2012-2015 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 <xen/version.h>
+#include <xen/hypercall.h>
+#include <xen/domain_page.h>
+#include <xen/param.h>
+#include <asm/guest_access.h>
+#include <asm/guest/hyperv-tlfs.h>
+#include <asm/paging.h>
+#include <asm/p2m.h>
+#include <asm/apic.h>
+#include <asm/hvm/support.h>
+#include <public/sched.h>
+#include <public/hvm/hvm_op.h>
+
+/*
+ * VMware hardware version 7 defines some of these cpuid levels,
+ * below is a brief description about those.
+ *
+ *     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
+ */
+
+void cpuid_vmware_leaves(const struct vcpu *v, uint32_t leaf,
+                         uint32_t subleaf, struct cpuid_leaf *res)
+{
+    struct domain *d = current->domain;
+
+    ASSERT(has_vmware_cpuid(d));
+    ASSERT(leaf >= 0x40000000 && leaf < 0x40000100);
+
+    leaf -= 0x40000000;
+
+    switch ( leaf )
+    {
+    case 0x0:
+        res->a = 0x40000010; /* Maximum leaf */
+        memcpy(&res->b, "VMwa", 4);
+        memcpy(&res->c, "reVM", 4);
+        memcpy(&res->d, "ware", 4);
+        break;
+
+    case 0x10:
+        /* (Virtual) TSC frequency in kHz. */
+        res->a = d->arch.tsc_khz;
+        /* (Virtual) Bus (local apic timer) frequency in kHz. */
+        res->b = 1000000ull / APIC_BUS_CYCLE_NS;
+        res->c = 0;          /* Reserved */
+        res->d = 0;          /* Reserved */
+        break;
+    }
+}
+
+/*
+ * 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 bc5b8f8..b1e93bb 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -885,8 +885,11 @@ static void do_trap(struct cpu_user_regs *regs)
 int guest_rdmsr_xen(const struct vcpu *v, uint32_t idx, uint64_t *val)
 {
     const struct domain *d = v->domain;
-    /* Optionally shift out of the way of Viridian architectural MSRs. */
-    uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
+    /*
+     * Optionally shift out of the way of Viridian or VMware
+     * architectural leaves.
+     */
+    uint32_t base = is_viridian_or_vmware_cpuid(d) ? 0x40000200 : 0x40000000;
 
     switch ( idx - base )
     {
@@ -901,8 +904,11 @@ int guest_rdmsr_xen(const struct vcpu *v, uint32_t idx, uint64_t *val)
 int guest_wrmsr_xen(struct vcpu *v, uint32_t idx, uint64_t val)
 {
     struct domain *d = v->domain;
-    /* Optionally shift out of the way of Viridian architectural MSRs. */
-    uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
+    /*
+     * Optionally shift out of the way of Viridian or VMware
+     * architectural leaves.
+     */
+    uint32_t base = is_viridian_or_vmware_cpuid(d) ? 0x40000200 : 0x40000000;
 
     switch ( idx - base )
     {
@@ -959,9 +965,10 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
 {
     const struct domain *d = v->domain;
     const struct cpuid_policy *p = d->arch.cpuid;
-    uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
+    uint32_t base = is_viridian_or_vmware_cpuid(d) ? 0x40000100 : 0x40000000;
     uint32_t idx  = leaf - base;
-    unsigned int limit = is_viridian_domain(d) ? p->hv2_limit : p->hv_limit;
+    unsigned int limit = is_viridian_or_vmware_cpuid(d) ?
+        p->hv2_limit : p->hv_limit;
 
     if ( limit == 0 )
         /* Default number of leaves */
@@ -989,7 +996,7 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
     case 2:
         res->a = 1;            /* Number of hypercall-transfer pages */
                                /* MSR base address */
-        res->b = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
+        res->b = is_viridian_or_vmware_cpuid(d) ? 0x40000200 : 0x40000000;
         if ( is_pv_domain(d) ) /* Features */
             res->c |= XEN_CPUID_FEAT1_MMU_PT_UPDATE_PRESERVE_AD;
         break;
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 9d247ba..0f64a4b 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -147,6 +147,9 @@ struct hvm_domain {
 
     uint64_t              *params;
 
+    /* emulated VMware Hardware Version */
+    uint32_t               vmware_hwver;
+
     /* Memory ranges with pinned cache attributes. */
     struct list_head       pinned_cacheattr_ranges;
 
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index be0d8b0..cb55dfb 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -474,6 +474,18 @@ static inline bool hvm_get_guest_bndcfgs(struct vcpu *v, u64 *val)
 #define has_viridian_synic(d) \
     (is_viridian_domain(d) && (viridian_feature_mask(d) & HVMPV_synic))
 
+#define vmware_feature_mask(d) \
+    ((d)->arch.hvm.vmware_hwver)
+
+#define is_vmware_domain(d) \
+    (is_hvm_domain(d) && vmware_feature_mask(d))
+
+#define has_vmware_cpuid(d) \
+    (is_hvm_domain(d) && (vmware_feature_mask(d) >= 7))
+
+#define is_viridian_or_vmware_cpuid(d) \
+    (is_viridian_domain(d) || has_vmware_cpuid(d))
+
 static inline void hvm_inject_exception(
     unsigned int vector, unsigned int type,
     unsigned int insn_len, int error_code)
@@ -754,6 +766,9 @@ static inline bool hvm_has_set_descriptor_access_exiting(void)
 #define is_viridian_domain(d) ((void)(d), false)
 #define is_viridian_vcpu(v) ((void)(v), false)
 #define has_viridian_time_ref_count(d) ((void)(d), false)
+#define is_vmware_domain(d) ((void)(d), false)
+#define has_vmware_cpuid(d) ((void)(d), false)
+#define is_viridian_or_vmware_cpuid(d) ((void)(d), false)
 #define hvm_long_mode_active(v) ((void)(v), false)
 #define hvm_get_guest_time(v) ((void)(v), 0)
 
diff --git a/xen/include/asm-x86/hvm/vmware.h b/xen/include/asm-x86/hvm/vmware.h
new file mode 100644
index 0000000..98248e7
--- /dev/null
+++ b/xen/include/asm-x86/hvm/vmware.h
@@ -0,0 +1,33 @@
+/*
+ * asm-x86/hvm/vmware.h
+ *
+ * Copyright (C) 2012-2015 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>
+
+void cpuid_vmware_leaves(const struct vcpu *v, uint32_t leaf,
+                         uint32_t subleaf, struct cpuid_leaf *res);
+
+#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/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index 629cb2b..54b1c4d 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -304,6 +304,7 @@ struct xen_arch_domainconfig {
                                      XEN_X86_EMU_PIT | XEN_X86_EMU_USE_PIRQ |\
                                      XEN_X86_EMU_VPCI)
     uint32_t emulation_flags;
+    uint32_t vmware_hwver;
 };
 
 /* Location of online VCPU bitmap. */
-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Xen-devel] [XEN PATCH v14 3/8] tools: Add vmware_hwver support
  2020-08-19 16:51   ` [Xen-devel] [XEN PATCH v14 2/8] xen: Add support for VMware cpuid leaves Don Slutz
@ 2020-08-19 16:51     ` Don Slutz
  2020-08-19 16:51       ` [Xen-devel] [XEN PATCH v14 4/8] vmware: Add VMware provided include file Don Slutz
  2020-09-30 14:24     ` [XEN PATCH v14 2/8] xen: Add support for VMware cpuid leaves Jan Beulich
  1 sibling, 1 reply; 16+ messages in thread
From: Don Slutz @ 2020-08-19 16:51 UTC (permalink / raw)
  To: xen-devel
  Cc: Aravind Gopalakrishnan, Boris Ostrovsky, Eddie Dong,
	Ian Campbell, Ian Jackson, Jan Beulich, Jun Nakajima,
	Keir Fraser, Kevin Tian, Stefano Stabellini,
	Suravee Suthikulpanit, Tim Deegan, Andrew Cooper,
	Konrad Rzeszutek Wilk, George Dunlap, Don Slutz, Don Slutz

From: Don Slutz <dslutz@verizon.com>

This is used to set xen_arch_domainconfig vmware_hw. It is set to
the emulated VMware virtual hardware version.

Currently 0, 3-4, 6-11 are good values.  However the code only
checks for == 0, != 0, or < 7.

Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <don.slutz@gmail.com>
---
Acked-by: Ian Campbell <ian.campbell@citrix.com>

v14:
  Reworked to current code.

v13:
  Added: Acked-by: Ian Campbell

v12:
    I'm not sure this hunk has anything to do with this patch, nor
    what the semantic difference between the old and new text is
    supposed to be.
      Dropped comment change.


v11:
  Dropped "If non-zero then default VGA to VMware's VGA"

v10:
    LIBXL_HAVE_LIBXL_VGA_INTERFACE_TYPE_VMWARE &
    LIBXL_HAVE_BUILDINFO_HVM_VMWARE_HWVER are arriving together
    a single umbrella could be used.
      Since I split the LIBXL_VGA_INTERFACE_TYPE_VMWARE into
      it's own patch, this is not longer true.
      But I did use 1 for the 2 c_info changes.
    Please use GCSPRINTF.
  Remove vga=vmware from here.

v9:
      I assumed that s/vmware_hw/vmware_hwver/ is not a big enough
      change to drop the Reviewed-by.  Did a minor edit to the
      commit message to add 7 to the list of values checked.

v7:
    Default handling of hvm.vga.kind bad.
      Fixed.
    Default of vmware_port should be based on vmware_hw.
      Done. 

v5:
      Anything looking for Xen according to the Xen cpuid instructions...
        Adjusted doc to new wording.

 docs/man/xl.cfg.5.pod.in    | 17 +++++++++++++++++
 tools/libxl/libxl_create.c  |  4 +++-
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/libxl_x86.c     |  3 +--
 tools/xl/xl_parse.c         |  3 +++
 5 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index a9eae96..10eac33 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2353,6 +2353,23 @@ The viridian option can be specified as a boolean. A value of true (1)
 is equivalent to the list [ "defaults" ], and a value of false (0) is
 equivalent to an empty list.
 
+=item B<vmware_hwver=NUMBER>
+
+Turns on or off the exposure of VMware cpuid.  The number is
+VMware's hardware version number, where 0 is off.  A number >= 7
+is needed to enable exposure of VMware cpuid.
+
+The hardware version number (vmware_hwver) comes 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_hwver = 7.
+
+=back
+
 =back
 
 =head3 Emulated VGA Graphics Device
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 2814818..e28d175 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -499,7 +499,7 @@ int libxl__domain_build(libxl__gc *gc,
         vments[4] = "start_time";
         vments[5] = GCSPRINTF("%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000);
 
-        localents = libxl__calloc(gc, 13, sizeof(char *));
+        localents = libxl__calloc(gc, 15, sizeof(char *));
         i = 0;
         localents[i++] = "platform/acpi";
         localents[i++] = libxl__acpi_defbool_val(info) ? "1" : "0";
@@ -509,6 +509,8 @@ int libxl__domain_build(libxl__gc *gc,
         localents[i++] = libxl_defbool_val(info->u.hvm.acpi_s4) ? "1" : "0";
         localents[i++] = "platform/acpi_laptop_slate";
         localents[i++] = libxl_defbool_val(info->u.hvm.acpi_laptop_slate) ? "1" : "0";
+        localents[i++] = "platform/vmware_hwver";
+        localents[i++] = GCSPRINTF("%d", d_config->c_info.vmware_hwver);
         if (info->u.hvm.mmio_hole_memkb) {
             uint64_t max_ram_below_4g =
                 (1ULL << 32) - (info->u.hvm.mmio_hole_memkb << 10);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 36350d2..89a9ee7 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -420,6 +420,7 @@ libxl_domain_create_info = Struct("domain_create_info",[
     ("driver_domain",libxl_defbool),
     ("passthrough",  libxl_passthrough),
     ("xend_suspend_evtchn_compat",libxl_defbool),
+    ("vmware_hwver", uint32),
     ], dir=DIR_IN)
 
 libxl_domain_restore_params = Struct("domain_restore_params", [
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 3418a81..0ee7418 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -21,8 +21,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
         abort();
     }
 
-    /* Note: will be changed in next patch (tools: Add ...). */
-    config->arch.vmware_hwver = 0;
+    config->arch.vmware_hwver = d_config->c_info.vmware_hwver;
     return 0;
 }
 
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index c74a9e3..4794398 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1573,6 +1573,9 @@ void parse_config_data(const char *config_source,
     b_info->cmdline = parse_cmdline(config);
 
     xlu_cfg_get_defbool(config, "driver_domain", &c_info->driver_domain, 0);
+    if (!xlu_cfg_get_long(config, "vmware_hwver",  &l, 1))
+        c_info->vmware_hwver = l;
+
     xlu_cfg_get_defbool(config, "acpi", &b_info->acpi, 0);
 
     xlu_cfg_replace_string (config, "bootloader", &b_info->bootloader, 0);
-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Xen-devel] [XEN PATCH v14 4/8] vmware: Add VMware provided include file.
  2020-08-19 16:51     ` [Xen-devel] [XEN PATCH v14 3/8] tools: Add vmware_hwver support Don Slutz
@ 2020-08-19 16:51       ` Don Slutz
  2020-08-19 16:51         ` [Xen-devel] [XEN PATCH v14 5/8] xen: Add vmware_port support Don Slutz
  0 siblings, 1 reply; 16+ messages in thread
From: Don Slutz @ 2020-08-19 16:51 UTC (permalink / raw)
  To: xen-devel
  Cc: Aravind Gopalakrishnan, Boris Ostrovsky, Eddie Dong,
	Ian Campbell, Ian Jackson, Jan Beulich, Jun Nakajima,
	Keir Fraser, Kevin Tian, Stefano Stabellini,
	Suravee Suthikulpanit, Tim Deegan, Andrew Cooper,
	Konrad Rzeszutek Wilk, George Dunlap, Don Slutz, Don Slutz

From: Don Slutz <dslutz@verizon.com>

This file: backdoor_def.h comes from:

http://packages.vmware.com/tools/esx/3.5latest/rhel4/SRPMS/index.html
 open-vm-tools-kmod-7.4.8-396269.423167.src.rpm
  open-vm-tools-kmod-7.4.8.tar.gz
   vmhgfs/backdoor_def.h

and is unchanged.

Added the badly named include file includeCheck.h also.  It only has
a comment and is provided so that backdoor_def.h can be used without
change.

Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <don.slutz@gmail.com>
---
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

v11,v12,v13,v14:
  No change

v10:
   Add Acked-by: Andrew Cooper

v9:
    Either the description is wrong, or the patch is stale.
      stale commit message -- fixed.
    I'd say a file with a single comment line in it would suffice.
      Done.

 xen/arch/x86/hvm/vmware/backdoor_def.h | 167 +++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/vmware/includeCheck.h |   1 +
 2 files changed, 168 insertions(+)
 create mode 100644 xen/arch/x86/hvm/vmware/backdoor_def.h
 create mode 100644 xen/arch/x86/hvm/vmware/includeCheck.h

diff --git a/xen/arch/x86/hvm/vmware/backdoor_def.h b/xen/arch/x86/hvm/vmware/backdoor_def.h
new file mode 100644
index 0000000..e76795f
--- /dev/null
+++ b/xen/arch/x86/hvm/vmware/backdoor_def.h
@@ -0,0 +1,167 @@
+/* **********************************************************
+ * Copyright 1998 VMware, Inc.  All rights reserved. 
+ * **********************************************************
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation version 2 and no later version.
+ *
+ * This program 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.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
+ */
+
+/*
+ * backdoor_def.h --
+ *
+ * This contains backdoor defines that can be included from
+ * an assembly language file.
+ */
+
+
+
+#ifndef _BACKDOOR_DEF_H_
+#define _BACKDOOR_DEF_H_
+
+#define INCLUDE_ALLOW_MODULE
+#define INCLUDE_ALLOW_USERLEVEL
+#define INCLUDE_ALLOW_VMMEXT
+#define INCLUDE_ALLOW_VMCORE
+#define INCLUDE_ALLOW_VMKERNEL
+#include "includeCheck.h"
+
+/*
+ * If you want to add a new low-level backdoor call for a guest userland
+ * application, please consider using the GuestRpc mechanism instead. --hpreg
+ */
+
+#define BDOOR_MAGIC 0x564D5868
+
+/* Low-bandwidth backdoor port. --hpreg */
+
+#define BDOOR_PORT 0x5658
+
+#define BDOOR_CMD_GETMHZ      		   1
+/*
+ * BDOOR_CMD_APMFUNCTION is used by:
+ *
+ * o The FrobOS code, which instead should either program the virtual chipset
+ *   (like the new BIOS code does, matthias offered to implement that), or not
+ *   use any VM-specific code (which requires that we correctly implement
+ *   "power off on CLI HLT" for SMP VMs, boris offered to implement that)
+ *
+ * o The old BIOS code, which will soon be jettisoned
+ *
+ *  --hpreg
+ */
+#define BDOOR_CMD_APMFUNCTION 		   2
+#define BDOOR_CMD_GETDISKGEO  		   3
+#define BDOOR_CMD_GETPTRLOCATION	      4
+#define BDOOR_CMD_SETPTRLOCATION	      5
+#define BDOOR_CMD_GETSELLENGTH		   6
+#define BDOOR_CMD_GETNEXTPIECE		   7
+#define BDOOR_CMD_SETSELLENGTH		   8
+#define BDOOR_CMD_SETNEXTPIECE		   9
+#define BDOOR_CMD_GETVERSION		      10
+#define BDOOR_CMD_GETDEVICELISTELEMENT	11
+#define BDOOR_CMD_TOGGLEDEVICE		   12
+#define BDOOR_CMD_GETGUIOPTIONS		   13
+#define BDOOR_CMD_SETGUIOPTIONS		   14
+#define BDOOR_CMD_GETSCREENSIZE		   15
+#define BDOOR_CMD_MONITOR_CONTROL       16
+#define BDOOR_CMD_GETHWVERSION          17
+#define BDOOR_CMD_OSNOTFOUND            18
+#define BDOOR_CMD_GETUUID               19
+#define BDOOR_CMD_GETMEMSIZE            20
+#define BDOOR_CMD_HOSTCOPY              21 /* Devel only */
+/* BDOOR_CMD_GETOS2INTCURSOR, 22, is very old and defunct. Reuse. */
+#define BDOOR_CMD_GETTIME               23 /* Deprecated. Use GETTIMEFULL. */
+#define BDOOR_CMD_STOPCATCHUP           24
+#define BDOOR_CMD_PUTCHR	        25 /* Devel only */
+#define BDOOR_CMD_ENABLE_MSG	        26 /* Devel only */
+#define BDOOR_CMD_GOTO_TCL	        27 /* Devel only */
+#define BDOOR_CMD_INITPCIOPROM		28
+#define BDOOR_CMD_INT13			29
+#define BDOOR_CMD_MESSAGE               30
+#define BDOOR_CMD_RSVD0                 31
+#define BDOOR_CMD_RSVD1                 32
+#define BDOOR_CMD_RSVD2                 33
+#define BDOOR_CMD_ISACPIDISABLED	34
+#define BDOOR_CMD_TOE			35 /* Not in use */
+/* BDOOR_CMD_INITLSIOPROM, 36, was merged with 28. Reuse. */
+#define BDOOR_CMD_PATCH_SMBIOS_STRUCTS  37
+#define BDOOR_CMD_MAPMEM                38 /* Devel only */
+#define BDOOR_CMD_ABSPOINTER_DATA	39
+#define BDOOR_CMD_ABSPOINTER_STATUS	40
+#define BDOOR_CMD_ABSPOINTER_COMMAND	41
+#define BDOOR_CMD_TIMER_SPONGE          42
+#define BDOOR_CMD_PATCH_ACPI_TABLES	43
+/* Catch-all to allow synchronous tests */
+#define BDOOR_CMD_DEVEL_FAKEHARDWARE	44 /* Debug only - needed in beta */
+#define BDOOR_CMD_GETHZ      		45
+#define BDOOR_CMD_GETTIMEFULL           46
+#define BDOOR_CMD_STATELOGGER           47
+#define BDOOR_CMD_CHECKFORCEBIOSSETUP	48
+#define BDOOR_CMD_LAZYTIMEREMULATION    49
+#define BDOOR_CMD_BIOSBBS               50
+#define BDOOR_CMD_MAX                   51
+
+/* 
+ * IMPORTANT NOTE: When modifying the behavior of an existing backdoor command,
+ * you must adhere to the semantics expected by the oldest Tools who use that
+ * command. Specifically, do not alter the way in which the command modifies 
+ * the registers. Otherwise backwards compatibility will suffer.
+ */
+
+/* High-bandwidth backdoor port. --hpreg */
+
+#define BDOORHB_PORT 0x5659
+
+#define BDOORHB_CMD_MESSAGE 0
+#define BDOORHB_CMD_MAX 1
+
+/*
+ * There is another backdoor which allows access to certain TSC-related
+ * values using otherwise illegal PMC indices when the pseudo_perfctr
+ * control flag is set.
+ */
+
+#define BDOOR_PMC_HW_TSC      0x10000
+#define BDOOR_PMC_REAL_NS     0x10001
+#define BDOOR_PMC_APPARENT_NS 0x10002
+
+#define IS_BDOOR_PMC(index)  (((index) | 3) == 0x10003)
+#define BDOOR_CMD(ecx)       ((ecx) & 0xffff)
+
+
+#ifdef VMM
+/*
+ *----------------------------------------------------------------------
+ *
+ * Backdoor_CmdRequiresFullyValidVCPU --
+ *
+ *    A few backdoor commands require the full VCPU to be valid
+ *    (including GDTR, IDTR, TR and LDTR). The rest get read/write
+ *    access to GPRs and read access to Segment registers (selectors).
+ *
+ * Result:
+ *    True iff VECX contains a command that require the full VCPU to
+ *    be valid.
+ *
+ *----------------------------------------------------------------------
+ */
+static INLINE Bool
+Backdoor_CmdRequiresFullyValidVCPU(unsigned cmd)
+{
+   return cmd == BDOOR_CMD_RSVD0 ||
+          cmd == BDOOR_CMD_RSVD1 ||
+          cmd == BDOOR_CMD_RSVD2;
+}
+#endif
+
+#endif
diff --git a/xen/arch/x86/hvm/vmware/includeCheck.h b/xen/arch/x86/hvm/vmware/includeCheck.h
new file mode 100644
index 0000000..3b63fa4
--- /dev/null
+++ b/xen/arch/x86/hvm/vmware/includeCheck.h
@@ -0,0 +1 @@
+/* Nothing here.  Just to use backdoor_def.h without change. */
-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Xen-devel] [XEN PATCH v14 5/8] xen: Add vmware_port support
  2020-08-19 16:51       ` [Xen-devel] [XEN PATCH v14 4/8] vmware: Add VMware provided include file Don Slutz
@ 2020-08-19 16:51         ` Don Slutz
  2020-08-19 16:52           ` [Xen-devel] [XEN PATCH v14 6/8] tools: " Don Slutz
  2020-10-01 13:04           ` [XEN PATCH v14 5/8] xen: Add vmware_port support Jan Beulich
  0 siblings, 2 replies; 16+ messages in thread
From: Don Slutz @ 2020-08-19 16:51 UTC (permalink / raw)
  To: xen-devel
  Cc: Aravind Gopalakrishnan, Boris Ostrovsky, Eddie Dong,
	Ian Campbell, Ian Jackson, Jan Beulich, Jun Nakajima,
	Keir Fraser, Kevin Tian, Stefano Stabellini,
	Suravee Suthikulpanit, Tim Deegan, Andrew Cooper,
	Konrad Rzeszutek Wilk, George Dunlap, Don Slutz, Don Slutz

From: Don Slutz <dslutz@verizon.com>

This includes adding is_vmware_port_enabled

This is a new xen_arch_domainconfig flag,
XEN_DOMCTL_CONFIG_VMWARE_PORT_MASK.

This enables limited support of VMware's hyper-call.

This is both a more complete support then in currently provided by
QEMU and/or KVM and less.  The missing part requires QEMU changes
and has been left out until the QEMU patches are accepted upstream.

VMware's hyper-call is also known as VMware Backdoor I/O Port.

Note: this support does not depend on vmware_hw being non-zero.

Summary is that VMware treats "in (%dx),%eax" (or "out %eax,(%dx)")
to port 0x5658 specially.  Note: since many operations return data
in EAX, "in (%dx),%eax" is the one to use.  The other lengths like
"in (%dx),%al" will still do things, only AL part of EAX will be
changed.  For "out %eax,(%dx)" of all lengths, EAX will remain
unchanged.

An open source example of using this is:

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

Which only uses "inl (%dx)".  Also

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

Some of the best info is at:

https://sites.google.com/site/chitchatvmback/backdoor

Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <don.slutz@gmail.com>
---
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

v14:
  Reworked to current code.
  Drop arch_flags and use XEN_X86_EMU_VMWARE_PORT which will not be
    added to "all".

v13:
  Changed to uint32_t arch_flags, since the emulation_flags is this.

v12:
     Surrounding code avoiding the use of "break" makes the result
     look rather inconsistent. Please move this up immediately after
     the XSM check, or drop the "break".
       Moved it up.

v11:
   Dropped ASSERT(is_hvm_domain(currd))
    Newline after break;

v10:
    Probably better as EOPNOTSUPP, as it is a configuration problem.
    This function looks as if it should be static.
    I would suggest putting vmport_register declaration in hvm.h ...
    As indicated before, I don't think this is a good use case for a
    domain creation flag.
      Switch to the new config way.
    struct domain *d => struct domain *currd
    Are you sure you don't want to zero the high halves of 64-bit ...
      Comment added.
   Then just have this handled into the default case.
      Reworked new_eax handling.
   is_hvm_domain(currd)
   And - why here rather than before the switch() or even right at the
   start of the function?
      Moved to start.
   With that, is it really correct that OUT updates the other registers
   just like IN? If so, this deserves a comment, so that readers won't
   think this is in error.
     All done in comment at start.


v9:
  Switch to x86_emulator to handle #GP code moved to next patch.
    Can you explain why a HVM param isn't suitable here?
      Issue with changing QEMU on the fly.
      Andrew Cooper: My recommendation is still to use a creation flag
        So no change.
    Please move SVM's identical definition into ...
      Did this as #1.  No longer needed, but since the patch was ready
      I have included it.
    --Lots of questions about code that no long is part of this patch. --
    With this, is handling other than 32-bit in/out really
    meaningful/correct?
      Added comment about this.
    Since you can't get here for PV, I can't see what you need this.
      Changed to an ASSERT.
    Why version 4?
      Added comment about this.
    -- Several questions about register changes.
      Re-coded to use new_eax and set *val to this.
      Change to generealy use reg->_e..
    These ei1/ei2 checks belong in the callers imo -
      Moved.
    the "port" function parameter isn't even checked
      Add check for exact match.
    If dropping the code is safe without also forbidding the
    combination of nested and VMware emulation.
      Added the forbidding the combination of nested and VMware.
      Mostly do to the cases of the nested virtual code is the one
      to handle VMware stuff if needed, not the root one.  Also I am
      having issues testing xen nested in xen and using hvm.

v7:
      More on AMD in the commit message.
      Switch to only change 32bit part of registers, what VMware
        does.
    Too much logging and tracing.
      Dropped a lot of it.  This includes vmport_debug=

v6:
      Dropped the attempt to use svm_nextrip_insn_length via
      __get_instruction_length (added in v2).  Just always look
      at upto 15 bytes on AMD.

v5:
      we should make sure that svm_vmexit_gp_intercept is not executed for
      any other guest.
        Added an ASSERT on is_vmware_port_enabled.
      magic integers?
        Added #define for them.
      I am fairly certain that you need some brackets here.
        Added brackets.

 xen/arch/x86/domain.c             |  15 ++--
 xen/arch/x86/hvm/hvm.c            |   9 +++
 xen/arch/x86/hvm/vmware/Makefile  |   1 +
 xen/arch/x86/hvm/vmware/vmport.c  | 148 ++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/hvm/domain.h  |   3 +
 xen/include/asm-x86/hvm/hvm.h     |   2 +
 xen/include/public/arch-x86/xen.h |   4 ++
 7 files changed, 177 insertions(+), 5 deletions(-)
 create mode 100644 xen/arch/x86/hvm/vmware/vmport.c

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index a317907..0cf73ef 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -504,6 +504,8 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
 
 static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
 {
+    uint32_t all_emflags = emflags & XEN_X86_EMU_ALL;
+
 #ifdef CONFIG_HVM
     /* This doesn't catch !CONFIG_HVM case but it is better than nothing */
     BUILD_BUG_ON(X86_EMU_ALL != XEN_X86_EMU_ALL);
@@ -512,14 +514,15 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
     if ( is_hvm_domain(d) )
     {
         if ( is_hardware_domain(d) &&
-             emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC) )
+             all_emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC) )
             return false;
         if ( !is_hardware_domain(d) &&
-             emflags != (X86_EMU_ALL & ~X86_EMU_VPCI) &&
-             emflags != X86_EMU_LAPIC )
+             all_emflags != (X86_EMU_ALL & ~X86_EMU_VPCI) &&
+             all_emflags != X86_EMU_LAPIC )
             return false;
     }
-    else if ( emflags != 0 && emflags != X86_EMU_PIT )
+    else if ( emflags & XEN_X86_EMU_VMWARE_PORT ||
+              (all_emflags != 0 && all_emflags != X86_EMU_PIT) )
     {
         /* PV or classic PVH. */
         return false;
@@ -581,7 +584,7 @@ int arch_domain_create(struct domain *d,
     if ( is_hardware_domain(d) && is_pv_domain(d) )
         emflags |= XEN_X86_EMU_PIT;
 
-    if ( emflags & ~XEN_X86_EMU_ALL )
+    if ( emflags & ~(XEN_X86_EMU_ALL | XEN_X86_EMU_VMWARE_PORT) )
     {
         printk(XENLOG_G_ERR "d%d: Invalid emulation bitmap: %#x\n",
                d->domain_id, emflags);
@@ -600,6 +603,8 @@ int arch_domain_create(struct domain *d,
     if ( is_hvm_domain(d) )
     {
         d->arch.hvm.vmware_hwver = config->arch.vmware_hwver;
+        d->arch.hvm.is_vmware_port_enabled =
+            !!(emflags & XEN_X86_EMU_VMWARE_PORT);
     }
 
     HYPERVISOR_COMPAT_VIRT_START(d) =
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e91169f..42d96b1 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -697,6 +697,9 @@ int hvm_domain_initialise(struct domain *d)
     if ( hvm_tsc_scaling_supported )
         d->arch.hvm.tsc_scaling_ratio = hvm_default_tsc_scaling_ratio;
 
+    if ( d->arch.hvm.is_vmware_port_enabled )
+        vmport_register(d);
+
     rc = viridian_domain_init(d);
     if ( rc )
         goto fail2;
@@ -4214,6 +4217,12 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
         rc = xsm_hvm_param_nested(XSM_PRIV, d);
         if ( rc )
             break;
+        /* Prevent nestedhvm enable with vmport */
+        if ( value && d->arch.hvm.is_vmware_port_enabled )
+        {
+            rc = -EOPNOTSUPP;
+            break;
+        }
         if ( value > 1 )
             rc = -EINVAL;
         /*
diff --git a/xen/arch/x86/hvm/vmware/Makefile b/xen/arch/x86/hvm/vmware/Makefile
index f864486..74aea16 100644
--- a/xen/arch/x86/hvm/vmware/Makefile
+++ b/xen/arch/x86/hvm/vmware/Makefile
@@ -1 +1,2 @@
 obj-y += vmware.o
+obj-y += vmport.o
diff --git a/xen/arch/x86/hvm/vmware/vmport.c b/xen/arch/x86/hvm/vmware/vmport.c
new file mode 100644
index 0000000..863ec50
--- /dev/null
+++ b/xen/arch/x86/hvm/vmware/vmport.c
@@ -0,0 +1,148 @@
+/*
+ * HVM VMPORT emulation
+ *
+ * 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/lib.h>
+#include <asm/hvm/hvm.h>
+#include <asm/hvm/support.h>
+
+#include "backdoor_def.h"
+
+static int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
+{
+    struct cpu_user_regs *regs = guest_cpu_user_regs();
+
+    /*
+     * While VMware expects only 32-bit in, they do support using
+     * other sizes and out.  However they do require only the 1 port
+     * and the correct value in eax.  Since some of the data
+     * returned in eax is smaller the 32 bits and/or you only need
+     * the other registers the dir and bytes do not need any
+     * checking.  The caller will handle the bytes, and dir is
+     * handled below for eax.
+     */
+    if ( port == BDOOR_PORT && regs->eax == BDOOR_MAGIC )
+    {
+        uint32_t new_eax = ~0u;
+        uint64_t value;
+        struct vcpu *curr = current;
+        struct domain *currd = curr->domain;
+
+        /*
+         * VMware changes the other (non eax) registers ignoring dir
+         * (IN vs OUT).  It also changes only the 32-bit part
+         * leaving the high 32-bits unchanged, unlike what one would
+         * expect to happen.
+         */
+        switch ( regs->ecx & 0xffff )
+        {
+        case BDOOR_CMD_GETMHZ:
+            new_eax = currd->arch.tsc_khz / 1000;
+            break;
+
+        case BDOOR_CMD_GETVERSION:
+            /* MAGIC */
+            regs->ebx = BDOOR_MAGIC;
+            /* VERSION_MAGIC */
+            new_eax = 6;
+            /* Claim we are an ESX. VMX_TYPE_SCALABLE_SERVER */
+            regs->ecx = 2;
+            break;
+
+        case BDOOR_CMD_GETHWVERSION:
+            /* vmware_hw */
+            new_eax = currd->arch.hvm.vmware_hwver;
+            /*
+             * Returning zero is not the best.  VMware was not at
+             * all consistent in the handling of this command until
+             * VMware hardware version 4.  So it is better to claim
+             * 4 then 0.  This should only happen in strange configs.
+             */
+            if ( !new_eax )
+                new_eax = 4;
+            break;
+
+        case BDOOR_CMD_GETHZ:
+        {
+            struct segment_register sreg;
+
+            hvm_get_segment_register(curr, x86_seg_ss, &sreg);
+            if ( sreg.dpl == 0 )
+            {
+                value = currd->arch.tsc_khz * 1000;
+                /* apic-frequency (bus speed) */
+                regs->ecx = 1000000000ULL / APIC_BUS_CYCLE_NS;
+                /* High part of tsc-frequency */
+                regs->ebx = value >> 32;
+                /* Low part of tsc-frequency */
+                new_eax = value;
+            }
+            break;
+
+        }
+        case BDOOR_CMD_GETTIME:
+            value = get_localtime_us(currd) -
+                currd->time_offset.seconds * 1000000ULL;
+            /* hostUsecs */
+            regs->ebx = value % 1000000UL;
+            /* hostSecs */
+            new_eax = value / 1000000ULL;
+            /* maxTimeLag */
+            regs->ecx = 1000000;
+            /* offset to GMT in minutes */
+            regs->edx = currd->time_offset.seconds / 60;
+            break;
+
+        case BDOOR_CMD_GETTIMEFULL:
+            /* BDOOR_MAGIC */
+            new_eax = BDOOR_MAGIC;
+            value = get_localtime_us(currd) -
+                currd->time_offset.seconds * 1000000ULL;
+            /* hostUsecs */
+            regs->ebx = value % 1000000UL;
+            /* hostSecs low 32 bits */
+            regs->edx = value / 1000000ULL;
+            /* hostSecs high 32 bits */
+            regs->esi = (value / 1000000ULL) >> 32;
+            /* maxTimeLag */
+            regs->ecx = 1000000;
+            break;
+
+        default:
+            /* Let backing DM handle */
+            return X86EMUL_UNHANDLEABLE;
+        }
+        if ( dir == IOREQ_READ )
+            *val = new_eax;
+    }
+    else if ( dir == IOREQ_READ )
+        *val = ~0u;
+
+    return X86EMUL_OKAY;
+}
+
+void vmport_register(struct domain *d)
+{
+    register_portio_handler(d, BDOOR_PORT, 4, vmport_ioport);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-set-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 0f64a4b..355777c 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -162,6 +162,9 @@ struct hvm_domain {
     spinlock_t             uc_lock;
     bool_t                 is_in_uc_mode;
 
+    /* VMware backdoor port available */
+    bool_t                 is_vmware_port_enabled;
+
     /* hypervisor intercepted msix table */
     struct list_head       msixtbl_list;
 
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index cb55dfb..74a9590 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -789,6 +789,8 @@ static inline bool hvm_has_set_descriptor_access_exiting(void)
 
 #endif  /* CONFIG_HVM */
 
+void vmport_register(struct domain *d);
+
 #endif /* __ASM_X86_HVM_HVM_H__ */
 
 /*
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index 54b1c4d..105c6a3 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -296,6 +296,10 @@ struct xen_arch_domainconfig {
 #define XEN_X86_EMU_USE_PIRQ        (1U<<_XEN_X86_EMU_USE_PIRQ)
 #define _XEN_X86_EMU_VPCI           10
 #define XEN_X86_EMU_VPCI            (1U<<_XEN_X86_EMU_VPCI)
+/* Enable use of vmware backdoor port.
+ * Not part of XEN_X86_EMU_ALL */
+#define _XEN_X86_EMU_VMWARE_PORT    11
+#define XEN_X86_EMU_VMWARE_PORT     (1U<<_XEN_X86_EMU_VMWARE_PORT)
 
 #define XEN_X86_EMU_ALL             (XEN_X86_EMU_LAPIC | XEN_X86_EMU_HPET |  \
                                      XEN_X86_EMU_PM | XEN_X86_EMU_RTC |      \
-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Xen-devel] [XEN PATCH v14 6/8] tools: Add vmware_port support
  2020-08-19 16:51         ` [Xen-devel] [XEN PATCH v14 5/8] xen: Add vmware_port support Don Slutz
@ 2020-08-19 16:52           ` Don Slutz
  2020-08-19 16:52             ` [Xen-devel] [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT Don Slutz
  2020-10-01 13:04           ` [XEN PATCH v14 5/8] xen: Add vmware_port support Jan Beulich
  1 sibling, 1 reply; 16+ messages in thread
From: Don Slutz @ 2020-08-19 16:52 UTC (permalink / raw)
  To: xen-devel
  Cc: Aravind Gopalakrishnan, Boris Ostrovsky, Eddie Dong,
	Ian Campbell, Ian Jackson, Jan Beulich, Jun Nakajima,
	Keir Fraser, Kevin Tian, Stefano Stabellini,
	Suravee Suthikulpanit, Tim Deegan, Andrew Cooper,
	Konrad Rzeszutek Wilk, George Dunlap, Don Slutz, Don Slutz

From: Don Slutz <don.slutz@gmail.com>

This new libxl_domain_create_info field is used to set
XEN_DOMCTL_CONFIG_VMWARE_PORT_MASK in the xc_domain_configuration_t
for x86.

In xen it is is_vmware_port_enabled.

If is_vmware_port_enabled then
  enable a limited support of VMware's hyper-call.

VMware's hyper-call is also known as VMware Backdoor I/O Port.

if vmware_port is not specified in the config file, let
"vmware_hwver != 0" be the default value.  This means that only
vmware_hwver = 7 needs to be specified to enable both features.

vmware_hwver = 7 is special because that is what controls the
enable of CPUID leaves for VMware (vmware_hwver >= 7).

Note: vmware_port and nestedhvm cannot be specified at the
same time.

Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <don.slutz@gmail.com>
---
Acked-by: Ian Campbell <ian.campbell@citrix.com>

v14:
  Reworked to current code.

v13:
  Added Acked-by: Ian Campbell

v12:
    s/come/comes/

      In v11 this seems to have morphed into only
    LIBXL_HAVE_LIBXL_VGA_INTERFACE_TYPE_VMWARE being provided, which
    is clearly not an appropriate umbrella #define.

    "#define LIBXL_HAVE_CREATEINFO_VMWARE 1"
    Lets just have a single one of these indicating support for
    vmware, it should be added at the end of the series after all
    the baseline vmware functionality is in place. I think that
    means hwver, vga=vmware and this port stuff.

      Make (tools: Add vga=vmware) no longer independent.
      Change the #define to "LIBXL_HAVE_VMWARE"


v11:
  Dropped "If non-zero then default VGA to VMware's VGA"

v10:
    If..." at the start of the sentence ...
    Also, why is 7 special?


 docs/man/xl.cfg.5.pod.in    | 15 +++++++++++++++
 tools/libxl/libxl.h         |  5 +++++
 tools/libxl/libxl_create.c  | 10 ++++++++++
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/libxl_x86.c     |  2 ++
 tools/xl/xl_parse.c         |  1 +
 6 files changed, 34 insertions(+)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 10eac33..3c73985 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2359,6 +2359,8 @@ Turns on or off the exposure of VMware cpuid.  The number is
 VMware's hardware version number, where 0 is off.  A number >= 7
 is needed to enable exposure of VMware cpuid.
 
+If not zero it changes the default for vmware_port to on.
+
 The hardware version number (vmware_hwver) comes from VMware config files.
 
 =over 4
@@ -2370,6 +2372,19 @@ For vssd:VirtualSystemType == vmx-07, vmware_hwver = 7.
 
 =back
 
+=item B<vmware_port=BOOLEAN>
+
+Turns on or off the exposure of VMware port.  This is known as
+vmport in QEMU.  Also called VMware Backdoor I/O Port.  Not all
+defined VMware backdoor commands are implemented.  All of the
+ones that Linux kernel uses are defined.
+
+Defaults to enabled if vmware_hwver is non-zero (i.e. enabled)
+otherwise defaults to disabled.
+
+Note: vmware_port and nestedhvm cannot be specified at the
+same time.
+
 =back
 
 =head3 Emulated VGA Graphics Device
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 1cd6c38..48ab231 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -439,6 +439,11 @@
 #define LIBXL_HAVE_CREATEINFO_PASSTHROUGH 1
 
 /*
+ * libxl has VMware changes.
+ */
+#define LIBXL_HAVE_VMWARE 1
+
+/*
  * libxl ABI compatibility
  *
  * The only guarantee which libxl makes regarding ABI compatibility
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index e28d175..6689443 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -51,6 +51,7 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
         libxl_defbool_setdefault(&c_info->oos, true);
     }
 
+    libxl_defbool_setdefault(&c_info->vmware_port, c_info->vmware_hwver != 0);
     libxl_defbool_setdefault(&c_info->run_hotplug_scripts, true);
     libxl_defbool_setdefault(&c_info->driver_domain, false);
 
@@ -1185,6 +1186,15 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
         goto error_out;
     }
 
+    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
+        libxl_defbool_val(d_config->b_info.nested_hvm) &&
+        libxl_defbool_val(d_config->c_info.vmware_port)) {
+        ret = ERROR_INVAL;
+        LOGD(ERROR, domid,
+            "vmware_port and nestedhvm cannot be enabled simultaneously\n");
+        goto error_out;
+    }
+
     if (d_config->c_info.type != LIBXL_DOMAIN_TYPE_PV &&
         (libxl_defbool_val(d_config->b_info.nested_hvm) &&
         ((d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 89a9ee7..f563980 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -421,6 +421,7 @@ libxl_domain_create_info = Struct("domain_create_info",[
     ("passthrough",  libxl_passthrough),
     ("xend_suspend_evtchn_compat",libxl_defbool),
     ("vmware_hwver", uint32),
+    ("vmware_port",  libxl_defbool),
     ], dir=DIR_IN)
 
 libxl_domain_restore_params = Struct("domain_restore_params", [
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 0ee7418..8da9913 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -22,6 +22,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
     }
 
     config->arch.vmware_hwver = d_config->c_info.vmware_hwver;
+    if (libxl_defbool_val(d_config->c_info.vmware_port))
+        config->arch.emulation_flags |= XEN_X86_EMU_VMWARE_PORT;
     return 0;
 }
 
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 4794398..b655e25 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1321,6 +1321,7 @@ void parse_config_data(const char *config_source,
     }
 
     xlu_cfg_get_defbool(config, "oos", &c_info->oos, 0);
+    xlu_cfg_get_defbool(config, "vmware_port", &c_info->vmware_port, 0);
 
     if (!xlu_cfg_get_string (config, "pool", &buf, 0))
         xlu_cfg_replace_string(config, "pool", &c_info->pool_name, 0);
-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Xen-devel] [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT
  2020-08-19 16:52           ` [Xen-devel] [XEN PATCH v14 6/8] tools: " Don Slutz
@ 2020-08-19 16:52             ` Don Slutz
  2020-08-19 16:52               ` [Xen-devel] [XEN PATCH v14 8/8] Add xentrace to vmware_port Don Slutz
  2020-10-01 14:41               ` [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT Jan Beulich
  0 siblings, 2 replies; 16+ messages in thread
From: Don Slutz @ 2020-08-19 16:52 UTC (permalink / raw)
  To: xen-devel
  Cc: Aravind Gopalakrishnan, Boris Ostrovsky, Eddie Dong,
	Ian Campbell, Ian Jackson, Jan Beulich, Jun Nakajima,
	Keir Fraser, Kevin Tian, Stefano Stabellini,
	Suravee Suthikulpanit, Tim Deegan, Andrew Cooper,
	Konrad Rzeszutek Wilk, George Dunlap, Don Slutz, Don Slutz

From: Don Slutz <dslutz@verizon.com>

This adds synchronization of the 6 vcpu registers (only 32bits of
them) that QEMU's vmport.c and vmmouse.c needs between Xen and QEMU.
This is how VMware defined the use of these registers.

This is to avoid a 2nd and 3rd exchange between QEMU and Xen to
fetch and put these 6 vcpu registers used by the code in QEMU's
vmport.c and vmmouse.c

In the tools, enable usage of QEMU's vmport code.

The currently most useful VMware port support that QEMU has is the
VMware mouse support.  Xorg included a VMware mouse support that
uses absolute mode.  This make using a mouse in X11 much nicer.

Also added HVM_IOREQSRV_DISABLE_VMPORT to allow ioreq server(s)
to not get the mapping to the 6 vcpu registers.

Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <don.slutz@gmail.com>
---
Acked-by: Ian Campbell <ian.campbell@citrix.com>

v14:
  Paul Durrant: is_vmware already incorporated !is_mmio...
    Reworked the "Verify the emulation request has been correctly re-issued"
    to have a pre if.
  Paul Durrant: This could be done in a single statement.
    Made a single statement.
  Paul Durrant: Is every ioreq server going to have one of these?
    Code no longer exists.  But, there is a way to not have it.
    See HVM_IOREQSRV_DISABLE_VMPORT
  Jan Beulich: Long line needs breaking up.
    Fixed 
  Jan Beulich: The parameter should no longer be named "buf".
    Changed to pt.
  Jan Beulich: Do you really need to set up a (dummy) range set for this
    Last version did not finsh on this.  Here is what I think now:
      hvm_select_ioreq_server() uses this "dummy" range to see if a given
      IOREQ Server should be picked.  XEN_DMOP_map_io_range_to_ioreq_server
      and XEN_DMOP_unmap_io_range_from_ioreq_server can be used to enable
      or disable vmport usage at any time.      

  Jan Beulich: Several comments on vmport_check_port
    All changed.
  Jan Beulich: Lower case please (and ...
    Changed to lower case in the enum.

  Jan Beulich: Was left off in the middle
      I think haveing the current state of the code posted will help.

  Jan Beulich: don't think having something prefixed vmware_ in the Xen public
    headers is a good idea.
      Also did not reach a clear resolution.
        Last statement:
          All of this may be true and fine, but none of this is - to me - a
          reason to introduce new unclean names into the Xen public
          interface. In no event do I see qemu dictating naming to us.
        My current answer:
          My take on this is that you want me to use a different name,
          is vmport_regs a name you can live with?

  Jan Beulich: Also throughout the series I didn't find any code addition to
    guarantee (perhaps at build time) that BDOOR_PORT doesn't
    collide with any other use ports 
      Added BUILD_BUG_ON for all that can be checked at build time,
      which I think answers this.

  Jan Beulich: But accessing BDOOR_PORT + 1 with a 4-byte operation ought to
    be undefined (or properly split up). After all you don't know what is
    on BDOOR_PORT + 4.
      Changed to only allow BDOOR_PORT with a 4-byte operation.



  Fixed bug in hvm_alloc_legacy_ioreq_gfn: the if is always false.


v13:
  Rebased on staging (not a simple rebase, needed rework to function
  with changes).
    I would have made this !vmport_check_port ...
       Changed to !is_vmware, and invert vmport_check_port's return value.
  Use 1 one for "list_for_each_entry ( sv, ..."
  Added full stop in comments.

v12:
  Rebase changes.

  Pass size to vmport_check_port() -- required if overlap
  I.E. inl on port 0x5657, 0x5656, 0x5655, 0x5659, 0x565a,
  and 0x565b.

  Move define of vmport_check_port() into this patch from ring3
  patch.

v11:
  No change

v10:
  These literals should become an enum.
    I don't think the invalidate type is needed.
    Code handling "case X86EMUL_UNHANDLEABLE:" in emulate.c
    is unclear.
    Comment about "special' range of 1" is not clear.


v9:
  New code was presented as an RFC before this.

  Paul Durrant sugested I add support for other IOREQ types
  to HVMOP_map_io_range_to_ioreq_server.
    I have done this.


 tools/libs/devicemodel/core.c    |   4 +-
 tools/libxc/include/xc_dom.h     |   2 +-
 tools/libxc/xc_dom_x86.c         |   3 +
 tools/libxl/libxl_dm.c           |   2 +
 xen/arch/x86/hvm/dm.c            |   3 +-
 xen/arch/x86/hvm/emulate.c       |  57 ++++++-
 xen/arch/x86/hvm/hvm.c           |   4 +
 xen/arch/x86/hvm/ioreq.c         | 348 +++++++++++++++++++++++++++++++++------
 xen/arch/x86/hvm/vmware/vmport.c |  36 ++++
 xen/include/asm-x86/hvm/domain.h |   4 +-
 xen/include/asm-x86/hvm/hvm.h    |   1 +
 xen/include/public/hvm/dm_op.h   |  21 ++-
 xen/include/public/hvm/ioreq.h   |  17 ++
 xen/include/public/hvm/params.h  |   2 +-
 14 files changed, 443 insertions(+), 61 deletions(-)

diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
index 4d40639..72702d7 100644
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -166,7 +166,7 @@ static int xendevicemodel_op(
 }
 
 int xendevicemodel_create_ioreq_server(
-    xendevicemodel_handle *dmod, domid_t domid, int handle_bufioreq,
+    xendevicemodel_handle *dmod, domid_t domid, int flags,
     ioservid_t *id)
 {
     struct xen_dm_op op;
@@ -178,7 +178,7 @@ int xendevicemodel_create_ioreq_server(
     op.op = XEN_DMOP_create_ioreq_server;
     data = &op.u.create_ioreq_server;
 
-    data->handle_bufioreq = handle_bufioreq;
+    data->flags = flags;
 
     rc = xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
     if (rc)
diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 52a4d6c..7413917 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -20,7 +20,7 @@
 #include <xenguest.h>
 
 #define INVALID_PFN ((xen_pfn_t)-1)
-#define X86_HVM_NR_SPECIAL_PAGES    8
+#define X86_HVM_NR_SPECIAL_PAGES    9
 #define X86_HVM_END_SPECIAL_REGION  0xff000u
 #define XG_MAX_MODULES 2
 
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 9439805..bd42f42 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -67,6 +67,7 @@
 #define SPECIALPAGE_IOREQ    5
 #define SPECIALPAGE_IDENT_PT 6
 #define SPECIALPAGE_CONSOLE  7
+#define SPECIALPAGE_VMPORT_REGS 8
 #define special_pfn(x) \
     (X86_HVM_END_SPECIAL_REGION - X86_HVM_NR_SPECIAL_PAGES + (x))
 
@@ -657,6 +658,8 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
                      special_pfn(SPECIALPAGE_BUFIOREQ));
     xc_hvm_param_set(xch, domid, HVM_PARAM_IOREQ_PFN,
                      special_pfn(SPECIALPAGE_IOREQ));
+    xc_hvm_param_set(xch, domid, HVM_PARAM_VMPORT_REGS_PFN,
+                     special_pfn(SPECIALPAGE_VMPORT_REGS));
     xc_hvm_param_set(xch, domid, HVM_PARAM_CONSOLE_PFN,
                      special_pfn(SPECIALPAGE_CONSOLE));
     xc_hvm_param_set(xch, domid, HVM_PARAM_PAGING_RING_PFN,
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 415c12e..9be4548 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1822,6 +1822,8 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
             }
         }
 
+        if (libxl_defbool_val(c_info->vmware_port))
+            machinearg = GCSPRINTF("%s,vmport=on", machinearg);
         flexarray_append(dm_args, machinearg);
         for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; i++)
             flexarray_append(dm_args, b_info->extra_hvm[i]);
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 9930d68..087ad05 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -419,8 +419,7 @@ static int dm_op(const struct dmop_args *op_args)
         if ( data->pad[0] || data->pad[1] || data->pad[2] )
             break;
 
-        rc = hvm_create_ioreq_server(d, data->handle_bufioreq,
-                                     &data->id);
+        rc = hvm_create_ioreq_server(d, data->flags, &data->id);
         break;
     }
 
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 8b4e73a..befa55e 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -28,6 +28,8 @@
 #include <asm/iocap.h>
 #include <asm/vm_event.h>
 
+vmware_regs_t *get_vmport_regs_any(struct hvm_ioreq_server *s, struct vcpu *v);
+
 struct hvmemul_cache
 {
     /* The cache is disabled as long as num_ents > max_ents. */
@@ -173,6 +175,8 @@ static int hvmemul_do_io(
     };
     void *p_data = (void *)data;
     int rc;
+    bool_t is_vmware = !is_mmio && !data_is_addr &&
+        vmport_check_port(p.addr, p.size);
 
     /*
      * Weird-sized accesses have undefined behaviour: we discard writes
@@ -189,11 +193,17 @@ static int hvmemul_do_io(
     case STATE_IOREQ_NONE:
         break;
     case STATE_IORESP_READY:
+    {
+        uint8_t calc_type = is_mmio ? IOREQ_TYPE_COPY : IOREQ_TYPE_PIO;
+
+        if ( is_vmware )
+            calc_type = IOREQ_TYPE_VMWARE_PORT;
+
         vio->io_req.state = STATE_IOREQ_NONE;
         p = vio->io_req;
 
         /* Verify the emulation request has been correctly re-issued */
-        if ( (p.type != (is_mmio ? IOREQ_TYPE_COPY : IOREQ_TYPE_PIO)) ||
+        if ( (p.type != calc_type) ||
              (p.addr != addr) ||
              (p.size != size) ||
              (p.count > *reps) ||
@@ -202,7 +212,7 @@ static int hvmemul_do_io(
              (p.data_is_ptr != data_is_addr) ||
              (data_is_addr && (p.data != data)) )
             domain_crash(currd);
-
+    }
         if ( data_is_addr )
             return X86EMUL_UNHANDLEABLE;
 
@@ -322,6 +332,49 @@ static int hvmemul_do_io(
             }
         }
 
+        if ( unlikely(is_vmware) )
+        {
+            vmware_regs_t *vr;
+
+            BUILD_BUG_ON(sizeof(ioreq_t) < sizeof(vmware_regs_t));
+
+            p.type = vio->io_req.type = IOREQ_TYPE_VMWARE_PORT;
+            s = hvm_select_ioreq_server(currd, &p);
+            vr = get_vmport_regs_any(s, curr);
+
+            /*
+             * If there is no suitable backing DM, just ignore accesses.  If
+             * we do not have access to registers to pass to QEMU, just
+             * ignore access.
+             */
+            if ( !s || !vr )
+            {
+                rc = hvm_process_io_intercept(&null_handler, &p);
+                vio->io_req.state = STATE_IOREQ_NONE;
+            }
+            else
+            {
+                const struct cpu_user_regs *regs = guest_cpu_user_regs();
+
+                p.data = regs->rax;
+                /* The code in QEMU that uses these registers,
+                 * vmport.c and vmmouse.c, only uses the 32bit part
+                 * of the register.  This is how VMware defined the
+                 * use of these registers.
+                 */
+                vr->ebx = regs->ebx;
+                vr->ecx = regs->ecx;
+                vr->edx = regs->edx;
+                vr->esi = regs->esi;
+                vr->edi = regs->edi;
+
+                rc = hvm_send_ioreq(s, &p, 0);
+                if ( rc != X86EMUL_RETRY || currd->is_shutting_down )
+                    vio->io_req.state = STATE_IOREQ_NONE;
+            }
+            break;
+        }
+
         if ( !s )
             s = hvm_select_ioreq_server(currd, &p);
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 42d96b1..03d2ad5 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4122,6 +4122,7 @@ static int hvm_allow_set_param(struct domain *d,
         /* Fall through */
     case HVM_PARAM_IOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_PFN:
+    case HVM_PARAM_VMPORT_REGS_PFN:
     case HVM_PARAM_IOREQ_SERVER_PFN:
     case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
     case HVM_PARAM_ALTP2M:
@@ -4279,10 +4280,13 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
 
     case HVM_PARAM_IOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_PFN:
+    case HVM_PARAM_VMPORT_REGS_PFN:
         BUILD_BUG_ON(HVM_PARAM_IOREQ_PFN >
                      sizeof(d->arch.hvm.ioreq_gfn.legacy_mask) * 8);
         BUILD_BUG_ON(HVM_PARAM_BUFIOREQ_PFN >
                      sizeof(d->arch.hvm.ioreq_gfn.legacy_mask) * 8);
+        BUILD_BUG_ON(HVM_PARAM_VMPORT_REGS_PFN >
+                     sizeof(d->arch.hvm.ioreq_gfn.legacy_mask) * 8);
         if ( value )
             set_bit(index, &d->arch.hvm.ioreq_gfn.legacy_mask);
         break;
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index 1cc27df..a5d124e 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -108,6 +108,44 @@ static struct hvm_ioreq_vcpu *get_pending_vcpu(const struct vcpu *v,
     return NULL;
 }
 
+static vmware_regs_t *get_vmport_regs_one(struct hvm_ioreq_server *s,
+                                          struct vcpu *v)
+{
+    struct hvm_ioreq_vcpu *sv;
+
+    list_for_each_entry ( sv, &s->ioreq_vcpu_list, list_entry )
+    {
+        if ( sv->vcpu == v )
+        {
+            shared_vmport_iopage_t *p = s->vmport_ioreq.va;
+            if ( !p )
+                return NULL;
+            return &p->vcpu_vmport_regs[v->vcpu_id];
+        }
+    }
+    return NULL;
+}
+
+vmware_regs_t *get_vmport_regs_any(struct hvm_ioreq_server *s, struct vcpu *v)
+{
+    struct domain *d = v->domain;
+    unsigned int id;
+
+    ASSERT((v == current) || !vcpu_runnable(v));
+
+    if ( s )
+        return get_vmport_regs_one(s, v);
+
+    FOR_EACH_IOREQ_SERVER(d, id, s)
+    {
+        vmware_regs_t *ret = get_vmport_regs_one(s, v);
+
+        if ( ret )
+            return ret;
+    }
+    return NULL;
+}
+
 bool hvm_io_pending(struct vcpu *v)
 {
     return get_pending_vcpu(v, NULL);
@@ -206,6 +244,26 @@ bool handle_hvm_io_completion(struct vcpu *v)
         return handle_mmio();
 
     case HVMIO_pio_completion:
+        if ( vio->io_req.type == IOREQ_TYPE_VMWARE_PORT )
+        {
+            vmware_regs_t *vr = get_vmport_regs_any(NULL, v);
+
+            if ( vr )
+            {
+                struct cpu_user_regs *regs = guest_cpu_user_regs();
+
+                /* The code in QEMU that uses these registers,
+                 * vmport.c and vmmouse.c, only uses the 32bit part
+                 * of the register.  This is how VMware defined the
+                 * use of these registers.
+                 */
+                regs->ebx = vr->ebx;
+                regs->ecx = vr->ecx;
+                regs->edx = vr->edx;
+                regs->esi = vr->esi;
+                regs->edi = vr->edi;
+            }
+        }
         return handle_pio(vio->io_req.addr, vio->io_req.size,
                           vio->io_req.dir);
 
@@ -233,16 +291,28 @@ static gfn_t hvm_alloc_legacy_ioreq_gfn(struct hvm_ioreq_server *s)
     unsigned int i;
 
     BUILD_BUG_ON(HVM_PARAM_BUFIOREQ_PFN != HVM_PARAM_IOREQ_PFN + 1);
+    BUILD_BUG_ON(HVM_PARAM_VMPORT_REGS_PFN != HVM_PARAM_BUFIOREQ_PFN + 1);
 
     for ( i = HVM_PARAM_IOREQ_PFN; i <= HVM_PARAM_BUFIOREQ_PFN; i++ )
     {
-        if ( !test_and_clear_bit(i, &d->arch.hvm.ioreq_gfn.legacy_mask) )
+        if ( test_and_clear_bit(i, &d->arch.hvm.ioreq_gfn.legacy_mask) )
             return _gfn(d->arch.hvm.params[i]);
     }
 
     return INVALID_GFN;
 }
 
+static gfn_t hvm_alloc_legacy_vmport_gfn(struct hvm_ioreq_server *s)
+{
+    struct domain *d = s->target;
+    unsigned int i = HVM_PARAM_VMPORT_REGS_PFN;
+
+    if ( test_and_clear_bit(i, &d->arch.hvm.ioreq_gfn.legacy_mask) )
+        return _gfn(d->arch.hvm.params[i]);
+
+    return INVALID_GFN;
+}
+
 static gfn_t hvm_alloc_ioreq_gfn(struct hvm_ioreq_server *s)
 {
     struct domain *d = s->target;
@@ -267,12 +337,12 @@ static bool hvm_free_legacy_ioreq_gfn(struct hvm_ioreq_server *s,
     struct domain *d = s->target;
     unsigned int i;
 
-    for ( i = HVM_PARAM_IOREQ_PFN; i <= HVM_PARAM_BUFIOREQ_PFN; i++ )
+    for ( i = HVM_PARAM_IOREQ_PFN; i <= HVM_PARAM_VMPORT_REGS_PFN; i++ )
     {
         if ( gfn_eq(gfn, _gfn(d->arch.hvm.params[i])) )
              break;
     }
-    if ( i > HVM_PARAM_BUFIOREQ_PFN )
+    if ( i > HVM_PARAM_VMPORT_REGS_PFN )
         return false;
 
     set_bit(i, &d->arch.hvm.ioreq_gfn.legacy_mask);
@@ -293,9 +363,29 @@ static void hvm_free_ioreq_gfn(struct hvm_ioreq_server *s, gfn_t gfn)
     }
 }
 
-static void hvm_unmap_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
+typedef enum {
+    ioreq_pt_ioreq,
+    ioreq_pt_bufioreq,
+    ioreq_pt_vmport,
+} ioreq_pt_;
+
+static void hvm_unmap_ioreq_gfn(struct hvm_ioreq_server *s, ioreq_pt_ pt)
 {
-    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
+    struct hvm_ioreq_page *iorp = NULL;
+
+    switch ( pt )
+    {
+    case ioreq_pt_ioreq:
+        iorp = &s->ioreq;
+        break;
+    case ioreq_pt_bufioreq:
+        iorp = &s->bufioreq;
+        break;
+    case ioreq_pt_vmport:
+        iorp = &s->vmport_ioreq;
+        break;
+    }
+    ASSERT(iorp);
 
     if ( gfn_eq(iorp->gfn, INVALID_GFN) )
         return;
@@ -307,12 +397,26 @@ static void hvm_unmap_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
     iorp->gfn = INVALID_GFN;
 }
 
-static int hvm_map_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
+static int hvm_map_ioreq_gfn(struct hvm_ioreq_server *s, ioreq_pt_ pt)
 {
     struct domain *d = s->target;
-    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
+    struct hvm_ioreq_page *iorp = NULL;
     int rc;
 
+    switch ( pt )
+    {
+    case ioreq_pt_ioreq:
+        iorp = &s->ioreq;
+        break;
+    case ioreq_pt_bufioreq:
+        iorp = &s->bufioreq;
+        break;
+    case ioreq_pt_vmport:
+        iorp = &s->vmport_ioreq;
+        break;
+    }
+    ASSERT(iorp);
+
     if ( iorp->page )
     {
         /*
@@ -329,7 +433,10 @@ static int hvm_map_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
     if ( d->is_dying )
         return -EINVAL;
 
-    iorp->gfn = hvm_alloc_ioreq_gfn(s);
+    if ( pt == ioreq_pt_vmport )
+        iorp->gfn = hvm_alloc_legacy_vmport_gfn(s);
+    else
+        iorp->gfn = hvm_alloc_ioreq_gfn(s);
 
     if ( gfn_eq(iorp->gfn, INVALID_GFN) )
         return -ENOMEM;
@@ -338,16 +445,30 @@ static int hvm_map_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
                                  &iorp->va);
 
     if ( rc )
-        hvm_unmap_ioreq_gfn(s, buf);
+        hvm_unmap_ioreq_gfn(s, pt);
 
     return rc;
 }
 
-static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf)
+static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, ioreq_pt_ pt)
 {
-    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
+    struct hvm_ioreq_page *iorp = NULL;
     struct page_info *page;
 
+    switch ( pt )
+    {
+    case ioreq_pt_ioreq:
+        iorp = &s->ioreq;
+        break;
+    case ioreq_pt_bufioreq:
+        iorp = &s->bufioreq;
+        break;
+    case ioreq_pt_vmport:
+        iorp = &s->vmport_ioreq;
+        break;
+    }
+    ASSERT(iorp);
+
     if ( iorp->page )
     {
         /*
@@ -391,10 +512,25 @@ static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf)
     return -ENOMEM;
 }
 
-static void hvm_free_ioreq_mfn(struct hvm_ioreq_server *s, bool buf)
+static void hvm_free_ioreq_mfn(struct hvm_ioreq_server *s, ioreq_pt_ pt)
 {
-    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
-    struct page_info *page = iorp->page;
+    struct hvm_ioreq_page *iorp = NULL;
+    struct page_info *page = NULL;
+
+    switch ( pt )
+    {
+    case ioreq_pt_ioreq:
+        iorp = &s->ioreq;
+        break;
+    case ioreq_pt_bufioreq:
+        iorp = &s->bufioreq;
+        break;
+    case ioreq_pt_vmport:
+        iorp = &s->vmport_ioreq;
+        break;
+    }
+    ASSERT(iorp);
+    page = iorp->page;
 
     if ( !page )
         return;
@@ -418,7 +554,9 @@ bool is_ioreq_server_page(struct domain *d, const struct page_info *page)
 
     FOR_EACH_IOREQ_SERVER(d, id, s)
     {
-        if ( (s->ioreq.page == page) || (s->bufioreq.page == page) )
+        if ( (s->ioreq.page == page) ||
+             (s->bufioreq.page == page) ||
+             (s->vmport_ioreq.page == page) )
         {
             found = true;
             break;
@@ -430,11 +568,25 @@ bool is_ioreq_server_page(struct domain *d, const struct page_info *page)
     return found;
 }
 
-static void hvm_remove_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
+static void hvm_remove_ioreq_gfn(struct hvm_ioreq_server *s, ioreq_pt_ pt)
 
 {
     struct domain *d = s->target;
-    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
+    struct hvm_ioreq_page *iorp = NULL;
+
+    switch ( pt )
+    {
+    case ioreq_pt_ioreq:
+        iorp = &s->ioreq;
+        break;
+    case ioreq_pt_bufioreq:
+        iorp = &s->bufioreq;
+        break;
+    case ioreq_pt_vmport:
+        iorp = &s->vmport_ioreq;
+        break;
+    }
+    ASSERT(iorp);
 
     if ( gfn_eq(iorp->gfn, INVALID_GFN) )
         return;
@@ -445,12 +597,26 @@ static void hvm_remove_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
     clear_page(iorp->va);
 }
 
-static int hvm_add_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
+static int hvm_add_ioreq_gfn(struct hvm_ioreq_server *s, ioreq_pt_ pt)
 {
     struct domain *d = s->target;
-    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
+    struct hvm_ioreq_page *iorp = NULL;
     int rc;
 
+    switch ( pt )
+    {
+    case ioreq_pt_ioreq:
+        iorp = &s->ioreq;
+        break;
+    case ioreq_pt_bufioreq:
+        iorp = &s->bufioreq;
+        break;
+    case ioreq_pt_vmport:
+        iorp = &s->vmport_ioreq;
+        break;
+    }
+    ASSERT(iorp);
+
     if ( gfn_eq(iorp->gfn, INVALID_GFN) )
         return 0;
 
@@ -480,6 +646,9 @@ static void hvm_update_ioreq_evtchn(struct hvm_ioreq_server *s,
 #define HANDLE_BUFIOREQ(s) \
     ((s)->bufioreq_handling != HVM_IOREQSRV_BUFIOREQ_OFF)
 
+#define HANDLE_VMPORT_IOREQ(s) \
+    ((s)->target->arch.hvm.is_vmware_port_enabled)
+
 static int hvm_ioreq_server_add_vcpu(struct hvm_ioreq_server *s,
                                      struct vcpu *v)
 {
@@ -590,42 +759,72 @@ static int hvm_ioreq_server_map_pages(struct hvm_ioreq_server *s)
 {
     int rc;
 
-    rc = hvm_map_ioreq_gfn(s, false);
+    rc = hvm_map_ioreq_gfn(s, ioreq_pt_ioreq);
 
     if ( !rc && HANDLE_BUFIOREQ(s) )
-        rc = hvm_map_ioreq_gfn(s, true);
+        rc = hvm_map_ioreq_gfn(s, ioreq_pt_bufioreq);
 
     if ( rc )
-        hvm_unmap_ioreq_gfn(s, false);
+    {
+        hvm_unmap_ioreq_gfn(s, ioreq_pt_ioreq);
+        return rc;
+    }
+
+    if ( HANDLE_VMPORT_IOREQ(s) )
+    {
+        rc = hvm_map_ioreq_gfn(s, ioreq_pt_vmport);
+
+        if ( rc )
+        {
+            hvm_unmap_ioreq_gfn(s, ioreq_pt_bufioreq);
+            hvm_unmap_ioreq_gfn(s, ioreq_pt_ioreq);
+        }
+    }
 
     return rc;
 }
 
 static void hvm_ioreq_server_unmap_pages(struct hvm_ioreq_server *s)
 {
-    hvm_unmap_ioreq_gfn(s, true);
-    hvm_unmap_ioreq_gfn(s, false);
+    hvm_unmap_ioreq_gfn(s, ioreq_pt_vmport);
+    hvm_unmap_ioreq_gfn(s, ioreq_pt_bufioreq);
+    hvm_unmap_ioreq_gfn(s, ioreq_pt_ioreq);
 }
 
 static int hvm_ioreq_server_alloc_pages(struct hvm_ioreq_server *s)
 {
     int rc;
 
-    rc = hvm_alloc_ioreq_mfn(s, false);
+    rc = hvm_alloc_ioreq_mfn(s, ioreq_pt_ioreq);
 
-    if ( !rc && (s->bufioreq_handling != HVM_IOREQSRV_BUFIOREQ_OFF) )
-        rc = hvm_alloc_ioreq_mfn(s, true);
+    if ( !rc && HANDLE_BUFIOREQ(s) )
+        rc = hvm_alloc_ioreq_mfn(s, ioreq_pt_bufioreq);
 
     if ( rc )
-        hvm_free_ioreq_mfn(s, false);
+    {
+        hvm_free_ioreq_mfn(s, ioreq_pt_ioreq);
+        return rc;
+    }
+
+    if ( HANDLE_VMPORT_IOREQ(s) )
+    {
+        rc = hvm_alloc_ioreq_mfn(s, ioreq_pt_vmport);
+
+        if ( rc )
+        {
+            hvm_free_ioreq_mfn(s, ioreq_pt_bufioreq);
+            hvm_free_ioreq_mfn(s, ioreq_pt_ioreq);
+        }
+    }
 
     return rc;
 }
 
 static void hvm_ioreq_server_free_pages(struct hvm_ioreq_server *s)
 {
-    hvm_free_ioreq_mfn(s, true);
-    hvm_free_ioreq_mfn(s, false);
+    hvm_free_ioreq_mfn(s, ioreq_pt_vmport);
+    hvm_free_ioreq_mfn(s, ioreq_pt_bufioreq);
+    hvm_free_ioreq_mfn(s, ioreq_pt_ioreq);
 }
 
 static void hvm_ioreq_server_free_rangesets(struct hvm_ioreq_server *s)
@@ -645,12 +844,38 @@ static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s,
     for ( i = 0; i < NR_IO_RANGE_TYPES; i++ )
     {
         char *name;
+        char *type_name = NULL;
+        unsigned int limit;
 
-        rc = asprintf(&name, "ioreq_server %d %s", id,
-                      (i == XEN_DMOP_IO_RANGE_PORT) ? "port" :
-                      (i == XEN_DMOP_IO_RANGE_MEMORY) ? "memory" :
-                      (i == XEN_DMOP_IO_RANGE_PCI) ? "pci" :
-                      "");
+        switch ( i )
+        {
+        case XEN_DMOP_IO_RANGE_PORT:
+            type_name = "port";
+            limit = MAX_NR_IO_RANGES;
+            break;
+        case XEN_DMOP_IO_RANGE_MEMORY:
+            type_name = "memory";
+            limit = MAX_NR_IO_RANGES;
+            break;
+        case XEN_DMOP_IO_RANGE_PCI:
+            type_name = "pci";
+            limit = MAX_NR_IO_RANGES;
+            break;
+        case XEN_DMOP_IO_RANGE_VMWARE_PORT:
+            type_name = "VMware port";
+            limit = 1;
+            break;
+        case XEN_DMOP_IO_RANGE_TIMEOFFSET:
+            type_name = "timeoffset";
+            limit = 1;
+            break;
+        default:
+            break;
+        }
+        if ( !type_name )
+            continue;
+
+        rc = asprintf(&name, "ioreq_server %d %s", id, type_name);
         if ( rc )
             goto fail;
 
@@ -663,7 +888,11 @@ static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s,
         if ( !s->range[i] )
             goto fail;
 
-        rangeset_limit(s->range[i], MAX_NR_IO_RANGES);
+        rangeset_limit(s->range[i], limit);
+
+        /* VMware port */
+        if ( i == XEN_DMOP_IO_RANGE_VMWARE_PORT && s->vmport_enabled )
+            rc = rangeset_add_range(s->range[i], 1, 1);
     }
 
     return 0;
@@ -683,8 +912,9 @@ static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s)
     if ( s->enabled )
         goto done;
 
-    hvm_remove_ioreq_gfn(s, false);
-    hvm_remove_ioreq_gfn(s, true);
+    hvm_remove_ioreq_gfn(s, ioreq_pt_vmport);
+    hvm_remove_ioreq_gfn(s, ioreq_pt_bufioreq);
+    hvm_remove_ioreq_gfn(s, ioreq_pt_ioreq);
 
     s->enabled = true;
 
@@ -704,8 +934,9 @@ static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s)
     if ( !s->enabled )
         goto done;
 
-    hvm_add_ioreq_gfn(s, true);
-    hvm_add_ioreq_gfn(s, false);
+    hvm_add_ioreq_gfn(s, ioreq_pt_vmport);
+    hvm_add_ioreq_gfn(s, ioreq_pt_bufioreq);
+    hvm_add_ioreq_gfn(s, ioreq_pt_ioreq);
 
     s->enabled = false;
 
@@ -714,7 +945,7 @@ static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s)
 }
 
 static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
-                                 struct domain *d, int bufioreq_handling,
+                                 struct domain *d, int flags,
                                  ioservid_t id)
 {
     struct domain *currd = current->domain;
@@ -730,14 +961,18 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
     INIT_LIST_HEAD(&s->ioreq_vcpu_list);
     spin_lock_init(&s->bufioreq_lock);
 
+    s->vmport_enabled = d->arch.hvm.is_vmware_port_enabled &&
+        !(flags & HVM_IOREQSRV_DISABLE_VMPORT);
+
     s->ioreq.gfn = INVALID_GFN;
     s->bufioreq.gfn = INVALID_GFN;
+    s->vmport_ioreq.gfn = INVALID_GFN;
 
     rc = hvm_ioreq_server_alloc_rangesets(s, id);
     if ( rc )
         return rc;
 
-    s->bufioreq_handling = bufioreq_handling;
+    s->bufioreq_handling = flags & HVM_IOREQSRV_BUFIOREQ_MASK;
 
     for_each_vcpu ( d, v )
     {
@@ -780,14 +1015,15 @@ static void hvm_ioreq_server_deinit(struct hvm_ioreq_server *s)
     put_domain(s->emulator);
 }
 
-int hvm_create_ioreq_server(struct domain *d, int bufioreq_handling,
+int hvm_create_ioreq_server(struct domain *d, int flags,
                             ioservid_t *id)
 {
     struct hvm_ioreq_server *s;
     unsigned int i;
     int rc;
 
-    if ( bufioreq_handling > HVM_IOREQSRV_BUFIOREQ_ATOMIC )
+    if ( flags & ~HVM_IOREQSRV_FLAGS_MASK ||
+         (flags & HVM_IOREQSRV_BUFIOREQ_MASK) > HVM_IOREQSRV_BUFIOREQ_ATOMIC )
         return -EINVAL;
 
     s = xzalloc(struct hvm_ioreq_server);
@@ -813,7 +1049,7 @@ int hvm_create_ioreq_server(struct domain *d, int bufioreq_handling,
      */
     set_ioreq_server(d, i, s);
 
-    rc = hvm_ioreq_server_init(s, d, bufioreq_handling, i);
+    rc = hvm_ioreq_server_init(s, d, flags, i);
     if ( rc )
     {
         set_ioreq_server(d, i, NULL);
@@ -1004,6 +1240,8 @@ int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id,
     case XEN_DMOP_IO_RANGE_PORT:
     case XEN_DMOP_IO_RANGE_MEMORY:
     case XEN_DMOP_IO_RANGE_PCI:
+    case XEN_DMOP_IO_RANGE_TIMEOFFSET:
+    case XEN_DMOP_IO_RANGE_VMWARE_PORT:
         r = s->range[type];
         break;
 
@@ -1056,6 +1294,8 @@ int hvm_unmap_io_range_from_ioreq_server(struct domain *d, ioservid_t id,
     case XEN_DMOP_IO_RANGE_PORT:
     case XEN_DMOP_IO_RANGE_MEMORY:
     case XEN_DMOP_IO_RANGE_PCI:
+    case XEN_DMOP_IO_RANGE_TIMEOFFSET:
+    case XEN_DMOP_IO_RANGE_VMWARE_PORT:
         r = s->range[type];
         break;
 
@@ -1248,7 +1488,10 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
     uint64_t addr;
     unsigned int id;
 
-    if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
+    if ( p->type != IOREQ_TYPE_COPY &&
+         p->type != IOREQ_TYPE_PIO &&
+         p->type != IOREQ_TYPE_VMWARE_PORT &&
+         p->type != IOREQ_TYPE_TIMEOFFSET )
         return NULL;
 
     cf8 = d->arch.hvm.pci_cf8;
@@ -1282,8 +1525,9 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
     }
     else
     {
-        type = (p->type == IOREQ_TYPE_PIO) ?
-                XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY;
+        type = (p->type == IOREQ_TYPE_PIO) ? XEN_DMOP_IO_RANGE_PORT : 
+            (p->type == IOREQ_TYPE_VMWARE_PORT) ? XEN_DMOP_IO_RANGE_VMWARE_PORT :
+            XEN_DMOP_IO_RANGE_MEMORY;
         addr = p->addr;
     }
 
@@ -1326,6 +1570,14 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
             }
 
             break;
+
+        case XEN_DMOP_IO_RANGE_VMWARE_PORT:
+        case XEN_DMOP_IO_RANGE_TIMEOFFSET:
+            /* The 'special' range of [1,1] is checked for being enabled. */
+            if ( rangeset_contains_singleton(r, 1) )
+                return s;
+
+            break;
         }
     }
 
diff --git a/xen/arch/x86/hvm/vmware/vmport.c b/xen/arch/x86/hvm/vmware/vmport.c
index 863ec50..3c2e7d5 100644
--- a/xen/arch/x86/hvm/vmware/vmport.c
+++ b/xen/arch/x86/hvm/vmware/vmport.c
@@ -14,6 +14,7 @@
  */
 
 #include <xen/lib.h>
+#include <asm/mc146818rtc.h>
 #include <asm/hvm/hvm.h>
 #include <asm/hvm/support.h>
 
@@ -23,6 +24,32 @@ static int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
 {
     struct cpu_user_regs *regs = guest_cpu_user_regs();
 
+#define port_overlap(p, n) \
+    ((p + n > BDOOR_PORT) && (p + n <= BDOOR_PORT + 4) ? 1 : \
+    (BDOOR_PORT + 4 > p) && (BDOOR_PORT + 4 <= p + n) ? 1 : 0)
+
+    BUILD_BUG_ON(port_overlap(PIT_BASE, 4));
+    BUILD_BUG_ON(port_overlap(0x61, 1));
+    BUILD_BUG_ON(port_overlap(XEN_HVM_DEBUGCONS_IOPORT, 1));
+    BUILD_BUG_ON(port_overlap(0xcf8, 4));
+/* #define TMR_VAL_ADDR_V0  (ACPI_PM_TMR_BLK_ADDRESS_V0) */
+    BUILD_BUG_ON(port_overlap(ACPI_PM_TMR_BLK_ADDRESS_V0, 4));
+/* #define PM1a_STS_ADDR_V0 (ACPI_PM1A_EVT_BLK_ADDRESS_V0) */
+    BUILD_BUG_ON(port_overlap(ACPI_PM1A_EVT_BLK_ADDRESS_V0, 4));
+    BUILD_BUG_ON(port_overlap(RTC_PORT(0), 2));
+    BUILD_BUG_ON(port_overlap(0x3c4, 2));
+    BUILD_BUG_ON(port_overlap(0x3ce, 2));
+/*
+ * acpi_smi_cmd can not be checked at build time:
+ *   xen/include/asm-x86/acpi.h:extern u32 acpi_smi_cmd;
+ *   xen/arch/x86/acpi/boot.c: acpi_smi_cmd = fadt->smi_command;
+ BUILD_BUG_ON(port_overlap(acpi_smi_cmd, 1));
+*/
+    BUILD_BUG_ON(port_overlap(0x20, 2));
+    BUILD_BUG_ON(port_overlap(0xa0, 2));
+    BUILD_BUG_ON(port_overlap(0x4d0, 1));
+    BUILD_BUG_ON(port_overlap(0x4d1, 1));
+
     /*
      * While VMware expects only 32-bit in, they do support using
      * other sizes and out.  However they do require only the 1 port
@@ -137,6 +164,15 @@ void vmport_register(struct domain *d)
     register_portio_handler(d, BDOOR_PORT, 4, vmport_ioport);
 }
 
+bool_t vmport_check_port(unsigned int port, unsigned int bytes)
+{
+    struct domain *currd = current->domain;
+
+    return is_hvm_domain(currd) &&
+           currd->arch.hvm.is_vmware_port_enabled &&
+           (port >= BDOOR_PORT) && ((port + bytes) <= (BDOOR_PORT + 4));
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 355777c..cdfa62e 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -43,7 +43,7 @@ struct hvm_ioreq_vcpu {
     bool             pending;
 };
 
-#define NR_IO_RANGE_TYPES (XEN_DMOP_IO_RANGE_PCI + 1)
+#define NR_IO_RANGE_TYPES (XEN_DMOP_IO_RANGE_VMWARE_PORT + 1)
 #define MAX_NR_IO_RANGES  256
 
 struct hvm_ioreq_server {
@@ -54,6 +54,7 @@ struct hvm_ioreq_server {
 
     struct hvm_ioreq_page  ioreq;
     struct list_head       ioreq_vcpu_list;
+    struct hvm_ioreq_page  vmport_ioreq;
     struct hvm_ioreq_page  bufioreq;
 
     /* Lock to serialize access to buffered ioreq ring */
@@ -62,6 +63,7 @@ struct hvm_ioreq_server {
     struct rangeset        *range[NR_IO_RANGE_TYPES];
     bool                   enabled;
     uint8_t                bufioreq_handling;
+    bool_t                 vmport_enabled;
 };
 
 #ifdef CONFIG_MEM_SHARING
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 74a9590..48bcc98 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -790,6 +790,7 @@ static inline bool hvm_has_set_descriptor_access_exiting(void)
 #endif  /* CONFIG_HVM */
 
 void vmport_register(struct domain *d);
+bool_t vmport_check_port(unsigned int port, unsigned int bytes);
 
 #endif /* __ASM_X86_HVM_HVM_H__ */
 
diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
index fd00e9d..039b9e4 100644
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -60,16 +60,24 @@ typedef uint16_t ioservid_t;
  *                               secondary emulator.
  *
  * The <id> handed back is unique for target domain. The valur of
- * <handle_bufioreq> should be one of HVM_IOREQSRV_BUFIOREQ_* defined in
- * hvm_op.h. If the value is HVM_IOREQSRV_BUFIOREQ_OFF then  the buffered
+ * <flags> should be one of HVM_IOREQSRV_BUFIOREQ_* defined in
+ * hvm_op.h and ored with HVM_IOREQSRV_DISABLE_VMPORT defined below
+ * if not the 1st IOREQ Server that supports VMware port operation.
+ * If the value is HVM_IOREQSRV_BUFIOREQ_OFF then the buffered
  * ioreq ring will not be allocated and hence all emulation requests to
  * this server will be synchronous.
  */
 #define XEN_DMOP_create_ioreq_server 1
 
 struct xen_dm_op_create_ioreq_server {
-    /* IN - should server handle buffered ioreqs */
-    uint8_t handle_bufioreq;
+    /* IN - should server handle buffered ioreqs and/or vmport regs */
+#define HVM_IOREQSRV_BUFIOREQ_MASK   3
+/*
+ * Disable vmport regs mapping.
+ */
+#define HVM_IOREQSRV_DISABLE_VMPORT  4
+#define HVM_IOREQSRV_FLAGS_MASK      7
+    uint8_t flags;
     uint8_t pad[3];
     /* OUT - server id */
     ioservid_t id;
@@ -132,6 +140,9 @@ struct xen_dm_op_get_ioreq_server_info {
  *
  * NOTE: unless an emulation request falls entirely within a range mapped
  * by a secondary emulator, it will not be passed to that emulator.
+ *
+ * NOTE: The 'special' range of [1,1] is what is checked for on
+ * TIMEOFFSET and VMWARE_PORT.
  */
 #define XEN_DMOP_map_io_range_to_ioreq_server 3
 #define XEN_DMOP_unmap_io_range_from_ioreq_server 4
@@ -145,6 +156,8 @@ struct xen_dm_op_ioreq_server_range {
 # define XEN_DMOP_IO_RANGE_PORT   0 /* I/O port range */
 # define XEN_DMOP_IO_RANGE_MEMORY 1 /* MMIO range */
 # define XEN_DMOP_IO_RANGE_PCI    2 /* PCI segment/bus/dev/func range */
+# define XEN_DMOP_IO_RANGE_TIMEOFFSET 7 /* TIMEOFFSET special range */
+# define XEN_DMOP_IO_RANGE_VMWARE_PORT 9 /* VMware port special range */
     /* IN - inclusive start and end of range */
     uint64_aligned_t start, end;
 };
diff --git a/xen/include/public/hvm/ioreq.h b/xen/include/public/hvm/ioreq.h
index c511fae..19a17ac 100644
--- a/xen/include/public/hvm/ioreq.h
+++ b/xen/include/public/hvm/ioreq.h
@@ -37,6 +37,7 @@
 #define IOREQ_TYPE_PCI_CONFIG   2
 #define IOREQ_TYPE_TIMEOFFSET   7
 #define IOREQ_TYPE_INVALIDATE   8 /* mapcache */
+#define IOREQ_TYPE_VMWARE_PORT  9 /* pio + vmport registers */
 
 /*
  * VMExit dispatcher should cooperate with instruction decoder to
@@ -48,6 +49,8 @@
  *
  * 63....48|47..40|39..35|34..32|31........0
  * SEGMENT |BUS   |DEV   |FN    |OFFSET
+ *
+ * For I/O type IOREQ_TYPE_VMWARE_PORT also use the vmware_regs.
  */
 struct ioreq {
     uint64_t addr;          /* physical address */
@@ -66,11 +69,25 @@ struct ioreq {
 };
 typedef struct ioreq ioreq_t;
 
+struct vmware_regs {
+    uint32_t esi;
+    uint32_t edi;
+    uint32_t ebx;
+    uint32_t ecx;
+    uint32_t edx;
+};
+typedef struct vmware_regs vmware_regs_t;
+
 struct shared_iopage {
     struct ioreq vcpu_ioreq[1];
 };
 typedef struct shared_iopage shared_iopage_t;
 
+struct shared_vmport_iopage {
+    struct vmware_regs vcpu_vmport_regs[1];
+};
+typedef struct shared_vmport_iopage shared_vmport_iopage_t;
+
 struct buf_ioreq {
     uint8_t  type;   /* I/O type                    */
     uint8_t  pad:1;
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 0a91bfa..d300fae 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -94,8 +94,8 @@
 #define HVM_PARAM_STORE_EVTCHN 2
 
 #define HVM_PARAM_IOREQ_PFN    5
-
 #define HVM_PARAM_BUFIOREQ_PFN 6
+#define HVM_PARAM_VMPORT_REGS_PFN 7
 
 #if defined(__i386__) || defined(__x86_64__)
 
-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Xen-devel] [XEN PATCH v14 8/8] Add xentrace to vmware_port
  2020-08-19 16:52             ` [Xen-devel] [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT Don Slutz
@ 2020-08-19 16:52               ` Don Slutz
  2020-10-16  9:32                 ` Jan Beulich
  2020-10-01 14:41               ` [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT Jan Beulich
  1 sibling, 1 reply; 16+ messages in thread
From: Don Slutz @ 2020-08-19 16:52 UTC (permalink / raw)
  To: xen-devel
  Cc: Aravind Gopalakrishnan, Boris Ostrovsky, Eddie Dong,
	Ian Campbell, Ian Jackson, Jan Beulich, Jun Nakajima,
	Keir Fraser, Kevin Tian, Stefano Stabellini,
	Suravee Suthikulpanit, Tim Deegan, Andrew Cooper,
	Konrad Rzeszutek Wilk, George Dunlap, Don Slutz, Don Slutz

From: Don Slutz <dslutz@verizon.com>

Also added missing TRAP_DEBUG & VLAPIC.

Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <don.slutz@gmail.com>
---
Acked-by: Ian Campbell <ian.campbell@citrix.com>

v14:
  Reworked to current code.
  Added VMPORT_SEND because I wanted to see it during testing.

v13:
    Please do this by extending the existing infrastructure rather
    than special-casing 7 on the side.  (i.e. extend ND to take 7
    parameters, and introduce HVMTRACE_7D)
    = { d1, d2, d3, d4, d5, d6, d7 } will be far shorter, linewise.

v12:
  Switch VMPORT_IGNORED to port, regs->_eax.

v11:
  No change

v10:
  Added Acked-by: Ian Campbell
  Added back in the trace point calls.

    Why is cmd in this patch?
      Because the trace points use it.

v9:
  Dropped unneed VMPORT_UNHANDLED, VMPORT_DECODE.

v7:
      Dropped some of the new traces.
      Added HVMTRACE_ND7.

v6:
      Dropped the attempt to use svm_nextrip_insn_length via
      __get_instruction_length (added in v2).  Just always look
      at upto 15 bytes on AMD.

v5:
      exitinfo1 is used twice.
        Fixed.

 tools/xentrace/formats           |  6 ++++
 xen/arch/x86/hvm/ioreq.c         |  3 ++
 xen/arch/x86/hvm/svm/svm.c       |  6 ++--
 xen/arch/x86/hvm/vmware/vmport.c | 16 +++++++++--
 xen/arch/x86/hvm/vmx/vmx.c       |  6 ++--
 xen/include/asm-x86/hvm/trace.h  | 60 ++++++++++++++++++++--------------------
 xen/include/public/trace.h       |  4 +++
 7 files changed, 62 insertions(+), 39 deletions(-)

diff --git a/tools/xentrace/formats b/tools/xentrace/formats
index deac4d8..9e7056c 100644
--- a/tools/xentrace/formats
+++ b/tools/xentrace/formats
@@ -132,6 +132,12 @@
 0x00082020  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  INTR_WINDOW [ value = 0x%(1)08x ]
 0x00082021  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  NPF         [ gpa = 0x%(2)08x%(1)08x mfn = 0x%(4)08x%(3)08x qual = 0x%(5)04x p2mt = 0x%(6)04x ]
 0x00082023  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  TRAP        [ vector = 0x%(1)02x ]
+0x00082024  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  TRAP_DEBUG  [ exit_qualification = 0x%(1)08x ]
+0x00082025  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  VLAPIC
+0x00082028  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  VMPORT_HANDLED   [ cmd = %(1)d eax = 0x%(2)08x ebx = 0x%(3)08x ecx = 0x%(4)08x edx = 0x%(5)08x esi = 0x%(6)08x edi = 0x%(7)08x ]
+0x00082029  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  VMPORT_IGNORED   [ port = %(1)d eax = 0x%(2)08x ]
+0x0008202a  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  VMPORT_QEMU      [ eax = 0x%(1)08x ebx = 0x%(2)08x ecx = 0x%(3)08x edx = 0x%(4)08x esi = 0x%(5)08x edi = 0x%(6)08x ]
+0x0008202b  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  VMPORT_SEND      [ cmd = 0x%(1)08x ebx = 0x%(2)08x ecx = 0x%(3)08x edx = 0x%(4)08x esi = 0x%(5)08x edi = 0x%(6)08x ]
 
 0x0010f001  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  page_grant_map      [ domid = %(1)d ]
 0x0010f002  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  page_grant_unmap    [ domid = %(1)d ]
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index a5d124e..c64fda8 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -262,6 +262,9 @@ bool handle_hvm_io_completion(struct vcpu *v)
                 regs->edx = vr->edx;
                 regs->esi = vr->esi;
                 regs->edi = vr->edi;
+                HVMTRACE_ND(VMPORT_QEMU, 0, 1/*cycles*/, 6,
+                            vio->io_req.data, regs->ebx, regs->ecx,
+                            regs->edx, regs->esi, regs->edi, 0);
             }
         }
         return handle_pio(vio->io_req.addr, vio->io_req.size,
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index ca3bbfc..60c030a 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1052,7 +1052,7 @@ void svm_vmenter_helper(const struct cpu_user_regs *regs)
     if ( unlikely(tb_init_done) )
         HVMTRACE_ND(VMENTRY,
                     nestedhvm_vcpu_in_guestmode(curr) ? TRC_HVM_NESTEDFLAG : 0,
-                    1/*cycles*/, 0, 0, 0, 0, 0, 0, 0);
+                    1/*cycles*/, 0, 0, 0, 0, 0, 0, 0, 0);
 
     svm_sync_vmcb(curr, vmcb_needs_vmsave);
 
@@ -2531,11 +2531,11 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
     if ( hvm_long_mode_active(v) )
         HVMTRACE_ND(VMEXIT64, vcpu_guestmode ? TRC_HVM_NESTEDFLAG : 0,
                     1/*cycles*/, 3, exit_reason,
-                    regs->eip, regs->rip >> 32, 0, 0, 0);
+                    regs->eip, regs->rip >> 32, 0, 0, 0, 0);
     else
         HVMTRACE_ND(VMEXIT, vcpu_guestmode ? TRC_HVM_NESTEDFLAG : 0,
                     1/*cycles*/, 2, exit_reason,
-                    regs->eip, 0, 0, 0, 0);
+                    regs->eip, 0, 0, 0, 0, 0);
 
     if ( vcpu_guestmode )
     {
diff --git a/xen/arch/x86/hvm/vmware/vmport.c b/xen/arch/x86/hvm/vmware/vmport.c
index 3c2e7d5..0467aff 100644
--- a/xen/arch/x86/hvm/vmware/vmport.c
+++ b/xen/arch/x86/hvm/vmware/vmport.c
@@ -17,6 +17,7 @@
 #include <asm/mc146818rtc.h>
 #include <asm/hvm/hvm.h>
 #include <asm/hvm/support.h>
+#include <asm/hvm/trace.h>
 
 #include "backdoor_def.h"
 
@@ -62,6 +63,7 @@ static int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
     if ( port == BDOOR_PORT && regs->eax == BDOOR_MAGIC )
     {
         uint32_t new_eax = ~0u;
+        uint16_t cmd = regs->ecx;
         uint64_t value;
         struct vcpu *curr = current;
         struct domain *currd = curr->domain;
@@ -72,7 +74,7 @@ static int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
          * leaving the high 32-bits unchanged, unlike what one would
          * expect to happen.
          */
-        switch ( regs->ecx & 0xffff )
+        switch ( cmd )
         {
         case BDOOR_CMD_GETMHZ:
             new_eax = currd->arch.tsc_khz / 1000;
@@ -147,14 +149,22 @@ static int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
             break;
 
         default:
+            HVMTRACE_6D(VMPORT_SEND, cmd, regs->ebx, regs->ecx,
+                        regs->edx, regs->esi, regs->edi);
             /* Let backing DM handle */
             return X86EMUL_UNHANDLEABLE;
         }
+        HVMTRACE_7D(VMPORT_HANDLED, cmd, new_eax, regs->ebx, regs->ecx,
+                    regs->edx, regs->esi, regs->edi);
         if ( dir == IOREQ_READ )
             *val = new_eax;
     }
-    else if ( dir == IOREQ_READ )
-        *val = ~0u;
+    else
+    {
+        HVMTRACE_2D(VMPORT_IGNORED, port, regs->eax);
+        if ( dir == IOREQ_READ )
+            *val = ~0u;
+    }
 
     return X86EMUL_OKAY;
 }
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index a0d58ff..8cba2a3 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3683,10 +3683,10 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
 
     if ( hvm_long_mode_active(v) )
         HVMTRACE_ND(VMEXIT64, 0, 1/*cycles*/, 3, exit_reason,
-                    regs->eip, regs->rip >> 32, 0, 0, 0);
+                    regs->eip, regs->rip >> 32, 0, 0, 0, 0);
     else
         HVMTRACE_ND(VMEXIT, 0, 1/*cycles*/, 2, exit_reason,
-                    regs->eip, 0, 0, 0, 0);
+                    regs->eip, 0, 0, 0, 0, 0);
 
     perfc_incra(vmexits, exit_reason);
 
@@ -4463,7 +4463,7 @@ bool vmx_vmenter_helper(const struct cpu_user_regs *regs)
     if ( unlikely(curr->arch.hvm.vmx.lbr_flags & LBR_FIXUP_MASK) )
         lbr_fixup();
 
-    HVMTRACE_ND(VMENTRY, 0, 1/*cycles*/, 0, 0, 0, 0, 0, 0, 0);
+    HVMTRACE_ND(VMENTRY, 0, 1/*cycles*/, 0, 0, 0, 0, 0, 0, 0, 0);
 
     __vmwrite(GUEST_RIP,    regs->rip);
     __vmwrite(GUEST_RSP,    regs->rsp);
diff --git a/xen/include/asm-x86/hvm/trace.h b/xen/include/asm-x86/hvm/trace.h
index 5cd459b..9d7f04e 100644
--- a/xen/include/asm-x86/hvm/trace.h
+++ b/xen/include/asm-x86/hvm/trace.h
@@ -56,6 +56,10 @@
 #define DO_TRC_HVM_TRAP             DEFAULT_HVM_MISC
 #define DO_TRC_HVM_TRAP_DEBUG       DEFAULT_HVM_MISC
 #define DO_TRC_HVM_VLAPIC           DEFAULT_HVM_MISC
+#define DO_TRC_HVM_VMPORT_HANDLED   DEFAULT_HVM_IO
+#define DO_TRC_HVM_VMPORT_IGNORED   DEFAULT_HVM_IO
+#define DO_TRC_HVM_VMPORT_QEMU      DEFAULT_HVM_IO
+#define DO_TRC_HVM_VMPORT_SEND      DEFAULT_HVM_IO
 
 
 #define TRC_PAR_LONG(par) ((par)&0xFFFFFFFF),((par)>>32)
@@ -67,38 +71,34 @@
 #define TRACE_2_LONG_4D(_e, d1, d2, d3, d4, ...) \
     TRACE_6D(_e, d1, d2, d3, d4)
 
-#define HVMTRACE_ND(evt, modifier, cycles, count, d1, d2, d3, d4, d5, d6) \
-    do {                                                                  \
-        if ( unlikely(tb_init_done) && DO_TRC_HVM_ ## evt )               \
-        {                                                                 \
-            struct {                                                      \
-                u32 d[6];                                                 \
-            } _d;                                                         \
-            _d.d[0]=(d1);                                                 \
-            _d.d[1]=(d2);                                                 \
-            _d.d[2]=(d3);                                                 \
-            _d.d[3]=(d4);                                                 \
-            _d.d[4]=(d5);                                                 \
-            _d.d[5]=(d6);                                                 \
-            __trace_var(TRC_HVM_ ## evt | (modifier), cycles,             \
-                        sizeof(*_d.d) * count, &_d);                      \
-        }                                                                 \
+#define HVMTRACE_ND(evt, modifier, cycles, count, d1, d2, d3, d4, d5, d6, d7) \
+    do {                                                                      \
+        if ( unlikely(tb_init_done) && DO_TRC_HVM_ ## evt )                   \
+        {                                                                     \
+            struct {                                                          \
+                u32 d[7];                                                     \
+            } _d = { { d1, d2, d3, d4, d5, d6, d7 } };                        \
+            __trace_var(TRC_HVM_ ## evt | (modifier), cycles,                 \
+                        sizeof(*_d.d) * count, &_d);                          \
+        }                                                                     \
     } while(0)
 
-#define HVMTRACE_6D(evt, d1, d2, d3, d4, d5, d6)    \
-    HVMTRACE_ND(evt, 0, 0, 6, d1, d2, d3, d4, d5, d6)
-#define HVMTRACE_5D(evt, d1, d2, d3, d4, d5)        \
-    HVMTRACE_ND(evt, 0, 0, 5, d1, d2, d3, d4, d5,  0)
-#define HVMTRACE_4D(evt, d1, d2, d3, d4)            \
-    HVMTRACE_ND(evt, 0, 0, 4, d1, d2, d3, d4,  0,  0)
-#define HVMTRACE_3D(evt, d1, d2, d3)                \
-    HVMTRACE_ND(evt, 0, 0, 3, d1, d2, d3,  0,  0,  0)
-#define HVMTRACE_2D(evt, d1, d2)                    \
-    HVMTRACE_ND(evt, 0, 0, 2, d1, d2,  0,  0,  0,  0)
-#define HVMTRACE_1D(evt, d1)                        \
-    HVMTRACE_ND(evt, 0, 0, 1, d1,  0,  0,  0,  0,  0)
-#define HVMTRACE_0D(evt)                            \
-    HVMTRACE_ND(evt, 0, 0, 0,  0,  0,  0,  0,  0,  0)
+#define HVMTRACE_7D(evt, d1, d2, d3, d4, d5, d6, d7) \
+    HVMTRACE_ND(evt, 0, 0, 7, d1, d2, d3, d4, d5, d6, d7)
+#define HVMTRACE_6D(evt, d1, d2, d3, d4, d5, d6)     \
+    HVMTRACE_ND(evt, 0, 0, 6, d1, d2, d3, d4, d5, d6, 0)
+#define HVMTRACE_5D(evt, d1, d2, d3, d4, d5)         \
+    HVMTRACE_ND(evt, 0, 0, 5, d1, d2, d3, d4, d5,  0, 0)
+#define HVMTRACE_4D(evt, d1, d2, d3, d4)             \
+    HVMTRACE_ND(evt, 0, 0, 4, d1, d2, d3, d4,  0,  0, 0)
+#define HVMTRACE_3D(evt, d1, d2, d3)                 \
+    HVMTRACE_ND(evt, 0, 0, 3, d1, d2, d3,  0,  0,  0, 0)
+#define HVMTRACE_2D(evt, d1, d2)                     \
+    HVMTRACE_ND(evt, 0, 0, 2, d1, d2,  0,  0,  0,  0, 0)
+#define HVMTRACE_1D(evt, d1)                         \
+    HVMTRACE_ND(evt, 0, 0, 1, d1,  0,  0,  0,  0,  0, 0)
+#define HVMTRACE_0D(evt)                             \
+    HVMTRACE_ND(evt, 0, 0, 0,  0,  0,  0,  0,  0,  0, 0)
 
 #define HVMTRACE_LONG_1D(evt, d1)                  \
                    HVMTRACE_2D(evt ## 64, (d1) & 0xFFFFFFFF, (d1) >> 32)
diff --git a/xen/include/public/trace.h b/xen/include/public/trace.h
index d5fa4ae..6b93f62 100644
--- a/xen/include/public/trace.h
+++ b/xen/include/public/trace.h
@@ -237,6 +237,10 @@
 #define TRC_HVM_VLAPIC           (TRC_HVM_HANDLER + 0x25)
 #define TRC_HVM_XCR_READ64      (TRC_HVM_HANDLER + TRC_64_FLAG + 0x26)
 #define TRC_HVM_XCR_WRITE64     (TRC_HVM_HANDLER + TRC_64_FLAG + 0x27)
+#define TRC_HVM_VMPORT_HANDLED  (TRC_HVM_HANDLER + 0x28)
+#define TRC_HVM_VMPORT_IGNORED  (TRC_HVM_HANDLER + 0x29)
+#define TRC_HVM_VMPORT_QEMU     (TRC_HVM_HANDLER + 0x2a)
+#define TRC_HVM_VMPORT_SEND     (TRC_HVM_HANDLER + 0x2b)
 
 #define TRC_HVM_IOPORT_WRITE    (TRC_HVM_HANDLER + 0x216)
 #define TRC_HVM_IOMEM_WRITE     (TRC_HVM_HANDLER + 0x217)
-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [XEN PATCH v14 2/8] xen: Add support for VMware cpuid leaves
  2020-08-19 16:51   ` [Xen-devel] [XEN PATCH v14 2/8] xen: Add support for VMware cpuid leaves Don Slutz
  2020-08-19 16:51     ` [Xen-devel] [XEN PATCH v14 3/8] tools: Add vmware_hwver support Don Slutz
@ 2020-09-30 14:24     ` Jan Beulich
  1 sibling, 0 replies; 16+ messages in thread
From: Jan Beulich @ 2020-09-30 14:24 UTC (permalink / raw)
  To: Don Slutz
  Cc: xen-devel, Aravind Gopalakrishnan, Boris Ostrovsky, Ian Jackson,
	Jun Nakajima, Kevin Tian, Stefano Stabellini, Tim Deegan,
	Andrew Cooper, Konrad Rzeszutek Wilk, George Dunlap

On 19.08.2020 18:51, Don Slutz wrote:
> Since I need to change xen/arch/x86/hvm/Makefile; also add
> a newline at end of file.

Should this have been removed?

Also please update / trim your Cc list. I've dropped / replaced a
number of entries which I'm sure would have bounced.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -597,6 +597,11 @@ int arch_domain_create(struct domain *d,
>      }
>      d->arch.emulation_flags = emflags;
>  
> +    if ( is_hvm_domain(d) )
> +    {
> +        d->arch.hvm.vmware_hwver = config->arch.vmware_hwver;
> +    }

As per the description it's not like any value is okay. Shouldn't
you refuse bad values in arch_sanitise_domain_config()?

Also please drop the unnecessary braces.

> --- a/xen/arch/x86/hvm/Makefile
> +++ b/xen/arch/x86/hvm/Makefile
> @@ -1,6 +1,7 @@
>  obj-y += svm/
>  obj-y += vmx/
>  obj-y += viridian/
> +obj-y += vmware/

Generally we try to sort such lists alphabetically. I realize a
mistake was already made when Viridian gained its own subdir, but
please don't widen the issue.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -68,6 +68,7 @@
>  #include <asm/hvm/viridian.h>
>  #include <asm/hvm/vm_event.h>
>  #include <asm/altp2m.h>
> +#include <asm/hvm/vmware.h>

Like above, please try to honor (partial) sorting in #include-s
as well.

> @@ -4109,6 +4110,13 @@ static int hvm_allow_set_param(struct domain *d,
>      {
>      /* The following parameters should only be changed once. */
>      case HVM_PARAM_VIRIDIAN:
> +        /* Disallow if vmware_hwver is in use */
> +        if ( d->arch.hvm.vmware_hwver )
> +        {
> +            rc = -EOPNOTSUPP;
> +            break;
> +        }
> +        /* Fall through */

Afaic the comment is too redundant with the code. If at least it
wouldn't name the field name, but say e.g. "VMware emulation",
things would already be better. Using something like "can't
coexist" instead of "disallow" may further improve usefulness.

> --- /dev/null
> +++ b/xen/arch/x86/hvm/vmware/vmware.c
> @@ -0,0 +1,82 @@
> +/*
> + * arch/x86/hvm/vmware/cpuid.c
> + *
> + * Copyright (C) 2012-2015 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 <xen/version.h>
> +#include <xen/hypercall.h>
> +#include <xen/domain_page.h>
> +#include <xen/param.h>
> +#include <asm/guest_access.h>
> +#include <asm/guest/hyperv-tlfs.h>
> +#include <asm/paging.h>
> +#include <asm/p2m.h>
> +#include <asm/apic.h>
> +#include <asm/hvm/support.h>
> +#include <public/sched.h>
> +#include <public/hvm/hvm_op.h>

Please sort each sub-section alphabetically, and please remove
ones you don't really need - the list looks surprisingly long for
just CPUID handling.

> +/*
> + * VMware hardware version 7 defines some of these cpuid levels,
> + * below is a brief description about those.
> + *
> + *     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
> + */
> +
> +void cpuid_vmware_leaves(const struct vcpu *v, uint32_t leaf,
> +                         uint32_t subleaf, struct cpuid_leaf *res)
> +{
> +    struct domain *d = current->domain;

Surely v->domain, and please add const.

> +    ASSERT(has_vmware_cpuid(d));
> +    ASSERT(leaf >= 0x40000000 && leaf < 0x40000100);

What earlier check guarantees this?

> +    leaf -= 0x40000000;
> +
> +    switch ( leaf )
> +    {
> +    case 0x0:
> +        res->a = 0x40000010; /* Maximum leaf */
> +        memcpy(&res->b, "VMwa", 4);
> +        memcpy(&res->c, "reVM", 4);
> +        memcpy(&res->d, "ware", 4);
> +        break;
> +
> +    case 0x10:
> +        /* (Virtual) TSC frequency in kHz. */
> +        res->a = d->arch.tsc_khz;
> +        /* (Virtual) Bus (local apic timer) frequency in kHz. */
> +        res->b = 1000000ull / APIC_BUS_CYCLE_NS;
> +        res->c = 0;          /* Reserved */
> +        res->d = 0;          /* Reserved */
> +        break;
> +    }

No further dependency on the selected version?

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -885,8 +885,11 @@ static void do_trap(struct cpu_user_regs *regs)
>  int guest_rdmsr_xen(const struct vcpu *v, uint32_t idx, uint64_t *val)
>  {
>      const struct domain *d = v->domain;
> -    /* Optionally shift out of the way of Viridian architectural MSRs. */
> -    uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
> +    /*
> +     * Optionally shift out of the way of Viridian or VMware
> +     * architectural leaves.
> +     */
> +    uint32_t base = is_viridian_or_vmware_cpuid(d) ? 0x40000200 : 0x40000000;
>  
>      switch ( idx - base )
>      {
> @@ -901,8 +904,11 @@ int guest_rdmsr_xen(const struct vcpu *v, uint32_t idx, uint64_t *val)
>  int guest_wrmsr_xen(struct vcpu *v, uint32_t idx, uint64_t val)
>  {
>      struct domain *d = v->domain;
> -    /* Optionally shift out of the way of Viridian architectural MSRs. */
> -    uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
> +    /*
> +     * Optionally shift out of the way of Viridian or VMware
> +     * architectural leaves.
> +     */
> +    uint32_t base = is_viridian_or_vmware_cpuid(d) ? 0x40000200 : 0x40000000;
>  
>      switch ( idx - base )
>      {

How do these (MSR related) changes correspond to the subject of this
change? (Mentioning why they're needed in the description would help.)

Also your choice of name (is_viridian_or_vmware_cpuid()) wouldn't scale
if there were one or more further hypervisor emulations added. I don't
have a good suggestion for a name right away, but one should be found.

> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -474,6 +474,18 @@ static inline bool hvm_get_guest_bndcfgs(struct vcpu *v, u64 *val)
>  #define has_viridian_synic(d) \
>      (is_viridian_domain(d) && (viridian_feature_mask(d) & HVMPV_synic))
>  
> +#define vmware_feature_mask(d) \
> +    ((d)->arch.hvm.vmware_hwver)

Why "mask"? This is simply a numeric value, isn't it? Also why do
this and ...

> +#define is_vmware_domain(d) \
> +    (is_hvm_domain(d) && vmware_feature_mask(d))
> +
> +#define has_vmware_cpuid(d) \
> +    (is_hvm_domain(d) && (vmware_feature_mask(d) >= 7))

... these not live in the new vmware.h?

> --- /dev/null
> +++ b/xen/include/asm-x86/hvm/vmware.h
> @@ -0,0 +1,33 @@
> +/*
> + * asm-x86/hvm/vmware.h
> + *
> + * Copyright (C) 2012-2015 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>
> +
> +void cpuid_vmware_leaves(const struct vcpu *v, uint32_t leaf,
> +                         uint32_t subleaf, struct cpuid_leaf *res);

At the example of this, as per ./CODING_STYLE please avoid the use
of uint<N>_t when more basic types (unsigned int here) are fine to
use. With that you won't need xen/types.h anymore. You'll want to
forward-declare the two struct-s the prototype uses in any event,
though.

> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -304,6 +304,7 @@ struct xen_arch_domainconfig {
>                                       XEN_X86_EMU_PIT | XEN_X86_EMU_USE_PIRQ |\
>                                       XEN_X86_EMU_VPCI)
>      uint32_t emulation_flags;
> +    uint32_t vmware_hwver;
>  };

As per the comment above this struct XEN_DOMCTL_INTERFACE_VERSION
would need bumping with such an addition, unless it already has
been in the current release cycle.

Also - is VMware really x86-only?

Jan


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [XEN PATCH v14 5/8] xen: Add vmware_port support
  2020-08-19 16:51         ` [Xen-devel] [XEN PATCH v14 5/8] xen: Add vmware_port support Don Slutz
  2020-08-19 16:52           ` [Xen-devel] [XEN PATCH v14 6/8] tools: " Don Slutz
@ 2020-10-01 13:04           ` Jan Beulich
  1 sibling, 0 replies; 16+ messages in thread
From: Jan Beulich @ 2020-10-01 13:04 UTC (permalink / raw)
  To: Don Slutz
  Cc: xen-devel, Boris Ostrovsky, Ian Jackson, Jun Nakajima,
	Kevin Tian, Stefano Stabellini, Tim Deegan, Andrew Cooper,
	Konrad Rzeszutek Wilk, George Dunlap

On 19.08.2020 18:51, Don Slutz wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -504,6 +504,8 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>  
>  static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>  {
> +    uint32_t all_emflags = emflags & XEN_X86_EMU_ALL;
> +
>  #ifdef CONFIG_HVM
>      /* This doesn't catch !CONFIG_HVM case but it is better than nothing */
>      BUILD_BUG_ON(X86_EMU_ALL != XEN_X86_EMU_ALL);
> @@ -512,14 +514,15 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>      if ( is_hvm_domain(d) )
>      {
>          if ( is_hardware_domain(d) &&
> -             emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC) )
> +             all_emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC) )
>              return false;
>          if ( !is_hardware_domain(d) &&
> -             emflags != (X86_EMU_ALL & ~X86_EMU_VPCI) &&
> -             emflags != X86_EMU_LAPIC )
> +             all_emflags != (X86_EMU_ALL & ~X86_EMU_VPCI) &&
> +             all_emflags != X86_EMU_LAPIC )
>              return false;
>      }
> -    else if ( emflags != 0 && emflags != X86_EMU_PIT )
> +    else if ( emflags & XEN_X86_EMU_VMWARE_PORT ||
> +              (all_emflags != 0 && all_emflags != X86_EMU_PIT) )
>      {
>          /* PV or classic PVH. */
>          return false;
> @@ -581,7 +584,7 @@ int arch_domain_create(struct domain *d,
>      if ( is_hardware_domain(d) && is_pv_domain(d) )
>          emflags |= XEN_X86_EMU_PIT;
>  
> -    if ( emflags & ~XEN_X86_EMU_ALL )
> +    if ( emflags & ~(XEN_X86_EMU_ALL | XEN_X86_EMU_VMWARE_PORT) )
>      {
>          printk(XENLOG_G_ERR "d%d: Invalid emulation bitmap: %#x\n",
>                 d->domain_id, emflags);

Seeing code churn like this one I'm inclined to suggest this shouldn't
be part of this field. Either take it from the top bits of the field
you add in patch 3, or add yet another field. See how the various
Viridian sub-features also didn't go here.

> @@ -600,6 +603,8 @@ int arch_domain_create(struct domain *d,
>      if ( is_hvm_domain(d) )
>      {
>          d->arch.hvm.vmware_hwver = config->arch.vmware_hwver;
> +        d->arch.hvm.is_vmware_port_enabled =
> +            !!(emflags & XEN_X86_EMU_VMWARE_PORT);

While I expect this to move anyway, as a general remark: No need for
!! when the lvalue is of type bool. But then why have the separate
boolean anyway? With how you have things now, you could as well
look at d->arch.emulation_flags, and with the change suggested above
you'd again have another field where the information is already
present.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -697,6 +697,9 @@ int hvm_domain_initialise(struct domain *d)
>      if ( hvm_tsc_scaling_supported )
>          d->arch.hvm.tsc_scaling_ratio = hvm_default_tsc_scaling_ratio;
>  
> +    if ( d->arch.hvm.is_vmware_port_enabled )
> +        vmport_register(d);
> +
>      rc = viridian_domain_init(d);
>      if ( rc )
>          goto fail2;
> @@ -4214,6 +4217,12 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
>          rc = xsm_hvm_param_nested(XSM_PRIV, d);
>          if ( rc )
>              break;
> +        /* Prevent nestedhvm enable with vmport */
> +        if ( value && d->arch.hvm.is_vmware_port_enabled )
> +        {
> +            rc = -EOPNOTSUPP;
> +            break;
> +        }

Be aware that this case block is about to disappear.

> --- a/xen/arch/x86/hvm/vmware/Makefile
> +++ b/xen/arch/x86/hvm/vmware/Makefile
> @@ -1 +1,2 @@
>  obj-y += vmware.o
> +obj-y += vmport.o

Alphabetically sorted please, again.

> --- /dev/null
> +++ b/xen/arch/x86/hvm/vmware/vmport.c
> @@ -0,0 +1,148 @@
> +/*
> + * HVM VMPORT emulation
> + *
> + * 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/lib.h>
> +#include <asm/hvm/hvm.h>
> +#include <asm/hvm/support.h>
> +
> +#include "backdoor_def.h"
> +
> +static int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
> +{
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +
> +    /*
> +     * While VMware expects only 32-bit in, they do support using
> +     * other sizes and out.  However they do require only the 1 port
> +     * and the correct value in eax.  Since some of the data
> +     * returned in eax is smaller the 32 bits and/or you only need
> +     * the other registers the dir and bytes do not need any
> +     * checking.  The caller will handle the bytes, and dir is
> +     * handled below for eax.
> +     */
> +    if ( port == BDOOR_PORT && regs->eax == BDOOR_MAGIC )
> +    {
> +        uint32_t new_eax = ~0u;
> +        uint64_t value;
> +        struct vcpu *curr = current;
> +        struct domain *currd = curr->domain;

Both of these ought to be possible to gain const.

> +        /*
> +         * VMware changes the other (non eax) registers ignoring dir
> +         * (IN vs OUT).  It also changes only the 32-bit part
> +         * leaving the high 32-bits unchanged, unlike what one would
> +         * expect to happen.
> +         */
> +        switch ( regs->ecx & 0xffff )
> +        {
> +        case BDOOR_CMD_GETMHZ:
> +            new_eax = currd->arch.tsc_khz / 1000;
> +            break;
> +
> +        case BDOOR_CMD_GETVERSION:
> +            /* MAGIC */
> +            regs->ebx = BDOOR_MAGIC;

I don't think the comment is of much use here.

> +            /* VERSION_MAGIC */
> +            new_eax = 6;

Didn't the earlier patch talk about version 7?

> +            /* Claim we are an ESX. VMX_TYPE_SCALABLE_SERVER */
> +            regs->ecx = 2;
> +            break;
> +
> +        case BDOOR_CMD_GETHWVERSION:
> +            /* vmware_hw */
> +            new_eax = currd->arch.hvm.vmware_hwver;
> +            /*
> +             * Returning zero is not the best.  VMware was not at
> +             * all consistent in the handling of this command until
> +             * VMware hardware version 4.  So it is better to claim
> +             * 4 then 0.  This should only happen in strange configs.
> +             */
> +            if ( !new_eax )
> +                new_eax = 4;

Doesn't ->arch.hvm.vmware_hwver == 0 mean "VMware emulation disabled"?
Or are the two setting indeed meant to be entirely independent?

> +            break;
> +
> +        case BDOOR_CMD_GETHZ:
> +        {
> +            struct segment_register sreg;
> +
> +            hvm_get_segment_register(curr, x86_seg_ss, &sreg);
> +            if ( sreg.dpl == 0 )

Do you perhaps mean hvm_get_cpl() here?

> +            {
> +                value = currd->arch.tsc_khz * 1000;

No matter that value is uint64_t, you'll only ever get a 32-bit
value calculated here unless you e.g. use 1000UL.

> +                /* apic-frequency (bus speed) */
> +                regs->ecx = 1000000000ULL / APIC_BUS_CYCLE_NS;
> +                /* High part of tsc-frequency */
> +                regs->ebx = value >> 32;
> +                /* Low part of tsc-frequency */
> +                new_eax = value;
> +            }
> +            break;
> +
> +        }
> +        case BDOOR_CMD_GETTIME:
> +            value = get_localtime_us(currd) -
> +                currd->time_offset.seconds * 1000000ULL;

Whereas I don't see the need for the ULL here - seconds is a 64-bit
type already, and you'll wrongly convert from signed to unsigned.

> +            /* hostUsecs */
> +            regs->ebx = value % 1000000UL;
> +            /* hostSecs */
> +            new_eax = value / 1000000ULL;

Why once UL and once ULL? Neither of the suffixes seems necessary,
but at the very least you want to be consistent (unless there's a
reason not to be). (This, the previous, and the next comment apply
again further down.)

> +            /* maxTimeLag */
> +            regs->ecx = 1000000;

And this value is coming from where? If it can't be calculated,
please have the comment say how it was determined.

> +            /* offset to GMT in minutes */
> +            regs->edx = currd->time_offset.seconds / 60;
> +            break;
> +
> +        case BDOOR_CMD_GETTIMEFULL:
> +            /* BDOOR_MAGIC */
> +            new_eax = BDOOR_MAGIC;

Again, the comment isn't very helpful.

> +            value = get_localtime_us(currd) -
> +                currd->time_offset.seconds * 1000000ULL;
> +            /* hostUsecs */
> +            regs->ebx = value % 1000000UL;
> +            /* hostSecs low 32 bits */
> +            regs->edx = value / 1000000ULL;
> +            /* hostSecs high 32 bits */
> +            regs->esi = (value / 1000000ULL) >> 32;
> +            /* maxTimeLag */
> +            regs->ecx = 1000000;
> +            break;
> +
> +        default:
> +            /* Let backing DM handle */
> +            return X86EMUL_UNHANDLEABLE;

If so here, why not also ...

> +        }
> +        if ( dir == IOREQ_READ )
> +            *val = new_eax;
> +    }
> +    else if ( dir == IOREQ_READ )
> +        *val = ~0u;

... here?

> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -162,6 +162,9 @@ struct hvm_domain {
>      spinlock_t             uc_lock;
>      bool_t                 is_in_uc_mode;
>  
> +    /* VMware backdoor port available */
> +    bool_t                 is_vmware_port_enabled;

While as per above I assume this will go away again, as a general remark:
"bool" please in new additions, or even when just touching existing lines.

Jan


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT
  2020-08-19 16:52             ` [Xen-devel] [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT Don Slutz
  2020-08-19 16:52               ` [Xen-devel] [XEN PATCH v14 8/8] Add xentrace to vmware_port Don Slutz
@ 2020-10-01 14:41               ` Jan Beulich
  2020-10-06  8:13                 ` Paul Durrant
  1 sibling, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2020-10-01 14:41 UTC (permalink / raw)
  To: Don Slutz
  Cc: xen-devel, Boris Ostrovsky, Ian Jackson, Jun Nakajima,
	Kevin Tian, Stefano Stabellini, Tim Deegan, Andrew Cooper,
	Konrad Rzeszutek Wilk, George Dunlap, Paul Durrant

On 19.08.2020 18:52, Don Slutz wrote:
> This adds synchronization of the 6 vcpu registers (only 32bits of
> them) that QEMU's vmport.c and vmmouse.c needs between Xen and QEMU.
> This is how VMware defined the use of these registers.
> 
> This is to avoid a 2nd and 3rd exchange between QEMU and Xen to
> fetch and put these 6 vcpu registers used by the code in QEMU's
> vmport.c and vmmouse.c

I'm unconvinced this warrants a new ioreq type, and all the overhead
associated with it. I'd be curious to know what Paul or the qemu
folks think here.

> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -67,6 +67,7 @@
>  #define SPECIALPAGE_IOREQ    5
>  #define SPECIALPAGE_IDENT_PT 6
>  #define SPECIALPAGE_CONSOLE  7
> +#define SPECIALPAGE_VMPORT_REGS 8
>  #define special_pfn(x) \
>      (X86_HVM_END_SPECIAL_REGION - X86_HVM_NR_SPECIAL_PAGES + (x))
>  
> @@ -657,6 +658,8 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>                       special_pfn(SPECIALPAGE_BUFIOREQ));
>      xc_hvm_param_set(xch, domid, HVM_PARAM_IOREQ_PFN,
>                       special_pfn(SPECIALPAGE_IOREQ));
> +    xc_hvm_param_set(xch, domid, HVM_PARAM_VMPORT_REGS_PFN,
> +                     special_pfn(SPECIALPAGE_VMPORT_REGS));

I don't think we want to see new special PFNs appear. This ought to
be made work through the acquire_resource interface instead.

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -28,6 +28,8 @@
>  #include <asm/iocap.h>
>  #include <asm/vm_event.h>
>  
> +vmware_regs_t *get_vmport_regs_any(struct hvm_ioreq_server *s, struct vcpu *v);

Prototypes need to be in scope for both consumer and producer, to
ensure changes done on either side get reflected on the other (or
suitably diagnosed by the compiler).

> @@ -173,6 +175,8 @@ static int hvmemul_do_io(
>      };
>      void *p_data = (void *)data;
>      int rc;
> +    bool_t is_vmware = !is_mmio && !data_is_addr &&
> +        vmport_check_port(p.addr, p.size);

As to the data_is_addr part - what about REP INS / REP OUTS?

> @@ -189,11 +193,17 @@ static int hvmemul_do_io(
>      case STATE_IOREQ_NONE:
>          break;
>      case STATE_IORESP_READY:
> +    {
> +        uint8_t calc_type = is_mmio ? IOREQ_TYPE_COPY : IOREQ_TYPE_PIO;
> +
> +        if ( is_vmware )
> +            calc_type = IOREQ_TYPE_VMWARE_PORT;
> +
>          vio->io_req.state = STATE_IOREQ_NONE;
>          p = vio->io_req;
>  
>          /* Verify the emulation request has been correctly re-issued */
> -        if ( (p.type != (is_mmio ? IOREQ_TYPE_COPY : IOREQ_TYPE_PIO)) ||
> +        if ( (p.type != calc_type) ||
>               (p.addr != addr) ||
>               (p.size != size) ||
>               (p.count > *reps) ||
> @@ -202,7 +212,7 @@ static int hvmemul_do_io(
>               (p.data_is_ptr != data_is_addr) ||
>               (data_is_addr && (p.data != data)) )
>              domain_crash(currd);
> -
> +    }
>          if ( data_is_addr )
>              return X86EMUL_UNHANDLEABLE;
>  
> @@ -322,6 +332,49 @@ static int hvmemul_do_io(
>              }
>          }
>  
> +        if ( unlikely(is_vmware) )
> +        {
> +            vmware_regs_t *vr;
> +
> +            BUILD_BUG_ON(sizeof(ioreq_t) < sizeof(vmware_regs_t));
> +
> +            p.type = vio->io_req.type = IOREQ_TYPE_VMWARE_PORT;
> +            s = hvm_select_ioreq_server(currd, &p);
> +            vr = get_vmport_regs_any(s, curr);

The function tries to give you vr even if s is NULL - at best you're
going to have inconsistent pointers in the end. I think the function
either wants to return NULL for NULL input, or you want to avoid
calling the function when s is NULL.

> +            /*
> +             * If there is no suitable backing DM, just ignore accesses.  If
> +             * we do not have access to registers to pass to QEMU, just
> +             * ignore access.
> +             */
> +            if ( !s || !vr )
> +            {
> +                rc = hvm_process_io_intercept(&null_handler, &p);
> +                vio->io_req.state = STATE_IOREQ_NONE;
> +            }
> +            else
> +            {
> +                const struct cpu_user_regs *regs = guest_cpu_user_regs();
> +
> +                p.data = regs->rax;
> +                /* The code in QEMU that uses these registers,
> +                 * vmport.c and vmmouse.c, only uses the 32bit part
> +                 * of the register.  This is how VMware defined the
> +                 * use of these registers.
> +                 */

Comment style (also elsewhere).

> +                vr->ebx = regs->ebx;
> +                vr->ecx = regs->ecx;
> +                vr->edx = regs->edx;
> +                vr->esi = regs->esi;
> +                vr->edi = regs->edi;

In the description you tale about 6 registers. Is ebp missing here
(and below)?

> +                rc = hvm_send_ioreq(s, &p, 0);
> +                if ( rc != X86EMUL_RETRY || currd->is_shutting_down )
> +                    vio->io_req.state = STATE_IOREQ_NONE;
> +            }
> +            break;
> +        }
> +
>          if ( !s )
>              s = hvm_select_ioreq_server(currd, &p);

Please consider moving most of the if()'s body above below here, so
this remains a single, common call. Presumably even some more code
below here should remain common. The more code you duplicate, the
higher the risk of things getting updated in one place but not the
other.

> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -108,6 +108,44 @@ static struct hvm_ioreq_vcpu *get_pending_vcpu(const struct vcpu *v,
>      return NULL;
>  }
>  
> +static vmware_regs_t *get_vmport_regs_one(struct hvm_ioreq_server *s,
> +                                          struct vcpu *v)
> +{
> +    struct hvm_ioreq_vcpu *sv;
> +
> +    list_for_each_entry ( sv, &s->ioreq_vcpu_list, list_entry )
> +    {
> +        if ( sv->vcpu == v )
> +        {
> +            shared_vmport_iopage_t *p = s->vmport_ioreq.va;
> +            if ( !p )
> +                return NULL;
> +            return &p->vcpu_vmport_regs[v->vcpu_id];
> +        }
> +    }
> +    return NULL;
> +}
> +
> +vmware_regs_t *get_vmport_regs_any(struct hvm_ioreq_server *s, struct vcpu *v)
> +{
> +    struct domain *d = v->domain;
> +    unsigned int id;
> +
> +    ASSERT((v == current) || !vcpu_runnable(v));
> +
> +    if ( s )
> +        return get_vmport_regs_one(s, v);
> +
> +    FOR_EACH_IOREQ_SERVER(d, id, s)
> +    {
> +        vmware_regs_t *ret = get_vmport_regs_one(s, v);
> +
> +        if ( ret )
> +            return ret;
> +    }
> +    return NULL;
> +}

I think the naming wants improving, to take less reference to
get_ioreq() and more to more modern, properly prefixed naming.
E.g. vmport_get_regs() with the static helper becoming
_vmport_get_regs_one() or just _vmport_get_regs() (but as per
above I'm unconvinced the helper is needed).

Also for both functions (and generally)
- add const to pointed to types whenever possible,
- have a blank line between declarations and statements,
- put a blank line before a function's main return.

> @@ -206,6 +244,26 @@ bool handle_hvm_io_completion(struct vcpu *v)
>          return handle_mmio();
>  
>      case HVMIO_pio_completion:
> +        if ( vio->io_req.type == IOREQ_TYPE_VMWARE_PORT )
> +        {
> +            vmware_regs_t *vr = get_vmport_regs_any(NULL, v);

Why NULL? Isn't s the server you're after? Also - const.

> @@ -233,16 +291,28 @@ static gfn_t hvm_alloc_legacy_ioreq_gfn(struct hvm_ioreq_server *s)
>      unsigned int i;
>  
>      BUILD_BUG_ON(HVM_PARAM_BUFIOREQ_PFN != HVM_PARAM_IOREQ_PFN + 1);
> +    BUILD_BUG_ON(HVM_PARAM_VMPORT_REGS_PFN != HVM_PARAM_BUFIOREQ_PFN + 1);
>  
>      for ( i = HVM_PARAM_IOREQ_PFN; i <= HVM_PARAM_BUFIOREQ_PFN; i++ )

Without this for() changing I don't see why you put the BUILD_BUG_ON()
here.

>      {
> -        if ( !test_and_clear_bit(i, &d->arch.hvm.ioreq_gfn.legacy_mask) )
> +        if ( test_and_clear_bit(i, &d->arch.hvm.ioreq_gfn.legacy_mask) )

I can't believe this to be a correct change, or if there is a bug
to be fixed here, for this to belong here.

> @@ -293,9 +363,29 @@ static void hvm_free_ioreq_gfn(struct hvm_ioreq_server *s, gfn_t gfn)
>      }
>  }
>  
> -static void hvm_unmap_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
> +typedef enum {
> +    ioreq_pt_ioreq,
> +    ioreq_pt_bufioreq,
> +    ioreq_pt_vmport,
> +} ioreq_pt_;

Why the trailing underscore? And may I ask what "pt" stands for?

> +static void hvm_unmap_ioreq_gfn(struct hvm_ioreq_server *s, ioreq_pt_ pt)
>  {
> -    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> +    struct hvm_ioreq_page *iorp = NULL;
> +
> +    switch ( pt )
> +    {
> +    case ioreq_pt_ioreq:
> +        iorp = &s->ioreq;
> +        break;
> +    case ioreq_pt_bufioreq:
> +        iorp = &s->bufioreq;
> +        break;
> +    case ioreq_pt_vmport:
> +        iorp = &s->vmport_ioreq;
> +        break;
> +    }
> +    ASSERT(iorp);
>  
>      if ( gfn_eq(iorp->gfn, INVALID_GFN) )
>          return;

For an ASSERT() like this, please take a look at the bottom of ./CODING_STYLE.
I think you want a default case in the switch() body with ASSERT_UNREACHABLE()
and "return" instead. (Just in case this won't go away anyway, or some similar
construct then appears elsewhere.)

> @@ -329,7 +433,10 @@ static int hvm_map_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
>      if ( d->is_dying )
>          return -EINVAL;
>  
> -    iorp->gfn = hvm_alloc_ioreq_gfn(s);
> +    if ( pt == ioreq_pt_vmport )
> +        iorp->gfn = hvm_alloc_legacy_vmport_gfn(s);
> +    else
> +        iorp->gfn = hvm_alloc_ioreq_gfn(s);

I'm unconvinced the separate function is warranted, in case this stays in the
first place.

> @@ -645,12 +844,38 @@ static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s,
>      for ( i = 0; i < NR_IO_RANGE_TYPES; i++ )
>      {
>          char *name;
> +        char *type_name = NULL;
> +        unsigned int limit;
>  
> -        rc = asprintf(&name, "ioreq_server %d %s", id,
> -                      (i == XEN_DMOP_IO_RANGE_PORT) ? "port" :
> -                      (i == XEN_DMOP_IO_RANGE_MEMORY) ? "memory" :
> -                      (i == XEN_DMOP_IO_RANGE_PCI) ? "pci" :
> -                      "");
> +        switch ( i )
> +        {
> +        case XEN_DMOP_IO_RANGE_PORT:
> +            type_name = "port";
> +            limit = MAX_NR_IO_RANGES;
> +            break;
> +        case XEN_DMOP_IO_RANGE_MEMORY:
> +            type_name = "memory";
> +            limit = MAX_NR_IO_RANGES;
> +            break;
> +        case XEN_DMOP_IO_RANGE_PCI:
> +            type_name = "pci";
> +            limit = MAX_NR_IO_RANGES;
> +            break;
> +        case XEN_DMOP_IO_RANGE_VMWARE_PORT:
> +            type_name = "VMware port";
> +            limit = 1;
> +            break;
> +        case XEN_DMOP_IO_RANGE_TIMEOFFSET:
> +            type_name = "timeoffset";
> +            limit = 1;
> +            break;

Personally I'd prefer if you simply added a single line to the
asprintf() invocation above. I don't see at all why the time offset
thingy is appearing here (and elsewhere below) all of the sudden.
And there's no point for the limit variable afaict, as you ...

> @@ -663,7 +888,11 @@ static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s,
>          if ( !s->range[i] )
>              goto fail;
>  
> -        rangeset_limit(s->range[i], MAX_NR_IO_RANGES);
> +        rangeset_limit(s->range[i], limit);
> +
> +        /* VMware port */
> +        if ( i == XEN_DMOP_IO_RANGE_VMWARE_PORT && s->vmport_enabled )
> +            rc = rangeset_add_range(s->range[i], 1, 1);

... add the only wanted range here and don't allow further additions.

> @@ -714,7 +945,7 @@ static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s)
>  }
>  
>  static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
> -                                 struct domain *d, int bufioreq_handling,
> +                                 struct domain *d, int flags,

Can these flags have a negative value passed?

> @@ -1282,8 +1525,9 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
>      }
>      else
>      {
> -        type = (p->type == IOREQ_TYPE_PIO) ?
> -                XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY;
> +        type = (p->type == IOREQ_TYPE_PIO) ? XEN_DMOP_IO_RANGE_PORT : 
> +            (p->type == IOREQ_TYPE_VMWARE_PORT) ? XEN_DMOP_IO_RANGE_VMWARE_PORT :
> +            XEN_DMOP_IO_RANGE_MEMORY;

Indentation.

> @@ -23,6 +24,32 @@ static int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
>  {
>      struct cpu_user_regs *regs = guest_cpu_user_regs();
>  
> +#define port_overlap(p, n) \
> +    ((p + n > BDOOR_PORT) && (p + n <= BDOOR_PORT + 4) ? 1 : \
> +    (BDOOR_PORT + 4 > p) && (BDOOR_PORT + 4 <= p + n) ? 1 : 0)

It's only used in BUILD_BUG_ON(), but I still think this is far more
involved than it needs to be. The typical overlap check goes along
the lines of (start1 < end2 && start2 < end1), and there shouldn't
be any use of a conditional operator needed when all you want are
values of 0 and 1 (or true/false).

> +    BUILD_BUG_ON(port_overlap(PIT_BASE, 4));
> +    BUILD_BUG_ON(port_overlap(0x61, 1));
> +    BUILD_BUG_ON(port_overlap(XEN_HVM_DEBUGCONS_IOPORT, 1));
> +    BUILD_BUG_ON(port_overlap(0xcf8, 4));
> +/* #define TMR_VAL_ADDR_V0  (ACPI_PM_TMR_BLK_ADDRESS_V0) */
> +    BUILD_BUG_ON(port_overlap(ACPI_PM_TMR_BLK_ADDRESS_V0, 4));
> +/* #define PM1a_STS_ADDR_V0 (ACPI_PM1A_EVT_BLK_ADDRESS_V0) */
> +    BUILD_BUG_ON(port_overlap(ACPI_PM1A_EVT_BLK_ADDRESS_V0, 4));

What are these comments about?

> +    BUILD_BUG_ON(port_overlap(RTC_PORT(0), 2));
> +    BUILD_BUG_ON(port_overlap(0x3c4, 2));
> +    BUILD_BUG_ON(port_overlap(0x3ce, 2));
> +/*
> + * acpi_smi_cmd can not be checked at build time:
> + *   xen/include/asm-x86/acpi.h:extern u32 acpi_smi_cmd;
> + *   xen/arch/x86/acpi/boot.c: acpi_smi_cmd = fadt->smi_command;
> + BUILD_BUG_ON(port_overlap(acpi_smi_cmd, 1));

In this case I think the BUILD_BUG_ON() would still be better to
align with the others, even if commented out.

> +*/
> +    BUILD_BUG_ON(port_overlap(0x20, 2));
> +    BUILD_BUG_ON(port_overlap(0xa0, 2));
> +    BUILD_BUG_ON(port_overlap(0x4d0, 1));
> +    BUILD_BUG_ON(port_overlap(0x4d1, 1));

#undef port_overlap

> @@ -137,6 +164,15 @@ void vmport_register(struct domain *d)
>      register_portio_handler(d, BDOOR_PORT, 4, vmport_ioport);
>  }
>  
> +bool_t vmport_check_port(unsigned int port, unsigned int bytes)

bool

> +{
> +    struct domain *currd = current->domain;

const

> @@ -66,11 +69,25 @@ struct ioreq {
>  };
>  typedef struct ioreq ioreq_t;
>  
> +struct vmware_regs {
> +    uint32_t esi;
> +    uint32_t edi;
> +    uint32_t ebx;
> +    uint32_t ecx;
> +    uint32_t edx;
> +};
> +typedef struct vmware_regs vmware_regs_t;
> +
>  struct shared_iopage {
>      struct ioreq vcpu_ioreq[1];
>  };
>  typedef struct shared_iopage shared_iopage_t;
>  
> +struct shared_vmport_iopage {
> +    struct vmware_regs vcpu_vmport_regs[1];
> +};
> +typedef struct shared_vmport_iopage shared_vmport_iopage_t;

I wonder if this layout wouldn't better include some padding, so that
entries are a multiple of 16 bytes apart, to reduce cache line bouncing.

> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -94,8 +94,8 @@
>  #define HVM_PARAM_STORE_EVTCHN 2
>  
>  #define HVM_PARAM_IOREQ_PFN    5
> -
>  #define HVM_PARAM_BUFIOREQ_PFN 6
> +#define HVM_PARAM_VMPORT_REGS_PFN 7

Is it just lucky conincidence that 7 was unused, or are you risking
collision with some old piece of software? (But as said earlier, this
is likely to go away anyway.)

Jan


^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT
  2020-10-01 14:41               ` [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT Jan Beulich
@ 2020-10-06  8:13                 ` Paul Durrant
  2020-10-13  9:37                   ` Jan Beulich
  0 siblings, 1 reply; 16+ messages in thread
From: Paul Durrant @ 2020-10-06  8:13 UTC (permalink / raw)
  To: 'Jan Beulich', 'Don Slutz'
  Cc: xen-devel, 'Boris Ostrovsky', 'Ian Jackson',
	'Jun Nakajima', 'Kevin Tian',
	'Stefano Stabellini', 'Tim Deegan',
	'Andrew Cooper', 'Konrad Rzeszutek Wilk',
	'George Dunlap'



> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 01 October 2020 15:42
> To: Don Slutz <don.slutz@gmail.com>
> Cc: xen-devel@lists.xen.org; Boris Ostrovsky <boris.ostrovsky@oracle.com>; Ian Jackson
> <iwj@xenproject.org>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>;
> Stefano Stabellini <sstabellini@kernel.org>; Tim Deegan <tim@xen.org>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; George Dunlap
> <George.Dunlap@eu.citrix.com>; Paul Durrant <paul@xen.org>
> Subject: Re: [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT
> 
> On 19.08.2020 18:52, Don Slutz wrote:
> > This adds synchronization of the 6 vcpu registers (only 32bits of
> > them) that QEMU's vmport.c and vmmouse.c needs between Xen and QEMU.
> > This is how VMware defined the use of these registers.
> >
> > This is to avoid a 2nd and 3rd exchange between QEMU and Xen to
> > fetch and put these 6 vcpu registers used by the code in QEMU's
> > vmport.c and vmmouse.c
> 
> I'm unconvinced this warrants a new ioreq type, and all the overhead
> associated with it. I'd be curious to know what Paul or the qemu
> folks think here.
> 

The current shared ioreq_t does appear have enough space to accommodate 6 32-bit registers (in the addr, data, count and size) fields co couldn't the new IOREQ_TYPE_VMWARE_PORT type be dealt with by simply unioning the regs with these fields? That avoids the need for a whole new shared page.

  Paul



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT
  2020-10-06  8:13                 ` Paul Durrant
@ 2020-10-13  9:37                   ` Jan Beulich
  2020-10-13  9:50                     ` Paul Durrant
  0 siblings, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2020-10-13  9:37 UTC (permalink / raw)
  To: paul
  Cc: 'Don Slutz', xen-devel, 'Boris Ostrovsky',
	'Ian Jackson', 'Jun Nakajima',
	'Kevin Tian', 'Stefano Stabellini',
	'Tim Deegan', 'Andrew Cooper',
	'Konrad Rzeszutek Wilk', 'George Dunlap'

On 06.10.2020 10:13, Paul Durrant wrote:
> 
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 01 October 2020 15:42
>> To: Don Slutz <don.slutz@gmail.com>
>> Cc: xen-devel@lists.xen.org; Boris Ostrovsky <boris.ostrovsky@oracle.com>; Ian Jackson
>> <iwj@xenproject.org>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>;
>> Stefano Stabellini <sstabellini@kernel.org>; Tim Deegan <tim@xen.org>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; George Dunlap
>> <George.Dunlap@eu.citrix.com>; Paul Durrant <paul@xen.org>
>> Subject: Re: [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT
>>
>> On 19.08.2020 18:52, Don Slutz wrote:
>>> This adds synchronization of the 6 vcpu registers (only 32bits of
>>> them) that QEMU's vmport.c and vmmouse.c needs between Xen and QEMU.
>>> This is how VMware defined the use of these registers.
>>>
>>> This is to avoid a 2nd and 3rd exchange between QEMU and Xen to
>>> fetch and put these 6 vcpu registers used by the code in QEMU's
>>> vmport.c and vmmouse.c
>>
>> I'm unconvinced this warrants a new ioreq type, and all the overhead
>> associated with it. I'd be curious to know what Paul or the qemu
>> folks think here.
>>
> 
> The current shared ioreq_t does appear have enough space to accommodate 6 32-bit registers (in the addr, data, count and size) fields co couldn't the new IOREQ_TYPE_VMWARE_PORT type be dealt with by simply unioning the regs with these fields? That avoids the need for a whole new shared page.

Hmm, yes, good point. But this is assuming we're going to be fine with
using 32-bit registers now and going forward. Personally I'd prefer a
mechanism less constrained by the specific needs of the current VMware
interface, i.e. potentially allowing to scale to 64-bit registers as
well as any of the remaining 9 ones (leaving aside %rsp).

Jan


^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT
  2020-10-13  9:37                   ` Jan Beulich
@ 2020-10-13  9:50                     ` Paul Durrant
  0 siblings, 0 replies; 16+ messages in thread
From: Paul Durrant @ 2020-10-13  9:50 UTC (permalink / raw)
  To: 'Jan Beulich'
  Cc: 'Don Slutz', xen-devel, 'Boris Ostrovsky',
	'Ian Jackson', 'Jun Nakajima',
	'Kevin Tian', 'Stefano Stabellini',
	'Tim Deegan', 'Andrew Cooper',
	'Konrad Rzeszutek Wilk', 'George Dunlap'

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 13 October 2020 10:38
> To: paul@xen.org
> Cc: 'Don Slutz' <don.slutz@gmail.com>; xen-devel@lists.xen.org; 'Boris Ostrovsky'
> <boris.ostrovsky@oracle.com>; 'Ian Jackson' <iwj@xenproject.org>; 'Jun Nakajima'
> <jun.nakajima@intel.com>; 'Kevin Tian' <kevin.tian@intel.com>; 'Stefano Stabellini'
> <sstabellini@kernel.org>; 'Tim Deegan' <tim@xen.org>; 'Andrew Cooper' <andrew.cooper3@citrix.com>;
> 'Konrad Rzeszutek Wilk' <konrad.wilk@oracle.com>; 'George Dunlap' <George.Dunlap@eu.citrix.com>
> Subject: Re: [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT
> 
> On 06.10.2020 10:13, Paul Durrant wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 01 October 2020 15:42
> >> To: Don Slutz <don.slutz@gmail.com>
> >> Cc: xen-devel@lists.xen.org; Boris Ostrovsky <boris.ostrovsky@oracle.com>; Ian Jackson
> >> <iwj@xenproject.org>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>;
> >> Stefano Stabellini <sstabellini@kernel.org>; Tim Deegan <tim@xen.org>; Andrew Cooper
> >> <andrew.cooper3@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; George Dunlap
> >> <George.Dunlap@eu.citrix.com>; Paul Durrant <paul@xen.org>
> >> Subject: Re: [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT
> >>
> >> On 19.08.2020 18:52, Don Slutz wrote:
> >>> This adds synchronization of the 6 vcpu registers (only 32bits of
> >>> them) that QEMU's vmport.c and vmmouse.c needs between Xen and QEMU.
> >>> This is how VMware defined the use of these registers.
> >>>
> >>> This is to avoid a 2nd and 3rd exchange between QEMU and Xen to
> >>> fetch and put these 6 vcpu registers used by the code in QEMU's
> >>> vmport.c and vmmouse.c
> >>
> >> I'm unconvinced this warrants a new ioreq type, and all the overhead
> >> associated with it. I'd be curious to know what Paul or the qemu
> >> folks think here.
> >>
> >
> > The current shared ioreq_t does appear have enough space to accommodate 6 32-bit registers (in the
> addr, data, count and size) fields co couldn't the new IOREQ_TYPE_VMWARE_PORT type be dealt with by
> simply unioning the regs with these fields? That avoids the need for a whole new shared page.
> 
> Hmm, yes, good point. But this is assuming we're going to be fine with
> using 32-bit registers now and going forward. Personally I'd prefer a
> mechanism less constrained by the specific needs of the current VMware
> interface, i.e. potentially allowing to scale to 64-bit registers as
> well as any of the remaining 9 ones (leaving aside %rsp).
> 

I think that should probably be additional work, not needed for this series. We could look to expand and re-structure the ioreq_t structure with some headroom. An emulator aware of the new structure to resource map a different set of shared pages.

  Paul

> Jan




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Xen-devel] [XEN PATCH v14 8/8] Add xentrace to vmware_port
  2020-08-19 16:52               ` [Xen-devel] [XEN PATCH v14 8/8] Add xentrace to vmware_port Don Slutz
@ 2020-10-16  9:32                 ` Jan Beulich
  0 siblings, 0 replies; 16+ messages in thread
From: Jan Beulich @ 2020-10-16  9:32 UTC (permalink / raw)
  To: Don Slutz
  Cc: xen-devel, Aravind Gopalakrishnan, Boris Ostrovsky, Eddie Dong,
	Ian Campbell, Ian Jackson, Jun Nakajima, Keir Fraser, Kevin Tian,
	Stefano Stabellini, Suravee Suthikulpanit, Tim Deegan,
	Andrew Cooper, Konrad Rzeszutek Wilk, George Dunlap, Don Slutz

On 19.08.2020 18:52, Don Slutz wrote:
> From: Don Slutz <dslutz@verizon.com>
> 
> Also added missing TRAP_DEBUG & VLAPIC.
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> CC: Don Slutz <don.slutz@gmail.com>
> ---
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> v14:
>   Reworked to current code.
>   Added VMPORT_SEND because I wanted to see it during testing.
> 
> v13:
>     Please do this by extending the existing infrastructure rather
>     than special-casing 7 on the side.  (i.e. extend ND to take 7
>     parameters, and introduce HVMTRACE_7D)
>     = { d1, d2, d3, d4, d5, d6, d7 } will be far shorter, linewise.

I think this would have wanted to split into two patches right
at the time: One for the extension, and another for the new
VMware logic. But see below.

> @@ -62,6 +63,7 @@ static int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
>      if ( port == BDOOR_PORT && regs->eax == BDOOR_MAGIC )
>      {
>          uint32_t new_eax = ~0u;
> +        uint16_t cmd = regs->ecx;
>          uint64_t value;
>          struct vcpu *curr = current;
>          struct domain *currd = curr->domain;
> @@ -72,7 +74,7 @@ static int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
>           * leaving the high 32-bits unchanged, unlike what one would
>           * expect to happen.
>           */
> -        switch ( regs->ecx & 0xffff )
> +        switch ( cmd )
>          {
>          case BDOOR_CMD_GETMHZ:
>              new_eax = currd->arch.tsc_khz / 1000;
> @@ -147,14 +149,22 @@ static int vmport_ioport(int dir, uint32_t port, uint32_t bytes, uint32_t *val)
>              break;
>  
>          default:
> +            HVMTRACE_6D(VMPORT_SEND, cmd, regs->ebx, regs->ecx,
> +                        regs->edx, regs->esi, regs->edi);

With cmd derived from regs->ecx, why pass the same value twice here?

>              /* Let backing DM handle */
>              return X86EMUL_UNHANDLEABLE;
>          }
> +        HVMTRACE_7D(VMPORT_HANDLED, cmd, new_eax, regs->ebx, regs->ecx,
> +                    regs->edx, regs->esi, regs->edi);

None of the cases making it here consumes or alter regs->edi. Why
record / report its value? Without this, the entire widening to 7
parameters becomes unnecessary for now, afaics.

Jan


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-19 16:51 [Xen-devel] [XEN PATCH v14 0/8] Xen VMware tools support Don Slutz
2020-08-19 16:51 ` [Xen-devel] [XEN PATCH v14 1/8] tools: Add vga=vmware Don Slutz
2020-08-19 16:51   ` [Xen-devel] [XEN PATCH v14 2/8] xen: Add support for VMware cpuid leaves Don Slutz
2020-08-19 16:51     ` [Xen-devel] [XEN PATCH v14 3/8] tools: Add vmware_hwver support Don Slutz
2020-08-19 16:51       ` [Xen-devel] [XEN PATCH v14 4/8] vmware: Add VMware provided include file Don Slutz
2020-08-19 16:51         ` [Xen-devel] [XEN PATCH v14 5/8] xen: Add vmware_port support Don Slutz
2020-08-19 16:52           ` [Xen-devel] [XEN PATCH v14 6/8] tools: " Don Slutz
2020-08-19 16:52             ` [Xen-devel] [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT Don Slutz
2020-08-19 16:52               ` [Xen-devel] [XEN PATCH v14 8/8] Add xentrace to vmware_port Don Slutz
2020-10-16  9:32                 ` Jan Beulich
2020-10-01 14:41               ` [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT Jan Beulich
2020-10-06  8:13                 ` Paul Durrant
2020-10-13  9:37                   ` Jan Beulich
2020-10-13  9:50                     ` Paul Durrant
2020-10-01 13:04           ` [XEN PATCH v14 5/8] xen: Add vmware_port support Jan Beulich
2020-09-30 14:24     ` [XEN PATCH v14 2/8] xen: Add support for VMware cpuid leaves Jan Beulich

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git