Message-ID: <a0feef4a-1e12-445c-8a17-0f2ecc4d7c85@amd.com>
Date: Tue, 4 Mar 2025 19:58:28 -0600
From: "Kalra, Ashish" <ashish.kalra@....com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: pbonzini@...hat.com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
 thomas.lendacky@....com, john.allen@....com, herbert@...dor.apana.org.au,
 michael.roth@....com, dionnaglaze@...gle.com, nikunj@....com,
 ardb@...nel.org, kevinloughlin@...gle.com, Neeraj.Upadhyay@....com,
 aik@....com, kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-crypto@...r.kernel.org, linux-coco@...ts.linux.dev
Subject: Re: [PATCH v5 6/7] KVM: SVM: Add support to initialize SEV/SNP
 functionality in KVM


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

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

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

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

Thanks,
Ashish

Powered by blists - more mailing lists