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

[Kalra, Ashish]

Hello Sean,

On 1/14/2025 4:31 PM, Sean Christopherson wrote:
> 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.

I agree.

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

Currently SNP support on IOMMU is enabled via the following code path:

rootfs_initcall(pci_iommu_init) -> pci_iommu_init() -> amd_iommu_init() -> iommu_snp_enable()

And, smp_rmptable_init() needs to be executed after iommu_snp_enable() and that's why we can't 
call snp_rmptable_init() as early as mem_encrypt_init() or post arch ACPI callbacks, etc.

But, there is a patch from the AMD IOMMU team, which calls iommu_snp_enable() early after
early_amd_iommu_init() is executed and this will happen during AMD IOMMU driver initialization
with the following code path:

apic_intr_mode_init() -> enable_IR_x2apic() -> irq_remapping_enable() -> amd_iommu_enable() -> iommu_snp_enable()

This AMD IOMMU driver patch moves SNP enable check before enabling IOMMUs as certain IOMMU buffer
sizes may change depending on SNP support being enabled. 

With this AMD IOMMU driver patch applied, we can now call snp_rmptable_init() early with a subsys_initcall(). 

That fixes the issue with SNP host enabling code being called later than KVM initialization
with kvm_amd module built-in.

I will post a fix for the SNP host support broken with kvm_amd module built-in with this AMD IOMMU driver
patch to call iommu_snp_enable() early and the subsys_initcall() change for snp_rmptable_init() fix
on top of it. 

> 
>>> 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
>   *

I have tested your patch for KVM explicitly calling into the PSP driver and this works well, with this
patch applied as expected PSP driver initialization completes before KVM initialization.

So we can continue with the approach to move SEV/SNP initialization stuff to KVM.
I will add your patch to the v4 of these series i am going to post next.

Thanks,
Ashish