Re: [PATCH] kvm: implement VEX prefix decoder, bextr/andn

[Paolo Bonzini <pbonzini@xxxxxxxxxx>]

Thanks, I have some comments on the decoding logic.

On 26/06/2018 14:14, Mason Lee Back wrote:
> +	if (ctxt->b == 0xC4 || ctxt->b == 0xC5) {

This should be exactly the condition that you are removing below:

	if ((ctxt->b == 0xc5 || ctxt->b == 0xc4) &&
	    (mode == X86EMUL_MODE_PROT64 || (ctxt->modrm & 0xc0) == 0xc0))

so that you don't need the "reinterpret as LES or LDS" todo.  It should also
be moved earlier, right after done_prefixes.  The existing code starting
at

	/* Opcode byts(s).  */

and ending just before

	ctxt->d = opcode.flags;

will become the "else" branch.

Otherwise looks at least... sane. :)

Paolo

> +		ctxt->vex.prefix = ctxt->b;
> +		if (ctxt->b == 0xC4) {
> +			ctxt->vex.value = insn_fetch(u16, ctxt);
> +		} else {
> +			ctxt->vex.value = insn_fetch(u8, ctxt) << 8;
> +			ctxt->vex.r = ctxt->vex.w;
> +			ctxt->vex.w = 1;
> +			ctxt->vex.x = 1;
> +			ctxt->vex.b = 1;
> +			ctxt->vex.m = 1;
> +		}
>  
> -	/* vex-prefix instructions are not implemented */
> -	if (ctxt->opcode_len == 1 && (ctxt->b == 0xc5 || ctxt->b == 0xc4) &&
> -	    (mode == X86EMUL_MODE_PROT64 || (ctxt->modrm & 0xc0) == 0xc0)) {
> -		ctxt->d = NotImpl;
> +		if (mode != X86EMUL_MODE_PROT64 && (!ctxt->vex.r || !ctxt->vex.x)) {
> +			/* todo: reinterpret as LES (0xC4) or LDS (0xC5) instruction */
> +			return EMULATION_FAILED;
> +		}
> +
> +		ctxt->rex_prefix |= ctxt->vex.r ? 0 : (1 << 2); /* rex.r */
> +		ctxt->rex_prefix |= ctxt->vex.x ? 0 : (1 << 1); /* rex.x */
> +		ctxt->rex_prefix |= ctxt->vex.b ? 0 : (1 << 0); /* rex.b */
> +		if (mode == X86EMUL_MODE_PROT64 && ctxt->vex.w) {
> +			ctxt->op_bytes = 8;
> +		}

Please set ctxt->rex_prefix too for consistency.
> +
> +		ctxt->b = insn_fetch(u8, ctxt);
> +		switch (ctxt->vex.m) {
> +		case 1:
> +			opcode = twobyte_table[ctxt->b];

"ctxt->opcode_len = 2;" missing here.

> +			break;
> +		case 2:
> +			opcode = opcode_map_0f_38[ctxt->b];

"ctxt->opcode_len = 3;" missing here.

> +			break;
> +		case 3:
> +			/* KVM doesn't support this */
> +			return EMULATION_FAILED;
> +		default:
> +			return EMULATION_FAILED;
> +		}
>  	}