| Message ID | 27a491ee16015824b416e72921b02a02c27433f7.1740512583.git.ashish.kalra@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Move initializing SEV/SNP functionality to KVM | expand |
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@amd.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@google.com> >> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> >> Signed-off-by: Ashish Kalra <ashish.kalra@amd.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 >>
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.
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)