Re: [PATCH v11 6/9] xen: Add ring 3 vmware_port support

[Don Slutz <don.slutz@gmail.com>]

On 06/04/15 10:14, George Dunlap wrote:
> On 06/04/2015 01:37 PM, Don Slutz wrote:
>> On 06/03/15 12:58, George Dunlap wrote:
>>> On 06/03/2015 05:41 PM, Don Slutz wrote:
>>>> On 06/03/15 12:23, George Dunlap wrote:
>>>>> On 06/03/2015 04:58 PM, Andrew Cooper wrote:
>>>>>> On 03/06/15 16:26, George Dunlap wrote:
>>>>>>> On 05/22/2015 04:50 PM, 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.


>>> (As an aside, I think my description does a better job of alerting a
>>> reviewer to what's going on in this patch -- you might consider stealing
>>> part of it if you end up re-submitting this one.)
>>>
>>
>> I would be happy to "steal" the description part.  I normally give
>> credit to the author in the "what has changed".  I could also add to the
>> commit message:
>>
>>
>> George Dunlap summarized this change as:
>>
>> VMWare allows ring3 to access the magic port regardless of whether the
>> guest OS has enabled access to that IO port or not.
>>
>> In order to emulate this, we need to:
>> * Trap to Xen on #GPs rather than just letting the hardware handle it
>> * Emulate all instructions which cause a #GP, just to see if they might
>>   be an IO instruction accessing the magic port.
>> * If it is an IO instruction, and it's accessing the magic port, then we
>>   skip the ioport access checks (which will cause the instruction to
>>   execute as though it had been given access).
>> * Under all other circumstances (we hope) the emulator in Xen will do
>>   exactly what the hardware just did, and deliver a #GP to the guest.
>>
>> In an attempt to make this more safe, emulation ops that write (such as
>> write and cmpxchg) are replaced with stubs which always return an error.
> 
> I would take the "(we hope)" out, since that's really part of the second
> half (me doubting whether such a patch is wise).  Not a good idea to
> doubt whether a patch is a good idea in the commit message. :-)
> 
> If you feel like credit is necessary, I'd just put at the bottom
> somewhere in parentheses something like this:
> 
> (h/t to George Dunlap for the patch summary.)
> 

Ok.  Thanks,

   -Don Slutz


>  -George
>