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: <e0a91991-9648-5c3e-62dc-342c9d8ed957@intel.com>
Date:   Fri, 25 Feb 2022 11:46:04 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
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, aarcange@redhat.com,
        ak@linux.intel.com, dan.j.williams@intel.com, david@redhat.com,
        hpa@zytor.com, jgross@suse.com, jmattson@google.com,
        joro@8bytes.org, jpoimboe@redhat.com, knsathya@kernel.org,
        pbonzini@redhat.com, sdeep@vmware.com, seanjc@google.com,
        tony.luck@intel.com, vkuznets@redhat.com, wanpengli@tencent.com,
        thomas.lendacky@amd.com, brijesh.singh@amd.com, x86@kernel.org,
        linux-kernel@vger.kernel.org,
        Sean Christopherson <sean.j.christopherson@intel.com>
References: <20220224155630.52734-1-kirill.shutemov@linux.intel.com>
 <20220224155630.52734-8-kirill.shutemov@linux.intel.com>
 <c6ad42a0-ab19-befd-5760-2bcc992df732@intel.com>
 <20220225193059.6zn6owzpbggxfqqv@black.fi.intel.com>
From:   Dave Hansen <dave.hansen@intel.com>
Subject: Re: [PATCHv4 07/30] x86/traps: Add #VE support for TDX guest
In-Reply-To: <20220225193059.6zn6owzpbggxfqqv@black.fi.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.