linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dave Hansen <dave.hansen@intel.com>
To: Kuppuswamy Sathyanarayanan 
	<sathyanarayanan.kuppuswamy@linux.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 11:35:48 -0700	[thread overview]
Message-ID: <24d0fe72-78b4-6550-e5d8-dd511dcbfef3@intel.com> (raw)
In-Reply-To: <20210903172812.1097643-12-sathyanarayanan.kuppuswamy@linux.intel.com>

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 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.

--

> 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?

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.

> 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.

> 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.

> +	return ret;
> +}
> +
>  unsigned long tdx_get_ve_info(struct ve_info *ve)
>  {
>  	struct tdx_module_output out = {0};
> @@ -193,6 +208,9 @@ int tdx_handle_virtualization_exception(struct pt_regs *regs,
>  	case EXIT_REASON_MSR_WRITE:
>  		ret = tdx_write_msr_safe(regs->cx, regs->ax, regs->dx);
>  		break;
> +	case EXIT_REASON_CPUID:
> +		ret = tdx_handle_cpuid(regs);
> +		break;
>  	default:
>  		pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
>  		return -EFAULT;
> 


  reply	other threads:[~2021-09-03 18:35 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 [this message]
2021-09-03 19:14     ` Kuppuswamy, Sathyanarayanan
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=24d0fe72-78b4-6550-e5d8-dd511dcbfef3@intel.com \
    --to=dave.hansen@intel.com \
    --cc=ak@linux.intel.com \
    --cc=bp@alien8.de \
    --cc=dan.j.williams@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=sathyanarayanan.kuppuswamy@linux.intel.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).