Re: [PATCH v2 3/3] x86/sev: Add SEV-SNP CipherTextHiding support

[Sean Christopherson <seanjc@google.com>]

On Thu, Nov 21, 2024, Ashish Kalra wrote:
> 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 ?

So the above says:

 SEV_CMD_SNP_INIT{_ES} cannot be executed if SEV_CMD_INIT{_EX} has been executed.

but the existing comment in _sev_platform_init_locked() says:

	/*
	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
	 * so perform SEV-SNP initialization at probe time.
	 */

Which one is correct?  I don't think it matters in the end, just trying to wrap my
head around everything.

And IIUC, SEV_CMD_SNP_INIT{_EX} can be executed before firmware hotload, but
SEV_CMD_INIT{_EX} cannot.  Is that correct?  Because if firmware hotload can't
be done while SEV VMs are _active_, then that's a very different situation.

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

I don't love the implicit behavior, but assuming hotloading firmware can't be done
after SEV_CMD_INIT{_EX}, that does seem like the least awful solution.

To summarize, if the above assumptions hold:

 1. Initialize SNP when kvm-amd.ko is loaded.
 2. Define CipherTextHiding and ASID params kvm-amd.ko.
 3. Initialize SEV+ at first use.

Just to triple check: that will allow firmware hotloading even if kvm-amd.ko is
built-in, correct?  I.e. doesn't requires deferring kvm-amd.ko load until after
firmware hotloading.