Re: [PATCH v9 06/13] xen: Add ring 3 vmware_port support

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 18/02/15 17:03, Don Slutz wrote:
> On 02/17/15 09:38, Andrew Cooper wrote:
>> On 16/02/15 23:05, Don Slutz wrote:
>>> 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.
>>>
>>> This instruction is allowed to be used from ring 3.  To
>>> support this the vmexit for GP needs to be enabled.  I have not
>>> fully tested that nested HVM is doing the right thing for this.
>>>
>>> The support included is enough to allow VMware tools to install in a
>>> HVM domU.
>>>
>>> Enable no-fault of pio in x86_emulate for VMware port
>>>
>>> Also adjust the emulation registers after doing a VMware
>>> backdoor operation.
>>>
>>> Add new routine hvm_emulate_one_gp() to be used by the #GP fault
>>> handler.
>>>
>>> Some of the best info is at:
>>>
>>> https://sites.google.com/site/chitchatvmback/backdoor
>>>
>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>> ---
>>> v9:
>>>    Split #GP handling (or skipping of #GP) code out of previous
>>>    patch to help with the review process.
>>>    Switch to x86_emulator to handle #GP
>>>    I think the hvm_emulate_ops_gp() covers all needed ops.  Not able
>>> to validate
>>>    all paths though _hvm_emulate_one().
>>>
>>>  xen/arch/x86/hvm/emulate.c             | 62
>>> ++++++++++++++++++++++++++++++++--
>>>  xen/arch/x86/hvm/svm/svm.c             | 27 +++++++++++++++
>>>  xen/arch/x86/hvm/svm/vmcb.c            |  2 ++
>>>  xen/arch/x86/hvm/vmware/vmport.c       | 11 ++++++
>>>  xen/arch/x86/hvm/vmx/vmcs.c            |  2 ++
>>>  xen/arch/x86/hvm/vmx/vmx.c             | 38 +++++++++++++++++++++
>>>  xen/arch/x86/x86_emulate/x86_emulate.c | 25 +++++++++++---
>>>  xen/arch/x86/x86_emulate/x86_emulate.h |  8 +++++
>>>  xen/include/asm-x86/hvm/emulate.h      |  2 ++
>>>  xen/include/asm-x86/hvm/vmport.h       |  1 +
>>>  10 files changed, 172 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>>> index 636c909..a6a6a5c 100644
>>> --- a/xen/arch/x86/hvm/emulate.c
>>> +++ b/xen/arch/x86/hvm/emulate.c
>>> @@ -22,6 +22,7 @@
>>>  #include <asm/hvm/trace.h>
>>>  #include <asm/hvm/support.h>
>>>  #include <asm/hvm/svm/svm.h>
>>> +#include <asm/hvm/vmport.h>
>>>
>>>  static void hvmtrace_io_assist(int is_mmio, ioreq_t *p)
>>>  {
>>> @@ -776,6 +777,7 @@ static int hvmemul_read_io_discard(
>>>      unsigned long *val,
>>>      struct x86_emulate_ctxt *ctxt)
>>>  {
>>> +    ctxt->do_vmport = 0;
>>
>> 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.

Something along the lines of:

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
b/xen/arch/x86/x86_emulate/x86_emulate.c
index 5e9e040..dd40d6a 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -3394,7 +3394,8 @@ static int inject_swint(enum x86_swint_type type,
                              ? insn_fetch_type(uint8_t)
                              : (uint16_t)_regs.edx);
         op_bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
-        if ( (rc = ioport_access_check(port, op_bytes, ctxt, ops)) != 0 )
+        if ( ((rc = ioport_access_check(port, op_bytes, ctxt, ops)) !=
0) ||
+             (ops->vmport_check && ((rc = ops->vmport_check(port,
ctxt)) != 0)) )
             goto done;
         if ( b & 2 )
         {

would be far less invasive and AFAICT, replace the entire rest of your
patch.

In this case, if ioport_access_check() succeeds, or if it fails and
vmport_check subsequently succeeds, the standard ioport dispatch will
run, and hit vmport_ioport().

~Andrew