Re: [Xen-devel] [PATCH v4 6/6] x86emul: improve CR/DR access handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 10/01/17 13:01, Jan Beulich wrote:
>>>> On 10.01.17 at 12:59, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 10/01/17 09:06, Jan Beulich wrote:
>>> - don't accept LOCK for DR accesses (it's undefined in the manuals)
>>> - only accept LOCK for CR accesses when the respective feature flag is
>>>   set (which would not normally be the case for Intel)
>>> - add (rather than or) 8 when LOCK is present; real hardware #UDs
>>>   when both REX.W and LOCK are present, implying that these would
>>>   rather access hypothetical CR16...23
>>> - eliminate explicit decode_register() calls
>>> - streamline remaining read/write code
>>>
>>> No further functional change, i.e. not addressing the missing exception
>>> generation (#UD for invalid CR/DR encodings, #GP(0) for invalid write
>>> values, #DB for DR accesses with DR7.GD set).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -194,7 +194,8 @@ static const opcode_desc_t twobyte_table
>>>      ImplicitOps|ModRM, ImplicitOps|ModRM, ImplicitOps|ModRM, 
>>> ImplicitOps|ModRM,
>>>      ImplicitOps|ModRM, ImplicitOps|ModRM, ImplicitOps|ModRM, 
>>> ImplicitOps|ModRM,
>>>      /* 0x20 - 0x27 */
>>> -    ImplicitOps|ModRM, ImplicitOps|ModRM, ImplicitOps|ModRM, 
>>> ImplicitOps|ModRM,
>>> +    DstMem|SrcImplicit|ModRM, DstMem|SrcImplicit|ModRM,
>>> +    DstImplicit|SrcMem|ModRM, DstImplicit|SrcMem|ModRM,
>> These are Src/Dst Reg rather than Mem, although its not immediately
>> obvious how this would alter the outcome.
> No - SrcReg / DstReg describe operands encoded in bit 3..5 of a
> ModRM byte, whereas SrcMem / DstMem describe such encoded in
> bits 0..2 together with 6..7. The fact that mod is required to be 3
> for these insns isn't being expressed in the static table at all. (And
> I guess you then understand how getting this wrong here would
> affect the outcome.)

Ok.  Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

I will fix the naming here.  The Mem part of {Src,Dst}Mem should be RM
to make things clearer.

~Andrew

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