Message-ID: <cf34c479-c741-4173-8a94-b2e69e89810b@amd.com>
Date: Fri, 28 Feb 2025 14:38:46 -0600
From: "Kalra, Ashish" <ashish.kalra@....com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: pbonzini@...hat.com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
 thomas.lendacky@....com, john.allen@....com, herbert@...dor.apana.org.au,
 michael.roth@....com, dionnaglaze@...gle.com, nikunj@....com,
 ardb@...nel.org, kevinloughlin@...gle.com, Neeraj.Upadhyay@....com,
 aik@....com, kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-crypto@...r.kernel.org, linux-coco@...ts.linux.dev
Subject: Re: [PATCH v5 6/7] KVM: SVM: Add support to initialize SEV/SNP
 functionality in KVM

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@....com>
>>
>> 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@...gle.com>
>> Reviewed-by: Tom Lendacky <thomas.lendacky@....com>
>> Signed-off-by: Ashish Kalra <ashish.kalra@....com>
>> ---
>>  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
>>

Powered by blists - more mailing lists