Messages in this thread | | | Date | Tue, 17 May 2022 11:14:13 -0700 | Subject | Re: [PATCH] x86/tdx: Handle load_unaligned_zeropad() page-cross to a shared page | From | Dave Hansen <> |
| |
On 5/17/22 10:40, Kirill A. Shutemov wrote: >> >> ve_info is a software structure. Why not just add a: >> >> bool ip_adjusted; >> >> which defaults to false, then we have: >> >> /* >> * Adjust RIP if the exception was handled >> * but RIP was not adjusted. >> */ >> if (!ret && !ve_info->ip_adjusted) >> regs->ip += ve_info->instr_len; >> >> One other oddity I just stumbled upon: >> >> static bool handle_mmio(struct pt_regs *regs, struct ve_info *ve) >> { >> ... >> ve->instr_len = insn.length; >> >> Why does that need to override 've->instr_len'? What was wrong with the >> gunk in r10 that came out of TDX_GET_VEINFO? > TDX module doesn't decode MMIO instruction and does not provide valid size > of it. We had to do it manually, based on decoding.
That's worth a comment, don't you think? I'd add one both in where the ve_info is filled and where ve->instr_len is adjusted.
> Given that we had to adjust IP in handle_mmio() anyway, do you still think > "ve->instr_len = 0;" is wrong? I dislike ip_adjusted more.
Something is wrong about it.
You could call it 've->instr_bytes_to_handle' or something. Then it makes actual logical sense when you handle it to zero it out. I just want it to be more explicit when the upper levels need to do something.
Does ve->instr_len==0 both when the TDX module isn't providing instruction sizes *and* when no handling is necessary? That seems like an unfortunate logical multiplexing of 0.
| |