From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v9 06/13] xen: Add ring 3 vmware_port support Date: Sat, 21 Feb 2015 08:36:11 -0500 Message-ID: <54E889CB.6000900@terremark.com> References: <1424127915-27004-1-git-send-email-dslutz@verizon.com> <1424127915-27004-7-git-send-email-dslutz@verizon.com> <54E35261.4010208@citrix.com> <54E4C5C8.6060003@terremark.com> <54E4D7A2.3090505@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54E4D7A2.3090505@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , Don Slutz , xen-devel@lists.xen.org Cc: Kevin Tian , Keir Fraser , Ian Campbell , Stefano Stabellini , Jun Nakajima , Eddie Dong , Ian Jackson , Tim Deegan , George Dunlap , Aravind Gopalakrishnan , Jan Beulich , Boris Ostrovsky , Suravee Suthikulpanit List-Id: xen-devel@lists.xenproject.org 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 >>>> --- >>>> 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 >>>> #include >>>> #include >>>> +#include >>>> >>>> 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 > ~Andrew >