| Message ID | 14f97f58d6150c6784909261db7f9a05d8d32566.1735931639.git.ashish.kalra@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| 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) >>
Hello All, On 1/8/2025 6:27 PM, Kalra, Ashish wrote: > > > 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. > >>> It looks like i have hit a serious blocker issue with this approach of moving SEV/SNP initialization to KVM module load time. While testing with kvm_amd and PSP driver built-in, it looks like kvm_amd driver is being loaded/initialized before PSP driver is loaded, and that causes sev_platform_init() call from sev_hardware_setup(kvm_amd) to fail: [ 10.717898] kvm_amd: TSC scaling supported [ 10.722470] kvm_amd: Nested Virtualization enabled [ 10.727816] kvm_amd: Nested Paging enabled [ 10.732388] kvm_amd: LBR virtualization supported [ 10.737639] kvm_amd: SEV enabled (ASIDs 100 - 509) [ 10.742985] kvm_amd: SEV-ES enabled (ASIDs 1 - 99) [ 10.748333] kvm_amd: SEV-SNP enabled (ASIDs 1 - 99) [ 10.753768] PSP driver not init <<<---- sev_platform_init() returns failure as PSP driver is still not initialized [ 10.757563] kvm_amd: Virtual VMLOAD VMSAVE supported [ 10.763124] kvm_amd: Virtual GIF supported ... ... [ 12.514857] ccp 0000:23:00.1: enabling device (0000 -> 0002) [ 12.521691] ccp 0000:23:00.1: no command queues available [ 12.527991] ccp 0000:23:00.1: sev enabled [ 12.532592] ccp 0000:23:00.1: psp enabled [ 12.537382] ccp 0000:a2:00.1: enabling device (0000 -> 0002) [ 12.544389] ccp 0000:a2:00.1: no command queues available [ 12.550627] ccp 0000:a2:00.1: psp enabled depmod -> modules.builtin show kernel/arch/x86/kvm/kvm_amd.ko higher on the list and before kernel/drivers/crypto/ccp/ccp.ko modules.builtin: kernel/arch/x86/kvm/kvm.ko kernel/arch/x86/kvm/kvm-amd.ko ... ... kernel/drivers/crypto/ccp/ccp.ko I believe that the modules which are compiled first get called first and it looks like that the only way to change the order for builtin modules is by changing which makefiles get compiled first ? Is there a way to change the load order of built-in modules and/or change dependency of built-in modules ? As of now, this looks like to be a blocker for moving SEV/SNP init to KVM module load time as this approach will not work if kvm_amd is built-in. Thanks, Ashish >>>> >>>>> 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 Fri, Jan 10, 2025, Ashish Kalra wrote: > It looks like i have hit a serious blocker issue with this approach of moving > SEV/SNP initialization to KVM module load time. > > While testing with kvm_amd and PSP driver built-in, it looks like kvm_amd > driver is being loaded/initialized before PSP driver is loaded, and that > causes sev_platform_init() call from sev_hardware_setup(kvm_amd) to fail: > > [ 10.717898] kvm_amd: TSC scaling supported > [ 10.722470] kvm_amd: Nested Virtualization enabled > [ 10.727816] kvm_amd: Nested Paging enabled > [ 10.732388] kvm_amd: LBR virtualization supported > [ 10.737639] kvm_amd: SEV enabled (ASIDs 100 - 509) > [ 10.742985] kvm_amd: SEV-ES enabled (ASIDs 1 - 99) > [ 10.748333] kvm_amd: SEV-SNP enabled (ASIDs 1 - 99) > [ 10.753768] PSP driver not init <<<---- sev_platform_init() returns failure as PSP driver is still not initialized > [ 10.757563] kvm_amd: Virtual VMLOAD VMSAVE supported > [ 10.763124] kvm_amd: Virtual GIF supported > ... > ... > [ 12.514857] ccp 0000:23:00.1: enabling device (0000 -> 0002) > [ 12.521691] ccp 0000:23:00.1: no command queues available > [ 12.527991] ccp 0000:23:00.1: sev enabled > [ 12.532592] ccp 0000:23:00.1: psp enabled > [ 12.537382] ccp 0000:a2:00.1: enabling device (0000 -> 0002) > [ 12.544389] ccp 0000:a2:00.1: no command queues available > [ 12.550627] ccp 0000:a2:00.1: psp enabled > > depmod -> modules.builtin show kernel/arch/x86/kvm/kvm_amd.ko higher on the list and before kernel/drivers/crypto/ccp/ccp.ko > > modules.builtin: > kernel/arch/x86/kvm/kvm.ko > kernel/arch/x86/kvm/kvm-amd.ko > ... > ... > kernel/drivers/crypto/ccp/ccp.ko > > I believe that the modules which are compiled first get called first and it > looks like that the only way to change the order for builtin modules is by > changing which makefiles get compiled first ? > > Is there a way to change the load order of built-in modules and/or change > dependency of built-in modules ? The least awful option I know of would be to have the PSP use a higher priority initcall type so that it runs before the standard initcalls. When compiled as a module, all initcall types are #defined to module_init. E.g. this should work, /cross fingers diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c index 7eb3e4668286..02c49fbf6198 100644 --- a/drivers/crypto/ccp/sp-dev.c +++ b/drivers/crypto/ccp/sp-dev.c @@ -295,5 +295,6 @@ static void __exit sp_mod_exit(void) #endif } -module_init(sp_mod_init); +/* The PSP needs to be initialized before dependent modules, e.g. before KVM. */ +subsys_initcall(sp_mod_init); module_exit(sp_mod_exit);
On Fri, Jan 10, 2025 at 4:40 PM Sean Christopherson <seanjc@google.com> wrote: > > On Fri, Jan 10, 2025, Ashish Kalra wrote: > > It looks like i have hit a serious blocker issue with this approach of moving > > SEV/SNP initialization to KVM module load time. > > > > While testing with kvm_amd and PSP driver built-in, it looks like kvm_amd > > driver is being loaded/initialized before PSP driver is loaded, and that > > causes sev_platform_init() call from sev_hardware_setup(kvm_amd) to fail: > > > > [ 10.717898] kvm_amd: TSC scaling supported > > [ 10.722470] kvm_amd: Nested Virtualization enabled > > [ 10.727816] kvm_amd: Nested Paging enabled > > [ 10.732388] kvm_amd: LBR virtualization supported > > [ 10.737639] kvm_amd: SEV enabled (ASIDs 100 - 509) > > [ 10.742985] kvm_amd: SEV-ES enabled (ASIDs 1 - 99) > > [ 10.748333] kvm_amd: SEV-SNP enabled (ASIDs 1 - 99) > > [ 10.753768] PSP driver not init <<<---- sev_platform_init() returns failure as PSP driver is still not initialized > > [ 10.757563] kvm_amd: Virtual VMLOAD VMSAVE supported > > [ 10.763124] kvm_amd: Virtual GIF supported > > ... > > ... > > [ 12.514857] ccp 0000:23:00.1: enabling device (0000 -> 0002) > > [ 12.521691] ccp 0000:23:00.1: no command queues available > > [ 12.527991] ccp 0000:23:00.1: sev enabled > > [ 12.532592] ccp 0000:23:00.1: psp enabled > > [ 12.537382] ccp 0000:a2:00.1: enabling device (0000 -> 0002) > > [ 12.544389] ccp 0000:a2:00.1: no command queues available > > [ 12.550627] ccp 0000:a2:00.1: psp enabled > > > > depmod -> modules.builtin show kernel/arch/x86/kvm/kvm_amd.ko higher on the list and before kernel/drivers/crypto/ccp/ccp.ko > > > > modules.builtin: > > kernel/arch/x86/kvm/kvm.ko > > kernel/arch/x86/kvm/kvm-amd.ko > > ... > > ... > > kernel/drivers/crypto/ccp/ccp.ko > > > > I believe that the modules which are compiled first get called first and it > > looks like that the only way to change the order for builtin modules is by > > changing which makefiles get compiled first ? > > > > Is there a way to change the load order of built-in modules and/or change > > dependency of built-in modules ? > > The least awful option I know of would be to have the PSP use a higher priority > initcall type so that it runs before the standard initcalls. When compiled as > a module, all initcall types are #defined to module_init. > > E.g. this should work, /cross fingers > > diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c > index 7eb3e4668286..02c49fbf6198 100644 > --- a/drivers/crypto/ccp/sp-dev.c > +++ b/drivers/crypto/ccp/sp-dev.c > @@ -295,5 +295,6 @@ static void __exit sp_mod_exit(void) > #endif > } > > -module_init(sp_mod_init); > +/* The PSP needs to be initialized before dependent modules, e.g. before KVM. */ > +subsys_initcall(sp_mod_init); I was 2 seconds from clicking send with this exact suggestion. There are examples in 'drivers/' that use subsys_initcall / module_exit pairs. > module_exit(sp_mod_exit);
On Fri, Jan 10, 2025, Dionna Amalie Glaze wrote: > On Fri, Jan 10, 2025 at 4:40 PM Sean Christopherson <seanjc@google.com> wrote: > > > Is there a way to change the load order of built-in modules and/or change > > > dependency of built-in modules ? > > > > The least awful option I know of would be to have the PSP use a higher priority > > initcall type so that it runs before the standard initcalls. When compiled as > > a module, all initcall types are #defined to module_init. > > > > E.g. this should work, /cross fingers > > > > diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c > > index 7eb3e4668286..02c49fbf6198 100644 > > --- a/drivers/crypto/ccp/sp-dev.c > > +++ b/drivers/crypto/ccp/sp-dev.c > > @@ -295,5 +295,6 @@ static void __exit sp_mod_exit(void) > > #endif > > } > > > > -module_init(sp_mod_init); > > +/* The PSP needs to be initialized before dependent modules, e.g. before KVM. */ > > +subsys_initcall(sp_mod_init); > > I was 2 seconds from clicking send with this exact suggestion. There > are examples in 'drivers/' that use subsys_initcall / module_exit > pairs. Ha! For once, I wasn't too slow due to writing an overly verbose message :-)
On 1/10/2025 6:40 PM, Sean Christopherson wrote: > On Fri, Jan 10, 2025, Ashish Kalra wrote: >> It looks like i have hit a serious blocker issue with this approach of moving >> SEV/SNP initialization to KVM module load time. >> >> While testing with kvm_amd and PSP driver built-in, it looks like kvm_amd >> driver is being loaded/initialized before PSP driver is loaded, and that >> causes sev_platform_init() call from sev_hardware_setup(kvm_amd) to fail: >> >> [ 10.717898] kvm_amd: TSC scaling supported >> [ 10.722470] kvm_amd: Nested Virtualization enabled >> [ 10.727816] kvm_amd: Nested Paging enabled >> [ 10.732388] kvm_amd: LBR virtualization supported >> [ 10.737639] kvm_amd: SEV enabled (ASIDs 100 - 509) >> [ 10.742985] kvm_amd: SEV-ES enabled (ASIDs 1 - 99) >> [ 10.748333] kvm_amd: SEV-SNP enabled (ASIDs 1 - 99) >> [ 10.753768] PSP driver not init <<<---- sev_platform_init() returns failure as PSP driver is still not initialized >> [ 10.757563] kvm_amd: Virtual VMLOAD VMSAVE supported >> [ 10.763124] kvm_amd: Virtual GIF supported >> ... >> ... >> [ 12.514857] ccp 0000:23:00.1: enabling device (0000 -> 0002) >> [ 12.521691] ccp 0000:23:00.1: no command queues available >> [ 12.527991] ccp 0000:23:00.1: sev enabled >> [ 12.532592] ccp 0000:23:00.1: psp enabled >> [ 12.537382] ccp 0000:a2:00.1: enabling device (0000 -> 0002) >> [ 12.544389] ccp 0000:a2:00.1: no command queues available >> [ 12.550627] ccp 0000:a2:00.1: psp enabled >> >> depmod -> modules.builtin show kernel/arch/x86/kvm/kvm_amd.ko higher on the list and before kernel/drivers/crypto/ccp/ccp.ko >> >> modules.builtin: >> kernel/arch/x86/kvm/kvm.ko >> kernel/arch/x86/kvm/kvm-amd.ko >> ... >> ... >> kernel/drivers/crypto/ccp/ccp.ko >> >> I believe that the modules which are compiled first get called first and it >> looks like that the only way to change the order for builtin modules is by >> changing which makefiles get compiled first ? >> >> Is there a way to change the load order of built-in modules and/or change >> dependency of built-in modules ? > > The least awful option I know of would be to have the PSP use a higher priority > initcall type so that it runs before the standard initcalls. When compiled as > a module, all initcall types are #defined to module_init. > > E.g. this should work, /cross fingers > > diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c > index 7eb3e4668286..02c49fbf6198 100644 > --- a/drivers/crypto/ccp/sp-dev.c > +++ b/drivers/crypto/ccp/sp-dev.c > @@ -295,5 +295,6 @@ static void __exit sp_mod_exit(void) > #endif > } > > -module_init(sp_mod_init); > +/* The PSP needs to be initialized before dependent modules, e.g. before KVM. */ > +subsys_initcall(sp_mod_init); > module_exit(sp_mod_exit); Thanks for the suggestion, but there are actually two major issues here: With the above change, PSP driver initialization fails as following: ... [ 7.274005] pci 0000:20:08.1: bridge window [mem 0xf6200000-0xf64fffff]: not claimed; can't enable device [ 7.277945] pci 0000:20:08.1: Error enabling bridge (-22), continuing [ 7.281947] ccp 0000:23:00.1: BAR 2 [mem 0xf6300000-0xf63fffff]: not claimed; can't enable device [ 7.285945] ccp 0000:23:00.1: pcim_enable_device failed (-22) [ 7.289943] ccp 0000:23:00.1: initialization failed [ 7.293944] ccp 0000:23:00.1: probe with driver ccp failed with error -22 [ 7.301981] pci 0000:a0:08.1: bridge window [mem 0xb6200000-0xb63fffff]: not claimed; can't enable device [ 7.313956] pci 0000:a0:08.1: Error enabling bridge (-22), continuing [ 7.321947] ccp 0000:a2:00.1: BAR 2 [mem 0xb6200000-0xb62fffff]: not claimed; can't enable device [ 7.329945] ccp 0000:a2:00.1: pcim_enable_device failed (-22) [ 7.337943] ccp 0000:a2:00.1: initialization failed [ 7.341946] ccp 0000:a2:00.1: probe with driver ccp failed with error -22 ... It looks as PCI bus resource allocation is still not done, hence PSP driver cannot be enabled as early as subsys_initcall, it can be initialized probably via device_initcall(), but then that will be too late as kvm_amd would have been initialized before that. Additionally, it looks like that there is an issue with SNP host support being enabled with kvm_amd module being built-in: SNP host support is enabled in snp_rmptable_init() in arch/x86/virt/svm/sev.c, which is invoked as a device_initcall(). Here device_initcall() is used as snp_rmptable_init() expects AMD IOMMU SNP support to be enabled prior to it and the AMD IOMMU driver is initialized after PCI bus enumeration. Now, if kvm_amd module is built-in, it gets initialized before SNP host support is enabled in snp_rmptable_init() : [ 10.131811] kvm_amd: TSC scaling supported [ 10.136384] kvm_amd: Nested Virtualization enabled [ 10.141734] kvm_amd: Nested Paging enabled [ 10.146304] kvm_amd: LBR virtualization supported [ 10.151557] kvm_amd: SEV enabled (ASIDs 100 - 509) [ 10.156905] kvm_amd: SEV-ES enabled (ASIDs 1 - 99) [ 10.162256] kvm_amd: SEV-SNP enabled (ASIDs 1 - 99) [ 10.167701] PSP driver not init [ 10.171508] kvm_amd: Virtual VMLOAD VMSAVE supported [ 10.177052] kvm_amd: Virtual GIF supported ... ... [ 10.201648] kvm_amd: in svm_enable_virtualization_cpu WRMSR VM_HSAVE_PA non-zero And then svm_x86_ops->enable_virtualization_cpu() (svm_enable_virtualization_cpu) programs MSR_VM_HSAVE_PA as following: wrmsrl(MSR_VM_HSAVE_PA, sd->save_area_pa); So VM_HSAVE_PA is non-zero before SNP support is enabled on all CPUs. snp_rmptable_init() gets invoked after svm_enable_virtualization_cpu() as following : ... [ 11.256138] kvm_amd: in svm_enable_virtualization_cpu WRMSR VM_HSAVE_PA non-zero ... [ 11.264918] SEV-SNP: in snp_rmptable_init This triggers a #GP exception in snp_rmptable_init() when snp_enable() is invoked to set SNP_EN in SYSCFG MSR: [ 11.294289] unchecked MSR access error: WRMSR to 0xc0010010 (tried to write 0x0000000003fc0000) at rIP: 0xffffffffaf5d5c28 (native_write_msr+0x8/0x30) ... [ 11.294404] Call Trace: [ 11.294482] <IRQ> [ 11.294513] ? show_stack_regs+0x26/0x30 [ 11.294522] ? ex_handler_msr+0x10f/0x180 [ 11.294529] ? search_extable+0x2b/0x40 [ 11.294538] ? fixup_exception+0x2dd/0x340 [ 11.294542] ? exc_general_protection+0x14f/0x440 [ 11.294550] ? asm_exc_general_protection+0x2b/0x30 [ 11.294557] ? __pfx_snp_enable+0x10/0x10 [ 11.294567] ? native_write_msr+0x8/0x30 [ 11.294570] ? __snp_enable+0x5d/0x70 [ 11.294575] snp_enable+0x19/0x20 [ 11.294578] __flush_smp_call_function_queue+0x9c/0x3a0 [ 11.294586] generic_smp_call_function_single_interrupt+0x17/0x20 [ 11.294589] __sysvec_call_function+0x20/0x90 [ 11.294596] sysvec_call_function+0x80/0xb0 [ 11.294601] </IRQ> [ 11.294603] <TASK> [ 11.294605] asm_sysvec_call_function+0x1f/0x30 ... [ 11.294631] arch_cpu_idle+0xd/0x20 [ 11.294633] default_idle_call+0x34/0xd0 [ 11.294636] do_idle+0x1f1/0x230 [ 11.294643] ? complete+0x71/0x80 [ 11.294649] cpu_startup_entry+0x30/0x40 [ 11.294652] start_secondary+0x12d/0x160 [ 11.294655] common_startup_64+0x13e/0x141 [ 11.294662] </TASK> This #GP exception is getting triggered due to the following errata for AMD family 19h Models 10h-1Fh Processors: Processor may generate spurious #GP(0) Exception on WRMSR instruction: Description: The Processor will generate a spurious #GP(0) Exception on a WRMSR instruction if the following conditions are all met: - the target of the WRMSR is a SYSCFG register. - the write changes the value of SYSCFG.SNPEn from 0 to 1. - One of the threads that share the physical core has a non-zero value in the VM_HSAVE_PA MSR. The suggested workaround is when enabling SNP, program VM_HSAVE_PA to 0h on both threads that share a physical core before setting SYSCFG.SNPEn The document being referred to above: https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/revision-guides/57095-PUB_1_01.pdf Therefore, with kvm_amd module being built-in, KVM/SVM initialization happens before Host SNP is enabled and this SVM initialization sets VM_HSAVE_PA to non-zero, which then triggers this #GP when SYSCFG.SNPEn is being set and this will subsequently cause SNP_INIT(_EX) to fail with INVALID_CONFIG error as SYSCFG[SnpEn] is not set on all CPUs. So it looks like the current SNP host enabling code and effectively SNP is broken with respect to the KVM module being built-in. Essentially SNP host enabling code should be invoked before KVM initialization, which is currently not the case when KVM is built-in. Additionally, the PSP driver probably needs to be initialized at device_initcall level if it is built-in, but that is much later than KVM module initialization, therefore, that is blocker for moving SEV/SNP initialization to KVM module load time instead of PSP module probe time. Do note that i have verified and tested that PSP module initialization works when invoked as a device_initcall(). Thanks, Ashish
On 1/13/2025 9:03 AM, Kalra, Ashish wrote: > > On 1/10/2025 6:40 PM, Sean Christopherson wrote: >> On Fri, Jan 10, 2025, Ashish Kalra wrote: >>> It looks like i have hit a serious blocker issue with this approach of moving >>> SEV/SNP initialization to KVM module load time. >>> >>> While testing with kvm_amd and PSP driver built-in, it looks like kvm_amd >>> driver is being loaded/initialized before PSP driver is loaded, and that >>> causes sev_platform_init() call from sev_hardware_setup(kvm_amd) to fail: >>> >>> [ 10.717898] kvm_amd: TSC scaling supported >>> [ 10.722470] kvm_amd: Nested Virtualization enabled >>> [ 10.727816] kvm_amd: Nested Paging enabled >>> [ 10.732388] kvm_amd: LBR virtualization supported >>> [ 10.737639] kvm_amd: SEV enabled (ASIDs 100 - 509) >>> [ 10.742985] kvm_amd: SEV-ES enabled (ASIDs 1 - 99) >>> [ 10.748333] kvm_amd: SEV-SNP enabled (ASIDs 1 - 99) >>> [ 10.753768] PSP driver not init <<<---- sev_platform_init() returns failure as PSP driver is still not initialized >>> [ 10.757563] kvm_amd: Virtual VMLOAD VMSAVE supported >>> [ 10.763124] kvm_amd: Virtual GIF supported >>> ... >>> ... >>> [ 12.514857] ccp 0000:23:00.1: enabling device (0000 -> 0002) >>> [ 12.521691] ccp 0000:23:00.1: no command queues available >>> [ 12.527991] ccp 0000:23:00.1: sev enabled >>> [ 12.532592] ccp 0000:23:00.1: psp enabled >>> [ 12.537382] ccp 0000:a2:00.1: enabling device (0000 -> 0002) >>> [ 12.544389] ccp 0000:a2:00.1: no command queues available >>> [ 12.550627] ccp 0000:a2:00.1: psp enabled >>> >>> depmod -> modules.builtin show kernel/arch/x86/kvm/kvm_amd.ko higher on the list and before kernel/drivers/crypto/ccp/ccp.ko >>> >>> modules.builtin: >>> kernel/arch/x86/kvm/kvm.ko >>> kernel/arch/x86/kvm/kvm-amd.ko >>> ... >>> ... >>> kernel/drivers/crypto/ccp/ccp.ko >>> >>> I believe that the modules which are compiled first get called first and it >>> looks like that the only way to change the order for builtin modules is by >>> changing which makefiles get compiled first ? >>> >>> Is there a way to change the load order of built-in modules and/or change >>> dependency of built-in modules ? >> >> The least awful option I know of would be to have the PSP use a higher priority >> initcall type so that it runs before the standard initcalls. When compiled as >> a module, all initcall types are #defined to module_init. >> >> E.g. this should work, /cross fingers >> >> diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c >> index 7eb3e4668286..02c49fbf6198 100644 >> --- a/drivers/crypto/ccp/sp-dev.c >> +++ b/drivers/crypto/ccp/sp-dev.c >> @@ -295,5 +295,6 @@ static void __exit sp_mod_exit(void) >> #endif >> } >> >> -module_init(sp_mod_init); >> +/* The PSP needs to be initialized before dependent modules, e.g. before KVM. */ >> +subsys_initcall(sp_mod_init); >> module_exit(sp_mod_exit); > > Thanks for the suggestion, but there are actually two major issues here: > > With the above change, PSP driver initialization fails as following: > > ... > [ 7.274005] pci 0000:20:08.1: bridge window [mem 0xf6200000-0xf64fffff]: not claimed; can't enable device > [ 7.277945] pci 0000:20:08.1: Error enabling bridge (-22), continuing > [ 7.281947] ccp 0000:23:00.1: BAR 2 [mem 0xf6300000-0xf63fffff]: not claimed; can't enable device > [ 7.285945] ccp 0000:23:00.1: pcim_enable_device failed (-22) > [ 7.289943] ccp 0000:23:00.1: initialization failed > [ 7.293944] ccp 0000:23:00.1: probe with driver ccp failed with error -22 > [ 7.301981] pci 0000:a0:08.1: bridge window [mem 0xb6200000-0xb63fffff]: not claimed; can't enable device > [ 7.313956] pci 0000:a0:08.1: Error enabling bridge (-22), continuing > [ 7.321947] ccp 0000:a2:00.1: BAR 2 [mem 0xb6200000-0xb62fffff]: not claimed; can't enable device > [ 7.329945] ccp 0000:a2:00.1: pcim_enable_device failed (-22) > [ 7.337943] ccp 0000:a2:00.1: initialization failed > [ 7.341946] ccp 0000:a2:00.1: probe with driver ccp failed with error -22 > ... > > It looks as PCI bus resource allocation is still not done, hence PSP driver cannot be enabled as early as subsys_initcall, > it can be initialized probably via device_initcall(), but then that will be too late as kvm_amd would have been initialized before that. > > Additionally, it looks like that there is an issue with SNP host support being enabled with kvm_amd module being built-in: > > SNP host support is enabled in snp_rmptable_init() in arch/x86/virt/svm/sev.c, which is invoked as a device_initcall(). > Here device_initcall() is used as snp_rmptable_init() expects AMD IOMMU SNP support to be enabled prior to it and the AMD IOMMU > driver is initialized after PCI bus enumeration. > > Now, if kvm_amd module is built-in, it gets initialized before SNP host support is enabled in snp_rmptable_init() : > > [ 10.131811] kvm_amd: TSC scaling supported > [ 10.136384] kvm_amd: Nested Virtualization enabled > [ 10.141734] kvm_amd: Nested Paging enabled > [ 10.146304] kvm_amd: LBR virtualization supported > [ 10.151557] kvm_amd: SEV enabled (ASIDs 100 - 509) > [ 10.156905] kvm_amd: SEV-ES enabled (ASIDs 1 - 99) > [ 10.162256] kvm_amd: SEV-SNP enabled (ASIDs 1 - 99) > [ 10.167701] PSP driver not init > [ 10.171508] kvm_amd: Virtual VMLOAD VMSAVE supported > [ 10.177052] kvm_amd: Virtual GIF supported > ... > ... > [ 10.201648] kvm_amd: in svm_enable_virtualization_cpu WRMSR VM_HSAVE_PA non-zero > > And then svm_x86_ops->enable_virtualization_cpu() (svm_enable_virtualization_cpu) programs MSR_VM_HSAVE_PA as following: > wrmsrl(MSR_VM_HSAVE_PA, sd->save_area_pa); > > So VM_HSAVE_PA is non-zero before SNP support is enabled on all CPUs. > > snp_rmptable_init() gets invoked after svm_enable_virtualization_cpu() as following : > ... > [ 11.256138] kvm_amd: in svm_enable_virtualization_cpu WRMSR VM_HSAVE_PA non-zero > ... > [ 11.264918] SEV-SNP: in snp_rmptable_init > > This triggers a #GP exception in snp_rmptable_init() when snp_enable() is invoked to set SNP_EN in SYSCFG MSR: > > [ 11.294289] unchecked MSR access error: WRMSR to 0xc0010010 (tried to write 0x0000000003fc0000) at rIP: 0xffffffffaf5d5c28 (native_write_msr+0x8/0x30) > ... > [ 11.294404] Call Trace: > [ 11.294482] <IRQ> > [ 11.294513] ? show_stack_regs+0x26/0x30 > [ 11.294522] ? ex_handler_msr+0x10f/0x180 > [ 11.294529] ? search_extable+0x2b/0x40 > [ 11.294538] ? fixup_exception+0x2dd/0x340 > [ 11.294542] ? exc_general_protection+0x14f/0x440 > [ 11.294550] ? asm_exc_general_protection+0x2b/0x30 > [ 11.294557] ? __pfx_snp_enable+0x10/0x10 > [ 11.294567] ? native_write_msr+0x8/0x30 > [ 11.294570] ? __snp_enable+0x5d/0x70 > [ 11.294575] snp_enable+0x19/0x20 > [ 11.294578] __flush_smp_call_function_queue+0x9c/0x3a0 > [ 11.294586] generic_smp_call_function_single_interrupt+0x17/0x20 > [ 11.294589] __sysvec_call_function+0x20/0x90 > [ 11.294596] sysvec_call_function+0x80/0xb0 > [ 11.294601] </IRQ> > [ 11.294603] <TASK> > [ 11.294605] asm_sysvec_call_function+0x1f/0x30 > ... > [ 11.294631] arch_cpu_idle+0xd/0x20 > [ 11.294633] default_idle_call+0x34/0xd0 > [ 11.294636] do_idle+0x1f1/0x230 > [ 11.294643] ? complete+0x71/0x80 > [ 11.294649] cpu_startup_entry+0x30/0x40 > [ 11.294652] start_secondary+0x12d/0x160 > [ 11.294655] common_startup_64+0x13e/0x141 > [ 11.294662] </TASK> > > This #GP exception is getting triggered due to the following errata for AMD family 19h Models 10h-1Fh Processors: > > Processor may generate spurious #GP(0) Exception on WRMSR instruction: > Description: > The Processor will generate a spurious #GP(0) Exception on a WRMSR instruction if the following conditions are all met: > - the target of the WRMSR is a SYSCFG register. > - the write changes the value of SYSCFG.SNPEn from 0 to 1. > - One of the threads that share the physical core has a non-zero value in the VM_HSAVE_PA MSR. > > The suggested workaround is when enabling SNP, program VM_HSAVE_PA to 0h on both threads that share a physical core before setting SYSCFG.SNPEn > > The document being referred to above: > https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/revision-guides/57095-PUB_1_01.pdf > > Therefore, with kvm_amd module being built-in, KVM/SVM initialization happens before Host SNP is enabled and this SVM initialization > sets VM_HSAVE_PA to non-zero, which then triggers this #GP when SYSCFG.SNPEn is being set and this will subsequently cause SNP_INIT(_EX) to fail > with INVALID_CONFIG error as SYSCFG[SnpEn] is not set on all CPUs. > > So it looks like the current SNP host enabling code and effectively SNP is broken with respect to the KVM module being built-in. > > Essentially SNP host enabling code should be invoked before KVM initialization, which is currently not the case when KVM is built-in. > > Additionally, the PSP driver probably needs to be initialized at device_initcall level if it is built-in, but that is much later than KVM > module initialization, therefore, that is blocker for moving SEV/SNP initialization to KVM module load time instead of PSP module probe time. > Do note that i have verified and tested that PSP module initialization works when invoked as a device_initcall(). > As a follow-up to the above issues, i have an important question: Do we really need kvm_amd module to be built-in for SEV/SNP support ? Is there any usage case/scenario where the kvm_amd module needs to be built-in for SEV/SNP support ? If we can have a requirement that kvm_amd will always be loaded as a module (for SEV/SNP usage case), then it automatically fixes the above two issues & additionally we can continue on this approach to move SEV/SNP initialization stuff to KVM from the PSP driver. Tom and i had a discussion about it and we realized as so far no one has reported this issue of SNP support being broken with respect to kvm_amd module being built-in (from the time SNP support has gone upstream), it looks like no one is currently using kvm_amd module being built-in for SNP ? Looking for feedback/comments on the above. Thanks, Ashish
On Tue, Jan 14, 2025, Ashish Kalra wrote: > On 1/13/2025 9:03 AM, Kalra, Ashish wrote: > > SNP host support is enabled in snp_rmptable_init() in > > arch/x86/virt/svm/sev.c, which is invoked as a device_initcall(). Here > > device_initcall() is used as snp_rmptable_init() expects AMD IOMMU SNP > > support to be enabled prior to it and the AMD IOMMU driver is initialized > > after PCI bus enumeration. Ugh. So. Many. Dependencies. That's a kernel bug, full stop. RMP initialization very obviously is not device initialization. Why isn't snp_rmptable_init() called from mem_encrypt_init()? AFAICT, arch_cpu_finalize_init() is called after IOMMU initialziation. And if that doesn't work, hack it into arch_post_acpi_subsys_init(). Using device_initcall() to initialization the RMP is insane, IMO. > > Additionally, the PSP driver probably needs to be initialized at > > device_initcall level if it is built-in, but that is much later than KVM > > module initialization, therefore, that is blocker for moving SEV/SNP > > initialization to KVM module load time instead of PSP module probe time. > > Do note that i have verified and tested that PSP module initialization > > works when invoked as a device_initcall(). > > As a follow-up to the above issues, i have an important question: > > Do we really need kvm_amd module to be built-in for SEV/SNP support ? Yes. > Is there any usage case/scenario where the kvm_amd module needs to be > built-in for SEV/SNP support ? Don't care. I am 100% against setting a precedent of tying features to KVM being a module or not, especially since this is a solvable problem. Ideally, the initcall infrastructure would let modules express dependencies, but I can appreciate that solving this generically would require a high amount of complexity. Having KVM explicitly call into the PSP driver as needed isn't difficult, just gross. But for me, it's still far better giving up and requiring everything to be modules. E.g. diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 943bd074a5d3..a2ee12e998f0 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -2972,6 +2972,16 @@ void __init sev_hardware_setup(void) WARN_ON_ONCE(!boot_cpu_has(X86_FEATURE_FLUSHBYASID))) goto out; + /* + * The kernel's initcall infrastructure lacks the ability to express + * dependencies between initcalls, where as the modules infrastructure + * automatically handles dependencies via symbol loading. Ensure the + * PSP SEV driver is initialized before proceeding if KVM is built-in, + * as the dependency isn't handled by the initcall infrastructure. + */ + if (IS_BUILTIN(CONFIG_KVM_AMD) && sev_module_init()) + goto out; + /* Retrieve SEV CPUID information */ cpuid(0x8000001f, &eax, &ebx, &ecx, &edx); diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c index 7eb3e4668286..a0cdc03984cb 100644 --- a/drivers/crypto/ccp/sp-dev.c +++ b/drivers/crypto/ccp/sp-dev.c @@ -253,8 +253,12 @@ struct sp_device *sp_get_psp_master_device(void) static int __init sp_mod_init(void) { #ifdef CONFIG_X86 + static bool initialized; int ret; + if (initialized) + return 0; + ret = sp_pci_init(); if (ret) return ret; @@ -263,6 +267,7 @@ static int __init sp_mod_init(void) psp_pci_init(); #endif + initialized = true; return 0; #endif @@ -279,6 +284,13 @@ static int __init sp_mod_init(void) return -ENODEV; } +#if IS_BUILTIN(CONFIG_KVM_AMD) && IS_ENABLED(CONFIG_KVM_AMD_SEV) +int __init sev_module_init(void) +{ + return sp_mod_init(); +} +#endif + static void __exit sp_mod_exit(void) { #ifdef CONFIG_X86 diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h index 903ddfea8585..0138d22b46ac 100644 --- a/include/linux/psp-sev.h +++ b/include/linux/psp-sev.h @@ -814,6 +814,8 @@ struct sev_data_snp_commit { #ifdef CONFIG_CRYPTO_DEV_SP_PSP +int __init sev_module_init(void); + /** * sev_platform_init - perform SEV INIT command *
Hello Sean, On 1/14/2025 4:31 PM, Sean Christopherson wrote: > On Tue, Jan 14, 2025, Ashish Kalra wrote: >> On 1/13/2025 9:03 AM, Kalra, Ashish wrote: >>> SNP host support is enabled in snp_rmptable_init() in >>> arch/x86/virt/svm/sev.c, which is invoked as a device_initcall(). Here >>> device_initcall() is used as snp_rmptable_init() expects AMD IOMMU SNP >>> support to be enabled prior to it and the AMD IOMMU driver is initialized >>> after PCI bus enumeration. > > Ugh. So. Many. Dependencies. > > That's a kernel bug, full stop. RMP initialization very obviously is not device > initialization. I agree. > > Why isn't snp_rmptable_init() called from mem_encrypt_init()? AFAICT, > arch_cpu_finalize_init() is called after IOMMU initialziation. And if that > doesn't work, hack it into arch_post_acpi_subsys_init(). Using device_initcall() > to initialization the RMP is insane, IMO. Currently SNP support on IOMMU is enabled via the following code path: rootfs_initcall(pci_iommu_init) -> pci_iommu_init() -> amd_iommu_init() -> iommu_snp_enable() And, smp_rmptable_init() needs to be executed after iommu_snp_enable() and that's why we can't call snp_rmptable_init() as early as mem_encrypt_init() or post arch ACPI callbacks, etc. But, there is a patch from the AMD IOMMU team, which calls iommu_snp_enable() early after early_amd_iommu_init() is executed and this will happen during AMD IOMMU driver initialization with the following code path: apic_intr_mode_init() -> enable_IR_x2apic() -> irq_remapping_enable() -> amd_iommu_enable() -> iommu_snp_enable() This AMD IOMMU driver patch moves SNP enable check before enabling IOMMUs as certain IOMMU buffer sizes may change depending on SNP support being enabled. With this AMD IOMMU driver patch applied, we can now call snp_rmptable_init() early with a subsys_initcall(). That fixes the issue with SNP host enabling code being called later than KVM initialization with kvm_amd module built-in. I will post a fix for the SNP host support broken with kvm_amd module built-in with this AMD IOMMU driver patch to call iommu_snp_enable() early and the subsys_initcall() change for snp_rmptable_init() fix on top of it. > >>> Additionally, the PSP driver probably needs to be initialized at >>> device_initcall level if it is built-in, but that is much later than KVM >>> module initialization, therefore, that is blocker for moving SEV/SNP >>> initialization to KVM module load time instead of PSP module probe time. >>> Do note that i have verified and tested that PSP module initialization >>> works when invoked as a device_initcall(). >> >> As a follow-up to the above issues, i have an important question: >> >> Do we really need kvm_amd module to be built-in for SEV/SNP support ? > > Yes. > >> Is there any usage case/scenario where the kvm_amd module needs to be >> built-in for SEV/SNP support ? > > Don't care. I am 100% against setting a precedent of tying features to KVM > being a module or not, especially since this is a solvable problem. > > Ideally, the initcall infrastructure would let modules express dependencies, but > I can appreciate that solving this generically would require a high amount of > complexity. > > Having KVM explicitly call into the PSP driver as needed isn't difficult, just > gross. But for me, it's still far better giving up and requiring everything to > be modules. > > E.g. > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 943bd074a5d3..a2ee12e998f0 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -2972,6 +2972,16 @@ void __init sev_hardware_setup(void) > WARN_ON_ONCE(!boot_cpu_has(X86_FEATURE_FLUSHBYASID))) > goto out; > > + /* > + * The kernel's initcall infrastructure lacks the ability to express > + * dependencies between initcalls, where as the modules infrastructure > + * automatically handles dependencies via symbol loading. Ensure the > + * PSP SEV driver is initialized before proceeding if KVM is built-in, > + * as the dependency isn't handled by the initcall infrastructure. > + */ > + if (IS_BUILTIN(CONFIG_KVM_AMD) && sev_module_init()) > + goto out; > + > /* Retrieve SEV CPUID information */ > cpuid(0x8000001f, &eax, &ebx, &ecx, &edx); > > diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c > index 7eb3e4668286..a0cdc03984cb 100644 > --- a/drivers/crypto/ccp/sp-dev.c > +++ b/drivers/crypto/ccp/sp-dev.c > @@ -253,8 +253,12 @@ struct sp_device *sp_get_psp_master_device(void) > static int __init sp_mod_init(void) > { > #ifdef CONFIG_X86 > + static bool initialized; > int ret; > > + if (initialized) > + return 0; > + > ret = sp_pci_init(); > if (ret) > return ret; > @@ -263,6 +267,7 @@ static int __init sp_mod_init(void) > psp_pci_init(); > #endif > > + initialized = true; > return 0; > #endif > > @@ -279,6 +284,13 @@ static int __init sp_mod_init(void) > return -ENODEV; > } > > +#if IS_BUILTIN(CONFIG_KVM_AMD) && IS_ENABLED(CONFIG_KVM_AMD_SEV) > +int __init sev_module_init(void) > +{ > + return sp_mod_init(); > +} > +#endif > + > static void __exit sp_mod_exit(void) > { > #ifdef CONFIG_X86 > diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h > index 903ddfea8585..0138d22b46ac 100644 > --- a/include/linux/psp-sev.h > +++ b/include/linux/psp-sev.h > @@ -814,6 +814,8 @@ struct sev_data_snp_commit { > > #ifdef CONFIG_CRYPTO_DEV_SP_PSP > > +int __init sev_module_init(void); > + > /** > * sev_platform_init - perform SEV INIT command > * I have tested your patch for KVM explicitly calling into the PSP driver and this works well, with this patch applied as expected PSP driver initialization completes before KVM initialization. So we can continue with the approach to move SEV/SNP initialization stuff to KVM. I will add your patch to the v4 of these series i am going to post next. Thanks, Ashish
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)