Message-ID: <Zz9w67Ajxb-KQFZZ@google.com>
Date: Thu, 21 Nov 2024 09:42:03 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Tom Lendacky <thomas.lendacky@....com>
Cc: Ashish Kalra <ashish.kalra@....com>, Peter Gonda <pgonda@...gle.com>, pbonzini@...hat.com, 
	tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, 
	dave.hansen@...ux.intel.com, hpa@...or.com, herbert@...dor.apana.org.au, 
	x86@...nel.org, john.allen@....com, davem@...emloft.net, michael.roth@....com, 
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-crypto@...r.kernel.org
Subject: Re: [PATCH v2 3/3] x86/sev: Add SEV-SNP CipherTextHiding support

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.

Powered by blists - more mailing lists