Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <89f55141-a71c-1fd8-e1be-96355c63c03a@intel.com>
Date:   Wed, 25 May 2022 09:02:48 -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: [PATCHv3 2/3] x86/tdx: Clarify RIP adjustments in #VE handler
Content-Language: en-US
To:     "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        luto@kernel.org, peterz@infradead.org
Cc:     ak@linux.intel.com, dan.j.williams@intel.com, david@redhat.com,
        hpa@zytor.com, linux-kernel@vger.kernel.org,
        sathyanarayanan.kuppuswamy@linux.intel.com, seanjc@google.com,
        thomas.lendacky@amd.com, x86@kernel.org
References: <20220524221012.62332-1-kirill.shutemov@linux.intel.com>
 <20220524221012.62332-3-kirill.shutemov@linux.intel.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <20220524221012.62332-3-kirill.shutemov@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/24/22 15:10, Kirill A. Shutemov wrote:
> +static int ve_instr_len(struct ve_info *ve)
> +{
> +	/*
> +	 * If the #VE happened due to instruction execution, GET_VEINFO
> +	 * provides info on the instruction.
> +	 *
> +	 * For #VE due to EPT violation, info provided by GET_VEINFO not usable
> +	 * and kernel has to decode instruction manually to find out its
> +	 * length. Catch such cases.
> +	 */
> +	if (WARN_ON_ONCE(ve->exit_reason == EXIT_REASON_EPT_VIOLATION))
> +		return 0;
> +
> +	return ve->instr_len;
> +}

I'm not super happy with how this comment ended up.  First, let's put
the comment next to the code to which it applies, like:

	/*
	 * ve->instr_len is not defined for EPT violations.  For those,
	 * the kernel must decode instructions manually and should not
	 * be using this function.
	 */
	if (WARN_ON_ONCE(ve->exit_reason == EXIT_REASON_EPT_VIOLATION))
		return 0;

	/*
	 * Assume that the #VE occurred due to instruction execution.
	 */
	return ve->instr_len;

Second, there also needs to be *something* here to link this back to the
TDX module spec and the VMCS documentation.  To make actual sense of
this, you need to tie together something like three or four pieces of
disparate information scattered across two massive documents.

Intel really made this quite the scavenger hunt.  Time to atone for the
sins of your colleagues by tying all of those things together in one place.