Re: [PATCH v3 0/5] efi: Don't initalize SEV-SNP from the EFI stub

[Tom Lendacky <thomas.lendacky@amd.com>]

On 4/24/25 09:18, Tom Lendacky wrote:
> On 4/24/25 02:22, Ard Biesheuvel wrote:
>> On Tue, 22 Apr 2025 at 18:40, Ard Biesheuvel <ardb@google.com> wrote:
>>>
>>> On Tue, Apr 22, 2025 at 5:51 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>>>
>>>> On 4/22/25 05:07, Ard Biesheuvel wrote:
>>>>> From: Ard Biesheuvel <ardb@kernel.org>
>>>>>
>>>>
>>>> Hi Ard,
>>>>
>>>> I'll try to get to reviewing and testing this series very soon.
>>>
>>> Thanks.
>>>
>>>> But one
>>>> thing I can see is that we never set the snp_vmpl level anymore in the
>>>> EFI stub and so PVALIDATE will fail when running under an SVSM.
>>>>
>>>> But I don't think this series is completely at fault. This goes back to
>>>> fixing memory acceptance before sev_enable() was called and I missed the
>>>> SVSM situation. So I don't think we can completely remove all SNP
>>>> initialization and might have to do svsm_setup_ca() which has a pre-req
>>>> on setup_cpuid_table()...  sigh.
>>>>
>>
>> Why is that, though? The EFI stub never replaces the #VC and #PF
>> handlers, and so cpuid instructions will be handled as before, right?
>> And the SVSM setup code will run again when the core kernel boots and
>> this time, it will need to update the cpuid tables to record the SVSM
>> presence.
> 
> It's more of a statement about the CPUID table modifications made by
> svsm_setup_ca() that need to be skipped if setup_cpuid_table() isn't
> called, not the use of CPUID itself.
> 
> But taking a closer look, snp_cpuid_get_table() is actually returning
> the address of cpuid_table_copy, which is a static in the file. So maybe
> it isn't an issue because the loop at the end of svsm_setup_ca() will
> not crash, which was the main concern.
> 
> I think we can use CPUID 0x8000001f_EAX[28] to detect an SVSM and read
> MSR 0xc001f000 to get the CAA. OVMF has that support, just would need to
> figure out where to check for it, then we can probably skip the
> svsm_setup_ca() and do everything in the snp_accept_memory() path.
> 
> Let me take a look...

Initial look at something like this works (along with the fix for the
mistake I made in OVMF). I need to test the kexec path to be certain,
though.

diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index 478c65149cf0..d2f9cbbe943b 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -142,6 +142,7 @@ u64 svsm_get_caa_pa(void)
 int svsm_perform_call_protocol(struct svsm_call *call);
 
 u8 snp_vmpl;
+bool snp_vmpl_checked;
 
 /* Include code for early handlers */
 #include "../../boot/startup/sev-shared.c"
@@ -241,6 +242,29 @@ static bool early_setup_ghcb(void)
 
 void snp_accept_memory(phys_addr_t start, phys_addr_t end)
 {
+	if (!snp_vmpl_checked) {
+		unsigned int eax, ebx, ecx, edx;
+
+		/*
+		 * CPUID Fn8000_001F_EAX[28] - SVSM support
+		 */
+		eax = 0x8000001f;
+		ecx = 0;
+		native_cpuid(&eax, &ebx, &ecx, &edx);
+		if (eax & BIT(28)) {
+			struct msr m;
+
+			/* Obtain the address of the calling area to use */
+			boot_rdmsr(MSR_SVSM_CAA, &m);
+			boot_svsm_caa = (void *)m.q;
+			boot_svsm_caa_pa = m.q;
+
+			snp_vmpl = 2;
+		}
+
+		snp_vmpl_checked = true;
+	}
+
 	for (phys_addr_t pa = start; pa < end; pa += PAGE_SIZE)
 		__page_state_change(pa, SNP_PAGE_STATE_PRIVATE);
 }
diff --git a/arch/x86/boot/startup/sev-shared.c b/arch/x86/boot/startup/sev-shared.c
index 173f3d1f777a..5cca01700280 100644
--- a/arch/x86/boot/startup/sev-shared.c
+++ b/arch/x86/boot/startup/sev-shared.c
@@ -1342,6 +1342,8 @@ static bool __head svsm_setup_ca(const struct cc_blob_sev_info *cc_info)
 
 	BUILD_BUG_ON(sizeof(*secrets_page) != PAGE_SIZE);
 
+	snp_vmpl_checked = true;
+
 	/*
 	 * Check if running at VMPL0.
 	 *
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 5b145446e991..5011b3a93a21 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -99,6 +99,7 @@ DEFINE_PER_CPU(struct sev_es_save_area *, sev_vmsa);
  */
 u8 snp_vmpl __ro_after_init;
 EXPORT_SYMBOL_GPL(snp_vmpl);
+bool snp_vmpl_checked __ro_after_init;
 
 static u64 __init get_snp_jump_table_addr(void)
 {

> 
> Thanks,
> Tom