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

Tom Lendacky
2021-11-29T21:31:23Z

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;
> +}
>