On 12/10/2024 7:01 PM, Kalra, Ashish wrote:
>
>
> On 12/10/2024 6:48 PM, Kalra, Ashish wrote:
>>
>>
>> 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()
>
> Which also assists in using the same internal interface __sev_firmware_shutdown()
> to be called both with sev_platform_shutdown() and the SNP panic notifier to shutdown
> both SEV and SNP (in that order).
>
> Thanks,
> Ashish
>
>>
>> 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()).
>>
Worked on separating SEV and SNP initialization and got it working, but it needs
additional fix in qemu to remove the check for SEV-ES being already initialized (i.e, SEV
INIT being already done) as below to ensure that SEV INIT is done on demand when
SEV/SEV-ES guests are being launched:
diff --git a/target/i386/sev.c b/target/i386/sev.c
index a0d271f898..bb541f9d41 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1503,15 +1503,6 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
}
}
- if (sev_es_enabled() && !sev_snp_enabled()) {
- if (!(status.flags & SEV_STATUS_FLAGS_CONFIG_ES)) {
- error_setg(errp, "%s: guest policy requires SEV-ES, but "
- "host SEV-ES support unavailable",
- __func__);
- return -1;
- }
- }
-
trace_kvm_sev_init();
switch (x86_klass->kvm_type(X86_CONFIDENTIAL_GUEST(sev_common))) {
case KVM_X86_DEFAULT_VM:
>>
>>>
>>> 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))
>>
>
