[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220517201710.ixbpsaga5jzvokvy@black.fi.intel.com>
Date: Tue, 17 May 2022 23:17:10 +0300
From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
To: Dave Hansen <dave.hansen@...el.com>, seanjc@...gle.com
Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
luto@...nel.org, peterz@...radead.org,
sathyanarayanan.kuppuswamy@...ux.intel.com, ak@...ux.intel.com,
dan.j.williams@...el.com, david@...hat.com, hpa@...or.com,
thomas.lendacky@....com, x86@...nel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/tdx: Handle load_unaligned_zeropad() page-cross to a
shared page
On Tue, May 17, 2022 at 11:14:13AM -0700, Dave Hansen wrote:
> 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.
Okay. Will do.
> > 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.
For EPT violation, ve->instr_len has *something* (not zero) that doesn't
match the actual instruction size. I dig out that it is filled with data
from VMREAD(0x440C), but I don't know where is the ultimate origin of the
data.
I don't understand virtualization side of the thing well enough.
Maybe someone who knows virtualtion could comment here. Sean?
--
Kirill A. Shutemov
Powered by blists - more mailing lists