lkml.org 
[lkml]   [2022]   [May]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] x86/tdx: Handle load_unaligned_zeropad() page-cross to a shared page
From
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)

\
 
 \ /
  Last update: 2022-05-19 20:34    [W:0.098 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site