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 14:42:24 +0100	[thread overview]
Message-ID: <b87626a6-3ed5-8f0c-2f38-1b981e523d1a@citrix.com> (raw)
In-Reply-To: <57DA61F7020000780010F124@prv-mh.provo.novell.com>

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.

>>> @@ -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. */

~Andrew

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