[v5,6/7] KVM: SVM: Add support to initialize SEV/SNP functionality in KVM

Commit Message

Kalra, Ashish Feb. 25, 2025, 9:01 p.m. UTC

From: Ashish Kalra <[e-mail]>

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 <[e-mail]>
Reviewed-by: Tom Lendacky <[e-mail]>
Signed-off-by: Ashish Kalra <[e-mail]>
---
 arch/x86/kvm/svm/sev.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Sean Christopherson Feb. 28, 2025, 6:31 p.m. UTC | #1

On Tue, Feb 25, 2025, Ashish Kalra wrote:
> From: Ashish Kalra <[e-mail]>
> 
> 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 <[e-mail]>
> Reviewed-by: Tom Lendacky <[e-mail]>
> Signed-off-by: Ashish Kalra <[e-mail]>
> ---
>  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.
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
>

Kalra, Ashish Feb. 28, 2025, 8:38 p.m. UTC | #2

Hello Sean,

On 2/28/2025 12:31 PM, Sean Christopherson wrote:
> On Tue, Feb 25, 2025, Ashish Kalra wrote:
>> From: Ashish Kalra <[e-mail]>
>>
>> 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 <[e-mail]>
>> Reviewed-by: Tom Lendacky <[e-mail]>
>> Signed-off-by: Ashish Kalra <[e-mail]>
>> ---
>>  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
>>

Sean Christopherson Feb. 28, 2025, 10:32 p.m. UTC | #3

On Fri, Feb 28, 2025, Ashish Kalra wrote:
> Hello Sean,
> 
> On 2/28/2025 12:31 PM, Sean Christopherson wrote:
> > On Tue, Feb 25, 2025, Ashish Kalra wrote:
> >> +	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.

Doesn't that mean sev_platform_init() is broken and should error out if SNP
setup fails?  Because this doesn't match the above (or I'm misreading one or both).

	rc = __sev_snp_init_locked(&args->error);
	if (rc && rc != -ENODEV) {
		/*
		 * Don't abort the probe if SNP INIT failed,
		 * continue to initialize the legacy SEV firmware.
		 */
		dev_err(sev->dev, "SEV-SNP: failed to INIT, continue SEV INIT\n");
	}

And doesn't the min version check completely wreck everything?  I.e. if SNP *must*
be initialized if SYSCFG.SNPEn is set in order to utilize SEV/SEV-ES, then shouldn't
this be a fatal error too?

	if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) {
		dev_dbg(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n",
			SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
		return 0;
	}

And then aren't all of the bare calls to __sev_platform_init_locked() broken too?
E.g. if userspace calls sev_ioctl_do_pek_csr() without loading KVM, then SNP won't
be initialized and __sev_platform_init_locked() will fail, no?

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

In that case, I vote to kill off deferred initialization entirely, and commit to
enabling all of SEV+ when KVM loads (which we should have done from day one).
Assuming we can do that in a way that's compatible with the /dev/sev ioctls.

Kalra, Ashish March 3, 2025, 6:50 p.m. UTC | #4

Hello Sean,

On 2/28/2025 4:32 PM, Sean Christopherson wrote:
> On Fri, Feb 28, 2025, Ashish Kalra wrote:
>> Hello Sean,
>>
>> On 2/28/2025 12:31 PM, Sean Christopherson wrote:
>>> On Tue, Feb 25, 2025, Ashish Kalra wrote:
>>>> +	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.
> 
> Doesn't that mean sev_platform_init() is broken and should error out if SNP
> setup fails?  Because this doesn't match the above (or I'm misreading one or both).
> 
> 	rc = __sev_snp_init_locked(&args->error);
> 	if (rc && rc != -ENODEV) {
> 		/*
> 		 * Don't abort the probe if SNP INIT failed,
> 		 * continue to initialize the legacy SEV firmware.
> 		 */
> 		dev_err(sev->dev, "SEV-SNP: failed to INIT, continue SEV INIT\n");
> 	}
> 

Yes, i realized this is true and we need to return here if rc != -ENODEV.

So i will add a pre-patch to the series to fix this.

> And doesn't the min version check completely wreck everything?  I.e. if SNP *must*
> be initialized if SYSCFG.SNPEn is set in order to utilize SEV/SEV-ES, then shouldn't
> this be a fatal error too?
> 
> 	if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) {
> 		dev_dbg(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n",
> 			SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
> 		return 0;
> 	}
> 

Yes, this is also true, we need to return an error here.

> And then aren't all of the bare calls to __sev_platform_init_locked() broken too?
> E.g. if userspace calls sev_ioctl_do_pek_csr() without loading KVM, then SNP won't
> be initialized and __sev_platform_init_locked() will fail, no?

Yes, we should be calling _sev_platform_init_locked() here instead of__sev_platform_init_locked()
to ensure that both implicit SNP and SEV INIT is done for these ioctls and followed by 
__sev_firmware_shutdown() to do both SEV and SNP shutdown.

> 
>> 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.
> 
> In that case, I vote to kill off deferred initialization entirely, and commit to
> enabling all of SEV+ when KVM loads (which we should have done from day one).
> Assuming we can do that in a way that's compatible with the /dev/sev ioctls.

Yes, that's what seems to be the right approach to enabling all SEV+ when KVM loads. 

For SEV firmware hotloading we will do implicit SEV Shutdown prior to DLFW_EX
and SEV (re)INIT after that to ensure that SEV is in UNINIT state before
DLFW_EX.

We still probably want to keep the deferred initialization for SEV in 
__sev_guest_init() by calling sev_platform_init() to support the SEV INIT_EX
case.

Thanks,
Ashish

Sean Christopherson March 3, 2025, 8:49 p.m. UTC | #5

On Mon, Mar 03, 2025, Ashish Kalra wrote:
> On 2/28/2025 4:32 PM, Sean Christopherson wrote:
> > On Fri, Feb 28, 2025, Ashish Kalra wrote:
> >> 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.
> > 
> > In that case, I vote to kill off deferred initialization entirely, and commit to
> > enabling all of SEV+ when KVM loads (which we should have done from day one).
> > Assuming we can do that in a way that's compatible with the /dev/sev ioctls.
> 
> Yes, that's what seems to be the right approach to enabling all SEV+ when KVM loads. 
> 
> For SEV firmware hotloading we will do implicit SEV Shutdown prior to DLFW_EX
> and SEV (re)INIT after that to ensure that SEV is in UNINIT state before
> DLFW_EX.
> 
> We still probably want to keep the deferred initialization for SEV in 
> __sev_guest_init() by calling sev_platform_init() to support the SEV INIT_EX
> case.

Refresh me, how does INIT_EX fit into all of this?  I.e. why does it need special
casing?

Kalra, Ashish March 3, 2025, 9:39 p.m. UTC | #6

On 3/3/2025 2:49 PM, Sean Christopherson wrote:
> On Mon, Mar 03, 2025, Ashish Kalra wrote:
>> On 2/28/2025 4:32 PM, Sean Christopherson wrote:
>>> On Fri, Feb 28, 2025, Ashish Kalra wrote:
>>>> 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.
>>>
>>> In that case, I vote to kill off deferred initialization entirely, and commit to
>>> enabling all of SEV+ when KVM loads (which we should have done from day one).
>>> Assuming we can do that in a way that's compatible with the /dev/sev ioctls.
>>
>> Yes, that's what seems to be the right approach to enabling all SEV+ when KVM loads. 
>>
>> For SEV firmware hotloading we will do implicit SEV Shutdown prior to DLFW_EX
>> and SEV (re)INIT after that to ensure that SEV is in UNINIT state before
>> DLFW_EX.
>>
>> We still probably want to keep the deferred initialization for SEV in 
>> __sev_guest_init() by calling sev_platform_init() to support the SEV INIT_EX
>> case.
> 
> Refresh me, how does INIT_EX fit into all of this?  I.e. why does it need special
> casing?

For SEV INIT_EX, we need the filesystem to be up and running as the user-supplied
SEV related persistent data is read from a regular file and provided to the
INIT_EX command.

Now, with the modified SEV/SNP init flow, when SEV/SNP initialization is 
performed during KVM module load, then as i believe the filesystem will be
mounted before KVM module loads, so SEV INIT_EX can be supported without
any issues.

Therefore, we don't need deferred initialization support for SEV INIT_EX
in case of KVM being loaded as a module.

But if KVM module is built-in, then filesystem will not be mounted when 
SEV/SNP initialization is done during KVM initialization and in that case
SEV INIT_EX cannot be supported. 

Therefore to support SEV INIT_EX when KVM module is built-in, the following
will need to be done:

- Boot kernel with psp_init_on_probe=false command line.
- This ensures that during KVM initialization, only SNP INIT is done.
- Later at runtime, when filesystem has already been mounted, 
SEV VM launch will trigger deferred SEV (INIT_EX) initialization
(via the __sev_guest_init() -> sev_platform_init() code path).

NOTE: psp_init_on_probe module parameter and deferred SEV initialization
during SEV VM launch (__sev_guest_init()->sev_platform_init()) was added
specifically to support SEV INIT_EX case.

Thanks,
Ashish

Sean Christopherson March 4, 2025, 9:58 p.m. UTC | #7

On Mon, Mar 03, 2025, Ashish Kalra wrote:
> On 3/3/2025 2:49 PM, Sean Christopherson wrote:
> > On Mon, Mar 03, 2025, Ashish Kalra wrote:
> >> On 2/28/2025 4:32 PM, Sean Christopherson wrote:
> >>> On Fri, Feb 28, 2025, Ashish Kalra wrote:
> >>>> 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.
> >>>
> >>> In that case, I vote to kill off deferred initialization entirely, and commit to
> >>> enabling all of SEV+ when KVM loads (which we should have done from day one).
> >>> Assuming we can do that in a way that's compatible with the /dev/sev ioctls.
> >>
> >> Yes, that's what seems to be the right approach to enabling all SEV+ when KVM loads. 
> >>
> >> For SEV firmware hotloading we will do implicit SEV Shutdown prior to DLFW_EX
> >> and SEV (re)INIT after that to ensure that SEV is in UNINIT state before
> >> DLFW_EX.
> >>
> >> We still probably want to keep the deferred initialization for SEV in 
> >> __sev_guest_init() by calling sev_platform_init() to support the SEV INIT_EX
> >> case.
> > 
> > Refresh me, how does INIT_EX fit into all of this?  I.e. why does it need special
> > casing?
> 
> For SEV INIT_EX, we need the filesystem to be up and running as the user-supplied
> SEV related persistent data is read from a regular file and provided to the
> INIT_EX command.
> 
> Now, with the modified SEV/SNP init flow, when SEV/SNP initialization is 
> performed during KVM module load, then as i believe the filesystem will be
> mounted before KVM module loads, so SEV INIT_EX can be supported without
> any issues.
> 
> Therefore, we don't need deferred initialization support for SEV INIT_EX
> in case of KVM being loaded as a module.
> 
> But if KVM module is built-in, then filesystem will not be mounted when 
> SEV/SNP initialization is done during KVM initialization and in that case
> SEV INIT_EX cannot be supported. 
> 
> Therefore to support SEV INIT_EX when KVM module is built-in, the following
> will need to be done:
> 
> - Boot kernel with psp_init_on_probe=false command line.
> - This ensures that during KVM initialization, only SNP INIT is done.
> - Later at runtime, when filesystem has already been mounted, 
> SEV VM launch will trigger deferred SEV (INIT_EX) initialization
> (via the __sev_guest_init() -> sev_platform_init() code path).
> 
> NOTE: psp_init_on_probe module parameter and deferred SEV initialization
> during SEV VM launch (__sev_guest_init()->sev_platform_init()) was added
> specifically to support SEV INIT_EX case.

Ugh.  That's quite the unworkable mess.  sev_hardware_setup() can't determine
if SEV/SEV-ES is fully supported without initializing the platform, but userspace
needs KVM to do initialization so that SEV platform status reads out correctly.

Aha!

Isn't that a Google problem?  And one that resolves itself if initialization is
done on kvm-amd.ko load?

A system/kernel _could_ be configured to use a path during initcalls, with the
approproate initramfs magic.  So there's no hard requirement that makes init_ex_path
incompatible with CRYPTO_DEV_CCP_DD=y or CONFIG_KVM_AMD=y.  Google's environment
simply doesn't jump through those hoops.

But Google _does_ build kvm-amd.ko as a module.

So rather than carry a bunch of hard-to-follow code (and potentially impossible
constraints), always do initialization at kvm-amd.ko load, and require the platform
owner to ensure init_ex_path can be resolved when sev_hardware_setup() runs, i.e.
when kvm-amd.ko is loaded or its initcall runs.

Kalra, Ashish March 5, 2025, 1:58 a.m. UTC | #8

On 3/4/2025 3:58 PM, Sean Christopherson wrote:
> On Mon, Mar 03, 2025, Ashish Kalra wrote:
>> On 3/3/2025 2:49 PM, Sean Christopherson wrote:
>>> On Mon, Mar 03, 2025, Ashish Kalra wrote:
>>>> On 2/28/2025 4:32 PM, Sean Christopherson wrote:
>>>>> On Fri, Feb 28, 2025, Ashish Kalra wrote:
>>>>>> 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.
>>>>>
>>>>> In that case, I vote to kill off deferred initialization entirely, and commit to
>>>>> enabling all of SEV+ when KVM loads (which we should have done from day one).
>>>>> Assuming we can do that in a way that's compatible with the /dev/sev ioctls.
>>>>
>>>> Yes, that's what seems to be the right approach to enabling all SEV+ when KVM loads. 
>>>>
>>>> For SEV firmware hotloading we will do implicit SEV Shutdown prior to DLFW_EX
>>>> and SEV (re)INIT after that to ensure that SEV is in UNINIT state before
>>>> DLFW_EX.
>>>>
>>>> We still probably want to keep the deferred initialization for SEV in 
>>>> __sev_guest_init() by calling sev_platform_init() to support the SEV INIT_EX
>>>> case.
>>>
>>> Refresh me, how does INIT_EX fit into all of this?  I.e. why does it need special
>>> casing?
>>
>> For SEV INIT_EX, we need the filesystem to be up and running as the user-supplied
>> SEV related persistent data is read from a regular file and provided to the
>> INIT_EX command.
>>
>> Now, with the modified SEV/SNP init flow, when SEV/SNP initialization is 
>> performed during KVM module load, then as i believe the filesystem will be
>> mounted before KVM module loads, so SEV INIT_EX can be supported without
>> any issues.
>>
>> Therefore, we don't need deferred initialization support for SEV INIT_EX
>> in case of KVM being loaded as a module.
>>
>> But if KVM module is built-in, then filesystem will not be mounted when 
>> SEV/SNP initialization is done during KVM initialization and in that case
>> SEV INIT_EX cannot be supported. 
>>
>> Therefore to support SEV INIT_EX when KVM module is built-in, the following
>> will need to be done:
>>
>> - Boot kernel with psp_init_on_probe=false command line.
>> - This ensures that during KVM initialization, only SNP INIT is done.
>> - Later at runtime, when filesystem has already been mounted, 
>> SEV VM launch will trigger deferred SEV (INIT_EX) initialization
>> (via the __sev_guest_init() -> sev_platform_init() code path).
>>
>> NOTE: psp_init_on_probe module parameter and deferred SEV initialization
>> during SEV VM launch (__sev_guest_init()->sev_platform_init()) was added
>> specifically to support SEV INIT_EX case.
> 
> Ugh.  That's quite the unworkable mess.  sev_hardware_setup() can't determine
> if SEV/SEV-ES is fully supported without initializing the platform, but userspace
> needs KVM to do initialization so that SEV platform status reads out correctly.
> 
> Aha!
> 
> Isn't that a Google problem?  And one that resolves itself if initialization is
> done on kvm-amd.ko load?

Yes, SEV INIT_EX is mainly used/required by Google.

> 
> A system/kernel _could_ be configured to use a path during initcalls, with the
> approproate initramfs magic.  So there's no hard requirement that makes init_ex_path
> incompatible with CRYPTO_DEV_CCP_DD=y or CONFIG_KVM_AMD=y.  Google's environment
> simply doesn't jump through those hoops.
> 
> But Google _does_ build kvm-amd.ko as a module.
> 
> So rather than carry a bunch of hard-to-follow code (and potentially impossible
> constraints), always do initialization at kvm-amd.ko load, and require the platform
> owner to ensure init_ex_path can be resolved when sev_hardware_setup() runs, i.e.
> when kvm-amd.ko is loaded or its initcall runs.

So you are proposing that we drop all deferred initialization support for SEV, i.e,
we drop the psp_init_on_probe module parameter for CCP driver, remove the probe
field from sev_platform_init_args and correspondingly drop any support to skip/defer
SEV INIT in _sev_platform_init_locked() and then also drop all existing support in
KVM for SEV deferred initialization, i.e, remove the call to sev_platform_init()
from __sev_guest_init().

Thanks,
Ashish

Kalra, Ashish March 5, 2025, 10:17 p.m. UTC | #9

On 3/4/2025 7:58 PM, Kalra, Ashish wrote:
> 
> On 3/4/2025 3:58 PM, Sean Christopherson wrote:
>> On Mon, Mar 03, 2025, Ashish Kalra wrote:
>>> On 3/3/2025 2:49 PM, Sean Christopherson wrote:
>>>> On Mon, Mar 03, 2025, Ashish Kalra wrote:
>>>>> On 2/28/2025 4:32 PM, Sean Christopherson wrote:
>>>>>> On Fri, Feb 28, 2025, Ashish Kalra wrote:
>>>>>>> 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.
>>>>>>
>>>>>> In that case, I vote to kill off deferred initialization entirely, and commit to
>>>>>> enabling all of SEV+ when KVM loads (which we should have done from day one).
>>>>>> Assuming we can do that in a way that's compatible with the /dev/sev ioctls.
>>>>>
>>>>> Yes, that's what seems to be the right approach to enabling all SEV+ when KVM loads. 
>>>>>
>>>>> For SEV firmware hotloading we will do implicit SEV Shutdown prior to DLFW_EX
>>>>> and SEV (re)INIT after that to ensure that SEV is in UNINIT state before
>>>>> DLFW_EX.
>>>>>
>>>>> We still probably want to keep the deferred initialization for SEV in 
>>>>> __sev_guest_init() by calling sev_platform_init() to support the SEV INIT_EX
>>>>> case.
>>>>
>>>> Refresh me, how does INIT_EX fit into all of this?  I.e. why does it need special
>>>> casing?
>>>
>>> For SEV INIT_EX, we need the filesystem to be up and running as the user-supplied
>>> SEV related persistent data is read from a regular file and provided to the
>>> INIT_EX command.
>>>
>>> Now, with the modified SEV/SNP init flow, when SEV/SNP initialization is 
>>> performed during KVM module load, then as i believe the filesystem will be
>>> mounted before KVM module loads, so SEV INIT_EX can be supported without
>>> any issues.
>>>
>>> Therefore, we don't need deferred initialization support for SEV INIT_EX
>>> in case of KVM being loaded as a module.
>>>
>>> But if KVM module is built-in, then filesystem will not be mounted when 
>>> SEV/SNP initialization is done during KVM initialization and in that case
>>> SEV INIT_EX cannot be supported. 
>>>
>>> Therefore to support SEV INIT_EX when KVM module is built-in, the following
>>> will need to be done:
>>>
>>> - Boot kernel with psp_init_on_probe=false command line.
>>> - This ensures that during KVM initialization, only SNP INIT is done.
>>> - Later at runtime, when filesystem has already been mounted, 
>>> SEV VM launch will trigger deferred SEV (INIT_EX) initialization
>>> (via the __sev_guest_init() -> sev_platform_init() code path).
>>>
>>> NOTE: psp_init_on_probe module parameter and deferred SEV initialization
>>> during SEV VM launch (__sev_guest_init()->sev_platform_init()) was added
>>> specifically to support SEV INIT_EX case.
>>
>> Ugh.  That's quite the unworkable mess.  sev_hardware_setup() can't determine
>> if SEV/SEV-ES is fully supported without initializing the platform, but userspace
>> needs KVM to do initialization so that SEV platform status reads out correctly.
>>
>> Aha!
>>
>> Isn't that a Google problem?  And one that resolves itself if initialization is
>> done on kvm-amd.ko load?
> 
> Yes, SEV INIT_EX is mainly used/required by Google.
> 
>>
>> A system/kernel _could_ be configured to use a path during initcalls, with the
>> approproate initramfs magic.  So there's no hard requirement that makes init_ex_path
>> incompatible with CRYPTO_DEV_CCP_DD=y or CONFIG_KVM_AMD=y.  Google's environment
>> simply doesn't jump through those hoops.
>>
>> But Google _does_ build kvm-amd.ko as a module.
>>
>> So rather than carry a bunch of hard-to-follow code (and potentially impossible
>> constraints), always do initialization at kvm-amd.ko load, and require the platform
>> owner to ensure init_ex_path can be resolved when sev_hardware_setup() runs, i.e.
>> when kvm-amd.ko is loaded or its initcall runs.
> 
> So you are proposing that we drop all deferred initialization support for SEV, i.e,
> we drop the psp_init_on_probe module parameter for CCP driver, remove the probe
> field from sev_platform_init_args and correspondingly drop any support to skip/defer
> SEV INIT in _sev_platform_init_locked() and then also drop all existing support in
> KVM for SEV deferred initialization, i.e, remove the call to sev_platform_init()
> from __sev_guest_init().
> 

Also looking at the patch commit logs for psp_init_on_probe parameter:
https://lore.kernel.org/lkml/[e-mail]/

User may decouple module init from PSP init due to use of the INIT_EX support in
upcoming patch which allows for users to save PSP's internal state to file. The
file may be unavailable at module init.

So it probably makes sense to keep SEV deferred initialization support there, as it may
not only be filesystem unavailability at CCP module load (or KVM module load with new flow),
but user may have the file available only later after module load/init.

Thanks,
Ashish

Kalra, Ashish March 12, 2025, 11:02 p.m. UTC | #10

Hello Sean,

On 3/4/2025 3:58 PM, Sean Christopherson wrote:
> On Mon, Mar 03, 2025, Ashish Kalra wrote:
>> On 3/3/2025 2:49 PM, Sean Christopherson wrote:
>>> On Mon, Mar 03, 2025, Ashish Kalra wrote:
>>>> On 2/28/2025 4:32 PM, Sean Christopherson wrote:
>>>>> On Fri, Feb 28, 2025, Ashish Kalra wrote:
>>>>>> 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.
>>>>>
>>>>> In that case, I vote to kill off deferred initialization entirely, and commit to
>>>>> enabling all of SEV+ when KVM loads (which we should have done from day one).
>>>>> Assuming we can do that in a way that's compatible with the /dev/sev ioctls.
>>>>
>>>> Yes, that's what seems to be the right approach to enabling all SEV+ when KVM loads. 
>>>>
>>>> For SEV firmware hotloading we will do implicit SEV Shutdown prior to DLFW_EX
>>>> and SEV (re)INIT after that to ensure that SEV is in UNINIT state before
>>>> DLFW_EX.
>>>>
>>>> We still probably want to keep the deferred initialization for SEV in 
>>>> __sev_guest_init() by calling sev_platform_init() to support the SEV INIT_EX
>>>> case.
>>>
>>> Refresh me, how does INIT_EX fit into all of this?  I.e. why does it need special
>>> casing?
>>
>> For SEV INIT_EX, we need the filesystem to be up and running as the user-supplied
>> SEV related persistent data is read from a regular file and provided to the
>> INIT_EX command.
>>
>> Now, with the modified SEV/SNP init flow, when SEV/SNP initialization is 
>> performed during KVM module load, then as i believe the filesystem will be
>> mounted before KVM module loads, so SEV INIT_EX can be supported without
>> any issues.
>>
>> Therefore, we don't need deferred initialization support for SEV INIT_EX
>> in case of KVM being loaded as a module.
>>
>> But if KVM module is built-in, then filesystem will not be mounted when 
>> SEV/SNP initialization is done during KVM initialization and in that case
>> SEV INIT_EX cannot be supported. 
>>
>> Therefore to support SEV INIT_EX when KVM module is built-in, the following
>> will need to be done:
>>
>> - Boot kernel with psp_init_on_probe=false command line.
>> - This ensures that during KVM initialization, only SNP INIT is done.
>> - Later at runtime, when filesystem has already been mounted, 
>> SEV VM launch will trigger deferred SEV (INIT_EX) initialization
>> (via the __sev_guest_init() -> sev_platform_init() code path).
>>
>> NOTE: psp_init_on_probe module parameter and deferred SEV initialization
>> during SEV VM launch (__sev_guest_init()->sev_platform_init()) was added
>> specifically to support SEV INIT_EX case.
> 
> Ugh.  That's quite the unworkable mess.  sev_hardware_setup() can't determine
> if SEV/SEV-ES is fully supported without initializing the platform, but userspace
> needs KVM to do initialization so that SEV platform status reads out correctly.
> 

Revisiting this one again and following up on it: 

Actually SEV platform status command does not need SEV INIT to have been
done, this command can be executed in SEV UNINIT state. 

Hence, qemu can issue SEV PLATFORM_STATUS command to determine if SEV-ES is
initialized (i.e. SEV INIT has been completed) before launching SEV-ES
VMs. 

The issue is this additional check in qemu to ensure SEV INIT 
has been done before launching SEV-ES VMs, as below: 

target/i386/sev.c:
..
static int sev_common_kvm_init(..)
{
..
	sev_platform_ioctl(sev_fd, SEV_PLATFORM_STATUS, &status, &fw_error);
..
	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", ..
		}
	}
..
	sev_ioctl(sev_fd, KVM_SEV_INIT2, &args, &fw_error);
..
}

So v6 of this patch-series always does both SEV and SNP INIT(_EX) at kvm-amd.ko
load. 

But also does keep the SEV deferred initialization support in __sev_guest_init()
to handle SEV INIT_EX, in case the file containing SEV persistent data is
available later after module load.

Let me know if you have any more questions on this.

Thanks,
Ashish

> Aha!
> 
> Isn't that a Google problem?  And one that resolves itself if initialization is
> done on kvm-amd.ko load?
> 
> A system/kernel _could_ be configured to use a path during initcalls, with the
> approproate initramfs magic.  So there's no hard requirement that makes init_ex_path
> incompatible with CRYPTO_DEV_CCP_DD=y or CONFIG_KVM_AMD=y.  Google's environment
> simply doesn't jump through those hoops.
> 
> But Google _does_ build kvm-amd.ko as a module.
> 
> So rather than carry a bunch of hard-to-follow code (and potentially impossible
> constraints), always do initialization at kvm-amd.ko load, and require the platform
> owner to ensure init_ex_path can be resolved when sev_hardware_setup() runs, i.e.
> when kvm-amd.ko is loaded or its initcall runs.

Patch

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.).
+	 */
+	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 */
+	sev_platform_shutdown();
 }
 
 int sev_cpu_init(struct svm_cpu_data *sd)