Re: [PATCHv4 07/30] x86/traps: Add #VE support for TDX guest

[Dave Hansen <dave.hansen@intel.com>]

On 2/25/22 11:30, Kirill A. Shutemov wrote:
> On Thu, Feb 24, 2022 at 10:36:02AM -0800, Dave Hansen wrote:
>> On 2/24/22 07:56, Kirill A. Shutemov wrote:
>>> Virtualization Exceptions (#VE) are delivered to TDX guests due to
>>> specific guest actions which may happen in either user space or the
>>> kernel:
>>>
>>>  * Specific instructions (WBINVD, for example)
>>>  * Specific MSR accesses
>>>  * Specific CPUID leaf accesses
>>>  * Access to unmapped pages (EPT violation)
>>
>> Considering that you're talking partly about userspace, it would be nice
>> to talk about what "unmapped" really means here.
> 
> I'm not sure what you want to see here. Doesn't EPT violation describe it?
> 
> It can happen to userspace too, but we don't expect it to be use used and
> SIGSEGV the process if it happens.

How about just:

 * Access to specific guest physical addresses

That makes it clear that we're not really talking about userspace
unmapped pages.

...
>>> +	 * module also treats virtual NMIs as inhibited if the #VE valid flag is
>>> +	 * set, e.g. so that NMI=>#VE will not result in a #DF.
>>> +	 */
>>
>> Are we missing anything valuable if we just trim the comment down to
>> something like:
>>
>> 	/*
>> 	 * Called during #VE handling to retrieve the #VE info from the
>> 	 * TDX module.
>>  	 *
>> 	 * This should called done early in #VE handling.  A "nested"
>> 	 * #VE which occurs before this will raise a #DF and is not
>> 	 * recoverable.
>> 	 */
> 
> This variant of the comment lost information about #VE-valid flag and
> doesn't describe how NMI is inhibited.

IMNHO, the "#VE valid" flag is a super-fine implementation detail.  I'd
personally deal with that in Documentation or the changelog instead of a
comment.

>>> +#ifdef CONFIG_INTEL_TDX_GUEST
>>> +
>>> +#define VE_FAULT_STR "VE fault"
>>> +
>>> +static void ve_raise_fault(struct pt_regs *regs, long error_code)
>>> +{
>>> +	if (user_mode(regs)) {
>>> +		gp_user_force_sig_segv(regs, X86_TRAP_VE, error_code, VE_FAULT_STR);
>>> +		return;
>>> +	}
>>> +
>>> +	if (gp_try_fixup_and_notify(regs, X86_TRAP_VE, error_code, VE_FAULT_STR))
>>> +		return;
>>> +
>>> +	die_addr(VE_FAULT_STR, regs, error_code, 0);
>>> +}
>>> +
>>> +/*
>>> + * Virtualization Exceptions (#VE) are delivered to TDX guests due to
>>> + * specific guest actions which may happen in either user space or the
>>> + * kernel:
>>> + *
>>> + *  * Specific instructions (WBINVD, for example)
>>> + *  * Specific MSR accesses
>>> + *  * Specific CPUID leaf accesses
>>> + *  * Access to unmapped pages (EPT violation)
>>> + *
>>> + * In the settings that Linux will run in, virtualization exceptions are
>>> + * never generated on accesses to normal, TD-private memory that has been
>>> + * accepted.
>>
>> This actually makes a lot more sense as a code comment than changelog.
>> It would be really nice to circle back here and actually refer to the
>> functions that accept memory.
> 
> We don't have such functions at this point in the patchset. Do you want
> the comment to be updated once we get them introduced?

Yes, please.  Supplement the comment when the functions are introduced
later.