Re: [PATCH v2] x86: Skip WBINVD instruction for VM guest

From: Dave Hansen <dave.hansen@intel.com>
To: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Kuppuswamy Sathyanarayanan 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	x86@kernel.org, "Rafael J . Wysocki" <rjw@rjwysocki.net>,
	"H . Peter Anvin" <hpa@zytor.com>,
	Tony Luck <tony.luck@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Andi Kleen <ak@linux.intel.com>,
	Kuppuswamy Sathyanarayanan <knsathya@kernel.org>,
	linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org
Subject: Re: [PATCH v2] x86: Skip WBINVD instruction for VM guest
Date: Mon, 6 Dec 2021 07:35:18 -0800	[thread overview]
Message-ID: <5bc40889-445d-5cac-3396-d39d53ee92c7@intel.com> (raw)
In-Reply-To: <20211204005427.ccinxlwwab3jsuct@black.fi.intel.com>

On 12/3/21 4:54 PM, Kirill A. Shutemov wrote:
> On Fri, Dec 03, 2021 at 04:20:34PM -0800, Dave Hansen wrote:
>>> TDX doesn't support these S- and C-states. TDX is only supports S0 and S5.
>>
>> This makes me a bit nervous.  Is this "the first TDX implementation
>> supports..." or "the TDX architecture *prohibits* supporting S1 (or
>> whatever"?
> 
> TDX Virtual Firmware Design Guide only states that "ACPI S3 (not supported
> by TDX guests)".
> 
> Kernel reports in dmesg "ACPI: PM: (supports S0 S5)".

Those describe the current firmware implementation, not a guarantee
provided by the TDX architecture forever.

> But I don't see how any state beyond S0 and S5 make sense in TDX context.
> Do you?

Do existing (non-TDX) VMs use anything other than S0 and S5?  If so, I'd
say yes.

>> I really think we need some kind of architecture guarantee.  Without
>> that, we risk breaking things if someone at our employer simply changes
>> their mind.
> 
> Guarantees are hard.
> 
> If somebody change their mind we will get unexpected #VE and crash.
> I think it is acceptable way to handle unexpected change in confidential
> computing environment.

Architectural guarantees are quite easy, actually.  They're just a
contract that two parties agree to.  In this case, the contract would be
that TDX firmware *PROMISES* not to enumerate support for additional
sleep states over what the implementation does today.  If future
firmware breaks that promise (and the kernel crashes) we get to come
after them with torches and pitchforks to fix the firmware.

The contract let's us do things in the OS like:

	WARN_ON(sleep_states[ACPI_STATE_S3]);

We also don't need *formal* documentation of such things.  We really
just need to have a chat.

It would be perfectly sufficient if we go bug Intel's TDX architecture
folks and say, "Hey, Linux is going to crash if you ever implement any
actual sleep states.  The current implementation is fine here, but is it
OK if future implementations are restricted from doing this?"

But, the trick is that we need a contract.  A contract requires a
"meeting of the minds" first.