Message-ID: <5b77d19d-3f34-46d7-b307-738643504cd5@amd.com>
Date: Fri, 6 Dec 2024 23:21:06 -0600
From: "Kalra, Ashish" <ashish.kalra@....com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Tom Lendacky <thomas.lendacky@....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

Hello Sean, 

Please see my responses below:

On 12/6/2024 4:30 PM, Sean Christopherson wrote:
> 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.

Here is a summary:

1). SNP_INIT(_EX) cannot be done if SEV INIT has been done, the above comment mentions the same,  
in other words, if SEV INIT has completed successfully then any subsequent SNP_INIT(_EX) will fail.

2). Also if SNP is enabled (SNPEn) system-wide, which is done during early kernel boot, then 
SNP_INIT_EX has to be done before SEV INIT, otherwise SEV INIT will fail. 

SNP is enabled system-wide if SNP support and RMP table support is enabled in BIOS and 
additionally RMP table sanity checks are successful, RMP table is mapped and SNP support is
enabled on the IOMMU.

This also means that if we only want to launch legacy VMs (SEV/SEV-ES), but SNP has been enabled
at early kernel boot then we still need to do SNP_INIT(_EX) before SEV INIT. 

Considering all these, SNP_INIT(_EX) is always attempted before SEV INIT as above.

If SNP is not enabled system-wide (CC_ATTR_HOST_SEV_SNP is not set), then SNP_INIT(_EX) will
be skipped.

If we specify KVM module parameter sev_snp=false, but if SNPEn is set system-wide, we
will still do SNP_INIT(_EX) before SEV INIT to ensure that SEV INIT is successful. 
But do note, in this configuration launching any SNP VMs will fail.

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

Yes, that is true. As per SNP Firmware API specs, SNP DOWNLOAD_FIRMWARE_EX adds support 
for provisional updates and for updates while SNP firmware is in the INIT state.

The SNP firmware may be in any state. SEV must be in the UNINIT state.

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

Yes, the above summary is correct except for (3).

The initial set of patches will initialize SNP and SEV both at kvm-amd.ko module load,
similar to PSP module load/probe time.

For backward compatibility, the PSP module parameter psp_init_on_probe will still be
supported, i believe it is used for INIT_EX support.

I have the base set of patches ready which remove SEV/SNP platform initialization from
PSP module probe time and instead move it to KVM module load time (sev_hardware_setup)
and similarly for platform shutdown.

I will post this patch-set early next week.

On top of this base patch-set we will push the SNP CipherTextHiding support and SEV
firmware hotloading support. The SEV firmware hotloading support will add support
for (3) above.

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

Yes, this should work, for supporting firmware hotloading, the PSP driver's
psp_init_on_probe parameter will need to be set to false, which will ensure
that SEV INIT is not done during SEV/SNP platform initialization at KVM module
probe time and instead it will be done implicitly at first SEV/SEV-ES VM launch.

Hopefully, this summarizes all the possible configurations for SEV/SNP platform
initialization.

Thanks,
Ashish

Powered by blists - more mailing lists