Message-ID: <f02fee7d-27e8-4ddc-b349-6d0f8c7919fa@amd.com>
Date: Tue, 14 Jan 2025 15:14:31 -0600
From: "Kalra, Ashish" <ashish.kalra@....com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Tom Lendacky <thomas.lendacky@....com>, pbonzini@...hat.com,
 tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
 john.allen@....com, herbert@...dor.apana.org.au, davem@...emloft.net,
 michael.roth@....com, dionnaglaze@...gle.com, kvm@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
 linux-coco@...ts.linux.dev
Subject: Re: [PATCH v3 6/7] KVM: SVM: Add support to initialize SEV/SNP
 functionality in KVM


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

Powered by blists - more mailing lists