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

["Kalra, Ashish" <ashish.kalra@amd.com>]

Hello All,

On 1/8/2025 6:27 PM, Kalra, Ashish wrote:
> 
> 
> On 1/8/2025 11:22 AM, Tom Lendacky wrote:
>> On 1/7/25 12:34, Kalra, Ashish wrote:
>>> On 1/7/2025 10:42 AM, Tom Lendacky wrote:
>>>> On 1/3/25 14:01, Ashish Kalra wrote:
>>>>> From: Ashish Kalra <ashish.kalra@amd.com>
>>>>>
>>>>> Remove platform initialization of SEV/SNP from PSP driver probe time and
>>>>
>>>> Actually, you're not removing it, yet...
>>>>
>>>>> move it to KVM module load time so that KVM can do SEV/SNP platform
>>>>> initialization explicitly if it actually wants to use SEV/SNP
>>>>> functionality.
>>>>>
>>>>> With this patch, KVM will explicitly call into the PSP driver at load time
>>>>> to initialize SEV/SNP by default but this behavior can be altered with KVM
>>>>> module parameters to not do SEV/SNP platform initialization at module load
>>>>> time if required. Additionally SEV/SNP platform shutdown is invoked during
>>>>> KVM module unload time.
>>>>>
>>>>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>>>>> ---
>>>>>  arch/x86/kvm/svm/sev.c | 15 ++++++++++++++-
>>>>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>>> index 943bd074a5d3..0dc8294582c6 100644
>>>>> --- a/arch/x86/kvm/svm/sev.c
>>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>>> @@ -444,7 +444,6 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>>>>>  	if (ret)
>>>>>  		goto e_no_asid;
>>>>>  
>>>>> -	init_args.probe = false;
>>>>>  	ret = sev_platform_init(&init_args);
>>>>>  	if (ret)
>>>>>  		goto e_free;
>>>>> @@ -2953,6 +2952,7 @@ void __init sev_set_cpu_caps(void)
>>>>>  void __init sev_hardware_setup(void)
>>>>>  {
>>>>>  	unsigned int eax, ebx, ecx, edx, sev_asid_count, sev_es_asid_count;
>>>>> +	struct sev_platform_init_args init_args = {0};
>>>>
>>>> Will this cause issues if KVM is built-in and INIT_EX is being used
>>>> (init_ex_path ccp parameter)? The probe parameter is used for
>>>> initialization done before the filesystem is available.
>>>>
>>>
>>> Yes, this will cause issues if KVM is builtin and INIT_EX is being used,
>>> but my question is how will INIT_EX be used when we move SEV INIT
>>> to KVM ?
>>>
>>> If we continue to use the probe field here and also continue to support
>>> psp_init_on_probe module parameter for CCP, how will SEV INIT_EX be
>>> invoked ? 
>>>
>>> How is SEV INIT_EX invoked in PSP driver currently if psp_init_on_probe
>>> parameter is set to false ?
>>>
>>> The KVM path to invoke sev_platform_init() when a SEV VM is being launched 
>>> cannot be used because QEMU checks for SEV to be initialized before
>>> invoking this code path to launch the guest.
>>
>> Qemu only requires that for an SEV-ES guest. I was able to use the
>> init_ex_path=/root/... and psp_init_on_probe=0 to successfully delay SEV
>> INIT_EX and launch an SEV guest.
>>
> 
> Thanks Tom, i will make sure that we continue to support both the probe
> field and psp_init_on_probe module parameter for CCP modules as part of v4.
> 
>>>

It looks like i have hit a serious blocker issue with this approach of moving
SEV/SNP initialization to KVM module load time. 

While testing with kvm_amd and PSP driver built-in, it looks like kvm_amd
driver is being loaded/initialized before PSP driver is loaded, and that
causes sev_platform_init() call from sev_hardware_setup(kvm_amd) to fail:

[   10.717898] kvm_amd: TSC scaling supported
[   10.722470] kvm_amd: Nested Virtualization enabled
[   10.727816] kvm_amd: Nested Paging enabled
[   10.732388] kvm_amd: LBR virtualization supported
[   10.737639] kvm_amd: SEV enabled (ASIDs 100 - 509)
[   10.742985] kvm_amd: SEV-ES enabled (ASIDs 1 - 99)
[   10.748333] kvm_amd: SEV-SNP enabled (ASIDs 1 - 99)
[   10.753768] PSP driver not init                        <<<---- sev_platform_init() returns failure as PSP driver is still not initialized
[   10.757563] kvm_amd: Virtual VMLOAD VMSAVE supported
[   10.763124] kvm_amd: Virtual GIF supported
...
...
[   12.514857] ccp 0000:23:00.1: enabling device (0000 -> 0002)
[   12.521691] ccp 0000:23:00.1: no command queues available
[   12.527991] ccp 0000:23:00.1: sev enabled
[   12.532592] ccp 0000:23:00.1: psp enabled
[   12.537382] ccp 0000:a2:00.1: enabling device (0000 -> 0002)
[   12.544389] ccp 0000:a2:00.1: no command queues available
[   12.550627] ccp 0000:a2:00.1: psp enabled

depmod -> modules.builtin show kernel/arch/x86/kvm/kvm_amd.ko higher on the list and before kernel/drivers/crypto/ccp/ccp.ko

modules.builtin: 
kernel/arch/x86/kvm/kvm.ko
kernel/arch/x86/kvm/kvm-amd.ko
...
...
kernel/drivers/crypto/ccp/ccp.ko

I believe that the modules which are compiled first get called first and it looks like that the only way to change the order for
builtin modules is by changing which makefiles get compiled first ?

Is there a way to change the load order of built-in modules and/or change dependency of built-in modules ?

As of now, this looks like to be a blocker for moving SEV/SNP init to KVM module load time as this approach will not
work if kvm_amd is built-in. 

Thanks,
Ashish

>>>>
>>>>>  	bool sev_snp_supported = false;
>>>>>  	bool sev_es_supported = false;
>>>>>  	bool sev_supported = false;
>>>>> @@ -3069,6 +3069,16 @@ void __init sev_hardware_setup(void)
>>>>>  	sev_supported_vmsa_features = 0;
>>>>>  	if (sev_es_debug_swap_enabled)
>>>>>  		sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP;
>>>>> +
>>>>> +	if (!sev_enabled)
>>>>> +		return;
>>>>> +
>>>>> +	/*
>>>>> +	 * NOTE: Always do SNP INIT regardless of sev_snp_supported
>>>>> +	 * as SNP INIT has to be done to launch legacy SEV/SEV-ES
>>>>> +	 * VMs in case SNP is enabled system-wide.
>>>>> +	 */
>>>>> +	sev_platform_init(&init_args);
>>>>>  }
>>>>>  
>>>>>  void sev_hardware_unsetup(void)
>>>>> @@ -3084,6 +3094,9 @@ void sev_hardware_unsetup(void)
>>>>>  
>>>>>  	misc_cg_set_capacity(MISC_CG_RES_SEV, 0);
>>>>>  	misc_cg_set_capacity(MISC_CG_RES_SEV_ES, 0);
>>>>> +
>>>>> +	/* Do SEV and SNP Shutdown */
>>>>> +	sev_platform_shutdown();
>>>>>  }
>>>>>  
>>>>>  int sev_cpu_init(struct svm_cpu_data *sd)
>>>
>