Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <c761e774-8014-6fa9-cf21-e7cd8f7aca54@intel.com>
Date:   Tue, 17 May 2022 11:14:13 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Subject: Re: [PATCH] x86/tdx: Handle load_unaligned_zeropad() page-cross to a
 shared page
Content-Language: en-US
To:     "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc:     tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        luto@kernel.org, peterz@infradead.org,
        sathyanarayanan.kuppuswamy@linux.intel.com, ak@linux.intel.com,
        dan.j.williams@intel.com, david@redhat.com, hpa@zytor.com,
        seanjc@google.com, thomas.lendacky@amd.com, x86@kernel.org,
        linux-kernel@vger.kernel.org
References: <20220517153021.11116-1-kirill.shutemov@linux.intel.com>
 <e73cb19e-7dab-2fc1-b947-fac70fd607d2@intel.com>
 <20220517174042.v6s7wm3u5j2ebaoq@black.fi.intel.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <20220517174042.v6s7wm3u5j2ebaoq@black.fi.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.