Re: [PATCH v6 08/42] x86/sev-es: initialize sev_status/features within #VC handler

From: Borislav Petkov <bp@alien8.de>
To: Michael Roth <michael.roth@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>,
	x86@kernel.org, linux-kernel@vger.kernel.org,
	kvm@vger.kernel.org, linux-efi@vger.kernel.org,
	platform-driver-x86@vger.kernel.org, linux-coco@lists.linux.dev,
	linux-mm@kvack.org, Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Joerg Roedel <jroedel@suse.de>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Ard Biesheuvel <ardb@kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <seanjc@google.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Jim Mattson <jmattson@google.com>,
	Andy Lutomirski <luto@kernel.org>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Sergio Lopez <slp@redhat.com>, Peter Gonda <pgonda@google.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
	David Rientjes <rientjes@google.com>,
	Dov Murik <dovmurik@linux.ibm.com>,
	Tobin Feldman-Fitzthum <tobin@ibm.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	"Kirill A . Shutemov" <kirill@shutemov.name>,
	Andi Kleen <ak@linux.intel.com>,
	"Dr . David Alan Gilbert" <dgilbert@redhat.com>,
	tony.luck@intel.com, marcorr@google.com,
	sathyanarayanan.kuppuswamy@linux.intel.com
Subject: Re: [PATCH v6 08/42] x86/sev-es: initialize sev_status/features within #VC handler
Date: Wed, 20 Oct 2021 20:01:07 +0200	[thread overview]
Message-ID: <YXBZYws8NnxiQJD7@zn.tnic> (raw)
In-Reply-To: <20211020161023.hzbj53ehmzjrt4xd@amd.com>

On Wed, Oct 20, 2021 at 11:10:23AM -0500, Michael Roth wrote:
> [Sorry for the wall of text, just trying to work through everything.]

And I'm going to respond in a couple of mails just for my own sanity.

> I'm not sure if this is pertaining to using the CPUID table prior to
> sme_enable(), or just the #VC-based SEV MSR read. The following comments
> assume the former. If that assumption is wrong you can basically ignore
> the rest of this email :)

This is pertaining to me wanting to show you that the design of this SNP
support needs to be sane and maintainable and every function needs to
make sense not only now but in the future.

In this particular example, we should set sev_status *once*, *before*
anything accesses it so that it is prepared when something needs it. Not
do a #VC and go, "oh, btw, is sev_status set? No? Ok, lemme set it."
which basically means our design is seriously lacking.

And I had suggested a similar thing for TDX and tglx was 100% right in
shooting it down because we do properly designed things - not, get stuff
in so that vendor is happy and then, once the vendor programmers have
disappeared to do their next enablement task, the maintainers get to mop
up and maintain it forever.

Because this mopping up doesn't scale - trust me.

> [The #VC-based SEV MSR read is not necessary for anything in sme_enable(),
> it's simply a way to determine whether the guest is an SNP guest, without
> any reliance on CPUID, which seemed useful in the context of doing some
> additional sanity checks against the SNP CPUID table and determining that
> it's appropriate to use it early on (rather than just trust that this is an
> SNP guest by virtue of the CC blob being present, and then failing later
> once sme_enable() checks for the SNP feature bits through the normal
> mechanism, as was done in v5).]

So you need to make up your mind here design-wise, what you wanna do.

The proper thing to do would be, to detect *everything*, detect whether
this is an SNP guest, yadda yadda, everything your code is going to need
later on, and then be done with it.

Then you continue with the boot and now your other code queries
everything that has been detected up til now and uses it.

End of mail 1.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette