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

[Don Slutz <dslutz@verizon.com>]

On 02/24/15 03:34, Jan Beulich wrote:
>>>> On 23.02.15 at 18:11, <dslutz@verizon.com> wrote:
>> On 02/23/15 10:12, Jan Beulich wrote:
>>>>>> On 17.02.15 at 00:05, <dslutz@verizon.com> 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
>