[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 21/02/15 13:36, Don Slutz wrote:
> On 02/18/15 13:19, Andrew Cooper wrote:
>> 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@xxxxxxxxxxx>
>>>>> ---
>>>>> 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().
>>
> Yes, but since vmport_ioport changes guest_cpu_user_regs() which
> are different then _regs and get modified by:
>
>     *ctxt->regs = _regs;
>  done:
>     return rc;
>
> which will drop all changes to other registers by vmport_ioport.  This
> is why I added the flag do_vmport. Which must be set in all case where
> vmport_ioport is called via x86_emulate.
>
> So I could call on it 1st.  This would make the change smaller.
>
>    -Don Slutz
>

How about this then?

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
b/xen/arch/x86/x86_emulate/x86_emulate.c
index 5e9e040..2aea91c 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -3393,8 +3393,11 @@ static int inject_swint(enum x86_swint_type type,
         unsigned int port = ((b < 0xe8)
                              ? insn_fetch_type(uint8_t)
                              : (uint16_t)_regs.edx);
+        bool_t vmport = (ops->vmport_check && /* Vmware backdoor? */
+                         (ops->vmport_check(port, ctxt) == 0));
         op_bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
-        if ( (rc = ioport_access_check(port, op_bytes, ctxt, ops)) != 0 )
+        if ( !vmport &&
+             (rc = ioport_access_check(port, op_bytes, ctxt, ops)) != 0 )
             goto done;
         if ( b & 2 )
         {
@@ -3413,6 +3416,10 @@ static int inject_swint(enum x86_swint_type type,
         }
         if ( rc != 0 )
             goto done;
+        if ( vmport )
+        {
+            /* fill _regs */
+        }
         break;
     }
 

~Andrew


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.