Message-ID: <7ea2b3e8-56b7-418f-8551-b905bf10fecb@amd.com>
Date: Thu, 21 Nov 2024 15:00:04 -0600
From: "Kalra, Ashish" <ashish.kalra@....com>
To: Sean Christopherson <seanjc@...gle.com>,
 Tom Lendacky <thomas.lendacky@....com>
Cc: 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 11/21/2024 11:42 AM, Sean Christopherson wrote:
> 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.

Ok, so kvm module load will init SEV+ if indicated by it's module parameters.

But, there are additional concerns here. 

SNP will still have to be initialized first, because SNP_INIT will fail if SEV INIT has been done.

Additionally, to support SEV firmware hotloading (DLFW_EX), SEV can't be initialized. 

So probably, we will have to retain some PSP style SEV+ initialization here, SNP_INIT is always
done first and then SEV INIT is skipped if explicitly specified by a module param. This allows
SEV firmware hotloading to be supported.

But, then with SEV firmware hotload support how do we do SEV INIT without unloading and reloading KVM module ?

This can reuse the current support (in KVM) to do SEV INIT implicitly when the first SEV VM is run:
sev_guest_init() -> sev_platform_init() 

> 
>>> 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.
>>>

Yes, i do think this can be an issue as those ioctl's may be in use by userspace tools like
sevtool, etc., and require SEV implicit initialization and we will have to preserve this 
behavior if indicated by a PSP module param.

Thanks,
Ashish

>>> 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