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

[Don Slutz <dslutz@xxxxxxxxxxx>]

On 02/21/15 10:40, Andrew Cooper wrote:
> 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;
>      }
>  

It looks ok.  Will test.

   -Don Slutz


> ~Andrew
> 

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