Messages in this thread | | | Date | Thu, 19 May 2022 11:33:08 -0700 | Subject | Re: [PATCH] x86/tdx: Handle load_unaligned_zeropad() page-cross to a shared page | From | Dave Hansen <> |
| |
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)
| |