Hello Sean,
On 2/28/2025 12:31 PM, Sean Christopherson wrote:
> On Tue, Feb 25, 2025, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@xxxxxxx>
>>
>> Move platform initialization of SEV/SNP from CCP driver probe time to
>> KVM module load time so that KVM can do SEV/SNP platform initialization
>> explicitly if it actually wants to use SEV/SNP functionality.
>>
>> Add support for KVM to explicitly call into the CCP driver at load time
>> to initialize SEV/SNP. If required, this behavior can be altered with KVM
>> module parameters to not do SEV/SNP platform initialization at module load
>> time. Additionally, a corresponding SEV/SNP platform shutdown is invoked
>> during KVM module unload time.
>>
>> Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
>> Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
>> Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
>> ---
>> arch/x86/kvm/svm/sev.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 74525651770a..0bc6c0486071 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -2933,6 +2933,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};
>> bool sev_snp_supported = false;
>> bool sev_es_supported = false;
>> bool sev_supported = false;
>> @@ -3059,6 +3060,17 @@ 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;
>> +
>> + /*
>> + * Always perform SEV initialization at setup time to avoid
>> + * complications when performing SEV initialization later
>> + * (such as suspending active guests, etc.).
>
> This is misleading and wildly incomplete. *SEV* doesn't have complications, *SNP*
> has complications. And looking through sev_platform_init(), all of this code
> is buggy.
>
> The sev_platform_init() return code is completely disconnected from SNP setup.
> It can return errors even if SNP setup succeeds, and can return success even if
> SNP setup fails.
>
> I also think it makes sense to require SNP to be initialized during KVM setup.
There are a few important considerations here:
This is true that we require SNP to be initialized during KVM setup
and also as mentioned earlier we need SNP to be initialized (SNP_INIT_EX
should be done) for SEV INIT to succeed if SNP host support is enabled.
So we essentially have to do SNP_INIT(_EX) for launching SEV/SEV-ES VMs when
SNP host support is enabled. In other words, if SNP_INIT(_EX) is not issued or
fails then SEV/SEV-ES VMs can't be launched once SNP host support (SYSCFG.SNPEn)
is enabled as SEV INIT will fail in such a situation.
And the other consideration is that runtime setup of especially SEV-ES VMs will not
work if/when first SEV-ES VM is launched, if SEV INIT has not been issued at
KVM setup time.
This is because qemu has a check for SEV INIT to have been done (via SEV platform
status command) prior to launching SEV-ES VMs via KVM_SEV_INIT2 ioctl.
So effectively, __sev_guest_init() does not get invoked in case of launching
SEV_ES VMs, if sev_platform_init() has not been done to issue SEV INIT in
sev_hardware_setup().
In other words the deferred initialization only works for SEV VMs and not SEV-ES VMs.
For this reason, we decided to do sev_platform_init() to do both SNP and SEV/SEV-ES
initialization (SEV INIT) as part of sev_hardware_setup() and then do an implicit
SEV shutdown prior to SNP_DOWNLOAD_FIRMWARE_EX command followed by (implicit) SEV INIT
after the DLFW_EX command to facilitate SEV firmware hotloading.
Thanks,
Ashish
> I don't see anything in __sev_snp_init_locked() that suggests SNP initialization
> can magically succeed at runtime if it failed at boot. To keep things sane and
> simple, I think KVM should reject module load if SNP is requested, setup fails,
> and kvm-amd.ko is a module. If kvm-amd.ko is built-in and SNP fails, just disable
> SNP support. I.e. when possible, let userspace decide what to do, but don't bring
> down all of KVM just because SNP setup failed.
>
> The attached patches are compile-tested (mostly), can you please test them and
> slot them in?
>
>> + */
>> + init_args.probe = true;
>> + sev_platform_init(&init_args);
>> }
>>
>> void sev_hardware_unsetup(void)
>> @@ -3074,6 +3086,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 */
>
> Meh, just omit this comment.
>
>> + sev_platform_shutdown();
>> }
>>
>> int sev_cpu_init(struct svm_cpu_data *sd)
>> --
>> 2.34.1
>>
