Re: [PATCHv3 3/4] x86/insn-eval: Introduce insn_decode_mmio()

[Tom Lendacky <thomas.lendacky@amd.com>]

On 11/3/21 10:45 AM, Kirill A. Shutemov wrote:
> From: "Kirill A. Shutemov" <kirill@shutemov.name>
> 
> In preparation for sharing MMIO instruction decode between SEV-ES and
> TDX, factor out the common decode into a new insn_decode_mmio() helper.
> 
> For regular virtual machine, MMIO is handled by the VMM and KVM
> emulates instructions that caused MMIO. But, this model doesn't work
> for a secure VMs (like SEV or TDX) as VMM doesn't have access to the
> guest memory and register state. So, for TDX or SEV VMM needs
> assistance in handling MMIO. It induces exception in the guest. Guest
> has to decode the instruction and handle it on its own.
> 
> The code is based on the current SEV MMIO implementation.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reviewed-by: Andi Kleen <ak@linux.intel.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> ---
>   arch/x86/include/asm/insn-eval.h | 12 +++++
>   arch/x86/lib/insn-eval.c         | 84 ++++++++++++++++++++++++++++++++
>   2 files changed, 96 insertions(+)
> 
> diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h
> index 5c6869565fb3..010d31726d06 100644
> --- a/arch/x86/include/asm/insn-eval.h
> +++ b/arch/x86/include/asm/insn-eval.h
> @@ -29,4 +29,16 @@ int insn_fetch_from_user_inatomic(struct pt_regs *regs,
>   bool insn_decode_from_regs(struct insn *insn, struct pt_regs *regs,
>   			   unsigned char buf[MAX_INSN_SIZE], int buf_size);
>   
> +enum mmio_type {
> +	MMIO_DECODE_FAILED,
> +	MMIO_WRITE,
> +	MMIO_WRITE_IMM,
> +	MMIO_READ,
> +	MMIO_READ_ZERO_EXTEND,
> +	MMIO_READ_SIGN_EXTEND,
> +	MMIO_MOVS,
> +};
> +
> +enum mmio_type insn_decode_mmio(struct insn *insn, int *bytes);
> +
>   #endif /* _ASM_X86_INSN_EVAL_H */
> diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> index 385232a67281..ffd040c1c97b 100644
> --- a/arch/x86/lib/insn-eval.c
> +++ b/arch/x86/lib/insn-eval.c
> @@ -1560,3 +1560,87 @@ bool insn_decode_from_regs(struct insn *insn, struct pt_regs *regs,
>   
>   	return true;
>   }
> +
> +/**
> + * insn_decode_mmio() - Decode a MMIO instruction
> + * @insn:	Structure to store decoded instruction
> + * @bytes:	Returns size of memory operand
> + *
> + * Decodes instruction that used for Memory-mapped I/O.
> + *
> + * Returns:
> + *
> + * Type of the instruction. Size of the memory operand is stored in
> + * @bytes. If decode failed, MMIO_DECODE_FAILED returned.
> + */
> +enum mmio_type insn_decode_mmio(struct insn *insn, int *bytes)
> +{
> +	int type = MMIO_DECODE_FAILED;

Wouldn't "enum mmio_type" vs "int" be better here ?

Thanks,
Tom

> +
> +	*bytes = 0;
> +
> +	if (insn_get_opcode(insn))
> +		return MMIO_DECODE_FAILED;
> +
> +	switch (insn->opcode.bytes[0]) {
> +	case 0x88: /* MOV m8,r8 */
> +		*bytes = 1;
> +		fallthrough;
> +	case 0x89: /* MOV m16/m32/m64, r16/m32/m64 */
> +		if (!*bytes)
> +			*bytes = insn->opnd_bytes;
> +		type = MMIO_WRITE;
> +		break;
> +
> +	case 0xc6: /* MOV m8, imm8 */
> +		*bytes = 1;
> +		fallthrough;
> +	case 0xc7: /* MOV m16/m32/m64, imm16/imm32/imm64 */
> +		if (!*bytes)
> +			*bytes = insn->opnd_bytes;
> +		type = MMIO_WRITE_IMM;
> +		break;
> +
> +	case 0x8a: /* MOV r8, m8 */
> +		*bytes = 1;
> +		fallthrough;
> +	case 0x8b: /* MOV r16/r32/r64, m16/m32/m64 */
> +		if (!*bytes)
> +			*bytes = insn->opnd_bytes;
> +		type = MMIO_READ;
> +		break;
> +
> +	case 0xa4: /* MOVS m8, m8 */
> +		*bytes = 1;
> +		fallthrough;
> +	case 0xa5: /* MOVS m16/m32/m64, m16/m32/m64 */
> +		if (!*bytes)
> +			*bytes = insn->opnd_bytes;
> +		type = MMIO_MOVS;
> +		break;
> +
> +	case 0x0f: /* Two-byte instruction */
> +		switch (insn->opcode.bytes[1]) {
> +		case 0xb6: /* MOVZX r16/r32/r64, m8 */
> +			*bytes = 1;
> +			fallthrough;
> +		case 0xb7: /* MOVZX r32/r64, m16 */
> +			if (!*bytes)
> +				*bytes = 2;
> +			type = MMIO_READ_ZERO_EXTEND;
> +			break;
> +
> +		case 0xbe: /* MOVSX r16/r32/r64, m8 */
> +			*bytes = 1;
> +			fallthrough;
> +		case 0xbf: /* MOVSX r32/r64, m16 */
> +			if (!*bytes)
> +				*bytes = 2;
> +			type = MMIO_READ_SIGN_EXTEND;
> +			break;
> +		}
> +		break;
> +	}
> +
> +	return type;
> +}
>