Re: [PATCHv4 17/30] x86/tdx: Add port I/O emulation

[Dave Hansen <dave.hansen@intel.com>]

On 2/24/22 07:56, Kirill A. Shutemov wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> TDX hypervisors cannot emulate instructions directly. This includes
> port I/O which is normally emulated in the hypervisor. All port I/O
> instructions inside TDX trigger the #VE exception in the guest and
> would be normally emulated there.
> 
> Use a hypercall to emulate port I/O. Extend the
> tdx_handle_virt_exception() and add support to handle the #VE due to
> port I/O instructions.
> 
> String I/O operations are not supported in TDX. Unroll them by declaring
> CC_ATTR_GUEST_UNROLL_STRING_IO confidential computing attribute.
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Reviewed-by: Andi Kleen <ak@linux.intel.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  arch/x86/coco/core.c |  7 +++++-
>  arch/x86/coco/tdx.c  | 54 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
> index 9113baebbfd2..5615b75e6fc6 100644
> --- a/arch/x86/coco/core.c
> +++ b/arch/x86/coco/core.c
> @@ -18,7 +18,12 @@ static u64 cc_mask __ro_after_init;
>  
>  static bool intel_cc_platform_has(enum cc_attr attr)
>  {
> -	return false;
> +	switch (attr) {
> +	case CC_ATTR_GUEST_UNROLL_STRING_IO:
> +		return true;
> +	default:
> +		return false;
> +	}
>  }
>  
>  /*
> diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
> index 15519e498679..2e342760b1d2 100644
> --- a/arch/x86/coco/tdx.c
> +++ b/arch/x86/coco/tdx.c
> @@ -19,6 +19,12 @@
>  #define EPT_READ	0
>  #define EPT_WRITE	1
>  
> +/* See Exit Qualification for I/O Instructions in VMX documentation */
> +#define VE_IS_IO_IN(e)		((e) & BIT(3))
> +#define VE_GET_IO_SIZE(e)	(((e) & GENMASK(2, 0)) + 1)
> +#define VE_GET_PORT_NUM(e)	((e) >> 16)
> +#define VE_IS_IO_STRING(e)	((e) & BIT(4))
> +
>  static struct {
>  	unsigned int gpa_width;
>  	unsigned long attributes;
> @@ -292,6 +298,52 @@ static bool handle_mmio(struct pt_regs *regs, struct ve_info *ve)
>  	return true;
>  }
>  
> +/*
> + * Emulate I/O using hypercall.
> + *
> + * Assumes the IO instruction was using ax, which is enforced
> + * by the standard io.h macros.
> + *
> + * Return True on success or False on failure.
> + */
> +static bool handle_io(struct pt_regs *regs, u32 exit_qual)
> +{
> +	struct tdx_hypercall_args args = {
> +		.r10 = TDX_HYPERCALL_STANDARD,
> +		.r11 = EXIT_REASON_IO_INSTRUCTION,
> +	};
> +	int size, port;
> +	u64 mask;
> +	bool in, ret;
> +
> +	if (VE_IS_IO_STRING(exit_qual))
> +		return false;
> +
> +	in   = VE_IS_IO_IN(exit_qual);
> +	size = VE_GET_IO_SIZE(exit_qual);
> +	port = VE_GET_PORT_NUM(exit_qual);
> +	mask = GENMASK(BITS_PER_BYTE * size, 0);
> +
> +	args.r12 = size;
> +	args.r13 = !in;
> +	args.r14 = port;
> +	args.r15 = in ? 0 : regs->ax;
> +
> +	/*
> +	 * Emulate the I/O read/write via hypercall. More info about
> +	 * ABI can be found in TDX Guest-Host-Communication Interface
> +	 * (GHCI) section titled "TDG.VP.VMCALL<Instruction.IO>".
> +	 */
> +	ret = !__tdx_hypercall(&args, in ? TDX_HCALL_HAS_OUTPUT : 0);
> +	if (!ret || !in)
> +		return ret;
> +
> +	regs->ax &= ~mask;
> +	regs->ax |= ret ? args.r11 & mask : UINT_MAX;
> +
> +	return ret;
> +}


I can't help but think this would be more clear if the in and !in sideds
were separated:

	if (!in) {
		args.r15 = regs->ax;
		ret = !__tdx_hypercall(&args, 0);
	} else {
		args.r15 = 0;
		ret = !__tdx_hypercall(&args, TDX_HCALL_HAS_OUTPUT);
		regs->ax &= ~mask;
		if (ret)
			regs->ax |= args.r11 & mask
		else
			regs->ax |= UINT_MAX;
	}

	return ret;

The ->ax mangling also needs a comment.  It looks like it's trying to
inject -1 when there's a failure.  Even if the roots of this are in the
TDX spec(s), it would be nice to express the intent of this code.

I also really dislike using 'ret' as a bool return type.  Wouldn't this
be about a billion times clearer if 'ret' was renamed to 'success'?

Right now, this code is effectively doing:

	if (!ret)
		return error;

Which is actually functionally correct here.  All other int-typed 'ret'
code in the kernel do this:

	if (ret)
		return error;

which would be functionally _wrong_.  Can you please take mercy on my
little brain an at least make the code look different if it behaves the
opposite from the same literal code in other spots in the kernel?

Heck, even 'bool handled' would be pretty nice.

>  void tdx_get_ve_info(struct ve_info *ve)
>  {
>  	struct tdx_module_output out;
> @@ -347,6 +399,8 @@ static bool virt_exception_kernel(struct pt_regs *regs, struct ve_info *ve)
>  		return handle_cpuid(regs);
>  	case EXIT_REASON_EPT_VIOLATION:
>  		return handle_mmio(regs, ve);
> +	case EXIT_REASON_IO_INSTRUCTION:
> +		return handle_io(regs, ve->exit_qual);
>  	default:
>  		pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
>  		return false;