linux-kernel - Re: [PATCH v3 6/7] KVM: SVM: Add support to initialize SEV/SNP functionality in KVM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z4bl0D4CbtHgwGGW@google.com>
Date: Tue, 14 Jan 2025 14:31:44 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Ashish Kalra <ashish.kalra@....com>
Cc: Tom Lendacky <thomas.lendacky@....com>, pbonzini@...hat.com, tglx@...utronix.de, 
	mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com, x86@...nel.org, 
	hpa@...or.com, john.allen@....com, herbert@...dor.apana.org.au, 
	davem@...emloft.net, michael.roth@....com, dionnaglaze@...gle.com, 
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-crypto@...r.kernel.org, linux-coco@...ts.linux.dev
Subject: Re: [PATCH v3 6/7] KVM: SVM: Add support to initialize SEV/SNP
 functionality in KVM

On Tue, Jan 14, 2025, Ashish Kalra wrote:
> On 1/13/2025 9:03 AM, Kalra, Ashish wrote:
> > SNP host support is enabled in snp_rmptable_init() in
> > arch/x86/virt/svm/sev.c, which is invoked as a device_initcall().  Here
> > device_initcall() is used as snp_rmptable_init() expects AMD IOMMU SNP
> > support to be enabled prior to it and the AMD IOMMU driver is initialized
> > after PCI bus enumeration. 

Ugh.  So. Many. Dependencies.

That's a kernel bug, full stop.  RMP initialization very obviously is not device
initialization.

Why isn't snp_rmptable_init() called from mem_encrypt_init()?  AFAICT,
arch_cpu_finalize_init() is called after IOMMU initialziation.  And if that
doesn't work, hack it into arch_post_acpi_subsys_init().  Using device_initcall()
to initialization the RMP is insane, IMO.

> > Additionally, the PSP driver probably needs to be initialized at
> > device_initcall level if it is built-in, but that is much later than KVM
> > module initialization, therefore, that is blocker for moving SEV/SNP
> > initialization to KVM module load time instead of PSP module probe time.
> > Do note that i have verified and tested that PSP module initialization
> > works when invoked as a device_initcall(). 
> 
> As a follow-up to the above issues, i have an important question: 
> 
> Do we really need kvm_amd module to be built-in for SEV/SNP support ?

Yes.

> Is there any usage case/scenario where the kvm_amd module needs to be
> built-in for SEV/SNP support ?

Don't care.  I am 100% against setting a precedent of tying features to KVM
being a module or not, especially since this is a solvable problem.

Ideally, the initcall infrastructure would let modules express dependencies, but
I can appreciate that solving this generically would require a high amount of
complexity.

Having KVM explicitly call into the PSP driver as needed isn't difficult, just
gross.  But for me, it's still far better giving up and requiring everything to
be modules.

E.g.

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 943bd074a5d3..a2ee12e998f0 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2972,6 +2972,16 @@ void __init sev_hardware_setup(void)
            WARN_ON_ONCE(!boot_cpu_has(X86_FEATURE_FLUSHBYASID)))
                goto out;
 
+       /*
+        * The kernel's initcall infrastructure lacks the ability to express
+        * dependencies between initcalls, where as the modules infrastructure
+        * automatically handles dependencies via symbol loading.  Ensure the
+        * PSP SEV driver is initialized before proceeding if KVM is built-in,
+        * as the dependency isn't handled by the initcall infrastructure.
+        */
+       if (IS_BUILTIN(CONFIG_KVM_AMD) && sev_module_init())
+               goto out;
+
        /* Retrieve SEV CPUID information */
        cpuid(0x8000001f, &eax, &ebx, &ecx, &edx);
 
diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c
index 7eb3e4668286..a0cdc03984cb 100644
--- a/drivers/crypto/ccp/sp-dev.c
+++ b/drivers/crypto/ccp/sp-dev.c
@@ -253,8 +253,12 @@ struct sp_device *sp_get_psp_master_device(void)
 static int __init sp_mod_init(void)
 {
 #ifdef CONFIG_X86
+       static bool initialized;
        int ret;
 
+       if (initialized)
+               return 0;
+
        ret = sp_pci_init();
        if (ret)
                return ret;
@@ -263,6 +267,7 @@ static int __init sp_mod_init(void)
        psp_pci_init();
 #endif
 
+       initialized = true;
        return 0;
 #endif
 
@@ -279,6 +284,13 @@ static int __init sp_mod_init(void)
        return -ENODEV;
 }
 
+#if IS_BUILTIN(CONFIG_KVM_AMD) && IS_ENABLED(CONFIG_KVM_AMD_SEV)
+int __init sev_module_init(void)
+{
+       return sp_mod_init();
+}
+#endif
+
 static void __exit sp_mod_exit(void)
 {
 #ifdef CONFIG_X86
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 903ddfea8585..0138d22b46ac 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -814,6 +814,8 @@ struct sev_data_snp_commit {
 
 #ifdef CONFIG_CRYPTO_DEV_SP_PSP
 
+int __init sev_module_init(void);
+
 /**
  * sev_platform_init - perform SEV INIT command
  *