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: <30b7f884-11a3-5d2e-d883-17e2d5af7c74@intel.com>
Date:   Thu, 24 Feb 2022 11:36:47 -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:     Sean Christopherson <seanjc@google.com>
Cc:     "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        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, 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
References: <20220224155630.52734-1-kirill.shutemov@linux.intel.com>
 <20220224155630.52734-10-kirill.shutemov@linux.intel.com>
 <67dfdb0c-c483-ec3c-4fb8-57086ffde9bf@intel.com>
 <YhfWwHE4nZfQWPCl@google.com>
From:   Dave Hansen <dave.hansen@intel.com>
Subject: Re: [PATCHv4 09/30] x86/tdx: Add MSR support for TDX guests
In-Reply-To: <YhfWwHE4nZfQWPCl@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/24/22 11:04, Sean Christopherson wrote:
> On Thu, Feb 24, 2022, Dave Hansen wrote:
>> On 2/24/22 07:56, Kirill A. Shutemov wrote:
>>> diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
>>> index 0a2e6be0cdae..89992593a209 100644
>>> --- a/arch/x86/coco/tdx.c
>>> +++ b/arch/x86/coco/tdx.c
>>> @@ -116,6 +116,44 @@ void __cpuidle tdx_safe_halt(void)
>>>  		WARN_ONCE(1, "HLT instruction emulation failed\n");
>>>  }
>>>  
>>> +static bool read_msr(struct pt_regs *regs)
>>> +{
>>> +	struct tdx_hypercall_args args = {
>>> +		.r10 = TDX_HYPERCALL_STANDARD,
>>> +		.r11 = EXIT_REASON_MSR_READ,
>> Just a minor note: these "EXIT_REASON_FOO"'s in r11 are effectively
>> *the* hypercall being made, right?
>>
>> The hypercall is being made in response to what would have otherwise
>> been a MSR read VMEXIT.  But, it's a *bit* goofy to see them here when
>> the TDX guest isn't doing any kind of VMEXIT.
> But the TDX guest is doing a VM-Exit, that's all TDCALL is, an exit to the host.
> r10 states that this is a GHCI-standard hypercall, r11 holds the reason why the
> guest is exiting to the host.  The guest could pretty it up by redefining all the
> VM-Exit reasons as TDX_REQUEST_MSR_READ or whatever, but IMO diverging from
> directly using EXIT_REASON_* will be annoying in the long run, e.g. will make it
> more difficult to grep KVM + kernel to understand the end-to-end flow.

I understand that it looks like an "exit" if you know how it's
implemented, know the history and squint at it funny.  But, r11 is not
an exit reason.  It's a hypercall number that just sometimes happens to
also take an exit reason as a convention.  Don't confuse that with "r11
*is* an exit reason".

Heck, look at the GHCI spec.  Does it simply cede some of the
sub-function space and map them directly to VMEXIT reasons?  Nope.  It
goes to the trouble of individually defining them:

	12 Instruction.HLT
	30 Instruction.IO
	31 Instruction.RDMSR
	32 Instruction.WRMSR
	48 #VE.RequestMMIO
	65 Instruction.PCONFIG

I'm not saying we need 15 new #defines.  It would be really nice like
you say to be able to connect the host and guest sides with a grep.  I
wouldn't hate if we did something like:

	.r11 = hcall_func(EXIT_REASON_MSR_READ),

That retains greppability, but also tells you that r11 is a function
number.  It even gives a nice place to stick a comment to say what the
heck is going on:

/*
 * The TDG.VP.VMCALL-Instruction-execution sub-functions are defined
 * independently from but are currently matched 1:1 with VMX
 * EXIT_REASONs.  Reusing the KVM EXIT_REASON macros makes it easier to
 * connect the host and guest sides of these calls.
 */
static u64 hcall_func(u64 exit_reason)
{
	return exit_reason;
}

Like I said, this is all a minor note.  But, things like this really go
a long way for folks like me who don't spend our days looking at the KVM
code or thinking deeply about how the hypercall is implemented.