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: Tue, 24 Feb 2015 12:14:27 -0500 Message-ID: <54ECB173.2020208@terremark.com> References: <1424127915-27004-1-git-send-email-dslutz@verizon.com> <1424127915-27004-7-git-send-email-dslutz@verizon.com> <54EB515F0200007800062915@mail.emea.novell.com> <54EB5F2D.5020500@terremark.com> <54EC45B50200007800062EF5@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54EC45B50200007800062EF5@mail.emea.novell.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: Jan Beulich , Don Slutz Cc: Jun Nakajima , Tim Deegan , Kevin Tian , Keir Fraser , Ian Campbell , Stefano Stabellini , George Dunlap , Andrew Cooper , Ian Jackson , xen-devel@lists.xen.org, Eddie Dong , Aravind Gopalakrishnan , Suravee Suthikulpanit , Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org On 02/24/15 03:34, Jan Beulich wrote: >>>> On 23.02.15 at 18:11, wrote: >> On 02/23/15 10:12, Jan Beulich wrote: >>>>>> On 17.02.15 at 00:05, wrote: >>>> @@ -393,6 +393,11 @@ struct x86_emulate_ops >>>> enum x86_segment seg, >>>> unsigned long offset, >>>> struct x86_emulate_ctxt *ctxt); >>>> + >>>> + /* vmport_check */ >>>> + int (*vmport_check)( >>>> + unsigned int port, >>>> + struct x86_emulate_ctxt *ctxt); >>> >>> I hope that this will no longer be needed with the adjustments >>> Andrew suggested. In light of that I only skimmed the patch, >>> awaiting the next version to be less involved. >>> >> >> My understanding is that it is needed. The code in >> xen/arch/x86/x86_emulate is not to access code directly >> in xen/arch/x86/hvm. >> >> tools/tests/x86_emulator/test_x86_emulator.c would not build if >> the routine was access directly. >> >> >> Here is the code that passed my testing: >> >> case 0xe4: /* in imm8,%al */ >> case 0xe5: /* in imm8,%eax */ >> case 0xe6: /* out %al,imm8 */ >> case 0xe7: /* out %eax,imm8 */ >> case 0xec: /* in %dx,%al */ >> case 0xed: /* in %dx,%eax */ >> case 0xee: /* out %al,%dx */ >> case 0xef: /* out %eax,%dx */ { >> 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 ( !vmport && >> (rc = ioport_access_check(port, op_bytes, ctxt, ops)) != 0 ) >> goto done; > > Hmm, this looks ugly. At the very least - for this to become half > way acceptable - this should not be VMware specific in any way. I.e. > just have a generic (and generically usable) hook here which your > VMware port code then just happens to use. > I can see changing the name to something like "skip_pio_port_access_check". But to make it more generic it would also need to return 3 values (enum or #define). 1) do not skip 2) skip but no registers 3) skip and registers. This is because the code soon below (which was not included in the reply): + if ( vmport ) + { + _regs._ebx = ctxt->regs->_ebx; + _regs._ecx = ctxt->regs->_ecx; + _regs._edx = ctxt->regs->_edx; + _regs._esi = ctxt->regs->_esi; + _regs._edi = ctxt->regs->_edi; + } Which I think can be made more generic: unsigned long save_ip = _regs.eip; _regs = *ctxt->regs; _regs.eip = save_ip; (or just switch to rbx etc) is needed here. 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. Which I would assume would mean to also have the same set of arguments as ioport_access_check(), even though most will be ignored. -Don Slutz > Jan >