Re: [PATCH] x86/tdx: Handle load_unaligned_zeropad() page-cross to a shared page

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

On 5/19/22 11:07, Kirill A. Shutemov wrote:
> On Tue, May 17, 2022 at 03:16:42PM -0700, Dave Hansen wrote:
>> See?  Now everybody that goes and writes a new #VE exception helper has
>> a chance of actually getting this right.  As it stands, if someone adds
>> one more of these, they'll probably get random behavior.  This way, they
>> actually have to choose.  They _might_ even go looking at the SDM.
> 
> Okay. See below. Does it match what you had in mind?

Looks close.

> BTW, I found a bug in tdx_early_handle_ve(). It didn't update RIP.
> I don't know how it happend. Maybe got lost on the way upstream.

Huh, so refactoring things instead of depending on magic hidden behavior
helps find bugs?  Interesting.  ;)

> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 9955b5a89df8..d2635ac52d9b 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -123,7 +123,7 @@ static u64 __cpuidle __halt(const bool irq_disabled, const bool do_sti)
>  	return __tdx_hypercall(&args, do_sti ? TDX_HCALL_ISSUE_STI : 0);
>  }
>  
> -static bool handle_halt(void)
> +static int handle_halt(struct ve_info *ve)
>  {
>  	/*
>  	 * Since non safe halt is mainly used in CPU offlining
> @@ -134,9 +134,9 @@ static bool handle_halt(void)
>  	const bool do_sti = false;
>  
>  	if (__halt(irq_disabled, do_sti))
> -		return false;
> +		return -EIO;
>  
> -	return true;
> +	return ve->instr_len;
>  }

Ideally each of these would include a comment about why we can get the
isntruction length from ve_info.  That "why" is currently a bit weak,
but it's something like:

	/*
	 * In TDX guests, HLT is configured to cause exits.  Assume that
	 * the TDX module has provided the "VM-exit instruction length".
	 */

It would be nice to have some central discussion of this too to explain
that the TDX documentation is currently lacking here, but we don't need
to repeat that part in a comment 6 different times.

...
>  /* Handle the kernel #VE */
> -static bool virt_exception_kernel(struct pt_regs *regs, struct ve_info *ve)
> +static int virt_exception_kernel(struct pt_regs *regs, struct ve_info *ve)
>  {

/*
 * Handle kernel #VEs.  On success, returns the number of
 * bytes RIP should be incremented (>=0) or -errno on error.
 */

>  	switch (ve->exit_reason) {
>  	case EXIT_REASON_HLT:
> -		return handle_halt();
> +		return handle_halt(ve);
>  	case EXIT_REASON_MSR_READ:
> -		return read_msr(regs);
> +		return read_msr(regs, ve);
>  	case EXIT_REASON_MSR_WRITE:
> -		return write_msr(regs);
> +		return write_msr(regs, ve);
>  	case EXIT_REASON_CPUID:
> -		return handle_cpuid(regs);
> +		return handle_cpuid(regs, ve);
>  	case EXIT_REASON_EPT_VIOLATION:
>  		return handle_mmio(regs, ve);
>  	case EXIT_REASON_IO_INSTRUCTION:
> -		return handle_io(regs, ve->exit_qual);
> +		return handle_io(regs, ve);
>  	default:
>  		pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
> -		return false;
> +		return -EIO;
>  	}
>  }
>  
>  bool tdx_handle_virt_exception(struct pt_regs *regs, struct ve_info *ve)
>  {
> -	bool ret;
> +	int ret;

'ret' is usually used for return values of *this* function.

Let's give it a better name, please.

>  	if (user_mode(regs))
>  		ret = virt_exception_user(regs, ve);
>  	else
>  		ret = virt_exception_kernel(regs, ve);
>  
> +	if (ret < 0)
> +		return false;
> +
>  	/* After successful #VE handling, move the IP */
> -	if (ret)
> -		regs->ip += ve->instr_len;
> +	regs->ip += ret;
>  
> -	return ret;
> +	return true;
>  }
>  
>  static bool tdx_tlb_flush_required(bool private)