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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Subject: Re: [PATCH 09/17] SVM: use generic instruction decoding
Date: Tue, 27 Sep 2016 16:53:19 +0100	[thread overview]
Message-ID: <4e1f10dd-0013-6520-0fb1-c58249ce5e01@citrix.com> (raw)
In-Reply-To: <57EA969E0200007800112DE6@prv-mh.provo.novell.com>

On 27/09/16 14:56, Jan Beulich wrote:
>>>> On 27.09.16 at 15:42, <andrew.cooper3@citrix.com> wrote:
>> On 15/09/16 07:55, Jan Beulich wrote:
>>>>>> On 14.09.16 at 19:56, <andrew.cooper3@citrix.com> 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.

Ah yes - I see now.  The split between #ifdef NDEBUG and #ifndef NDEBUG
is the confusing factor.

~Andrew

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