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

Dave Hansen
2022-02-25T19:46:19Z

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.