On 3/4/2025 7:58 PM, Kalra, Ashish wrote: > > 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(). > Also looking at the patch commit logs for psp_init_on_probe parameter: https://lore.kernel.org/lkml/20211115174102.2211126-5-pgonda@xxxxxxxxxx/ User may decouple module init from PSP init due to use of the INIT_EX support in upcoming patch which allows for users to save PSP's internal state to file. The file may be unavailable at module init. So it probably makes sense to keep SEV deferred initialization support there, as it may not only be filesystem unavailability at CCP module load (or KVM module load with new flow), but user may have the file available only later after module load/init. Thanks, Ashish
