| Message ID | 14f97f58d6150c6784909261db7f9a05d8d32566.1735931639.git.ashish.kalra@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Move initializing SEV/SNP functionality to KVM | expand |
On 1/3/25 14:01, Ashish Kalra wrote: > From: Ashish Kalra <ashish.kalra@amd.com> > > Remove platform initialization of SEV/SNP from PSP driver probe time and Actually, you're not removing it, yet... > move it to KVM module load time so that KVM can do SEV/SNP platform > initialization explicitly if it actually wants to use SEV/SNP > functionality. > > With this patch, KVM will explicitly call into the PSP driver at load time > to initialize SEV/SNP by default but this behavior can be altered with KVM > module parameters to not do SEV/SNP platform initialization at module load > time if required. Additionally SEV/SNP platform shutdown is invoked during > KVM module unload time. > > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > --- > arch/x86/kvm/svm/sev.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 943bd074a5d3..0dc8294582c6 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -444,7 +444,6 @@ 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); > if (ret) > goto e_free; > @@ -2953,6 +2952,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}; Will this cause issues if KVM is built-in and INIT_EX is being used (init_ex_path ccp parameter)? The probe parameter is used for initialization done before the filesystem is available. Thanks, Tom > bool sev_snp_supported = false; > bool sev_es_supported = false; > bool sev_supported = false; > @@ -3069,6 +3069,16 @@ 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; > + > + /* > + * NOTE: Always do SNP INIT regardless of sev_snp_supported > + * as SNP INIT has to be done to launch legacy SEV/SEV-ES > + * VMs in case SNP is enabled system-wide. > + */ > + sev_platform_init(&init_args); > } > > void sev_hardware_unsetup(void) > @@ -3084,6 +3094,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)
On 1/7/2025 10:42 AM, Tom Lendacky wrote: > On 1/3/25 14:01, Ashish Kalra wrote: >> From: Ashish Kalra <ashish.kalra@amd.com> >> >> Remove platform initialization of SEV/SNP from PSP driver probe time and > > Actually, you're not removing it, yet... > >> move it to KVM module load time so that KVM can do SEV/SNP platform >> initialization explicitly if it actually wants to use SEV/SNP >> functionality. >> >> With this patch, KVM will explicitly call into the PSP driver at load time >> to initialize SEV/SNP by default but this behavior can be altered with KVM >> module parameters to not do SEV/SNP platform initialization at module load >> time if required. Additionally SEV/SNP platform shutdown is invoked during >> KVM module unload time. >> >> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> >> --- >> arch/x86/kvm/svm/sev.c | 15 ++++++++++++++- >> 1 file changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c >> index 943bd074a5d3..0dc8294582c6 100644 >> --- a/arch/x86/kvm/svm/sev.c >> +++ b/arch/x86/kvm/svm/sev.c >> @@ -444,7 +444,6 @@ 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); >> if (ret) >> goto e_free; >> @@ -2953,6 +2952,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}; > > Will this cause issues if KVM is built-in and INIT_EX is being used > (init_ex_path ccp parameter)? The probe parameter is used for > initialization done before the filesystem is available. > Yes, this will cause issues if KVM is builtin and INIT_EX is being used, but my question is how will INIT_EX be used when we move SEV INIT to KVM ? If we continue to use the probe field here and also continue to support psp_init_on_probe module parameter for CCP, how will SEV INIT_EX be invoked ? How is SEV INIT_EX invoked in PSP driver currently if psp_init_on_probe parameter is set to false ? The KVM path to invoke sev_platform_init() when a SEV VM is being launched cannot be used because QEMU checks for SEV to be initialized before invoking this code path to launch the guest. Thanks, Ashish > Thanks, > Tom > >> bool sev_snp_supported = false; >> bool sev_es_supported = false; >> bool sev_supported = false; >> @@ -3069,6 +3069,16 @@ 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; >> + >> + /* >> + * NOTE: Always do SNP INIT regardless of sev_snp_supported >> + * as SNP INIT has to be done to launch legacy SEV/SEV-ES >> + * VMs in case SNP is enabled system-wide. >> + */ >> + sev_platform_init(&init_args); >> } >> >> void sev_hardware_unsetup(void) >> @@ -3084,6 +3094,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)
+Adding Peter On 1/7/2025 12:34 PM, Kalra, Ashish wrote: > > > On 1/7/2025 10:42 AM, Tom Lendacky wrote: >> On 1/3/25 14:01, Ashish Kalra wrote: >>> From: Ashish Kalra <ashish.kalra@amd.com> >>> >>> Remove platform initialization of SEV/SNP from PSP driver probe time and >> >> Actually, you're not removing it, yet... >> >>> move it to KVM module load time so that KVM can do SEV/SNP platform >>> initialization explicitly if it actually wants to use SEV/SNP >>> functionality. >>> >>> With this patch, KVM will explicitly call into the PSP driver at load time >>> to initialize SEV/SNP by default but this behavior can be altered with KVM >>> module parameters to not do SEV/SNP platform initialization at module load >>> time if required. Additionally SEV/SNP platform shutdown is invoked during >>> KVM module unload time. >>> >>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> >>> --- >>> arch/x86/kvm/svm/sev.c | 15 ++++++++++++++- >>> 1 file changed, 14 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c >>> index 943bd074a5d3..0dc8294582c6 100644 >>> --- a/arch/x86/kvm/svm/sev.c >>> +++ b/arch/x86/kvm/svm/sev.c >>> @@ -444,7 +444,6 @@ 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); >>> if (ret) >>> goto e_free; >>> @@ -2953,6 +2952,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}; >> >> Will this cause issues if KVM is built-in and INIT_EX is being used >> (init_ex_path ccp parameter)? The probe parameter is used for >> initialization done before the filesystem is available. >> > > Yes, this will cause issues if KVM is builtin and INIT_EX is being used, > but my question is how will INIT_EX be used when we move SEV INIT > to KVM ? > > If we continue to use the probe field here and also continue to support > psp_init_on_probe module parameter for CCP, how will SEV INIT_EX be > invoked ? > > How is SEV INIT_EX invoked in PSP driver currently if psp_init_on_probe > parameter is set to false ? > > The KVM path to invoke sev_platform_init() when a SEV VM is being launched > cannot be used because QEMU checks for SEV to be initialized before > invoking this code path to launch the guest. Peter, I believe that you have a different path to test SEV INIT_EX which won't be affected by this QEMU check. I will add back the probe field and psp_init_on_probe parameter for the CCP module, but i will need your help to test and verify if SEV INIT_EX works with this patch-set. Thanks, Ashish > >> Thanks, >> Tom >> >>> bool sev_snp_supported = false; >>> bool sev_es_supported = false; >>> bool sev_supported = false; >>> @@ -3069,6 +3069,16 @@ 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; >>> + >>> + /* >>> + * NOTE: Always do SNP INIT regardless of sev_snp_supported >>> + * as SNP INIT has to be done to launch legacy SEV/SEV-ES >>> + * VMs in case SNP is enabled system-wide. >>> + */ >>> + sev_platform_init(&init_args); >>> } >>> >>> void sev_hardware_unsetup(void) >>> @@ -3084,6 +3094,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) >
On 1/7/25 12:34, Kalra, Ashish wrote: > On 1/7/2025 10:42 AM, Tom Lendacky wrote: >> On 1/3/25 14:01, Ashish Kalra wrote: >>> From: Ashish Kalra <ashish.kalra@amd.com> >>> >>> Remove platform initialization of SEV/SNP from PSP driver probe time and >> >> Actually, you're not removing it, yet... >> >>> move it to KVM module load time so that KVM can do SEV/SNP platform >>> initialization explicitly if it actually wants to use SEV/SNP >>> functionality. >>> >>> With this patch, KVM will explicitly call into the PSP driver at load time >>> to initialize SEV/SNP by default but this behavior can be altered with KVM >>> module parameters to not do SEV/SNP platform initialization at module load >>> time if required. Additionally SEV/SNP platform shutdown is invoked during >>> KVM module unload time. >>> >>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> >>> --- >>> arch/x86/kvm/svm/sev.c | 15 ++++++++++++++- >>> 1 file changed, 14 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c >>> index 943bd074a5d3..0dc8294582c6 100644 >>> --- a/arch/x86/kvm/svm/sev.c >>> +++ b/arch/x86/kvm/svm/sev.c >>> @@ -444,7 +444,6 @@ 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); >>> if (ret) >>> goto e_free; >>> @@ -2953,6 +2952,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}; >> >> Will this cause issues if KVM is built-in and INIT_EX is being used >> (init_ex_path ccp parameter)? The probe parameter is used for >> initialization done before the filesystem is available. >> > > Yes, this will cause issues if KVM is builtin and INIT_EX is being used, > but my question is how will INIT_EX be used when we move SEV INIT > to KVM ? > > If we continue to use the probe field here and also continue to support > psp_init_on_probe module parameter for CCP, how will SEV INIT_EX be > invoked ? > > How is SEV INIT_EX invoked in PSP driver currently if psp_init_on_probe > parameter is set to false ? > > The KVM path to invoke sev_platform_init() when a SEV VM is being launched > cannot be used because QEMU checks for SEV to be initialized before > invoking this code path to launch the guest. Qemu only requires that for an SEV-ES guest. I was able to use the init_ex_path=/root/... and psp_init_on_probe=0 to successfully delay SEV INIT_EX and launch an SEV guest. Thanks, Tom > > Thanks, > Ashish > >> Thanks, >> Tom >> >>> bool sev_snp_supported = false; >>> bool sev_es_supported = false; >>> bool sev_supported = false; >>> @@ -3069,6 +3069,16 @@ 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; >>> + >>> + /* >>> + * NOTE: Always do SNP INIT regardless of sev_snp_supported >>> + * as SNP INIT has to be done to launch legacy SEV/SEV-ES >>> + * VMs in case SNP is enabled system-wide. >>> + */ >>> + sev_platform_init(&init_args); >>> } >>> >>> void sev_hardware_unsetup(void) >>> @@ -3084,6 +3094,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) >
On 1/8/2025 11:22 AM, Tom Lendacky wrote: > On 1/7/25 12:34, Kalra, Ashish wrote: >> On 1/7/2025 10:42 AM, Tom Lendacky wrote: >>> On 1/3/25 14:01, Ashish Kalra wrote: >>>> From: Ashish Kalra <ashish.kalra@amd.com> >>>> >>>> Remove platform initialization of SEV/SNP from PSP driver probe time and >>> >>> Actually, you're not removing it, yet... >>> >>>> move it to KVM module load time so that KVM can do SEV/SNP platform >>>> initialization explicitly if it actually wants to use SEV/SNP >>>> functionality. >>>> >>>> With this patch, KVM will explicitly call into the PSP driver at load time >>>> to initialize SEV/SNP by default but this behavior can be altered with KVM >>>> module parameters to not do SEV/SNP platform initialization at module load >>>> time if required. Additionally SEV/SNP platform shutdown is invoked during >>>> KVM module unload time. >>>> >>>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> >>>> --- >>>> arch/x86/kvm/svm/sev.c | 15 ++++++++++++++- >>>> 1 file changed, 14 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c >>>> index 943bd074a5d3..0dc8294582c6 100644 >>>> --- a/arch/x86/kvm/svm/sev.c >>>> +++ b/arch/x86/kvm/svm/sev.c >>>> @@ -444,7 +444,6 @@ 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); >>>> if (ret) >>>> goto e_free; >>>> @@ -2953,6 +2952,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}; >>> >>> Will this cause issues if KVM is built-in and INIT_EX is being used >>> (init_ex_path ccp parameter)? The probe parameter is used for >>> initialization done before the filesystem is available. >>> >> >> Yes, this will cause issues if KVM is builtin and INIT_EX is being used, >> but my question is how will INIT_EX be used when we move SEV INIT >> to KVM ? >> >> If we continue to use the probe field here and also continue to support >> psp_init_on_probe module parameter for CCP, how will SEV INIT_EX be >> invoked ? >> >> How is SEV INIT_EX invoked in PSP driver currently if psp_init_on_probe >> parameter is set to false ? >> >> The KVM path to invoke sev_platform_init() when a SEV VM is being launched >> cannot be used because QEMU checks for SEV to be initialized before >> invoking this code path to launch the guest. > > Qemu only requires that for an SEV-ES guest. I was able to use the > init_ex_path=/root/... and psp_init_on_probe=0 to successfully delay SEV > INIT_EX and launch an SEV guest. > Thanks Tom, i will make sure that we continue to support both the probe field and psp_init_on_probe module parameter for CCP modules as part of v4. >> >>> Thanks, >>> Tom >>> >>>> bool sev_snp_supported = false; >>>> bool sev_es_supported = false; >>>> bool sev_supported = false; >>>> @@ -3069,6 +3069,16 @@ 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; >>>> + >>>> + /* >>>> + * NOTE: Always do SNP INIT regardless of sev_snp_supported >>>> + * as SNP INIT has to be done to launch legacy SEV/SEV-ES >>>> + * VMs in case SNP is enabled system-wide. >>>> + */ >>>> + sev_platform_init(&init_args); >>>> } >>>> >>>> void sev_hardware_unsetup(void) >>>> @@ -3084,6 +3094,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) >>
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 943bd074a5d3..0dc8294582c6 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -444,7 +444,6 @@ 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); if (ret) goto e_free; @@ -2953,6 +2952,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; @@ -3069,6 +3069,16 @@ 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; + + /* + * NOTE: Always do SNP INIT regardless of sev_snp_supported + * as SNP INIT has to be done to launch legacy SEV/SEV-ES + * VMs in case SNP is enabled system-wide. + */ + sev_platform_init(&init_args); } void sev_hardware_unsetup(void) @@ -3084,6 +3094,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)