On Thu, Nov 21, 2024, Tom Lendacky wrote: > On 11/21/24 10:56, Sean Christopherson wrote: > > On Thu, Nov 21, 2024, Ashish Kalra wrote: > > Actually, IMO, the behavior of _sev_platform_init_locked() and pretty much all of > > the APIs that invoke it are flawed, and make all of this way more confusing and > > convoluted than it needs to be. > > > > IIUC, SNP initialization is forced during probe purely because SNP can't be > > initialized if VMs are running. But the only in-tree user of SEV-XXX functionality > > is KVM, and KVM depends on whatever this driver is called. So forcing SNP > > initialization because a hypervisor could be running legacy VMs make no sense. > > Just require KVM to initialize SEV functionality if KVM wants to use SEV+. > > When we say legacy VMs, that also means non-SEV VMs. So you can't have any > VM running within a VMRUN instruction. Yeah, I know. But if KVM initializes the PSP SEV stuff when KVM is loaded, then KVM can't possibly be running VMs of any kind. > Or... > > > > > /* > > * Legacy guests cannot be running while SNP_INIT(_EX) is executing, > > * so perform SEV-SNP initialization at probe time. > > */ > > rc = __sev_snp_init_locked(&args->error); > > > > Rather than automatically init SEV+ functionality, can we instead do something > > like the (half-baked pseudo-patch) below? I.e. delete all paths that implicitly > > init the PSP, and force KVM to explicitly initialize the PSP if KVM wants to use > > SEV+. Then we can put the CipherText and SNP ASID params in KVM. > > ... do you mean at module load time (based on the module parameters)? Or > when the first SEV VM is run? I would think the latter, as the parameters > are all true by default. If the latter, that would present a problem of > having to ensure no VMs are active while performing the SNP_INIT. kvm-amd.ko load time. > > That would also allow (a) registering the SNP panic notifier if and only if SNP > > is actually initailized and (b) shutting down SEV+ in the PSP when KVM is unloaded. > > Arguably, the PSP should be shutdown when KVM is unloaded, irrespective of the > > CipherText and SNP ASID knobs. But with those knobs, it becomes even more desirable, > > because it would allow userspace to reload *KVM* in order to change the CipherText > > and SNP ASID module params. I.e. doesn't require unloading the entire CCP driver. > > > > If dropping the implicit initialization in some of the ioctls would break existing > > userspace, then maybe we could add a module param (or Kconfig?) to preserve that > > behavior? I'm not familiar with what actually uses /dev/sev. > > > > Side topic #1, sev_pci_init() is buggy. It should destroy SEV if getting the > > API version fails after a firmware update. > > True, we'll look at doing a fix for that. > > > > > Side topic #2, the version check is broken, as it returns "success" when > > initialization quite obviously failed. > > That is ok because you can still initialize SEV / SEV-ES support. Right, but as I've complained elsewhere, KVM shouldn't think SNP is supported when in reality firmware is effectively too old.
