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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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().

>>      if ( (inst_len = svm_nextrip_insn_length(v)) != 0 )
>>          return inst_len;
>>  
>>      if ( vmcb->exitcode == VMEXIT_IOIO )
>>          return vmcb->exitinfo2 - vmcb->rip;
>> +#endif
>>  
>> -    /* Fetch up to the next page break; we'll fetch from the next page
>> -     * later if we have to. */
>> -    fetch_addr = svm_rip2pointer(v, &fetch_limit);
>> -    if ( vmcb->rip > fetch_limit )
>> -        return 0;
>> -    max_len = min(fetch_limit - vmcb->rip + 1, MAX_INST_LEN + 0UL);
>> -    fetch_len = min_t(unsigned int, max_len,
>> -                      PAGE_SIZE - (fetch_addr & ~PAGE_MASK));
>> -    if ( !fetch(vmcb, buf, fetch_addr, fetch_len) )
>> +    ASSERT(v == current);
>> +    hvm_emulate_prepare(&ctxt, guest_cpu_user_regs());
>> +    hvm_emulate_init(&ctxt, NULL, 0);
>> +    state = x86_decode_insn(&ctxt.ctxt, hvmemul_insn_fetch);
>> +    if ( IS_ERR_OR_NULL(state) )
>>          return 0;
>>  
>> -    while ( (inst_len < max_len) && is_prefix(buf[inst_len]) )
>> -    {
>> -        inst_len++;
>> -        if ( inst_len >= fetch_len )
>> -        {
>> -            if ( !fetch(vmcb, buf + fetch_len, fetch_addr + fetch_len,
>> -                        max_len - fetch_len) )
>> -                return 0;
>> -            fetch_len = max_len;
>> -        }
>> +    inst_len = x86_insn_length(state, &ctxt.ctxt);
>> +    modrm_mod = x86_insn_modrm(state, &modrm_rm, &modrm_reg);
>> +    x86_emulate_free_state(state);
> 
> From an API point of view, it is weird to have x86_emulate_free_state()
> without a matching allocation function.  Perhaps that is just me.

With x86_decode_insn() returning the state, that to me _is_ the
allocation function.

> However, the x86_insn_modrm() API is definitely more weird.  Wouldn't it
> be more natural to take optional pointers for the mod, rm and reg parts
> individually?

I could change it to that, but I did it this way because without mod
at least rm is meaningless. Or said differently, I can't really see there
being a caller not caring about mod.

>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -382,7 +382,7 @@ struct operand {
>>      } mem;
>>  };
>>  #ifdef __x86_64__
>> -#define REG_POISON ((unsigned long *) 0x8086000000008086UL) /* 
>> non-canonical */
>> +#define REG_POISON ((void *)0x8086000000008086UL) /* non-canonical */
>>  #else
>>  #define REG_POISON NULL /* 32-bit builds are for user-space, so NULL is OK. 
>> */
>>  #endif
> 
> Given that these are now used for general pointer poisoning, they should
> be renamed.  There are only 3 instances.

Okay. I'll make the PTR_POISON then.

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

>> +unsigned int
>> +x86_insn_length(const struct x86_emulate_state *state,
>> +                const struct x86_emulate_ctxt *ctxt)
>> +{
>> +    check_state(state);
>> +
>> +    return state->eip - ctxt->regs->eip;
> 
> Is it worth stashing a starting eip?  This calculation will go wrong
> after the emulated state has been committed.

This function (taking a state parameter) can't be called by users of
x86_emulate(), and I don't think we need to cater for callers
committing state themselves - they should clearly use the result of
this function for what to commit in the first place.

Jan

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