Re: [PATCH v2 3/3] x86/sev: Add SEV-SNP CipherTextHiding support

[Kalra, Ashish]

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