From: "Kuppuswamy, Sathyanarayanan" <sathyanarayanan.kuppuswamy@linux.intel.com>
To: Dave Hansen <dave.hansen@intel.com>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
x86@kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
Juergen Gross <jgross@suse.com>, Deep Shah <sdeep@vmware.com>,
VMware Inc <pv-drivers@vmware.com>,
Vitaly Kuznetsov <vkuznets@redhat.com>,
Wanpeng Li <wanpengli@tencent.com>,
Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>
Cc: Peter H Anvin <hpa@zytor.com>, Tony Luck <tony.luck@intel.com>,
Dan Williams <dan.j.williams@intel.com>,
Andi Kleen <ak@linux.intel.com>,
Kirill Shutemov <kirill.shutemov@linux.intel.com>,
Sean Christopherson <seanjc@google.com>,
Kuppuswamy Sathyanarayanan <knsathya@kernel.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 11/11] x86/tdx: Handle CPUID via #VE
Date: Fri, 3 Sep 2021 12:14:48 -0700 [thread overview]
Message-ID: <26e79e8f-ba96-9087-04dd-283eadd8c693@linux.intel.com> (raw)
In-Reply-To: <24d0fe72-78b4-6550-e5d8-dd511dcbfef3@intel.com>
On 9/3/21 11:35 AM, Dave Hansen wrote:
> On 9/3/21 10:28 AM, Kuppuswamy Sathyanarayanan wrote:
>> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>>
>> TDX has three classes of CPUID leaves: some CPUID leaves are always
>> handled by the CPU, others are handled by the TDX module, and some
>> others are handled by the VMM. Since the VMM cannot directly intercept
>> the instruction these are reflected with a #VE exception to the guest,
>> which then converts it into a hypercall to the VMM, or handled
>> directly.
>
> Does this patch do any of the "handled directly" leaves? If not, why
> mention it?
It was added to give more information about CPUID leaves handling. Since
it has nothing to do with this patch, I can remove it.
>
> It would also be nice to mention that this applies to both kernel and
> userspace use of CPUID. It talks a bit about early kernel use, which
> makes it seem like this is kernel-only.
>
> I also think it's a mistake to talk about TDX-module handling. For
> *this* patch, it doesn't matter.
>
> Here's a reformatted replacement changelog:
>
> --
>
> When running virtualized, the CPUID instruction is handled differently
> based on the leaf being accessed. The behavior depends only on on the
> leaf and applies equally to both kernel/ring-0 and userspace/ring-3
> execution of CPUID. Historically, there are two basic classes:
>
> * Leaves handled transparently to the guest
> * Leaves handled by the VMM
>
> In a typical guest without TDX, "handled by the VMM" leaves cause a
> VMEXIT. TDX replaces these VMEXITs with a #VE exception in the guest.
> The guest typically handles the #VE by making a hypercall to the VMM.
>
> The TDX module spec talks about a few more classes of CPUID handling.
> But, for the purposes of this patch, the "handled transparently" CPUID
> leaves are all lumped together because the guest handling is the same.
>
> --
Thanks. I will use above commit log in next version.
>
>> The TDX module specification [1], sec 16.2 has a full list of CPUID
>
> ^ I think we can spare the extra four bytes to make "sec" ->
> "section".
>
> I also opened up the pdf from [1] an searched for "16.2". I found:
>
> 16.2. Branch Prediction Side Channel Attacks Mitigation
> Mechanisms
>
> There is, however, a:
>
> 18.2. CPUID Virtualization
>
> section. Did you, perhaps, mean to reference that instead?
It looks like I have been using previous version of the TDX module spec
(Sep 2020). In the newer version, it is changed to 18.2.
To avoid confusion I will use the section title for reference.
>
> Which kinda argues for not using these section numbers at *all*.
> Perhaps you should just mention the section titles, as they're obviously
> less volatile. That's probably a comment that applies to *ALL* of your
> changelogs across *ALL* TDX patches. This just proves that the section
> numbers are worthless.
Makes sense. I will fix it in all TDX patch series.
>
>> leaves which are handled natively or by the TDX module. Only unknown
>
> Just in terms of nice writing, it would be great to use the same
> language when you refer to the same concept. Earlier you called this
> "handled by the CPU". But, here you refer to it as being "handled
> natively". Neither is wrong, but this puts a burden on the reader to
> make a connection rather than doing it for them as the writer.
Ok. I will keep this in mind for future submissions. For this patch
your commit log refactor fixes this issue.
>
>> CPUIDs are handled by the #VE method. In practice this typically only
>> applies to the hypervisor-specific CPUIDs unknown to the native CPU.
>>
>> Therefore there is no risk of causing this in early CPUID code which
>> runs before the #VE handler is set up because it will never access
>> those exotic CPUID leaves.
>
> This never actually makes a transition from background to telling what
> the patch does. I think this needs at least this:
>
> Allow the the #VE code to handle any CPUID leaves which cause a
> #VE. Unconditionally make a TDCALL to the VMM.
>
>> diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
>> index 5c52dde4a5fd..c65c117aff5f 100644
>> --- a/arch/x86/kernel/tdx.c
>> +++ b/arch/x86/kernel/tdx.c
>> @@ -150,6 +150,21 @@ static int tdx_write_msr_safe(unsigned int msr, unsigned int low,
>> return ret ? -EIO : 0;
>> }
>>
>> +static u64 tdx_handle_cpuid(struct pt_regs *regs)
>> +{
>> + struct tdx_hypercall_output out = {0};
>> + u64 ret;
>> +
>> + ret = _tdx_hypercall(EXIT_REASON_CPUID, regs->ax, regs->cx, 0, 0, &out);
>> +
>> + regs->ax = out.r12;
>> + regs->bx = out.r13;
>> + regs->cx = out.r14;
>> + regs->dx = out.r15;
>
> This probably needs a comment about why this is shuffling registers
> around like this.
I will add the ABI details here and also spell out what we are getting
in R12-R15 registers.
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
next prev parent reply other threads:[~2021-09-03 19:14 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-03 17:28 [PATCH v6 00/11] Add TDX Guest Support (Initial support) Kuppuswamy Sathyanarayanan
2021-09-03 17:28 ` [PATCH v6 01/11] x86/paravirt: Move halt paravirt calls under CONFIG_PARAVIRT Kuppuswamy Sathyanarayanan
2021-09-03 18:22 ` Borislav Petkov
2021-09-03 19:03 ` Kuppuswamy, Sathyanarayanan
2021-09-03 20:33 ` Borislav Petkov
2021-09-03 17:28 ` [PATCH v6 02/11] x86/tdx: Introduce INTEL_TDX_GUEST config option Kuppuswamy Sathyanarayanan
2021-09-28 11:37 ` Joerg Roedel
2021-09-28 12:52 ` Kuppuswamy, Sathyanarayanan
2021-09-03 17:28 ` [PATCH v6 03/11] x86/cpufeatures: Add TDX Guest CPU feature Kuppuswamy Sathyanarayanan
2021-09-28 11:39 ` Joerg Roedel
2021-09-28 12:53 ` Kuppuswamy, Sathyanarayanan
2021-09-03 17:28 ` [PATCH v6 04/11] x86/tdx: Add protected guest support for TDX guest Kuppuswamy Sathyanarayanan
2021-09-28 11:46 ` Joerg Roedel
2021-09-28 12:56 ` Kuppuswamy, Sathyanarayanan
2021-09-28 13:04 ` Joerg Roedel
2021-09-28 13:18 ` Borislav Petkov
2021-09-03 17:28 ` [PATCH v6 05/11] x86/tdx: Add __tdx_module_call() and __tdx_hypercall() helper functions Kuppuswamy Sathyanarayanan
2021-09-03 17:28 ` [PATCH v6 06/11] x86/traps: Add #VE support for TDX guest Kuppuswamy Sathyanarayanan
2021-09-28 12:16 ` Joerg Roedel
2021-09-28 14:05 ` Dave Hansen
2021-09-28 15:22 ` Joerg Roedel
2021-09-28 15:25 ` Dave Hansen
2021-09-28 15:23 ` Dave Hansen
2021-09-28 16:59 ` Kuppuswamy, Sathyanarayanan
2021-09-03 17:28 ` [PATCH v6 07/11] x86/tdx: Add HLT " Kuppuswamy Sathyanarayanan
2021-09-03 17:28 ` [PATCH v6 08/11] x86/tdx: Wire up KVM hypercalls Kuppuswamy Sathyanarayanan
2021-09-03 17:28 ` [PATCH v6 09/11] x86/tdx: Add MSR support for TDX guest Kuppuswamy Sathyanarayanan
2021-09-03 17:28 ` [PATCH v6 10/11] x86/tdx: Don't write CSTAR MSR on Intel Kuppuswamy Sathyanarayanan
2021-09-03 17:28 ` [PATCH v6 11/11] x86/tdx: Handle CPUID via #VE Kuppuswamy Sathyanarayanan
2021-09-03 18:35 ` Dave Hansen
2021-09-03 19:14 ` Kuppuswamy, Sathyanarayanan [this message]
2021-09-03 23:43 ` Sean Christopherson
2021-09-03 23:54 ` Andi Kleen
2021-09-04 0:00 ` Dave Hansen
2021-09-04 0:05 ` Andi Kleen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=26e79e8f-ba96-9087-04dd-283eadd8c693@linux.intel.com \
--to=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=ak@linux.intel.com \
--cc=bp@alien8.de \
--cc=dan.j.williams@intel.com \
--cc=dave.hansen@intel.com \
--cc=hpa@zytor.com \
--cc=jgross@suse.com \
--cc=jmattson@google.com \
--cc=joro@8bytes.org \
--cc=kirill.shutemov@linux.intel.com \
--cc=knsathya@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=pbonzini@redhat.com \
--cc=pv-drivers@vmware.com \
--cc=sdeep@vmware.com \
--cc=seanjc@google.com \
--cc=tglx@linutronix.de \
--cc=tony.luck@intel.com \
--cc=vkuznets@redhat.com \
--cc=wanpengli@tencent.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).