LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Borislav Petkov <bp@alien8.de>
Cc: Kuppuswamy Sathyanarayanan 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Andy Lutomirski <luto@kernel.org>, Peter H Anvin <hpa@zytor.com>,
	Dave Hansen <dave.hansen@intel.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>,
	Kuppuswamy Sathyanarayanan <knsathya@kernel.org>,
	x86@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 08/12] x86/tdx: Add HLT support for TDX guest
Date: Tue, 24 Aug 2021 17:06:21 +0000	[thread overview]
Message-ID: <YSUnDQUrGYc8aY9j@google.com> (raw)
In-Reply-To: <YSUaAQPiBUqubBHM@zn.tnic>

On Tue, Aug 24, 2021, Borislav Petkov wrote:
> On Wed, Aug 04, 2021 at 11:13:25AM -0700, Kuppuswamy Sathyanarayanan wrote:
> > +static __cpuidle void tdg_safe_halt(void)
> > +{
> > +	u64 ret;
> > +
> > +	/*
> > +	 * Enable interrupts next to the TDVMCALL to avoid
> > +	 * performance degradation.
> 
> That comment needs some more love to say exactly what the problem is.

LOL, I guess hanging the vCPU counts as degraded performance.  But this comment
can and should go away entirely...

> > +	 */
> > +	local_irq_enable();

...because this is broken.  It's also disturbing because it suggests that these
patches are not being tested.

The STI _must_ immediately precede TDCALL, and it _must_ execute with interrupts
disabled.  The whole point of the STI blocking shadow is to ensure interrupts are
blocked until _after_ the HLT completes so that a wake event is not recongized
before the HLT, in which case the vCPU will get stuck in HLT because its wake
event alreadyfired.  Enabling IRQs well before the TDCALL defeats the purpose of
the STI dance in __tdx_hypercall().

There's even a massive comment in __tdx_hypercall() explaining all this...

> > +
> > +	/* IRQ is enabled, So set R12 as 0 */

It would be helpful to use local variables to document what's up, e.g.

 	const bool irqs_enabled = true;
	const bool do_sti = true;

	ret = _tdx_hypercall(EXIT_REASON_HLT, irqs_enabled0, 0, 0, do_sti, NULL);
	
> > +	ret = _tdx_hypercall(EXIT_REASON_HLT, 0, 0, 0, 1, NULL);
> > +
> > +	/* It should never fail */
> > +	BUG_ON(ret);
> > +}
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

  reply	other threads:[~2021-08-24 17:48 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-04 18:13 [PATCH v5 00/12] Add TDX Guest Support (Initial support) Kuppuswamy Sathyanarayanan
2021-08-04 18:13 ` [PATCH v5 01/12] x86/paravirt: Move halt paravirt calls under CONFIG_PARAVIRT Kuppuswamy Sathyanarayanan
2021-08-12  7:18   ` Borislav Petkov
2021-08-12 17:17     ` Kuppuswamy, Sathyanarayanan
2021-08-17 12:50     ` Juergen Gross
2021-08-17 13:16       ` Kuppuswamy, Sathyanarayanan
2021-08-17 13:28         ` Juergen Gross
2021-08-17 13:39           ` Kuppuswamy, Sathyanarayanan
2021-08-17 13:47             ` Juergen Gross
2021-08-17 13:50               ` Kuppuswamy, Sathyanarayanan
2021-08-04 18:13 ` [PATCH v5 02/12] x86/tdx: Introduce INTEL_TDX_GUEST config option Kuppuswamy Sathyanarayanan
2021-08-04 18:13 ` [PATCH v5 03/12] x86/cpufeatures: Add TDX Guest CPU feature Kuppuswamy Sathyanarayanan
2021-08-12  7:39   ` Borislav Petkov
2021-08-04 18:13 ` [PATCH v5 04/12] x86/tdx: Add protected guest support for TDX guest Kuppuswamy Sathyanarayanan
2021-08-04 21:59   ` Sean Christopherson
2021-08-04 22:03     ` Dave Hansen
2021-08-04 22:26       ` Kuppuswamy, Sathyanarayanan
2021-08-04 22:42         ` Sean Christopherson
2021-08-04 23:00           ` Kuppuswamy, Sathyanarayanan
2021-08-12  7:53             ` Borislav Petkov
2021-08-12 17:18               ` Kuppuswamy, Sathyanarayanan
2021-08-20 14:28                 ` Borislav Petkov
2021-08-20 16:42                   ` Kuppuswamy, Sathyanarayanan
2021-08-20 16:59                     ` Borislav Petkov
2021-08-20 17:11                       ` Kuppuswamy, Sathyanarayanan
2021-08-04 18:13 ` [PATCH v5 05/12] x86/tdx: Add __tdx_module_call() and __tdx_hypercall() helper functions Kuppuswamy Sathyanarayanan
2021-08-20 15:16   ` Borislav Petkov
2021-08-04 18:13 ` [PATCH v5 06/12] x86/tdx: Get TD execution environment information via TDINFO Kuppuswamy Sathyanarayanan
2021-08-04 22:38   ` Sean Christopherson
2021-08-20 17:13   ` Borislav Petkov
2021-08-20 17:31     ` Kuppuswamy, Sathyanarayanan
2021-08-20 17:35       ` Borislav Petkov
2021-08-20 18:29         ` Kuppuswamy, Sathyanarayanan
2021-08-20 18:58           ` Andi Kleen
2021-08-20 19:01             ` Kuppuswamy, Sathyanarayanan
2021-08-04 18:13 ` [PATCH v5 07/12] x86/traps: Add #VE support for TDX guest Kuppuswamy Sathyanarayanan
2021-08-24 10:17   ` Borislav Petkov
2021-08-24 17:32     ` Kuppuswamy, Sathyanarayanan
2021-08-24 17:36       ` Dave Hansen
2021-08-24 17:46       ` Borislav Petkov
2021-09-02 15:24         ` Kuppuswamy, Sathyanarayanan
2021-09-03 10:17           ` Borislav Petkov
2021-08-04 18:13 ` [PATCH v5 08/12] x86/tdx: Add HLT " Kuppuswamy Sathyanarayanan
2021-08-24 16:10   ` Borislav Petkov
2021-08-24 17:06     ` Sean Christopherson [this message]
2021-08-24 17:25       ` Andi Kleen
2021-08-24 17:27       ` Borislav Petkov
2021-08-24 17:47         ` Sean Christopherson
2021-08-24 17:50           ` Borislav Petkov
2021-08-31 20:49         ` Kuppuswamy, Sathyanarayanan
2021-09-01  7:42           ` Borislav Petkov
2021-08-24 18:18       ` Kuppuswamy, Sathyanarayanan
2021-08-24 18:28         ` Andi Kleen
2021-08-24 17:35     ` Kuppuswamy, Sathyanarayanan
2021-08-24 17:48       ` Borislav Petkov
2021-08-04 18:13 ` [PATCH v5 09/12] x86/tdx: Wire up KVM hypercalls Kuppuswamy Sathyanarayanan
2021-08-24 16:34   ` Borislav Petkov
2021-08-24 18:11     ` Kuppuswamy, Sathyanarayanan
2021-08-24 18:29       ` Borislav Petkov
2021-08-24 19:11         ` Kuppuswamy, Sathyanarayanan
2021-08-24 19:39           ` Borislav Petkov
2021-08-04 18:13 ` [PATCH v5 10/12] x86/tdx: Add MSR support for TDX guest Kuppuswamy Sathyanarayanan
2021-08-24 16:55   ` Borislav Petkov
2021-08-24 18:12     ` Kuppuswamy, Sathyanarayanan
2021-08-04 18:13 ` [PATCH v5 11/12] x86/tdx: Don't write CSTAR MSR on Intel Kuppuswamy Sathyanarayanan
2021-08-04 18:31   ` Sean Christopherson
2021-08-04 21:03     ` Kuppuswamy, Sathyanarayanan
2021-08-04 21:44       ` Sean Christopherson
2021-08-04 21:48       ` Dave Hansen
2021-08-04 22:23         ` Kuppuswamy, Sathyanarayanan
2021-08-04 18:13 ` [PATCH v5 12/12] x86/tdx: Handle CPUID via #VE Kuppuswamy Sathyanarayanan
2021-08-24 17:48   ` Borislav Petkov

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=YSUnDQUrGYc8aY9j@google.com \
    --to=seanjc@google.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=kirill.shutemov@linux.intel.com \
    --cc=knsathya@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=sathyanarayanan.kuppuswamy@linux.intel.com \
    --cc=tglx@linutronix.de \
    --cc=tony.luck@intel.com \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH v5 08/12] x86/tdx: Add HLT support for TDX guest' \
    /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

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