On 12/10/2024 4:57 PM, Sean Christopherson wrote:
> On Tue, Dec 10, 2024, Ashish Kalra wrote:
>> On 12/9/2024 7:30 PM, Sean Christopherson wrote:
>>> Why can't we simply separate SNP initialization from SEV+ initialization?
>>
>> Yes we can do that, by default KVM module load time will only do SNP initialization,
>> and then we will do SEV initialization if a SEV VM is being launched.
>>
>> This will remove the probe parameter from init_args above, but will need to add another
>> parameter like VM type to specify if SNP or SEV initialization is to be performed with
>> the sev_platform_init() call.
>
> Any reason not to simply use separate APIs? E.g. sev_snp_platform_init() and
> sev_platform_init()?
One reason is the need to do SEV SHUTDOWN before SNP_SHUTDOWN if any SEV VMs are active
and this is taken care with the single API interface sev_platform_shutdown(), so that's
why considering using a consistent API interface for both INIT and SHUTDOWN ...
- sev_platform_init()
- sev_platform_shutdown()
We can use separate APIs, but then we probably need the same for shutdown too and KVM
will need to keep track of any active SEV VMs and ensure to call sev_platform_shutdown()
before sev_snp_platform_shutdown() (as part of sev_hardware_unsetup()).
Thanks,
Ashish
>
> And if the cc_platform_has(CC_ATTR_HOST_SEV_SNP) check is moved inside of
> sev_snp_platform_init() (probably needs to be there anyways), then the KVM code
> is quite simple and will undergo minimal churn.
>
> E.g.
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 5e4581ed0ef1..7e75bc55d017 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -404,7 +404,6 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
> unsigned long vm_type)
> {
> struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> - struct sev_platform_init_args init_args = {0};
> bool es_active = vm_type != KVM_X86_SEV_VM;
> u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
> int ret;
> @@ -444,8 +443,7 @@ 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);
> + ret = sev_platform_init();
> if (ret)
> goto e_free;
>
> @@ -3053,7 +3051,7 @@ void __init sev_hardware_setup(void)
> sev_es_asid_count = min_sev_asid - 1;
> WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count));
> sev_es_supported = true;
> - sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP);
> + sev_snp_supported = sev_snp_enabled && !sev_snp_platform_init();
>
> out:
> if (boot_cpu_has(X86_FEATURE_SEV))
