Re: [Xen-devel] [PATCH 09/17] SVM: use generic instruction decoding

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.09.16 at 15:42, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 15/09/16 07:55, Jan Beulich wrote:
>>>>> On 14.09.16 at 19:56, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 08/09/16 14:14, Jan Beulich wrote:
>>>>  int __get_instruction_length_from_list(struct vcpu *v,
>>>>          const enum instruction_index *list, unsigned int list_count)
>>>>  {
>>>>      struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>>>> -    unsigned int i, j, inst_len = 0;
>>>> -    enum instruction_index instr = 0;
>>>> -    u8 buf[MAX_INST_LEN];
>>>> -    const u8 *opcode = NULL;
>>>> -    unsigned long fetch_addr, fetch_limit;
>>>> -    unsigned int fetch_len, max_len;
>>>> +    struct hvm_emulate_ctxt ctxt;
>>>> +    struct x86_emulate_state *state;
>>>> +    unsigned int inst_len, j, modrm_rm, modrm_reg;
>>>> +    int modrm_mod;
>>>>  
>>>> +#ifdef NDEBUG
>>> Presumably this is just for your testing?
>> No, I actually meant it to stay that way. Along the lines of the extra
>> debugging code we have in map_domain_page().
> 
> I was never very happy with the older version of this debugging.  Surely
> in a case like this, we should use the intercept information when
> available, and check it against the emulator in a debug build.
> 
> That way, we don't entirely change the underlying logic in this function
> between a debug and non debug build.

But that is exactly what the code is doing:

#ifndef NDEBUG
    if ( vmcb->exitcode == VMEXIT_IOIO )
        j = vmcb->exitinfo2 - vmcb->rip;
    else
        j = svm_nextrip_insn_length(v);
    if ( j && j != inst_len )
    {
        gprintk(XENLOG_WARNING, "insn-len[%02x]=%u (exp %u)\n",
                ctxt.ctxt.opcode, inst_len, j);
        return j;
    }
#endif

I.e. in case of a mismatch we use the data from hardware, plus a
message gets logged. In case of a match we further exercise the
opcode lookup logic, which non-debug builds would never hit on
capable hardware.

>>>> @@ -1658,6 +1662,11 @@ x86_decode_base(
>>>>  
>>>>      switch ( ctxt->opcode )
>>>>      {
>>>> +    case 0x90: /* nop / pause */
>>>> +        if ( repe_prefix() )
>>>> +            ctxt->opcode |= X86EMUL_OPC_F3(0, 0);
>>>> +        break;
>>> Why is it necessary to special case the rep prefix handling in this case?
>> Because SVM's pause intercept should not mistakenly also accept a
>> plain NOP.
> 
> How about a comment to this effect:
> 
> /* Note: Prefixes such as rep/repne are only encoded when semantically
> meaningful to the instruction, to reduce the complexity of interpreting
> this opcode representation. */

Yes, I've added a slight variation of this to the definition of
X86EMUL_OPC_PFX_MASK.

Jan


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